Re: [zones-discuss] zones code review

2010-01-19 Thread Edward Pilatowicz
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 INTERPO

Re: [zones-discuss] zones code review

2010-01-19 Thread Jerry Jelinek
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

Re: [zones-discuss] zones code review

2010-01-19 Thread Edward Pilatowicz
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: > >> > >>6

Re: [zones-discuss] zones code review

2010-01-18 Thread Jerry Jelinek
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

Re: [zones-discuss] zones code review

2010-01-15 Thread Edward Pilatowicz
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 handl

Re: [zones-discuss] zones code review

2010-01-15 Thread Jordan Vaughan
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

Re: [zones-discuss] zones code review

2009-12-19 Thread Jerry Jelinek
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-

Re: [zones-discuss] zones code review

2009-12-18 Thread Edward Pilatowicz
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

Re: [zones-discuss] zones code review

2009-12-18 Thread Jerry Jelinek
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

Re: [zones-discuss] zones code review

2009-12-18 Thread Jerry Jelinek
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 p

Re: [zones-discuss] zones code review

2009-12-17 Thread Edward Pilatowicz
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. > >... > >*/ > >#

Re: [zones-discuss] zones code review

2009-12-17 Thread Jerry Jelinek
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.) ho

Re: [zones-discuss] zones code review

2009-12-17 Thread Edward Pilatowicz
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

Re: [zones-discuss] zones code review

2009-12-17 Thread Jerry Jelinek
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

Re: [zones-discuss] zones code review

2009-12-16 Thread Edward Pilatowicz
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/ >

Re: [zones-discuss] zones code review

2009-12-16 Thread Jerry Jelinek
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"

Re: [zones-discuss] zones code review

2009-12-16 Thread Edward Pilatowicz
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

Re: [zones-discuss] zones code review

2009-12-16 Thread Jerry Jelinek
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-dis

Re: [zones-discuss] zones code review

2009-12-16 Thread Jerry Jelinek
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

Re: [zones-discuss] zones code review

2009-12-15 Thread Jordan Vaughan
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

Re: [zones-discuss] zones code review

2009-12-15 Thread Edward Pilatowicz
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: > >

Re: [zones-discuss] zones code review

2009-12-15 Thread Jerry Jelinek
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 lw

Re: [zones-discuss] zones code review

2009-12-15 Thread Edward Pilatowicz
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/~gjel