On Tue, Dec 15, 2009 at 01:28:01PM -0700, Jerry Jelinek wrote: > Ed, > > Thanks for reviewing this. My responses to your comments are > in-line. > > Edward Pilatowicz wrote: > >On Tue, Dec 15, 2009 at 08:39:12AM -0700, Jerry Jelinek wrote: > >>I have an initial code review for the fix for bug: > >> > >>6768950 panic[cpu1]/thread=ffffff084ce0b3e0: syscall_asm_amd64.s:480 > >> lwp ffffff0756a8cdc0, pcb_rupdate != 0 > >> > >>There is a webrev at: > >> > >>http://cr.opensolaris.org/~gjelinek/webrev.6768950/ > >> > >>The code changes in the sn1 and solaris10 brands are basically > >>identical. I know there is a lot of common code there but I > >>didn't want to clutter up this bug fix with the unrelated changes > >>necessary to make the code common. I'll be addressing that with > >>a separate fix. > >> > >>My initial testing of these changes looks good but I still need > >>to run more extensive tests. > >> ... > >- prior to these changes V_URET_ADDR wasn't always set, so the different > > brand syscall callbacks would get the userland return address from > > their syscall specific locations (registers, interrupt stack, etc). but > > now since V_URET_ADDR is always set, perhaps the callback handlers could > > be made more consistent by always getting the value from the stack (ie, > > via V_URET_ADDR)? > > > >- so following up with the last comment (and getting more into potential > > comminization work) it seems to me like it might be benificial to move > > all the syscall mechanism specific handling code out of the actual brand > > callbacks and into BRAND_CALLBACK. you've already started doing this by > > having BRAND_CALLBACK be aware of how to get the userland return > > address. (prior to that it didn't have any dependancy upon the > > different syscall mechanisms, except when deciding which brand callback > > to invoke.) continuing down that path we could move all the syscall > > specific handling code into BRAND_CALLBACK. then each brand would only > > deliver a single callback which would take one parameter, the syscall > > number. it would return one value, a userland return address. then > > BRAND_CALLBACK could handle all the different syscall specific return > > paths. this would also be benificial in the future since if a new > > syscall mechanism was introduced, we wouldn't have to update any actual > > brand callbacks, just BRAND_CALLBACK. thoughts? > > For these last two I agree that there are some good opportunities here and > I was torn between doing a bunch more clean up on this and deferring that > work to the fix for: > > 6900207 code can be shared between solaris10 and ipkg brands > > Since bug 6768950 is serious and I'd like to get the fix done sooner > rather than later, I'd like to defer some of these other changes to 6900207. > I was about to start on that anyway so once 6768950 is done I'm going to > immediately start work on a bunch of ideas I have for making the code shared > and simpler. I was also going to roll a fix for: > > 6887823 brandz on x86 should ignore %gs on 64-bit kernels > > into that same set of cleanup. I definitely agree with your comments here > but I'm worried about the fix for 6768950 taking too long. >
sounds good. ed _______________________________________________ zones-discuss mailing list zones-discuss@opensolaris.org
