On Mon, Jan 18, 2010 at 12:51:27PM -0700, Jerry Jelinek wrote: > Ed, > > Thanks for reviewing this, my responses are inline. > > On 01/15/10 19:26, Edward Pilatowicz wrote: > >On Thu, Jan 14, 2010 at 09:18:20AM -0700, Jerry Jelinek wrote: > >>I need a code review for my proposed fix for: > >> > >>6887823 brandz on x86 should ignore %gs and simplify brand hooks > >> > >>There is a webrev at: > >> > >>http://cr.opensolaris.org/~gjelinek/webrev.6887823/ > >> > >>This simplifies some of the handling for the %gs register, cleans up > >>the interfaces with the brand modules, and consolidates common code > >>into a single file. Although the webrev looks large, most of this is > >>because > >>of moving the common code. > >> > > > >hey jerry, > >looks good. a few comments below. > >ed > > > >---------- > >usr/src/uts/i86pc/ml/syscall_asm.s > > > >- so i think the following comment is true for lcall and int syscalls on > > 32-bit kernels: > > > > * -------------------------------------- > > * | user's %ss | > > * | | user's %esp | > > * | | EFLAGS register | > > * | | user's %cs | > > * | | user's %eip (user return address) | > > * | | 'scratch space' | > > > > but what about sysenter syscalls? none of that stuff will be on the > > stack. (that's why BRAND_CALLBACK used to explicitly push the user > > return address onto the stack.) have you tested with sysenter > > syscalls on 32-bit kernels? > > I haven't really changed this. If you look at the original code in > syscall_asm.s, > all it was doing was re-pushing a value that was already on the stack. This > is line > 152 in the original code in syscall_asm.s. The reason this works is that the > sysenter callback doesn't use the data from the stack since the return address > is in the %edx register (see the comment on the callback in > s10_brand_asm.s or sn1_brand_asm.s). The SYSCALL_EMUL macro > expects the return address to be in a register, not on the stack. >
ok. i got it this time around. man, this stuff is always confusing... > >---------- > >usr/src/uts/intel/brand/*/*_brand_asm.s > > > >- you've removed all the comments that explain how the stack frame looks > > and what the assumptions are when we enter the different handlers. i > > realize this is all documented in brand_asm.h now, but now folks don't > > know how to map each of the handlers to their running conditions. it'd > > be nice if there was a comment for each handler pointing people to that > > the correct comments that apply to each handler. > > > > perhaps it would make sense to add some "tokens" to the comments in > > brand_asm.h like: > > 32-BIT INTERPOSITION STACK > > 32-BIT LCALL/INT STACK > > 64-BIT INTERPOSITION STACK > > 64-BIT LCALL/INT STACK > > > > and then in the comments for each brand callback you could refer the > > reader to the appropriate tokens in brand_asm.h. > > I added this. > could you add references to these tokens to lx_brand_asm.s as well? thanks for cleaning all this stuff up. ed _______________________________________________ zones-discuss mailing list zones-discuss@opensolaris.org