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

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

Zhao Yakui (3):
  arch/x86: add ACRN hypervisor guest
  arch/x86/acrn: Use HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector
  arch/x86/acrn: add hypercall for acrn_hypervisor

 arch/x86/Kconfig                      |  8 ++++
 arch/x86/entry/entry_64.S             |  5 +++
 arch/x86/include/asm/acrn_hypercall.h | 85 +++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/acrnhyper.h      | 18 ++++++++
 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            | 75 +++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c      |  4 ++
 arch/x86/kernel/irq.c                 |  2 +-
 10 files changed, 199 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 1/3] arch/x86: add ACRN hypervisor 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 to enable the Linux guest on acrn-hypervisor.

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

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        | 36 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
 5 files changed, 50 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/acrn.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 90b562a..c7b64e7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -843,6 +843,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
+	bool "Enable services on ACRN hypervisor"
+	depends on X86_64 && PARAVIRT
+	help
+	  This option allows to run Linux as guest in ACRN hypervisor.
+	  It is needed if you want to run ACRN services linux on top of
+	  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..cde1436 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)			+= 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..ddeaafb
--- /dev/null
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACRN hypervisor 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 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,
+};
+EXPORT_SYMBOL(x86_hyper_acrn);
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 479ca47..5a6f072 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
+	&x86_hyper_acrn,
+#endif
 };
 
 enum x86_hypervisor_type x86_hyper_type;
-- 
2.7.4

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

[Zhao Yakui]

WHen it works in hypervisor guest mode, 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 as notify vector to kernel.
And two APIs are added so that the other module can add/remove the
hypervisor callback irq handler.

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 | 16 ++++++++++++++++
 arch/x86/include/asm/hardirq.h   |  2 +-
 arch/x86/kernel/cpu/acrn.c       | 38 ++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/irq.c            |  2 +-
 5 files changed, 61 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..23f12e8 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)
+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..2562a82
--- /dev/null
+++ b/arch/x86/include/asm/acrnhyper.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_ACRNHYPER_H
+#define _ASM_X86_ACRNHYPER_H
+
+#include <linux/types.h>
+#include <asm/io.h>
+
+#ifdef CONFIG_ACRN
+/* ACRN Hypervisor callback */
+void acrn_hv_callback_vector(void);
+
+void acrn_setup_intr_irq(void (*handler)(void));
+void acrn_remove_intr_irq(void);
+#endif
+
+#endif
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index d9069bb..5aa52d2 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)
+#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) || defined(CONFIG_ACRN)
 	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 ddeaafb..c802fe6 100644
--- a/arch/x86/kernel/cpu/acrn.c
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -10,6 +10,11 @@
  */
 
 #include <asm/hypervisor.h>
+#include <asm/acrnhyper.h>
+#include <asm/irq_vectors.h>
+#include <asm/irq_regs.h>
+#include <asm/desc.h>
+#include <linux/interrupt.h>
 
 static uint32_t __init acrn_detect(void)
 {
@@ -18,6 +23,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 +33,37 @@ static bool acrn_x2apic_available(void)
 	return false;
 }
 
+
+static void (*acrn_intr_handler)(void);
+/*
+ * Handler for ACRN_HV_CALLBACK.
+ */
+__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);
+}
+
+void acrn_setup_intr_irq(void (*handler)(void))
+{
+	acrn_intr_handler = handler;
+}
+EXPORT_SYMBOL(acrn_setup_intr_irq);
+
+void acrn_remove_intr_irq(void)
+{
+	acrn_intr_handler = NULL;
+}
+EXPORT_SYMBOL(acrn_remove_intr_irq);
+
 const 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..6b3eaab 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)
+#if IS_ENABLED(CONFIG_HYPERV) || defined(CONFIG_XEN) || defined(CONFIG_ACRN)
 	if (test_bit(HYPERVISOR_CALLBACK_VECTOR, system_vectors)) {
 		seq_printf(p, "%*s: ", prec, "HYP");
 		for_each_online_cpu(j)
-- 
2.7.4

[RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor

[Zhao Yakui]

When acrn_hypervisor is detected, the other module may communicate with
the hypervisor to 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.

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 | 85 +++++++++++++++++++++++++++++++++++
 1 file changed, 85 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..7ffa759
--- /dev/null
+++ b/arch/x86/include/asm/acrn_hypercall.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */
+/*
+ * acrn_hypercall.h : acrn hypervisor call API
+ */
+
+#ifndef __ACRN_HYPERCALL_H__
+#define __ACRN_HYPERCALL_H__
+
+#include <linux/errno.h>
+
+#ifdef CONFIG_ACRN
+
+static inline long acrn_hypercall0(unsigned long hcall_id)
+{
+
+	/* x86-64 System V ABI register usage */
+	register signed long    result asm("rax");
+	register unsigned long  r8 asm("r8")  = hcall_id;
+
+	/* Execute vmcall */
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=r"(result)
+			:  "r"(r8));
+
+	/* Return result to caller */
+	return result;
+}
+
+static inline long acrn_hypercall1(unsigned long hcall_id,
+				   unsigned long param1)
+{
+
+	/* x86-64 System V ABI register usage */
+	register signed long    result asm("rax");
+	register unsigned long  r8 asm("r8")  = hcall_id;
+
+	/* Execute vmcall */
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=r"(result)
+			: "D"(param1), "r"(r8));
+
+	/* Return result to caller */
+	return result;
+}
+
+static inline long acrn_hypercall2(unsigned long hcall_id,
+				   unsigned long param1,
+				   unsigned long param2)
+{
+
+	/* x86-64 System V ABI register usage */
+	register signed long    result asm("rax");
+	register unsigned long  r8 asm("r8")  = hcall_id;
+
+	/* Execute vmcall */
+	asm volatile(".byte 0x0F,0x01,0xC1\n"
+			: "=r"(result)
+			: "D"(param1), "S"(param2), "r"(r8));
+
+	/* Return result to caller */
+	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 1/3] arch/x86: add ACRN hypervisor guest

[Thomas Gleixner]

Zhao,

On Thu, 7 Mar 2019, Zhao Yakui wrote:

> ACRN is one open-source hypervisour, which is maintained by Linux
> foundation. This is to add the para-virtualization support so that
> it allows to enable the Linux guest on acrn-hypervisor.
> 
> This adds x86_hyper_acrn into supported hypervisors array, which
> enables ACRN services guest running on ACRN hypervisor. It is

What is a ACRN services guest? 

> restricted to X86_64.
> 
> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>

This SOB chain is wrong. If Jason wrote the patch, then there should be a
'From: Jason ..' line at the top of the change log. If you both wrote it
then Jason's SOB wants to be preceeded by a 'Co-developed-by: Jason....'
line. See Documentation/process/

> +config ACRN
> +	bool "Enable services on ACRN hypervisor"
> +	depends on X86_64 && PARAVIRT
> +	help
> +	  This option allows to run Linux as guest in ACRN hypervisor.
> +	  It is needed if you want to run ACRN services linux on top of
> +	  ACRN hypervisor.

This help text does not make any sense to me.

> +const 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,
> +};
> +EXPORT_SYMBOL(x86_hyper_acrn);

Whay is this exported? The only user of this is hypervisor.c and that is
built in. Please remove.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 7 Mar 2019, Zhao Yakui wrote:

> WHen it works in hypervisor guest mode, 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 as notify vector to kernel.
> And two APIs are added so that the other module can add/remove the
> hypervisor callback irq handler.

Which other module?

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

Same SOB issue.

> +#if IS_ENABLED(CONFIG_ACRN)
> +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..2562a82
> --- /dev/null
> +++ b/arch/x86/include/asm/acrnhyper.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_ACRNHYPER_H
> +#define _ASM_X86_ACRNHYPER_H
> +
> +#include <linux/types.h>
> +#include <asm/io.h>
> +
> +#ifdef CONFIG_ACRN
> +/* ACRN Hypervisor callback */
> +void acrn_hv_callback_vector(void);

What declares acrn_hv_vector_handler() ?

>  #include <asm/hypervisor.h>
> +#include <asm/acrnhyper.h>
> +#include <asm/irq_vectors.h>
> +#include <asm/irq_regs.h>
> +#include <asm/desc.h>
> +#include <linux/interrupt.h>

First of all the include order is always:

 #include <linux/...>
 #include <linux/...>

 #include <asm/...>

Aside of that including 'linux/irq.h' should include everything you
need. No need for gazillion of includes.

>  static bool acrn_x2apic_available(void)
> @@ -26,6 +33,37 @@ static bool acrn_x2apic_available(void)
>  	return false;
>  }
>  
> +

Stray newline

> +static void (*acrn_intr_handler)(void);
> +/*

Which should have been above this comment. Glueing stuff together makes it
unreadable.

> + * Handler for ACRN_HV_CALLBACK.

Err? This handles the HYPERVISOR_CALLBACK_VECTOR

> + */
> +__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);
> +}
> +
> +void acrn_setup_intr_irq(void (*handler)(void))
> +{
> +	acrn_intr_handler = handler;
> +}
> +EXPORT_SYMBOL(acrn_setup_intr_irq);
> +
> +void acrn_remove_intr_irq(void)
> +{
> +	acrn_intr_handler = NULL;
> +}
> +EXPORT_SYMBOL(acrn_remove_intr_irq);

Where is the code which uses these exports? We are not adding exports just
because or for consumption by out of tree modules.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 7 Mar 2019, Zhao Yakui wrote:

> When acrn_hypervisor is detected, the other module may communicate with

Which other module?

> the hypervisor to 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.
 
> +++ b/arch/x86/include/asm/acrn_hypercall.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */

So the hypercall header is GPL-2.0+ and the acrn.c file is GPL-2.0. Not
that I personally care, but is this intended?

> +/*
> + * acrn_hypercall.h : acrn hypervisor call API
> + */
> +
> +#ifndef __ACRN_HYPERCALL_H__
> +#define __ACRN_HYPERCALL_H__
> +
> +#include <linux/errno.h>
> +
> +#ifdef CONFIG_ACRN
> +
> +static inline long acrn_hypercall0(unsigned long hcall_id)
> +{
> +
> +	/* x86-64 System V ABI register usage */

Well, yes. But can you please provide a link to the hypercall specification
in the changelog?

Also instead of repeating the same comment over and over, explain the
calling convention in a top level comment.

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

I appreciate the attempt of making this tabular, but it's unreadable and
has random white space in it. Also please use 'long' and not 'signed long'.

	register unsigned long	r8	asm("r8") = hcall_id;
	register long		result	asm("rax");
	
> +
> +	/* Execute vmcall */
> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
> +			: "=r"(result)
> +			:  "r"(r8));
> +
> +	/* Return result to caller */

Please do not comment the obvious. It's entirely clear that this returns
'result' to the caller. Comments are there to explain what's not obvious.

> +	return result;
> +}

Thanks,

	tglx

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

[Borislav Petkov]

On Fri, Mar 22, 2019 at 04:44:37PM +0100, Thomas Gleixner wrote:
> > +EXPORT_SYMBOL(acrn_remove_intr_irq);
> 
> Where is the code which uses these exports? We are not adding exports just
> because or for consumption by out of tree modules.

Right, if anything, all new exports should be _GPL.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest

[Sean Christopherson]

On Thu, Mar 07, 2019 at 10:04:09AM +0800, Zhao Yakui wrote:
> ACRN is one open-source hypervisour, which is maintained by Linux
> foundation. This is to add the para-virtualization support so that
> it allows to enable the Linux guest on acrn-hypervisor.
> 
> This adds x86_hyper_acrn into supported hypervisors array, which
> enables ACRN services guest running on ACRN hypervisor. It is
> restricted to X86_64.
> 
> 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        | 36 ++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/hypervisor.c  |  4 ++++
>  5 files changed, 50 insertions(+)
>  create mode 100644 arch/x86/kernel/cpu/acrn.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 90b562a..c7b64e7 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -843,6 +843,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

My personal preference would be to use ACRN_GUEST.  Inevitably there will
be a need to wrap ACRN code with an ifdef, and seeing "GUEST" in such
cases immediately lets readers know the code in question is related to
running as a guest without having to remember that ACRN is a hypervisor.
This is especially true for readers that have never heard of ACRN.

> +	bool "Enable services on ACRN hypervisor"
> +	depends on X86_64 && PARAVIRT
> +	help
> +	  This option allows to run Linux as guest in ACRN hypervisor.
> +	  It is needed if you want to run ACRN services linux on top of
> +	  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..cde1436 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)			+= 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..ddeaafb
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/acrn.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ACRN hypervisor 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 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,
> +};
> +EXPORT_SYMBOL(x86_hyper_acrn);
> diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
> index 479ca47..5a6f072 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
> +	&x86_hyper_acrn,
> +#endif
>  };
>  
>  enum x86_hypervisor_type x86_hyper_type;
> -- 
> 2.7.4
> 

RE: [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest

[Zhao, Yakui]

Hi,  Tglx

       Thanks for your nice review. 
>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de]
>Sent: Friday, March 22, 2019 11:20 PM
>To: Zhao, Yakui <yakui.zhao@intel.com>
>Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ
><jason.cj.chen@intel.com>
>Subject: Re: [RFC PATCH 1/3] arch/x86: add ACRN hypervisor guest
>
>Zhao,
>
>On Thu, 7 Mar 2019, Zhao Yakui wrote:
>
>> ACRN is one open-source hypervisour, which is maintained by Linux
>> foundation. This is to add the para-virtualization support so that it
>> allows to enable the Linux guest on acrn-hypervisor.
>>
>> This adds x86_hyper_acrn into supported hypervisors array, which
>> enables ACRN services guest running on ACRN hypervisor. It is
>
>What is a ACRN services guest?

It should be "ACRN guest".

>
>> restricted to X86_64.
>>
>> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
>> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
>
>This SOB chain is wrong. If Jason wrote the patch, then there should be a
>'From: Jason ..' line at the top of the change log. If you both wrote it then
>Jason's SOB wants to be preceeded by a 'Co-developed-by: Jason....'
>line. See Documentation/process/

Sure. It will be updated to "Co-developed-by: Jason" in next version.

>
>> +config ACRN
>> +	bool "Enable services on ACRN hypervisor"
>> +	depends on X86_64 && PARAVIRT
>> +	help
>> +	  This option allows to run Linux as guest in ACRN hypervisor.
>> +	  It is needed if you want to run ACRN services linux on top of
>> +	  ACRN hypervisor.
>
>This help text does not make any sense to me.

"Enable Services on ACRN hypervisor" is a little confusing. 
In next version it will be changed to "ACRN_GUEST". How about the below:
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

>
>> +const 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, };
>> +EXPORT_SYMBOL(x86_hyper_acrn);
>
>Whay is this exported? The only user of this is hypervisor.c and that is built in.
>Please remove.

You are right.
It will be removed and the x86_hyper_acrn will also be added into "__initconst" section.

>
>Thanks,
>
>	tglx

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

[Zhao, Yakui]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de]
>Sent: Friday, March 22, 2019 11:45 PM
>To: Zhao, Yakui <yakui.zhao@intel.com>
>Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ
><jason.cj.chen@intel.com>
>Subject: Re: [RFC PATCH 2/3] arch/x86/acrn: Use
>HYPERVISOR_CALLBACK_VECTOR for Acrn upcall vector
>
>On Thu, 7 Mar 2019, Zhao Yakui wrote:
>
>> WHen it works in hypervisor guest mode, 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 as notify vector to kernel.
>> And two APIs are added so that the other module can add/remove the
>> hypervisor callback irq handler.
>
>Which other module?
>

This work is divided into two steps. The first is to add the support of acrn-guest under arch/x86.
The second is to add one driver that allows the acrn-guest to communicate with the acrn-hypervisor.
As the second part has the dependency on the first part,  we hope that the acrn-guest is added firstly and the
required API is also added.

>> Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
>> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
>
>Same SOB issue.

Sure. It will be updated.

>
>> +#if IS_ENABLED(CONFIG_ACRN)
>> +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..2562a82
>> --- /dev/null
>> +++ b/arch/x86/include/asm/acrnhyper.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef _ASM_X86_ACRNHYPER_H
>> +#define _ASM_X86_ACRNHYPER_H
>> +
>> +#include <linux/types.h>
>> +#include <asm/io.h>
>> +
>> +#ifdef CONFIG_ACRN
>> +/* ACRN Hypervisor callback */
>> +void acrn_hv_callback_vector(void);
>
>What declares acrn_hv_vector_handler() ?

Acrn_hv_callback_vector is defined in arch/x86/entry/entry_64.S, which will be used as
the parameter of alloc_intr_gate

Acrn_hv_vector_handler is the real ISR handler, which is defined in acrn.c.

>
>>  #include <asm/hypervisor.h>
>> +#include <asm/acrnhyper.h>
>> +#include <asm/irq_vectors.h>
>> +#include <asm/irq_regs.h>
>> +#include <asm/desc.h>
>> +#include <linux/interrupt.h>
>
>First of all the include order is always:
>
> #include <linux/...>
> #include <linux/...>
>
> #include <asm/...>
>
>Aside of that including 'linux/irq.h' should include everything you need. No
>need for gazillion of includes.

OK. The include order will be updated in next version.

>
>>  static bool acrn_x2apic_available(void) @@ -26,6 +33,37 @@ static
>> bool acrn_x2apic_available(void)
>>  	return false;
>>  }
>>
>> +
>
>Stray newline
>

Sure. The extra newline will be removed.

>> +static void (*acrn_intr_handler)(void);
>> +/*
>
>Which should have been above this comment. Glueing stuff together makes it
>unreadable.
>
>> + * Handler for ACRN_HV_CALLBACK.
>
>Err? This handles the HYPERVISOR_CALLBACK_VECTOR
>

I will remove the confusing comment.
In fact this acrn_intr_handle is similar to the vmbus_handler in arch/x86/kernel/cpu/mshyperv.c

>> + */
>> +__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);
>> +}
>> +
>> +void acrn_setup_intr_irq(void (*handler)(void)) {
>> +	acrn_intr_handler = handler;
>> +}
>> +EXPORT_SYMBOL(acrn_setup_intr_irq);
>> +
>> +void acrn_remove_intr_irq(void)
>> +{
>> +	acrn_intr_handler = NULL;
>> +}
>> +EXPORT_SYMBOL(acrn_remove_intr_irq);
>
>Where is the code which uses these exports? We are not adding exports just
>because or for consumption by out of tree modules.

Understand it.
Is it reasonable that the above two functions are added in the driver patch set?


>
>Thanks,
>
>	tglx

RE: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor

[Zhao, Yakui]

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de]
>Sent: Saturday, March 23, 2019 12:02 AM
>To: Zhao, Yakui <yakui.zhao@intel.com>
>Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ
><jason.cj.chen@intel.com>
>Subject: Re: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor
>
>On Thu, 7 Mar 2019, Zhao Yakui wrote:
>
>> When acrn_hypervisor is detected, the other module may communicate
>> with
>
>Which other module?
>
>> the hypervisor to 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.
>
>> +++ b/arch/x86/include/asm/acrn_hypercall.h
>> @@ -0,0 +1,85 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause */
>
>So the hypercall header is GPL-2.0+ and the acrn.c file is GPL-2.0. Not that I
>personally care, but is this intended?

Sorry for my fault. It should be 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
>> +
>> +static inline long acrn_hypercall0(unsigned long hcall_id) {
>> +
>> +	/* x86-64 System V ABI register usage */
>
>Well, yes. But can you please provide a link to the hypercall specification in the
>changelog?
>
>Also instead of repeating the same comment over and over, explain the calling
>convention in a top level comment.

This is the low-level implementation of low-level hypercall. It is similar to 
Kvm_hypercall1/2/3 in arch/x86/include/asm/kvm_para.h.
Does it make sense that the below comments are added before acrn_hypercall0/1/2?
/*
 * 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.
 */   

Of course I will also remove the redundant comments.

>
>> +	register signed long    result asm("rax");
>> +	register unsigned long  r8 asm("r8")  = hcall_id;
>
>I appreciate the attempt of making this tabular, but it's unreadable and has
>random white space in it. Also please use 'long' and not 'signed long'.

Ok. The type will be refined.


>
>	register unsigned long	r8	asm("r8") = hcall_id;
>	register long		result	asm("rax");
>
>> +
>> +	/* Execute vmcall */
>> +	asm volatile(".byte 0x0F,0x01,0xC1\n"
>> +			: "=r"(result)
>> +			:  "r"(r8));
>> +
>> +	/* Return result to caller */
>
>Please do not comment the obvious. It's entirely clear that this returns 'result'
>to the caller. Comments are there to explain what's not obvious.

Thanks for pointing out the issue.
I will remove the redundant comment.

>
>> +	return result;
>> +}
>
>Thanks,
>
>	tglx

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

[Thomas Gleixner]

On Mon, 25 Mar 2019, Zhao, Yakui wrote:
> >> +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef _ASM_X86_ACRNHYPER_H
> >> +#define _ASM_X86_ACRNHYPER_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <asm/io.h>
> >> +
> >> +#ifdef CONFIG_ACRN
> >> +/* ACRN Hypervisor callback */
> >> +void acrn_hv_callback_vector(void);
> >
> >What declares acrn_hv_vector_handler() ?
> 
> Acrn_hv_callback_vector is defined in arch/x86/entry/entry_64.S, which will be used as
> the parameter of alloc_intr_gate
> 
> Acrn_hv_vector_handler is the real ISR handler, which is defined in acrn.c.

I know how that works and I was not asking where stuff is defined. I was
asking where it is declared. Global functions need a declaration in a
header file.

> >> +void acrn_remove_intr_irq(void)
> >> +{
> >> +	acrn_intr_handler = NULL;
> >> +}
> >> +EXPORT_SYMBOL(acrn_remove_intr_irq);
> >
> >Where is the code which uses these exports? We are not adding exports just
> >because or for consumption by out of tree modules.
> 
> Understand it.
> Is it reasonable that the above two functions are added in the driver patch set?

Yes, because then we see the context.

Thanks,

	tglx

RE: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor

[Thomas Gleixner]

On Mon, 25 Mar 2019, Zhao, Yakui wrote:
> 
> >-----Original Message-----
> >From: Thomas Gleixner [mailto:tglx@linutronix.de]
> >Sent: Saturday, March 23, 2019 12:02 AM
> >To: Zhao, Yakui <yakui.zhao@intel.com>
> >Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ
> ><jason.cj.chen@intel.com>
> >Subject: Re: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor


Can you please fix your mail client not to copy the full email headers on
the reply?

The simple: On $DATE, someone wrote: like the below is sufficient.

> >> +static inline long acrn_hypercall0(unsigned long hcall_id) {
> >> +
> >> +	/* x86-64 System V ABI register usage */
> >
> >Well, yes. But can you please provide a link to the hypercall specification in the
> >changelog?
> >
> >Also instead of repeating the same comment over and over, explain the calling
> >convention in a top level comment.
> 
> This is the low-level implementation of low-level hypercall. It is similar to 
> Kvm_hypercall1/2/3 in arch/x86/include/asm/kvm_para.h.
> Does it make sense that the below comments are added before acrn_hypercall0/1/2?
> /*
>  * 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.
>  */   

Yes, that's much more helpful.

Thanks,

	tglx

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

[Zhao, Yakui]

On 2019年03月25日 16:27, Thomas Gleixner wrote:
> On Mon, 25 Mar 2019, Zhao, Yakui wrote:
>>>> +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef _ASM_X86_ACRNHYPER_H
>>>> +#define _ASM_X86_ACRNHYPER_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +#include <asm/io.h>
>>>> +
>>>> +#ifdef CONFIG_ACRN
>>>> +/* ACRN Hypervisor callback */
>>>> +void acrn_hv_callback_vector(void);
>>>
>>> What declares acrn_hv_vector_handler() ?
>>
>> Acrn_hv_callback_vector is defined in arch/x86/entry/entry_64.S, which will be used as
>> the parameter of alloc_intr_gate
>>
>> Acrn_hv_vector_handler is the real ISR handler, which is defined in acrn.c.
> 
> I know how that works and I was not asking where stuff is defined. I was
> asking where it is declared. Global functions need a declaration in a
> header file.

Sure. It will be declared in next version.

> 
>>>> +void acrn_remove_intr_irq(void)
>>>> +{
>>>> +	acrn_intr_handler = NULL;
>>>> +}
>>>> +EXPORT_SYMBOL(acrn_remove_intr_irq);
>>>
>>> Where is the code which uses these exports? We are not adding exports just
>>> because or for consumption by out of tree modules.
>>
>> Understand it.
>> Is it reasonable that the above two functions are added in the driver patch set?
> 
> Yes, because then we see the context.

OK. They will be removed.  And it will be included in driver patch set.

> 
> Thanks,
> 
> 	tglx
> 

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

[Zhao, Yakui]

On 2019年03月25日 16:30, Thomas Gleixner wrote:
> On Mon, 25 Mar 2019, Zhao, Yakui wrote:
>>
>>> -----Original Message-----
>>> From: Thomas Gleixner [mailto:tglx@linutronix.de]
>>> Sent: Saturday, March 23, 2019 12:02 AM
>>> To: Zhao, Yakui <yakui.zhao@intel.com>
>>> Cc: linux-kernel@vger.kernel.org; x86@kernel.org; Chen, Jason CJ
>>> <jason.cj.chen@intel.com>
>>> Subject: Re: [RFC PATCH 3/3] arch/x86/acrn: add hypercall for acrn_hypervisor
> 
> 
> Can you please fix your mail client not to copy the full email headers on
> the reply?
> 
> The simple: On $DATE, someone wrote: like the below is sufficient.

thanks for the reminder.

> 
>>>> +static inline long acrn_hypercall0(unsigned long hcall_id) {
>>>> +
>>>> +	/* x86-64 System V ABI register usage */
>>>
>>> Well, yes. But can you please provide a link to the hypercall specification in the
>>> changelog?
>>>
>>> Also instead of repeating the same comment over and over, explain the calling
>>> convention in a top level comment.
>>
>> This is the low-level implementation of low-level hypercall. It is similar to
>> Kvm_hypercall1/2/3 in arch/x86/include/asm/kvm_para.h.
>> Does it make sense that the below comments are added before acrn_hypercall0/1/2?
>> /*
>>   * 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.
>>   */
> 
> Yes, that's much more helpful.
> 
> Thanks,
> 
> 	tglx
>