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

Reply via email to