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

[Wei Liu <wl@xxxxxxx>]

On Sun, Jan 05, 2020 at 10:06:08PM +0000, Andrew Cooper wrote:
> 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.

But r8 is only listed in clobber list, so it certainly doesn't overlap
with any input register. I fail to see what the bug (if there is any) is
here.

I think what you're asking for here is an optimisation. Is that correct?
I don't mind changing the code. What I need is clarification here.

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

The call sites will be like

        struct hv_input_arg *input_arg;
        input_arg = per_cpu_input_page;
        input_arg.foo = xxx;
        input_arg.bar = xxx;

        hv_do_hypercall(control, virt_to_maddr(input_arg), NULL);

.

(Alternatively, we can put virt_to_maddr in hv_do_hypercall now that
we're sure the input page is from xenheap)

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

See Linux commit fc53662f13b.

I will also ask my contact in Hyper-V team for clarification.

Wei.

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