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
|
Home