[PATCH hyperv-next v4 0/6] arm64: hyperv: Support Virtual Trust Level Boot

[PATCH hyperv-next v4 0/6] arm64: hyperv: Support Virtual Trust Level Boot

[Roman Kisel]

This patch set allows 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.

The OpenHCL paravisor https://github.com/microsoft/openvmm/tree/main/openhcl
can serve as a practical application of these patches on ARM64.

For validation, I built kernels for the {x86_64, ARM64} x {VTL0, VTL2} set with
a small initrd embedded into the kernel and booted VMs managed by Hyper-V and
OpenVMM off of that.

[V4]
    - Fixed wording to match acronyms defined in the "Terms and Abbreviations"
      section of the SMCCC specification throughout the patch series.
      **Thank you, Michael!**

    - Replaced the hypervisor ID containing ASCII with an UUID as
      required by the specification.
      **Thank you, Michael!**

    - Added an explicit check for `SMCCC_RET_NOT_SUPPORTED` when discovering the
      hypervisor presence to make the backward compatibility obvious.
      **Thank you, Saurabh!**

    - Split the fix for `get_vtl(void)` out to make it easier to backport.
    - Refactored the configuration options as requested to eliminate the risk
      of building non-functional kernels with randomly selected options.
      **Thank you, Michael!**

    - Refactored the changes not to introduce an additional file with
      a one-line function.
      **Thank you, Wei!**

    - Fixed change description for the VMBus DeviceTree changes, used
      `scripts/get_maintainers.pl` on the latest kernel to get the up-to-date list
      of maintainers as requested.
      **Thank you, Krzysztof!**

    - Removed the added (paranoidal+superfluous) checks for DMA coherence in the
      VMBus driver and instead relied on the DMA and the OF subsystem code.
      **Thank you, Arnd, Krzysztof, Michael!**

    - Used another set of APIs for discovering the hardware interrupt number
      in the VMBus driver to be able to build the driver as a module.
      **Thank you, Michael, Saurabh!**

    - Renamed the newly introduced `get_vmbus_root_device(void)` function to
      `hv_get_vmbus_root_device(void)` as requested.
      **Thank you, Wei!**

    - Applied the suggested small-scale refactoring to simplify changes to the Hyper-V
      PCI driver. Taking the offered liberty of doing the large scale refactoring
      in another patch series.
      **Thank you, Michael!**

    - Added a fix for the issue discovered internally where the CPU would not
      get the interrupt from a PCI device attached to VTL2 as the shared peripheral
      interrupt number (SPI) was not offset by 32 (the first valid SPI number).
      **Thank you, Brian!**

[V3]
    https://lore.kernel.org/lkml/20240726225910.1912537-1-romank@linux.microsoft.com/
    - Employed the SMCCC function 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/

Roman Kisel (6):
  arm64: hyperv: Use SMCCC to detect hypervisor presence
  Drivers: hv: Enable VTL mode for arm64
  Drivers: hv: Provide arch-neutral implementation of get_vtl()
  dt-bindings: microsoft,vmbus: Add GIC and DMA coherence to the example
  Drivers: hv: vmbus: Get the IRQ number from DeviceTree
  PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

 .../bindings/bus/microsoft,vmbus.yaml         | 11 +++
 arch/arm64/hyperv/mshyperv.c                  | 43 ++++++++++--
 arch/arm64/include/asm/mshyperv.h             |  5 ++
 arch/x86/hyperv/hv_init.c                     | 34 ---------
 drivers/hv/Kconfig                            | 10 +--
 drivers/hv/hv_common.c                        | 32 +++++++++
 drivers/hv/vmbus_drv.c                        | 59 +++++++++++++---
 drivers/pci/controller/pci-hyperv.c           | 69 +++++++++++++++++--
 include/asm-generic/mshyperv.h                |  6 ++
 include/hyperv/hvgdk_mini.h                   |  2 +-
 include/linux/hyperv.h                        |  2 +
 11 files changed, 215 insertions(+), 58 deletions(-)


base-commit: 2e03358be78b65d28b66e17aca9e0c8700b0df78
-- 
2.43.0

[PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC 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. Then
use the vendor-specific hypervisor service call (implemented
recently in Hyper-V) via SMCCC in the non-ACPI case.

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

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index fc49949b7df6..fe6185bf3bf2 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -27,6 +27,36 @@ 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_smccc(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);
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+		return false;
+
+	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 +65,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_smccc())
 		return 0;
 
 	/* Setup the guest ID */
@@ -72,6 +100,11 @@ static int __init hyperv_init(void)
 		return ret;
 	}
 
+	ms_hyperv.vtl = get_vtl();
+	/* Report if non-default VTL */
+	if (ms_hyperv.vtl > 0)
+		pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
+
 	ms_hyperv_late_init();
 
 	hyperv_initialized = true;
diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h
index 2e2f83bafcfb..a6d7eb9e167b 100644
--- a/arch/arm64/include/asm/mshyperv.h
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -50,4 +50,9 @@ static inline u64 hv_get_msr(unsigned int reg)
 
 #include <asm-generic/mshyperv.h>
 
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x4d32ba58U
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0xcd244764U
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0x8eef6c75U
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0x16597024U
+
 #endif
-- 
2.43.0

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Arnd Bergmann]

On Wed, Feb 12, 2025, at 02:43, Roman Kisel wrote:
> +static bool hyperv_detect_via_smccc(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);
> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
> +		return false;
> +
> +	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;
> +}

I had to double-check that this function is safe to call on
other hypervisors, at least when they follow the smccc spec.

Seeing that we have the same helper function checking for
ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_* and there was another
patch set adding a copy for gunyah, I wonder if we can
put this into a drivers/firmware/smccc/smccc.c directly
the same way we handle soc_id, and make it return a uuid_t,
or perhaps take a constant uuid_t to compare against.

      Arnd

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Roman Kisel]

On 2/11/2025 10:54 PM, Arnd Bergmann wrote:
> On Wed, Feb 12, 2025, at 02:43, Roman Kisel wrote:
>> +static bool hyperv_detect_via_smccc(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);
>> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
>> +		return false;
>> +
>> +	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;
>> +}
> 
> I had to double-check that this function is safe to call on
> other hypervisors, at least when they follow the smccc spec.
> 
> Seeing that we have the same helper function checking for
> ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_* and there was another
> patch set adding a copy for gunyah, I wonder if we can
> put this into a drivers/firmware/smccc/smccc.c directly
> the same way we handle soc_id, and make it return a uuid_t,
> or perhaps take a constant uuid_t to compare against.

That would be very neat! I implemented the idea [1], please let me know
what you think. I can use that in the next version of the patch series
if looks good.

There is a function and a macro to make calling
the function easier. As the SMCCC header is used by the assembler, too,
hence I had to add __ASSEBLER__ guardrails. Another option could be to
pass four u32's not to use uuid_t so the header stays a healthy food
for the assembler :) For Gunyah, that would be

ARM_SMCCC_HYP_PRESENT(GUNYAH)

when using the change below.


 From f0d645e900c24f5be045b0f831f1e11494967b7f Mon Sep 17 00:00:00 2001
From: Roman Kisel <romank@linux.microsoft.com>
Date: Thu, 13 Feb 2025 15:10:45 -0800
Subject: [PATCH] drivers, smcc: Introduce arm_smccc_hyp_present

---
  arch/arm64/hyperv/mshyperv.c       | 18 +----------------
  drivers/firmware/smccc/kvm_guest.c |  9 +--------
  drivers/firmware/smccc/smccc.c     | 24 ++++++++++++++++++++++
  include/linux/arm-smccc.h          | 32 ++++++++++++++++++++++++++++++
  4 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index 16e721d8e5df..0c5babe9e1ff 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -41,22 +41,6 @@ static bool hyperv_detect_via_acpi(void)
  #endif
  }

-static bool hyperv_detect_via_smccc(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);
-	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
-		return false;
-
-	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;
@@ -69,7 +53,7 @@ static int __init hyperv_init(void)
  	 *
  	 * In such cases, do nothing and return success.
  	 */
-	if (!hyperv_detect_via_acpi() && !hyperv_detect_via_smccc())
+	if (!hyperv_detect_via_acpi() && !ARM_SMCCC_HYP_PRESENT(HYPERV))
  		return 0;

  	/* Setup the guest ID */
diff --git a/drivers/firmware/smccc/kvm_guest.c 
b/drivers/firmware/smccc/kvm_guest.c
index f3319be20b36..ae37476cabc1 100644
--- a/drivers/firmware/smccc/kvm_guest.c
+++ b/drivers/firmware/smccc/kvm_guest.c
@@ -17,14 +17,7 @@ void __init kvm_init_hyp_services(void)
  	struct arm_smccc_res res;
  	u32 val[4];

-	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
-		return;
-
-	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
-	if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
-	    res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
-	    res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
-	    res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
+	if (!ARM_SMCCC_HYP_PRESENT(KVM))
  		return;

  	memset(&res, 0, sizeof(res));
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index a74600d9f2d7..86f75f44895f 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -67,6 +67,30 @@ s32 arm_smccc_get_soc_id_revision(void)
  }
  EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);

+bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
+{
+	struct arm_smccc_res res = {};
+	struct {
+		u32 dwords[4]
+	} __packed res_uuid;
+
+	BUILD_BUG_ON(sizeof(res_uuid) != sizeof(uuid_t));
+
+	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);
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+		return false;
+
+	res_uuid.dwords[0] = res.a0;
+	res_uuid.dwords[1] = res.a1;
+	res_uuid.dwords[2] = res.a2;
+	res_uuid.dwords[3] = res.a3;
+
+	return uuid_equal((uuid_t *)&res_uuid, hyp_uuid);
+}
+EXPORT_SYMBOL_GPL(arm_smccc_hyp_present);
+
  static int __init smccc_devices_init(void)
  {
  	struct platform_device *pdev;
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 67f6fdf2e7cd..63925506a0e5 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -7,6 +7,11 @@

  #include <linux/args.h>
  #include <linux/init.h>
+
+#ifndef __ASSEMBLER__
+#include <linux/uuid.h>
+#endif
+
  #include <uapi/linux/const.h>

  /*
@@ -333,6 +338,33 @@ s32 arm_smccc_get_soc_id_version(void);
   */
  s32 arm_smccc_get_soc_id_revision(void);

+#ifndef __ASSEMBLER__
+
+/**
+ * arm_smccc_hyp_present(const uuid_t *hyp_uuid)
+ *
+ * Returns `true` if the hypervisor advertises its presence via SMCCC.
+ *
+ * When the function returns `false`, the caller shall not assume that
+ * there is no hypervisor running. Instead, the caller must fall back to
+ * other approaches if any are available.
+ */
+bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);
+
+#define ARM_SMCCC_HYP_PRESENT(HYP) 								\
+	({															\
+		const u32 uuid_as_dwords[4] = {							\
+			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_0,			\
+			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_1,			\
+			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_2,			\
+			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_3			\
+		};														\
+																\
+		arm_smccc_hyp_present((const uuid_t *)uuid_as_dwords);	\
+	})															\
+
+#endif /* !__ASSEMBLER__ */
+
  /**
   * struct arm_smccc_res - Result from SMC/HVC call
   * @a0-a3 result values from registers 0 to 3
-- 
2.43.0



> 
>        Arnd

-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Arnd Bergmann]

On Fri, Feb 14, 2025, at 00:23, Roman Kisel wrote:
> On 2/11/2025 10:54 PM, Arnd Bergmann wrote:

> index a74600d9f2d7..86f75f44895f 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -67,6 +67,30 @@ s32 arm_smccc_get_soc_id_revision(void)
>   }
>   EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
>
> +bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)

The interface looks good to me.

> +{
> +	struct arm_smccc_res res = {};
> +	struct {
> +		u32 dwords[4]
> +	} __packed res_uuid;

The structure definition here looks odd because of the
unexplained __packed attribute and the nonstandard byteorder.

The normal uuid_t is defined as an array of 16 bytes,
so if you try to represent it in 32-bit words you need to
decide between __le32 and __be32 representation.

> +	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);
> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
> +		return false;
> +
> +	res_uuid.dwords[0] = res.a0;
> +	res_uuid.dwords[1] = res.a1;
> +	res_uuid.dwords[2] = res.a2;
> +	res_uuid.dwords[3] = res.a3;
> +
> +	return uuid_equal((uuid_t *)&res_uuid, hyp_uuid);

The SMCCC standard defines the four words to be little-endian,
so in order to compare them against a uuid byte array, you'd
need to declare the array as __le32 and swap the result
members with cpu_to_le32().

Alternatively you could pass the four u32 values into the
function in place of the uuid.

Since the callers have the same endianess confusion, your
implementation ends up working correctly even on big-endian,
but I find it harder to follow when you call uuid_equal() on
something that is not the actual uuid_t value.

> +
> +#define ARM_SMCCC_HYP_PRESENT(HYP) 								\
> +	({															\
> +		const u32 uuid_as_dwords[4] = {							\
> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_0,			\
> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_1,			\

I don't think using a macro is helpful here, it just makes
it impossible to grep for ARM_SMCCC_VENDOR_HYP_UID_* values when
reading the source.

I would suggest moving the UUID values into a variable next
to the caller like

#define ARM_SMCCC_VENDOR_HYP_UID_KVM \
    UUID_INIT(0x28b46fb6, 0x2ec5, 0x11e9, 0xa9, 0xca, 0x4b, 0x56, 0x4d, 0x00, 0x3a, 0x74)

and then just pass that into arm_smccc_hyp_present(). (please
double-check the endianess of the definition here, I probably
got it wrong myself).

     Arnd

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Roman Kisel]

On 2/14/2025 12:05 AM, Arnd Bergmann wrote:
> On Fri, Feb 14, 2025, at 00:23, Roman Kisel wrote:
>> On 2/11/2025 10:54 PM, Arnd Bergmann wrote:
> 
>> index a74600d9f2d7..86f75f44895f 100644
>> --- a/drivers/firmware/smccc/smccc.c
>> +++ b/drivers/firmware/smccc/smccc.c
>> @@ -67,6 +67,30 @@ s32 arm_smccc_get_soc_id_revision(void)
>>    }
>>    EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
>>
>> +bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
> 
> The interface looks good to me.

Great :)

> 
>> +{
>> +	struct arm_smccc_res res = {};
>> +	struct {
>> +		u32 dwords[4]
>> +	} __packed res_uuid;
> 
> The structure definition here looks odd because of the
> unexplained __packed attribute and the nonstandard byteorder.
> 

Fair points, thank you, will straighten this out!

> The normal uuid_t is defined as an array of 16 bytes,
> so if you try to represent it in 32-bit words you need to
> decide between __le32 and __be32 representation.
> 
>> +	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);
>> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
>> +		return false;
>> +
>> +	res_uuid.dwords[0] = res.a0;
>> +	res_uuid.dwords[1] = res.a1;
>> +	res_uuid.dwords[2] = res.a2;
>> +	res_uuid.dwords[3] = res.a3;
>> +
>> +	return uuid_equal((uuid_t *)&res_uuid, hyp_uuid);
> 
> The SMCCC standard defines the four words to be little-endian,
> so in order to compare them against a uuid byte array, you'd
> need to declare the array as __le32 and swap the result
> members with cpu_to_le32().
> 
> Alternatively you could pass the four u32 values into the
> function in place of the uuid.
> 
> Since the callers have the same endianess confusion, your
> implementation ends up working correctly even on big-endian,
> but I find it harder to follow when you call uuid_equal() on
> something that is not the actual uuid_t value.
> 

I'll make sure the implementation is clearer, thanks!

>> +
>> +#define ARM_SMCCC_HYP_PRESENT(HYP) 								\
>> +	({															\
>> +		const u32 uuid_as_dwords[4] = {							\
>> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_0,			\
>> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_1,			\
> 
> I don't think using a macro is helpful here, it just makes
> it impossible to grep for ARM_SMCCC_VENDOR_HYP_UID_* values when
> reading the source.
> 
> I would suggest moving the UUID values into a variable next
> to the caller like
> 
> #define ARM_SMCCC_VENDOR_HYP_UID_KVM \
>      UUID_INIT(0x28b46fb6, 0x2ec5, 0x11e9, 0xa9, 0xca, 0x4b, 0x56, 0x4d, 0x00, 0x3a, 0x74)
> 
> and then just pass that into arm_smccc_hyp_present(). (please
> double-check the endianess of the definition here, I probably
> got it wrong myself).

Will remove the macro and will use UUID_INIT, appreciate taking the
time to review the draft and your suggestions on improving it very much!

> 
>       Arnd

-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Roman Kisel]

Hi Arnd,

[...]

>> I would suggest moving the UUID values into a variable next
>> to the caller like
>>
>> #define ARM_SMCCC_VENDOR_HYP_UID_KVM \
>>      UUID_INIT(0x28b46fb6, 0x2ec5, 0x11e9, 0xa9, 0xca, 0x4b, 0x56, 
>> 0x4d, 0x00, 0x3a, 0x74)
>>
>> and then just pass that into arm_smccc_hyp_present(). (please
>> double-check the endianess of the definition here, I probably
>> got it wrong myself).

I worked out a variation [1] of the change that you said looked good.

Here, there is a helper macro for creating uuid_t's when checking
for the hypervisor running via SMCCC to avoid using the bare UUID_INIT. 
Valiadted with KVM/arm64 and Hyper-V/arm64. Do you think this is a
better approach than converting by hand?

If that looks too heavy, maybe could leave out converting the expected
register values to UUID, and pass the expected register values to
arm_smccc_hyp_present directly. That way, instead of

bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);

we'd have

bool arm_smccc_hyp_present(u32 reg0, u32 reg1, u32 reg2, u32 reg2);


Please let me know what you think!

[1]
---
  arch/arm64/hyperv/mshyperv.c       | 16 +++++---------
  drivers/firmware/smccc/kvm_guest.c | 13 +++++------
  drivers/firmware/smccc/smccc.c     | 19 ++++++++++++++++
  include/linux/arm-smccc.h          | 35 ++++++++++++++++++++++++++++++
  4 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index 16e721d8e5df..b8468bd65ec0 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -43,18 +43,12 @@ static bool hyperv_detect_via_acpi(void)

  static bool hyperv_detect_via_smccc(void)
  {
-	struct arm_smccc_res res = {};
+	uuid_t hyperv_uuid = HYP_UUID_INIT(ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0,
+		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1,
+		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2,
+		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3);

-	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);
-	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
-		return false;
-
-	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;
+	return arm_smccc_hyp_present(&hyperv_uuid);
  }

  static int __init hyperv_init(void)
diff --git a/drivers/firmware/smccc/kvm_guest.c 
b/drivers/firmware/smccc/kvm_guest.c
index f3319be20b36..48c3622b2aa6 100644
--- a/drivers/firmware/smccc/kvm_guest.c
+++ b/drivers/firmware/smccc/kvm_guest.c
@@ -14,17 +14,14 @@ static DECLARE_BITMAP(__kvm_arm_hyp_services, 
ARM_SMCCC_KVM_NUM_FUNCS) __ro_afte

  void __init kvm_init_hyp_services(void)
  {
+	uuid_t kvm_uuid = HYP_UUID_INIT(ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0,
+		ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1,
+		ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2,
+		ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3);
  	struct arm_smccc_res res;
  	u32 val[4];

-	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
-		return;
-
-	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
-	if (res.a0 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0 ||
-	    res.a1 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1 ||
-	    res.a2 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 ||
-	    res.a3 != ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3)
+	if (!arm_smccc_hyp_present(&kvm_uuid))
  		return;

  	memset(&res, 0, sizeof(res));
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index a74600d9f2d7..0943abb3f4c9 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -67,6 +67,25 @@ s32 arm_smccc_get_soc_id_revision(void)
  }
  EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);

+bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
+{
+	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);
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+		return false;
+
+	return ({
+		const uuid_t uuid = HYP_UUID_INIT(res.a0, res.a1, res.a2, res.a3);
+		const bool present = uuid_equal(&uuid, hyp_uuid);
+
+		present;
+	});
+}
+EXPORT_SYMBOL_GPL(arm_smccc_hyp_present);
+
  static int __init smccc_devices_init(void)
  {
  	struct platform_device *pdev;
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 67f6fdf2e7cd..60be36254364 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -7,6 +7,11 @@

  #include <linux/args.h>
  #include <linux/init.h>
+
+#ifndef __ASSEMBLER__
+#include <linux/uuid.h>
+#endif
+
  #include <uapi/linux/const.h>

  /*
@@ -333,6 +338,36 @@ s32 arm_smccc_get_soc_id_version(void);
   */
  s32 arm_smccc_get_soc_id_revision(void);

+#ifndef __ASSEMBLER__
+
+/**
+ * arm_smccc_hyp_present(const uuid_t *hyp_uuid)
+ *
+ * Returns `true` if the hypervisor advertises its presence via SMCCC.
+ *
+ * When the function returns `false`, the caller shall not assume that
+ * there is no hypervisor running. Instead, the caller must fall back to
+ * other approaches if any are available.
+ */
+bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);
+
+#define HYP_UUID_INIT(r0, r1, r2, r3) \
+	UUID_INIT( \
+		cpu_to_le32(lower_32_bits(r0)), \
+		cpu_to_le32(lower_32_bits(r1)) & 0xffff, \
+		cpu_to_le32(lower_32_bits(r1)) >> 16, \
+		cpu_to_le32(lower_32_bits(r2)) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r2)) >> 8) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r2)) >> 16) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r2)) >> 24) & 0xff, \
+		cpu_to_le32(lower_32_bits(r3)) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r3)) >> 8) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r3)) >> 16) & 0xff, \
+		(cpu_to_le32(lower_32_bits(r3)) >> 24) & 0xff \
+	)
+
+#endif /* !__ASSEMBLER__ */
+
  /**
   * struct arm_smccc_res - Result from SMC/HVC call
   * @a0-a3 result values from registers 0 to 3


-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Arnd Bergmann]

On Tue, Feb 25, 2025, at 00:22, Roman Kisel wrote:
> Hi Arnd,
>
> [...]
>
>>> I would suggest moving the UUID values into a variable next
>>> to the caller like
>>>
>>> #define ARM_SMCCC_VENDOR_HYP_UID_KVM \
>>>      UUID_INIT(0x28b46fb6, 0x2ec5, 0x11e9, 0xa9, 0xca, 0x4b, 0x56, 
>>> 0x4d, 0x00, 0x3a, 0x74)
>>>
>>> and then just pass that into arm_smccc_hyp_present(). (please
>>> double-check the endianess of the definition here, I probably
>>> got it wrong myself).
>
> I worked out a variation [1] of the change that you said looked good.
>
> Here, there is a helper macro for creating uuid_t's when checking
> for the hypervisor running via SMCCC to avoid using the bare UUID_INIT. 
> Valiadted with KVM/arm64 and Hyper-V/arm64. Do you think this is a
> better approach than converting by hand?
>
> If that looks too heavy, maybe could leave out converting the expected
> register values to UUID, and pass the expected register values to
> arm_smccc_hyp_present directly. That way, instead of
>
> bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);
>
> we'd have
>
> bool arm_smccc_hyp_present(u32 reg0, u32 reg1, u32 reg2, u32 reg2);
>
>
> Please let me know what you think!

The patch looks correct to me, but I agree it's a little silly
to convert register values into uuid format on both sides.

>   static bool hyperv_detect_via_smccc(void)
>   {
> -	struct arm_smccc_res res = {};
> +	uuid_t hyperv_uuid = HYP_UUID_INIT(ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0,
> +		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1,
> +		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2,
> +		ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3);

If you want to declare a uuid here, I think you should remove the
ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_{0,1,2,3} macros and just
have UUID in normal UUID_INIT() notation as we do for
other UUIDs.

If you want to keep the four 32-bit values and pass them into
arm_smccc_hyp_present() directly, I think that is also fine,
but in that case, I would try to avoid calling it a UUID.

How are the kvm and hyperv values specified originally?
From the SMCCC document it seems like they are meant to be
UUIDs, so I would expect them to be in canonical form rather
than the smccc return values, but I could not find a document
for them.

     Arnd

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Roman Kisel]

On 2/24/2025 11:24 PM, Arnd Bergmann wrote:
> On Tue, Feb 25, 2025, at 00:22, Roman Kisel wrote:
>> Hi Arnd,

[...]

> If you want to declare a uuid here, I think you should remove the
> ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_{0,1,2,3} macros and just
> have UUID in normal UUID_INIT() notation as we do for
> other UUIDs.

I'd gladly stick to that provided I have your support of touching
KVM's code! As the SMCCC document states, there shall be an UUID,
and in the kernel, there would be

#define ARM_SMCCC_VENDOR_KVM_UID UUID_INIT(.......)
#define ARM_SMCCC_VENDOR_HYP_UID UUID_INIT(.......)

Hence, the ARM_SMCCC_VENDOR_HYP_UID_*_REG_{0,1,2,3} can be removed as
you're suggesting.

That looks enticing enough semantically as though we're building layers
from the SMCCC spec down to the "on-wire format" -- the only part that
needs "deserializing" the UUID from `struct arm_smccc_res` the
hypervisor returns.

To add to that, anyone who wishes to implement a hypervisor for arm64
will have to use some RFC 9562-compliant UUID generating facility. Thus,
the UUID predates these 4 dwords. Using UUIDs in the kernel code will
relieve of the chore of figuring out the 4 dwords from the UUID.

Also, for the Gunyah folks will be pretty easy to use this infra:
define the UUID in the header (1 line), call the new function (1 line),
done.

> 
> If you want to keep the four 32-bit values and pass them into
> arm_smccc_hyp_present() directly, I think that is also fine,
> but in that case, I would try to avoid calling it a UUID.

IMO, that approach provides a simplicity where anyone can see if the
code is wrong from a quick glance: just compare 4 dwords. The fact that
the 4 dwords form an UUID is bypassed though (as it is in the existing
code). Somehow feels not spec-accurate imo. Also when I remove the UID
part from the names, I'm going to have a rather weak justification as
to why this is a benefit.

Likely, there are two levels of improvement here:

1. Just refactor the common parts out and have
    `bool arm_smccc_hyp_present(u32 reg0, u32 reg1, u32 reg2, u32 reg2);`

2. Introduce the UUID usage throughout and have a spec-accurate
    prototype of
    `bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);`

and would be great to go for the second one :)

> 
> How are the kvm and hyperv values specified originally?
>>From the SMCCC document it seems like they are meant to be
> UUIDs, so I would expect them to be in canonical form rather
> than the smccc return values, but I could not find a document
> for them.

For hyperv case, `uuidgen` produced the UUID and that is used.
Likely the same for kvm.

> 
>       Arnd

-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Arnd Bergmann]

On Tue, Feb 25, 2025, at 23:25, Roman Kisel wrote:
> On 2/24/2025 11:24 PM, Arnd Bergmann wrote:
>> On Tue, Feb 25, 2025, at 00:22, Roman Kisel wrote:
>>> Hi Arnd,
>>
>> If you want to declare a uuid here, I think you should remove the
>> ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_{0,1,2,3} macros and just
>> have UUID in normal UUID_INIT() notation as we do for
>> other UUIDs.
>
> I'd gladly stick to that provided I have your support of touching
> KVM's code! As the SMCCC document states, there shall be an UUID,
> and in the kernel, there would be
>
> #define ARM_SMCCC_VENDOR_KVM_UID UUID_INIT(.......)
> #define ARM_SMCCC_VENDOR_HYP_UID UUID_INIT(.......)
>
> Hence, the ARM_SMCCC_VENDOR_HYP_UID_*_REG_{0,1,2,3} can be removed as
> you're suggesting.

Yes, I think that's the best way forward, as it improves
the existing KVM code and all future functions like it.

    Arnd

RE: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 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. Then
> use the vendor-specific hypervisor service call (implemented
> recently in Hyper-V) via SMCCC in the non-ACPI case.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/arm64/hyperv/mshyperv.c      | 43 +++++++++++++++++++++++++++----
>  arch/arm64/include/asm/mshyperv.h |  5 ++++
>  2 files changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
> index fc49949b7df6..fe6185bf3bf2 100644
> --- a/arch/arm64/hyperv/mshyperv.c
> +++ b/arch/arm64/hyperv/mshyperv.c
> @@ -27,6 +27,36 @@ 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+. */

The comment is correct, but to me doesn't tell the full story.
I initially wondered why the revision test < 6 was being done,
since Hyper-V for ARM64 always provides ACPI 6.x or greater.
But the test is needed to catch running in some unknown
non-Hyper-V environment that has ACPI 5.x or less. In such a
case, it can't be Hyper-V, so just return false instead of testing
a bogus hypervisor_id field. Maybe a comment would help
explain it to someone like me who was confused. :-)

> +	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_smccc(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);
> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
> +		return false;
> +
> +	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 +65,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_smccc())
>  		return 0;
> 
>  	/* Setup the guest ID */
> @@ -72,6 +100,11 @@ static int __init hyperv_init(void)
>  		return ret;
>  	}
> 
> +	ms_hyperv.vtl = get_vtl();

The above statement looks like it will fail to compile on
arm64 since the get_vtl() function is entirely on the x86
side until Patch 3 of this series. As of this Patch 1, there's
no declaration of get_vtl() available to arm64.

> +	/* Report if non-default VTL */
> +	if (ms_hyperv.vtl > 0)
> +		pr_info("Linux runs in Hyper-V Virtual Trust Level\n");

Could this message include the VTL number as well? In the long
run, I think there will be code at non-zero VTLs other than VTL 2.

> +
>  	ms_hyperv_late_init();
> 
>  	hyperv_initialized = true;
> diff --git a/arch/arm64/include/asm/mshyperv.h
> b/arch/arm64/include/asm/mshyperv.h
> index 2e2f83bafcfb..a6d7eb9e167b 100644
> --- a/arch/arm64/include/asm/mshyperv.h
> +++ b/arch/arm64/include/asm/mshyperv.h
> @@ -50,4 +50,9 @@ static inline u64 hv_get_msr(unsigned int reg)
> 
>  #include <asm-generic/mshyperv.h>
> 
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x4d32ba58U
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0xcd244764U
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0x8eef6c75U
> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0x16597024U
> +
>  #endif
> --
> 2.43.0
> 

Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

[Roman Kisel]

On 2/19/2025 3:13 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 PM
>>

[...]

>>   }
>>
>> +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+. */
> 
> The comment is correct, but to me doesn't tell the full story.
> I initially wondered why the revision test < 6 was being done,
> since Hyper-V for ARM64 always provides ACPI 6.x or greater.
> But the test is needed to catch running in some unknown
> non-Hyper-V environment that has ACPI 5.x or less. In such a
> case, it can't be Hyper-V, so just return false instead of testing
> a bogus hypervisor_id field. Maybe a comment would help
> explain it to someone like me who was confused. :-)
> 

Thanks, I'll extend the comment to tell the whole story!

[...]

>> +	ms_hyperv.vtl = get_vtl();
> 
> The above statement looks like it will fail to compile on
> arm64 since the get_vtl() function is entirely on the x86
> side until Patch 3 of this series. As of this Patch 1, there's
> no declaration of get_vtl() available to arm64.
> 

I used my working branch for testing
https://github.com/romank-msft/linux-hyperv/tree/vtl_mode_arm64_v4
and didn't move that code around when chunking into patches.
Will fix the workflow, thanks for catching this!

>> +	/* Report if non-default VTL */
>> +	if (ms_hyperv.vtl > 0)
>> +		pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> 
> Could this message include the VTL number as well? In the long
> run, I think there will be code at non-zero VTLs other than VTL 2.
> 

Absolutely, will add!

>> +
>>   	ms_hyperv_late_init();
>>
>>   	hyperv_initialized = true;
>> diff --git a/arch/arm64/include/asm/mshyperv.h
>> b/arch/arm64/include/asm/mshyperv.h
>> index 2e2f83bafcfb..a6d7eb9e167b 100644
>> --- a/arch/arm64/include/asm/mshyperv.h
>> +++ b/arch/arm64/include/asm/mshyperv.h
>> @@ -50,4 +50,9 @@ static inline u64 hv_get_msr(unsigned int reg)
>>
>>   #include <asm-generic/mshyperv.h>
>>
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x4d32ba58U
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0xcd244764U
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0x8eef6c75U
>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0x16597024U
>> +
>>   #endif
>> --
>> 2.43.0
>>

-- 
Thank you,
Roman

[PATCH hyperv-next v4 2/6] 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 unconditionally.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 drivers/hv/Kconfig | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 862c47b191af..db9912ef96a8 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -5,18 +5,20 @@ 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
 	help
 	  Select this option to run Linux as a Hyper-V client operating
 	  system.
 
 config HYPERV_VTL_MODE
 	bool "Enable Linux to boot in VTL context"
-	depends on X86_64 && HYPERV
+	depends on (X86 || ARM64)
 	depends on SMP
+	select OF_EARLY_FLATTREE
+	select OF
 	default n
 	help
 	  Virtual Secure Mode (VSM) is a set of hypervisor capabilities and
@@ -31,7 +33,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.43.0

RE: [PATCH hyperv-next v4 2/6] Drivers: hv: Enable VTL mode for arm64

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 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 unconditionally.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/hv/Kconfig | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 862c47b191af..db9912ef96a8 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -5,18 +5,20 @@ 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
>  	help
>  	  Select this option to run Linux as a Hyper-V client operating
>  	  system.
> 
>  config HYPERV_VTL_MODE
>  	bool "Enable Linux to boot in VTL context"
> -	depends on X86_64 && HYPERV
> +	depends on (X86 || ARM64)

Any reason to choose "X86" instead of "X86_64"? I can't
imagine VTL mode making any sense for 32-bit, but maybe
I'm wrong! :-)

>  	depends on SMP
> +	select OF_EARLY_FLATTREE
> +	select OF
>  	default n
>  	help
>  	  Virtual Secure Mode (VSM) is a set of hypervisor capabilities and
> @@ -31,7 +33,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.43.0
> 

Re: [PATCH hyperv-next v4 2/6] Drivers: hv: Enable VTL mode for arm64

[Roman Kisel]

On 2/19/2025 3:14 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 PM

[...]

>>   config HYPERV_VTL_MODE
>>   	bool "Enable Linux to boot in VTL context"
>> -	depends on X86_64 && HYPERV
>> +	depends on (X86 || ARM64)
> 
> Any reason to choose "X86" instead of "X86_64"? I can't
> imagine VTL mode making any sense for 32-bit, but maybe
> I'm wrong! :-)

You're 100% right, appreciate your review very much :)
Will tweak this in the next submission.

-- 
Thank you,
Roman

[PATCH hyperv-next v4 3/6] 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.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 arch/x86/hyperv/hv_init.c      | 34 ----------------------------------
 drivers/hv/hv_common.c         | 32 ++++++++++++++++++++++++++++++++
 include/asm-generic/mshyperv.h |  6 ++++++
 include/hyperv/hvgdk_mini.h    |  2 +-
 4 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 173005e6a95d..383bca1a3ae2 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -411,40 +411,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_input_get_vp_registers *input;
-	struct hv_output_get_vp_registers *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, names, 1));
-	input->partition_id = HV_PARTITION_ID_SELF;
-	input->vp_index = HV_VP_INDEX_SELF;
-	input->input_vtl.as_uint8 = 0;
-	input->names[0] = HV_REGISTER_VSM_VP_STATUS;
-
-	ret = hv_do_hypercall(control, input, output);
-	if (hv_result_success(ret)) {
-		ret = output->values[0].reg8 & 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/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index af5d1dc451f6..70f754710170 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -283,6 +283,38 @@ static inline bool hv_output_page_exists(void)
 	return hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE);
 }
 
+#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_input_get_vp_registers *input;
+	struct hv_output_get_vp_registers *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, names, 1));
+	input->partition_id = HV_PARTITION_ID_SELF;
+	input->vp_index = HV_VP_INDEX_SELF;
+	input->input_vtl.as_uint8 = 0;
+	input->names[0] = HV_REGISTER_VSM_VP_STATUS;
+
+	ret = hv_do_hypercall(control, input, output);
+	if (hv_result_success(ret)) {
+		ret = output->values[0].reg8 & HV_VTL_MASK;
+	} else {
+		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
+		BUG();
+	}
+
+	local_irq_restore(flags);
+	return ret;
+}
+#endif
+
 int __init hv_common_init(void)
 {
 	int i;
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index a7bbe504e4f3..bb36856c3467 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -314,4 +314,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
diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index 155615175965..0f8443595732 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h
@@ -1202,7 +1202,7 @@ struct hv_send_ipi {	 /* HV_INPUT_SEND_SYNTHETIC_CLUSTER_IPI */
 	u64 cpu_mask;
 } __packed;
 
-#define	HV_X64_VTL_MASK			GENMASK(3, 0)
+#define	HV_VTL_MASK			GENMASK(3, 0)
 
 /* Hyper-V memory host visibility */
 enum hv_mem_host_visibility {
-- 
2.43.0

RE: [PATCH hyperv-next v4 3/6] Drivers: hv: Provide arch-neutral implementation of get_vtl()

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 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.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c      | 34 ----------------------------------
>  drivers/hv/hv_common.c         | 32 ++++++++++++++++++++++++++++++++
>  include/asm-generic/mshyperv.h |  6 ++++++
>  include/hyperv/hvgdk_mini.h    |  2 +-
>  4 files changed, 39 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 173005e6a95d..383bca1a3ae2 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -411,40 +411,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_input_get_vp_registers *input;
> -	struct hv_output_get_vp_registers *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, names, 1));
> -	input->partition_id = HV_PARTITION_ID_SELF;
> -	input->vp_index = HV_VP_INDEX_SELF;
> -	input->input_vtl.as_uint8 = 0;
> -	input->names[0] = HV_REGISTER_VSM_VP_STATUS;
> -
> -	ret = hv_do_hypercall(control, input, output);
> -	if (hv_result_success(ret)) {
> -		ret = output->values[0].reg8 & 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/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index af5d1dc451f6..70f754710170 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -283,6 +283,38 @@ static inline bool hv_output_page_exists(void)
>  	return hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE);
>  }
> 
> +#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_input_get_vp_registers *input;
> +	struct hv_output_get_vp_registers *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, names, 1));
> +	input->partition_id = HV_PARTITION_ID_SELF;
> +	input->vp_index = HV_VP_INDEX_SELF;
> +	input->input_vtl.as_uint8 = 0;
> +	input->names[0] = HV_REGISTER_VSM_VP_STATUS;
> +
> +	ret = hv_do_hypercall(control, input, output);
> +	if (hv_result_success(ret)) {
> +		ret = output->values[0].reg8 & HV_VTL_MASK;
> +	} else {
> +		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
> +		BUG();
> +	}
> +
> +	local_irq_restore(flags);
> +	return ret;
> +}
> +#endif
> +
>  int __init hv_common_init(void)
>  {
>  	int i;
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index a7bbe504e4f3..bb36856c3467 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -314,4 +314,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
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index 155615175965..0f8443595732 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -1202,7 +1202,7 @@ struct hv_send_ipi {	 /*
> HV_INPUT_SEND_SYNTHETIC_CLUSTER_IPI */
>  	u64 cpu_mask;
>  } __packed;
> 
> -#define	HV_X64_VTL_MASK			GENMASK(3, 0)
> +#define	HV_VTL_MASK			GENMASK(3, 0)
> 
>  /* Hyper-V memory host visibility */
>  enum hv_mem_host_visibility {
> --
> 2.43.0
> 

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

[PATCH hyperv-next v4 4/6] dt-bindings: microsoft,vmbus: Add GIC and DMA coherence to the example

[Roman Kisel]

The existing example lacks the GIC interrupt controller property
making it not possible to boot on ARM64, and it lacks the DMA
coherence property making the kernel do more work on maintaining
CPU caches on ARM64 although the VMBus trancations are cache-coherent.

Add the GIC node, specify DMA coherence, and define interrupt-parent
and interrupts properties in the example to provide a complete reference
for platforms utilizing GIC-based interrupts, and add the DMA coherence
property to not do extra work on the architectures where DMA defaults to
non cache-coherent.

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

Re: [PATCH hyperv-next v4 4/6] dt-bindings: microsoft,vmbus: Add GIC and DMA coherence to the example

[Krzysztof Kozlowski]

On Tue, Feb 11, 2025 at 05:43:19PM -0800, Roman Kisel wrote:
> The existing example lacks the GIC interrupt controller property
> making it not possible to boot on ARM64, and it lacks the DMA

GIC controller is not relevant to this binding.

> coherence property making the kernel do more work on maintaining
> CPU caches on ARM64 although the VMBus trancations are cache-coherent.
> 
> Add the GIC node, specify DMA coherence, and define interrupt-parent
> and interrupts properties in the example to provide a complete reference
> for platforms utilizing GIC-based interrupts, and add the DMA coherence
> property to not do extra work on the architectures where DMA defaults to
> non cache-coherent.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  .../devicetree/bindings/bus/microsoft,vmbus.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Last time I said: not tested by automation.
Now: I see automation build failures, although I do not see anything
incorrect in the code, so that's a bit surprising. Please confirm that
binding was tested on latest dtschema.

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

I fail to see how this is relevant here. This is example only of vmbus.
Look how other bindings are done. Drop the example.


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

Use proper defines for known constants.

Best regards,
Krzysztof

Re: [PATCH hyperv-next v4 4/6] dt-bindings: microsoft,vmbus: Add GIC and DMA coherence to the example

[Roman Kisel]

On 2/11/2025 10:42 PM, Krzysztof Kozlowski wrote:
> On Tue, Feb 11, 2025 at 05:43:19PM -0800, Roman Kisel wrote:
>> The existing example lacks the GIC interrupt controller property
>> making it not possible to boot on ARM64, and it lacks the DMA
> 
> GIC controller is not relevant to this binding.
> 

Will remove, thank you for pointing that out!

>> coherence property making the kernel do more work on maintaining
>> CPU caches on ARM64 although the VMBus trancations are cache-coherent.
>>
>> Add the GIC node, specify DMA coherence, and define interrupt-parent
>> and interrupts properties in the example to provide a complete reference
>> for platforms utilizing GIC-based interrupts, and add the DMA coherence
>> property to not do extra work on the architectures where DMA defaults to
>> non cache-coherent.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   .../devicetree/bindings/bus/microsoft,vmbus.yaml      | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
> 
> Last time I said: not tested by automation.
> Now: I see automation build failures, although I do not see anything
> incorrect in the code, so that's a bit surprising. Please confirm that
> binding was tested on latest dtschema.

They weren't for which I am sorry. Read through

https://www.kernel.org/doc/html/v6.14-rc2/devicetree/bindings/writing-schema.html

and was able to see and fix the break by bringing the YAML to [1].
Getting now this

/Documentation/devicetree/bindings/bus/microsoft,vmbus.example.dtb: 
vmbus@ff0000000: 'dma-coherent', 'interrupts' do not match any of the 
regexes: 'pinctrl-[0-9]+'
         from schema $id: 
http://devicetree.org/schemas/bus/microsoft,vmbus.yaml#

so maybe I need to add some more to the "requires" section. Will follow
other examples as you suggested.

> 
>>
>> 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>;
>> +            }
> 
> I fail to see how this is relevant here. This is example only of vmbus.
> Look how other bindings are done. Drop the example.

The bus refers to the interrupt controller, and I didn't have it, so
added it :) Now I in other examples that is not required, and the
tooling generates fake intc's. Appreciate your advice very much!

> 
> 
>> +
>>               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>;
> 
> Use proper defines for known constants.

Will do as in [1], thank you!

> 

[1]

--- a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
+++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
@@ -28,6 +28,7 @@ properties:
  required:
    - compatible
    - ranges
+  - interrupts
    - '#address-cells'
    - '#size-cells'

@@ -35,6 +36,8 @@ additionalProperties: false

  examples:
    - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
      soc {
          #address-cells = <2>;
          #size-cells = <1>;
@@ -44,14 +47,6 @@ 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>;
@@ -59,7 +54,7 @@ examples:
                  ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
                  dma-coherent;
                  interrupt-parent = <&gic>;
-                interrupts = <1 2 1>;
+                interrupts = <GIC_PPI 2 IRQ_TYPE_EDGE_RISING>;
              };
          };
      };

> Best regards,
> Krzysztof

-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 4/6] dt-bindings: microsoft,vmbus: Add GIC and DMA coherence to the example

[Roman Kisel]

On 2/12/2025 3:57 PM, Roman Kisel wrote:
> 

[...]

Thank you for your guidance!! The below passes tests and addresses the
feedback you have provided. If no further comments from you, I'll
send the file in this form in the next version of the patch series (also
fixing the commit title and description).


# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/bus/microsoft,vmbus.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: Microsoft Hyper-V VMBus

maintainers:
   - Saurabh Sengar <ssengar@linux.microsoft.com>

description:
   VMBus is a software bus that implement the protocols for communication
   between the root or host OS and guest OSs (virtual machines).

properties:
   compatible:
     const: microsoft,vmbus

   ranges: true

   '#address-cells':
     const: 2

   '#size-cells':
     const: 1

required:
   - compatible
   - ranges
   - interrupts
   - '#address-cells'
   - '#size-cells'

additionalProperties: true

examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     soc {
         #address-cells = <2>;
         #size-cells = <1>;
         bus {
             compatible = "simple-bus";
             #address-cells = <2>;
             #size-cells = <1>;
             ranges;

             vmbus@ff0000000 {
                 compatible = "microsoft,vmbus";
                 #address-cells = <2>;
                 #size-cells = <1>;
                 ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
                 dma-coherent;
                 interrupt-parent = <&gic>;
                 interrupts = <GIC_PPI 2 IRQ_TYPE_EDGE_RISING>;
             };
         };
     };


>> Best regards,
>> Krzysztof
> 

-- 
Thank you,
Roman

[PATCH hyperv-next v4 5/6] Drivers: hv: vmbus: Get the IRQ number from DeviceTree

[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
from DT.

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

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0f6cd44fff29..9d0c2dbd2a69 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2335,6 +2335,36 @@ static int vmbus_acpi_add(struct platform_device *pdev)
 }
 #endif
 
+static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
+{
+	struct irq_data *data;
+	int irq;
+	irq_hw_number_t hwirq;
+
+	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;
+	}
+
+	data = irq_get_irq_data(irq);
+	if (!data) {
+		pr_err("No interrupt data for VMBus virq %d\n", irq);
+		return -ENODEV;
+	}
+	hwirq = irqd_to_hwirq(data);
+
+	vmbus_irq = irq;
+	vmbus_interrupt = 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;
@@ -2349,6 +2379,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;
 
-- 
2.43.0

RE: [PATCH hyperv-next v4 5/6] Drivers: hv: vmbus: Get the IRQ number from DeviceTree

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 PM
> 
> The VMBus driver uses ACPI for interrupt assignment on
> arm64 hence it won't function in the VTL mode where only
> DeviceTree can be used.
> 
> Update the VMBus driver to discover interrupt configuration
> from DT.
> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 0f6cd44fff29..9d0c2dbd2a69 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2335,6 +2335,36 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  }
>  #endif
> 
> +static int __maybe_unused vmbus_set_irq(struct platform_device *pdev)
> +{
> +	struct irq_data *data;
> +	int irq;
> +	irq_hw_number_t hwirq;
> +
> +	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;
> +	}
> +
> +	data = irq_get_irq_data(irq);
> +	if (!data) {
> +		pr_err("No interrupt data for VMBus virq %d\n", irq);
> +		return -ENODEV;
> +	}
> +	hwirq = irqd_to_hwirq(data);
> +
> +	vmbus_irq = irq;
> +	vmbus_interrupt = 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;
> @@ -2349,6 +2379,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;
> 
> --
> 2.43.0
> 

Having to do the #ifdef HYPERVISOR_CALLBACK_VECTOR is
unfortunate, but I don't immediately have a cleaner approach
to offer.

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

[PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[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 | 69 ++++++++++++++++++++++++++---
 include/linux/hyperv.h              |  2 +
 3 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void)
+{
+	return vmbus_root_device;
+}
+EXPORT_SYMBOL_GPL(hv_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;
 }
 
@@ -1920,7 +1927,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;
@@ -2282,7 +2289,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
@@ -2373,7 +2380,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)
@@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void)
 	if (ret)
 		return ret;
 
-	if (!hv_dev) {
+	if (!vmbus_root_device) {
 		ret = -ENODEV;
 		goto cleanup;
 	}
@@ -2723,7 +2730,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 cdd5be16021d..24725bea9ef1 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>
 
 /*
@@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain,
 	int ret;
 
 	fwspec.fwnode = domain->parent->fwnode;
-	fwspec.param_count = 2;
-	fwspec.param[0] = hwirq;
-	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+	if (is_of_node(fwspec.fwnode)) {
+		/* SPI lines for OF translations start at offset 32 */
+		fwspec.param_count = 3;
+		fwspec.param[0] = 0;
+		fwspec.param[1] = hwirq - 32;
+		fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
+	} else {
+		fwspec.param_count = 2;
+		fwspec.param[0] = hwirq;
+		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+	}
 
 	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
 	if (ret)
@@ -887,6 +896,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(hv_get_vmbus_root_device()->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 +944,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 4179add2864b..2be4dd83b0e1 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
 	return dev_get_drvdata(&dev->device);
 }
 
+struct device *hv_get_vmbus_root_device(void);
+
 struct hv_ring_buffer_debug_info {
 	u32 current_interrupt_mask;
 	u32 current_read_index;
-- 
2.43.0

Re: [PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[Bjorn Helgaas]

On Tue, Feb 11, 2025 at 05:43:21PM -0800, 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 | 69 ++++++++++++++++++++++++++---
>  include/linux/hyperv.h              |  2 +
>  3 files changed, 80 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void)
> +{
> +	return vmbus_root_device;
> +}
> +EXPORT_SYMBOL_GPL(hv_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;
>  }
>  
> @@ -1920,7 +1927,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;
> @@ -2282,7 +2289,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
> @@ -2373,7 +2380,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)
> @@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void)
>  	if (ret)
>  		return ret;
>  
> -	if (!hv_dev) {
> +	if (!vmbus_root_device) {
>  		ret = -ENODEV;
>  		goto cleanup;
>  	}
> @@ -2723,7 +2730,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 cdd5be16021d..24725bea9ef1 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>
>  
>  /*
> @@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain,
>  	int ret;
>  
>  	fwspec.fwnode = domain->parent->fwnode;
> -	fwspec.param_count = 2;
> -	fwspec.param[0] = hwirq;
> -	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +	if (is_of_node(fwspec.fwnode)) {
> +		/* SPI lines for OF translations start at offset 32 */
> +		fwspec.param_count = 3;
> +		fwspec.param[0] = 0;
> +		fwspec.param[1] = hwirq - 32;
> +		fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else {
> +		fwspec.param_count = 2;
> +		fwspec.param[0] = hwirq;
> +		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +	}
>  
>  	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>  	if (ret)
> @@ -887,6 +896,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(hv_get_vmbus_root_device()->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.

The rest of this file fits in 80 columns; please wrap this to match.

> +	 */
> +	if (!domain)
> +		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");

Is there a way to include a hint about what specific part of the
devicetree to look at, e.g., the node that lacks a parent?

> +	return domain;
> +}
> +
> +#endif
> +
>  static int hv_pci_irqchip_init(void)
>  {
>  	static struct hv_pci_chip_data *chip_data;
> @@ -906,10 +944,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.

Add a blank line if you intend a new paragraph here.  Otherwise, wrap
to fill 78 columns or so.

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

And here.

> +	 * None of these two cases reaches for the MSI parent domain.

I don't know what "reaches for the MSI parent domain" means.  Neither
"searches for"?

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

I don't know if acpi_irq_create_hierarchy() is helping or hurting
here.  It obscures the fact that the only difference is the first
argument to irq_domain_create_hierarchy().  If we could open-code or
have a helper to figure out that irq_domain "parent" argument for the
ACPI case, then we'd only have one call of
irq_domain_create_hierarchy() here and it seems like it might be
simpler.

>  	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 4179add2864b..2be4dd83b0e1 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
>  	return dev_get_drvdata(&dev->device);
>  }
>  
> +struct device *hv_get_vmbus_root_device(void);
> +
>  struct hv_ring_buffer_debug_info {
>  	u32 current_interrupt_mask;
>  	u32 current_read_index;
> -- 
> 2.43.0
> 

Re: [PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[Roman Kisel]

On 2/12/2025 9:42 AM, Bjorn Helgaas wrote:
> On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote:

[...]

>> +	 * function called later.
> 
> The rest of this file fits in 80 columns; please wrap this to match.
> 

Will fix, thank you for taking the time to review that!

>> +	 */
>> +	if (!domain)
>> +		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
> 
> Is there a way to include a hint about what specific part of the
> devicetree to look at, e.g., the node that lacks a parent?

I'll improve this, will mention the bus, thanks!

[...]

>> +	 * the messy ifdef below.
> 
> Add a blank line if you intend a new paragraph here.  Otherwise, wrap
> to fill 78 columns or so.

Will fix this, appreciate noticing that!

> 
>> +	 * 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.
> 
> And here.

Will fix, thanks!

>> +	 * None of these two cases reaches for the MSI parent domain.
> 
> I don't know what "reaches for the MSI parent domain" means.  Neither
> "searches for"?
> 

My bad, sorry about the incomprehensible phrasing! Will fix this, thank
you!

>>   	 */
>> -	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
> 
> I don't know if acpi_irq_create_hierarchy() is helping or hurting
> here.  It obscures the fact that the only difference is the first
> argument to irq_domain_create_hierarchy().  If we could open-code or
> have a helper to figure out that irq_domain "parent" argument for the
> ACPI case, then we'd only have one call of
> irq_domain_create_hierarchy() here and it seems like it might be
> simpler.
> 

That looks quite dirty, no dispute over that... The root device was
static/provate for the ACPI case, and I didn't go for changing the ACPI
subsystem code to improve this patch, thought the only user wouldn't
justify tinkering with the whole ACPI subsystem. Maybe I also will
need to see if that can be used from a module/builti-in, locking,
bogus usage, i.e. all that normally comes with promoting a private
interface to public.

Let me work out the details and post the change here to see what
feedback that receives.

Last but certainly not least: owing a great debt of gratitude to you
(and all other folks) for helping in bringing this to the best shape
possible!

-- 
Thank you,
Roman

Re: [PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[Roman Kisel]

On 2/12/2025 9:42 AM, Bjorn Helgaas wrote:
> On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote:

[...]

>>   	 */
>> -	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
> 
> I don't know if acpi_irq_create_hierarchy() is helping or hurting
> here.  It obscures the fact that the only difference is the first
> argument to irq_domain_create_hierarchy().  If we could open-code or
> have a helper to figure out that irq_domain "parent" argument for the
> ACPI case, then we'd only have one call of
> irq_domain_create_hierarchy() here and it seems like it might be
> simpler.
> 

Hey Bjorn, folks,

I've added few ACPI maintainers and the ACPI list as we're discussing
making a small change to the ACPI subsystem to make one static variable
available to make the code above less messy.

Change [1] makes the GSI dispatcher function available to
the outside world. Would you suggest going in that direction or there
is a better approach to converge the code above that deals with IRQ
domains both in the ACPI and DT cases?

[1]

 From c6fb8bda21d6c00a308b1febc201a3a7e704c5a9 Mon Sep 17 00:00:00 2001
From: Roman Kisel <romank@linux.microsoft.com>
Date: Wed, 19 Feb 2025 15:04:06 -0800
Subject: [PATCH] Refactor the ACPI GIC case

---
  drivers/acpi/irq.c                  | 14 ++++++-
  drivers/pci/controller/pci-hyperv.c | 62 +++++++++++++++++------------
  include/linux/acpi.h                |  5 ++-
  3 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index 1687483ff319..6243db610137 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c
@@ -12,7 +12,7 @@

  enum acpi_irq_model_id acpi_irq_model;

-static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi);
+static acpi_gsi_domain_disp_fn acpi_get_gsi_domain_id;
  static u32 (*acpi_gsi_to_irq_fallback)(u32 gsi);

  /**
@@ -307,12 +307,22 @@ EXPORT_SYMBOL_GPL(acpi_irq_get);
   *	for a given GSI
   */
  void __init acpi_set_irq_model(enum acpi_irq_model_id model,
-			       struct fwnode_handle *(*fn)(u32))
+	acpi_gsi_domain_disp_fn fn)
  {
  	acpi_irq_model = model;
  	acpi_get_gsi_domain_id = fn;
  }

+/**
+ * acpi_get_gsi_dispatcher - Returns dispatcher function that
+ *                           computes the domain fwnode for a
+ *                           given GSI.
+ */
+acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void)
+{
+	return acpi_get_gsi_domain_id;
+}
+
  /**
   * acpi_set_gsi_to_irq_fallback - Register a GSI transfer
   * callback to fallback to arch specified implementation.
diff --git a/drivers/pci/controller/pci-hyperv.c 
b/drivers/pci/controller/pci-hyperv.c
index 24725bea9ef1..59e670e1cb6e 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -910,16 +910,29 @@ static struct irq_domain 
*hv_pci_of_irq_domain_parent(void)
  		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.
-	 */
+	return domain;
+}
+
+#endif
+
+#ifdef CONFIG_ACPI
+
+static struct irq_domain *hv_pci_acpi_irq_domain_parent(void)
+{
+	struct irq_domain *domain;
+	acpi_gsi_domain_disp_fn gsi_domain_disp_fn;
+
+	if (acpi_irq_model != ACPI_IRQ_MODEL_GIC)
+		return NULL;
+	gsi_domain_disp_fn = acpi_get_gsi_dispatcher();
+	if (!gsi_domain_disp_fn)
+		return NULL;
+	domain = irq_find_matching_fwnode(gsi_domain_disp_fn(0),
+				     DOMAIN_BUS_ANY);
+
  	if (!domain)
-		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
+		return NULL;
+
  	return domain;
  }

@@ -929,6 +942,7 @@ static int hv_pci_irqchip_init(void)
  {
  	static struct hv_pci_chip_data *chip_data;
  	struct fwnode_handle *fn = NULL;
+	struct irq_domain *irq_domain_parent = NULL;
  	int ret = -ENOMEM;

  	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
@@ -944,29 +958,25 @@ 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.
  	 */
  #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);
+		irq_domain_parent = hv_pci_acpi_irq_domain_parent();
  #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);
+	if (!irq_domain_parent)
+		irq_domain_parent = hv_pci_of_irq_domain_parent();
  #endif
+	if (!irq_domain_parent) {
+		WARN_ONCE(1, "Invalid firmware configuration for VMBus interrupts\n");
+		ret = -EINVAL;
+		goto free_chip;
+	}
+
+	hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+		irq_domain_parent, 0, HV_PCI_MSI_SPI_NR,
+		fn, &hv_pci_domain_ops,
+		chip_data);

  	if (!hv_msi_gic_irq_domain) {
  		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6adcd1b92b20..cd70a72c7073 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -336,8 +336,11 @@ int acpi_register_gsi (struct device *dev, u32 gsi, 
int triggering, int polarity
  int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
  int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi);

+typedef struct fwnode_handle *(*acpi_gsi_domain_disp_fn)(u32);
+
  void acpi_set_irq_model(enum acpi_irq_model_id model,
-			struct fwnode_handle *(*)(u32));
+	acpi_gsi_domain_disp_fn fn);
+acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void);
  void acpi_set_gsi_to_irq_fallback(u32 (*)(u32));

  struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
-- 
2.43.0



-- 
Thank you,
Roman

RE: [PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[Michael Kelley]

From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 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>
> ---
>  drivers/hv/vmbus_drv.c              | 23 ++++++----
>  drivers/pci/controller/pci-hyperv.c | 69 ++++++++++++++++++++++++++---
>  include/linux/hyperv.h              |  2 +
>  3 files changed, 80 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void)
> +{
> +	return vmbus_root_device;
> +}
> +EXPORT_SYMBOL_GPL(hv_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;
>  }
> 
> @@ -1920,7 +1927,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;
> @@ -2282,7 +2289,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
> @@ -2373,7 +2380,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)
> @@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void)
>  	if (ret)
>  		return ret;
> 
> -	if (!hv_dev) {
> +	if (!vmbus_root_device) {
>  		ret = -ENODEV;
>  		goto cleanup;
>  	}
> @@ -2723,7 +2730,7 @@ static int __init hv_acpi_init(void)
> 
>  cleanup:
>  	platform_driver_unregister(&vmbus_platform_driver);
> -	hv_dev = NULL;
> +	vmbus_root_device = NULL;
>  	return ret;
>  }

These changes to rename hv_dev to vmbus_root_device, along with the
introduction of hv_get_vmbus_root_device(), seem like a separate
patch from the vPCI changes. The rename is definitely needed because
"hv_dev" as a symbol is very overloaded. But the rename is "no functional
change", and it doesn't touch the pci-hyperv.c file. You don't have a
consumer for hv_get_vmbus_root_device() until the vPCI changes, but
that seems OK to me to be in the subsequent patch.

> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index cdd5be16021d..24725bea9ef1 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>
> 
>  /*
> @@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain,
>  	int ret;
> 
>  	fwspec.fwnode = domain->parent->fwnode;
> -	fwspec.param_count = 2;
> -	fwspec.param[0] = hwirq;
> -	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +	if (is_of_node(fwspec.fwnode)) {
> +		/* SPI lines for OF translations start at offset 32 */
> +		fwspec.param_count = 3;
> +		fwspec.param[0] = 0;
> +		fwspec.param[1] = hwirq - 32;
> +		fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> +	} else {
> +		fwspec.param_count = 2;
> +		fwspec.param[0] = hwirq;
> +		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +	}
> 
>  	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>  	if (ret)
> @@ -887,6 +896,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(hv_get_vmbus_root_device()->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 +944,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 4179add2864b..2be4dd83b0e1 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
>  	return dev_get_drvdata(&dev->device);
>  }
> 
> +struct device *hv_get_vmbus_root_device(void);
> +
>  struct hv_ring_buffer_debug_info {
>  	u32 current_interrupt_mask;
>  	u32 current_read_index;
> --
> 2.43.0
> 

Re: [PATCH hyperv-next v4 6/6] PCI: hv: Get vPCI MSI IRQ domain from DeviceTree

[Roman Kisel]

On 2/19/2025 3:29 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 PM

[...]

>>   }
> 
> These changes to rename hv_dev to vmbus_root_device, along with the
> introduction of hv_get_vmbus_root_device(), seem like a separate
> patch from the vPCI changes. The rename is definitely needed because
> "hv_dev" as a symbol is very overloaded. But the rename is "no functional
> change", and it doesn't touch the pci-hyperv.c file. You don't have a
> consumer for hv_get_vmbus_root_device() until the vPCI changes, but
> that seems OK to me to be in the subsequent patch.
> 

Thanks, will split the NFC out! I've asked the ACPI maintainers if a
small change in ACPI would be fine to make the functional part of this
patch more palatable, too.

-- 
Thank you,
Roman