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

Reply via email to