On Mon, Jan 18, 2010 at 12:51:27PM -0700, Jerry Jelinek wrote:
> 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:
> >>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
> >>of moving the common code.
> >hey jerry,
> >looks good. a few comments below.
> >- 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
> 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...
> >- 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.
zones-discuss mailing list