[PATCH v3 0/7] arm64: hyperv: Support Virtual Trust Level Boot

[PATCH v3 0/7] arm64: hyperv: Support Virtual Trust Level Boot

[Roman Kisel]

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

[V3]
    - Employed the SMC recently implemented in the Microsoft Hyper-V hypervisor
      to detect running on Hyper-V/arm64. No dependence on ACPI/DT is needed
      anymore although the source code still falls back to ACPI as the new
      hypervisor might be available only in the Windows Insiders channel just
      yet.
    - As a part of the above, refactored detecting the hypervisor via ACPI FADT.
    - There was a suggestion to explore whether it is feasible or not to express
      that ACPI must be absent for the VTL mode and present for the regular guests
      in the Hyper-V Kconfig file.
      My current conclusion is that this will require refactoring in many places.
      That becomes especially convoluted on x86_64 due to the MSI and APIC
      dependencies. I'd ask to let us tackle that in another patch series (or chalk
      up to nice-have's rather than fires to put out) to separate concerns and
      decrease chances of breakage.
    - While refactoring `get_vtl(void)` and the related code, fixed the hypercall
      output address not to overlap with the input as the Hyper-V TLFS mandates:
      "The input and output parameter lists cannot overlap or cross page boundaries."
      See https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
      for more.
      Some might argue that should've been a topic for a separate patch series;
      I'd counter that the change is well-contained (one line), has no dependencies,
      and makes the code legal.
    - Made the VTL boot code (c)leaner as was suggested.
    - Set DMA cache coherency for the VMBus.
    - Updated DT bindings in the VMBus documentation (separated out into a new patch).
    - Fixed `vmbus_set_irq` to use the API that works both for the ACPI and OF.
    - Reworked setting up the vPCI MSI IRQ domain in the non-ACPI case. The logic
      looks a bit fiddly/ad-hoc as I couldn't find the API that would fit the bill.
      Added comments to explain myself.

[V2]
    https://lore.kernel.org/all/20240514224508.212318-1-romank@linux.microsoft.com/
    - 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

[V1]
    https://lore.kernel.org/all/20240510160602.1311352-1-romank@linux.microsoft.com/

For validation, I built kernels for the arch'es in question with the small initrd
embedded into the kernel and booted the Hyper-V VMs off of that.

Roman Kisel (7):
  arm64: hyperv: Use SMC to detect hypervisor presence
  Drivers: hv: Enable VTL mode for arm64
  Drivers: hv: Provide arch-neutral implementation of get_vtl()
  arm64: hyperv: Boot in a Virtual Trust Level
  dt-bindings: bus: Add Hyper-V VMBus cache coherency and IRQs
  Drivers: hv: vmbus: Get the IRQ number from DT
  PCI: hv: Get vPCI MSI IRQ domain from DT

 .../bindings/bus/microsoft,vmbus.yaml         | 11 +++
 arch/arm64/hyperv/Makefile                    |  1 +
 arch/arm64/hyperv/hv_vtl.c                    | 13 ++++
 arch/arm64/hyperv/mshyperv.c                  | 40 +++++++++--
 arch/arm64/include/asm/mshyperv.h             | 12 ++++
 arch/x86/hyperv/hv_init.c                     | 34 ---------
 arch/x86/include/asm/hyperv-tlfs.h            |  7 --
 drivers/hv/Kconfig                            |  6 +-
 drivers/hv/hv_common.c                        | 47 +++++++++++-
 drivers/hv/vmbus_drv.c                        | 72 ++++++++++++++++---
 drivers/pci/controller/pci-hyperv.c           | 55 +++++++++++++-
 include/asm-generic/hyperv-tlfs.h             |  7 ++
 include/asm-generic/mshyperv.h                |  6 ++
 include/linux/hyperv.h                        |  2 +
 14 files changed, 251 insertions(+), 62 deletions(-)
 create mode 100644 arch/arm64/hyperv/hv_vtl.c


base-commit: 831bcbcead6668ebf20b64fdb27518f1362ace3a
-- 
2.34.1

[PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

The arm64 Hyper-V startup path relies on ACPI to detect
running under a Hyper-V compatible hypervisor. That
doesn't work on non-ACPI systems.

Hoist the ACPI detection logic into a separate function,
use the new SMC added recently to Hyper-V to use in the
non-ACPI case.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
 arch/arm64/include/asm/mshyperv.h |  5 +++++
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index b1a4de4eee29..341f98312667 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
 	return 0;
 }
 
+static bool hyperv_detect_via_acpi(void)
+{
+	if (acpi_disabled)
+		return false;
+#if IS_ENABLED(CONFIG_ACPI)
+	/* Hypervisor ID is only available in ACPI v6+. */
+	if (acpi_gbl_FADT.header.revision < 6)
+		return false;
+	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
+#else
+	return false;
+#endif
+}
+
+static bool hyperv_detect_via_smc(void)
+{
+	struct arm_smccc_res res = {};
+
+	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
+		return false;
+	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
+
+	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
+		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
+		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
+		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
+}
+
 static int __init hyperv_init(void)
 {
 	struct hv_get_vp_registers_output	result;
@@ -35,13 +63,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_via_acpi() && !hyperv_detect_via_smc())
 		return 0;
 
 	/* Setup the guest ID */
diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
index a975e1a689dd..a7a3586f7cb1 100644
--- a/arch/arm64/include/asm/mshyperv.h
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
 
 #include <asm-generic/mshyperv.h>
 
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
+
 #endif
-- 
2.34.1

[PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Roman Kisel]

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 <romank@linux.microsoft.com>
---
 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.34.1

[PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Roman Kisel]

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. Fix the hypercall output address in `get_vtl(void)`
not to overlap with the hypercall input area to adhere to
the Hyper-V TLFS.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_init.c          | 34 ---------------------
 arch/x86/include/asm/hyperv-tlfs.h |  7 -----
 drivers/hv/hv_common.c             | 47 ++++++++++++++++++++++++++++--
 include/asm-generic/hyperv-tlfs.h  |  7 +++++
 include/asm-generic/mshyperv.h     |  6 ++++
 5 files changed, 58 insertions(+), 43 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 9c452bfbd571..7d6c1523b0b5 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -339,8 +339,8 @@ int __init hv_common_init(void)
 	hyperv_pcpu_input_arg = alloc_percpu(void  *);
 	BUG_ON(!hyperv_pcpu_input_arg);
 
-	/* Allocate the per-CPU state for output arg for root */
-	if (hv_root_partition) {
+	/* Allocate the per-CPU state for output arg for root or a VTL */
+	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
 		hyperv_pcpu_output_arg = alloc_percpu(void *);
 		BUG_ON(!hyperv_pcpu_output_arg);
 	}
@@ -656,3 +656,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 = *this_cpu_ptr(hyperv_pcpu_output_arg);
+
+	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 814207e7c37f..271c365973d6 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 8fe7aaab2599..85a5b8cb1702 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -315,4 +315,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.34.1

[PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Roman Kisel]

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 <romank@linux.microsoft.com>
---
 arch/arm64/hyperv/Makefile        |  1 +
 arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
 arch/arm64/hyperv/mshyperv.c      |  4 ++++
 arch/arm64/include/asm/mshyperv.h |  7 +++++++
 4 files changed, 25 insertions(+)
 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..38642b7b6be0
--- /dev/null
+++ b/arch/arm64/hyperv/hv_vtl.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024, Microsoft, Inc.
+ *
+ * Author : Roman Kisel <romank@linux.microsoft.com>
+ */
+
+#include <asm/mshyperv.h>
+
+void __init hv_vtl_init_platform(void)
+{
+	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
+}
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index 341f98312667..8fd04d6e4800 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -98,6 +98,10 @@ static int __init hyperv_init(void)
 		return ret;
 	}
 
+	/* Find the VTL */
+	ms_hyperv.vtl = get_vtl();
+	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 a7a3586f7cb1..63d6bb6998fc 100644
--- a/arch/arm64/include/asm/mshyperv.h
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -49,6 +49,13 @@ 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) {}
+#endif
+
 #include <asm-generic/mshyperv.h>
 
 #define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
-- 
2.34.1

[PATCH v3 5/7] dt-bindings: bus: Add Hyper-V VMBus cache coherency and IRQs

[Roman Kisel]

Add dt-bindings for the Hyper-V VMBus DMA cache coherency
and interrupt specification.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 .../devicetree/bindings/bus/microsoft,vmbus.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
index a8d40c766dcd..5ec69226ab85 100644
--- a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
+++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
@@ -44,11 +44,22 @@ examples:
             #size-cells = <1>;
             ranges;
 
+            gic: intc@fe200000 {
+              compatible = "arm,gic-v3";
+              reg = <0x0 0xfe200000 0x0 0x10000>,   /* GIC Dist */
+                    <0x0 0xfe280000 0x0 0x200000>;  /* GICR */
+              interrupt-controller;
+              #interrupt-cells = <3>;
+            }
+
             vmbus@ff0000000 {
                 compatible = "microsoft,vmbus";
                 #address-cells = <2>;
                 #size-cells = <1>;
                 ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
+                dma-coherent;
+                interrupt-parent = <&gic>;
+                interrupts = <1 2 1>;
             };
         };
     };
-- 
2.34.1

[PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel]

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 and indicate DMA cache coherency.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 drivers/hv/vmbus_drv.c | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 12a707ab73f8..7eee7caff5f6 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2306,6 +2306,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
 }
 #endif
 
+static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
+{
+	struct irq_desc *desc;
+	int irq;
+
+	irq = platform_get_irq(pdev, 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("No interrupt descriptor for VMBus 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;
@@ -2320,6 +2348,12 @@ static int vmbus_device_add(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+#ifndef HYPERVISOR_CALLBACK_VECTOR
+	ret = vmbus_set_irq(pdev);
+	if (ret)
+		return ret;
+#endif
+
 	for_each_of_range(&parser, &range) {
 		struct resource *res;
 
@@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
 		cur_res = &res->sibling;
 	}
 
+	/*
+	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
+	 * might default to 'not coherent' on some architectures.
+	 * Avoid high-cost cache coherency maintenance done by the CPU.
+	 */
+#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
+	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
+	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
+
+	if (!of_property_read_bool(np, "dma-coherent"))
+		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
+	pdev->dev.dma_coherent = true;
+
+#endif
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Roman Kisel]

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 get vPCI MSI IRQ domain in the DeviceTree
case, too.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 drivers/hv/vmbus_drv.c              | 23 +++++++-----
 drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
 include/linux/hyperv.h              |  2 ++
 3 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 7eee7caff5f6..038bd9be64b7 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -45,7 +45,8 @@ struct vmbus_dynid {
 	struct hv_vmbus_device_id id;
 };
 
-static struct device  *hv_dev;
+/* VMBus Root Device */
+static struct device  *vmbus_root_device;
 
 static int hyperv_cpuhp_online;
 
@@ -80,9 +81,15 @@ static struct resource *fb_mmio;
 static struct resource *hyperv_mmio;
 static DEFINE_MUTEX(hyperv_mmio_lock);
 
+struct device *get_vmbus_root_device(void)
+{
+	return vmbus_root_device;
+}
+EXPORT_SYMBOL_GPL(get_vmbus_root_device);
+
 static int vmbus_exists(void)
 {
-	if (hv_dev == NULL)
+	if (vmbus_root_device == NULL)
 		return -ENODEV;
 
 	return 0;
@@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
 	 * On x86/x64 coherence is assumed and these calls have no effect.
 	 */
 	hv_setup_dma_ops(child_device,
-		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
+		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
 	return 0;
 }
 
@@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 		     &child_device_obj->channel->offermsg.offer.if_instance);
 
 	child_device_obj->device.bus = &hv_bus;
-	child_device_obj->device.parent = hv_dev;
+	child_device_obj->device.parent = vmbus_root_device;
 	child_device_obj->device.release = vmbus_device_release;
 
 	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
@@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
 	struct acpi_device *ancestor;
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 
-	hv_dev = &device->dev;
+	vmbus_root_device = &device->dev;
 
 	/*
 	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
@@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
-	hv_dev = &pdev->dev;
+	vmbus_root_device = &pdev->dev;
 
 	ret = of_range_parser_init(&parser, np);
 	if (ret)
@@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
 	if (ret)
 		return ret;
 
-	if (!hv_dev) {
+	if (!vmbus_root_device) {
 		ret = -ENODEV;
 		goto cleanup;
 	}
@@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
 
 cleanup:
 	platform_driver_unregister(&vmbus_platform_driver);
-	hv_dev = NULL;
+	vmbus_root_device = NULL;
 	return ret;
 }
 
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 5992280e8110..cdecefd1f9bd 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -50,6 +50,7 @@
 #include <linux/irqdomain.h>
 #include <linux/acpi.h>
 #include <linux/sizes.h>
+#include <linux/of_irq.h>
 #include <asm/mshyperv.h>
 
 /*
@@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
 	.activate = hv_pci_vec_irq_domain_activate,
 };
 
+#ifdef CONFIG_OF
+
+static struct irq_domain *hv_pci_of_irq_domain_parent(void)
+{
+	struct device_node *parent;
+	struct irq_domain *domain;
+
+	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);
+	domain = NULL;
+	if (parent) {
+		domain = irq_find_host(parent);
+		of_node_put(parent);
+	}
+
+	/*
+	 * `domain == NULL` shouldn't happen.
+	 *
+	 * If somehow the code does end up in that state, treat this as a configuration
+	 * issue rather than a hard error, emit a warning, and let the code proceed.
+	 * The NULL parent domain is an acceptable option for the `irq_domain_create_hierarchy`
+	 * function called later.
+	 */
+	if (!domain)
+		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
+	return domain;
+}
+
+#endif
+
 static int hv_pci_irqchip_init(void)
 {
 	static struct hv_pci_chip_data *chip_data;
@@ -906,10 +936,29 @@ static int hv_pci_irqchip_init(void)
 	 * IRQ domain once enabled, should not be removed since there is no
 	 * way to ensure that all the corresponding devices are also gone and
 	 * no interrupts will be generated.
+	 *
+	 * In the ACPI case, the parent IRQ domain is supplied by the ACPI
+	 * subsystem, and it is the default GSI domain pointing to the GIC.
+	 * Neither is available outside of the ACPI subsystem, cannot avoid
+	 * the messy ifdef below.
+	 * There is apparently no such default in the OF subsystem, and
+	 * `hv_pci_of_irq_domain_parent` finds the parent IRQ domain that
+	 * points to the GIC as well.
+	 * None of these two cases reaches for the MSI parent domain.
 	 */
-	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
-							  fn, &hv_pci_domain_ops,
-							  chip_data);
+#ifdef CONFIG_ACPI
+	if (!acpi_disabled)
+		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
+			fn, &hv_pci_domain_ops,
+			chip_data);
+#endif
+#if defined(CONFIG_OF)
+	if (!hv_msi_gic_irq_domain)
+		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+			hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
+			fn, &hv_pci_domain_ops,
+			chip_data);
+#endif
 
 	if (!hv_msi_gic_irq_domain) {
 		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 5e39baa7f6cb..b4aa1f579a97 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1346,6 +1346,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
 	return dev_get_drvdata(&dev->device);
 }
 
+struct device *get_vmbus_root_device(void);
+
 struct hv_ring_buffer_debug_info {
 	u32 current_interrupt_mask;
 	u32 current_read_index;
-- 
2.34.1

Re: [PATCH v3 5/7] dt-bindings: bus: Add Hyper-V VMBus cache coherency and IRQs

[Krzysztof Kozlowski]

On 27/07/2024 00:59, Roman Kisel wrote:
> Add dt-bindings for the Hyper-V VMBus DMA cache coherency
> and interrupt specification.
> 

You did not add any bindings. I don't understand this description.

Anyway, not tested.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


Best regards,
Krzysztof

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Krzysztof Kozlowski]

On 27/07/2024 00:59, Roman Kisel wrote:
> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>  		cur_res = &res->sibling;
>  	}
>  
> +	/*
> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
> +	 * might default to 'not coherent' on some architectures.
> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
> +	 */
> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> +
> +	if (!of_property_read_bool(np, "dma-coherent"))
> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");

Why do you need this property at all, if it is allways dma-coherent? Are
you supporting dma-noncoherent somewhere?

Best regards,
Krzysztof

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Arnd Bergmann]

On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
> On 27/07/2024 00:59, Roman Kisel wrote:
>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>  		cur_res = &res->sibling;
>>  	}
>>  
>> +	/*
>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>> +	 * might default to 'not coherent' on some architectures.
>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>> +	 */
>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>> +
>> +	if (!of_property_read_bool(np, "dma-coherent"))
>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
>
> Why do you need this property at all, if it is allways dma-coherent? Are
> you supporting dma-noncoherent somewhere?

It's just a sanity check that the DT is well-formed.

Since the dma-coherent property is interpreted by common code, it's
not up to hv to change the default for the platform. I'm not sure
if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
check to determine that an architecture defaults to noncoherent
though, as the function may be needed to do something else.

The global "dma_default_coherent' may be a better thing to check
for. This is e.g. set on powerpc64, riscv and on specific mips
platforms, but it's never set on arm64 as far as I can tell.

     Arnd

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Krzysztof Kozlowski]

On 27/07/2024 11:17, Arnd Bergmann wrote:
> On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
>> On 27/07/2024 00:59, Roman Kisel wrote:
>>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>>  		cur_res = &res->sibling;
>>>  	}
>>>  
>>> +	/*
>>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>>> +	 * might default to 'not coherent' on some architectures.
>>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>>> +	 */
>>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>>> +
>>> +	if (!of_property_read_bool(np, "dma-coherent"))
>>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
>>
>> Why do you need this property at all, if it is allways dma-coherent? Are
>> you supporting dma-noncoherent somewhere?
> 
> It's just a sanity check that the DT is well-formed.
> 
> Since the dma-coherent property is interpreted by common code, it's
> not up to hv to change the default for the platform. I'm not sure
> if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
> check to determine that an architecture defaults to noncoherent
> though, as the function may be needed to do something else.
> 
> The global "dma_default_coherent' may be a better thing to check
> for. This is e.g. set on powerpc64, riscv and on specific mips
> platforms, but it's never set on arm64 as far as I can tell.

Kernel's task is not to validate the DT. Even if it was, above code
works poor. What if someone adds 'dma-noncoherent'?

The job of the bindings and DT schema is to validate and check DT if is
well formed.


Best regards,
Krzysztof

Re: [PATCH v3 5/7] dt-bindings: bus: Add Hyper-V VMBus cache coherency and IRQs

[Roman Kisel]

On 7/27/2024 1:53 AM, Krzysztof Kozlowski wrote:
> On 27/07/2024 00:59, Roman Kisel wrote:
>> Add dt-bindings for the Hyper-V VMBus DMA cache coherency
>> and interrupt specification.
>>
> 
> You did not add any bindings. I don't understand this description.
> 
My bad, extended the example for the interrupt controller node.
Will fix in the next version.

> Anyway, not tested.
> 
Understood. I didn't realize that yml goes through testing, thought it
was akin to a machine-readable portion of the documentation.

> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
I used the "cached" version of the get_maintainers.pl output produced 
for v2. The bad assumption was that adding a yml do Documentation would 
not affect that. Will fix, appreciate helping me so much in getting this 
into a good shape!

> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
> 
> 
> Best regards,
> Krzysztof
> 

-- 
Thank you,
Roman

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel]

On 7/27/2024 1:56 AM, Krzysztof Kozlowski wrote:
> On 27/07/2024 00:59, Roman Kisel wrote:
>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>   		cur_res = &res->sibling;
>>   	}
>>   
>> +	/*
>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>> +	 * might default to 'not coherent' on some architectures.
>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>> +	 */
>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>> +
>> +	if (!of_property_read_bool(np, "dma-coherent"))
>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
> 
> Why do you need this property at all, if it is allways dma-coherent? Are
> you supporting dma-noncoherent somewhere?
No support for non-coherent. Wanted to do a sanity check. Had better 
check for dma-noncoherent and warn about that I believe.

> 
> Best regards,
> Krzysztof
> 

-- 
Thank you,
Roman

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel]

On 7/27/2024 2:17 AM, Arnd Bergmann wrote:
> On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
>> On 27/07/2024 00:59, Roman Kisel wrote:
>>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>>   		cur_res = &res->sibling;
>>>   	}
>>>   
>>> +	/*
>>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>>> +	 * might default to 'not coherent' on some architectures.
>>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>>> +	 */
>>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>>> +
>>> +	if (!of_property_read_bool(np, "dma-coherent"))
>>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
>>
>> Why do you need this property at all, if it is allways dma-coherent? Are
>> you supporting dma-noncoherent somewhere?
> 
> It's just a sanity check that the DT is well-formed.
> 
> Since the dma-coherent property is interpreted by common code, it's
> not up to hv to change the default for the platform. I'm not sure
> if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
> check to determine that an architecture defaults to noncoherent
> though, as the function may be needed to do something else.
I used the ifdef as the dma_coherent field is declared under these macros:

#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
extern bool dma_default_coherent;
static inline bool dev_is_dma_coherent(struct device *dev)
{
	return dev->dma_coherent;
}
#else
#define dma_default_coherent true

static inline bool dev_is_dma_coherent(struct device *dev)
{
	return true;
}

i.e., there is no API to set dma_coherent. As I see it, the options
are either warn the user if they forgot to add `dma-coherent`

if (!dev_is_dma_coherent(dev)) pr_warn("add dma-coherent to be faster\n"),

or warn and force the flag to true. Maybe just warn
the user I think now... The code will be cleaner (no need to emulate
a-would-be set_dma_coherent) , and the user will
know how to make the system perform at its best.

Appreciate sharing the reservations about that piece!

> 
> The global "dma_default_coherent' may be a better thing to check
> for. This is e.g. set on powerpc64, riscv and on specific mips
> platforms, but it's never set on arm64 as far as I can tell.
> 
>       Arnd

-- 
Thank you,
Roman

Re: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Wei Liu]

On Fri, Jul 26, 2024 at 03:59:10PM -0700, Roman Kisel wrote:
> 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 get vPCI MSI IRQ domain in the DeviceTree
> case, too.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c              | 23 +++++++-----
>  drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>  include/linux/hyperv.h              |  2 ++
>  3 files changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 7eee7caff5f6..038bd9be64b7 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>  	struct hv_vmbus_device_id id;
>  };
>  
> -static struct device  *hv_dev;
> +/* VMBus Root Device */
> +static struct device  *vmbus_root_device;

You're changing the name of the variable. That should preferably be done
in a separate patch. That's going to make this patch shorter and easier
to review.

>  
>  static int hyperv_cpuhp_online;
>  
> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>  static struct resource *hyperv_mmio;
>  static DEFINE_MUTEX(hyperv_mmio_lock);
>  
> +struct device *get_vmbus_root_device(void)
> +{
> +	return vmbus_root_device;
> +}
> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);

I would like you to add "hv_" prefix to this exported symbol, or rename
it to "vmbus_get_root_device()".

> +
>  static int vmbus_exists(void)
>  {
> -	if (hv_dev == NULL)
> +	if (vmbus_root_device == NULL)
>  		return -ENODEV;
>  
>  	return 0;
> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
>  	 * On x86/x64 coherence is assumed and these calls have no effect.
>  	 */
>  	hv_setup_dma_ops(child_device,
> -		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
> +		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>  	return 0;
>  }
>  
> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  		     &child_device_obj->channel->offermsg.offer.if_instance);
>  
>  	child_device_obj->device.bus = &hv_bus;
> -	child_device_obj->device.parent = hv_dev;
> +	child_device_obj->device.parent = vmbus_root_device;
>  	child_device_obj->device.release = vmbus_device_release;
>  
>  	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  	struct acpi_device *ancestor;
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  
> -	hv_dev = &device->dev;
> +	vmbus_root_device = &device->dev;
>  
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> -	hv_dev = &pdev->dev;
> +	vmbus_root_device = &pdev->dev;
>  
>  	ret = of_range_parser_init(&parser, np);
>  	if (ret)
> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>  	if (ret)
>  		return ret;
>  
> -	if (!hv_dev) {
> +	if (!vmbus_root_device) {
>  		ret = -ENODEV;
>  		goto cleanup;
>  	}
> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
>  
>  cleanup:
>  	platform_driver_unregister(&vmbus_platform_driver);
> -	hv_dev = NULL;
> +	vmbus_root_device = NULL;
>  	return ret;
>  }
>  
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 5992280e8110..cdecefd1f9bd 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -50,6 +50,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/acpi.h>
>  #include <linux/sizes.h>
> +#include <linux/of_irq.h>
>  #include <asm/mshyperv.h>
>  
>  /*
> @@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
>  	.activate = hv_pci_vec_irq_domain_activate,
>  };
>  
> +#ifdef CONFIG_OF
> +
> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
> +{
> +	struct device_node *parent;
> +	struct irq_domain *domain;
> +
> +	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);
> +	domain = NULL;
> +	if (parent) {
> +		domain = irq_find_host(parent);
> +		of_node_put(parent);
> +	}
> +

I cannot really comment on the ARM side of things around how this system
is set up. I will leave that to someone who's more familiar with the
matter to review.

Thanks,
Wei.

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Wei Liu]

On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
> The arm64 Hyper-V startup path relies on ACPI to detect
> running under a Hyper-V compatible hypervisor. That
> doesn't work on non-ACPI systems.
> 
> Hoist the ACPI detection logic into a separate function,
> use the new SMC added recently to Hyper-V to use in the
> non-ACPI case.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>

The change looks sensible.

Within one minor comment fixed below:

Acked-by: Wei Liu <wei.liu@kernel.org>

However I would also like to get an Acked-by or reivewed-by from someone
who works on the ARM64 side of things -- Saurabh, Boqun, Srivatsa, and
Jinank?

> ---
>  arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>  arch/arm64/include/asm/mshyperv.h |  5 +++++
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
[...]
> diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
> index a975e1a689dd..a7a3586f7cb1 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
>  
>  #include <asm-generic/mshyperv.h>
>  
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570

Presumably these are ASCII codes for an identifier string, please 
provide comments to explain what they are.

> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
> +
>  #endif
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Wei Liu]

On Fri, Jul 26, 2024 at 03:59:05PM -0700, Roman Kisel wrote:
> 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 <romank@linux.microsoft.com>

Acked-by: Wei Liu <wei.liu@kernel.org>

Re: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Wei Liu]

On Fri, Jul 26, 2024 at 03:59:06PM -0700, Roman Kisel wrote:
> 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. Fix the hypercall output address in `get_vtl(void)`
> not to overlap with the hypercall input area to adhere to
> the Hyper-V TLFS.

Can you split the fix out? That potentially can be backported.

> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> +
> +#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 = *this_cpu_ptr(hyperv_pcpu_output_arg);

Hmm... I don't remember why the old code didn't use
hyperv_pcpu_output_arg but instead reused input+OFFSET as output.

Saurabh, can you comment on this?

Thanks,
Wei.

Re: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Wei Liu]

On Fri, Jul 26, 2024 at 03:59:07PM -0700, Roman Kisel wrote:
> 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 <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/Makefile        |  1 +
>  arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>  arch/arm64/hyperv/mshyperv.c      |  4 ++++
>  arch/arm64/include/asm/mshyperv.h |  7 +++++++
>  4 files changed, 25 insertions(+)
>  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..38642b7b6be0
> --- /dev/null
> +++ b/arch/arm64/hyperv/hv_vtl.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024, Microsoft, Inc.
> + *
> + * Author : Roman Kisel <romank@linux.microsoft.com>
> + */
> +
> +#include <asm/mshyperv.h>
> +
> +void __init hv_vtl_init_platform(void)
> +{
> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> +}
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index 341f98312667..8fd04d6e4800 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>  		return ret;
>  	}
>  
> +	/* Find the VTL */
> +	ms_hyperv.vtl = get_vtl();
> +	hv_vtl_init_platform();

It doesn't make sense to me because this function unconditionally prints
Linux runs in Hyper-V Virtual Trust Level.

Thanks,
Wei.

> +
>  	ms_hyperv_late_init();
>  
>  	hyperv_initialized = true;
> diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
> index a7a3586f7cb1..63d6bb6998fc 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -49,6 +49,13 @@ 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) {}
> +#endif
> +
>  #include <asm-generic/mshyperv.h>
>  
>  #define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
> -- 
> 2.34.1
> 

RE: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM
> 
> The arm64 Hyper-V startup path relies on ACPI to detect
> running under a Hyper-V compatible hypervisor. That
> doesn't work on non-ACPI systems.
> 
> Hoist the ACPI detection logic into a separate function,
> use the new SMC added recently to Hyper-V to use in the
> non-ACPI case.

Wording seems slightly messed up.  Perhaps:

   Hoist the ACPI detection logic into a separate function. Then
   use the new SMC added recently to Hyper-V in the non-ACPI
   case.

Also, the phrase "the new SMC" seems a bit off to me.  The "Terms and
Abbreviations" section of the SMCCC specification defines "SMC" as
an instruction:

   Secure Monitor Call. An Arm assembler instruction that causes an
   exception that is taken synchronously into EL3.

More precisely, I think you mean a SMC "function identifier" that is
newly implemented by Hyper-V.  And the function identifier itself isn't
new; it's the Hyper-V implementation that's new.

Similar comment applies in the cover letter for this patch set, and
perhaps to the Subject line of this patch.

> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>  arch/arm64/include/asm/mshyperv.h |  5 +++++
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..341f98312667 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>  	return 0;
>  }
> 
> +static bool hyperv_detect_via_acpi(void)
> +{
> +	if (acpi_disabled)
> +		return false;
> +#if IS_ENABLED(CONFIG_ACPI)
> +	/* Hypervisor ID is only available in ACPI v6+. */
> +	if (acpi_gbl_FADT.header.revision < 6)
> +		return false;
> +	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
> +#else
> +	return false;
> +#endif
> +}
> +
> +static bool hyperv_detect_via_smc(void)
> +{
> +	struct arm_smccc_res res = {};
> +
> +	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> +		return false;
> +	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
> +
> +	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
> +		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
> +		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
> +		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
> +}
> +
>  static int __init hyperv_init(void)
>  {
>  	struct hv_get_vp_registers_output	result;
> @@ -35,13 +63,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_via_acpi() && !hyperv_detect_via_smc())
>  		return 0;
> 
>  	/* Setup the guest ID */
> diff --git a/arch/arm64/include/asm/mshyperv.h
> b/arch/arm64/include/asm/mshyperv.h
> index a975e1a689dd..a7a3586f7cb1 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
> 
>  #include <asm-generic/mshyperv.h>
> 
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
> +

Section 6.2 of the SMCCC specification says that the "Call UID Query"
returns a UUID. The above #defines look like an ASCII string is being
returned. Arguably the ASCII string can be treated as a set of 128 bits
just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V
be changed to return a real UUID? While the distinction probably
won't make a material difference here, we've had problems in the past
with Hyper-V doing slightly weird things that later caused unexpected
trouble. Please just get it right. :-)

Michael

>  #endif
> --
> 2.34.1
> 

RE: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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 <romank@linux.microsoft.com>
> ---
>  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.34.1
> 

In v2 of this patch, I suggested [1] making a couple additional minor changes
so that kernels built *without* HYPER_VTL_MODE would still require
ACPI.  Did that suggestion not work out?  If that's the case, I'm curious
about what goes wrong.

Michael

[1] https://lore.kernel.org/all/SN6PR02MB4157E15EFE263BBA3D8DFC51D4EC2@SN6PR02MB4157.namprd02.prod.outlook.com/

RE: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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. Fix the hypercall output address in `get_vtl(void)`
> not to overlap with the hypercall input area to adhere to
> the Hyper-V TLFS.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c          | 34 ---------------------
>  arch/x86/include/asm/hyperv-tlfs.h |  7 -----
>  drivers/hv/hv_common.c             | 47 ++++++++++++++++++++++++++++--
>  include/asm-generic/hyperv-tlfs.h  |  7 +++++
>  include/asm-generic/mshyperv.h     |  6 ++++
>  5 files changed, 58 insertions(+), 43 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 9c452bfbd571..7d6c1523b0b5 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -339,8 +339,8 @@ int __init hv_common_init(void)
>  	hyperv_pcpu_input_arg = alloc_percpu(void  *);
>  	BUG_ON(!hyperv_pcpu_input_arg);
> 
> -	/* Allocate the per-CPU state for output arg for root */
> -	if (hv_root_partition) {
> +	/* Allocate the per-CPU state for output arg for root or a VTL */
> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>  		hyperv_pcpu_output_arg = alloc_percpu(void *);
>  		BUG_ON(!hyperv_pcpu_output_arg);
>  	}
> @@ -656,3 +656,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 = *this_cpu_ptr(hyperv_pcpu_output_arg);

Rather than use the hyperv_pcpu_output_arg here, it's OK to
use a different area of the hyperv_pcpu_input_arg page.  For
example,

	output = (void *)input + HV_HYP_PAGE_SIZE/2;

The TLFS does not require that the input and output be in
separate pages.

While using the hyperv_pcpu_output_arg is conceptually a
bit cleaner, doing so requires allocating a 4K page per CPU that
is not otherwise used. The VTL 2 code wants to be frugal with
memory, and this seems like a good step in that direction. :-)

The hyperv_pcpu_output_arg was added for the cases where up
to a full page is needed for input and output on the same hypercall.
So far, the only case of that is when running in the root partition.

> +
> +	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 814207e7c37f..271c365973d6 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.

s/there is no an associated/there is no associated/

> + */
> +#define	HV_REGISTER_VSM_VP_STATUS	0x000D0003
> +#define	HV_VTL_MASK			GENMASK(3, 0)

Further down in hyperv-tlfs.h is a section devoted to register
definitions. It seems like this definition should go there in
numeric order, which is after HV_REGISTER_STIMER0_COUNT.

Michael

> +
>  /*
>   * Group B features.
>   */
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 8fe7aaab2599..85a5b8cb1702 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -315,4 +315,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.34.1
> 

RE: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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 <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/Makefile        |  1 +
>  arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>  arch/arm64/hyperv/mshyperv.c      |  4 ++++
>  arch/arm64/include/asm/mshyperv.h |  7 +++++++
>  4 files changed, 25 insertions(+)
>  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..38642b7b6be0
> --- /dev/null
> +++ b/arch/arm64/hyperv/hv_vtl.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024, Microsoft, Inc.
> + *
> + * Author : Roman Kisel <romank@linux.microsoft.com>
> + */
> +
> +#include <asm/mshyperv.h>
> +
> +void __init hv_vtl_init_platform(void)
> +{
> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> +}
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index 341f98312667..8fd04d6e4800 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>  		return ret;
>  	}
> 
> +	/* Find the VTL */
> +	ms_hyperv.vtl = get_vtl();
> +	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 a7a3586f7cb1..63d6bb6998fc 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -49,6 +49,13 @@ 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);

This declaration is spurious since you removed the function.

Michael

> +#else
> +static inline void __init hv_vtl_init_platform(void) {}
> +#endif
> +
>  #include <asm-generic/mshyperv.h>
> 
>  #define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
> --
> 2.34.1
> 

RE: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, July 29, 2024 9:51 AM
> 
> 
> On 7/27/2024 2:17 AM, Arnd Bergmann wrote:
> > On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
> >> On 27/07/2024 00:59, Roman Kisel wrote:
> >>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
> >>>   		cur_res = &res->sibling;
> >>>   	}
> >>>
> >>> +	/*
> >>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
> >>> +	 * might default to 'not coherent' on some architectures.
> >>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
> >>> +	 */
> >>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> >>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> >>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> >>> +
> >>> +	if (!of_property_read_bool(np, "dma-coherent"))
> >>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
> >>
> >> Why do you need this property at all, if it is allways dma-coherent? Are
> >> you supporting dma-noncoherent somewhere?
> >
> > It's just a sanity check that the DT is well-formed.

In my view, this chunk of code can be dropped entirely. The guest
should believe what the Hyper-V host tells it via DT, and that includes
operating in non-coherent mode. There might be some future case
where non-coherent DMA is correct. In such a case, we don't want to
have to come back and remove an overly aggressive sanity test from
Linux kernel code.

As Arnd noted, the dma-coherent (or dma-noncoherent) property should
be interpreted and applied to the device by common code. If that's not
working for some reason in this case, we should investigate why not.

Note that the ACPI code for VMBus does the same thing -- it believes and
uses whatever the _CCA property says. The exception is that there
are deployed version of Hyper-V that don't set _CCA at all, contrary to the
ACPI spec. So there's a hack in vmbus_acpi_add() to work around this case
and force coherent_dma. But that's the only place where the current
Hyper-V assumption of coherence comes into play. I sincerely hope Hyper-V
ensures that the DT correctly includes dma-coherent from the start, and
that we don't have to replicate the hack on the DT side.

Michael

> >
> > Since the dma-coherent property is interpreted by common code, it's
> > not up to hv to change the default for the platform. I'm not sure
> > if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
> > check to determine that an architecture defaults to noncoherent
> > though, as the function may be needed to do something else.
> I used the ifdef as the dma_coherent field is declared under these macros:
> 
> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> extern bool dma_default_coherent;
> static inline bool dev_is_dma_coherent(struct device *dev)
> {
> 	return dev->dma_coherent;
> }
> #else
> #define dma_default_coherent true
> 
> static inline bool dev_is_dma_coherent(struct device *dev)
> {
> 	return true;
> }
> 
> i.e., there is no API to set dma_coherent. As I see it, the options
> are either warn the user if they forgot to add `dma-coherent`
> 
> if (!dev_is_dma_coherent(dev)) pr_warn("add dma-coherent to be faster\n"),
> 
> or warn and force the flag to true. Maybe just warn
> the user I think now... The code will be cleaner (no need to emulate
> a-would-be set_dma_coherent) , and the user will
> know how to make the system perform at its best.
> 
> Appreciate sharing the reservations about that piece!
> 
> >
> > The global "dma_default_coherent' may be a better thing to check
> > for. This is e.g. set on powerpc64, riscv and on specific mips
> > platforms, but it's never set on arm64 as far as I can tell.
> >
> >       Arnd
> 
> --
> Thank you,
> Roman
> 

RE: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM
> 
> 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 get vPCI MSI IRQ domain in the DeviceTree
> case, too.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>

Overall, this makes sense to me, and I think it works. As you noted in the cover
letter for the patch series, it's a bit messy.  But see my two comments below.

> ---
>  drivers/hv/vmbus_drv.c              | 23 +++++++-----
>  drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>  include/linux/hyperv.h              |  2 ++
>  3 files changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 7eee7caff5f6..038bd9be64b7 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>  	struct hv_vmbus_device_id id;
>  };
> 
> -static struct device  *hv_dev;
> +/* VMBus Root Device */
> +static struct device  *vmbus_root_device;
> 
>  static int hyperv_cpuhp_online;
> 
> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>  static struct resource *hyperv_mmio;
>  static DEFINE_MUTEX(hyperv_mmio_lock);
> 
> +struct device *get_vmbus_root_device(void)
> +{
> +	return vmbus_root_device;
> +}
> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);
> +
>  static int vmbus_exists(void)
>  {
> -	if (hv_dev == NULL)
> +	if (vmbus_root_device == NULL)
>  		return -ENODEV;
> 
>  	return 0;
> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
>  	 * On x86/x64 coherence is assumed and these calls have no effect.
>  	 */
>  	hv_setup_dma_ops(child_device,
> -		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
> +		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>  	return 0;
>  }
> 
> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  		     &child_device_obj->channel->offermsg.offer.if_instance);
> 
>  	child_device_obj->device.bus = &hv_bus;
> -	child_device_obj->device.parent = hv_dev;
> +	child_device_obj->device.parent = vmbus_root_device;
>  	child_device_obj->device.release = vmbus_device_release;
> 
>  	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  	struct acpi_device *ancestor;
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
> 
> -	hv_dev = &device->dev;
> +	vmbus_root_device = &device->dev;
> 
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int ret;
> 
> -	hv_dev = &pdev->dev;
> +	vmbus_root_device = &pdev->dev;
> 
>  	ret = of_range_parser_init(&parser, np);
>  	if (ret)
> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>  	if (ret)
>  		return ret;
> 
> -	if (!hv_dev) {
> +	if (!vmbus_root_device) {
>  		ret = -ENODEV;
>  		goto cleanup;
>  	}
> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
> 
>  cleanup:
>  	platform_driver_unregister(&vmbus_platform_driver);
> -	hv_dev = NULL;
> +	vmbus_root_device = NULL;
>  	return ret;
>  }
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 5992280e8110..cdecefd1f9bd 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -50,6 +50,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/acpi.h>
>  #include <linux/sizes.h>
> +#include <linux/of_irq.h>
>  #include <asm/mshyperv.h>
> 
>  /*
> @@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
>  	.activate = hv_pci_vec_irq_domain_activate,
>  };
> 
> +#ifdef CONFIG_OF
> +
> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
> +{
> +	struct device_node *parent;
> +	struct irq_domain *domain;
> +
> +	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);

I think the above can be simplified to:

	parent = of_irq_find_parent(get_vmbus_root_device()->of_node);

Converting the vmbus_root_device to the platform device, and then accessing
the "dev" field of the platform device puts you back where you started with
the vmbus_root_device.  See the code in vmbus_device_add() where the
vmbus_root_device is set to the dev field of the platform device.

> +	domain = NULL;
> +	if (parent) {
> +		domain = irq_find_host(parent);
> +		of_node_put(parent);
> +	}
> +
> +	/*
> +	 * `domain == NULL` shouldn't happen.
> +	 *
> +	 * If somehow the code does end up in that state, treat this as a configuration
> +	 * issue rather than a hard error, emit a warning, and let the code proceed.
> +	 * The NULL parent domain is an acceptable option for the `irq_domain_create_hierarchy`
> +	 * function called later.
> +	 */
> +	if (!domain)
> +		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
> +	return domain;
> +}

Here's a thought, which may or may not be a good one:  Push some or all
the functionality of hv_pci_of_irq_domain_parent() into vmbus_device_add().
In the simplest case, have vmbus_device_add() store the of_node (which is
already the "np" local variable) in a global static variable, and provide
hv_get_vmbus_of_node() instead of get_vmbus_root_device().

The next step to consider would be to compute the parent in
vmbus_device_add(), and provide hv_get_vmbus_parent_of_node() instead
of hv_get_vmbus_of_node(). One difference is that vmbus_device_add()
runs for both x86 and arm64, while hv_pci_of_irq_domain_parent() is for
arm64 only.  The parent node may not exist on x86, but maybe that isn't
really a problem.

Pushing everything into vmbus_device_add() would entail doing the
irq_find_host(parent) as well, and the accessor function would just
return the IRQ domain. In that case, hv_pci_of_irq_domain_parent()
can go away. The domain might be NULL on x86, but that's OK
because the accessor function won't be called on x86.

Maybe there's a snag that prevents this idea from working well,
particularly on x86 where the domain will be NULL. But if it works,
it seems slightly less messy to me, though that is a judgment call.
I'll leave it to you to decide, and I'm OK either way. :-)

Michael

> +
> +#endif
> +
>  static int hv_pci_irqchip_init(void)
>  {
>  	static struct hv_pci_chip_data *chip_data;
> @@ -906,10 +936,29 @@ static int hv_pci_irqchip_init(void)
>  	 * IRQ domain once enabled, should not be removed since there is no
>  	 * way to ensure that all the corresponding devices are also gone and
>  	 * no interrupts will be generated.
> +	 *
> +	 * In the ACPI case, the parent IRQ domain is supplied by the ACPI
> +	 * subsystem, and it is the default GSI domain pointing to the GIC.
> +	 * Neither is available outside of the ACPI subsystem, cannot avoid
> +	 * the messy ifdef below.
> +	 * There is apparently no such default in the OF subsystem, and
> +	 * `hv_pci_of_irq_domain_parent` finds the parent IRQ domain that
> +	 * points to the GIC as well.
> +	 * None of these two cases reaches for the MSI parent domain.
>  	 */
> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> -							  fn, &hv_pci_domain_ops,
> -							  chip_data);
> +#ifdef CONFIG_ACPI
> +	if (!acpi_disabled)
> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> +#endif
> +#if defined(CONFIG_OF)
> +	if (!hv_msi_gic_irq_domain)
> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> +			hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> +#endif
> 
>  	if (!hv_msi_gic_irq_domain) {
>  		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 5e39baa7f6cb..b4aa1f579a97 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1346,6 +1346,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
>  	return dev_get_drvdata(&dev->device);
>  }
> 
> +struct device *get_vmbus_root_device(void);
> +
>  struct hv_ring_buffer_debug_info {
>  	u32 current_interrupt_mask;
>  	u32 current_read_index;
> --
> 2.34.1
> 

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Saurabh Singh Sengar]

On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
> The arm64 Hyper-V startup path relies on ACPI to detect
> running under a Hyper-V compatible hypervisor. That
> doesn't work on non-ACPI systems.
> 
> Hoist the ACPI detection logic into a separate function,
> use the new SMC added recently to Hyper-V to use in the
> non-ACPI case.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>  arch/arm64/include/asm/mshyperv.h |  5 +++++
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index b1a4de4eee29..341f98312667 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>  	return 0;
>  }
>  
> +static bool hyperv_detect_via_acpi(void)
> +{
> +	if (acpi_disabled)
> +		return false;
> +#if IS_ENABLED(CONFIG_ACPI)
> +	/* Hypervisor ID is only available in ACPI v6+. */
> +	if (acpi_gbl_FADT.header.revision < 6)
> +		return false;
> +	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
> +#else
> +	return false;
> +#endif
> +}
> +
> +static bool hyperv_detect_via_smc(void)
> +{
> +	struct arm_smccc_res res = {};
> +
> +	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> +		return false;
> +	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
> +
> +	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
> +		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
> +		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
> +		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
> +}

As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor,
can we add a comment about it, specifying the exact version in it would be great.

If someone attempts to build non-ACPI kernel on older Hyper-V what is the
behaviour of this function, do we need to safeguard or handle that case ?

- Saurabh

Re: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Saurabh Singh Sengar]

On Mon, Aug 05, 2024 at 03:01:58AM +0000, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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 <romank@linux.microsoft.com>
> > ---
> >  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.34.1
> > 
> 
> In v2 of this patch, I suggested [1] making a couple additional minor changes
> so that kernels built *without* HYPER_VTL_MODE would still require
> ACPI.  Did that suggestion not work out?  If that's the case, I'm curious
> about what goes wrong.

Hi Michael/Roman,
I was considering making HYPERV_VTL_MODE depend on CONFIG_OF. That should address
above concern as well. Do you see any potential issue with it.

- Saurabh

Re: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Saurabh Singh Sengar]

On Sat, Aug 03, 2024 at 01:21:52AM +0000, Wei Liu wrote:
> On Fri, Jul 26, 2024 at 03:59:06PM -0700, Roman Kisel wrote:
> > 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. Fix the hypercall output address in `get_vtl(void)`
> > not to overlap with the hypercall input area to adhere to
> > the Hyper-V TLFS.
> 
> Can you split the fix out? That potentially can be backported.
> 
> > 
> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> > ---
> > +
> > +#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 = *this_cpu_ptr(hyperv_pcpu_output_arg);
> 
> Hmm... I don't remember why the old code didn't use
> hyperv_pcpu_output_arg but instead reused input+OFFSET as output.
> 
> Saurabh, can you comment on this?

This was done to optimize memory usage. Michael has provided more
details on this in his review of the patch today.

Re: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Saurabh Singh Sengar]

On Fri, Jul 26, 2024 at 03:59:07PM -0700, Roman Kisel wrote:
> 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 <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/Makefile        |  1 +
>  arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>  arch/arm64/hyperv/mshyperv.c      |  4 ++++
>  arch/arm64/include/asm/mshyperv.h |  7 +++++++
>  4 files changed, 25 insertions(+)
>  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..38642b7b6be0
> --- /dev/null
> +++ b/arch/arm64/hyperv/hv_vtl.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024, Microsoft, Inc.
> + *
> + * Author : Roman Kisel <romank@linux.microsoft.com>
> + */
> +
> +#include <asm/mshyperv.h>
> +
> +void __init hv_vtl_init_platform(void)
> +{
> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> +}
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index 341f98312667..8fd04d6e4800 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>  		return ret;
>  	}
>  
> +	/* Find the VTL */
> +	ms_hyperv.vtl = get_vtl();
> +	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 a7a3586f7cb1..63d6bb6998fc 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -49,6 +49,13 @@ 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) {}
> +#endif
> +

These functions are defined in x86 header as well. We can move it to generic header.

- Saurabh

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Saurabh Singh Sengar]

On Fri, Jul 26, 2024 at 03:59:09PM -0700, 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 and indicate DMA cache coherency.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 12a707ab73f8..7eee7caff5f6 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2306,6 +2306,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  }
>  #endif
>  
> +static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
> +{
> +	struct irq_desc *desc;
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 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);

irq_to_desc is not an exported symbol if CONFIG_SPARSE_IRQ is enabled. This will
break the builds for HYPERV as module.

- Saurabh

RE: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Michael Kelley]

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, August 5, 2024 1:30 AM
> 
> On Fri, Jul 26, 2024 at 03:59:09PM -0700, 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 and indicate DMA cache coherency.
> >
> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> > ---
> >  drivers/hv/vmbus_drv.c | 49 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 12a707ab73f8..7eee7caff5f6 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -2306,6 +2306,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> >  }
> >  #endif
> >
> > +static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
> > +{
> > +	struct irq_desc *desc;
> > +	int irq;
> > +
> > +	irq = platform_get_irq(pdev, 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);
> 
> irq_to_desc is not an exported symbol if CONFIG_SPARSE_IRQ is enabled. This will
> break the builds for HYPERV as module.
> 

Instead, use irq_get_irq_data(), then irqd_to_hwirq().

Michael

Re: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Roman Kisel]

On 8/2/2024 6:20 PM, Wei Liu wrote:
> On Fri, Jul 26, 2024 at 03:59:10PM -0700, Roman Kisel wrote:
>> 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 get vPCI MSI IRQ domain in the DeviceTree
>> case, too.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   drivers/hv/vmbus_drv.c              | 23 +++++++-----
>>   drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>>   include/linux/hyperv.h              |  2 ++
>>   3 files changed, 69 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 7eee7caff5f6..038bd9be64b7 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>>   	struct hv_vmbus_device_id id;
>>   };
>>   
>> -static struct device  *hv_dev;
>> +/* VMBus Root Device */
>> +static struct device  *vmbus_root_device;
> 
> You're changing the name of the variable. That should preferably be done
> in a separate patch. That's going to make this patch shorter and easier
> to review.
> 
Will fix in v4, thanks!

>>   
>>   static int hyperv_cpuhp_online;
>>   
>> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>>   static struct resource *hyperv_mmio;
>>   static DEFINE_MUTEX(hyperv_mmio_lock);
>>   
>> +struct device *get_vmbus_root_device(void)
>> +{
>> +	return vmbus_root_device;
>> +}
>> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);
> 
> I would like you to add "hv_" prefix to this exported symbol, or rename
> it to "vmbus_get_root_device()".
> 
>> +
>>   static int vmbus_exists(void)
>>   {
>> -	if (hv_dev == NULL)
>> +	if (vmbus_root_device == NULL)
>>   		return -ENODEV;
>>   
>>   	return 0;
>> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
>>   	 * On x86/x64 coherence is assumed and these calls have no effect.
>>   	 */
>>   	hv_setup_dma_ops(child_device,
>> -		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
>> +		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>>   	return 0;
>>   }
>>   
>> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>>   		     &child_device_obj->channel->offermsg.offer.if_instance);
>>   
>>   	child_device_obj->device.bus = &hv_bus;
>> -	child_device_obj->device.parent = hv_dev;
>> +	child_device_obj->device.parent = vmbus_root_device;
>>   	child_device_obj->device.release = vmbus_device_release;
>>   
>>   	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
>> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>>   	struct acpi_device *ancestor;
>>   	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>>   
>> -	hv_dev = &device->dev;
>> +	vmbus_root_device = &device->dev;
>>   
>>   	/*
>>   	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
>> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
>>   	struct device_node *np = pdev->dev.of_node;
>>   	int ret;
>>   
>> -	hv_dev = &pdev->dev;
>> +	vmbus_root_device = &pdev->dev;
>>   
>>   	ret = of_range_parser_init(&parser, np);
>>   	if (ret)
>> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>>   	if (ret)
>>   		return ret;
>>   
>> -	if (!hv_dev) {
>> +	if (!vmbus_root_device) {
>>   		ret = -ENODEV;
>>   		goto cleanup;
>>   	}
>> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
>>   
>>   cleanup:
>>   	platform_driver_unregister(&vmbus_platform_driver);
>> -	hv_dev = NULL;
>> +	vmbus_root_device = NULL;
>>   	return ret;
>>   }
>>   
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 5992280e8110..cdecefd1f9bd 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -50,6 +50,7 @@
>>   #include <linux/irqdomain.h>
>>   #include <linux/acpi.h>
>>   #include <linux/sizes.h>
>> +#include <linux/of_irq.h>
>>   #include <asm/mshyperv.h>
>>   
>>   /*
>> @@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
>>   	.activate = hv_pci_vec_irq_domain_activate,
>>   };
>>   
>> +#ifdef CONFIG_OF
>> +
>> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
>> +{
>> +	struct device_node *parent;
>> +	struct irq_domain *domain;
>> +
>> +	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);
>> +	domain = NULL;
>> +	if (parent) {
>> +		domain = irq_find_host(parent);
>> +		of_node_put(parent);
>> +	}
>> +
> 
> I cannot really comment on the ARM side of things around how this system
> is set up. I will leave that to someone who's more familiar with the
> matter to review.
> 
> Thanks,
> Wei.

-- 
Thank you,
Roman

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

On 8/2/2024 6:21 PM, Wei Liu wrote:
> On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
>> The arm64 Hyper-V startup path relies on ACPI to detect
>> running under a Hyper-V compatible hypervisor. That
>> doesn't work on non-ACPI systems.
>>
>> Hoist the ACPI detection logic into a separate function,
>> use the new SMC added recently to Hyper-V to use in the
>> non-ACPI case.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> 
> The change looks sensible.
> 
> Within one minor comment fixed below:
> 
> Acked-by: Wei Liu <wei.liu@kernel.org>
> 
> However I would also like to get an Acked-by or reivewed-by from someone
> who works on the ARM64 side of things -- Saurabh, Boqun, Srivatsa, and
> Jinank?
> 
>> ---
>>   arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>>   arch/arm64/include/asm/mshyperv.h |  5 +++++
>>   2 files changed, 36 insertions(+), 5 deletions(-)
>>
> [...]
>> diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
>> index a975e1a689dd..a7a3586f7cb1 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
>>   
>>   #include <asm-generic/mshyperv.h>
>>   
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
> 
> Presumably these are ASCII codes for an identifier string, please
> provide comments to explain what they are.
> 
Michael posted a suggestion to change that altogether; will elaborate on 
the topic in the thread with Michael.

>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
>> +
>>   #endif
>> -- 
>> 2.34.1
>>

-- 
Thank you,
Roman

Re: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Roman Kisel]

On 8/2/2024 6:22 PM, Wei Liu wrote:
> On Fri, Jul 26, 2024 at 03:59:07PM -0700, Roman Kisel wrote:
>> 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 <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/Makefile        |  1 +
>>   arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>>   arch/arm64/hyperv/mshyperv.c      |  4 ++++
>>   arch/arm64/include/asm/mshyperv.h |  7 +++++++
>>   4 files changed, 25 insertions(+)
>>   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..38642b7b6be0
>> --- /dev/null
>> +++ b/arch/arm64/hyperv/hv_vtl.c
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2024, Microsoft, Inc.
>> + *
>> + * Author : Roman Kisel <romank@linux.microsoft.com>
>> + */
>> +
>> +#include <asm/mshyperv.h>
>> +
>> +void __init hv_vtl_init_platform(void)
>> +{
>> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
>> +}
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index 341f98312667..8fd04d6e4800 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>>   		return ret;
>>   	}
>>   
>> +	/* Find the VTL */
>> +	ms_hyperv.vtl = get_vtl();
>> +	hv_vtl_init_platform();
> 
> It doesn't make sense to me because this function unconditionally prints
> Linux runs in Hyper-V Virtual Trust Level.
> 
Thought to structure this as the similar parts of the VTL support are. 
Will remove, thank you!

> Thanks,
> Wei.
> 
>> +
>>   	ms_hyperv_late_init();
>>   
>>   	hyperv_initialized = true;
>> diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
>> index a7a3586f7cb1..63d6bb6998fc 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -49,6 +49,13 @@ 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) {}
>> +#endif
>> +
>>   #include <asm-generic/mshyperv.h>
>>   
>>   #define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
>> -- 
>> 2.34.1
>>

-- 
Thank you,
Roman

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote:
> On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
>> The arm64 Hyper-V startup path relies on ACPI to detect
>> running under a Hyper-V compatible hypervisor. That
>> doesn't work on non-ACPI systems.
>>
>> Hoist the ACPI detection logic into a separate function,
>> use the new SMC added recently to Hyper-V to use in the
>> non-ACPI case.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>>   arch/arm64/include/asm/mshyperv.h |  5 +++++
>>   2 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..341f98312667 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>   	return 0;
>>   }
>>   
>> +static bool hyperv_detect_via_acpi(void)
>> +{
>> +	if (acpi_disabled)
>> +		return false;
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +	/* Hypervisor ID is only available in ACPI v6+. */
>> +	if (acpi_gbl_FADT.header.revision < 6)
>> +		return false;
>> +	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
>> +#else
>> +	return false;
>> +#endif
>> +}
>> +
>> +static bool hyperv_detect_via_smc(void)
>> +{
>> +	struct arm_smccc_res res = {};
>> +
>> +	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
>> +		return false;
>> +	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
>> +
>> +	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
>> +		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
>> +		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
>> +		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
>> +}
> 
> As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor,
> can we add a comment about it, specifying the exact version in it would be great.
> 
I can add a comment about that, thought that would look as too much 
detail to refer to a version of the Windows insiders build in the 
comments in this code. Another option would be to entrench the logic in 
if statements which felt gross as there is a fallback.

> If someone attempts to build non-ACPI kernel on older Hyper-V what is the
> behaviour of this function, do we need to safeguard or handle that case ?
The function won't panic if that's what you're asking about, i.e. safe 
for runtime. That won't break the build either as it relies on the SMCCC 
spec, and that uses the smc or hvc instructions (the code does expect 
hvc to be the conduit and checks for that being the case). The 
hypervisor doesn't inject the exception in the guest for the unknown 
call, just returns SMCCC_RET_NOT_SUPPORTED in the first output register 
(the hypervisor got a unit-test for that, too).

That said, I think the logic is solid. Appreciate your question and your 
help! Will be glad to discuss other concerns should you have any.

> 
> - Saurabh

-- 
Thank you,
Roman

Re: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Roman Kisel]

On 8/4/2024 9:05 PM, Saurabh Singh Sengar wrote:
> On Mon, Aug 05, 2024 at 03:01:58AM +0000, Michael Kelley wrote:
>> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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 <romank@linux.microsoft.com>
>>> ---
>>>   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.34.1
>>>
>>
>> In v2 of this patch, I suggested [1] making a couple additional minor changes
>> so that kernels built *without* HYPER_VTL_MODE would still require
>> ACPI.  Did that suggestion not work out?  If that's the case, I'm curious
>> about what goes wrong.
> 
> Hi Michael/Roman,
> I was considering making HYPERV_VTL_MODE depend on CONFIG_OF. That should address
> above concern as well. Do you see any potential issue with it.
> 
Michael,

I ran into a pretty gnarly recursive dependencies which in all fairness 
might stem from not being fluent enough in the Kconfig language. Any 
help of how to approach implementing your idea would be greatly appreciated!

Saurabh,

I could try out the idea you're offering if you folks are fine with 
that. Or, we could let this be for the time being and grapple with that 
in a separate patch series :)

> - Saurabh

-- 
Thank you,
Roman

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Saurabh Singh Sengar]

On Mon, Aug 05, 2024 at 08:17:05AM -0700, Roman Kisel wrote:
> 
> 
> On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote:
> >On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
> >>The arm64 Hyper-V startup path relies on ACPI to detect
> >>running under a Hyper-V compatible hypervisor. That
> >>doesn't work on non-ACPI systems.
> >>
> >>Hoist the ACPI detection logic into a separate function,
> >>use the new SMC added recently to Hyper-V to use in the
> >>non-ACPI case.
> >>
> >>Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> >>---
> >>  arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
> >>  arch/arm64/include/asm/mshyperv.h |  5 +++++
> >>  2 files changed, 36 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> >>index b1a4de4eee29..341f98312667 100644
> >>--- a/arch/arm64/hyperv/mshyperv.c
> >>+++ b/arch/arm64/hyperv/mshyperv.c
> >>@@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
> >>  	return 0;
> >>  }
> >>+static bool hyperv_detect_via_acpi(void)
> >>+{
> >>+	if (acpi_disabled)
> >>+		return false;
> >>+#if IS_ENABLED(CONFIG_ACPI)
> >>+	/* Hypervisor ID is only available in ACPI v6+. */
> >>+	if (acpi_gbl_FADT.header.revision < 6)
> >>+		return false;
> >>+	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
> >>+#else
> >>+	return false;
> >>+#endif
> >>+}
> >>+
> >>+static bool hyperv_detect_via_smc(void)
> >>+{
> >>+	struct arm_smccc_res res = {};
> >>+
> >>+	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
> >>+		return false;
> >>+	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
> >>+
> >>+	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
> >>+		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
> >>+		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
> >>+		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
> >>+}
> >
> >As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor,
> >can we add a comment about it, specifying the exact version in it would be great.
> >
> I can add a comment about that, thought that would look as too much
> detail to refer to a version of the Windows insiders build in the
> comments in this code. Another option would be to entrench the logic
> in if statements which felt gross as there is a fallback.

I'll leave the decision to your judgment.

> 
> >If someone attempts to build non-ACPI kernel on older Hyper-V what is the
> >behaviour of this function, do we need to safeguard or handle that case ?
> The function won't panic if that's what you're asking about, i.e.
> safe for runtime. That won't break the build either as it relies on
> the SMCCC spec, and that uses the smc or hvc instructions (the code
> does expect hvc to be the conduit and checks for that being the
> case). The hypervisor doesn't inject the exception in the guest for
> the unknown call, just returns SMCCC_RET_NOT_SUPPORTED in the first
> output register (the hypervisor got a unit-test for that, too).

Looks good, have you considered checking for SMCCC_RET_NOT_SUPPORTED ?

- Saurabh

Re: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Roman Kisel]

On 8/4/2024 11:28 PM, Saurabh Singh Sengar wrote:
> On Fri, Jul 26, 2024 at 03:59:07PM -0700, Roman Kisel wrote:
>> 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 <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/Makefile        |  1 +
>>   arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>>   arch/arm64/hyperv/mshyperv.c      |  4 ++++
>>   arch/arm64/include/asm/mshyperv.h |  7 +++++++
>>   4 files changed, 25 insertions(+)
>>   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..38642b7b6be0
>> --- /dev/null
>> +++ b/arch/arm64/hyperv/hv_vtl.c
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2024, Microsoft, Inc.
>> + *
>> + * Author : Roman Kisel <romank@linux.microsoft.com>
>> + */
>> +
>> +#include <asm/mshyperv.h>
>> +
>> +void __init hv_vtl_init_platform(void)
>> +{
>> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
>> +}
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index 341f98312667..8fd04d6e4800 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>>   		return ret;
>>   	}
>>   
>> +	/* Find the VTL */
>> +	ms_hyperv.vtl = get_vtl();
>> +	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 a7a3586f7cb1..63d6bb6998fc 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -49,6 +49,13 @@ 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) {}
>> +#endif
>> +
> 
> These functions are defined in x86 header as well. We can move it to generic header.
> 
Will do, thanks!

> - Saurabh

-- 
Thank you,
Roman

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel]

On 8/5/2024 7:12 AM, Michael Kelley wrote:
> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, August 5, 2024 1:30 AM
>>
>> On Fri, Jul 26, 2024 at 03:59:09PM -0700, 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 and indicate DMA cache coherency.
>>>
>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>>> ---
>>>   drivers/hv/vmbus_drv.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>>> index 12a707ab73f8..7eee7caff5f6 100644
>>> --- a/drivers/hv/vmbus_drv.c
>>> +++ b/drivers/hv/vmbus_drv.c
>>> @@ -2306,6 +2306,34 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>>>   }
>>>   #endif
>>>
>>> +static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
>>> +{
>>> +	struct irq_desc *desc;
>>> +	int irq;
>>> +
>>> +	irq = platform_get_irq(pdev, 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);
>>
>> irq_to_desc is not an exported symbol if CONFIG_SPARSE_IRQ is enabled. This will
>> break the builds for HYPERV as module.
>>
> 
> Instead, use irq_get_irq_data(), then irqd_to_hwirq().
> 
Couldn't appreciate enough your indispensable advice, folks!

> Michael

-- 
Thank you,
Roman

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

On 8/5/2024 8:46 AM, Saurabh Singh Sengar wrote:
> On Mon, Aug 05, 2024 at 08:17:05AM -0700, Roman Kisel wrote:
>>
>>
>> On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote:
>>> On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote:
>>>> The arm64 Hyper-V startup path relies on ACPI to detect
>>>> running under a Hyper-V compatible hypervisor. That
>>>> doesn't work on non-ACPI systems.
>>>>
>>>> Hoist the ACPI detection logic into a separate function,
>>>> use the new SMC added recently to Hyper-V to use in the
>>>> non-ACPI case.
>>>>
>>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>>>> ---
>>>>   arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>>>>   arch/arm64/include/asm/mshyperv.h |  5 +++++
>>>>   2 files changed, 36 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>>>> index b1a4de4eee29..341f98312667 100644
>>>> --- a/arch/arm64/hyperv/mshyperv.c
>>>> +++ b/arch/arm64/hyperv/mshyperv.c
>>>> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>>>   	return 0;
>>>>   }
>>>> +static bool hyperv_detect_via_acpi(void)
>>>> +{
>>>> +	if (acpi_disabled)
>>>> +		return false;
>>>> +#if IS_ENABLED(CONFIG_ACPI)
>>>> +	/* Hypervisor ID is only available in ACPI v6+. */
>>>> +	if (acpi_gbl_FADT.header.revision < 6)
>>>> +		return false;
>>>> +	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
>>>> +#else
>>>> +	return false;
>>>> +#endif
>>>> +}
>>>> +
>>>> +static bool hyperv_detect_via_smc(void)
>>>> +{
>>>> +	struct arm_smccc_res res = {};
>>>> +
>>>> +	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
>>>> +		return false;
>>>> +	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
>>>> +
>>>> +	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
>>>> +		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
>>>> +		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
>>>> +		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
>>>> +}
>>>
>>> As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor,
>>> can we add a comment about it, specifying the exact version in it would be great.
>>>
>> I can add a comment about that, thought that would look as too much
>> detail to refer to a version of the Windows insiders build in the
>> comments in this code. Another option would be to entrench the logic
>> in if statements which felt gross as there is a fallback.
> 
> I'll leave the decision to your judgment.
> 
>>
>>> If someone attempts to build non-ACPI kernel on older Hyper-V what is the
>>> behaviour of this function, do we need to safeguard or handle that case ?
>> The function won't panic if that's what you're asking about, i.e.
>> safe for runtime. That won't break the build either as it relies on
>> the SMCCC spec, and that uses the smc or hvc instructions (the code
>> does expect hvc to be the conduit and checks for that being the
>> case). The hypervisor doesn't inject the exception in the guest for
>> the unknown call, just returns SMCCC_RET_NOT_SUPPORTED in the first
>> output register (the hypervisor got a unit-test for that, too).
> 
> Looks good, have you considered checking for SMCCC_RET_NOT_SUPPORTED ?
> 
No, I have not. Let me think out loud here... `a0` is compared to what 
must be return from the hypervisor the UID. That constant is an all-1 32 
or 64 bit pattern, high unlikely to see that as a part of the UID due to 
low entropy as I understand. I might've added the check though for the 
better code readability, and because we have this e-mail thread going 
on, looks like I must :) Let me do that in v4, thanks!

> - Saurabh

-- 
Thank you,
Roman

Re: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Roman Kisel]

On 8/5/2024 7:51 AM, Roman Kisel wrote:
> 
> 
> On 8/2/2024 6:20 PM, Wei Liu wrote:
>> On Fri, Jul 26, 2024 at 03:59:10PM -0700, Roman Kisel wrote:
>>> 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 get vPCI MSI IRQ domain in the 
>>> DeviceTree
>>> case, too.
>>>
>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>>> ---
>>>   drivers/hv/vmbus_drv.c              | 23 +++++++-----
>>>   drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>>>   include/linux/hyperv.h              |  2 ++
>>>   3 files changed, 69 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>>> index 7eee7caff5f6..038bd9be64b7 100644
>>> --- a/drivers/hv/vmbus_drv.c
>>> +++ b/drivers/hv/vmbus_drv.c
>>> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>>>       struct hv_vmbus_device_id id;
>>>   };
>>> -static struct device  *hv_dev;
>>> +/* VMBus Root Device */
>>> +static struct device  *vmbus_root_device;
>>
>> You're changing the name of the variable. That should preferably be done
>> in a separate patch. That's going to make this patch shorter and easier
>> to review.
>>
> Will fix in v4, thanks!
> 
>>>   static int hyperv_cpuhp_online;
>>> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>>>   static struct resource *hyperv_mmio;
>>>   static DEFINE_MUTEX(hyperv_mmio_lock);
>>> +struct device *get_vmbus_root_device(void)
>>> +{
>>> +    return vmbus_root_device;
>>> +}
>>> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);
>>
>> I would like you to add "hv_" prefix to this exported symbol, or rename
>> it to "vmbus_get_root_device()".
>>
Will do in v4, missed this suggestion in my first reply.

>>> +
>>>   static int vmbus_exists(void)
>>>   {
>>> -    if (hv_dev == NULL)
>>> +    if (vmbus_root_device == NULL)
>>>           return -ENODEV;
>>>       return 0;
>>> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device 
>>> *child_device)
>>>        * On x86/x64 coherence is assumed and these calls have no effect.
>>>        */
>>>       hv_setup_dma_ops(child_device,
>>> -        device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
>>> +        device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>>>       return 0;
>>>   }
>>> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device 
>>> *child_device_obj)
>>>                &child_device_obj->channel->offermsg.offer.if_instance);
>>>       child_device_obj->device.bus = &hv_bus;
>>> -    child_device_obj->device.parent = hv_dev;
>>> +    child_device_obj->device.parent = vmbus_root_device;
>>>       child_device_obj->device.release = vmbus_device_release;
>>>       child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
>>> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct 
>>> platform_device *pdev)
>>>       struct acpi_device *ancestor;
>>>       struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>>> -    hv_dev = &device->dev;
>>> +    vmbus_root_device = &device->dev;
>>>       /*
>>>        * Older versions of Hyper-V for ARM64 fail to include the _CCA
>>> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct 
>>> platform_device *pdev)
>>>       struct device_node *np = pdev->dev.of_node;
>>>       int ret;
>>> -    hv_dev = &pdev->dev;
>>> +    vmbus_root_device = &pdev->dev;
>>>       ret = of_range_parser_init(&parser, np);
>>>       if (ret)
>>> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>>>       if (ret)
>>>           return ret;
>>> -    if (!hv_dev) {
>>> +    if (!vmbus_root_device) {
>>>           ret = -ENODEV;
>>>           goto cleanup;
>>>       }
>>> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
>>>   cleanup:
>>>       platform_driver_unregister(&vmbus_platform_driver);
>>> -    hv_dev = NULL;
>>> +    vmbus_root_device = NULL;
>>>       return ret;
>>>   }
>>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/ 
>>> controller/pci-hyperv.c
>>> index 5992280e8110..cdecefd1f9bd 100644
>>> --- a/drivers/pci/controller/pci-hyperv.c
>>> +++ b/drivers/pci/controller/pci-hyperv.c
>>> @@ -50,6 +50,7 @@
>>>   #include <linux/irqdomain.h>
>>>   #include <linux/acpi.h>
>>>   #include <linux/sizes.h>
>>> +#include <linux/of_irq.h>
>>>   #include <asm/mshyperv.h>
>>>   /*
>>> @@ -887,6 +888,35 @@ static const struct irq_domain_ops 
>>> hv_pci_domain_ops = {
>>>       .activate = hv_pci_vec_irq_domain_activate,
>>>   };
>>> +#ifdef CONFIG_OF
>>> +
>>> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
>>> +{
>>> +    struct device_node *parent;
>>> +    struct irq_domain *domain;
>>> +
>>> +    parent = 
>>> of_irq_find_parent(to_platform_device(get_vmbus_root_device())- 
>>> >dev.of_node);
>>> +    domain = NULL;
>>> +    if (parent) {
>>> +        domain = irq_find_host(parent);
>>> +        of_node_put(parent);
>>> +    }
>>> +
>>
>> I cannot really comment on the ARM side of things around how this system
>> is set up. I will leave that to someone who's more familiar with the
>> matter to review.
>>
>> Thanks,
>> Wei.
> 

-- 
Thank you,
Roman

Re: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Roman Kisel]

On 8/4/2024 8:02 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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. Fix the hypercall output address in `get_vtl(void)`
>> not to overlap with the hypercall input area to adhere to
>> the Hyper-V TLFS.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/x86/hyperv/hv_init.c          | 34 ---------------------
>>   arch/x86/include/asm/hyperv-tlfs.h |  7 -----
>>   drivers/hv/hv_common.c             | 47 ++++++++++++++++++++++++++++--
>>   include/asm-generic/hyperv-tlfs.h  |  7 +++++
>>   include/asm-generic/mshyperv.h     |  6 ++++
>>   5 files changed, 58 insertions(+), 43 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 9c452bfbd571..7d6c1523b0b5 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -339,8 +339,8 @@ int __init hv_common_init(void)
>>   	hyperv_pcpu_input_arg = alloc_percpu(void  *);
>>   	BUG_ON(!hyperv_pcpu_input_arg);
>>
>> -	/* Allocate the per-CPU state for output arg for root */
>> -	if (hv_root_partition) {
>> +	/* Allocate the per-CPU state for output arg for root or a VTL */
>> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>>   		hyperv_pcpu_output_arg = alloc_percpu(void *);
>>   		BUG_ON(!hyperv_pcpu_output_arg);
>>   	}
>> @@ -656,3 +656,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 = *this_cpu_ptr(hyperv_pcpu_output_arg);
> 
> Rather than use the hyperv_pcpu_output_arg here, it's OK to
> use a different area of the hyperv_pcpu_input_arg page.  For
> example,
> 
> 	output = (void *)input + HV_HYP_PAGE_SIZE/2;
> 
> The TLFS does not require that the input and output be in
> separate pages.
> 
> While using the hyperv_pcpu_output_arg is conceptually a
> bit cleaner, doing so requires allocating a 4K page per CPU that
> is not otherwise used. The VTL 2 code wants to be frugal with
> memory, and this seems like a good step in that direction. :-)
> 
I agree on the both counts: the code looks conceptually cleaner now and 
VTL2 wants to be frugal with memory, esp that the output hypercall page 
is per-CPU so we have O(n) as the CPU count increases. Still, the output 
page will be needed for VTL2 (say to get/set registers just as done 
here). That said, with this patch we can achieve both the conceptual 
cleanliness and being ready to grow more on the primitives being built 
out in the VTL support patches.

> The hyperv_pcpu_output_arg was added for the cases where up
> to a full page is needed for input and output on the same hypercall.
> So far, the only case of that is when running in the root partition.
> 
>> +
>> +	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 814207e7c37f..271c365973d6 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.
> 
> s/there is no an associated/there is no associated/
> 
Much appreciated, will fix this!

>> + */
>> +#define	HV_REGISTER_VSM_VP_STATUS	0x000D0003
>> +#define	HV_VTL_MASK			GENMASK(3, 0)
> 
> Further down in hyperv-tlfs.h is a section devoted to register
> definitions. It seems like this definition should go there in
> numeric order, which is after HV_REGISTER_STIMER0_COUNT.
> 
Agreed, will fix, thanks!

> Michael
> 
>> +
>>   /*
>>    * Group B features.
>>    */
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index 8fe7aaab2599..85a5b8cb1702 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -315,4 +315,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.34.1
>>

-- 
Thank you,
Roman

Re: [PATCH v3 4/7] arm64: hyperv: Boot in a Virtual Trust Level

[Roman Kisel]

On 8/4/2024 8:02 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 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 <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/Makefile        |  1 +
>>   arch/arm64/hyperv/hv_vtl.c        | 13 +++++++++++++
>>   arch/arm64/hyperv/mshyperv.c      |  4 ++++
>>   arch/arm64/include/asm/mshyperv.h |  7 +++++++
>>   4 files changed, 25 insertions(+)
>>   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..38642b7b6be0
>> --- /dev/null
>> +++ b/arch/arm64/hyperv/hv_vtl.c
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2024, Microsoft, Inc.
>> + *
>> + * Author : Roman Kisel <romank@linux.microsoft.com>
>> + */
>> +
>> +#include <asm/mshyperv.h>
>> +
>> +void __init hv_vtl_init_platform(void)
>> +{
>> +	pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
>> +}
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index 341f98312667..8fd04d6e4800 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -98,6 +98,10 @@ static int __init hyperv_init(void)
>>   		return ret;
>>   	}
>>
>> +	/* Find the VTL */
>> +	ms_hyperv.vtl = get_vtl();
>> +	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 a7a3586f7cb1..63d6bb6998fc 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -49,6 +49,13 @@ 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);
> 
> This declaration is spurious since you removed the function.
> 
Will clean this up, thanks!

> Michael
> 
>> +#else
>> +static inline void __init hv_vtl_init_platform(void) {}
>> +#endif
>> +
>>   #include <asm-generic/mshyperv.h>
>>
>>   #define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
>> --
>> 2.34.1
>>

-- 
Thank you,
Roman

Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel]

On 8/4/2024 8:03 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, July 29, 2024 9:51 AM
>>
>>
>> On 7/27/2024 2:17 AM, Arnd Bergmann wrote:
>>> On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
>>>> On 27/07/2024 00:59, Roman Kisel wrote:
>>>>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>>>>    		cur_res = &res->sibling;
>>>>>    	}
>>>>>
>>>>> +	/*
>>>>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>>>>> +	 * might default to 'not coherent' on some architectures.
>>>>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>>>>> +	 */
>>>>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>>>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>>>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>>>>> +
>>>>> +	if (!of_property_read_bool(np, "dma-coherent"))
>>>>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
>>>>
>>>> Why do you need this property at all, if it is allways dma-coherent? Are
>>>> you supporting dma-noncoherent somewhere?
>>>
>>> It's just a sanity check that the DT is well-formed.
> 
> In my view, this chunk of code can be dropped entirely. The guest
> should believe what the Hyper-V host tells it via DT, and that includes
> operating in non-coherent mode. There might be some future case
> where non-coherent DMA is correct. In such a case, we don't want to
> have to come back and remove an overly aggressive sanity test from
> Linux kernel code.
> 
> As Arnd noted, the dma-coherent (or dma-noncoherent) property should
> be interpreted and applied to the device by common code. If that's not
> working for some reason in this case, we should investigate why not.
> 
> Note that the ACPI code for VMBus does the same thing -- it believes and
> uses whatever the _CCA property says. The exception is that there
> are deployed version of Hyper-V that don't set _CCA at all, contrary to the
> ACPI spec. So there's a hack in vmbus_acpi_add() to work around this case
> and force coherent_dma. But that's the only place where the current
> Hyper-V assumption of coherence comes into play. I sincerely hope Hyper-V
> ensures that the DT correctly includes dma-coherent from the start, and
> that we don't have to replicate the hack on the DT side.
> 
I was replicating the _CCA hack diligently a bit much too much, agreed. 
This great conversation really gives me reassurance that the code 
doesn't have to be paranoid, and I can happily remove this if statement.

> Michael
> 
>>>
>>> Since the dma-coherent property is interpreted by common code, it's
>>> not up to hv to change the default for the platform. I'm not sure
>>> if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
>>> check to determine that an architecture defaults to noncoherent
>>> though, as the function may be needed to do something else.
>> I used the ifdef as the dma_coherent field is declared under these macros:
>>
>> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>> extern bool dma_default_coherent;
>> static inline bool dev_is_dma_coherent(struct device *dev)
>> {
>> 	return dev->dma_coherent;
>> }
>> #else
>> #define dma_default_coherent true
>>
>> static inline bool dev_is_dma_coherent(struct device *dev)
>> {
>> 	return true;
>> }
>>
>> i.e., there is no API to set dma_coherent. As I see it, the options
>> are either warn the user if they forgot to add `dma-coherent`
>>
>> if (!dev_is_dma_coherent(dev)) pr_warn("add dma-coherent to be faster\n"),
>>
>> or warn and force the flag to true. Maybe just warn
>> the user I think now... The code will be cleaner (no need to emulate
>> a-would-be set_dma_coherent) , and the user will
>> know how to make the system perform at its best.
>>
>> Appreciate sharing the reservations about that piece!
>>
>>>
>>> The global "dma_default_coherent' may be a better thing to check
>>> for. This is e.g. set on powerpc64, riscv and on specific mips
>>> platforms, but it's never set on arm64 as far as I can tell.
>>>
>>>        Arnd
>>
>> --
>> Thank you,
>> Roman
>>

-- 
Thank you,
Roman

Re: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Roman Kisel]

On 8/4/2024 8:03 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM
>>
>> 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 get vPCI MSI IRQ domain in the DeviceTree
>> case, too.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> 
> Overall, this makes sense to me, and I think it works. As you noted in the cover
> letter for the patch series, it's a bit messy.  But see my two comments below.
> 
>> ---
>>   drivers/hv/vmbus_drv.c              | 23 +++++++-----
>>   drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>>   include/linux/hyperv.h              |  2 ++
>>   3 files changed, 69 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 7eee7caff5f6..038bd9be64b7 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>>   	struct hv_vmbus_device_id id;
>>   };
>>
>> -static struct device  *hv_dev;
>> +/* VMBus Root Device */
>> +static struct device  *vmbus_root_device;
>>
>>   static int hyperv_cpuhp_online;
>>
>> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>>   static struct resource *hyperv_mmio;
>>   static DEFINE_MUTEX(hyperv_mmio_lock);
>>
>> +struct device *get_vmbus_root_device(void)
>> +{
>> +	return vmbus_root_device;
>> +}
>> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);
>> +
>>   static int vmbus_exists(void)
>>   {
>> -	if (hv_dev == NULL)
>> +	if (vmbus_root_device == NULL)
>>   		return -ENODEV;
>>
>>   	return 0;
>> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
>>   	 * On x86/x64 coherence is assumed and these calls have no effect.
>>   	 */
>>   	hv_setup_dma_ops(child_device,
>> -		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
>> +		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>>   	return 0;
>>   }
>>
>> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>>   		     &child_device_obj->channel->offermsg.offer.if_instance);
>>
>>   	child_device_obj->device.bus = &hv_bus;
>> -	child_device_obj->device.parent = hv_dev;
>> +	child_device_obj->device.parent = vmbus_root_device;
>>   	child_device_obj->device.release = vmbus_device_release;
>>
>>   	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
>> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>>   	struct acpi_device *ancestor;
>>   	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>>
>> -	hv_dev = &device->dev;
>> +	vmbus_root_device = &device->dev;
>>
>>   	/*
>>   	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
>> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
>>   	struct device_node *np = pdev->dev.of_node;
>>   	int ret;
>>
>> -	hv_dev = &pdev->dev;
>> +	vmbus_root_device = &pdev->dev;
>>
>>   	ret = of_range_parser_init(&parser, np);
>>   	if (ret)
>> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>>   	if (ret)
>>   		return ret;
>>
>> -	if (!hv_dev) {
>> +	if (!vmbus_root_device) {
>>   		ret = -ENODEV;
>>   		goto cleanup;
>>   	}
>> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
>>
>>   cleanup:
>>   	platform_driver_unregister(&vmbus_platform_driver);
>> -	hv_dev = NULL;
>> +	vmbus_root_device = NULL;
>>   	return ret;
>>   }
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 5992280e8110..cdecefd1f9bd 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -50,6 +50,7 @@
>>   #include <linux/irqdomain.h>
>>   #include <linux/acpi.h>
>>   #include <linux/sizes.h>
>> +#include <linux/of_irq.h>
>>   #include <asm/mshyperv.h>
>>
>>   /*
>> @@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
>>   	.activate = hv_pci_vec_irq_domain_activate,
>>   };
>>
>> +#ifdef CONFIG_OF
>> +
>> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
>> +{
>> +	struct device_node *parent;
>> +	struct irq_domain *domain;
>> +
>> +	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);
> 
> I think the above can be simplified to:
> 
> 	parent = of_irq_find_parent(get_vmbus_root_device()->of_node);
> 
> Converting the vmbus_root_device to the platform device, and then accessing
> the "dev" field of the platform device puts you back where you started with
> the vmbus_root_device.  See the code in vmbus_device_add() where the
> vmbus_root_device is set to the dev field of the platform device.
> 
Appreciate your help with making this code looks better! Will try your 
suggestion out!

>> +	domain = NULL;
>> +	if (parent) {
>> +		domain = irq_find_host(parent);
>> +		of_node_put(parent);
>> +	}
>> +
>> +	/*
>> +	 * `domain == NULL` shouldn't happen.
>> +	 *
>> +	 * If somehow the code does end up in that state, treat this as a configuration
>> +	 * issue rather than a hard error, emit a warning, and let the code proceed.
>> +	 * The NULL parent domain is an acceptable option for the `irq_domain_create_hierarchy`
>> +	 * function called later.
>> +	 */
>> +	if (!domain)
>> +		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
>> +	return domain;
>> +}
> 
> Here's a thought, which may or may not be a good one:  Push some or all
> the functionality of hv_pci_of_irq_domain_parent() into vmbus_device_add().
> In the simplest case, have vmbus_device_add() store the of_node (which is
> already the "np" local variable) in a global static variable, and provide
> hv_get_vmbus_of_node() instead of get_vmbus_root_device().
> 
> The next step to consider would be to compute the parent in
> vmbus_device_add(), and provide hv_get_vmbus_parent_of_node() instead
> of hv_get_vmbus_of_node(). One difference is that vmbus_device_add()
> runs for both x86 and arm64, while hv_pci_of_irq_domain_parent() is for
> arm64 only.  The parent node may not exist on x86, but maybe that isn't
> really a problem.
> 
> Pushing everything into vmbus_device_add() would entail doing the
> irq_find_host(parent) as well, and the accessor function would just
> return the IRQ domain. In that case, hv_pci_of_irq_domain_parent()
> can go away. The domain might be NULL on x86, but that's OK
> because the accessor function won't be called on x86.
> 
> Maybe there's a snag that prevents this idea from working well,
> particularly on x86 where the domain will be NULL. But if it works,
> it seems slightly less messy to me, though that is a judgment call.
> I'll leave it to you to decide, and I'm OK either way. :-)
> 
I'll explore your great idea, thanks for sharing! Since v1 this code has 
improved quite a bit thanks to your suggestions :)

> Michael
> 
>> +
>> +#endif
>> +
>>   static int hv_pci_irqchip_init(void)
>>   {
>>   	static struct hv_pci_chip_data *chip_data;
>> @@ -906,10 +936,29 @@ static int hv_pci_irqchip_init(void)
>>   	 * IRQ domain once enabled, should not be removed since there is no
>>   	 * way to ensure that all the corresponding devices are also gone and
>>   	 * no interrupts will be generated.
>> +	 *
>> +	 * In the ACPI case, the parent IRQ domain is supplied by the ACPI
>> +	 * subsystem, and it is the default GSI domain pointing to the GIC.
>> +	 * Neither is available outside of the ACPI subsystem, cannot avoid
>> +	 * the messy ifdef below.
>> +	 * There is apparently no such default in the OF subsystem, and
>> +	 * `hv_pci_of_irq_domain_parent` finds the parent IRQ domain that
>> +	 * points to the GIC as well.
>> +	 * None of these two cases reaches for the MSI parent domain.
>>   	 */
>> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> -							  fn, &hv_pci_domain_ops,
>> -							  chip_data);
>> +#ifdef CONFIG_ACPI
>> +	if (!acpi_disabled)
>> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
>> +#endif
>> +#if defined(CONFIG_OF)
>> +	if (!hv_msi_gic_irq_domain)
>> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
>> +			hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
>> +#endif
>>
>>   	if (!hv_msi_gic_irq_domain) {
>>   		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
>> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
>> index 5e39baa7f6cb..b4aa1f579a97 100644
>> --- a/include/linux/hyperv.h
>> +++ b/include/linux/hyperv.h
>> @@ -1346,6 +1346,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
>>   	return dev_get_drvdata(&dev->device);
>>   }
>>
>> +struct device *get_vmbus_root_device(void);
>> +
>>   struct hv_ring_buffer_debug_info {
>>   	u32 current_interrupt_mask;
>>   	u32 current_read_index;
>> --
>> 2.34.1
>>

-- 
Thank you,
Roman

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

On 8/4/2024 8:01 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM
>>
>> The arm64 Hyper-V startup path relies on ACPI to detect
>> running under a Hyper-V compatible hypervisor. That
>> doesn't work on non-ACPI systems.
>>
>> Hoist the ACPI detection logic into a separate function,
>> use the new SMC added recently to Hyper-V to use in the
>> non-ACPI case.
> 
> Wording seems slightly messed up.  Perhaps:
> 
>     Hoist the ACPI detection logic into a separate function. Then
>     use the new SMC added recently to Hyper-V in the non-ACPI
>     case.
> 
> Also, the phrase "the new SMC" seems a bit off to me.  The "Terms and
> Abbreviations" section of the SMCCC specification defines "SMC" as
> an instruction:
> 
>     Secure Monitor Call. An Arm assembler instruction that causes an
>     exception that is taken synchronously into EL3.
> 
> More precisely, I think you mean a SMC "function identifier" that is
> newly implemented by Hyper-V.  And the function identifier itself isn't
> new; it's the Hyper-V implementation that's new.
> 
> Similar comment applies in the cover letter for this patch set, and
> perhaps to the Subject line of this patch.
> 
Will fix the wording, appreciate your help with bringing this into the 
best shape possible!

>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   arch/arm64/hyperv/mshyperv.c      | 36 ++++++++++++++++++++++++++-----
>>   arch/arm64/include/asm/mshyperv.h |  5 +++++
>>   2 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
>> index b1a4de4eee29..341f98312667 100644
>> --- a/arch/arm64/hyperv/mshyperv.c
>> +++ b/arch/arm64/hyperv/mshyperv.c
>> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
>>   	return 0;
>>   }
>>
>> +static bool hyperv_detect_via_acpi(void)
>> +{
>> +	if (acpi_disabled)
>> +		return false;
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +	/* Hypervisor ID is only available in ACPI v6+. */
>> +	if (acpi_gbl_FADT.header.revision < 6)
>> +		return false;
>> +	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
>> +#else
>> +	return false;
>> +#endif
>> +}
>> +
>> +static bool hyperv_detect_via_smc(void)
>> +{
>> +	struct arm_smccc_res res = {};
>> +
>> +	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
>> +		return false;
>> +	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
>> +
>> +	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
>> +		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
>> +		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
>> +		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
>> +}
>> +
>>   static int __init hyperv_init(void)
>>   {
>>   	struct hv_get_vp_registers_output	result;
>> @@ -35,13 +63,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_via_acpi() && !hyperv_detect_via_smc())
>>   		return 0;
>>
>>   	/* Setup the guest ID */
>> diff --git a/arch/arm64/include/asm/mshyperv.h
>> b/arch/arm64/include/asm/mshyperv.h
>> index a975e1a689dd..a7a3586f7cb1 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
>>
>>   #include <asm-generic/mshyperv.h>
>>
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
>> +
> 
> Section 6.2 of the SMCCC specification says that the "Call UID Query"
> returns a UUID. The above #defines look like an ASCII string is being
> returned. Arguably the ASCII string can be treated as a set of 128 bits
> just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V
> be changed to return a real UUID? While the distinction probably
> won't make a material difference here, we've had problems in the past
> with Hyper-V doing slightly weird things that later caused unexpected
> trouble. Please just get it right. :-)
> 
The above values don't violate anything in the spec, and I think it 
would be hard to give an example of what can be broken in the world by 
using the above values. I do understand what you're saying when you 
mention the UIDs & the spirit of the spec. Put on the quantitative 
footing, the Shannon entropy of these values is much lower than that of 
an UID. A cursory search in the kernel tree does turn up other UIDs that 
don't look too random.

As that is implemented only in the non-release versions, hardly someone 
has taken a dependency on the specific values in their production code. 
I guess that can be changed without causing any disturbance to the 
customers, will report of any concerns though.

> Michael
> 
>>   #endif
>> --
>> 2.34.1
>>

-- 
Thank you,
Roman

RE: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 8:24 AM
> 
> On 8/4/2024 9:05 PM, Saurabh Singh Sengar wrote:
> > On Mon, Aug 05, 2024 at 03:01:58AM +0000, Michael Kelley wrote:
> >> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59
> 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 <romank@linux.microsoft.com>
> >>> ---
> >>>   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.34.1
> >>>
> >>
> >> In v2 of this patch, I suggested [1] making a couple additional minor changes
> >> so that kernels built *without* HYPER_VTL_MODE would still require
> >> ACPI.  Did that suggestion not work out?  If that's the case, I'm curious
> >> about what goes wrong.
> >
> > Hi Michael/Roman,
> > I was considering making HYPERV_VTL_MODE depend on CONFIG_OF. That should
> address
> > above concern as well. Do you see any potential issue with it.
> >
> Michael,
> 
> I ran into a pretty gnarly recursive dependencies which in all fairness
> might stem from not being fluent enough in the Kconfig language. Any
> help of how to approach implementing your idea would be greatly appreciated!
> 

This is what I had in mind:

--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -5,7 +5,8 @@ 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)
+       depends on (ACPI || HYPERV_VTL_MODE)
        select PARAVIRT
        select X86_HV_CALLBACK_VECTOR if X86
        select OF_EARLY_FLATTREE if OF
@@ -15,7 +16,7 @@ config HYPERV

 config HYPERV_VTL_MODE
        bool "Enable Linux to boot in VTL context"
-       depends on X86_64 && HYPERV
+       depends on X86_64
        depends on SMP
        default n
        help

HYPERV_VTL_MODE can now be selected independently of HYPERV.
The existing code should be such that even if someone is building a
random config and gets HYPERV_VTL_MODE without HYPERV, the
kernel will build and run in a non-Hyper-V environment and isn't
broken somehow.

For HYPERV to be selected, either ACPI must already be selected, or
HYPERV_VTL_MODE must already be selected. So "normal" kernels are
still enforced to require ACPI. But if building with HYPERV_VTL_MODE,
then ACPI is optional.

Saurabh's idea of adding "depends on OF" to HYPERV_VTL_MODE
should also work with these changes.

I haven't fully tested the above with all the relevant combinations
on both x86 and arm64, but I think the logic makes sense.

Michael

RE: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:20 AM
> 
> On 8/4/2024 8:02 PM, Michael Kelley wrote:
> > From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59
> 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. Fix the hypercall output address in `get_vtl(void)`
> >> not to overlap with the hypercall input area to adhere to
> >> the Hyper-V TLFS.
> >>
> >> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> >> ---
> >>   arch/x86/hyperv/hv_init.c          | 34 ---------------------
> >>   arch/x86/include/asm/hyperv-tlfs.h |  7 -----
> >>   drivers/hv/hv_common.c             | 47 ++++++++++++++++++++++++++++--
> >>   include/asm-generic/hyperv-tlfs.h  |  7 +++++
> >>   include/asm-generic/mshyperv.h     |  6 ++++
> >>   5 files changed, 58 insertions(+), 43 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 9c452bfbd571..7d6c1523b0b5 100644
> >> --- a/drivers/hv/hv_common.c
> >> +++ b/drivers/hv/hv_common.c
> >> @@ -339,8 +339,8 @@ int __init hv_common_init(void)
> >>   	hyperv_pcpu_input_arg = alloc_percpu(void  *);
> >>   	BUG_ON(!hyperv_pcpu_input_arg);
> >>
> >> -	/* Allocate the per-CPU state for output arg for root */
> >> -	if (hv_root_partition) {
> >> +	/* Allocate the per-CPU state for output arg for root or a VTL */
> >> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
> >>   		hyperv_pcpu_output_arg = alloc_percpu(void *);
> >>   		BUG_ON(!hyperv_pcpu_output_arg);
> >>   	}
> >> @@ -656,3 +656,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 = *this_cpu_ptr(hyperv_pcpu_output_arg);
> >
> > Rather than use the hyperv_pcpu_output_arg here, it's OK to
> > use a different area of the hyperv_pcpu_input_arg page.  For
> > example,
> >
> > 	output = (void *)input + HV_HYP_PAGE_SIZE/2;
> >
> > The TLFS does not require that the input and output be in
> > separate pages.
> >
> > While using the hyperv_pcpu_output_arg is conceptually a
> > bit cleaner, doing so requires allocating a 4K page per CPU that
> > is not otherwise used. The VTL 2 code wants to be frugal with
> > memory, and this seems like a good step in that direction. :-)
> >
> I agree on the both counts: the code looks conceptually cleaner now and
> VTL2 wants to be frugal with memory, esp that the output hypercall page
> is per-CPU so we have O(n) as the CPU count increases. Still, the output
> page will be needed for VTL2 (say to get/set registers just as done
> here). That said, with this patch we can achieve both the conceptual
> cleanliness and being ready to grow more on the primitives being built
> out in the VTL support patches.
> 

Could you elaborate further on why the output page is needed for
VTL2? The get/set register hypercalls can operate with just the input
page (again, splitting it into two halves for input and output args) as
long as the number of registers acted on by a single hypercall isn't
more than a few dozen.

If you really *do* need the output page in VTL2 for other reasons
that I'm not aware of, then my suggestion isn't relevant and there's
no memory to be saved.

Michael

RE: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:51 AM

[snip]

> >> diff --git a/arch/arm64/include/asm/mshyperv.h
> >> b/arch/arm64/include/asm/mshyperv.h
> >> index a975e1a689dd..a7a3586f7cb1 100644
> >> --- a/arch/arm64/include/asm/mshyperv.h
> >> +++ b/arch/arm64/include/asm/mshyperv.h
> >> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
> >>
> >>   #include <asm-generic/mshyperv.h>
> >>
> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
> >> +
> >
> > Section 6.2 of the SMCCC specification says that the "Call UID Query"
> > returns a UUID. The above #defines look like an ASCII string is being
> > returned. Arguably the ASCII string can be treated as a set of 128 bits
> > just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V
> > be changed to return a real UUID? While the distinction probably
> > won't make a material difference here, we've had problems in the past
> > with Hyper-V doing slightly weird things that later caused unexpected
> > trouble. Please just get it right. :-)
> >
> The above values don't violate anything in the spec, and I think it
> would be hard to give an example of what can be broken in the world by
> using the above values.

Agreed.  However, note that UUIDs *do* have some internal structure.
See https://www.uuidtools.com/decode, for example. The UUID above
is:

4d734879-7065-7256-0000-000000000000

It has a version digit of "7", which is "unknown".  I'm no expert on UUIDs,
but apparently the "7" isn't necessarily invalid, though perhaps a bit unexpected.

> I do understand what you're saying when you
> mention the UIDs & the spirit of the spec. Put on the quantitative
> footing, the Shannon entropy of these values is much lower than that of
> an UID. A cursory search in the kernel tree does turn up other UIDs that
> don't look too random.
> 
> As that is implemented only in the non-release versions, hardly someone
> has taken a dependency on the specific values in their production code.
> I guess that can be changed without causing any disturbance to the
> customers, will report of any concerns though.

My last thought is that when dealing with the open source world, and
with published standards, it's usually best to do what's normal and
expected, rather than trying to make the case that something unexpected
is allowed by the spec. Doing the latter can come back to bite you in
completely unexpected ways. :-)

With that, I won't make any further comments on the topic. You do
whatever you can do in working with Hyper-V. Either way, it won't be
a blocker to my giving "Reviewed-by" on the next version of the
patch.

Michael

Re: [PATCH v3 1/7] arm64: hyperv: Use SMC to detect hypervisor presence

[Roman Kisel]

On 8/5/2024 1:30 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:51 AM
> 
> [snip]
> 
>>>> diff --git a/arch/arm64/include/asm/mshyperv.h
>>>> b/arch/arm64/include/asm/mshyperv.h
>>>> index a975e1a689dd..a7a3586f7cb1 100644
>>>> --- a/arch/arm64/include/asm/mshyperv.h
>>>> +++ b/arch/arm64/include/asm/mshyperv.h
>>>> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)
>>>>
>>>>    #include <asm-generic/mshyperv.h>
>>>>
>>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
>>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
>>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
>>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
>>>> +
>>>
>>> Section 6.2 of the SMCCC specification says that the "Call UID Query"
>>> returns a UUID. The above #defines look like an ASCII string is being
>>> returned. Arguably the ASCII string can be treated as a set of 128 bits
>>> just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V
>>> be changed to return a real UUID? While the distinction probably
>>> won't make a material difference here, we've had problems in the past
>>> with Hyper-V doing slightly weird things that later caused unexpected
>>> trouble. Please just get it right. :-)
>>>
>> The above values don't violate anything in the spec, and I think it
>> would be hard to give an example of what can be broken in the world by
>> using the above values.
> 
> Agreed.  However, note that UUIDs *do* have some internal structure.
> See https://www.uuidtools.com/decode, for example. The UUID above
> is:
> 
> 4d734879-7065-7256-0000-000000000000
> 
> It has a version digit of "7", which is "unknown".  I'm no expert on UUIDs,
> but apparently the "7" isn't necessarily invalid, though perhaps a bit unexpected.
> 
Understood! I made an assumption that's just an array of random bytes.
Thank you very much for explaining that to me :)

>> I do understand what you're saying when you
>> mention the UIDs & the spirit of the spec. Put on the quantitative
>> footing, the Shannon entropy of these values is much lower than that of
>> an UID. A cursory search in the kernel tree does turn up other UIDs that
>> don't look too random.
>>
>> As that is implemented only in the non-release versions, hardly someone
>> has taken a dependency on the specific values in their production code.
>> I guess that can be changed without causing any disturbance to the
>> customers, will report of any concerns though.
> 
> My last thought is that when dealing with the open source world, and
> with published standards, it's usually best to do what's normal and
> expected, rather than trying to make the case that something unexpected
> is allowed by the spec. Doing the latter can come back to bite you in
> completely unexpected ways. :-)
> 
Agreed, thanks for the discussion! I've discussed changing the values,
and a pull request to the hypervisor has been put up for that. So far, 
going well.

> With that, I won't make any further comments on the topic. You do
> whatever you can do in working with Hyper-V. Either way, it won't be
> a blocker to my giving "Reviewed-by" on the next version of the
> patch.
> 
Will be a badge of honor to me! Again, appreciate your words of advise 
and caution, and helping in bringing these patches into the best shape 
possible!

> Michael

-- 
Thank you,
Roman

Re: [PATCH v3 3/7] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Roman Kisel]

On 8/5/2024 1:13 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:20 AM
>>
>> On 8/4/2024 8:02 PM, Michael Kelley wrote:
>>> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59
>> 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. Fix the hypercall output address in `get_vtl(void)`
>>>> not to overlap with the hypercall input area to adhere to
>>>> the Hyper-V TLFS.
>>>>
>>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>>>> ---
>>>>    arch/x86/hyperv/hv_init.c          | 34 ---------------------
>>>>    arch/x86/include/asm/hyperv-tlfs.h |  7 -----
>>>>    drivers/hv/hv_common.c             | 47 ++++++++++++++++++++++++++++--
>>>>    include/asm-generic/hyperv-tlfs.h  |  7 +++++
>>>>    include/asm-generic/mshyperv.h     |  6 ++++
>>>>    5 files changed, 58 insertions(+), 43 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 9c452bfbd571..7d6c1523b0b5 100644
>>>> --- a/drivers/hv/hv_common.c
>>>> +++ b/drivers/hv/hv_common.c
>>>> @@ -339,8 +339,8 @@ int __init hv_common_init(void)
>>>>    	hyperv_pcpu_input_arg = alloc_percpu(void  *);
>>>>    	BUG_ON(!hyperv_pcpu_input_arg);
>>>>
>>>> -	/* Allocate the per-CPU state for output arg for root */
>>>> -	if (hv_root_partition) {
>>>> +	/* Allocate the per-CPU state for output arg for root or a VTL */
>>>> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>>>>    		hyperv_pcpu_output_arg = alloc_percpu(void *);
>>>>    		BUG_ON(!hyperv_pcpu_output_arg);
>>>>    	}
>>>> @@ -656,3 +656,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 = *this_cpu_ptr(hyperv_pcpu_output_arg);
>>>
>>> Rather than use the hyperv_pcpu_output_arg here, it's OK to
>>> use a different area of the hyperv_pcpu_input_arg page.  For
>>> example,
>>>
>>> 	output = (void *)input + HV_HYP_PAGE_SIZE/2;
>>>
>>> The TLFS does not require that the input and output be in
>>> separate pages.
>>>
>>> While using the hyperv_pcpu_output_arg is conceptually a
>>> bit cleaner, doing so requires allocating a 4K page per CPU that
>>> is not otherwise used. The VTL 2 code wants to be frugal with
>>> memory, and this seems like a good step in that direction. :-)
>>>
>> I agree on the both counts: the code looks conceptually cleaner now and
>> VTL2 wants to be frugal with memory, esp that the output hypercall page
>> is per-CPU so we have O(n) as the CPU count increases. Still, the output
>> page will be needed for VTL2 (say to get/set registers just as done
>> here). That said, with this patch we can achieve both the conceptual
>> cleanliness and being ready to grow more on the primitives being built
>> out in the VTL support patches.
>>
> 
> Could you elaborate further on why the output page is needed for
> VTL2? The get/set register hypercalls can operate with just the input
> page (again, splitting it into two halves for input and output args) as
> long as the number of registers acted on by a single hypercall isn't
> more than a few dozen.
> 
> If you really *do* need the output page in VTL2 for other reasons
> that I'm not aware of, then my suggestion isn't relevant and there's
> no memory to be saved.
VTL2 might potentially use any hypercalls being in some sense an exclave 
of the hypervisor living inside the guest quite similarly to the 
VBS/VTL1/SecureKernel.

The tradeoff here would be to save a page per processor at the cost of 
specializing the hypercall issuing code that would use a part of the 
input page to save memory (quite likely limiting which hypercalls can be 
used), or use the common implementation at the cost of spending one more 
page per processor. Less code means less maintenance usually so seems 
beneficial to choose the latter option although at the cost of using 
more memory.

> 
> Michael

-- 
Thank you,
Roman

RE: [PATCH v3 2/7] Drivers: hv: Enable VTL mode for arm64

[Roman Kisel]

> > 
> > On 8/4/2024 9:05 PM, Saurabh Singh Sengar wrote:
> > > On Mon, Aug 05, 2024 at 03:01:58AM +0000, Michael Kelley wrote:
> > >> From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59
> > 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 <romank@linux.microsoft.com>
> > >>> ---
> > >>>   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.34.1
> > >>>
> > >>
> > >> In v2 of this patch, I suggested [1] making a couple additional minor changes
> > >> so that kernels built *without* HYPER_VTL_MODE would still require
> > >> ACPI.  Did that suggestion not work out?  If that's the case, I'm curious
> > >> about what goes wrong.
> > >
> > > Hi Michael/Roman,
> > > I was considering making HYPERV_VTL_MODE depend on CONFIG_OF. That should
> > address
> > > above concern as well. Do you see any potential issue with it.
> > >
> > Michael,
> > 
> > I ran into a pretty gnarly recursive dependencies which in all fairness
> > might stem from not being fluent enough in the Kconfig language. Any
> > help of how to approach implementing your idea would be greatly appreciated!
> > 
> 
> This is what I had in mind:
> 
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -5,7 +5,8 @@ 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)
> +       depends on (ACPI || HYPERV_VTL_MODE)
        > select PARAVIRT
        > select X86_HV_CALLBACK_VECTOR if X86
        > select OF_EARLY_FLATTREE if OF
> @@ -15,7 +16,7 @@ config HYPERV
> 
 > config HYPERV_VTL_MODE
        > bool "Enable Linux to boot in VTL context"
> -       depends on X86_64 && HYPERV
> +       depends on X86_64
        > depends on SMP
        > default n
        > help
> 
> HYPERV_VTL_MODE can now be selected independently of HYPERV.
> The existing code should be such that even if someone is building a
> random config and gets HYPERV_VTL_MODE without HYPERV, the
> kernel will build and run in a non-Hyper-V environment and isn't
> broken somehow.
> 
> For HYPERV to be selected, either ACPI must already be selected, or
> HYPERV_VTL_MODE must already be selected. So "normal" kernels are
> still enforced to require ACPI. But if building with HYPERV_VTL_MODE,
> then ACPI is optional.
> 
Thanks a ton! Let me try this out for the (arch; VTL) build matrix :)

> Saurabh's idea of adding "depends on OF" to HYPERV_VTL_MODE
> should also work with these changes.
> 
> I haven't fully tested the above with all the relevant combinations
> on both x86 and arm64, but I think the logic makes sense.