Re: [RFC PATCH v5 4/4] x86/acrn: Add hypercall for ACRN guest

[Zhao, Yakui]

On 2019å04æ25æ 15:07, Ingo Molnar wrote:

Thanks for the review.
> * Zhao Yakui <yakui.zhao@xxxxxxxxx> wrote:
>
> > When ACRN hypervisor is detected, the hypercall is needed so that the
> > ACRN guest can query/config some settings. For example: it can be used
> > to query the resources in hypervisor and manage the CPU/memory/device/
> > interrupt for the guest operating system.
> >
> > So add the hypercall so that ACRN guest can communicate with the
> > low-level ACRN hypervisor. It is implemented with the VMCALL instruction.
> >
> > Co-developed-by: Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
> > Signed-off-by: Jason Chen CJ <jason.cj.chen@xxxxxxxxx>
> > Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx>
> > ---
> > V1->V2: Refine the comments for the function of acrn_hypercall0/1/2
> > v2->v3: Use the "vmcall" mnemonic to replace hard-code byte definition
> > v4->v5: Use _ASM_X86_ACRN_HYPERCALL_H instead of _ASM_X86_ACRNHYPERCALL_H to
> > align the header file of acrn_hypercall.h
> >          Use the "VMCALL" mnemonic in comment/commit log.
> >          Uppercase r8/rdi/rsi/rax for hypercall parameter registers in comment.
> > ---
> >   arch/x86/include/asm/acrn_hypercall.h | 82 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 82 insertions(+)
> >   create mode 100644 arch/x86/include/asm/acrn_hypercall.h
> >
> > diff --git a/arch/x86/include/asm/acrn_hypercall.h b/arch/x86/include/asm/acrn_hypercall.h
> > new file mode 100644
> > index 0000000..3594436
> > --- /dev/null
> > +++ b/arch/x86/include/asm/acrn_hypercall.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _ASM_X86_ACRN_HYPERCALL_H
> > +#define _ASM_X86_ACRN_HYPERCALL_H
>
>
> > +
> > +#include <linux/errno.h>
> > +
> > +#ifdef CONFIG_ACRN_GUEST
> > +
> > +/*
> > + * Hypercalls for ACRN guest
> > + *
> > + * Hypercall number is passed in R8 register.
> > + * Up to 2 arguments are passed in RDI, RSI.
> > + * Return value will be placed in RAX.
> > + */
> > +
> > +static inline long acrn_hypercall0(unsigned long hcall_id)
> > +{
> > +	register unsigned long r8 asm("r8") = hcall_id;
> > +	register long result asm("rax");
> > +
> > +	/* the hypercall is implemented with the VMCALL instruction.
> > +	 * asm indicates that inline assembler instruction is used.
> > +	 * volatile qualifier is added to avoid that it is dropped
> > +	 * because of compiler optimization.
> > +	 */
>
> Non-standard comment style.
>
> asm statements are volatile by default I believe.

For the basic asm:  it is volatile by default.
For the extend asm:  The volatile is needed to disable the certain 
optimization.
The below info is copied from: 
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm

The typical use of extended asm statements is to manipulate input values 
to produce output values. However, your asm statements may also produce 
side effects. If so, you may need to use the volatile qualifier to 
disable certain optimizations. See Volatile.


> I.e. the second and third sentences are partly obvious, superfluous and
> bogus.
>
> > +	asm volatile("vmcall"
> > +			: "=r"(result)
> > +			: "r"(r8));
> > +
> > +	return result;
> > +}
> > +
> > +static inline long acrn_hypercall1(unsigned long hcall_id,
> > +				   unsigned long param1)
> > +{
> > +	register unsigned long r8 asm("r8") = hcall_id;
> > +	register long result asm("rax");
> > +
> > +	asm volatile("vmcall"
> > +			: "=r"(result)
> > +			: "D"(param1), "r"(r8));
>
> Why are register variables used? Doesn't GCC figure it out correctly by
> default?

The parameter register for the VMCALL is predefined in ACRN hypervisor. 
Now the R8 is used to pass the hcall_id.
It seems that there is no special constraint for R8~R15.
So the explicit register variable is used so that the R8 can be passed.

> > +static inline long acrn_hypercall2(unsigned long hcall_id,
> > +				   unsigned long param1,
> > +				   unsigned long param2)
> > +{
> > +	register unsigned long r8 asm("r8") = hcall_id;
> > +	register long result asm("rax");
> > +
> > +	asm volatile("vmcall"
> > +			: "=r"(result)
> > +			: "D"(param1), "S"(param2), "r"(r8));
>
> Ditto.
>
> Thanks,
>
> 	Ingo