[RFC PATCH v5 0/4] x86: Add the support of ACRN guest under x86

[RFC PATCH v5 0/4] x86: Add the support of ACRN guest under x86

[Zhao Yakui]

ACRN is a flexible, lightweight reference hypervisor, built with real-time
and safety-criticality in mind, optimized to streamline embedded development
through an open source platform. It is built for embedded IOT with small
footprint and real-time features. More details can be found
in https://projectacrn.org/

This is the patch set that allows the Linux to work on ACRN hypervisor and it can
work with the following patch set to manage the Linux guest on ACRN hypervisor. It
includes the detection of ACRN hypervisor, upcall notification vector from
hypervisor, hypercall. The hypervisor detection is similar to Xen/VMWARE/Hyperv.
ACRN also uses the upcall notification mechanism similar to that in Xen/Microsoft
HyperV when it needs to send the notification to Linux OS. The hypercall provides
the mechanism that can be used to query/configure the ACRN hypervisor by Linux guest.

Following this patch set, we will send acrn driver part, which provides the interface
that can be used to manage the virtualized CPU/memory/device/interrupt for other guest
OS after the ACRN hypervisor is detected.

v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
understand.
	Remove the export of x86_hyper_acrn.
	Remove the unused API definition of acrn_setup_intr_handler and
acrn_remove_intr_handler.
        Adjust the order of header file
	Add the declaration of acrn_hv_vector_handler and tracing
definition of acrn_hv_callback_vector.
	Refine the comments for the function of acrn_hypercall0/1/2

v2-v3:  Add one new config symbol to unify the conditional definition
of hv_irq_callback_count
	Use the "vmcall" mnemonic to replace the hard-code byte definition
	Remove the unnecessary dependency of CONFIG_PARAVIRT for ACRN_GUEST

v3-v4:  Rename the file name of acrnhyper.h to acrn.h
	Refine the commit log and some other minor changes(more comments and 
redundant ifdef in acrn.h, sorting the header file in acrn.c)

v4->v5: Minor changes of comments/commit log in patch 04
	Use _ASM_X86_ACRN_HYPERCALL_H instead of _ASM_X86_ACRNHYPERCALL_H.
	Use the "VMCALL" mnemonic in comment/commit log.
	Uppercase r8/rdi/rsi/rax for hypercall parameter register in comment.

Zhao Yakui (4):
  x86/Kconfig: Add new config symbol to unify conditional definition of
    hv_irq_callback_count
  x86: Add the support of Linux guest on ACRN hypervisor
  x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector
  x86/acrn: Add hypercall for ACRN guest

 arch/x86/Kconfig                      | 16 +++++++
 arch/x86/entry/entry_64.S             |  5 +++
 arch/x86/include/asm/acrn.h           | 11 +++++
 arch/x86/include/asm/acrn_hypercall.h | 81 +++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/hardirq.h        |  2 +-
 arch/x86/include/asm/hypervisor.h     |  1 +
 arch/x86/kernel/cpu/Makefile          |  1 +
 arch/x86/kernel/cpu/acrn.c            | 61 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c      |  4 ++
 arch/x86/kernel/irq.c                 |  2 +-
 arch/x86/xen/Kconfig                  |  1 +
 drivers/hv/Kconfig                    |  1 +
 12 files changed, 184 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/acrn.h
 create mode 100644 arch/x86/include/asm/acrn_hypercall.h
 create mode 100644 arch/x86/kernel/cpu/acrn.c

-- 
2.7.4

[RFC PATCH v5 1/4] x86/Kconfig: Add new config symbol to unify conditional definition of hv_irq_callback_count

[Zhao Yakui]

Add a special Kconfig symbol X86_HV_CALLBACK_VECTOR so that the guests
using the hypervisor interrupt callback counter can select and thus
enable that counter. Select it when xen or hyperv support is enabled.
No functional changes.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
v3->v4: Follow the comments to refine the commit log.
v4->v5: No change
---
 arch/x86/Kconfig               | 3 +++
 arch/x86/include/asm/hardirq.h | 2 +-
 arch/x86/kernel/irq.c          | 2 +-
 arch/x86/xen/Kconfig           | 1 +
 drivers/hv/Kconfig             | 1 +
 5 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 62fc3fd..2fc9297 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -791,6 +791,9 @@ config QUEUED_LOCK_STAT
 	  behavior of paravirtualized queued spinlocks and report
 	  them on debugfs.
 
+config X86_HV_CALLBACK_VECTOR
+	def_bool n
+
 source "arch/x86/xen/Kconfig"
 
 config KVM_GUEST
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index d9069bb..0753379 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -37,7 +37,7 @@ typedef struct {
 #ifdef CONFIG_X86_MCE_AMD
 	unsigned int irq_deferred_error_count;
 #endif
-#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
+#ifdef CONFIG_X86_HV_CALLBACK_VECTOR
 	unsigned int irq_hv_callback_count;
 #endif
 #if IS_ENABLED(CONFIG_HYPERV)
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 59b5f2e..a147826 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -134,7 +134,7 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 		seq_printf(p, "%10u ", per_cpu(mce_poll_count, j));
 	seq_puts(p, "  Machine check polls\n");
 #endif
-#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
+#ifdef CONFIG_X86_HV_CALLBACK_VECTOR
 	if (test_bit(HYPERVISOR_CALLBACK_VECTOR, system_vectors)) {
 		seq_printf(p, "%*s: ", prec, "HYP");
 		for_each_online_cpu(j)
diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index e07abef..ba5a418 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -7,6 +7,7 @@ config XEN
 	bool "Xen guest support"
 	depends on PARAVIRT
 	select PARAVIRT_CLOCK
+	select X86_HV_CALLBACK_VECTOR
 	depends on X86_64 || (X86_32 && X86_PAE)
 	depends on X86_LOCAL_APIC && X86_TSC
 	help
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 1c1a251..cafcb97 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -6,6 +6,7 @@ config HYPERV
 	tristate "Microsoft Hyper-V client drivers"
 	depends on X86 && ACPI && X86_LOCAL_APIC && HYPERVISOR_GUEST
 	select PARAVIRT
+	select X86_HV_CALLBACK_VECTOR
 	help
 	  Select this option to run Linux as a Hyper-V client operating
 	  system.
-- 
2.7.4

[RFC PATCH v5 2/4] x86: Add the support of Linux guest on ACRN hypervisor

[Zhao Yakui]

ACRN is an open-source hypervisor maintained by Linux Foundation.
It is built for embedded IOT with small footprint and real-time features.
Add the ACRN guest support so that it allows Linux to be booted under
ACRN hypervisor. Following this patch it will setup the upcall
notification vector and enable hypercall. And after ACRN guest is
supported, the ACRN driver part can add the interface that is used to
manage the virtualized CPU/memory/device/interrupt for other guest system.

Co-developed-by: Jason Chen CJ <jason.cj.chen@intel.com>
Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
understand.
        Remove the export of x86_hyper_acrn.

v2->v3: Remove the unnecessary dependency of PARAVIRT
v3->v4: Refine the commit log and add meaningful description in Kconfig
v4->v5: Minor change for the commit log.
---
 arch/x86/Kconfig                  | 12 ++++++++++++
 arch/x86/include/asm/hypervisor.h |  1 +
 arch/x86/kernel/cpu/Makefile      |  1 +
 arch/x86/kernel/cpu/acrn.c        | 39 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
 5 files changed, 57 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/acrn.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2fc9297..8dc4200 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -845,6 +845,18 @@ config JAILHOUSE_GUEST
 	  cell. You can leave this option disabled if you only want to start
 	  Jailhouse and run Linux afterwards in the root cell.
 
+config ACRN_GUEST
+	bool "ACRN Guest support"
+	depends on X86_64
+	help
+	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
+	  this will allow the kernel to boot in virtualized environment under
+	  the ACRN hypervisor.
+	  ACRN is a flexible, lightweight reference open-source hypervisor, built
+	  with real-time and safety-criticality in mind. It is built for embedded
+	  IOT with small footprint and real-time features. More details can be
+	  found in https://projectacrn.org/
+
 endif #HYPERVISOR_GUEST
 
 source "arch/x86/Kconfig.cpu"
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 8c5aaba..50a30f6 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -29,6 +29,7 @@ enum x86_hypervisor_type {
 	X86_HYPER_XEN_HVM,
 	X86_HYPER_KVM,
 	X86_HYPER_JAILHOUSE,
+	X86_HYPER_ACRN,
 };
 
 #ifdef CONFIG_HYPERVISOR_GUEST
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index cfd24f9..17a7cdf 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_X86_CPU_RESCTRL)		+= resctrl/
 obj-$(CONFIG_X86_LOCAL_APIC)		+= perfctr-watchdog.o
 
 obj-$(CONFIG_HYPERVISOR_GUEST)		+= vmware.o hypervisor.o mshyperv.o
+obj-$(CONFIG_ACRN_GUEST)		+= acrn.o
 
 ifdef CONFIG_X86_FEATURE_NAMES
 quiet_cmd_mkcapflags = MKCAP   $@
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
new file mode 100644
index 0000000..f556640
--- /dev/null
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACRN detection support
+ *
+ * Copyright (C) 2019 Intel Corporation. All rights reserved.
+ *
+ * Jason Chen CJ <jason.cj.chen@intel.com>
+ * Zhao Yakui <yakui.zhao@intel.com>
+ *
+ */
+
+#include <asm/hypervisor.h>
+
+static uint32_t __init acrn_detect(void)
+{
+	return hypervisor_cpuid_base("ACRNACRNACRN\0\0", 0);
+}
+
+static void __init acrn_init_platform(void)
+{
+}
+
+static bool acrn_x2apic_available(void)
+{
+	/* x2apic is not supported now.
+	 * Later it needs to check the X86_FEATURE_X2APIC bit of cpu info
+	 * returned by CPUID to determine whether the x2apic is
+	 * supported in Linux guest.
+	 */
+	return false;
+}
+
+const __initconst struct hypervisor_x86 x86_hyper_acrn = {
+	.name                   = "ACRN",
+	.detect                 = acrn_detect,
+	.type			= X86_HYPER_ACRN,
+	.init.init_platform     = acrn_init_platform,
+	.init.x2apic_available  = acrn_x2apic_available,
+};
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 479ca47..87e39ad 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -32,6 +32,7 @@ extern const struct hypervisor_x86 x86_hyper_xen_pv;
 extern const struct hypervisor_x86 x86_hyper_xen_hvm;
 extern const struct hypervisor_x86 x86_hyper_kvm;
 extern const struct hypervisor_x86 x86_hyper_jailhouse;
+extern const struct hypervisor_x86 x86_hyper_acrn;
 
 static const __initconst struct hypervisor_x86 * const hypervisors[] =
 {
@@ -49,6 +50,9 @@ static const __initconst struct hypervisor_x86 * const hypervisors[] =
 #ifdef CONFIG_JAILHOUSE_GUEST
 	&x86_hyper_jailhouse,
 #endif
+#ifdef CONFIG_ACRN_GUEST
+	&x86_hyper_acrn,
+#endif
 };
 
 enum x86_hypervisor_type x86_hyper_type;
-- 
2.7.4

[RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Zhao Yakui]

Linux kernel uses the HYPERVISOR_CALLBACK_VECTOR for hypervisor upcall
vector. And it is already used for Xen and HyperV.
After ACRN hypervisor is detected, it will also use this defined vector
to notify ACRN guest.

Co-developed-by: Jason Chen CJ <jason.cj.chen@intel.com>
Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
V1->V2: Remove the unused API definition of acrn_setup_intr_handler and
acrn_remove_intr_handler.
        Adjust the order of header file
        Add the declaration of acrn_hv_vector_handler and tracing
definition of acrn_hv_callback_vector.

v2->v3: Select the X86_HV_CALLBACK_VECTOR for ACRN guest
v3->v4: Refine the file name of acrnhyper.h to acrn.h
v4->v5: no change
---
 arch/x86/Kconfig            |  1 +
 arch/x86/entry/entry_64.S   |  5 +++++
 arch/x86/include/asm/acrn.h | 11 +++++++++++
 arch/x86/kernel/cpu/acrn.c  | 22 ++++++++++++++++++++++
 4 files changed, 39 insertions(+)
 create mode 100644 arch/x86/include/asm/acrn.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 8dc4200..d7a10f6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -848,6 +848,7 @@ config JAILHOUSE_GUEST
 config ACRN_GUEST
 	bool "ACRN Guest support"
 	depends on X86_64
+	select X86_HV_CALLBACK_VECTOR
 	help
 	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
 	  this will allow the kernel to boot in virtualized environment under
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1f0efdb..d1b8ad3 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1129,6 +1129,11 @@ apicinterrupt3 HYPERV_STIMER0_VECTOR \
 	hv_stimer0_callback_vector hv_stimer0_vector_handler
 #endif /* CONFIG_HYPERV */
 
+#if IS_ENABLED(CONFIG_ACRN_GUEST)
+apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
+	acrn_hv_callback_vector acrn_hv_vector_handler
+#endif
+
 idtentry debug			do_debug		has_error_code=0	paranoid=1 shift_ist=DEBUG_STACK
 idtentry int3			do_int3			has_error_code=0
 idtentry stack_segment		do_stack_segment	has_error_code=1
diff --git a/arch/x86/include/asm/acrn.h b/arch/x86/include/asm/acrn.h
new file mode 100644
index 0000000..43ab032
--- /dev/null
+++ b/arch/x86/include/asm/acrn.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_ACRN_H
+#define _ASM_X86_ACRN_H
+
+void acrn_hv_callback_vector(void);
+#ifdef CONFIG_TRACING
+#define trace_acrn_hv_callback_vector acrn_hv_callback_vector
+#endif
+
+void acrn_hv_vector_handler(struct pt_regs *regs);
+#endif /* _ASM_X86_ACRN_H */
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
index f556640..d8072bf 100644
--- a/arch/x86/kernel/cpu/acrn.c
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -9,7 +9,11 @@
  *
  */
 
+#include <linux/interrupt.h>
+#include <asm/acrn.h>
+#include <asm/desc.h>
 #include <asm/hypervisor.h>
+#include <asm/irq_regs.h>
 
 static uint32_t __init acrn_detect(void)
 {
@@ -18,6 +22,8 @@ static uint32_t __init acrn_detect(void)
 
 static void __init acrn_init_platform(void)
 {
+	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
+			acrn_hv_callback_vector);
 }
 
 static bool acrn_x2apic_available(void)
@@ -30,6 +36,22 @@ static bool acrn_x2apic_available(void)
 	return false;
 }
 
+static void (*acrn_intr_handler)(void);
+
+__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	entering_ack_irq();
+	inc_irq_stat(irq_hv_callback_count);
+
+	if (acrn_intr_handler)
+		acrn_intr_handler();
+
+	exiting_irq();
+	set_irq_regs(old_regs);
+}
+
 const __initconst struct hypervisor_x86 x86_hyper_acrn = {
 	.name                   = "ACRN",
 	.detect                 = acrn_detect,
-- 
2.7.4

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

[Zhao Yakui]

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@intel.com>
Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
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.
+	 */
+	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));
+
+	return result;
+}
+
+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));
+
+	return result;
+}
+
+#else
+
+static inline long acrn_hypercall0(unsigned long hcall_id)
+{
+	return -ENOTSUPP;
+}
+
+static inline long acrn_hypercall1(unsigned long hcall_id,
+				   unsigned long param1)
+{
+	return -ENOTSUPP;
+}
+
+static inline long acrn_hypercall2(unsigned long hcall_id,
+				   unsigned long param1,
+				   unsigned long param2)
+{
+	return -ENOTSUPP;
+}
+#endif /* CONFIG_ACRN_GUEST */
+#endif /* _ASM_X86_ACRN_HYPERCALL_H */
-- 
2.7.4

Re: [RFC PATCH v5 0/4] x86: Add the support of ACRN guest under x86

[Thomas Gleixner]

On Wed, 24 Apr 2019, Zhao Yakui wrote:

> ACRN is a flexible, lightweight reference hypervisor, built with real-time
> and safety-criticality in mind, optimized to streamline embedded development
> through an open source platform. It is built for embedded IOT with small
> footprint and real-time features. More details can be found
> in https://projectacrn.org/
> 
> This is the patch set that allows the Linux to work on ACRN hypervisor and it can
> work with the following patch set to manage the Linux guest on ACRN hypervisor. It
> includes the detection of ACRN hypervisor, upcall notification vector from
> hypervisor, hypercall. The hypervisor detection is similar to Xen/VMWARE/Hyperv.
> ACRN also uses the upcall notification mechanism similar to that in Xen/Microsoft
> HyperV when it needs to send the notification to Linux OS. The hypercall provides
> the mechanism that can be used to query/configure the ACRN hypervisor by Linux guest.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [RFC PATCH v5 0/4] x86: Add the support of ACRN guest under x86

[Zhao, Yakui]

On 2019年04月25日 06:20, Thomas Gleixner wrote:
> On Wed, 24 Apr 2019, Zhao Yakui wrote:
> 
>> ACRN is a flexible, lightweight reference hypervisor, built with real-time
>> and safety-criticality in mind, optimized to streamline embedded development
>> through an open source platform. It is built for embedded IOT with small
>> footprint and real-time features. More details can be found
>> in https://projectacrn.org/
>>
>> This is the patch set that allows the Linux to work on ACRN hypervisor and it can
>> work with the following patch set to manage the Linux guest on ACRN hypervisor. It
>> includes the detection of ACRN hypervisor, upcall notification vector from
>> hypervisor, hypercall. The hypervisor detection is similar to Xen/VMWARE/Hyperv.
>> ACRN also uses the upcall notification mechanism similar to that in Xen/Microsoft
>> HyperV when it needs to send the notification to Linux OS. The hypercall provides
>> the mechanism that can be used to query/configure the ACRN hypervisor by Linux guest.
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks for the review.
I will remove the RFC in the next version.

Thanks
    Yakui

> 

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

[Ingo Molnar]

* Zhao Yakui <yakui.zhao@intel.com> 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@intel.com>
> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> ---
> 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.

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?

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

Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Ingo Molnar]

* Zhao Yakui <yakui.zhao@intel.com> wrote:

> Linux kernel uses the HYPERVISOR_CALLBACK_VECTOR for hypervisor upcall
> vector. And it is already used for Xen and HyperV.

English sentences should not be started with 'and'.

> After ACRN hypervisor is detected, it will also use this defined vector
> to notify ACRN guest.

Missing 'the', twice.

> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_ACRN_H
> +#define _ASM_X86_ACRN_H
> +
> +void acrn_hv_callback_vector(void);

Please mark these with 'extern', as customary in x86 headers.

>  
> +#include <linux/interrupt.h>
> +#include <asm/acrn.h>
> +#include <asm/desc.h>
>  #include <asm/hypervisor.h>
> +#include <asm/irq_regs.h>
>  
>  static uint32_t __init acrn_detect(void)
>  {
> @@ -18,6 +22,8 @@ static uint32_t __init acrn_detect(void)
>  
>  static void __init acrn_init_platform(void)
>  {
> +	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
> +			acrn_hv_callback_vector);

Why is this on two lines, not a single line?

> +static void (*acrn_intr_handler)(void);
> +
> +__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +
> +	entering_ack_irq();

Does the hypervisor model the APIC EOI command, i.e. does it require the 
APIC to be acked? I.e. would not acking the APIC create an IRQ storm?

> +	inc_irq_stat(irq_hv_callback_count);
> +
> +	if (acrn_intr_handler)
> +		acrn_intr_handler();

Nothing appears to be setting acrn_intr_handler, so this will never 
execute anything? Is more code relying on this?

Thanks,

	Ingo

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@intel.com> 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@intel.com>
>> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
>> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
>> ---
>> 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
> 

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

[Borislav Petkov]

On Thu, Apr 25, 2019 at 06:16:02PM +0800, Zhao, Yakui wrote:
> 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.

If you're going to use the constraint "D" for param1, you can just as
well do

	"=a" (result)

everywhere since you have the letter constraint for %rax instead of
declaring it with "register".

Also, you can completely get rid of those "register" declarations
and let gcc have all the freedom to pass in hcall_id and the other
parameters:

	unsigned long result;

        asm volatile("mov %[hcall_id], %%r8\n\t"
                     "vmcall\n\t"
                     : "=a" (result)
                     : [hcall_id] "g" (hcall_id)
                     : "r8");

        return result;

and %r8 will be in the clobber list so gcc will reload it if needed.

gcc turns it into

0000000000001040 <main>:
    1040:       4c 8b 05 e1 2f 00 00    mov    0x2fe1(%rip),%r8        # 4028 <hcall_id>
    1047:       0f 01 c1                vmcall
    104a:       c3                      retq 
    104b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)

here.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Zhao, Yakui]

On 2019年04月25日 15:17, Ingo Molnar wrote:
> 
> * Zhao Yakui <yakui.zhao@intel.com> wrote:
> 
>> Linux kernel uses the HYPERVISOR_CALLBACK_VECTOR for hypervisor upcall
>> vector. And it is already used for Xen and HyperV.
> 
> English sentences should not be started with 'and'.

OK. I will remove it.

> 
>> After ACRN hypervisor is detected, it will also use this defined vector
>> to notify ACRN guest.
> 
> Missing 'the', twice.

OK. It will be added.

> 
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_ACRN_H
>> +#define _ASM_X86_ACRN_H
>> +
>> +void acrn_hv_callback_vector(void);
> 
> Please mark these with 'extern', as customary in x86 headers.

OK. The "extern" will be added.

> 
>>   
>> +#include <linux/interrupt.h>
>> +#include <asm/acrn.h>
>> +#include <asm/desc.h>
>>   #include <asm/hypervisor.h>
>> +#include <asm/irq_regs.h>
>>   
>>   static uint32_t __init acrn_detect(void)
>>   {
>> @@ -18,6 +22,8 @@ static uint32_t __init acrn_detect(void)
>>   
>>   static void __init acrn_init_platform(void)
>>   {
>> +	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
>> +			acrn_hv_callback_vector);
> 
> Why is this on two lines, not a single line?

At first it used the long function name for acrn_hv_callback_vector.
As it exceeds 80 columns, it is split into two lines.
I will check it and see whether it can be fit into one single line.
If it is ok, it will be in one single line.

> 
>> +static void (*acrn_intr_handler)(void);
>> +
>> +__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs *regs)
>> +{
>> +	struct pt_regs *old_regs = set_irq_regs(regs);
>> +
>> +	entering_ack_irq();
> 
> Does the hypervisor model the APIC EOI command, i.e. does it require the
> APIC to be acked? I.e. would not acking the APIC create an IRQ storm?

The hypervisor requires that the APIC EOI should be acked. If the EOI 
APIC is not acked, the APIC ISR bit for the HYPERVISOR_CALLBACK_VECTOR 
will not be cleared and then it will block the interrupt whose vector is 
lower than HYPERVISOR_CALLBACK_VECTOR.

> 
>> +	inc_irq_stat(irq_hv_callback_count);
>> +
>> +	if (acrn_intr_handler)
>> +		acrn_intr_handler();
> 
> Nothing appears to be setting acrn_intr_handler, so this will never
> execute anything? Is more code relying on this?

Nothing will be done in this patch.
Later the driver part code will be added and it needs to add/remove the 
corresponding driver handler. It is similar to vmbus_handler in 
hyperv_vector_handler.

> 
> Thanks,
> 
> 	Ingo
> 

Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Ingo Molnar]

* Zhao, Yakui <yakui.zhao@intel.com> wrote:

> > > +	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
> > > +			acrn_hv_callback_vector);
> > 
> > Why is this on two lines, not a single line?
> 
> At first it used the long function name for acrn_hv_callback_vector.
> As it exceeds 80 columns, it is split into two lines.

No, it doesn't exceed 80 columns - the last character of that line is on 
column 71.

> > Does the hypervisor model the APIC EOI command, i.e. does it require the
> > APIC to be acked? I.e. would not acking the APIC create an IRQ storm?
> 
> The hypervisor requires that the APIC EOI should be acked. If the EOI APIC
> is not acked, the APIC ISR bit for the HYPERVISOR_CALLBACK_VECTOR will not
> be cleared and then it will block the interrupt whose vector is lower than
> HYPERVISOR_CALLBACK_VECTOR.

Ok!

Thanks,

	Ingo

Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Zhao, Yakui]

On 2019年04月26日 03:45, Ingo Molnar wrote:
> 
> * Zhao, Yakui <yakui.zhao@intel.com> wrote:
> 
>>>> +	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
>>>> +			acrn_hv_callback_vector);
>>>
>>> Why is this on two lines, not a single line?
>>
>> At first it used the long function name for acrn_hv_callback_vector.
>> As it exceeds 80 columns, it is split into two lines.
> 
> No, it doesn't exceed 80 columns - the last character of that line is on
> column 71.

Thanks for the helps.
It will be fixed.
> 
>>> Does the hypervisor model the APIC EOI command, i.e. does it require the
>>> APIC to be acked? I.e. would not acking the APIC create an IRQ storm?
>>
>> The hypervisor requires that the APIC EOI should be acked. If the EOI APIC
>> is not acked, the APIC ISR bit for the HYPERVISOR_CALLBACK_VECTOR will not
>> be cleared and then it will block the interrupt whose vector is lower than
>> HYPERVISOR_CALLBACK_VECTOR.
> 
> Ok!
> 

I will add some comments for calling entering_ack_irq in 
acrn_hv_callback_handler. Is this ok to you?

> Thanks,
> 
> 	Ingo
> 

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

[Zhao, Yakui]

On 2019年04月25日 19:00, Borislav Petkov wrote:
> On Thu, Apr 25, 2019 at 06:16:02PM +0800, Zhao, Yakui wrote:
>> 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.
> 
> If you're going to use the constraint "D" for param1, you can just as
> well do
> 
> 	"=a" (result)
> 
> everywhere since you have the letter constraint for %rax instead of
> declaring it with "register".
> 
> Also, you can completely get rid of those "register" declarations
> and let gcc have all the freedom to pass in hcall_id and the other
> parameters:
Thanks Borislav for providing the code.

It seems that it is seldom used in kernel although the explicit register 
variable is supported by GCC and makes the code look simpler. And it 
seems that the explicit register variable is not suppoorted by CLAG.


So the explicit register variable will be removed. I will follow the asm 
code from Borislav. Of course one minor change is that the "movq" is 
used instead of "mov".

Is this ok?

Thanks

> 
> 	unsigned long result;
> 
>          asm volatile("mov %[hcall_id], %%r8\n\t"
>                       "vmcall\n\t"
>                       : "=a" (result)
>                       : [hcall_id] "g" (hcall_id)
>                       : "r8");
> 
>          return result;
> 
> and %r8 will be in the clobber list so gcc will reload it if needed.
> 
> gcc turns it into
> 
> 0000000000001040 <main>:
>      1040:       4c 8b 05 e1 2f 00 00    mov    0x2fe1(%rip),%r8        # 4028 <hcall_id>
>      1047:       0f 01 c1                vmcall
>      104a:       c3                      retq
>      104b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> 
> here.
> 

Re: [RFC PATCH v5 3/4] x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for ACRN guest upcall vector

[Ingo Molnar]

* Zhao, Yakui <yakui.zhao@intel.com> wrote:

> > > > Does the hypervisor model the APIC EOI command, i.e. does it require the
> > > > APIC to be acked? I.e. would not acking the APIC create an IRQ storm?
> > > 
> > > The hypervisor requires that the APIC EOI should be acked. If the EOI APIC
> > > is not acked, the APIC ISR bit for the HYPERVISOR_CALLBACK_VECTOR will not
> > > be cleared and then it will block the interrupt whose vector is lower than
> > > HYPERVISOR_CALLBACK_VECTOR.
> > 
> > Ok!
> > 
> 
> I will add some comments for calling entering_ack_irq in
> acrn_hv_callback_handler. Is this ok to you?

Yeah, thanks!

	Ingo

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

[Borislav Petkov]

On Fri, Apr 26, 2019 at 11:18:48AM +0800, Zhao, Yakui wrote:
> It seems that it is seldom used in kernel although the explicit register
> variable is supported by GCC and makes the code look simpler. And it seems
> that the explicit register variable is not suppoorted by CLAG.

The more reason not to do it this way. Also, the "register" variable
specification is not very widespread in x86 when you look at

$ git grep -E "register\s.*asm" arch/x86/

output.

> So the explicit register variable will be removed. I will follow the asm
> code from Borislav. Of course one minor change is that the "movq" is used
> instead of "mov".

Does that matter if your destination register is 64-bit?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

[Zhao, Yakui]

On 2019年04月27日 16:58, Borislav Petkov wrote:
> On Fri, Apr 26, 2019 at 11:18:48AM +0800, Zhao, Yakui wrote:
>> It seems that it is seldom used in kernel although the explicit register
>> variable is supported by GCC and makes the code look simpler. And it seems
>> that the explicit register variable is not suppoorted by CLAG.
> 
> The more reason not to do it this way. Also, the "register" variable
> specification is not very widespread in x86 when you look at
> 
> $ git grep -E "register\s.*asm" arch/x86/
> 
> output.

Yes.  The explicit register variable is not very videspread for arch/x86.
So the register variable will be removed for ACRN hypercall.

> 
>> So the explicit register variable will be removed. I will follow the asm
>> code from Borislav. Of course one minor change is that the "movq" is used
>> instead of "mov".
> 
> Does that matter if your destination register is 64-bit?

Thanks for the reminder about the access width.
It is 64-bit register. What I said is the "movq", not "movl".
(I understand that movl is incorrect for 64-bit register).


Thanks
     Yakui

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

[Borislav Petkov]

On Sun, Apr 28, 2019 at 09:56:35AM +0800, Zhao, Yakui wrote:
> Thanks for the reminder about the access width.
> It is 64-bit register. What I said is the "movq", not "movl".
> (I understand that movl is incorrect for 64-bit register).

I didn't say anything about movl. I think what you're trying to say is
that because your inputs like hcall_id and param1/2 are unsigned longs,
you want a 64-bit move.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

[Zhao, Yakui]

On 2019年04月28日 18:03, Borislav Petkov wrote:
> On Sun, Apr 28, 2019 at 09:56:35AM +0800, Zhao, Yakui wrote:
>> Thanks for the reminder about the access width.
>> It is 64-bit register. What I said is the "movq", not "movl".
>> (I understand that movl is incorrect for 64-bit register).
> 
> I didn't say anything about movl. I think what you're trying to say is
> that because your inputs like hcall_id and param1/2 are unsigned longs,
> you want a 64-bit move.

Yes. "movq" only indicates explicitly that it is 64-bit mov as ACRN 
guest only works under 64-bit mode.
I also check the usage of "mov" and "movq" in this scenario. There is no 
difference except that the movq is an explicit 64-op.
Of course "mov" is also ok to me that if you prefer the "mov".

Thanks
   Yakui
> 

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

[Borislav Petkov]

On Mon, Apr 29, 2019 at 09:24:12AM +0800, Zhao, Yakui wrote:
> Yes. "movq" only indicates explicitly that it is 64-bit mov as ACRN guest
> only works under 64-bit mode.
> I also check the usage of "mov" and "movq" in this scenario. There is no
> difference except that the movq is an explicit 64-op.

Damn, I'm tired of explaining this: it is explicit only to the code
reader. gcc generates the *same* instruction no matter whether it has a
"q" suffix or not as long as the destination register is a 64-bit one.

If you prefer to have it explicit, sure, use "movq".

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

[Zhao, Yakui]

On 2019年04月29日 15:36, Borislav Petkov wrote:
> On Mon, Apr 29, 2019 at 09:24:12AM +0800, Zhao, Yakui wrote:
>> Yes. "movq" only indicates explicitly that it is 64-bit mov as ACRN guest
>> only works under 64-bit mode.
>> I also check the usage of "mov" and "movq" in this scenario. There is no
>> difference except that the movq is an explicit 64-op.
> 
> Damn, I'm tired of explaining this: it is explicit only to the code
> reader. gcc generates the *same* instruction no matter whether it has a
> "q" suffix or not as long as the destination register is a 64-bit one.
> 
> If you prefer to have it explicit, sure, use "movq".

Hi, Borislav

     Thanks for the detailed explanation.
     "movq" will be used so that it is explicit to the code reader 
although the same binary is generated for "movq" and "mov" in this scenario.

     And thank you again for giving a lot of helps about removing the 
usage of explicit register variable.

Best regards
    Yakui
>