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

From: Zhao, Yakui
Date: Thu Apr 25 2019 - 06:18:35 EST

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 */

+#include <linux/errno.h>
+ * 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:

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

+ 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

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));