This patch set enables the Hyper-V code to boot on ARM64 inside a Virtual Trust
Level. These levels are a part of the Virtual Secure Mode documented in the
Top-Level Functional Specification available at
https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/vsm
[V2]
- Decreased number of #ifdef's
- Updated the wording in the commit messages to adhere to the guidlines
- Sending to the correct set of maintainers and mail lists
Roman Kisel (6):
arm64/hyperv: Support DeviceTree
drivers/hv: Enable VTL mode for arm64
drivers/hv: arch-neutral implementation of get_vtl()
arm64/hyperv: Boot in a Virtual Trust Level
drivers/hv/vmbus: Get the irq number from DeviceTree
drivers/pci/hyperv/arm64: vPCI MSI IRQ domain from DT
arch/arm64/hyperv/Makefile | 1 +
arch/arm64/hyperv/hv_vtl.c | 19 +++++++++++++
arch/arm64/hyperv/mshyperv.c | 40 +++++++++++++++++++++++----
arch/arm64/include/asm/mshyperv.h | 8 ++++++
arch/x86/hyperv/hv_init.c | 34 -----------------------
arch/x86/hyperv/hv_vtl.c | 2 +-
arch/x86/include/asm/hyperv-tlfs.h | 7 -----
drivers/hv/Kconfig | 6 ++--
drivers/hv/hv_common.c | 43 +++++++++++++++++++++++++++++
drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++
drivers/pci/controller/pci-hyperv.c | 13 +++++++--
include/asm-generic/hyperv-tlfs.h | 7 +++++
include/asm-generic/mshyperv.h | 6 ++++
include/linux/acpi.h | 9 ++++++
14 files changed, 179 insertions(+), 53 deletions(-)
create mode 100644 arch/arm64/hyperv/hv_vtl.c
base-commit: f2580a907e5c0e8fc9354fd095b011301c64f949
--
2.45.0
Kconfig dependencies for arm64 guests on Hyper-V require that be ACPI enabled,
and limit VTL mode to x86/x64. To enable VTL mode on arm64 as well, update the
dependencies. Since VTL mode requires DeviceTree instead of ACPI, don’t require
arm64 guests on Hyper-V to have ACPI.
Signed-off-by: Roman Kisel <[email protected]>
---
drivers/hv/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 862c47b191af..a5cd1365e248 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -5,7 +5,7 @@ menu "Microsoft Hyper-V guest support"
config HYPERV
tristate "Microsoft Hyper-V client drivers"
depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
- || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
+ || (ARM64 && !CPU_BIG_ENDIAN)
select PARAVIRT
select X86_HV_CALLBACK_VECTOR if X86
select OF_EARLY_FLATTREE if OF
@@ -15,7 +15,7 @@ config HYPERV
config HYPERV_VTL_MODE
bool "Enable Linux to boot in VTL context"
- depends on X86_64 && HYPERV
+ depends on HYPERV
depends on SMP
default n
help
@@ -31,7 +31,7 @@ config HYPERV_VTL_MODE
Select this option to build a Linux kernel to run at a VTL other than
the normal VTL0, which currently is only VTL2. This option
- initializes the x86 platform for VTL2, and adds the ability to boot
+ initializes the kernel to run in VTL2, and adds the ability to boot
secondary CPUs directly into 64-bit context as required for VTLs other
than 0. A kernel built with this option must run at VTL2, and will
not run as a normal guest.
--
2.45.0
To run in the VTL mode, Hyper-V drivers have to know what
VTL the system boots in, and the arm64/hyperv code does not
have the means to compute that.
Refactor the code to hoist the function that detects VTL,
make it arch-neutral to be able to employ it to get the VTL
on arm64.
Signed-off-by: Roman Kisel <[email protected]>
---
arch/x86/hyperv/hv_init.c | 34 -----------------------
arch/x86/include/asm/hyperv-tlfs.h | 7 -----
drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++
include/asm-generic/hyperv-tlfs.h | 7 +++++
include/asm-generic/mshyperv.h | 6 +++++
5 files changed, 56 insertions(+), 41 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 17a71e92a343..c350fa05ee59 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void)
local_irq_restore(flags);
}
-#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
-static u8 __init get_vtl(void)
-{
- u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
- struct hv_get_vp_registers_input *input;
- struct hv_get_vp_registers_output *output;
- unsigned long flags;
- u64 ret;
-
- local_irq_save(flags);
- input = *this_cpu_ptr(hyperv_pcpu_input_arg);
- output = (struct hv_get_vp_registers_output *)input;
-
- memset(input, 0, struct_size(input, element, 1));
- input->header.partitionid = HV_PARTITION_ID_SELF;
- input->header.vpindex = HV_VP_INDEX_SELF;
- input->header.inputvtl = 0;
- input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS;
-
- ret = hv_do_hypercall(control, input, output);
- if (hv_result_success(ret)) {
- ret = output->as64.low & HV_X64_VTL_MASK;
- } else {
- pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
- BUG();
- }
-
- local_irq_restore(flags);
- return ret;
-}
-#else
-static inline u8 get_vtl(void) { return 0; }
-#endif
-
/*
* This function is to be invoked early in the boot sequence after the
* hypervisor has been detected.
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 3787d26810c1..9ee68eb8e6ff 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -309,13 +309,6 @@ enum hv_isolation_type {
#define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG)
#define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT)
-/*
- * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and
- * there is not associated MSR address.
- */
-#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003
-#define HV_X64_VTL_MASK GENMASK(3, 0)
-
/* Hyper-V memory host visibility */
enum hv_mem_host_visibility {
VMBUS_PAGE_NOT_VISIBLE = 0,
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index dde3f9b6871a..d4cf477a4d0c 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 param2)
return HV_STATUS_INVALID_PARAMETER;
}
EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
+
+#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
+u8 __init get_vtl(void)
+{
+ u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
+ struct hv_get_vp_registers_input *input;
+ struct hv_get_vp_registers_output *output;
+ unsigned long flags;
+ u64 ret;
+
+ local_irq_save(flags);
+ input = *this_cpu_ptr(hyperv_pcpu_input_arg);
+ output = (struct hv_get_vp_registers_output *)input;
+
+ memset(input, 0, struct_size(input, element, 1));
+ input->header.partitionid = HV_PARTITION_ID_SELF;
+ input->header.vpindex = HV_VP_INDEX_SELF;
+ input->header.inputvtl = 0;
+ input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS;
+
+ ret = hv_do_hypercall(control, input, output);
+ if (hv_result_success(ret)) {
+ ret = output->as64.low & HV_VTL_MASK;
+ } else {
+ pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
+
+ /*
+ * This is a dead end, something fundamental is broken.
+ *
+ * There is no sensible way of continuing as the Hyper-V drivers
+ * transitively depend via the vmbus driver on knowing which VTL
+ * they run in to establish communication with the host. The kernel
+ * is going to be worse off if continued booting than a panicked one,
+ * just hung and stuck, producing second-order failures, with neither
+ * a way to recover nor to provide expected services.
+ */
+ BUG();
+ }
+
+ local_irq_restore(flags);
+ return ret;
+}
+#endif
diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
index 87e3d49a4e29..682bcda3124f 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -75,6 +75,13 @@
/* AccessTscInvariantControls privilege */
#define HV_ACCESS_TSC_INVARIANT BIT(15)
+/*
+ * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS
+ * hvcall, and there is no an associated MSR on x86.
+ */
+#define HV_REGISTER_VSM_VP_STATUS 0x000D0003
+#define HV_VTL_MASK GENMASK(3, 0)
+
/*
* Group B features.
*/
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 99935779682d..ea434186d765 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -301,4 +301,10 @@ static inline enum hv_isolation_type hv_get_isolation_type(void)
}
#endif /* CONFIG_HYPERV */
+#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
+u8 __init get_vtl(void);
+#else
+static inline u8 get_vtl(void) { return 0; }
+#endif
+
#endif
--
2.45.0
To run in the VTL mode, Hyper-V drivers have to know what
VTL the system boots in, and the arm64/hyperv code does not
update the variable that stores the value.
Update the variable to enable the Hyper-V drivers to boot
in the VTL mode and print the VTL the code runs in.
Signed-off-by: Roman Kisel <[email protected]>
---
arch/arm64/hyperv/Makefile | 1 +
arch/arm64/hyperv/hv_vtl.c | 19 +++++++++++++++++++
arch/arm64/hyperv/mshyperv.c | 6 ++++++
arch/arm64/include/asm/mshyperv.h | 8 ++++++++
arch/x86/hyperv/hv_vtl.c | 2 +-
5 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/hyperv/hv_vtl.c
diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
index 87c31c001da9..9701a837a6e1 100644
--- a/arch/arm64/hyperv/Makefile
+++ b/arch/arm64/hyperv/Makefile
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0
obj-y := hv_core.o mshyperv.o
+obj-$(CONFIG_HYPERV_VTL_MODE) += hv_vtl.o
diff --git a/arch/arm64/hyperv/hv_vtl.c b/arch/arm64/hyperv/hv_vtl.c
new file mode 100644
index 000000000000..9b44cc49594c
--- /dev/null
+++ b/arch/arm64/hyperv/hv_vtl.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023, Microsoft, Inc.
+ *
+ * Author : Roman Kisel <[email protected]>
+ */
+
+#include <asm/mshyperv.h>
+
+void __init hv_vtl_init_platform(void)
+{
+ pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
+}
+
+int __init hv_vtl_early_init(void)
+{
+ return 0;
+}
+early_initcall(hv_vtl_early_init);
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index 208a3bcb9686..cbde483b167a 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -96,6 +96,12 @@ static int __init hyperv_init(void)
return ret;
}
+ /* Find the VTL */
+ ms_hyperv.vtl = get_vtl();
+ if (ms_hyperv.vtl > 0) /* non default VTL */
+ hv_vtl_early_init();
+
+ hv_vtl_init_platform();
ms_hyperv_late_init();
hyperv_initialized = true;
diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
index a975e1a689dd..4a8ff6be389c 100644
--- a/arch/arm64/include/asm/mshyperv.h
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -49,6 +49,14 @@ static inline u64 hv_get_msr(unsigned int reg)
ARM_SMCCC_OWNER_VENDOR_HYP, \
HV_SMCCC_FUNC_NUMBER)
+#ifdef CONFIG_HYPERV_VTL_MODE
+void __init hv_vtl_init_platform(void);
+int __init hv_vtl_early_init(void);
+#else
+static inline void __init hv_vtl_init_platform(void) {}
+static inline int __init hv_vtl_early_init(void) { return 0; }
+#endif
+
#include <asm-generic/mshyperv.h>
#endif
diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 92bd5a55f093..ae3105375a12 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -19,7 +19,7 @@ static struct real_mode_header hv_vtl_real_mode_header;
void __init hv_vtl_init_platform(void)
{
- pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
+ pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
x86_platform.realmode_reserve = x86_init_noop;
x86_platform.realmode_init = x86_init_noop;
--
2.45.0
The vmbus driver uses ACPI for interrupt assignment on
arm64 hence it won't function in the VTL mode where only
DeviceTree can be used.
Update the vmbus driver to discover interrupt configuration
via DeviceTree.
Signed-off-by: Roman Kisel <[email protected]>
---
drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index e25223cee3ab..52f01bd1c947 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -36,6 +36,7 @@
#include <linux/syscore_ops.h>
#include <linux/dma-map-ops.h>
#include <linux/pci.h>
+#include <linux/of_irq.h>
#include <clocksource/hyperv_timer.h>
#include <asm/mshyperv.h>
#include "hyperv_vmbus.h"
@@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
}
#endif
+static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
+{
+ struct irq_desc *desc;
+ int irq;
+
+ irq = of_irq_get(np, 0);
+ if (irq == 0) {
+ pr_err("VMBus interrupt mapping failure\n");
+ return -EINVAL;
+ }
+ if (irq < 0) {
+ pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
+ return irq;
+ }
+
+ desc = irq_to_desc(irq);
+ if (!desc) {
+ pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
+ return -ENODEV;
+ }
+
+ vmbus_irq = irq;
+ vmbus_interrupt = desc->irq_data.hwirq;
+ pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
+
+ return 0;
+}
+
static int vmbus_device_add(struct platform_device *pdev)
{
struct resource **cur_res = &hyperv_mmio;
@@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
int ret;
+ pr_debug("VMBus is present in DeviceTree\n");
+
hv_dev = &pdev->dev;
ret = of_range_parser_init(&parser, np);
if (ret)
return ret;
+#ifndef HYPERVISOR_CALLBACK_VECTOR
+ ret = vmbus_of_set_irq(np);
+ if (ret)
+ return ret;
+#endif
+
for_each_of_range(&parser, &range) {
struct resource *res;
--
2.45.0
The Virtual Trust Level platforms rely on DeviceTree, and the
arm64/hyperv code supports ACPI only. Update the logic to
support DeviceTree on boot as well as ACPI.
Signed-off-by: Roman Kisel <[email protected]>
---
arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index b1a4de4eee29..208a3bcb9686 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -15,6 +15,9 @@
#include <linux/errno.h>
#include <linux/version.h>
#include <linux/cpuhotplug.h>
+#include <linux/libfdt.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
#include <asm/mshyperv.h>
static bool hyperv_initialized;
@@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
return 0;
}
+static bool hyperv_detect_fdt(void)
+{
+#ifdef CONFIG_OF
+ const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
+ of_get_flat_dt_root(), "hypervisor");
+
+ return (hyp_node != -FDT_ERR_NOTFOUND) &&
+ of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
+#else
+ return false;
+#endif
+}
+
+static bool hyperv_detect_acpi(void)
+{
+#ifdef CONFIG_ACPI
+ return !acpi_disabled &&
+ !strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8);
+#else
+ return false;
+#endif
+}
+
static int __init hyperv_init(void)
{
struct hv_get_vp_registers_output result;
@@ -35,13 +61,11 @@ static int __init hyperv_init(void)
/*
* Allow for a kernel built with CONFIG_HYPERV to be running in
- * a non-Hyper-V environment, including on DT instead of ACPI.
+ * a non-Hyper-V environment.
+ *
* In such cases, do nothing and return success.
*/
- if (acpi_disabled)
- return 0;
-
- if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
+ if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
return 0;
/* Setup the guest ID */
--
2.45.0
The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
on arm64 thereby it won't be able to do that in the VTL mode where
only DeviceTree can be used.
Update the hyperv-pci driver to discover interrupt configuration
via DeviceTree.
Signed-off-by: Roman Kisel <[email protected]>
---
drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
include/linux/acpi.h | 9 +++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 1eaffff40b8d..ccc2b54206f4 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
* way to ensure that all the corresponding devices are also gone and
* no interrupts will be generated.
*/
- hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
- fn, &hv_pci_domain_ops,
- chip_data);
+ if (acpi_disabled)
+ hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+ irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
+ 0, HV_PCI_MSI_SPI_NR,
+ fn, &hv_pci_domain_ops,
+ chip_data);
+ else
+ hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
+ fn, &hv_pci_domain_ops,
+ chip_data);
if (!hv_msi_gic_irq_domain) {
pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b7165e52b3c6..498cbb2c40a1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
return false;
}
+static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
+ unsigned int size,
+ struct fwnode_handle *fwnode,
+ const struct irq_domain_ops *ops,
+ void *host_data)
+{
+ return NULL;
+}
+
#endif /* !CONFIG_ACPI */
extern void arch_post_acpi_subsys_init(void);
--
2.45.0
On 15/05/2024 00:43, Roman Kisel wrote:
> The Virtual Trust Level platforms rely on DeviceTree, and the
> arm64/hyperv code supports ACPI only. Update the logic to
> support DeviceTree on boot as well as ACPI.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..208a3bcb9686 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -15,6 +15,9 @@
> #include <linux/errno.h>
> #include <linux/version.h>
> #include <linux/cpuhotplug.h>
> +#include <linux/libfdt.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> #include <asm/mshyperv.h>
>
> static bool hyperv_initialized;
> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
> return 0;
> }
>
> +static bool hyperv_detect_fdt(void)
> +{
> +#ifdef CONFIG_OF
> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
> + of_get_flat_dt_root(), "hypervisor");
Why do you add an ABI for node name? Although name looks OK, but is it
really described in the spec that you depend on it? I really do not like
name dependencies...
Where is the binding for this?
Best regards,
Krzysztof
On 15/05/2024 00:43, Roman Kisel wrote:
> The vmbus driver uses ACPI for interrupt assignment on
> arm64 hence it won't function in the VTL mode where only
> DeviceTree can be used.
>
> Update the vmbus driver to discover interrupt configuration
> via DeviceTree.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index e25223cee3ab..52f01bd1c947 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/dma-map-ops.h>
> #include <linux/pci.h>
> +#include <linux/of_irq.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> #include "hyperv_vmbus.h"
> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> }
> #endif
>
> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
> +{
> + struct irq_desc *desc;
> + int irq;
> +
> + irq = of_irq_get(np, 0);
Where is the binding for this?
> + if (irq == 0) {
> + pr_err("VMBus interrupt mapping failure\n");
> + return -EINVAL;
> + }
> + if (irq < 0) {
> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
> + return irq;
> + }
> +
> + desc = irq_to_desc(irq);
> + if (!desc) {
> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
> + return -ENODEV;
> + }
> +
> + vmbus_irq = irq;
> + vmbus_interrupt = desc->irq_data.hwirq;
> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
> +
> + return 0;
> +}
> +
> static int vmbus_device_add(struct platform_device *pdev)
> {
> struct resource **cur_res = &hyperv_mmio;
> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> int ret;
>
> + pr_debug("VMBus is present in DeviceTree\n");
Not related and not really helpful. Simple entry/exit tracking is
provided already by tracing.
Best regards,
Krzysztof
On Tue, May 14, 2024 at 03:43:52PM -0700, Roman Kisel wrote:
> The vmbus driver uses ACPI for interrupt assignment on
In subject use the prefix "Drivers: hv: vmbus:".
It is preferred to us "VMbus/VMBus" instead of "vmbus" for all the
commit message and comments.
> arm64 hence it won't function in the VTL mode where only
> DeviceTree can be used.
>
> Update the vmbus driver to discover interrupt configuration
> via DeviceTree.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index e25223cee3ab..52f01bd1c947 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/dma-map-ops.h>
> #include <linux/pci.h>
> +#include <linux/of_irq.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> #include "hyperv_vmbus.h"
> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> }
> #endif
>
> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
> +{
> + struct irq_desc *desc;
> + int irq;
> +
> + irq = of_irq_get(np, 0);
> + if (irq == 0) {
> + pr_err("VMBus interrupt mapping failure\n");
> + return -EINVAL;
> + }
> + if (irq < 0) {
> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
> + return irq;
> + }
> +
> + desc = irq_to_desc(irq);
> + if (!desc) {
> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
> + return -ENODEV;
> + }
> +
> + vmbus_irq = irq;
> + vmbus_interrupt = desc->irq_data.hwirq;
> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
> +
> + return 0;
> +}
> +
> static int vmbus_device_add(struct platform_device *pdev)
> {
> struct resource **cur_res = &hyperv_mmio;
> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> int ret;
>
> + pr_debug("VMBus is present in DeviceTree\n");
> +
> hv_dev = &pdev->dev;
>
> ret = of_range_parser_init(&parser, np);
> if (ret)
> return ret;
>
> +#ifndef HYPERVISOR_CALLBACK_VECTOR
> + ret = vmbus_of_set_irq(np);
> + if (ret)
> + return ret;
> +#endif
> +
> for_each_of_range(&parser, &range) {
> struct resource *res;
>
> --
> 2.45.0
>
On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> on arm64 thereby it won't be able to do that in the VTL mode where
> only DeviceTree can be used.
>
> Update the hyperv-pci driver to discover interrupt configuration
> via DeviceTree.
Subject prefix should be "PCI: hv:"
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
> include/linux/acpi.h | 9 +++++++++
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..ccc2b54206f4 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
> * way to ensure that all the corresponding devices are also gone and
> * no interrupts will be generated.
> */
> - hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> - fn, &hv_pci_domain_ops,
> - chip_data);
> + if (acpi_disabled)
> + hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> + irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
> + 0, HV_PCI_MSI_SPI_NR,
> + fn, &hv_pci_domain_ops,
> + chip_data);
> + else
> + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> + fn, &hv_pci_domain_ops,
> + chip_data);
Upto 100 characters per line are supported now, we can have less
line breaks.
>
> if (!hv_msi_gic_irq_domain) {
> pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b7165e52b3c6..498cbb2c40a1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
> return false;
> }
>
> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> + unsigned int size,
> + struct fwnode_handle *fwnode,
> + const struct irq_domain_ops *ops,
> + void *host_data)
> +{
> + return NULL;
> +}
> +
> #endif /* !CONFIG_ACPI */
>
> extern void arch_post_acpi_subsys_init(void);
> --
> 2.45.0
>
From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>
> Kconfig dependencies for arm64 guests on Hyper-V require that be ACPI enabled,
> and limit VTL mode to x86/x64. To enable VTL mode on arm64 as well, update the
> dependencies. Since VTL mode requires DeviceTree instead of ACPI, don't require
> arm64 guests on Hyper-V to have ACPI.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/hv/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 862c47b191af..a5cd1365e248 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -5,7 +5,7 @@ menu "Microsoft Hyper-V guest support"
> config HYPERV
> tristate "Microsoft Hyper-V client drivers"
> depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
> - || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
> + || (ARM64 && !CPU_BIG_ENDIAN)
> select PARAVIRT
> select X86_HV_CALLBACK_VECTOR if X86
> select OF_EARLY_FLATTREE if OF
> @@ -15,7 +15,7 @@ config HYPERV
>
> config HYPERV_VTL_MODE
> bool "Enable Linux to boot in VTL context"
> - depends on X86_64 && HYPERV
> + depends on HYPERV
> depends on SMP
> default n
> help
These changes make it possible to build a normal VTL 0 Hyper-V
guest (i.e., CONFIG_HYPERV_VTL_MODE=n) if CONFIG_ACPI is
not set, which won't work. While we can say "don't do that", it
would be better if the Kconfig dependencies expressed that
requirement.
A possible fix is to remove the "depends on HYPERV" from
HYPERV_VTL_MODE. Then for HYPERV, make
the "depends on ACPI" be conditional on !HYPERV_VTL_MODE
(for both ARM64 and X86).
I think we originally had "depends on HYPERV" in
HYPERV_VTL_MODE because there was a VTL-related function
in a non-Hyper-V code path, and we wanted to prevent that code
from running in non-Hyper-V environments. But in practice, that
turned out not to work well because occasionally people would
do an "all config" build where both CONFIG_HYPERV and
CONFIG_HYPERV_VTL_MODE were set, and it would panic during
boot in their non-Hyper-V environment. Such people were not
happy. :-( So Saurabh made a relatively simple change (see commit
14058f72cf13e) that got the VTL code out of that non-Hyper-V code
path. With that change, it shouldn't matter if someone sets
CONFIG_HYPERV_VTL_MODE=y in a build where
CONFIG_HYPERV=n.
At least that's my theory. :-) Someone would need to check
it carefully.
Michael
From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>
> To run in the VTL mode, Hyper-V drivers have to know what
> VTL the system boots in, and the arm64/hyperv code does not
> have the means to compute that.
>
> Refactor the code to hoist the function that detects VTL,
> make it arch-neutral to be able to employ it to get the VTL
> on arm64.
Nit: In patches that just refactor and move some code around,
patch authors will often include a statement like "No functional
changes" (or the slightly more doubtful "No functional changes
intended") as a separate line at the end of the commit message.
It's just something the reader/reviewer can cross-check against.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> arch/x86/hyperv/hv_init.c | 34 -----------------------
> arch/x86/include/asm/hyperv-tlfs.h | 7 -----
> drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++
> include/asm-generic/hyperv-tlfs.h | 7 +++++
> include/asm-generic/mshyperv.h | 6 +++++
> 5 files changed, 56 insertions(+), 41 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 17a71e92a343..c350fa05ee59 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void)
> local_irq_restore(flags);
> }
>
> -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
> -static u8 __init get_vtl(void)
> -{
> - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
> - struct hv_get_vp_registers_input *input;
> - struct hv_get_vp_registers_output *output;
> - unsigned long flags;
> - u64 ret;
> -
> - local_irq_save(flags);
> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> - output = (struct hv_get_vp_registers_output *)input;
> -
> - memset(input, 0, struct_size(input, element, 1));
> - input->header.partitionid = HV_PARTITION_ID_SELF;
> - input->header.vpindex = HV_VP_INDEX_SELF;
> - input->header.inputvtl = 0;
> - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS;
> -
> - ret = hv_do_hypercall(control, input, output);
> - if (hv_result_success(ret)) {
> - ret = output->as64.low & HV_X64_VTL_MASK;
> - } else {
> - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
> - BUG();
> - }
> -
> - local_irq_restore(flags);
> - return ret;
> -}
> -#else
> -static inline u8 get_vtl(void) { return 0; }
> -#endif
> -
> /*
> * This function is to be invoked early in the boot sequence after the
> * hypervisor has been detected.
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 3787d26810c1..9ee68eb8e6ff 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -309,13 +309,6 @@ enum hv_isolation_type {
> #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG)
> #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT)
>
> -/*
> - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and
> - * there is not associated MSR address.
> - */
> -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003
> -#define HV_X64_VTL_MASK GENMASK(3, 0)
> -
> /* Hyper-V memory host visibility */
> enum hv_mem_host_visibility {
> VMBUS_PAGE_NOT_VISIBLE = 0,
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index dde3f9b6871a..d4cf477a4d0c 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64
> param2)
> return HV_STATUS_INVALID_PARAMETER;
> }
> EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
> +
> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
> +u8 __init get_vtl(void)
> +{
> + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
> + struct hv_get_vp_registers_input *input;
> + struct hv_get_vp_registers_output *output;
> + unsigned long flags;
> + u64 ret;
> +
> + local_irq_save(flags);
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + output = (struct hv_get_vp_registers_output *)input;
I gave some bad advice on the original version of the above code. Rather
than allocating a separate hypercall output area, I had suggested just setting
the hypercall output to be the same as the input area, which is done
above. While the overlap doesn't cause any problems for a hypercall returning
the value of single register, the TLFS says to never set up such an overlap.
Since this code is being moved anyway, there's an opportunity to fix
this by setting output to "input" + "struct_size(input, element, 1)". Or
put the output at the start of the second half of the page (note that the
size is HY_HYP_PAGE_SIZE, not PAGE_SIZE). The input and output for 1
register are relatively small and both easily fit with either approach. They
just shouldn't overlap.
Some might argue this tweak should be made in a separate patch, but
in my judgment, it's OK to do it here if you note it in the commit
message. Your call. Of course, if you make this change, my previous
comment about "No functional changes" no longer applies. :-)
Michael
> +
> + memset(input, 0, struct_size(input, element, 1));
> + input->header.partitionid = HV_PARTITION_ID_SELF;
> + input->header.vpindex = HV_VP_INDEX_SELF;
> + input->header.inputvtl = 0;
> + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS;
> +
> + ret = hv_do_hypercall(control, input, output);
> + if (hv_result_success(ret)) {
> + ret = output->as64.low & HV_VTL_MASK;
> + } else {
> + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
> +
> + /*
> + * This is a dead end, something fundamental is broken.
> + *
> + * There is no sensible way of continuing as the Hyper-V drivers
> + * transitively depend via the vmbus driver on knowing which VTL
> + * they run in to establish communication with the host. The kernel
> + * is going to be worse off if continued booting than a panicked one,
> + * just hung and stuck, producing second-order failures, with neither
> + * a way to recover nor to provide expected services.
> + */
> + BUG();
> + }
> +
> + local_irq_restore(flags);
> + return ret;
> +}
> +#endif
> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> index 87e3d49a4e29..682bcda3124f 100644
> --- a/include/asm-generic/hyperv-tlfs.h
> +++ b/include/asm-generic/hyperv-tlfs.h
> @@ -75,6 +75,13 @@
> /* AccessTscInvariantControls privilege */
> #define HV_ACCESS_TSC_INVARIANT BIT(15)
>
> +/*
> + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS
> + * hvcall, and there is no an associated MSR on x86.
> + */
> +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003
> +#define HV_VTL_MASK GENMASK(3, 0)
> +
> /*
> * Group B features.
> */
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 99935779682d..ea434186d765 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -301,4 +301,10 @@ static inline enum hv_isolation_type
> hv_get_isolation_type(void)
> }
> #endif /* CONFIG_HYPERV */
>
> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
> +u8 __init get_vtl(void);
> +#else
> +static inline u8 get_vtl(void) { return 0; }
> +#endif
> +
> #endif
> --
> 2.45.0
>
From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>
> To run in the VTL mode, Hyper-V drivers have to know what
> VTL the system boots in, and the arm64/hyperv code does not
> update the variable that stores the value.
>
> Update the variable to enable the Hyper-V drivers to boot
> in the VTL mode and print the VTL the code runs in.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> arch/arm64/hyperv/Makefile | 1 +
> arch/arm64/hyperv/hv_vtl.c | 19 +++++++++++++++++++
> arch/arm64/hyperv/mshyperv.c | 6 ++++++
> arch/arm64/include/asm/mshyperv.h | 8 ++++++++
> arch/x86/hyperv/hv_vtl.c | 2 +-
> 5 files changed, 35 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/hyperv/hv_vtl.c
>
> diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
> index 87c31c001da9..9701a837a6e1 100644
> --- a/arch/arm64/hyperv/Makefile
> +++ b/arch/arm64/hyperv/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-y := hv_core.o mshyperv.o
> +obj-$(CONFIG_HYPERV_VTL_MODE) += hv_vtl.o
> diff --git a/arch/arm64/hyperv/hv_vtl.c b/arch/arm64/hyperv/hv_vtl.c
> new file mode 100644
> index 000000000000..9b44cc49594c
> --- /dev/null
> +++ b/arch/arm64/hyperv/hv_vtl.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023, Microsoft, Inc.
> + *
> + * Author : Roman Kisel <[email protected]>
> + */
> +
> +#include <asm/mshyperv.h>
> +
> +void __init hv_vtl_init_platform(void)
> +{
> + pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
> +}
> +
> +int __init hv_vtl_early_init(void)
> +{
> + return 0;
> +}
> +early_initcall(hv_vtl_early_init);
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index 208a3bcb9686..cbde483b167a 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -96,6 +96,12 @@ static int __init hyperv_init(void)
> return ret;
> }
>
> + /* Find the VTL */
> + ms_hyperv.vtl = get_vtl();
> + if (ms_hyperv.vtl > 0) /* non default VTL */
> + hv_vtl_early_init();
Since hv_vtl_early_init() doesn't do anything on arm64, is the above
and the empty implementation of hv_vtl_early_init() really needed?
I thought maybe a subsequent patch in this series would populate
hv_vtl_early_init() to do something, but I didn't see such. I realize
the functions hv_vtl_init_platform() and hv_vtl_early_init() parallel
equivalent functions on x86, but I'd say drop hv_vtl_early_init() on
arm64 if it isn't needed.
Note too that the naming on the x86 side is arguably a bit messed
up. hv_vtl_init_platform() runs *before* hv_vtl_early_init(). But
typically in the Linux kernel, functions with "early init" in the name
run very early in boot, and that's not the case here. hv_vtl_init_platform()
is actually the function that runs very early in boot, but its name is
set up to parallel ms_hyperv_init_platform(), which calls it. On the
x86 side, I'd would argue for renaming hv_vtl_init_platform() to
hv_vtl_early_init(), and then hv_vtl_early_init() becomes hv_vtl_init().
But that's probably a separate patch. Here on arm64, perhaps all
you need is hv_vtl_init().
Michael
> +
> + hv_vtl_init_platform();
> ms_hyperv_late_init();
>
> hyperv_initialized = true;
> diff --git a/arch/arm64/include/asm/mshyperv.h
> b/arch/arm64/include/asm/mshyperv.h
> index a975e1a689dd..4a8ff6be389c 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -49,6 +49,14 @@ static inline u64 hv_get_msr(unsigned int reg)
> ARM_SMCCC_OWNER_VENDOR_HYP, \
> HV_SMCCC_FUNC_NUMBER)
>
> +#ifdef CONFIG_HYPERV_VTL_MODE
> +void __init hv_vtl_init_platform(void);
> +int __init hv_vtl_early_init(void);
> +#else
> +static inline void __init hv_vtl_init_platform(void) {}
> +static inline int __init hv_vtl_early_init(void) { return 0; }
> +#endif
> +
> #include <asm-generic/mshyperv.h>
>
> #endif
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 92bd5a55f093..ae3105375a12 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -19,7 +19,7 @@ static struct real_mode_header hv_vtl_real_mode_header;
>
> void __init hv_vtl_init_platform(void)
> {
> - pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> + pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
>
> x86_platform.realmode_reserve = x86_init_noop;
> x86_platform.realmode_init = x86_init_noop;
> --
> 2.45.0
>
From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>
> The vmbus driver uses ACPI for interrupt assignment on
> arm64 hence it won't function in the VTL mode where only
> DeviceTree can be used.
>
> Update the vmbus driver to discover interrupt configuration
> via DeviceTree.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index e25223cee3ab..52f01bd1c947 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/dma-map-ops.h>
> #include <linux/pci.h>
> +#include <linux/of_irq.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> #include "hyperv_vmbus.h"
> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> }
> #endif
>
> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
> +{
> + struct irq_desc *desc;
> + int irq;
> +
> + irq = of_irq_get(np, 0);
> + if (irq == 0) {
> + pr_err("VMBus interrupt mapping failure\n");
> + return -EINVAL;
> + }
> + if (irq < 0) {
> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
> + return irq;
> + }
> +
> + desc = irq_to_desc(irq);
> + if (!desc) {
> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
s/description/descriptor/
Or maybe slightly more compact overall: "No interrupt descriptor for VMBus virq %d\n".
> + return -ENODEV;
> + }
> +
> + vmbus_irq = irq;
> + vmbus_interrupt = desc->irq_data.hwirq;
> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
How does device DMA cache coherency get handled in the DeviceTree case on
arm64? For vmbus_acpi_add(), there's code to look at the _CCA flag, which is
required in ACPI for arm64. (There's also code to handle the Hyper-V bug where
_CCA is omitted.) I don't know DeviceTree, but is there a similar flag to indicate
device cache coherency? Of course, Hyper-V always assumes DMA cache
coherency, and that's a valid assumption for the server-class systems that
would run Hyper-V VMs on arm64. But the Linux DMA subsystem needs to be
told, and vmbus_dma_configure() needs to propagate the setting from the
VMBus device to the child VMBus devices. Everything still works if the Linux
DMA subsystem isn't told, but you end up with a perf hit. The DMA code
defaults to "not coherent" on arm64, and you'll get a lot of high-cost cache
coherency maintenance done by the CPU when it is unnecessary. This issue
doesn't arise on x86 since the architecture requires DMA cache coherency, and
the Linux default is "coherent".
> +
> + return 0;
> +}
> +
> static int vmbus_device_add(struct platform_device *pdev)
> {
> struct resource **cur_res = &hyperv_mmio;
> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> int ret;
>
> + pr_debug("VMBus is present in DeviceTree\n");
> +
I'm not clear on how interpret this debug message. Reaching this point in the code
path just means that acpi_disabled is "true". DeviceTree hasn't yet been searched to
see if VMBus is found.
> hv_dev = &pdev->dev;
>
> ret = of_range_parser_init(&parser, np);
> if (ret)
> return ret;
>
> +#ifndef HYPERVISOR_CALLBACK_VECTOR
> + ret = vmbus_of_set_irq(np);
> + if (ret)
> + return ret;
> +#endif
> +
> for_each_of_range(&parser, &range) {
> struct resource *res;
>
> --
> 2.45.0
>
From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>
> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> on arm64 thereby it won't be able to do that in the VTL mode where
> only DeviceTree can be used.
That sentence seems a bit weird. How about:
The hyperv-pci driver uses ACPI for MSI IRQ domain configuration on arm64.
It won't be able to do that in the VTL mode where only DeviceTree can be used.
>
> Update the hyperv-pci driver to discover interrupt configuration
> via DeviceTree.
"discover interrupt configuration"? I think that's a cut-and-paste error
from the previous patch.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
> include/linux/acpi.h | 9 +++++++++
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..ccc2b54206f4 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
> * way to ensure that all the corresponding devices are also gone and
> * no interrupts will be generated.
> */
> - hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> - fn, &hv_pci_domain_ops,
> - chip_data);
> + if (acpi_disabled)
> + hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> + irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
> + 0, HV_PCI_MSI_SPI_NR,
> + fn, &hv_pci_domain_ops,
> + chip_data);
Does the above really work? It seems doubtful to me that irq_find_matching_fwnode()
always finds the parent domain that you want. But I don't have a deep understanding
of how this works or is supposed to work, so I don't know for sure.
If the above *does* actually work for all cases, then should it also work for the ACPI
case? Then you could avoid the messiness when acpi_irq_create_hierarchy() doesn't
exist.
> + else
> + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> + fn, &hv_pci_domain_ops,
> + chip_data);
>
> if (!hv_msi_gic_irq_domain) {
> pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
I'm wondering if these are the only changes needed to make vPCI work on
arm64 with DeviceTree. The DMA coherence issue I mentioned in the previous patch
definitely affects vPCI devices, so it needs to be fully understood and verified to work
correctly.
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b7165e52b3c6..498cbb2c40a1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
> return false;
> }
>
> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> + unsigned int size,
> + struct fwnode_handle *fwnode,
> + const struct irq_domain_ops *ops,
> + void *host_data)
> +{
> + return NULL;
> +}
> +
> #endif /* !CONFIG_ACPI */
>
> extern void arch_post_acpi_subsys_init(void);
> --
> 2.45.0
>
On 5/15/2024 2:42 AM, Saurabh Singh Sengar wrote:
> On Tue, May 14, 2024 at 03:43:52PM -0700, Roman Kisel wrote:
>> The vmbus driver uses ACPI for interrupt assignment on
>
> In subject use the prefix "Drivers: hv: vmbus:".
> It is preferred to us "VMbus/VMBus" instead of "vmbus" for all the
> commit message and comments.
>
I can see "Drivers: hv: vmbus:" as a convention from the data[1], will
update the commit message per that.
The recent Michael Kelly's patchset where the Hyper-V documentation is
updated to use "VMBus" instead of the incorrect one (iiuc) "VMbus"
compels me to use "VMBus" out of the options you have enumerated, in the
commit description.
[1]
The data mining device was
git log --all --grep='Drivers: hv: vmbus:'
and variations thereof.
>> arm64 hence it won't function in the VTL mode where only
>> DeviceTree can be used.
>>
>> Update the vmbus driver to discover interrupt configuration
>> via DeviceTree.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index e25223cee3ab..52f01bd1c947 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -36,6 +36,7 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/dma-map-ops.h>
>> #include <linux/pci.h>
>> +#include <linux/of_irq.h>
>> #include <clocksource/hyperv_timer.h>
>> #include <asm/mshyperv.h>
>> #include "hyperv_vmbus.h"
>> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>> }
>> #endif
>>
>> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
>> +{
>> + struct irq_desc *desc;
>> + int irq;
>> +
>> + irq = of_irq_get(np, 0);
>> + if (irq == 0) {
>> + pr_err("VMBus interrupt mapping failure\n");
>> + return -EINVAL;
>> + }
>> + if (irq < 0) {
>> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
>> + return irq;
>> + }
>> +
>> + desc = irq_to_desc(irq);
>> + if (!desc) {
>> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
>> + return -ENODEV;
>> + }
>> +
>> + vmbus_irq = irq;
>> + vmbus_interrupt = desc->irq_data.hwirq;
>> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
>> +
>> + return 0;
>> +}
>> +
>> static int vmbus_device_add(struct platform_device *pdev)
>> {
>> struct resource **cur_res = &hyperv_mmio;
>> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
>> struct device_node *np = pdev->dev.of_node;
>> int ret;
>>
>> + pr_debug("VMBus is present in DeviceTree\n");
>> +
>> hv_dev = &pdev->dev;
>>
>> ret = of_range_parser_init(&parser, np);
>> if (ret)
>> return ret;
>>
>> +#ifndef HYPERVISOR_CALLBACK_VECTOR
>> + ret = vmbus_of_set_irq(np);
>> + if (ret)
>> + return ret;
>> +#endif
>> +
>> for_each_of_range(&parser, &range) {
>> struct resource *res;
>>
>> --
>> 2.45.0
>>
--
Thank you,
Roman
On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>> on arm64 thereby it won't be able to do that in the VTL mode where
>> only DeviceTree can be used.
>>
>> Update the hyperv-pci driver to discover interrupt configuration
>> via DeviceTree.
>
> Subject prefix should be "PCI: hv:"
>
Thanks!
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>> include/linux/acpi.h | 9 +++++++++
>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 1eaffff40b8d..ccc2b54206f4 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>> * way to ensure that all the corresponding devices are also gone and
>> * no interrupts will be generated.
>> */
>> - hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> - fn, &hv_pci_domain_ops,
>> - chip_data);
>> + if (acpi_disabled)
>> + hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
>> + irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
>> + 0, HV_PCI_MSI_SPI_NR,
>> + fn, &hv_pci_domain_ops,
>> + chip_data);
>> + else
>> + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> + fn, &hv_pci_domain_ops,
>> + chip_data);
>
> Upto 100 characters per line are supported now, we can have less
> line breaks.
>
Fewer line breaks would make this look nicer, let me know if you had any
particular style in mind.
>>
>> if (!hv_msi_gic_irq_domain) {
>> pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index b7165e52b3c6..498cbb2c40a1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>> return false;
>> }
>>
>> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
>> + unsigned int size,
>> + struct fwnode_handle *fwnode,
>> + const struct irq_domain_ops *ops,
>> + void *host_data)
>> +{
>> + return NULL;
>> +}
>> +
>> #endif /* !CONFIG_ACPI */
>>
>> extern void arch_post_acpi_subsys_init(void);
>> --
>> 2.45.0
>>
--
Thank you,
Roman
On 5/15/2024 12:47 AM, Krzysztof Kozlowski wrote:
> On 15/05/2024 00:43, Roman Kisel wrote:
>> The vmbus driver uses ACPI for interrupt assignment on
>> arm64 hence it won't function in the VTL mode where only
>> DeviceTree can be used.
>>
>> Update the vmbus driver to discover interrupt configuration
>> via DeviceTree.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index e25223cee3ab..52f01bd1c947 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -36,6 +36,7 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/dma-map-ops.h>
>> #include <linux/pci.h>
>> +#include <linux/of_irq.h>
>> #include <clocksource/hyperv_timer.h>
>> #include <asm/mshyperv.h>
>> #include "hyperv_vmbus.h"
>> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>> }
>> #endif
>>
>> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
>> +{
>> + struct irq_desc *desc;
>> + int irq;
>> +
>> + irq = of_irq_get(np, 0);
>
> Where is the binding for this?
>
Have not added to
Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml, my bad. Will
update the file.
>> + if (irq == 0) {
>> + pr_err("VMBus interrupt mapping failure\n");
>> + return -EINVAL;
>> + }
>> + if (irq < 0) {
>> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
>> + return irq;
>> + }
>> +
>> + desc = irq_to_desc(irq);
>> + if (!desc) {
>> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
>> + return -ENODEV;
>> + }
>> +
>> + vmbus_irq = irq;
>> + vmbus_interrupt = desc->irq_data.hwirq;
>> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
>> +
>> + return 0;
>> +}
>> +
>> static int vmbus_device_add(struct platform_device *pdev)
>> {
>> struct resource **cur_res = &hyperv_mmio;
>> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
>> struct device_node *np = pdev->dev.of_node;
>> int ret;
>>
>> + pr_debug("VMBus is present in DeviceTree\n");
>
> Not related and not really helpful. Simple entry/exit tracking is
> provided already by tracing.
>
True. Will remove.
>
> Best regards,
> Krzysztof
--
Thank you,
Roman
On 5/15/2024 12:45 AM, Krzysztof Kozlowski wrote:
> On 15/05/2024 00:43, Roman Kisel wrote:
>> The Virtual Trust Level platforms rely on DeviceTree, and the
>> arm64/hyperv code supports ACPI only. Update the logic to
>> support DeviceTree on boot as well as ACPI.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
>> 1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..208a3bcb9686 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -15,6 +15,9 @@
>> #include <linux/errno.h>
>> #include <linux/version.h>
>> #include <linux/cpuhotplug.h>
>> +#include <linux/libfdt.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> #include <asm/mshyperv.h>
>>
>> static bool hyperv_initialized;
>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>> return 0;
>> }
>>
>> +static bool hyperv_detect_fdt(void)
>> +{
>> +#ifdef CONFIG_OF
>> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>> + of_get_flat_dt_root(), "hypervisor");
>
> Why do you add an ABI for node name? Although name looks OK, but is it
> really described in the spec that you depend on it? I really do not like
> name dependencies...
Followed the existing DeviceTree's of naming and approaches in the
kernel to surprise less and "invent" even less. As for the spec, the
Hyper-V TLFS'es part discussing Hypervisor Discovery talks about x86
(https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/feature-discovery)
only and via the ISA-provided means only. For arm64, Hyper-V code
discovers the hypervisor presence via ACPI. Felt only natural to do the
same for DeviceTree and arm64.
>
> Where is the binding for this?
>
Have not added, my mistake. Will place under
Documentation/devicetree/bindings/bus/microsoft,hyperv.yaml
> Best regards,
> Krzysztof
--
Thank you,
Roman
On 5/15/2024 6:37 AM, Michael Kelley wrote:
> From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> Kconfig dependencies for arm64 guests on Hyper-V require that be ACPI enabled,
>> and limit VTL mode to x86/x64. To enable VTL mode on arm64 as well, update the
>> dependencies. Since VTL mode requires DeviceTree instead of ACPI, don't require
>> arm64 guests on Hyper-V to have ACPI.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/hv/Kconfig | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
>> index 862c47b191af..a5cd1365e248 100644
>> --- a/drivers/hv/Kconfig
>> +++ b/drivers/hv/Kconfig
>> @@ -5,7 +5,7 @@ menu "Microsoft Hyper-V guest support"
>> config HYPERV
>> tristate "Microsoft Hyper-V client drivers"
>> depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \
>> - || (ACPI && ARM64 && !CPU_BIG_ENDIAN)
>> + || (ARM64 && !CPU_BIG_ENDIAN)
>> select PARAVIRT
>> select X86_HV_CALLBACK_VECTOR if X86
>> select OF_EARLY_FLATTREE if OF
>> @@ -15,7 +15,7 @@ config HYPERV
>>
>> config HYPERV_VTL_MODE
>> bool "Enable Linux to boot in VTL context"
>> - depends on X86_64 && HYPERV
>> + depends on HYPERV
>> depends on SMP
>> default n
>> help
>
> These changes make it possible to build a normal VTL 0 Hyper-V
> guest (i.e., CONFIG_HYPERV_VTL_MODE=n) if CONFIG_ACPI is
> not set, which won't work. While we can say "don't do that", it
> would be better if the Kconfig dependencies expressed that
> requirement.
>
> A possible fix is to remove the "depends on HYPERV" from
> HYPERV_VTL_MODE. Then for HYPERV, make
> the "depends on ACPI" be conditional on !HYPERV_VTL_MODE
> (for both ARM64 and X86).
>
> I think we originally had "depends on HYPERV" in
> HYPERV_VTL_MODE because there was a VTL-related function
> in a non-Hyper-V code path, and we wanted to prevent that code
> from running in non-Hyper-V environments. But in practice, that
> turned out not to work well because occasionally people would
> do an "all config" build where both CONFIG_HYPERV and
> CONFIG_HYPERV_VTL_MODE were set, and it would panic during
> boot in their non-Hyper-V environment. Such people were not
> happy. :-( So Saurabh made a relatively simple change (see commit
> 14058f72cf13e) that got the VTL code out of that non-Hyper-V code
> path. With that change, it shouldn't matter if someone sets
> CONFIG_HYPERV_VTL_MODE=y in a build where
> CONFIG_HYPERV=n.
>
> At least that's my theory. :-) Someone would need to check
> it carefully.
I'll explore that, appreciate sharing the context!
>
> Michael
--
Thank you,
Roman
On 5/15/2024 6:38 AM, Michael Kelley wrote:
> From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> To run in the VTL mode, Hyper-V drivers have to know what
>> VTL the system boots in, and the arm64/hyperv code does not
>> have the means to compute that.
>>
>> Refactor the code to hoist the function that detects VTL,
>> make it arch-neutral to be able to employ it to get the VTL
>> on arm64.
>
> Nit: In patches that just refactor and move some code around,
> patch authors will often include a statement like "No functional
> changes" (or the slightly more doubtful "No functional changes
> intended") as a separate line at the end of the commit message.
> It's just something the reader/reviewer can cross-check against.
>
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> arch/x86/hyperv/hv_init.c | 34 -----------------------
>> arch/x86/include/asm/hyperv-tlfs.h | 7 -----
>> drivers/hv/hv_common.c | 43 ++++++++++++++++++++++++++++++
>> include/asm-generic/hyperv-tlfs.h | 7 +++++
>> include/asm-generic/mshyperv.h | 6 +++++
>> 5 files changed, 56 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 17a71e92a343..c350fa05ee59 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -413,40 +413,6 @@ static void __init hv_get_partition_id(void)
>> local_irq_restore(flags);
>> }
>>
>> -#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> -static u8 __init get_vtl(void)
>> -{
>> - u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>> - struct hv_get_vp_registers_input *input;
>> - struct hv_get_vp_registers_output *output;
>> - unsigned long flags;
>> - u64 ret;
>> -
>> - local_irq_save(flags);
>> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> - output = (struct hv_get_vp_registers_output *)input;
>> -
>> - memset(input, 0, struct_size(input, element, 1));
>> - input->header.partitionid = HV_PARTITION_ID_SELF;
>> - input->header.vpindex = HV_VP_INDEX_SELF;
>> - input->header.inputvtl = 0;
>> - input->element[0].name0 = HV_X64_REGISTER_VSM_VP_STATUS;
>> -
>> - ret = hv_do_hypercall(control, input, output);
>> - if (hv_result_success(ret)) {
>> - ret = output->as64.low & HV_X64_VTL_MASK;
>> - } else {
>> - pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>> - BUG();
>> - }
>> -
>> - local_irq_restore(flags);
>> - return ret;
>> -}
>> -#else
>> -static inline u8 get_vtl(void) { return 0; }
>> -#endif
>> -
>> /*
>> * This function is to be invoked early in the boot sequence after the
>> * hypervisor has been detected.
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index 3787d26810c1..9ee68eb8e6ff 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -309,13 +309,6 @@ enum hv_isolation_type {
>> #define HV_MSR_STIMER0_CONFIG (HV_X64_MSR_STIMER0_CONFIG)
>> #define HV_MSR_STIMER0_COUNT (HV_X64_MSR_STIMER0_COUNT)
>>
>> -/*
>> - * Registers are only accessible via HVCALL_GET_VP_REGISTERS hvcall and
>> - * there is not associated MSR address.
>> - */
>> -#define HV_X64_REGISTER_VSM_VP_STATUS 0x000D0003
>> -#define HV_X64_VTL_MASK GENMASK(3, 0)
>> -
>> /* Hyper-V memory host visibility */
>> enum hv_mem_host_visibility {
>> VMBUS_PAGE_NOT_VISIBLE = 0,
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index dde3f9b6871a..d4cf477a4d0c 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -660,3 +660,46 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64
>> param2)
>> return HV_STATUS_INVALID_PARAMETER;
>> }
>> EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
>> +
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void)
>> +{
>> + u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
>> + struct hv_get_vp_registers_input *input;
>> + struct hv_get_vp_registers_output *output;
>> + unsigned long flags;
>> + u64 ret;
>> +
>> + local_irq_save(flags);
>> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> + output = (struct hv_get_vp_registers_output *)input;
>
> I gave some bad advice on the original version of the above code. Rather
> than allocating a separate hypercall output area, I had suggested just setting
> the hypercall output to be the same as the input area, which is done
> above. While the overlap doesn't cause any problems for a hypercall returning
> the value of single register, the TLFS says to never set up such an overlap.
>
> Since this code is being moved anyway, there's an opportunity to fix
> this by setting output to "input" + "struct_size(input, element, 1)". Or
> put the output at the start of the second half of the page (note that the
> size is HY_HYP_PAGE_SIZE, not PAGE_SIZE). The input and output for 1
> register are relatively small and both easily fit with either approach. They
> just shouldn't overlap.
>
> Some might argue this tweak should be made in a separate patch, but
> in my judgment, it's OK to do it here if you note it in the commit
> message. Your call. Of course, if you make this change, my previous
> comment about "No functional changes" no longer applies. :-)
I'll update the code to adhere to the TLFS requirements, thank you :)
>
> Michael
>
>> +
>> + memset(input, 0, struct_size(input, element, 1));
>> + input->header.partitionid = HV_PARTITION_ID_SELF;
>> + input->header.vpindex = HV_VP_INDEX_SELF;
>> + input->header.inputvtl = 0;
>> + input->element[0].name0 = HV_REGISTER_VSM_VP_STATUS;
>> +
>> + ret = hv_do_hypercall(control, input, output);
>> + if (hv_result_success(ret)) {
>> + ret = output->as64.low & HV_VTL_MASK;
>> + } else {
>> + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
>> +
>> + /*
>> + * This is a dead end, something fundamental is broken.
>> + *
>> + * There is no sensible way of continuing as the Hyper-V drivers
>> + * transitively depend via the vmbus driver on knowing which VTL
>> + * they run in to establish communication with the host. The kernel
>> + * is going to be worse off if continued booting than a panicked one,
>> + * just hung and stuck, producing second-order failures, with neither
>> + * a way to recover nor to provide expected services.
>> + */
>> + BUG();
>> + }
>> +
>> + local_irq_restore(flags);
>> + return ret;
>> +}
>> +#endif
>> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
>> index 87e3d49a4e29..682bcda3124f 100644
>> --- a/include/asm-generic/hyperv-tlfs.h
>> +++ b/include/asm-generic/hyperv-tlfs.h
>> @@ -75,6 +75,13 @@
>> /* AccessTscInvariantControls privilege */
>> #define HV_ACCESS_TSC_INVARIANT BIT(15)
>>
>> +/*
>> + * This synthetic register is only accessible via the HVCALL_GET_VP_REGISTERS
>> + * hvcall, and there is no an associated MSR on x86.
>> + */
>> +#define HV_REGISTER_VSM_VP_STATUS 0x000D0003
>> +#define HV_VTL_MASK GENMASK(3, 0)
>> +
>> /*
>> * Group B features.
>> */
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 99935779682d..ea434186d765 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -301,4 +301,10 @@ static inline enum hv_isolation_type
>> hv_get_isolation_type(void)
>> }
>> #endif /* CONFIG_HYPERV */
>>
>> +#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>> +u8 __init get_vtl(void);
>> +#else
>> +static inline u8 get_vtl(void) { return 0; }
>> +#endif
>> +
>> #endif
>> --
>> 2.45.0
>>
--
Thank you,
Roman
On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
>
>
> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> > On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> > > The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> > > on arm64 thereby it won't be able to do that in the VTL mode where
> > > only DeviceTree can be used.
> > >
> > > Update the hyperv-pci driver to discover interrupt configuration
> > > via DeviceTree.
> >
> > Subject prefix should be "PCI: hv:"
> >
> Thanks!
"git log --oneline <file>" is a good guide in general and could be
used for other patches in this series as well.
> > > + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> > > + fn, &hv_pci_domain_ops,
> > > + chip_data);
> >
> > Upto 100 characters per line are supported now, we can have less
> > line breaks.
> >
> Fewer line breaks would make this look nicer, let me know if you had any
> particular style in mind.
Let's not use the checkpatch "$max_line_length = 100" as a guide.
The pci-hyperv.c file as a whole is obviously formatted to fit in 80
columns with few exceptions.
IMO it would not be an improvement to scatter random 100-column lines
throughout. That would just mean the file would look bad in an
80-column terminal and there would be a lot of wasted space in a
100-column terminal.
Bjorn
On 5/15/2024 6:39 AM, Michael Kelley wrote:
> From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> To run in the VTL mode, Hyper-V drivers have to know what
>> VTL the system boots in, and the arm64/hyperv code does not
>> update the variable that stores the value.
>>
>> Update the variable to enable the Hyper-V drivers to boot
>> in the VTL mode and print the VTL the code runs in.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> arch/arm64/hyperv/Makefile | 1 +
>> arch/arm64/hyperv/hv_vtl.c | 19 +++++++++++++++++++
>> arch/arm64/hyperv/mshyperv.c | 6 ++++++
>> arch/arm64/include/asm/mshyperv.h | 8 ++++++++
>> arch/x86/hyperv/hv_vtl.c | 2 +-
>> 5 files changed, 35 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm64/hyperv/hv_vtl.c
>>
>> diff --git a/arch/arm64/hyperv/Makefile b/arch/arm64/hyperv/Makefile
>> index 87c31c001da9..9701a837a6e1 100644
>> --- a/arch/arm64/hyperv/Makefile
>> +++ b/arch/arm64/hyperv/Makefile
>> @@ -1,2 +1,3 @@
>> # SPDX-License-Identifier: GPL-2.0
>> obj-y := hv_core.o mshyperv.o
>> +obj-$(CONFIG_HYPERV_VTL_MODE) += hv_vtl.o
>> diff --git a/arch/arm64/hyperv/hv_vtl.c b/arch/arm64/hyperv/hv_vtl.c
>> new file mode 100644
>> index 000000000000..9b44cc49594c
>> --- /dev/null
>> +++ b/arch/arm64/hyperv/hv_vtl.c
>> @@ -0,0 +1,19 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023, Microsoft, Inc.
>> + *
>> + * Author : Roman Kisel <[email protected]>
>> + */
>> +
>> +#include <asm/mshyperv.h>
>> +
>> +void __init hv_vtl_init_platform(void)
>> +{
>> + pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
>> +}
>> +
>> +int __init hv_vtl_early_init(void)
>> +{
>> + return 0;
>> +}
>> +early_initcall(hv_vtl_early_init);
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index 208a3bcb9686..cbde483b167a 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -96,6 +96,12 @@ static int __init hyperv_init(void)
>> return ret;
>> }
>>
>> + /* Find the VTL */
>> + ms_hyperv.vtl = get_vtl();
>> + if (ms_hyperv.vtl > 0) /* non default VTL */
>> + hv_vtl_early_init();
>
> Since hv_vtl_early_init() doesn't do anything on arm64, is the above
> and the empty implementation of hv_vtl_early_init() really needed?
> I thought maybe a subsequent patch in this series would populate
> hv_vtl_early_init() to do something, but I didn't see such. I realize
> the functions hv_vtl_init_platform() and hv_vtl_early_init() parallel
> equivalent functions on x86, but I'd say drop hv_vtl_early_init() on
> arm64 if it isn't needed.
>
> Note too that the naming on the x86 side is arguably a bit messed
> up. hv_vtl_init_platform() runs *before* hv_vtl_early_init(). But
> typically in the Linux kernel, functions with "early init" in the name
> run very early in boot, and that's not the case here. hv_vtl_init_platform()
> is actually the function that runs very early in boot, but its name is
> set up to parallel ms_hyperv_init_platform(), which calls it. On the
> x86 side, I'd would argue for renaming hv_vtl_init_platform() to
> hv_vtl_early_init(), and then hv_vtl_early_init() becomes hv_vtl_init().
> But that's probably a separate patch. Here on arm64, perhaps all
> you need is hv_vtl_init().
>
I'll clean that up, appreciate the thorough explanation!
> Michael
>
>> +
>> + hv_vtl_init_platform();
>> ms_hyperv_late_init();
>>
>> hyperv_initialized = true;
>> diff --git a/arch/arm64/include/asm/mshyperv.h
>> b/arch/arm64/include/asm/mshyperv.h
>> index a975e1a689dd..4a8ff6be389c 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -49,6 +49,14 @@ static inline u64 hv_get_msr(unsigned int reg)
>> ARM_SMCCC_OWNER_VENDOR_HYP, \
>> HV_SMCCC_FUNC_NUMBER)
>>
>> +#ifdef CONFIG_HYPERV_VTL_MODE
>> +void __init hv_vtl_init_platform(void);
>> +int __init hv_vtl_early_init(void);
>> +#else
>> +static inline void __init hv_vtl_init_platform(void) {}
>> +static inline int __init hv_vtl_early_init(void) { return 0; }
>> +#endif
>> +
>> #include <asm-generic/mshyperv.h>
>>
>> #endif
>> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
>> index 92bd5a55f093..ae3105375a12 100644
>> --- a/arch/x86/hyperv/hv_vtl.c
>> +++ b/arch/x86/hyperv/hv_vtl.c
>> @@ -19,7 +19,7 @@ static struct real_mode_header hv_vtl_real_mode_header;
>>
>> void __init hv_vtl_init_platform(void)
>> {
>> - pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
>> + pr_info("Linux runs in Hyper-V Virtual Trust Level %d\n", ms_hyperv.vtl);
>>
>> x86_platform.realmode_reserve = x86_init_noop;
>> x86_platform.realmode_init = x86_init_noop;
>> --
>> 2.45.0
>>
--
Thank you,
Roman
On 5/15/2024 6:44 AM, Michael Kelley wrote:
> From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> The vmbus driver uses ACPI for interrupt assignment on
>> arm64 hence it won't function in the VTL mode where only
>> DeviceTree can be used.
>>
>> Update the vmbus driver to discover interrupt configuration
>> via DeviceTree.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index e25223cee3ab..52f01bd1c947 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -36,6 +36,7 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/dma-map-ops.h>
>> #include <linux/pci.h>
>> +#include <linux/of_irq.h>
>> #include <clocksource/hyperv_timer.h>
>> #include <asm/mshyperv.h>
>> #include "hyperv_vmbus.h"
>> @@ -2316,6 +2317,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>> }
>> #endif
>>
>> +static int __maybe_unused vmbus_of_set_irq(struct device_node *np)
>> +{
>> + struct irq_desc *desc;
>> + int irq;
>> +
>> + irq = of_irq_get(np, 0);
>> + if (irq == 0) {
>> + pr_err("VMBus interrupt mapping failure\n");
>> + return -EINVAL;
>> + }
>> + if (irq < 0) {
>> + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq);
>> + return irq;
>> + }
>> +
>> + desc = irq_to_desc(irq);
>> + if (!desc) {
>> + pr_err("VMBus interrupt description can't be found for virq %d\n", irq);
>
> s/description/descriptor/
>
> Or maybe slightly more compact overall: "No interrupt descriptor for VMBus virq %d\n".
>
Yep, thanks!
>> + return -ENODEV;
>> + }
>> +
>> + vmbus_irq = irq;
>> + vmbus_interrupt = desc->irq_data.hwirq;
>> + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt);
>
> How does device DMA cache coherency get handled in the DeviceTree case on
> arm64? For vmbus_acpi_add(), there's code to look at the _CCA flag, which is
> required in ACPI for arm64. (There's also code to handle the Hyper-V bug where
> _CCA is omitted.) I don't know DeviceTree, but is there a similar flag to indicate
> device cache coherency? Of course, Hyper-V always assumes DMA cache
> coherency, and that's a valid assumption for the server-class systems that
> would run Hyper-V VMs on arm64. But the Linux DMA subsystem needs to be
> told, and vmbus_dma_configure() needs to propagate the setting from the
> VMBus device to the child VMBus devices. Everything still works if the Linux
> DMA subsystem isn't told, but you end up with a perf hit. The DMA code
> defaults to "not coherent" on arm64, and you'll get a lot of high-cost cache
> coherency maintenance done by the CPU when it is unnecessary. This issue
> doesn't arise on x86 since the architecture requires DMA cache coherency, and
> the Linux default is "coherent".
>
Appreciate the indispensable insight! I'll straighten this out.
>> +
>> + return 0;
>> +}
>> +
>> static int vmbus_device_add(struct platform_device *pdev)
>> {
>> struct resource **cur_res = &hyperv_mmio;
>> @@ -2324,12 +2353,20 @@ static int vmbus_device_add(struct platform_device *pdev)
>> struct device_node *np = pdev->dev.of_node;
>> int ret;
>>
>> + pr_debug("VMBus is present in DeviceTree\n");
>> +
>
> I'm not clear on how interpret this debug message. Reaching this point in the code
> path just means that acpi_disabled is "true". DeviceTree hasn't yet been searched to
> see if VMBus is found.
>
True. Will remove.
>> hv_dev = &pdev->dev;
>>
>> ret = of_range_parser_init(&parser, np);
>> if (ret)
>> return ret;
>>
>> +#ifndef HYPERVISOR_CALLBACK_VECTOR
>> + ret = vmbus_of_set_irq(np);
>> + if (ret)
>> + return ret;
>> +#endif
>> +
>> for_each_of_range(&parser, &range) {
>> struct resource *res;
>>
>> --
>> 2.45.0
>>
>
--
Thank you,
Roman
On 5/15/2024 6:47 AM, Michael Kelley wrote:
> From: Roman Kisel <[email protected]> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>> on arm64 thereby it won't be able to do that in the VTL mode where
>> only DeviceTree can be used.
>
> That sentence seems a bit weird. How about:
>
> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration on arm64.
> It won't be able to do that in the VTL mode where only DeviceTree can be used.
>
Agreed, appreciate your wordsmithing :)
>>
>> Update the hyperv-pci driver to discover interrupt configuration
>> via DeviceTree.
>
> "discover interrupt configuration"? I think that's a cut-and-paste error
> from the previous patch.
>
Guilty as charged :) Will fix.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>> include/linux/acpi.h | 9 +++++++++
>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 1eaffff40b8d..ccc2b54206f4 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>> * way to ensure that all the corresponding devices are also gone and
>> * no interrupts will be generated.
>> */
>> - hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> - fn, &hv_pci_domain_ops,
>> - chip_data);
>> + if (acpi_disabled)
>> + hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
>> + irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
>> + 0, HV_PCI_MSI_SPI_NR,
>> + fn, &hv_pci_domain_ops,
>> + chip_data);
>
> Does the above really work? It seems doubtful to me that irq_find_matching_fwnode()
> always finds the parent domain that you want. But I don't have a deep understanding
> of how this works or is supposed to work, so I don't know for sure.
>
> If the above *does* actually work for all cases, then should it also work for the ACPI
> case? Then you could avoid the messiness when acpi_irq_create_hierarchy() doesn't
> exist.
>
Have not got a system to validate this on. Conceptually (at my level of
ignorance) didn't look very off... Will use the "collapsed" version
you're suggesting as the litmus test.
>> + else
>> + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> + fn, &hv_pci_domain_ops,
>> + chip_data);
>>
>> if (!hv_msi_gic_irq_domain) {
>> pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
>
> I'm wondering if these are the only changes needed to make vPCI work on
> arm64 with DeviceTree. The DMA coherence issue I mentioned in the previous patch
> definitely affects vPCI devices, so it needs to be fully understood and verified to work
> correctly.
>
Likely not all as the code is venturing into the new territory composed
of the pieces never snapped in place together before. Will work on the
previous patch to resolve as many concerns as possible.
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index b7165e52b3c6..498cbb2c40a1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>> return false;
>> }
>>
>> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
>> + unsigned int size,
>> + struct fwnode_handle *fwnode,
>> + const struct irq_domain_ops *ops,
>> + void *host_data)
>> +{
>> + return NULL;
>> +}
>> +
>> #endif /* !CONFIG_ACPI */
>>
>> extern void arch_post_acpi_subsys_init(void);
>> --
>> 2.45.0
>>
--
Thank you,
Roman
On 5/15/2024 11:12 AM, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
>>
>>
>> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
>>> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>>>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>>>> on arm64 thereby it won't be able to do that in the VTL mode where
>>>> only DeviceTree can be used.
>>>>
>>>> Update the hyperv-pci driver to discover interrupt configuration
>>>> via DeviceTree.
>>>
>>> Subject prefix should be "PCI: hv:"
>>>
>> Thanks!
>
> "git log --oneline <file>" is a good guide in general and could be
> used for other patches in this series as well.
>
Many thanks for suggesting that :)
>>>> + hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>>>> + fn, &hv_pci_domain_ops,
>>>> + chip_data);
>>>
>>> Upto 100 characters per line are supported now, we can have less
>>> line breaks.
>>>
>> Fewer line breaks would make this look nicer, let me know if you had any
>> particular style in mind.
>
> Let's not use the checkpatch "$max_line_length = 100" as a guide.
>
> The pci-hyperv.c file as a whole is obviously formatted to fit in 80
> columns with few exceptions.
>
> IMO it would not be an improvement to scatter random 100-column lines
> throughout. That would just mean the file would look bad in an
> 80-column terminal and there would be a lot of wasted space in a
> 100-column terminal.
>
Appreciate showing me the data-driven way of reasoning about that very much!
> Bjorn
--
Thank you,
Roman
On Tue, May 14, 2024 at 03:43:48PM -0700, Roman Kisel wrote:
> The Virtual Trust Level platforms rely on DeviceTree, and the
> arm64/hyperv code supports ACPI only. Update the logic to
> support DeviceTree on boot as well as ACPI.
Could you use Call UID query from SMCCC? KVM [1] and Gunyah [2] have
been using this to identify if guest is running under those respective
hypervisors. This works in both DT and ACPI cases.
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..208a3bcb9686 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -15,6 +15,9 @@
> #include <linux/errno.h>
> #include <linux/version.h>
> #include <linux/cpuhotplug.h>
> +#include <linux/libfdt.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> #include <asm/mshyperv.h>
>
> static bool hyperv_initialized;
> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
> return 0;
> }
>
> +static bool hyperv_detect_fdt(void)
> +{
> +#ifdef CONFIG_OF
> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
> + of_get_flat_dt_root(), "hypervisor");
> +
> + return (hyp_node != -FDT_ERR_NOTFOUND) &&
> + of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
> +#else
> + return false;
> +#endif
> +}
> +
> +static bool hyperv_detect_acpi(void)
> +{
> +#ifdef CONFIG_ACPI
> + return !acpi_disabled &&
> + !strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8);
> +#else
> + return false;
> +#endif
> +}
> +
> static int __init hyperv_init(void)
> {
> struct hv_get_vp_registers_output result;
> @@ -35,13 +61,11 @@ static int __init hyperv_init(void)
>
> /*
> * Allow for a kernel built with CONFIG_HYPERV to be running in
> - * a non-Hyper-V environment, including on DT instead of ACPI.
> + * a non-Hyper-V environment.
> + *
> * In such cases, do nothing and return success.
> */
> - if (acpi_disabled)
> - return 0;
> -
> - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
> + if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
> return 0;
>
> /* Setup the guest ID */
> --
> 2.45.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Roman,
kernel test robot noticed the following build errors:
[auto build test ERROR on f2580a907e5c0e8fc9354fd095b011301c64f949]
url: https://github.com/intel-lab-lkp/linux/commits/Roman-Kisel/arm64-hyperv-Support-DeviceTree/20240515-064749
base: f2580a907e5c0e8fc9354fd095b011301c64f949
patch link: https://lore.kernel.org/r/20240514224508.212318-6-romank%40linux.microsoft.com
patch subject: [PATCH v2 5/6] drivers/hv/vmbus: Get the irq number from DeviceTree
config: arm64-randconfig-r053-20240515 (https://download.01.org/0day-ci/archive/20240516/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d3455f4ddd16811401fa153298fadd2f59f6914e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240516/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt7622-hif.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt7622-aud.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8186-img.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8186-ipe.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8186-mdp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8186-vdec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-img.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-ipe.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-vdec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-vdo0.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-vdo1.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-venc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-vpp0.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8188-vpp1.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-apusys_pll.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-cam.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-ccu.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-img.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-ipe.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-scp_adsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-vdec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-vdo0.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-vdo1.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-vpp0.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-vpp1.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/mediatek/clk-mt8195-wpe.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/meson-clkc-utils.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/meson-aoclk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/meson-eeclk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/a1-pll.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/gxbb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/gxbb-aoclk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/meson/s4-peripherals.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/qcom/lpass-gfm-sm8250.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/qcom/videocc-sdm845.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/versatile/clk-vexpress-osc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/clk/clk-gate_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/soc/amlogic/meson-clk-measure.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/hw_random/omap-rng.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/hw_random/omap3-rom-rng.o
WARNING: modpost: drivers/char/hw_random/mxc-rnga: section mismatch in reference: mxc_rnga_driver+0x10 (section: .data) -> mxc_rnga_remove (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/hw_random/cavium-rng.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/hw_random/cavium-rng-vf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/panel/panel-abt-y030xx067a.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/bridge/lontium-lt9611uxc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/bridge/sii9234.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tiny/bochs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-kunit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-ram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-raw-ram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spmi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/arizona.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/vexpress-sysreg.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/dax.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cxl/core/cxl_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cxl/cxl_acpi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cxl/cxl_port.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spi/spi-fsl-lib.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firewire/firewire-uapi-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/auxdisplay/hd44780_common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/i82092.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/misc/soc_button_array.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/tests/input_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rtc/lib_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-ccgx-ucsi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/dvb-frontends/au8522_decoder.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-async.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-fwnode.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/pwrseq_emmc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/host/tmio_mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/flash/leds-rt4505.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-belkin.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-evision.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-google-stadiaff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gyration.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-kensington.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-kye.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-microsoft.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-saitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sjoy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-speedlink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-topseed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-twinhan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-xinmo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zpff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zydacron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_powersave.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/arm-ccn.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/fsl_imx8_ddr_perf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/perf/marvell_cn10k_ddr_pmu.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/spmi-pmic-arb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/xilinx-ams.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/buffer/kfifo_buf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/siox/siox-bus-gpio.o
>> ERROR: modpost: "irq_to_desc" [drivers/hv/hv_vmbus.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 5/15/2024 3:02 PM, Elliot Berman wrote:
> On Tue, May 14, 2024 at 03:43:48PM -0700, Roman Kisel wrote:
>> The Virtual Trust Level platforms rely on DeviceTree, and the
>> arm64/hyperv code supports ACPI only. Update the logic to
>> support DeviceTree on boot as well as ACPI.
>
> Could you use Call UID query from SMCCC? KVM [1] and Gunyah [2] have
> been using this to identify if guest is running under those respective
> hypervisors. This works in both DT and ACPI cases.
>
> [1]: https://lore.kernel.org/all/[email protected]/
> [2]: https://lore.kernel.org/all/[email protected]/
That would be very neat indeed, thanks! Talking to the hypervisor folks.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
>> 1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..208a3bcb9686 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -15,6 +15,9 @@
>> #include <linux/errno.h>
>> #include <linux/version.h>
>> #include <linux/cpuhotplug.h>
>> +#include <linux/libfdt.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> #include <asm/mshyperv.h>
>>
>> static bool hyperv_initialized;
>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>> return 0;
>> }
>>
>> +static bool hyperv_detect_fdt(void)
>> +{
>> +#ifdef CONFIG_OF
>> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>> + of_get_flat_dt_root(), "hypervisor");
>> +
>> + return (hyp_node != -FDT_ERR_NOTFOUND) &&
>> + of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> +static bool hyperv_detect_acpi(void)
>> +{
>> +#ifdef CONFIG_ACPI
>> + return !acpi_disabled &&
>> + !strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8);
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> static int __init hyperv_init(void)
>> {
>> struct hv_get_vp_registers_output result;
>> @@ -35,13 +61,11 @@ static int __init hyperv_init(void)
>>
>> /*
>> * Allow for a kernel built with CONFIG_HYPERV to be running in
>> - * a non-Hyper-V environment, including on DT instead of ACPI.
>> + * a non-Hyper-V environment.
>> + *
>> * In such cases, do nothing and return success.
>> */
>> - if (acpi_disabled)
>> - return 0;
>> -
>> - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
>> + if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
>> return 0;
>>
>> /* Setup the guest ID */
>> --
>> 2.45.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Thank you,
Roman
On Tue, May 14, 2024 at 5:45 PM Roman Kisel <[email protected]> wrote:
>
> The vmbus driver uses ACPI for interrupt assignment on
> arm64 hence it won't function in the VTL mode where only
> DeviceTree can be used.
>
> Update the vmbus driver to discover interrupt configuration
> via DeviceTree.
>
> Signed-off-by: Roman Kisel <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index e25223cee3ab..52f01bd1c947 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/dma-map-ops.h>
> #include <linux/pci.h>
> +#include <linux/of_irq.h>
If you are using this header in a driver, you are doing it wrong. We
have common functions which work on both ACPI or DT, so use them if
you have a need to support both.
Though my first question on a binding will be the same as on every
'hypervisor binding'. Why can't you make your hypervisor interfaces
discoverable? It's all s/w, not some h/w device which is fixed.
Rob
On 15/05/2024 19:33, Roman Kisel wrote:
>>> static bool hyperv_initialized;
>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>> return 0;
>>> }
>>>
>>> +static bool hyperv_detect_fdt(void)
>>> +{
>>> +#ifdef CONFIG_OF
>>> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>> + of_get_flat_dt_root(), "hypervisor");
>>
>> Why do you add an ABI for node name? Although name looks OK, but is it
>> really described in the spec that you depend on it? I really do not like
>> name dependencies...
>
> Followed the existing DeviceTree's of naming and approaches in the
> kernel to surprise less and "invent" even less. As for the spec, the
I am sorry, but there is no approved existing approach of adding ABI for
node names. There are exceptions or specific cases, but that's not
"invent less" approach. ABI is defined by compatible.
Best regards,
Krzysztof
On 5/17/2024 10:14 AM, Rob Herring wrote:
> On Tue, May 14, 2024 at 5:45 PM Roman Kisel <[email protected]> wrote:
>>
>> The vmbus driver uses ACPI for interrupt assignment on
>> arm64 hence it won't function in the VTL mode where only
>> DeviceTree can be used.
>>
>> Update the vmbus driver to discover interrupt configuration
>> via DeviceTree.
>>
>> Signed-off-by: Roman Kisel <[email protected]>
>> ---
>> drivers/hv/vmbus_drv.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index e25223cee3ab..52f01bd1c947 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -36,6 +36,7 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/dma-map-ops.h>
>> #include <linux/pci.h>
>> +#include <linux/of_irq.h>
>
> If you are using this header in a driver, you are doing it wrong. We
> have common functions which work on both ACPI or DT, so use them if
> you have a need to support both.
>
Understood, thank you! I'll look more for the examples. If you happen to
have in mind the place where the idiomatic/more preferred approach is
used, please let me know, would owe you a great debt of gratitude.
> Though my first question on a binding will be the same as on every
> 'hypervisor binding'. Why can't you make your hypervisor interfaces
> discoverable? It's all s/w, not some h/w device which is fixed.
>
I've taken a look at the related art. AWS's Firecracker, Intel's Cloud
Hypervisor, Google's CrosVM, QEmu allow the guest use the
well-established battle-tested generic approaches (ACPI,
DeviceTree/OpenFirmware) of describing the virtual hardware and its
resources rather than making the guest use their own specific
interfaces. That holds true for the s/w devices like
"vcpu-stall-detector" and VirtIO that do not have counterparts built as
hardware, too.
Here, the guest needs to set up VMBus (the intra-partition communication
transport) to be able to talk to the host partition. Receiving a message
needs an interrupt service routine attached to the interrupt injected
into the guest virtual processor, and DeviceTree lets provide the
interrupt number. If a custom interface were used here, it'd look less
expected due to others relying on ACPI and DT for configuring virtual
devices and busses. A specialized interface would add more code (new
code) instead of relying on the approach that is widely used.
> Rob
--
Thank you,
Roman
On 5/19/2024 11:45 PM, Krzysztof Kozlowski wrote:
> On 15/05/2024 19:33, Roman Kisel wrote:
>>>> static bool hyperv_initialized;
>>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>>> return 0;
>>>> }
>>>>
>>>> +static bool hyperv_detect_fdt(void)
>>>> +{
>>>> +#ifdef CONFIG_OF
>>>> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>>> + of_get_flat_dt_root(), "hypervisor");
>>>
>>> Why do you add an ABI for node name? Although name looks OK, but is it
>>> really described in the spec that you depend on it? I really do not like
>>> name dependencies...
>>
>> Followed the existing DeviceTree's of naming and approaches in the
>> kernel to surprise less and "invent" even less. As for the spec, the
>
> I am sorry, but there is no approved existing approach of adding ABI for
> node names. There are exceptions or specific cases, but that's not
> "invent less" approach. ABI is defined by compatible.
I should check on the compatible instead of adding a node that is not
mentioned in the DeviceTree spec as I understand. Appreciate your help!
>
> Best regards,
> Krzysztof
>
--
Thank you,
Roman
On Wed, May 15, 2024 at 01:12:38PM -0500, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
> >
> >
> > On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> > > On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> > > > The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> > > > on arm64 thereby it won't be able to do that in the VTL mode where
> > > > only DeviceTree can be used.
> > > >
> > > > Update the hyperv-pci driver to discover interrupt configuration
> > > > via DeviceTree.
> > >
> > > Subject prefix should be "PCI: hv:"
I forgot to also suggest that the subject line begin with a verb,
e.g., "Get vPCI MSI IRQ domain from DT" or similar, again so it reads
consistently with previous commits.
Oh, I see patch 5/6, "Get the irq number from DeviceTree" is also very
similar. It would be nice if they matched, e.g., both used "IRQ" and
"DT".
Bjorn
On 5/16/2024 8:27 AM, Roman Kisel wrote:
>
>
> On 5/15/2024 3:02 PM, Elliot Berman wrote:
>> On Tue, May 14, 2024 at 03:43:48PM -0700, Roman Kisel wrote:
>>> The Virtual Trust Level platforms rely on DeviceTree, and the
>>> arm64/hyperv code supports ACPI only. Update the logic to
>>> support DeviceTree on boot as well as ACPI.
>>
>> Could you use Call UID query from SMCCC? KVM [1] and Gunyah [2] have
>> been using this to identify if guest is running under those respective
>> hypervisors. This works in both DT and ACPI cases.
>>
>> [1]: https://lore.kernel.org/all/[email protected]/
>> [2]:
>> https://lore.kernel.org/all/[email protected]/
>
> That would be very neat indeed, thanks! Talking to the hypervisor folks.
>
We have that now. Will send out the revised patches sometime during the
next week most likely.
>>>
>>> Signed-off-by: Roman Kisel <[email protected]>
>>> ---
>>> arch/arm64/hyperv/mshyperv.c | 34 +++++++++++++++++++++++++++++-----
>>> 1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>>> index b1a4de4eee29..208a3bcb9686 100644
>>> --- a/arch/arm64/hyperv/mshyperv.c
>>> +++ b/arch/arm64/hyperv/mshyperv.c
>>> @@ -15,6 +15,9 @@
>>> #include <linux/errno.h>
>>> #include <linux/version.h>
>>> #include <linux/cpuhotplug.h>
>>> +#include <linux/libfdt.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_fdt.h>
>>> #include <asm/mshyperv.h>
>>> static bool hyperv_initialized;
>>> @@ -27,6 +30,29 @@ int hv_get_hypervisor_version(union
>>> hv_hypervisor_version_info *info)
>>> return 0;
>>> }
>>> +static bool hyperv_detect_fdt(void)
>>> +{
>>> +#ifdef CONFIG_OF
>>> + const unsigned long hyp_node = of_get_flat_dt_subnode_by_name(
>>> + of_get_flat_dt_root(), "hypervisor");
>>> +
>>> + return (hyp_node != -FDT_ERR_NOTFOUND) &&
>>> + of_flat_dt_is_compatible(hyp_node, "microsoft,hyperv");
>>> +#else
>>> + return false;
>>> +#endif
>>> +}
>>> +
>>> +static bool hyperv_detect_acpi(void)
>>> +{
>>> +#ifdef CONFIG_ACPI
>>> + return !acpi_disabled &&
>>> + !strncmp((char *)&acpi_gbl_FADT.hypervisor_id,
>>> "MsHyperV", 8);
>>> +#else
>>> + return false;
>>> +#endif
>>> +}
>>> +
>>> static int __init hyperv_init(void)
>>> {
>>> struct hv_get_vp_registers_output result;
>>> @@ -35,13 +61,11 @@ static int __init hyperv_init(void)
>>> /*
>>> * Allow for a kernel built with CONFIG_HYPERV to be running in
>>> - * a non-Hyper-V environment, including on DT instead of ACPI.
>>> + * a non-Hyper-V environment.
>>> + *
>>> * In such cases, do nothing and return success.
>>> */
>>> - if (acpi_disabled)
>>> - return 0;
>>> -
>>> - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
>>> + if (!hyperv_detect_fdt() && !hyperv_detect_acpi())
>>> return 0;
>>> /* Setup the guest ID */
>>> --
>>> 2.45.0
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Thank you,
Roman
On 6/7/2024 12:55 PM, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 01:12:38PM -0500, Bjorn Helgaas wrote:
>> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
>>>
>>>
>>> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
>>>> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>>>>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>>>>> on arm64 thereby it won't be able to do that in the VTL mode where
>>>>> only DeviceTree can be used.
>>>>>
>>>>> Update the hyperv-pci driver to discover interrupt configuration
>>>>> via DeviceTree.
>>>>
>>>> Subject prefix should be "PCI: hv:"
>
> I forgot to also suggest that the subject line begin with a verb,
> e.g., "Get vPCI MSI IRQ domain from DT" or similar, again so it reads
> consistently with previous commits.
>
> Oh, I see patch 5/6, "Get the irq number from DeviceTree" is also very
> similar. It would be nice if they matched, e.g., both used "IRQ" and
> "DT".
>
> Bjorn
Will update, thanks! Going to send another version during the next week
most likely.
--
Thank you,
Roman