Re: [PATCH] Enble 6 argument hypercalls for HVMs

[Keir Fraser <keir@xen.org>]

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
>