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

Reply via email to