WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] 6 arguments hypercall for Linux

To: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] 6 arguments hypercall for Linux
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Fri, 24 Jun 2011 13:34:02 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 24 Jun 2011 05:36:13 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110624113638.GB14099@xxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <20110624113638.GB14099@xxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Fri, 2011-06-24 at 12:36 +0100, Jean Guyader wrote:
>                              From: 
> Jean Guyader
> <jean.guyader@xxxxxxxxxxxxx>
>                                To: 
> xen-devel@xxxxxxxxxxxxxxxxxxx
> <xen-devel@xxxxxxxxxxxxxxxxxxx>
>                           Subject: 
> [Xen-devel] [PATCH] 6 arguments
> hypercall for Linux
>                              Date: 
> Fri, 24 Jun 2011 12:36:38 +0100
>                        Message-id: 
> <20110624113638.GB14099@xxxxxxxxxxxxxxxxxxxxxxx>
> 
> 
> Hi,
> 
> This patch implements the 6 arguments hypercalls.
> The sixth argument is passed using ebp or r9.
> 
> Signed-off-by: Jean Guyader <jean.guyader@xxxxxxxxxx>
> 
> I tried to make this as clean as I could, but the hack we have
> to do to use ebp doesn't really help.

You seem to have included a whole bunch of code motion and whitespace
changes in this patch. It makes it really hard to review.

I guess the code motion is to allow moving some stuff into the existing 
#ifdef? I think it would be cleaner to simply repeat the ifdef in the
__HYPERCALL_<N>PARAM and ARG etc blocks as necessary. The benefit of
keeping those together I think outweighs the benefit of having a single
ifdef.
> 
> diff --git a/arch/x86/include/asm/xen/hypercall.h 
> b/arch/x86/include/asm/xen/hypercall.h
> index d240ea9..18327a7 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -74,8 +74,7 @@
>   *     - clobber the rest
>   *
>   * The result certainly isn't pretty, and it really shows up cpp's
> - * weakness as as macro language.  Sorry.  (But let's just give thanks
> - * there aren't more than 5 arguments...)
> + * weakness as as macro language.  Sorry.
>   */
>  
>  extern struct { char _entry[32]; } hypercall_page[];
> @@ -84,6 +83,13 @@ extern struct { char _entry[32]; } hypercall_page[];
>  #define __HYPERCALL_ENTRY(x)                                           \
>         [offset] "i" (__HYPERVISOR_##x * sizeof(hypercall_page[0]))
>  
> +#define __HYPERCALL_0PARAM     "=r" (__res)
> +#define __HYPERCALL_1PARAM     __HYPERCALL_0PARAM, "+r" (__arg1)
> +#define __HYPERCALL_2PARAM     __HYPERCALL_1PARAM, "+r" (__arg2)
> +#define __HYPERCALL_3PARAM     __HYPERCALL_2PARAM, "+r" (__arg3)
> +#define __HYPERCALL_4PARAM     __HYPERCALL_3PARAM, "+r" (__arg4)
> +#define __HYPERCALL_5PARAM     __HYPERCALL_4PARAM, "+r" (__arg5)

Why did these need to move and/or why is __HYPERCALL_6PARAM not declared
here too?

> +
>  #ifdef CONFIG_X86_32
>  #define __HYPERCALL_RETREG     "eax"
>  #define __HYPERCALL_ARG1REG    "ebx"
> @@ -91,6 +97,23 @@ extern struct { char _entry[32]; } hypercall_page[];
>  #define __HYPERCALL_ARG3REG    "edx"
>  #define __HYPERCALL_ARG4REG    "esi"
>  #define __HYPERCALL_ARG5REG    "edi"
> +
> +/*
> +** On 32b we are out of free registers to pass in
> +** the 6th argument of the hypercall, the last one
> +** available is ebp.
> +** %ebp is already being used by linux so we save it
> +** then we move %eax which is the 6th argument in %ebp.
> +** On the way back of the hypercall we restore %ebp.
> +*/

Linux comment style is:
   /*
    * Blah blah blah blah blah 
    * blah blah blah.
    */

> +#define __HYPERCALL6_PRE        "push %%ebp ; mov %%eax, %%ebp ; "
> +#define __HYPERCALL6_POST       ";" "pop %%ebp"

HYPERCALL_6... for consistency.

> +#define __HYPERCALL_6PARAM     __HYPERCALL_5PARAM
> +#define __HYPERCALL6_ENTRY(x, a6)                                       \
> +                                __HYPERCALL_ENTRY(x) , "0" ((long)(a6))
> +#define __HYPERCALL_6ARG(a1,a2,a3,a4,a5, a6)                            \
> +                                __HYPERCALL_5ARG(a1,a2,a3,a4, a5)

Can't you define this as "__HYPERCALL_5ARG(....) __res = (unsigned
long)a6"?

Then the the special HYPERCALL6_ENTRY goes away? HYPERCALL6_PRE remains
the same.

> +#define        __HYPERCALL_DECLS6      (void)0;
>  #else
>  #define __HYPERCALL_RETREG     "rax"
>  #define __HYPERCALL_ARG1REG    "rdi"
> @@ -98,6 +121,15 @@ extern struct { char _entry[32]; } hypercall_page[];
>  #define __HYPERCALL_ARG3REG    "rdx"
>  #define __HYPERCALL_ARG4REG    "r10"
>  #define __HYPERCALL_ARG5REG    "r8"
> +#define __HYPERCALL_ARG6REG    "r9"
> +#define __HYPERCALL6_PRE        ""
> +#define __HYPERCALL6_POST       ""
> +#define __HYPERCALL6_ENTRY(x, a6)   __HYPERCALL_ENTRY(x)
> +#define __HYPERCALL_6ARG(a1,a2,a3,a4,a5, a6)                                 
>   \
> +       __HYPERCALL_5ARG(a1,a2,a3,a4, a5)       __arg6 = (unsigned long)(a6);
> +#define __HYPERCALL_6PARAM     __HYPERCALL_5PARAM, "+r" (__arg6)
> +#define __HYPERCALL_DECLS6                                                   
>    \
> +       register unsigned long __arg6 asm(__HYPERCALL_ARG6REG) = __arg6;
>  #endif
>  
>  #define __HYPERCALL_DECLS                                              \
> @@ -106,27 +138,22 @@ extern struct { char _entry[32]; } hypercall_page[];
>         register unsigned long __arg2 asm(__HYPERCALL_ARG2REG) = __arg2; \
>         register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
>         register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
> -       register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5;
> -
> -#define __HYPERCALL_0PARAM     "=r" (__res)
> -#define __HYPERCALL_1PARAM     __HYPERCALL_0PARAM, "+r" (__arg1)
> -#define __HYPERCALL_2PARAM     __HYPERCALL_1PARAM, "+r" (__arg2)
> -#define __HYPERCALL_3PARAM     __HYPERCALL_2PARAM, "+r" (__arg3)
> -#define __HYPERCALL_4PARAM     __HYPERCALL_3PARAM, "+r" (__arg4)
> -#define __HYPERCALL_5PARAM     __HYPERCALL_4PARAM, "+r" (__arg5)
> +       register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5; \
> +       __HYPERCALL_DECLS6
>  
>  #define __HYPERCALL_0ARG()
> -#define __HYPERCALL_1ARG(a1)                                           \
> -       __HYPERCALL_0ARG()              __arg1 = (unsigned long)(a1);
> -#define __HYPERCALL_2ARG(a1,a2)                                              
>   \
> -       __HYPERCALL_1ARG(a1)            __arg2 = (unsigned long)(a2);
> -#define __HYPERCALL_3ARG(a1,a2,a3)                                     \
> -       __HYPERCALL_2ARG(a1,a2)         __arg3 = (unsigned long)(a3);
> -#define __HYPERCALL_4ARG(a1,a2,a3,a4)                                  \
> -       __HYPERCALL_3ARG(a1,a2,a3)      __arg4 = (unsigned long)(a4);
> -#define __HYPERCALL_5ARG(a1,a2,a3,a4,a5)                               \
> -       __HYPERCALL_4ARG(a1,a2,a3,a4)   __arg5 = (unsigned long)(a5);
> -
> +#define __HYPERCALL_1ARG(a1)                                                 
>   \
> +       __HYPERCALL_0ARG()                      __arg1 = (unsigned long)(a1);
> +#define __HYPERCALL_2ARG(a1,a2)                                              
>   \
> +       __HYPERCALL_1ARG(a1)                    __arg2 = (unsigned long)(a2);
> +#define __HYPERCALL_3ARG(a1,a2,a3)                                           
>   \
> +       __HYPERCALL_2ARG(a1,a2)                 __arg3 = (unsigned long)(a3);
> +#define __HYPERCALL_4ARG(a1,a2,a3,a4)                                        
>   \
> +       __HYPERCALL_3ARG(a1,a2,a3)              __arg4 = (unsigned long)(a4);
> +#define __HYPERCALL_5ARG(a1,a2,a3,a4,a5)                                     
>   \
> +       __HYPERCALL_4ARG(a1,a2,a3,a4)           __arg5 = (unsigned long)(a5);

You didn't actually change anything other than whitespace here?

> +
> +#define __HYPERCALL_CLOBBER6   "memory"
>  #define __HYPERCALL_CLOBBER5   "memory"
>  #define __HYPERCALL_CLOBBER4   __HYPERCALL_CLOBBER5, __HYPERCALL_ARG5REG
>  #define __HYPERCALL_CLOBBER3   __HYPERCALL_CLOBBER4, __HYPERCALL_ARG4REG
> @@ -200,6 +227,19 @@ extern struct { char _entry[32]; } hypercall_page[];
>         (type)__res;                                                    \
>  })
>  
> +#define _hypercall6(type, name, a1, a2, a3, a4, a5, a6)                \
> +({                                                                     \
> +       __HYPERCALL_DECLS;                                              \
> +       __HYPERCALL_6ARG(a1, a2, a3, a4, a5, a6);                       \
> +       asm volatile ( __HYPERCALL6_PRE                                 \
> +                       __HYPERCALL                                     \
> +                       __HYPERCALL6_POST                               \
> +                     : __HYPERCALL_6PARAM                              \
> +                     : __HYPERCALL6_ENTRY(name, a6)                    \
> +                     : __HYPERCALL_CLOBBER6);                          \
> +       (type)__res;                                                    \
> +})
> +
>  static inline long
>  privcmd_call(unsigned call,
>              unsigned long a1, unsigned long a2,
> 
> 
> 
> 
> 
> 
> 
> plain text document attachment (ATT00001.txt)
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>