[RFC PATCH v2 0/3] arch/x86: Add the support of ACRN guest under arch/x86

[RFC PATCH v2 0/3] arch/x86: Add the support of ACRN guest under arch/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

Zhao Yakui (3):
  arch/x86: add the support of ACRN guest
  arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector
  arch/x86/acrn: add hypercall for acrn_guest

 arch/x86/Kconfig                      |  8 ++++
 arch/x86/entry/entry_64.S             |  5 +++
 arch/x86/include/asm/acrn_hypercall.h | 84 +++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/acrnhyper.h      | 19 ++++++++
 arch/x86/include/asm/hardirq.h        |  3 +-
 arch/x86/include/asm/hypervisor.h     |  1 +
 arch/x86/kernel/cpu/Makefile          |  1 +
 arch/x86/kernel/cpu/acrn.c            | 57 ++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c      |  4 ++
 arch/x86/kernel/irq.c                 |  3 +-
 10 files changed, 183 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/acrn_hypercall.h
 create mode 100644 arch/x86/include/asm/acrnhyper.h
 create mode 100644 arch/x86/kernel/cpu/acrn.c

-- 
2.7.4

[RFC PATCH v2 1/3] arch/x86: add the support of ACRN guest

[Zhao Yakui]

ACRN is one open-source hypervisour, which is maintained by Linux
foundation. This is to add the para-virtualization support so that
it allows the Linux guest to run on acrn-hypervisor.

This adds x86_hyper_acrn into supported hypervisors array, which enables
Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.

v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
understand.
        Remove the export of x86_hyper_acrn.

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>
---
 arch/x86/Kconfig                  |  8 ++++++++
 arch/x86/include/asm/hypervisor.h |  1 +
 arch/x86/kernel/cpu/Makefile      |  1 +
 arch/x86/kernel/cpu/acrn.c        | 35 +++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
 5 files changed, 49 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/acrn.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3c..d73225e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -842,6 +842,14 @@ 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 && PARAVIRT
+	help
+	  This option allows to run Linux as guest in ACRN hypervisor. Enabling
+	  this will allow the kernel to boot in a paravirtualized environment
+	  under the ACRN hypervisor.
+
 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..3956567
--- /dev/null
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -0,0 +1,35 @@
+// 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)
+{
+	/* do not support x2apic */
+	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 v2 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn 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 kernel.

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.

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>
---
 arch/x86/entry/entry_64.S        |  5 +++++
 arch/x86/include/asm/acrnhyper.h | 19 +++++++++++++++++++
 arch/x86/include/asm/hardirq.h   |  3 ++-
 arch/x86/kernel/cpu/acrn.c       | 22 ++++++++++++++++++++++
 arch/x86/kernel/irq.c            |  3 ++-
 5 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/include/asm/acrnhyper.h

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/acrnhyper.h b/arch/x86/include/asm/acrnhyper.h
new file mode 100644
index 0000000..9f9c239
--- /dev/null
+++ b/arch/x86/include/asm/acrnhyper.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_ACRNHYPER_H
+#define _ASM_X86_ACRNHYPER_H
+
+#include <linux/types.h>
+#include <linux/atomic.h>
+#include <linux/nmi.h>
+#include <asm/io.h>
+
+#ifdef CONFIG_ACRN_GUEST
+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
+
+#endif
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index d9069bb..a8f4d08 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -37,7 +37,8 @@ typedef struct {
 #ifdef CONFIG_X86_MCE_AMD
 	unsigned int irq_deferred_error_count;
 #endif
-#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
+#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
+		defined(CONFIG_ACRN_GUEST)
 	unsigned int irq_hv_callback_count;
 #endif
 #if IS_ENABLED(CONFIG_HYPERV)
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
index 3956567..7a233b5 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/hypervisor.h>
+#include <asm/acrnhyper.h>
+#include <asm/desc.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)
@@ -26,6 +32,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,
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 59b5f2e..02c6ff6 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -134,7 +134,8 @@ 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)
+#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
+		defined(CONFIG_ACRN_GUEST)
 	if (test_bit(HYPERVISOR_CALLBACK_VECTOR, system_vectors)) {
 		seq_printf(p, "%*s: ", prec, "HYP");
 		for_each_online_cpu(j)
-- 
2.7.4

[RFC PATCH v2 3/3] arch/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 Guest system.

So the hypercall is added so that the kernel can communicate with the
low-level acrn-hypervisor. On x86 it is implemented by using vmacll when
the acrn hypervisor is enabled.

V1->V2: Refine the comments for the function of acrn_hypercall0/1/2

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>
---
 arch/x86/include/asm/acrn_hypercall.h | 84 +++++++++++++++++++++++++++++++++++
 1 file changed, 84 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..3262935
--- /dev/null
+++ b/arch/x86/include/asm/acrn_hypercall.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * acrn_hypercall.h : acrn hypervisor call API
+ */
+
+#ifndef __ACRN_HYPERCALL_H__
+#define __ACRN_HYPERCALL_H__
+
+#include <linux/errno.h>
+
+#ifdef CONFIG_ACRN_GUEST
+
+/*
+ * Hypercalls for ACRN Guest
+ *
+ * Hypercall number is passed in r8 register.
+ * Return value will be placed in rax.
+ * Up to 2 arguments are passed in rdi, rsi.
+ */
+
+static inline long acrn_hypercall0(unsigned long hcall_id)
+{
+
+	register long result asm("rax");
+	register unsigned long r8 asm("r8") = hcall_id;
+
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=r"(result)
+			:  "r"(r8));
+
+	return result;
+}
+
+static inline long acrn_hypercall1(unsigned long hcall_id,
+				   unsigned long param1)
+{
+
+	register long result asm("rax");
+	register unsigned long r8 asm("r8") = hcall_id;
+
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=r"(result)
+			: "D"(param1), "r"(r8));
+
+	return result;
+}
+
+static inline long acrn_hypercall2(unsigned long hcall_id,
+				   unsigned long param1,
+				   unsigned long param2)
+{
+
+	register long result asm("rax");
+	register unsigned long r8 asm("r8") = hcall_id;
+
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=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
+
+#endif /* __ACRN_HYPERCALL_H__ */
-- 
2.7.4

Re: [RFC PATCH v2 1/3] arch/x86: add the support of ACRN guest

[Thomas Gleixner]

Zhao,

On Tue, 26 Mar 2019, Zhao Yakui wrote:

Vs. the Subject line: arch/x86: add the support of ACRN guest

The proper prefix for x86 is surprisingly 'x86:' not 'arch/x86:'. Also
please start the first word after the colon with an upper case letter.

> ACRN is one open-source hypervisour, which is maintained by Linux

s/one/an/

> foundation.

by the Linuxfoundation.

> This is to add the para-virtualization support so that
> it allows the Linux guest to run on acrn-hypervisor.
> 
> This adds x86_hyper_acrn into supported hypervisors array, which enables
> Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.

Please do not use 'This is to add' or 'This adds'. Just say:

Add ....
 
> v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
> understand.
>         Remove the export of x86_hyper_acrn.

Thanks for having the version changes documented, but please put them after
the '---' line below and add another '---' before the diffstat. These
changes are not part of the final change log and if they are below then I
don't have to strip them manually.

> 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>
> ---
>  arch/x86/Kconfig                  |  8 ++++++++
>  arch/x86/include/asm/hypervisor.h |  1 +
>  arch/x86/kernel/cpu/Makefile      |  1 +
>  arch/x86/kernel/cpu/acrn.c        | 35 +++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
>  5 files changed, 49 insertions(+)
>  create mode 100644 arch/x86/kernel/cpu/acrn.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c1f9b3c..d73225e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -842,6 +842,14 @@ 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 && PARAVIRT

Why does this select PARAVIRT? The current patches are not implementing
anything of the paravirt functionality. Which part of paravirtualization
are you going to provide?

Thanks,

	tglx

Re: [RFC PATCH v2 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector

[Thomas Gleixner]

On Tue, 26 Mar 2019, Zhao Yakui wrote:
> diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
> index d9069bb..a8f4d08 100644
> --- a/arch/x86/include/asm/hardirq.h
> +++ b/arch/x86/include/asm/hardirq.h
> @@ -37,7 +37,8 @@ typedef struct {
>  #ifdef CONFIG_X86_MCE_AMD
>  	unsigned int irq_deferred_error_count;
>  #endif
> -#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
> +#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
> +		defined(CONFIG_ACRN_GUEST)

..

> @@ -134,7 +134,8 @@ 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)
> +#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
> +		defined(CONFIG_ACRN_GUEST)
>  	if (test_bit(HYPERVISOR_CALLBACK_VECTOR, system_vectors)) {
>  		seq_printf(p, "%*s: ", prec, "HYP");
>  		for_each_online_cpu(j)

This starts to become unreadable. Please create a new config symbol:

     config X86_HV_CALLBACK_VECTOR
     	def_bool

and select if from HYPERV, XEN and your new thing. That wants to be a
separate preparatory patch at the beginning of the series please.

Thanks,

	tglx

Re: [RFC PATCH v2 3/3] arch/x86/acrn: add hypercall for acrn_guest

[Thomas Gleixner]

On Tue, 26 Mar 2019, Zhao Yakui 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 Guest system.
> 
> So the hypercall is added so that the kernel can communicate with the
> low-level acrn-hypervisor. On x86 it is implemented by using vmacll when

is implemented with the VMCALL instruction

> the acrn hypervisor is enabled.
> 
> +++ b/arch/x86/include/asm/acrn_hypercall.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * acrn_hypercall.h : acrn hypervisor call API

No file names in headers please. They are pointless and get out of sync
when files are renamed.

> + */
> +
> +#ifndef __ACRN_HYPERCALL_H__
> +#define __ACRN_HYPERCALL_H__

asm headers start with

    __ASM_X86_....

> +
> +#include <linux/errno.h>
> +
> +#ifdef CONFIG_ACRN_GUEST
> +
> +/*
> + * Hypercalls for ACRN Guest
> + *
> + * Hypercall number is passed in r8 register.
> + * Return value will be placed in rax.
> + * Up to 2 arguments are passed in rdi, rsi.
> + */
> +
> +static inline long acrn_hypercall0(unsigned long hcall_id)
> +{
> +

Remove the empty new line please.

> +	register long result asm("rax");
> +	register unsigned long r8 asm("r8") = hcall_id;

Please order them the other way around, like a reverse christmas tree:

	register unsigned long r8 asm("r8") = hcall_id;
	register long result asm("rax");

That's way simpler to read.

> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
> +			: "=r"(result)
> +			:  "r"(r8));

Please mention in the changelog why this is implemented with bytes and not
with the proper mnemonic. I assume it depends on binutils, so please
document which version of binutils supports the mnemonic.

And in the first function, i.e. hypercall0, add a comment above the asm
volatile() to that effect as well. 

Thanks,

	tglx

Re: [RFC PATCH v2 3/3] arch/x86/acrn: add hypercall for acrn_guest

[Zhao, Yakui]

On 2019年04月06日 03:19, Thomas Gleixner wrote:
> On Tue, 26 Mar 2019, Zhao Yakui 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 Guest system.
>>
>> So the hypercall is added so that the kernel can communicate with the
>> low-level acrn-hypervisor. On x86 it is implemented by using vmacll when
> 
> is implemented with the VMCALL instruction
> 
>> the acrn hypervisor is enabled.
>>
>> +++ b/arch/x86/include/asm/acrn_hypercall.h
>> @@ -0,0 +1,84 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * acrn_hypercall.h : acrn hypervisor call API
> 
> No file names in headers please. They are pointless and get out of sync
> when files are renamed.

Sure. It will be removed in next vesion.

> 
>> + */
>> +
>> +#ifndef __ACRN_HYPERCALL_H__
>> +#define __ACRN_HYPERCALL_H__
> 
> asm headers start with
> 
>      __ASM_X86_....
> 
This will be fixed.

>> +
>> +#include <linux/errno.h>
>> +
>> +#ifdef CONFIG_ACRN_GUEST
>> +
>> +/*
>> + * Hypercalls for ACRN Guest
>> + *
>> + * Hypercall number is passed in r8 register.
>> + * Return value will be placed in rax.
>> + * Up to 2 arguments are passed in rdi, rsi.
>> + */
>> +
>> +static inline long acrn_hypercall0(unsigned long hcall_id)
>> +{
>> +
> 
> Remove the empty new line please.
> 
>> +	register long result asm("rax");
>> +	register unsigned long r8 asm("r8") = hcall_id;
> 
> Please order them the other way around, like a reverse christmas tree:
> 
> 	register unsigned long r8 asm("r8") = hcall_id;
> 	register long result asm("rax");
> 
> That's way simpler to read.

This will be fixed.

> 
>> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
>> +			: "=r"(result)
>> +			:  "r"(r8));
> 
> Please mention in the changelog why this is implemented with bytes and not
> with the proper mnemonic. I assume it depends on binutils, so please
> document which version of binutils supports the mnemonic.

I check the binutils version mentioned in Document/changes.
(binutils, 2.20)
It seems that the "vmcall" is already supported.
So the "vmcall" mnemonic will be used to make it readable.

> 
> And in the first function, i.e. hypercall0, add a comment above the asm
> volatile() to that effect as well.

Sure.


> 
> Thanks,
> 
> 	tglx
> 

Re: [RFC PATCH v2 2/3] arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector

[Zhao, Yakui]

On 2019年04月06日 03:07, Thomas Gleixner wrote:
> On Tue, 26 Mar 2019, Zhao Yakui wrote:
>> diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
>> index d9069bb..a8f4d08 100644
>> --- a/arch/x86/include/asm/hardirq.h
>> +++ b/arch/x86/include/asm/hardirq.h
>> @@ -37,7 +37,8 @@ typedef struct {
>>   #ifdef CONFIG_X86_MCE_AMD
>>   	unsigned int irq_deferred_error_count;
>>   #endif
>> -#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN)
>> +#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
>> +		defined(CONFIG_ACRN_GUEST)
> 
> ..
> 
>> @@ -134,7 +134,8 @@ 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)
>> +#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) ||\
>> +		defined(CONFIG_ACRN_GUEST)
>>   	if (test_bit(HYPERVISOR_CALLBACK_VECTOR, system_vectors)) {
>>   		seq_printf(p, "%*s: ", prec, "HYP");
>>   		for_each_online_cpu(j)
> 
> This starts to become unreadable. Please create a new config symbol:
> 
>       config X86_HV_CALLBACK_VECTOR
>       	def_bool
> 
> and select if from HYPERV, XEN and your new thing. That wants to be a
> separate preparatory patch at the beginning of the series please.

Thanks for the suggestion.
Sure. One new config will be added in next version.


> 
> Thanks,
> 
> 	tglx
> 

Re: [RFC PATCH v2 1/3] arch/x86: add the support of ACRN guest

[Zhao, Yakui]

On 2019年04月06日 03:01, Thomas Gleixner wrote:
> Zhao,
> 
> On Tue, 26 Mar 2019, Zhao Yakui wrote:
> 
> Vs. the Subject line: arch/x86: add the support of ACRN guest
> 
> The proper prefix for x86 is surprisingly 'x86:' not 'arch/x86:'. Also
> please start the first word after the colon with an upper case letter.
> 
>> ACRN is one open-source hypervisour, which is maintained by Linux
> 
> s/one/an/
> 
>> foundation.
> 
> by the Linuxfoundation.
> 
>> This is to add the para-virtualization support so that
>> it allows the Linux guest to run on acrn-hypervisor.
>>
>> This adds x86_hyper_acrn into supported hypervisors array, which enables
>> Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.
> 
> Please do not use 'This is to add' or 'This adds'. Just say:
> 
> Add ....
>   
>> v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
>> understand.
>>          Remove the export of x86_hyper_acrn.
> 
> Thanks for having the version changes documented, but please put them after
> the '---' line below and add another '---' before the diffstat. These
> changes are not part of the final change log and if they are below then I
> don't have to strip them manually.
> 

Sure.
It will be updated.

>> 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>
>> ---
>>   arch/x86/Kconfig                  |  8 ++++++++
>>   arch/x86/include/asm/hypervisor.h |  1 +
>>   arch/x86/kernel/cpu/Makefile      |  1 +
>>   arch/x86/kernel/cpu/acrn.c        | 35 +++++++++++++++++++++++++++++++++++
>>   arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
>>   5 files changed, 49 insertions(+)
>>   create mode 100644 arch/x86/kernel/cpu/acrn.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index c1f9b3c..d73225e 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -842,6 +842,14 @@ 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 && PARAVIRT
> 
> Why does this select PARAVIRT? The current patches are not implementing
> anything of the paravirt functionality. Which part of paravirtualization
> are you going to provide?

Thanks for your nice and careful review.
Yes. The CONFIG_PARAVIRT is not required.
It will be removed.

> 
> Thanks,
> 
> 	tglx
>