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

[permalink] [raw]

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

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

Co-developed-by: Jason Chen CJ <[email protected]>
Signed-off-by: Jason Chen CJ <[email protected]>
Signed-off-by: Zhao Yakui <[email protected]>
---
V1->V2: Remove the unused API definition of acrn_setup_intr_handler and
acrn_remove_intr_handler.
Adjust the order of header file
Add the declaration of acrn_hv_vector_handler and tracing
definition of acrn_hv_callback_vector.

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

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 8dc4200..d7a10f6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -848,6 +848,7 @@ config JAILHOUSE_GUEST
config ACRN_GUEST
bool "ACRN Guest support"
depends on X86_64
+ select X86_HV_CALLBACK_VECTOR
help
This option allows to run Linux as guest in ACRN hypervisor. Enabling
this will allow the kernel to boot in virtualized environment under
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 1f0efdb..d1b8ad3 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1129,6 +1129,11 @@ apicinterrupt3 HYPERV_STIMER0_VECTOR \
hv_stimer0_callback_vector hv_stimer0_vector_handler
#endif /* CONFIG_HYPERV */

+#if IS_ENABLED(CONFIG_ACRN_GUEST)
+apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
+ acrn_hv_callback_vector acrn_hv_vector_handler
+#endif
+
idtentry debug do_debug has_error_code=0 paranoid=1 shift_ist=DEBUG_STACK
idtentry int3 do_int3 has_error_code=0
idtentry stack_segment do_stack_segment has_error_code=1
diff --git a/arch/x86/include/asm/acrn.h b/arch/x86/include/asm/acrn.h
new file mode 100644
index 0000000..43ab032
--- /dev/null
+++ b/arch/x86/include/asm/acrn.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_ACRN_H
+#define _ASM_X86_ACRN_H
+
+void acrn_hv_callback_vector(void);
+#ifdef CONFIG_TRACING
+#define trace_acrn_hv_callback_vector acrn_hv_callback_vector
+#endif
+
+void acrn_hv_vector_handler(struct pt_regs *regs);
+#endif /* _ASM_X86_ACRN_H */
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
index f556640..d8072bf 100644
--- a/arch/x86/kernel/cpu/acrn.c
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -9,7 +9,11 @@
*
*/

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

static uint32_t __init acrn_detect(void)
{
@@ -18,6 +22,8 @@ static uint32_t __init acrn_detect(void)

static void __init acrn_init_platform(void)
{
+ alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR,
+ acrn_hv_callback_vector);
}

static bool acrn_x2apic_available(void)
@@ -30,6 +36,22 @@ static bool acrn_x2apic_available(void)
return false;
}

+static void (*acrn_intr_handler)(void);
+
+__visible void __irq_entry acrn_hv_vector_handler(struct pt_regs *regs)
+{
+ struct pt_regs *old_regs = set_irq_regs(regs);
+
+ entering_ack_irq();
+ inc_irq_stat(irq_hv_callback_count);
+
+ if (acrn_intr_handler)
+ acrn_intr_handler();
+
+ exiting_irq();
+ set_irq_regs(old_regs);
+}
+
const __initconst struct hypervisor_x86 x86_hyper_acrn = {
.name = "ACRN",
.detect = acrn_detect,
--
2.7.4

2019-04-24 00:58:16

[permalink] [raw]

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

When ACRN hypervisor is detected, the hypercall is needed so that the
ACRN guest can query/config some settings. For example: it can be used
to query the resources in hypervisor and manage the CPU/memory/device/
interrupt for the guest operating system.

So add the hypercall so that ACRN guest can communicate with the
low-level ACRN hypervisor. It is implemented with the VMCALL instruction.

Co-developed-by: Jason Chen CJ <[email protected]>
Signed-off-by: Jason Chen CJ <[email protected]>
Signed-off-by: Zhao Yakui <[email protected]>
---
V1->V2: Refine the comments for the function of acrn_hypercall0/1/2
v2->v3: Use the "vmcall" mnemonic to replace hard-code byte definition
v4->v5: Use _ASM_X86_ACRN_HYPERCALL_H instead of _ASM_X86_ACRNHYPERCALL_H to
align the header file of acrn_hypercall.h
Use the "VMCALL" mnemonic in comment/commit log.
Uppercase r8/rdi/rsi/rax for hypercall parameter registers in comment.
---
arch/x86/include/asm/acrn_hypercall.h | 82 +++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
create mode 100644 arch/x86/include/asm/acrn_hypercall.h

diff --git a/arch/x86/include/asm/acrn_hypercall.h b/arch/x86/include/asm/acrn_hypercall.h
new file mode 100644
index 0000000..3594436
--- /dev/null
+++ b/arch/x86/include/asm/acrn_hypercall.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_X86_ACRN_HYPERCALL_H
+#define _ASM_X86_ACRN_HYPERCALL_H
+
+#include <linux/errno.h>
+
+#ifdef CONFIG_ACRN_GUEST
+
+/*
+ * Hypercalls for ACRN guest
+ *
+ * Hypercall number is passed in R8 register.
+ * Up to 2 arguments are passed in RDI, RSI.
+ * Return value will be placed in RAX.
+ */
+
+static inline long acrn_hypercall0(unsigned long hcall_id)
+{
+ register unsigned long r8 asm("r8") = hcall_id;
+ register long result asm("rax");
+
+ /* the hypercall is implemented with the VMCALL instruction.
+ * asm indicates that inline assembler instruction is used.
+ * volatile qualifier is added to avoid that it is dropped
+ * because of compiler optimization.
+ */
+ asm volatile("vmcall"
+ : "=r"(result)
+ : "r"(r8));
+
+ return result;
+}
+
+static inline long acrn_hypercall1(unsigned long hcall_id,
+ unsigned long param1)
+{
+ register unsigned long r8 asm("r8") = hcall_id;
+ register long result asm("rax");
+
+ asm volatile("vmcall"
+ : "=r"(result)
+ : "D"(param1), "r"(r8));
+
+ return result;
+}
+
+static inline long acrn_hypercall2(unsigned long hcall_id,
+ unsigned long param1,
+ unsigned long param2)
+{
+ register unsigned long r8 asm("r8") = hcall_id;
+ register long result asm("rax");
+
+ asm volatile("vmcall"
+ : "=r"(result)
+ : "D"(param1), "S"(param2), "r"(r8));
+
+ return result;
+}
+
+#else
+
+static inline long acrn_hypercall0(unsigned long hcall_id)
+{
+ return -ENOTSUPP;
+}
+
+static inline long acrn_hypercall1(unsigned long hcall_id,
+ unsigned long param1)
+{
+ return -ENOTSUPP;
+}
+
+static inline long acrn_hypercall2(unsigned long hcall_id,
+ unsigned long param1,
+ unsigned long param2)
+{
+ return -ENOTSUPP;
+}
+#endif /* CONFIG_ACRN_GUEST */
+#endif /* _ASM_X86_ACRN_HYPERCALL_H */
--
2.7.4

2019-04-24 00:58:16

[permalink] [raw]

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

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

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

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 62fc3fd..2fc9297 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -791,6 +791,9 @@ config QUEUED_LOCK_STAT
behavior of paravirtualized queued spinlocks and report
them on debugfs.

+config X86_HV_CALLBACK_VECTOR
+ def_bool n
+
source "arch/x86/xen/Kconfig"

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

2019-04-24 00:58:16

[permalink] [raw]

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

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

Co-developed-by: Jason Chen CJ <[email protected]>
Signed-off-by: Jason Chen CJ <[email protected]>
Signed-off-by: Zhao Yakui <[email protected]>
---
v1->v2: Change the CONFIG_ACRN to CONFIG_ACRN_GUEST, which makes it easy to
understand.
Remove the export of x86_hyper_acrn.

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

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2fc9297..8dc4200 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -845,6 +845,18 @@ config JAILHOUSE_GUEST
cell. You can leave this option disabled if you only want to start
Jailhouse and run Linux afterwards in the root cell.

+config ACRN_GUEST
+ bool "ACRN Guest support"
+ depends on X86_64
+ help
+ This option allows to run Linux as guest in ACRN hypervisor. Enabling
+ this will allow the kernel to boot in virtualized environment under
+ the ACRN hypervisor.
+ ACRN is a flexible, lightweight reference open-source hypervisor, built
+ with real-time and safety-criticality in mind. It is built for embedded
+ IOT with small footprint and real-time features. More details can be
+ found in https://projectacrn.org/
+
endif #HYPERVISOR_GUEST

source "arch/x86/Kconfig.cpu"
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 8c5aaba..50a30f6 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -29,6 +29,7 @@ enum x86_hypervisor_type {
X86_HYPER_XEN_HVM,
X86_HYPER_KVM,
X86_HYPER_JAILHOUSE,
+ X86_HYPER_ACRN,
};

#ifdef CONFIG_HYPERVISOR_GUEST
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index cfd24f9..17a7cdf 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_X86_CPU_RESCTRL) += resctrl/
obj-$(CONFIG_X86_LOCAL_APIC) += perfctr-watchdog.o

obj-$(CONFIG_HYPERVISOR_GUEST) += vmware.o hypervisor.o mshyperv.o
+obj-$(CONFIG_ACRN_GUEST) += acrn.o

ifdef CONFIG_X86_FEATURE_NAMES
quiet_cmd_mkcapflags = MKCAP $@
diff --git a/arch/x86/kernel/cpu/acrn.c b/arch/x86/kernel/cpu/acrn.c
new file mode 100644
index 0000000..f556640
--- /dev/null
+++ b/arch/x86/kernel/cpu/acrn.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACRN detection support
+ *
+ * Copyright (C) 2019 Intel Corporation. All rights reserved.
+ *
+ * Jason Chen CJ <[email protected]>
+ * Zhao Yakui <[email protected]>
+ *
+ */
+
+#include <asm/hypervisor.h>
+
+static uint32_t __init acrn_detect(void)
+{
+ return hypervisor_cpuid_base("ACRNACRNACRN\0\0", 0);
+}
+
+static void __init acrn_init_platform(void)
+{
+}
+
+static bool acrn_x2apic_available(void)
+{
+ /* x2apic is not supported now.
+ * Later it needs to check the X86_FEATURE_X2APIC bit of cpu info
+ * returned by CPUID to determine whether the x2apic is
+ * supported in Linux guest.
+ */
+ return false;
+}
+
+const __initconst struct hypervisor_x86 x86_hyper_acrn = {
+ .name = "ACRN",
+ .detect = acrn_detect,
+ .type = X86_HYPER_ACRN,
+ .init.init_platform = acrn_init_platform,
+ .init.x2apic_available = acrn_x2apic_available,
+};
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 479ca47..87e39ad 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -32,6 +32,7 @@ extern const struct hypervisor_x86 x86_hyper_xen_pv;
extern const struct hypervisor_x86 x86_hyper_xen_hvm;
extern const struct hypervisor_x86 x86_hyper_kvm;
extern const struct hypervisor_x86 x86_hyper_jailhouse;
+extern const struct hypervisor_x86 x86_hyper_acrn;

static const __initconst struct hypervisor_x86 * const hypervisors[] =
{
@@ -49,6 +50,9 @@ static const __initconst struct hypervisor_x86 * const hypervisors[] =
#ifdef CONFIG_JAILHOUSE_GUEST
&x86_hyper_jailhouse,
#endif
+#ifdef CONFIG_ACRN_GUEST
+ &x86_hyper_acrn,
+#endif
};

enum x86_hypervisor_type x86_hyper_type;
--
2.7.4

2019-04-25 07:35:18

by Thomas Gleixner

[permalink] [raw]

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

On Wed, 24 Apr 2019, Zhao Yakui wrote:

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

Reviewed-by: Thomas Gleixner <[email protected]>

2019-04-25 08:17:55

[permalink] [raw]

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

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

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

Thanks
Yakui

>

2019-04-25 08:40:11

[permalink] [raw]

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

* Zhao Yakui <[email protected]> wrote:

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

English sentences should not be started with 'and'.

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

Missing 'the', twice.

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

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

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

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

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

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

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

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

Thanks,

Ingo

2019-04-25 12:05:00

[permalink] [raw]

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

* Zhao Yakui <[email protected]> wrote:

> When ACRN hypervisor is detected, the hypercall is needed so that the
> ACRN guest can query/config some settings. For example: it can be used
> to query the resources in hypervisor and manage the CPU/memory/device/
> interrupt for the guest operating system.
>
> So add the hypercall so that ACRN guest can communicate with the
> low-level ACRN hypervisor. It is implemented with the VMCALL instruction.
>
> Co-developed-by: Jason Chen CJ <[email protected]>
> Signed-off-by: Jason Chen CJ <[email protected]>
> Signed-off-by: Zhao Yakui <[email protected]>
> ---
> V1->V2: Refine the comments for the function of acrn_hypercall0/1/2
> v2->v3: Use the "vmcall" mnemonic to replace hard-code byte definition
> v4->v5: Use _ASM_X86_ACRN_HYPERCALL_H instead of _ASM_X86_ACRNHYPERCALL_H to
> align the header file of acrn_hypercall.h
> Use the "VMCALL" mnemonic in comment/commit log.
> Uppercase r8/rdi/rsi/rax for hypercall parameter registers in comment.
> ---
> arch/x86/include/asm/acrn_hypercall.h | 82 +++++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
> create mode 100644 arch/x86/include/asm/acrn_hypercall.h
>
> diff --git a/arch/x86/include/asm/acrn_hypercall.h b/arch/x86/include/asm/acrn_hypercall.h
> new file mode 100644
> index 0000000..3594436
> --- /dev/null
> +++ b/arch/x86/include/asm/acrn_hypercall.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_X86_ACRN_HYPERCALL_H
> +#define _ASM_X86_ACRN_HYPERCALL_H

> +
> +#include <linux/errno.h>
> +
> +#ifdef CONFIG_ACRN_GUEST
> +
> +/*
> + * Hypercalls for ACRN guest
> + *
> + * Hypercall number is passed in R8 register.
> + * Up to 2 arguments are passed in RDI, RSI.
> + * Return value will be placed in RAX.
> + */
> +
> +static inline long acrn_hypercall0(unsigned long hcall_id)
> +{
> + register unsigned long r8 asm("r8") = hcall_id;
> + register long result asm("rax");
> +
> + /* the hypercall is implemented with the VMCALL instruction.
> + * asm indicates that inline assembler instruction is used.
> + * volatile qualifier is added to avoid that it is dropped
> + * because of compiler optimization.
> + */

Non-standard comment style.

asm statements are volatile by default I believe.

I.e. the second and third sentences are partly obvious, superfluous and
bogus.

> + asm volatile("vmcall"
> + : "=r"(result)
> + : "r"(r8));
> +
> + return result;
> +}
> +
> +static inline long acrn_hypercall1(unsigned long hcall_id,
> + unsigned long param1)
> +{
> + register unsigned long r8 asm("r8") = hcall_id;
> + register long result asm("rax");
> +
> + asm volatile("vmcall"
> + : "=r"(result)
> + : "D"(param1), "r"(r8));

Why are register variables used? Doesn't GCC figure it out correctly by
default?

> +static inline long acrn_hypercall2(unsigned long hcall_id,
> + unsigned long param1,
> + unsigned long param2)
> +{
> + register unsigned long r8 asm("r8") = hcall_id;
> + register long result asm("rax");
> +
> + asm volatile("vmcall"
> + : "=r"(result)
> + : "D"(param1), "S"(param2), "r"(r8));

Ditto.

Thanks,

Ingo

2019-04-25 13:19:56

[permalink] [raw]

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

On 2019年04月25日 15:07, Ingo Molnar wrote:

Thanks for the review.
>
> * Zhao Yakui <[email protected]> wrote:
>
>> When ACRN hypervisor is detected, the hypercall is needed so that the
>> ACRN guest can query/config some settings. For example: it can be used
>> to query the resources in hypervisor and manage the CPU/memory/device/
>> interrupt for the guest operating system.
>>
>> So add the hypercall so that ACRN guest can communicate with the
>> low-level ACRN hypervisor. It is implemented with the VMCALL instruction.
>>
>> Co-developed-by: Jason Chen CJ <[email protected]>
>> Signed-off-by: Jason Chen CJ <[email protected]>
>> Signed-off-by: Zhao Yakui <[email protected]>
>> ---
>> V1->V2: Refine the comments for the function of acrn_hypercall0/1/2
>> v2->v3: Use the "vmcall" mnemonic to replace hard-code byte definition
>> v4->v5: Use _ASM_X86_ACRN_HYPERCALL_H instead of _ASM_X86_ACRNHYPERCALL_H to
>> align the header file of acrn_hypercall.h
>> Use the "VMCALL" mnemonic in comment/commit log.
>> Uppercase r8/rdi/rsi/rax for hypercall parameter registers in comment.
>> ---
>> arch/x86/include/asm/acrn_hypercall.h | 82 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 82 insertions(+)
>> create mode 100644 arch/x86/include/asm/acrn_hypercall.h
>>
>> diff --git a/arch/x86/include/asm/acrn_hypercall.h b/arch/x86/include/asm/acrn_hypercall.h
>> new file mode 100644
>> index 0000000..3594436
>> --- /dev/null
>> +++ b/arch/x86/include/asm/acrn_hypercall.h
>> @@ -0,0 +1,82 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef _ASM_X86_ACRN_HYPERCALL_H
>> +#define _ASM_X86_ACRN_HYPERCALL_H
>
>
>> +
>> +#include <linux/errno.h>
>> +
>> +#ifdef CONFIG_ACRN_GUEST
>> +
>> +/*
>> + * Hypercalls for ACRN guest
>> + *
>> + * Hypercall number is passed in R8 register.
>> + * Up to 2 arguments are passed in RDI, RSI.
>> + * Return value will be placed in RAX.
>> + */
>> +
>> +static inline long acrn_hypercall0(unsigned long hcall_id)
>> +{
>> + register unsigned long r8 asm("r8") = hcall_id;
>> + register long result asm("rax");
>> +
>> + /* the hypercall is implemented with the VMCALL instruction.
>> + * asm indicates that inline assembler instruction is used.
>> + * volatile qualifier is added to avoid that it is dropped
>> + * because of compiler optimization.
>> + */
>
> Non-standard comment style.
>
> asm statements are volatile by default I believe.

For the basic asm: it is volatile by default.
For the extend asm: The volatile is needed to disable the certain
optimization.
The below info is copied from:
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm

The typical use of extended asm statements is to manipulate input values
to produce output values. However, your asm statements may also produce
side effects. If so, you may need to use the volatile qualifier to
disable certain optimizations. See Volatile.

>
> I.e. the second and third sentences are partly obvious, superfluous and
> bogus.
>
>> + asm volatile("vmcall"
>> + : "=r"(result)
>> + : "r"(r8));
>> +
>> + return result;
>> +}
>> +
>> +static inline long acrn_hypercall1(unsigned long hcall_id,
>> + unsigned long param1)
>> +{
>> + register unsigned long r8 asm("r8") = hcall_id;
>> + register long result asm("rax");
>> +
>> + asm volatile("vmcall"
>> + : "=r"(result)
>> + : "D"(param1), "r"(r8));
>
> Why are register variables used? Doesn't GCC figure it out correctly by
> default?

The parameter register for the VMCALL is predefined in ACRN hypervisor.
Now the R8 is used to pass the hcall_id.
It seems that there is no special constraint for R8~R15.
So the explicit register variable is used so that the R8 can be passed.

>
>> +static inline long acrn_hypercall2(unsigned long hcall_id,
>> + unsigned long param1,
>> + unsigned long param2)
>> +{
>> + register unsigned long r8 asm("r8") = hcall_id;
>> + register long result asm("rax");
>> +
>> + asm volatile("vmcall"
>> + : "=r"(result)
>> + : "D"(param1), "S"(param2), "r"(r8));
>
> Ditto.
>
> Thanks,
>
> Ingo
>

2019-04-25 14:08:16

[permalink] [raw]

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

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

OK. I will remove it.

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

OK. It will be added.

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

OK. The "extern" will be added.

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

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

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

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

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

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

>
> Thanks,
>
> Ingo
>

2019-04-25 15:33:06

[permalink] [raw]

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

On Thu, Apr 25, 2019 at 06:16:02PM +0800, Zhao, Yakui wrote:
> The parameter register for the VMCALL is predefined in ACRN hypervisor. Now
> the R8 is used to pass the hcall_id.
> It seems that there is no special constraint for R8~R15.
> So the explicit register variable is used so that the R8 can be passed.

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

"=a" (result)

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

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

unsigned long result;

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

return result;

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

gcc turns it into

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

here.

--
Regards/Gruss,
Boris.

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

2019-04-25 19:48:06

[permalink] [raw]

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

* Zhao, Yakui <[email protected]> wrote:

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

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

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

Ok!

Thanks,

Ingo

2019-04-26 02:05:16

[permalink] [raw]

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

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

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

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

> Thanks,
>
> Ingo
>

2019-04-26 03:23:37

[permalink] [raw]

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

On 2019年04月25日 19:00, Borislav Petkov wrote:
> On Thu, Apr 25, 2019 at 06:16:02PM +0800, Zhao, Yakui wrote:
>> The parameter register for the VMCALL is predefined in ACRN hypervisor. Now
>> the R8 is used to pass the hcall_id.
>> It seems that there is no special constraint for R8~R15.
>> So the explicit register variable is used so that the R8 can be passed.
>
> If you're going to use the constraint "D" for param1, you can just as
> well do
>
> "=a" (result)
>
> everywhere since you have the letter constraint for %rax instead of
> declaring it with "register".
>
> Also, you can completely get rid of those "register" declarations
> and let gcc have all the freedom to pass in hcall_id and the other
> parameters:
Thanks Borislav for providing the code.

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

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

Is this ok?

Thanks

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

2019-04-26 06:00:45

[permalink] [raw]

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

* Zhao, Yakui <[email protected]> wrote:

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

Yeah, thanks!

Ingo

2019-04-27 09:01:08

[permalink] [raw]

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

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

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

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

output.

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

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

--
Regards/Gruss,
Boris.

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

2019-04-28 02:01:45

[permalink] [raw]

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

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

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

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

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

Thanks
Yakui

2019-04-28 10:04:30

[permalink] [raw]

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

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

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

--
Regards/Gruss,
Boris.

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

2019-04-29 01:28:31

[permalink] [raw]

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

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

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

Thanks
Yakui
>

2019-04-29 07:39:13

[permalink] [raw]

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

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

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

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

--
Regards/Gruss,
Boris.

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

2019-04-29 09:56:03