[PATCH] Enble 6 argument hypercalls for HVMs

[PATCH] Enble 6 argument hypercalls for HVMs

[Ross Philipson]

[-- Attachment #1: Type: text/plain, Size: 205 bytes --]

Enable 6 argument hypercalls for HVMs. The hypercall code handles a sixth argument in EBP or R9 but the HVM code is not passing the value.

Signed-off-by: Ross Philipson <ross.philipson@citrix.com>



[-- Attachment #2: hvm-6arg-hypercall.patch --]
[-- Type: application/octet-stream, Size: 2407 bytes --]

diff -r 5e443a99b1ff xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Tue Dec 14 17:38:18 2010 +0000
+++ b/xen/arch/x86/hvm/hvm.c	Tue Dec 14 13:43:08 2010 -0500
@@ -2514,7 +2514,8 @@
 }
 
 typedef unsigned long hvm_hypercall_t(
-    unsigned long, unsigned long, unsigned long, unsigned long, unsigned long);
+    unsigned long, unsigned long, unsigned long, unsigned long, unsigned long,
+    unsigned long);
 
 #define HYPERCALL(x)                                        \
     [ __HYPERVISOR_ ## x ] = (hvm_hypercall_t *) do_ ## x
@@ -2664,30 +2665,32 @@
 #ifdef __x86_64__
     if ( mode == 8 )
     {
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%lx, %lx, %lx, %lx, %lx)", eax,
-                    regs->rdi, regs->rsi, regs->rdx, regs->r10, regs->r8);
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%lx, %lx, %lx, %lx, %lx, %lx)", eax,
+                    regs->rdi, regs->rsi, regs->rdx, regs->r10, regs->r8, regs->r9);
 
         curr->arch.hvm_vcpu.hcall_64bit = 1;
         regs->rax = hvm_hypercall64_table[eax](regs->rdi,
                                                regs->rsi,
                                                regs->rdx,
                                                regs->r10,
-                                               regs->r8); 
+                                               regs->r8,
+                                               regs->r9); 
         curr->arch.hvm_vcpu.hcall_64bit = 0;
     }
     else
 #endif
     {
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%x, %x, %x, %x, %x)", eax,
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%x, %x, %x, %x, %x, %x)", eax,
                     (uint32_t)regs->ebx, (uint32_t)regs->ecx,
                     (uint32_t)regs->edx, (uint32_t)regs->esi,
-                    (uint32_t)regs->edi);
+                    (uint32_t)regs->edi, (uint32_t)regs->ebp);
 
         regs->eax = hvm_hypercall32_table[eax]((uint32_t)regs->ebx,
                                                (uint32_t)regs->ecx,
                                                (uint32_t)regs->edx,
                                                (uint32_t)regs->esi,
-                                               (uint32_t)regs->edi);
+                                               (uint32_t)regs->edi,
+                                               (uint32_t)regs->ebp);
     }
 
     HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u -> %lx",

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Jan Beulich]

>>> On 14.12.10 at 23:16, Ross Philipson <Ross.Philipson@citrix.com> wrote:
> Enable 6 argument hypercalls for HVMs. The hypercall code handles a sixth 
> argument in EBP or R9 but the HVM code is not passing the value.
> 
> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>

I'm curious what hypercall there is that takes 6 arguments,
particularly on 64-bit (where the maximum so far is 4).

Jan

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Keir Fraser]

On 15/12/2010 09:07, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 14.12.10 at 23:16, Ross Philipson <Ross.Philipson@citrix.com> wrote:
>> Enable 6 argument hypercalls for HVMs. The hypercall code handles a sixth
>> argument in EBP or R9 but the HVM code is not passing the value.
>> 
>> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
> 
> I'm curious what hypercall there is that takes 6 arguments,
> particularly on 64-bit (where the maximum so far is 4).

The v4v hypercalls in XenClient (not as yet submitted upstream) take 6
arguments. Multicalls also need fixing up for a sixth argument, making
everything consistent with existing PV hypercall logic.

 -- Keir

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

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Jan Beulich]

>>> On 15.12.10 at 11:06, Keir Fraser <keir@xen.org> wrote:
> On 15/12/2010 09:07, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>>>>> On 14.12.10 at 23:16, Ross Philipson <Ross.Philipson@citrix.com> wrote:
>>> Enable 6 argument hypercalls for HVMs. The hypercall code handles a sixth
>>> argument in EBP or R9 but the HVM code is not passing the value.
>>> 
>>> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
>> 
>> I'm curious what hypercall there is that takes 6 arguments,
>> particularly on 64-bit (where the maximum so far is 4).
> 
> The v4v hypercalls in XenClient (not as yet submitted upstream) take 6
> arguments. Multicalls also need fixing up for a sixth argument, making
> everything consistent with existing PV hypercall logic.

I would generally take this as an indication that this actually works,
but at least with tracing enabled I can't see how it would on 64-bit
(note the last two reloads):

        call  trace_hypercall
        /* Now restore all the registers that trace_hypercall clobbered */
        movq  UREGS_rax+SHADOW_BYTES(%rsp),%rax   /* Hypercall #  */
        movq  UREGS_rdi+SHADOW_BYTES(%rsp),%rdi   /* Arg 1        */
        movq  UREGS_rsi+SHADOW_BYTES(%rsp),%rsi   /* Arg 2        */
        movq  UREGS_rdx+SHADOW_BYTES(%rsp),%rdx   /* Arg 3        */
        movq  UREGS_r10+SHADOW_BYTES(%rsp),%rcx   /* Arg 4        */
        movq  UREGS_rdi+SHADOW_BYTES(%rsp),%r8    /* Arg 5        */
        movq  UREGS_rbp+SHADOW_BYTES(%rsp),%r9    /* Arg 6        */

Looking at this code also makes me wonder once again whether
it really is a good idea to have a generally not taken forward
branch here.

Jan

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Keir Fraser]

On 15/12/2010 10:40, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 15.12.10 at 11:06, Keir Fraser <keir@xen.org> wrote:
>> On 15/12/2010 09:07, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>>>>> On 14.12.10 at 23:16, Ross Philipson <Ross.Philipson@citrix.com> wrote:
>>>> Enable 6 argument hypercalls for HVMs. The hypercall code handles a sixth
>>>> argument in EBP or R9 but the HVM code is not passing the value.
>>>> 
>>>> Signed-off-by: Ross Philipson <ross.philipson@citrix.com>
>>> 
>>> I'm curious what hypercall there is that takes 6 arguments,
>>> particularly on 64-bit (where the maximum so far is 4).
>> 
>> The v4v hypercalls in XenClient (not as yet submitted upstream) take 6
>> arguments. Multicalls also need fixing up for a sixth argument, making
>> everything consistent with existing PV hypercall logic.
> 
> I would generally take this as an indication that this actually works,
> but at least with tracing enabled I can't see how it would on 64-bit
> (note the last two reloads):

Yes, well, obviously noone has tried 6-arg (or 5-arg!) hypercalls from a
64-bit PV guest with tracing enabled. The code appears obviously wrong here.
Cc'ing George -- he should be able to test this and submit the obvious
patch. This looks like a cut-n-paste error from x86_64/compat/entry.S into
x86_64/entry.S -- it wouldn't have been picked up by George because we don't
generally run any 64-bit PV guests.

>         call  trace_hypercall
>         /* Now restore all the registers that trace_hypercall clobbered */
>         movq  UREGS_rax+SHADOW_BYTES(%rsp),%rax   /* Hypercall #  */
>         movq  UREGS_rdi+SHADOW_BYTES(%rsp),%rdi   /* Arg 1        */
>         movq  UREGS_rsi+SHADOW_BYTES(%rsp),%rsi   /* Arg 2        */
>         movq  UREGS_rdx+SHADOW_BYTES(%rsp),%rdx   /* Arg 3        */
>         movq  UREGS_r10+SHADOW_BYTES(%rsp),%rcx   /* Arg 4        */
>         movq  UREGS_rdi+SHADOW_BYTES(%rsp),%r8    /* Arg 5        */
>         movq  UREGS_rbp+SHADOW_BYTES(%rsp),%r9    /* Arg 6        */
> 
> Looking at this code also makes me wonder once again whether
> it really is a good idea to have a generally not taken forward
> branch here.

Which generally not-taken branch? The 'je 1f' instruction generally *will*
be taken!

 -- Keir

> Jan
> 

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Jan Beulich]

>>> On 15.12.10 at 12:43, Keir Fraser <keir@xen.org> wrote:
> On 15/12/2010 10:40, "Jan Beulich" <JBeulich@novell.com> wrote:
>> Looking at this code also makes me wonder once again whether
>> it really is a good idea to have a generally not taken forward
>> branch here.
> 
> Which generally not-taken branch? The 'je 1f' instruction generally *will*
> be taken!

Oops, of course - it being taken is the problem, as it'll be statically
mis-predicted.

Jan

Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Keir Fraser]

On 15/12/2010 12:12, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 15.12.10 at 12:43, Keir Fraser <keir@xen.org> wrote:
>> On 15/12/2010 10:40, "Jan Beulich" <JBeulich@novell.com> wrote:
>>> Looking at this code also makes me wonder once again whether
>>> it really is a good idea to have a generally not taken forward
>>> branch here.
>> 
>> Which generally not-taken branch? The 'je 1f' instruction generally *will*
>> be taken!
> 
> Oops, of course - it being taken is the problem, as it'll be statically
> mis-predicted.

Well, moving the trace code out of line, perhaps into the fixup section,
would be good for cache locality. And then we would have a rarely taken
forward branch (since the fixup section comes after normal text).

I'm sure there are a bunch of places in our asm code where we could do this.
Feel free to fix them all up, if you like.

 -- Keir

> Jan
>