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

[RFC PATCH v3 0/4] 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

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

Zhao Yakui (4):
  x86: Add new config symbol to unify conditional definition of
    hv_irq_callback_count
  x86: Add the support of ACRN guest
  x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector
  x86: Add hypercall for acrn_guest

 arch/x86/Kconfig                      | 12 +++++
 arch/x86/entry/entry_64.S             |  5 +++
 arch/x86/include/asm/acrn_hypercall.h | 82 +++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/acrnhyper.h      | 19 ++++++++
 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            | 57 ++++++++++++++++++++++++
 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, 185 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 v3 1/4] x86: Add new config symbol to unify conditional definition of hv_irq_callback_count

[Zhao Yakui]

Now the CONFIG_HYPERV and CONFIG_XEN can be used to control the definition
/usage of hv_irq_callback_count. If another linux guest also needs to use
the hv_irq_callback_count, current conditional definition looks unreadable.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
---
 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 5ad9241..54d1fbc 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 v3 2/4] x86: Add the support of ACRN guest

[Zhao Yakui]

ACRN is an open-source hypervisor maintained by Linuxfoundation.
This is to add the Linux guest support on acrn-hypervisor.

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

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
---
 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 54d1fbc..d77d215 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -845,6 +845,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
+	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.
+
 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 v3 3/4] x86: 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 kernel.

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.
---
 arch/x86/Kconfig                 |  1 +
 arch/x86/entry/entry_64.S        |  5 +++++
 arch/x86/include/asm/acrnhyper.h | 19 +++++++++++++++++++
 arch/x86/kernel/cpu/acrn.c       | 22 ++++++++++++++++++++++
 4 files changed, 47 insertions(+)
 create mode 100644 arch/x86/include/asm/acrnhyper.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d77d215..ae4d38b 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/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/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,
-- 
2.7.4

[RFC PATCH v3 4/4] x86: 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.

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
---
 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..82c1577
--- /dev/null
+++ b/arch/x86/include/asm/acrn_hypercall.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_X86_ACRNHYPERCALL_H
+#define _ASM_X86_ACRNHYPERCALL_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 unsigned long r8 asm("r8") = hcall_id;
+	register long result asm("rax");
+
+	/* vmcall is used to implment the hypercall.
+	 * asm volatile is used 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
+
+#endif /* _ASM_X86_ACRNHYPERCALL_H */
-- 
2.7.4

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

[Borislav Petkov]

Subject: x86/kconfig: Add ...

On Mon, Apr 08, 2019 at 04:12:08PM +0800, Zhao Yakui wrote:
> Now the CONFIG_HYPERV and CONFIG_XEN can be used to control the definition
> /usage of hv_irq_callback_count. If another linux guest also needs to use
> the hv_irq_callback_count, current conditional definition looks unreadable.

Rewrite that to:

"Add a special Kconfig symbol X86_HV_CALLBACK_VECTOR which 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."

with that fixed you can add:

Reviewed-by: Borislav Petkov <bp@suse.de>

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Borislav Petkov]

On Mon, Apr 08, 2019 at 04:12:09PM +0800, Zhao Yakui wrote:
> ACRN is an open-source hypervisor maintained by Linuxfoundation.

I think tglx wanted to say "by the Linux Foundation" here.

> This is to add the Linux guest support on acrn-hypervisor.

I think you were told already:

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

Add ...."

So please take your time, work in *all* review feedback instead of
hurrying the next version out without addressing all review comments.

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

So this all talks about *what* the patch does. But that is visible from
the diff below and doesn't belong in the commit message.

Rather, your commit message should talk about *why* a change is being
done.

> 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
> ---
>  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 54d1fbc..d77d215 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -845,6 +845,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
> +	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.

WARNING: please write a paragraph that describes the config symbol fully
#47: FILE: arch/x86/Kconfig:848:
+config ACRN_GUEST

That help text above could use some of the explanation what ACRN is from
your 0/4 message.

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

Why?

This comment could explain why that choice has been made.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

[Borislav Petkov]

You can prefix your subject now like this:

x86/acrn: Use ...

On Mon, Apr 08, 2019 at 04:12:10PM +0800, Zhao Yakui wrote:
> 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.
> 
> 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.
> ---
>  arch/x86/Kconfig                 |  1 +
>  arch/x86/entry/entry_64.S        |  5 +++++
>  arch/x86/include/asm/acrnhyper.h | 19 +++++++++++++++++++
>  arch/x86/kernel/cpu/acrn.c       | 22 ++++++++++++++++++++++
>  4 files changed, 47 insertions(+)
>  create mode 100644 arch/x86/include/asm/acrnhyper.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d77d215..ae4d38b 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/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

Simply

	.../acrn.h

I'd say.

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

Why is that ifdef needed?

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

end marker:

#endif /* _ASM_X86_ACRNHYPER_H */

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

What's the sorting order here? Length or alphabetic? Or none? :)

linux/ path includes still need to come first, of course.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 4/4] x86: Add hypercall for acrn_guest

[Borislav Petkov]

On Mon, Apr 08, 2019 at 04:12:11PM +0800, 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.

Good example.

What is "Guest system" and why is capitalized? Do you mean "the guest
operating system" or simply "the guest"?

> So the hypercall is added so that the kernel can communicate with the

"So add the hypercall so that... "

> low-level acrn-hypervisor.

Is it acrn_hypervisor or acrn-hypervisor or the ACRN hypervisor or ...?

Unify the naming pls.

> On x86 it is implemented by using vmacll when

During last review Thomas said:

"is implemented with the VMCALL instruction"

You still have it wrong. Which makes me think you haven't even gone over
*all* review comments as this is the second missed review comment in a
4-patches set.

So I'm going to stop reviewing here and won't look at your patches until
you incorporate *all* review comments from all people.

-- 
Regards/Gruss,
    Boris.

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

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

[Zhao, Yakui]

On 2019年04月08日 17:35, Borislav Petkov wrote:
> Subject: x86/kconfig: Add ...
> 
> On Mon, Apr 08, 2019 at 04:12:08PM +0800, Zhao Yakui wrote:
>> Now the CONFIG_HYPERV and CONFIG_XEN can be used to control the definition
>> /usage of hv_irq_callback_count. If another linux guest also needs to use
>> the hv_irq_callback_count, current conditional definition looks unreadable.
> 
> Rewrite that to:
> 
> "Add a special Kconfig symbol X86_HV_CALLBACK_VECTOR which 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."
> 
> with that fixed you can add:
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>

Thanks for your review.
Will update the title and commit log as you suggested.

Thanks

> 
> Thx.
> 

Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

[Zhao, Yakui]

On 2019年04月08日 23:00, Borislav Petkov wrote:
> You can prefix your subject now like this:
> 
> x86/acrn: Use ...

Thanks for suggestion.
It will be updated in next version.

> 
> On Mon, Apr 08, 2019 at 04:12:10PM +0800, Zhao Yakui wrote:
>> 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.
>>
>> 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.
>> ---
>>   arch/x86/Kconfig                 |  1 +
>>   arch/x86/entry/entry_64.S        |  5 +++++
>>   arch/x86/include/asm/acrnhyper.h | 19 +++++++++++++++++++
>>   arch/x86/kernel/cpu/acrn.c       | 22 ++++++++++++++++++++++
>>   4 files changed, 47 insertions(+)
>>   create mode 100644 arch/x86/include/asm/acrnhyper.h
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index d77d215..ae4d38b 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/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
> 
> Simply
> 
> 	.../acrn.h
> 
> I'd say.

OK. The simplifed file name(acrn.h) will be used in next version.

> 
>> @@ -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
> 
> Why is that ifdef needed?

It is used to avoid that one function declaration has no definition when 
asm/acrnhyper.h is included and ACRN_GUEST is not enabled.

> 
>> +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);
> 
> end marker:
> 
> #endif /* _ASM_X86_ACRNHYPER_H */

It will be added in next version.

> 
>> 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>
> 
> What's the sorting order here? Length or alphabetic? Or none? :)

Sorry that I don't consider the oder.
The required new header file is appended one by one. Of course linux/ 
path and asm / are appended separately.
Do you have any suggestion about the header order?

> 
> linux/ path includes still need to come first, of course.
> 

Re: [RFC PATCH v3 4/4] x86: Add hypercall for acrn_guest

[Zhao, Yakui]

On 2019年04月08日 23:10, Borislav Petkov wrote:
> On Mon, Apr 08, 2019 at 04:12:11PM +0800, 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.
> 
> Good example.
> 
> What is "Guest system" and why is capitalized? Do you mean "the guest
> operating system" or simply "the guest"?

it means the "guest operating system".
It will be changed to "guest operating system".

> 
>> So the hypercall is added so that the kernel can communicate with the
> 
> "So add the hypercall so that..."
> 
>> low-level acrn-hypervisor.
> 
> Is it acrn_hypervisor or acrn-hypervisor or the ACRN hypervisor or ...?
> 
> Unify the naming pls.

Sure. They will be unified to "ACRN hypervisor" in next version.

> 
>> On x86 it is implemented by using vmacll when
> 
> During last review Thomas said:
> 
> "is implemented with the VMCALL instruction"
> 
> You still have it wrong. Which makes me think you haven't even gone over
> *all* review comments as this is the second missed review comment in a
> 4-patches set.
> 
> So I'm going to stop reviewing here and won't look at your patches until
> you incorporate *all* review comments from all people.
> 

Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Zhao, Yakui]

On 2019年04月08日 22:39, Borislav Petkov wrote:
> On Mon, Apr 08, 2019 at 04:12:09PM +0800, Zhao Yakui wrote:
>> ACRN is an open-source hypervisor maintained by Linuxfoundation.
> 
> I think tglx wanted to say "by the Linux Foundation" here.

Sure. It will be fixed.
> 
>> This is to add the Linux guest support on acrn-hypervisor.
> 
> I think you were told already:
> 
> "Please do not use 'This is to add' or 'This adds'. Just say:
> 
> Add ...."
> 
> So please take your time, work in *all* review feedback instead of
> hurrying the next version out without addressing all review comments.
> 
>> Add x86_hyper_acrn into supported hypervisors array, which enables
>> Linux ACRN guest running on ACRN hypervisor. It is restricted to X86_64.
> 
> So this all talks about *what* the patch does. But that is visible from
> the diff below and doesn't belong in the commit message.
> 
> Rather, your commit message should talk about *why* a change is being
> done.

I will refine the commit log and only talk about "why" a changed is added.

> 
>> 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
>> ---
>>   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 54d1fbc..d77d215 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -845,6 +845,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
>> +	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.
> 
> WARNING: please write a paragraph that describes the config symbol fully
> #47: FILE: arch/x86/Kconfig:848:
> +config ACRN_GUEST
> 
> That help text above could use some of the explanation what ACRN is from
> your 0/4 message.
> 

Make sense. I will add some description in patch 0 for the Kconfig 
description.

>> +
>>   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 */
> 
> Why?
> 
> This comment could explain why that choice has been made.
>

Currently the x2apic is not enabled in the first step.
Next step it needs to check the cpu info reported by ACRN hypervisor to 
determine whether the x2apic should be supported.
How about using the below comments?
" x2apic is not supported now. Later it will check the cpu info to 
determine whether the x2apic can be supported or not."

Thanks

Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Borislav Petkov]

On Wed, Apr 10, 2019 at 05:15:48PM +0800, Zhao, Yakui wrote:
> Currently the x2apic is not enabled in the first step.
> Next step it needs to check the cpu info reported by ACRN hypervisor to
> determine whether the x2apic should be supported.

What "cpu info"? CPUID or something ACRN-specific?

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

[Borislav Petkov]

On Wed, Apr 10, 2019 at 03:57:08PM +0800, Zhao, Yakui wrote:
> It is used to avoid that one function declaration has no definition
> when asm/acrnhyper.h is included and ACRN_GUEST is not enabled.

And that is a problem because...?

> Do you have any suggestion about the header order?
> > 
> > linux/ path includes still need to come first, of course.

As said above:

linux/ path first, then asm/

Feel free to look around the tree for inspiration :)

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Zhao, Yakui]

On 2019年04月11日 21:49, Borislav Petkov wrote:
> On Wed, Apr 10, 2019 at 05:15:48PM +0800, Zhao, Yakui wrote:
>> Currently the x2apic is not enabled in the first step.
>> Next step it needs to check the cpu info reported by ACRN hypervisor to
>> determine whether the x2apic should be supported.
> 
> What "cpu info"? CPUID or something ACRN-specific?

It is based on CPUID.
The low-level ACRN hypervisor can return the different output of CPUID 
when several linux guests executes the CPUID instruction. Then it can 
control whether x2apic is supported in one linux guest.

So we will leverage the X86_FEATURE_X2APIC bit from CPUID to indicate 
whether the x2apic is supported in linux guest when ACRN hypervisor is 
detected.
Is this fine to you?

Thanks
> 

Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

[Zhao, Yakui]

On 2019年04月11日 21:55, Borislav Petkov wrote:
> On Wed, Apr 10, 2019 at 03:57:08PM +0800, Zhao, Yakui wrote:
>> It is used to avoid that one function declaration has no definition
>> when asm/acrnhyper.h is included and ACRN_GUEST is not enabled.
> 
> And that is a problem because...?

This is not a problem.
I take a look at the header file of mshyperv.h and xen/events.h.
It seems that they have no such extra conditional definition.
To follow the same style, I will remove it.

> 
>> Do you have any suggestion about the header order?
>>>
>>> linux/ path includes still need to come first, of course.
> 
> As said above:
> 
> linux/ path first, then asm/
> 
> Feel free to look around the tree for inspiration :)

I take a look at the file of vwmare.c/mshyperv.c under the directory of 
arch/x86/kernel/cpu. It seems that the header file only follows the 
order of: linux/ path first and then asm/.

Anyway, I will follow your idea in previous email and use the alphabetic 
order.

Thanks
> 

Re: [RFC PATCH v3 2/4] x86: Add the support of ACRN guest

[Borislav Petkov]

On Fri, Apr 12, 2019 at 08:36:34AM +0800, Zhao, Yakui wrote:
> So we will leverage the X86_FEATURE_X2APIC bit from CPUID to indicate
> whether the x2apic is supported in linux guest when ACRN hypervisor is
> detected.
> Is this fine to you?

Yes, put that more precise info in the comment pls.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC PATCH v3 3/4] x86: Use HYPERVISOR_CALLBACK_VECTOR for acrn_guest upcall vector

[Borislav Petkov]

On Fri, Apr 12, 2019 at 09:00:10AM +0800, Zhao, Yakui wrote:
> This is not a problem.
> I take a look at the header file of mshyperv.h and xen/events.h.
> It seems that they have no such extra conditional definition.
> To follow the same style, I will remove it.

Yes, the less ifdeffery the better. If it is not really needed.

-- 
Regards/Gruss,
    Boris.

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