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 because
of moving the common code.
looks good. a few comments below.
- so i think the following comment is true for lcall and int syscalls on
* | 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
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.
- 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:
the SYS_ prefix is normally used by header files in /sys/.
for this file you should probably have:
- this file should probably #include the files which define macros
that it uses. for example, you should probably include:
i'm sure there are others as well.
- 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
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.
- 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
cmp $0, handler(scr); /* check 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
This is a good idea, I rewrote things along these lines.
There is a new webrev at:
zones-discuss mailing list