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.

----------
usr/src/uts/intel/brand/common/brand_asm.h

- could you create this file by doing an hg cp of sn1_brand_asm.h or
   s10_brand_asm.h?  (it preserves the history and also makes it easy
   to see changes to the defines.)

Done.

- this doesn't seem right:
        #ifndef _SYS_BRAND_ASM_H
   the SYS_ prefix is normally used by header files in /sys/.
   for this file you should probably have:
        #ifndef _BRAND_ASM_H
        #ifndef _COMMON_BRAND_ASM_H

Changed.

- this file should probably #include the files which define macros
   that it uses.  for example, you should probably include:
        #include<sys/asm_linkage.h>
   i'm sure there are others as well.

Changed.

----------
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.


----------
usr/src/uts/intel/brand/lx/lx_brand_asm.s

- in lx_brand_int80_callback() you have:
        * See the comment on CALC_TABLE_ADDR in brand_asm.h for an
   but this function never uses CALC_TABLE_ADDR.  it still does the
   offset calculation manually:
        shlq    $4, %rax

   i think you could clean this up by changing CHECK_FOR_HANDLER and
   CALC_TABLE_ADDR in brand_asm.h to be de
        CHECK_FOR_HANDLER(scr, handler)
        ...
        cmp     $0, handler(scr);           /* check handler */
   and:
        CALC_TABLE_ADDR(scr, handler)
        ...
        mov     handler(scr), scr;  /* get p_brand_data->handler */ \

   then you also don't need this at the top of every brand:
        #define USP_HANDLER ...

   and you can use CALC_TABLE_ADDR for jump table calculations in the lx
   brand.

This is a good idea, I rewrote things along these lines.

There is a new webrev at:

http://cr.opensolaris.org/~gjelinek/webrev.6887823/

Thanks,
Jerry

_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to