Re: [Xen-devel] [PATCH v3 2/5] x86/hyperv: provide Hyper-V hypercall functions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/2020 21:22, Wei Liu wrote:
> On Sun, Jan 05, 2020 at 07:08:28PM +0000, Andrew Cooper wrote:
>>> +static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input, 
>>> paddr_t output)
>>> +{
>>> +    uint64_t status;
>>> +
>>> +    asm volatile ("mov %[output], %%r8\n"
>>> +                  "call hv_hypercall_page"
>>> +                  : "=a" (status), "+c" (control),
>>> +                    "+d" (input) ASM_CALL_CONSTRAINT
>>> +                  : [output] "rm" (output)
>>> +                  : "cc", "memory", "r8", "r9", "r10", "r11");
>> I think you want:
>>
>> register unsigned long r8 asm("r8") = output;
>>
>> and "+r" (r8) as an output constraint.
> Although it is named `output`, it is really just an input parameter from
> Hyper-V's PoV.

Yes, but it is also clobbered.

This is an awkward corner case of gnu inline assembly.

It is not permitted to have a clobber list overlap with any input/output
operations, and because r8 doesn't have a unique letter, you can't do
the usual trick of "=r8" (discard) : "r8" (input).

The only available option is to mark it as read and written (which is
"+r" in the output list), and not use the C variable r8 at any point later.


Having looked through the spec a bit more, is this a wise API to have in
the first place?  input and output (perhaps better named input_addr and
output_addr) are fixed per CPU, and control is semantically linked to
the hypercall and its particular ABI.

I suppose the answer ultimately depends on what the callers look like.

>
>> In particular, that doesn't force the compiler to put output into a
>> register other than r8 (or worse, spill it to the stack) to have the
>> opaque blob of asm move it back into r8.  What it will do in practice is
>> cause the compiler to construct output directly in r8.
>>
>> As for the other clobbers, I can't find anything at all in the spec
>> which even mentions those registers.  There will be a decent improvement
>> to code generation if we don't force them to be spilled around a hypercall.
>>
> Neither can I. But Linux's commit says that's needed, so I chose to err
> on the safe side.

That's dull.  Is there any qualifying information?

>> However, HyperV will(may?) modify %xmm{0..5} in some cases, and this
>> will corrupt the current vcpu's FPU context.  Care will need to be taken
>> to spill these if necessary.
>>
> The hypercalls we care about (TLB, APIC etc) don't use that ABI as far
> as I can tell. At the very least Linux, which uses the same set of
> hypercalls, doesn't need to save / restore XMM registers around the
> calls.

Ok - it looks to be fine until we need to use a hypercall which uses the
fast extended ABI, which is the first to introduce the use of the %xmm
registers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel