Re: [zones-discuss] zones code review
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
Re: [zones-discuss] zones code review
On 01/19/10 14:00, Edward Pilatowicz wrote: 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? Ed, Will do, sorry for not adding this yesterday. Do you want to re-review it again after that comment change? Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Tue, Jan 19, 2010 at 02:33:15PM -0700, Jerry Jelinek wrote: On 01/19/10 14:00, Edward Pilatowicz wrote: 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? Ed, Will do, sorry for not adding this yesterday. Do you want to re-review it again after that comment change? not really. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On 01/14/10 08:18 AM, 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. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org Hi Jerry, This looks fine to me. Jordan ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Edward Pilatowicz wrote: so we're actually changing our stack pointer after entry into the kernel, so it's no longer necessarily matching the interrupt stack that the processor switched in automatically and saved the parameters on. notably we don't do this for 32-bit kernels. this means that de-referencing V_SSP is the right things todo. sorry for taking so long to understand this code... so one last comment nit and then i promise i'm done. could you update the the descriptions of the stack setup by BRAND_CALLBACK on 64-bit kernels to be more accurate for interrupt syscalls by changing: user stack pointer to: user (or interrupt) stack pointer thanks, and sorry again for the delays while i tried to understand the code better. Ed, Thanks for all of your comments. I'll make the update you suggested. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Edward Pilatowicz wrote: so now you have: ---8--- #define V_U_EIP (CLONGSIZE * 0) ... GET_V(%rsp, 1, V_SSP, %rax) /* get saved stack pointer */ SET_V(%rax, 0, V_U_EIP, %r15) /* save new return addr in %eip */ ---8--- but why can't this be identical to the 32-bit path? afaik, it seems like you could just do: ---8--- #define V_U_EIP (V_END + (CLONGSIZE * 0)) ... SET_V(%rsp, 1, V_U_EIP, %r15) /* save new return addr in %eip */ ---8--- why load V_SSP if we already know that the interrupt state is right on the stack above the callback arguments? (it seems we sholud just access the state directly without first loading V_SSP.) Ed, Because its not right above, all of the other register values are also pushed on the stack, so we need to go through the SSP to get to the right spot. I can add a comment explaining this but the 32bit and 64bit stacks are not identical. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Jerry Jelinek wrote: Because its not right above, all of the other register values are also pushed on the stack, so we need to go through the SSP to get to the right spot. I can add a comment explaining this but the 32bit and 64bit stacks are not identical. Ed, Actually, what I said above is not quite right. I think that its not the other registers but the alignment that is making the stacks different. I took another look at the AMD64 Architecture Programmers Manual, Volume 2: System Programming manual. This is discussed in section 8.9 Long-Mode Interrupt Control Transfers. You can see how the stack is different vs. the discussion in section 8.7. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Fri, Dec 18, 2009 at 08:19:02AM -0700, Jerry Jelinek wrote: Jerry Jelinek wrote: Because its not right above, all of the other register values are also pushed on the stack, so we need to go through the SSP to get to the right spot. I can add a comment explaining this but the 32bit and 64bit stacks are not identical. Ed, Actually, what I said above is not quite right. I think that its not the other registers but the alignment that is making the stacks different. I took another look at the AMD64 Architecture Programmers Manual, Volume 2: System Programming manual. This is discussed in section 8.9 Long-Mode Interrupt Control Transfers. You can see how the stack is different vs. the discussion in section 8.7. all the comments in our source code would seem to indicate that the stacks are the same. also, looking at the stack diagram in the v2 manual they seem to be the same to me (aisde from the obvious 32 vs 64 bit word size differences): Figure 8-9 Stack After Interrupt to Higher Privilege p280 (p320) Figure 8-14 Long-Mode Stack After Interrupt-Higher Privilege p292 (p332) also, right above figure 8-14 there is a discussion of the differences between the long mode switch vs the classic legacy switch, with the only difference being in how the SS is handled. i guess this boils down to is, is there a difference between the value of V_SSP and address of V_END, and if so why? i set some breakpoints in kmdb and compared these values and they are indeed different. looking at brand callback i can now see why, we do the following: ---8--- #define BRAND_CALLBACK(callback_id) \ movq%rsp, %gs:CPU_RTMP_RSP /* save the stack pointer */ ;\ movq%r15, %gs:CPU_RTMP_R15 /* save %r15*/ ;\ movq%gs:CPU_THREAD, %r15/* load the thread pointer */ ;\ movqT_STACK(%r15), %rsp /* switch to the kernel stack */ ;\ ---8--- so we're actually changing our stack pointer after entry into the kernel, so it's no longer necessarily matching the interrupt stack that the processor switched in automatically and saved the parameters on. notably we don't do this for 32-bit kernels. this means that de-referencing V_SSP is the right things todo. sorry for taking so long to understand this code... so one last comment nit and then i promise i'm done. could you update the the descriptions of the stack setup by BRAND_CALLBACK on 64-bit kernels to be more accurate for interrupt syscalls by changing: user stack pointer to: user (or interrupt) stack pointer thanks, and sorry again for the delays while i tried to understand the code better. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Edward Pilatowicz wrote: - CALC_TABLE_ADDR() a little clunky. (it has seperate 32 and 64 versions, it assumes the syscall number is in eax/rax, and it has a side effect of munging the syscall number.) how about defining just one version of CALC_TABLE_ADDR() as: ---8--- #define CALC_TABLE_ADDR(sysnum, rv) \ GET_P_BRAND_DATA(%rsp, 1, rv) /* get p_brand_data ptr */; \ mov SPD_HANDLER(rv), rv /* get p_brand_data-spd_handler ptr */;\ shl $4, sysnum /* syscall_num * 16 */; \ add sysnum, result /* calc JMP table address */; \ shr $4, sysnum /* syscall_num / 16 */ ---8--- Ed, I reworked the macros and I think its a lot cleaner now. Let me know what you think. The new webrev is at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Thu, Dec 17, 2009 at 09:39:34AM -0700, Jerry Jelinek wrote: Edward Pilatowicz wrote: - CALC_TABLE_ADDR() a little clunky. (it has seperate 32 and 64 versions, it assumes the syscall number is in eax/rax, and it has a side effect of munging the syscall number.) how about defining just one version of CALC_TABLE_ADDR() as: ---8--- #define CALC_TABLE_ADDR(sysnum, rv) \ GET_P_BRAND_DATA(%rsp, 1, rv) /* get p_brand_data ptr */; \ mov SPD_HANDLER(rv), rv /* get p_brand_data-spd_handler ptr */;\ shl $4, sysnum /* syscall_num * 16 */; \ add sysnum, result /* calc JMP table address */; \ shr $4, sysnum /* syscall_num / 16 */ ---8--- Ed, I reworked the macros and I think its a lot cleaner now. Let me know what you think. The new webrev is at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ this looks great. things are much simpler and the callbacks are more similar. :) i only have one nit. i think the following comment is incorrect: * To 'return' to our user-space handler, we just need to place its * address into the user's %ss. it should read: * To 'return' to our user-space handler we need to update the * user's %eip pointer in the saved interrupt state. The * interrupt state was pushed onto our stack automatically when * the interrupt occured, see the comments above. actually, now that i look at it some more... i think you could make the int91 callback simpler by making it almost the same as the like the 32-bit syscall callback. after all, the stack content basically is the same in both call paths... specifically, you could change the following: ---8--- /* * The saved stack pointer points at the state saved when we took * the interrupt: ... */ ENTRY(sn1_brand_int91_callback) ... movq%r15, %rax /* save new ret addr in %rax */ GET_V(%rsp, 1, V_SSP, %r15) /* Get saved %esp */ xchgq (%r15), %rax/* swap %rax and return addr */ ---8--- to be: ---8--- /* * The saved stack pointer (V_SSP) points to the interrupt specific * state, which is saved directly above the stack contents common to all * callbacks. ... */ #define V_U_SS (V_END + (CLONGSIZE * 4)) #define V_U_ESP (V_END + (CLONGSIZE * 3)) #define V_EFLAGS(V_END + (CLONGSIZE * 2)) #define V_U_CS (V_END + (CLONGSIZE * 1)) #define V_U_EIP (V_END + (CLONGSIZE * 0)) ENTRY(sn1_brand_int91_callback) ... SET_V(%rsp, 1, V_U_EIP, %r15) /* set user %eip to JMP table addr */ GET_V(%rsp, 1, V_URET_ADDR, %rax) /* save orig return addr in %eax */ ---8--- ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Edward Pilatowicz wrote: On Thu, Dec 17, 2009 at 09:39:34AM -0700, Jerry Jelinek wrote: Edward Pilatowicz wrote: - CALC_TABLE_ADDR() a little clunky. (it has seperate 32 and 64 versions, it assumes the syscall number is in eax/rax, and it has a side effect of munging the syscall number.) how about defining just one version of CALC_TABLE_ADDR() as: ---8--- #define CALC_TABLE_ADDR(sysnum, rv) \ GET_P_BRAND_DATA(%rsp, 1, rv) /* get p_brand_data ptr */; \ mov SPD_HANDLER(rv), rv /* get p_brand_data-spd_handler ptr */;\ shl $4, sysnum /* syscall_num * 16 */; \ add sysnum, result /* calc JMP table address */; \ shr $4, sysnum /* syscall_num / 16 */ ---8--- Ed, I reworked the macros and I think its a lot cleaner now. Let me know what you think. The new webrev is at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ this looks great. things are much simpler and the callbacks are more similar. :) i only have one nit. i think the following comment is incorrect: * To 'return' to our user-space handler, we just need to place its * address into the user's %ss. it should read: * To 'return' to our user-space handler we need to update the * user's %eip pointer in the saved interrupt state. The * interrupt state was pushed onto our stack automatically when * the interrupt occured, see the comments above. actually, now that i look at it some more... i think you could make the int91 callback simpler by making it almost the same as the like the 32-bit syscall callback. after all, the stack content basically is the same in both call paths... specifically, you could change the following: ---8--- /* * The saved stack pointer points at the state saved when we took * the interrupt: ... */ ENTRY(sn1_brand_int91_callback) ... movq%r15, %rax /* save new ret addr in %rax */ GET_V(%rsp, 1, V_SSP, %r15) /* Get saved %esp */ xchgq (%r15), %rax/* swap %rax and return addr */ ---8--- to be: ---8--- /* * The saved stack pointer (V_SSP) points to the interrupt specific * state, which is saved directly above the stack contents common to all * callbacks. ... */ #define V_U_SS (V_END + (CLONGSIZE * 4)) #define V_U_ESP (V_END + (CLONGSIZE * 3)) #define V_EFLAGS(V_END + (CLONGSIZE * 2)) #define V_U_CS (V_END + (CLONGSIZE * 1)) #define V_U_EIP (V_END + (CLONGSIZE * 0)) ENTRY(sn1_brand_int91_callback) ... SET_V(%rsp, 1, V_U_EIP, %r15) /* set user %eip to JMP table addr */ GET_V(%rsp, 1, V_URET_ADDR, %rax) /* save orig return addr in %eax */ ---8--- Ed, Thanks for the correction on the comment. I also updated the code as you suggested. I'm not sure if what I have now is better than before but its the same number of instructions and its more similar to the the 32-bit code path (although it can't be identical). I posted a new webrev at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ Let me know if you have any other comments. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Thu, Dec 17, 2009 at 04:24:22PM -0700, Jerry Jelinek wrote: Edward Pilatowicz wrote: to be: ---8--- /* * The saved stack pointer (V_SSP) points to the interrupt specific * state, which is saved directly above the stack contents common to all * callbacks. ... */ #define V_U_SS (V_END + (CLONGSIZE * 4)) #define V_U_ESP (V_END + (CLONGSIZE * 3)) #define V_EFLAGS(V_END + (CLONGSIZE * 2)) #define V_U_CS (V_END + (CLONGSIZE * 1)) #define V_U_EIP (V_END + (CLONGSIZE * 0)) ENTRY(sn1_brand_int91_callback) ... SET_V(%rsp, 1, V_U_EIP, %r15) /* set user %eip to JMP table addr */ GET_V(%rsp, 1, V_URET_ADDR, %rax) /* save orig return addr in %eax */ ---8--- Ed, Thanks for the correction on the comment. I also updated the code as you suggested. I'm not sure if what I have now is better than before but its the same number of instructions and its more similar to the the 32-bit code path (although it can't be identical). I posted a new webrev at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ Let me know if you have any other comments. so now you have: ---8--- #define V_U_EIP (CLONGSIZE * 0) ... GET_V(%rsp, 1, V_SSP, %rax) /* get saved stack pointer */ SET_V(%rax, 0, V_U_EIP, %r15) /* save new return addr in %eip */ ---8--- but why can't this be identical to the 32-bit path? afaik, it seems like you could just do: ---8--- #define V_U_EIP (V_END + (CLONGSIZE * 0)) ... SET_V(%rsp, 1, V_U_EIP, %r15) /* save new return addr in %eip */ ---8--- why load V_SSP if we already know that the interrupt state is right on the stack above the callback arguments? (it seems we sholud just access the state directly without first loading V_SSP.) ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Jordan Vaughan wrote: -- usr/src/lib/brand/sn1/sn1_brand/amd64/sn1_handler.s 44: Shouldn't this function be named sn1_handler_table? Jordan, Good catch, I'll fix that. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Ed, I've posted an updated webrev to address your comments. http://cr.opensolaris.org/~gjelinek/webrev.6768950/ Let me know if you have any other comments or see anything with the changes I made. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Wed, Dec 16, 2009 at 10:46:57AM -0700, Jerry Jelinek wrote: Ed, I've posted an updated webrev to address your comments. http://cr.opensolaris.org/~gjelinek/webrev.6768950/ usr/src/uts/intel/brand/sn1/sn1_brand_asm.s - i'd think the is 0 = syscall = MAX check would have to be done after CHECK_FOR_NATIVE(). - CALC_TABLE_ADDR() a little clunky. (it has seperate 32 and 64 versions, it assumes the syscall number is in eax/rax, and it has a side effect of munging the syscall number.) how about defining just one version of CALC_TABLE_ADDR() as: ---8--- #define CALC_TABLE_ADDR(sysnum, rv) \ GET_P_BRAND_DATA(%rsp, 1, rv) /* get p_brand_data ptr */; \ mov SPD_HANDLER(rv), rv /* get p_brand_data-spd_handler ptr */;\ shl $4, sysnum /* syscall_num * 16 */; \ add sysnum, result /* calc JMP table address */; \ shr $4, sysnum /* syscall_num / 16 */ ---8--- ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Ed, Edward Pilatowicz wrote: On Wed, Dec 16, 2009 at 10:46:57AM -0700, Jerry Jelinek wrote: Ed, I've posted an updated webrev to address your comments. http://cr.opensolaris.org/~gjelinek/webrev.6768950/ usr/src/uts/intel/brand/sn1/sn1_brand_asm.s - i'd think the is 0 = syscall = MAX check would have to be done after CHECK_FOR_NATIVE(). It is. I added it to the CHECK_FOR_INTERPOSITION macro which is called after the CHECK_FOR_NATIVE in the CALLBACK_PROLOGUE. - CALC_TABLE_ADDR() a little clunky. (it has seperate 32 and 64 versions, it assumes the syscall number is in eax/rax, and it has a side effect of munging the syscall number.) how about defining just one version of CALC_TABLE_ADDR() as: ---8--- #define CALC_TABLE_ADDR(sysnum, rv) \ GET_P_BRAND_DATA(%rsp, 1, rv) /* get p_brand_data ptr */; \ mov SPD_HANDLER(rv), rv /* get p_brand_data-spd_handler ptr */;\ shl $4, sysnum /* syscall_num * 16 */; \ add sysnum, result /* calc JMP table address */; \ shr $4, sysnum /* syscall_num / 16 */ ---8--- Since we don't care about preserving the syscall number that extra instruction has no value. However, I'll take another shot at trying to streamline this a bit. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On Wed, Dec 16, 2009 at 02:37:36PM -0700, Jerry Jelinek wrote: Ed, Edward Pilatowicz wrote: On Wed, Dec 16, 2009 at 10:46:57AM -0700, Jerry Jelinek wrote: Ed, I've posted an updated webrev to address your comments. http://cr.opensolaris.org/~gjelinek/webrev.6768950/ usr/src/uts/intel/brand/sn1/sn1_brand_asm.s - i'd think the is 0 = syscall = MAX check would have to be done after CHECK_FOR_NATIVE(). It is. I added it to the CHECK_FOR_INTERPOSITION macro which is called after the CHECK_FOR_NATIVE in the CALLBACK_PROLOGUE. oops. your right. i was misread this. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
Ed, Thanks for reviewing this. My responses to your comments are in-line. Edward Pilatowicz wrote: On Tue, Dec 15, 2009 at 08:39:12AM -0700, Jerry Jelinek wrote: I have an initial code review for the fix for bug: 6768950 panic[cpu1]/thread=ff084ce0b3e0: syscall_asm_amd64.s:480 lwp ff0756a8cdc0, pcb_rupdate != 0 There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ The code changes in the sn1 and solaris10 brands are basically identical. I know there is a lot of common code there but I didn't want to clutter up this bug fix with the unrelated changes necessary to make the code common. I'll be addressing that with a separate fix. My initial testing of these changes looks good but I still need to run more extensive tests. this looks great. i have some initial comments. -- usr/src/lib/brand/{sn1|solaris10}/*_brand/*/*_handler.s: - could you update the following lines with comments: xchgq CPTRSIZE(%rbp), %rax/* swap JMP table offset and ret addr */ shrq$4, %rax/* JMP table offset / JMP size = syscall num */ movq%rax, EH_LOCALS_GREG(REG_RAX)(%rbp) /* save syscall num */ Will do. -- usr/src/uts/i86pc/ml/syscall_asm.s: - don't you need to update this file as well? have you tested 32-bit kernels? No, this doesn't need to be updated since this code doesn't touch the user's stack. I have done preliminary testing with 32 bit kernels and the callbacks work correctly with the code as is. Thats because the 32 bit code is more like the 64 bit code that handles an interrupt stack where we already have the right data pushed. -- usr/src/uts/i86pc/ml/syscall_asm_amd64.s - perhaps you could do the following renames: BRAND_GET_RET_REG - BRAND_URET_FROM_REG BRAND_GET_RET_STACK - BRAND_URET_FROM_INTR_STACK Will do. - wrt this code: cmpq$NSYSCALL, %rax /* is 0 = syscall = MAX? */ jbe 0f /* yes, syscall is OK */ xorq%rax, %rax /* no, zero syscall number */ it's duplicated in every brand callback right after CALLBACK_PROLOGUE(). why not make it part of CALLBACK_PROLOGUE? Will do. also, if the syscall num is NSYSCALL, why not just jump to 9: and let the normal syscall path detect and return the error? OK. I was modeling this on the way lx did it but your suggestion seems better. - it seems like there should be a macro for this rough block of code (which calculates the jmp table address): GET_P_BRAND_DATA(%esp, 1, %edx);/* get p_brand_data ptr */ movlSPD_HANDLER(%edx), %edx /* get p_brand_data-spd_handler ptr */ shll$4, %eax addl%eax, %edx /* we'll return to our handler */ I'll put one together. - prior to these changes V_URET_ADDR wasn't always set, so the different brand syscall callbacks would get the userland return address from their syscall specific locations (registers, interrupt stack, etc). but now since V_URET_ADDR is always set, perhaps the callback handlers could be made more consistent by always getting the value from the stack (ie, via V_URET_ADDR)? - so following up with the last comment (and getting more into potential comminization work) it seems to me like it might be benificial to move all the syscall mechanism specific handling code out of the actual brand callbacks and into BRAND_CALLBACK. you've already started doing this by having BRAND_CALLBACK be aware of how to get the userland return address. (prior to that it didn't have any dependancy upon the different syscall mechanisms, except when deciding which brand callback to invoke.) continuing down that path we could move all the syscall specific handling code into BRAND_CALLBACK. then each brand would only deliver a single callback which would take one parameter, the syscall number. it would return one value, a userland return address. then BRAND_CALLBACK could handle all the different syscall specific return paths. this would also be benificial in the future since if a new syscall mechanism was introduced, we wouldn't have to update any actual brand callbacks, just BRAND_CALLBACK. thoughts? For these last two I agree that there are some good opportunities here and I was torn between doing a bunch more clean up on this and deferring that work to the fix for: 6900207 code can be shared between solaris10 and ipkg brands Since bug 6768950 is serious and I'd like to get the fix done sooner rather than later, I'd like to defer some of these other changes to 6900207. I was about to start on that anyway so once 6768950 is done I'm going to immediately start work on a bunch of ideas I have for making the code shared and simpler. I was also going to roll a fix for: 6887823 brandz on x86 should ignore %gs on 64-bit kernels into that same set of cleanup. I definitely agree with your comments here
Re: [zones-discuss] zones code review
On Tue, Dec 15, 2009 at 01:28:01PM -0700, Jerry Jelinek wrote: Ed, Thanks for reviewing this. My responses to your comments are in-line. Edward Pilatowicz wrote: On Tue, Dec 15, 2009 at 08:39:12AM -0700, Jerry Jelinek wrote: I have an initial code review for the fix for bug: 6768950 panic[cpu1]/thread=ff084ce0b3e0: syscall_asm_amd64.s:480 lwp ff0756a8cdc0, pcb_rupdate != 0 There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ The code changes in the sn1 and solaris10 brands are basically identical. I know there is a lot of common code there but I didn't want to clutter up this bug fix with the unrelated changes necessary to make the code common. I'll be addressing that with a separate fix. My initial testing of these changes looks good but I still need to run more extensive tests. ... - prior to these changes V_URET_ADDR wasn't always set, so the different brand syscall callbacks would get the userland return address from their syscall specific locations (registers, interrupt stack, etc). but now since V_URET_ADDR is always set, perhaps the callback handlers could be made more consistent by always getting the value from the stack (ie, via V_URET_ADDR)? - so following up with the last comment (and getting more into potential comminization work) it seems to me like it might be benificial to move all the syscall mechanism specific handling code out of the actual brand callbacks and into BRAND_CALLBACK. you've already started doing this by having BRAND_CALLBACK be aware of how to get the userland return address. (prior to that it didn't have any dependancy upon the different syscall mechanisms, except when deciding which brand callback to invoke.) continuing down that path we could move all the syscall specific handling code into BRAND_CALLBACK. then each brand would only deliver a single callback which would take one parameter, the syscall number. it would return one value, a userland return address. then BRAND_CALLBACK could handle all the different syscall specific return paths. this would also be benificial in the future since if a new syscall mechanism was introduced, we wouldn't have to update any actual brand callbacks, just BRAND_CALLBACK. thoughts? For these last two I agree that there are some good opportunities here and I was torn between doing a bunch more clean up on this and deferring that work to the fix for: 6900207 code can be shared between solaris10 and ipkg brands Since bug 6768950 is serious and I'd like to get the fix done sooner rather than later, I'd like to defer some of these other changes to 6900207. I was about to start on that anyway so once 6768950 is done I'm going to immediately start work on a bunch of ideas I have for making the code shared and simpler. I was also going to roll a fix for: 6887823 brandz on x86 should ignore %gs on 64-bit kernels into that same set of cleanup. I definitely agree with your comments here but I'm worried about the fix for 6768950 taking too long. sounds good. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] zones code review
On 12/15/09 07:39 AM, Jerry Jelinek wrote: I have an initial code review for the fix for bug: 6768950 panic[cpu1]/thread=ff084ce0b3e0: syscall_asm_amd64.s:480 lwp ff0756a8cdc0, pcb_rupdate != 0 There is a webrev at: http://cr.opensolaris.org/~gjelinek/webrev.6768950/ The code changes in the sn1 and solaris10 brands are basically identical. I know there is a lot of common code there but I didn't want to clutter up this bug fix with the unrelated changes necessary to make the code common. I'll be addressing that with a separate fix. My initial testing of these changes looks good but I still need to run more extensive tests. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org Hi Jerry, I'll add one question to Ed's suggestions: -- usr/src/lib/brand/sn1/sn1_brand/amd64/sn1_handler.s 44: Shouldn't this function be named sn1_handler_table? Jordan ___ zones-discuss mailing list zones-discuss@opensolaris.org