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? ---------- 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.) - 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 - 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. ---------- 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. ---------- 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. _______________________________________________ zones-discuss mailing list zones-discuss@opensolaris.org