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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/4/2024 8:01 PM, Michael Kelley wrote:
From: Roman Kisel <romank@xxxxxxxxxxxxxxxxxxx> Sent: Friday, July 26, 2024 3:59 PM

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

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

Wording seems slightly messed up.  Perhaps:

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

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

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

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

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

Will fix the wording, appreciate your help with bringing this into the best shape possible!


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

diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c
index b1a4de4eee29..341f98312667 100644
--- a/arch/arm64/hyperv/mshyperv.c
+++ b/arch/arm64/hyperv/mshyperv.c
@@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info)
  	return 0;
  }

+static bool hyperv_detect_via_acpi(void)
+{
+	if (acpi_disabled)
+		return false;
+#if IS_ENABLED(CONFIG_ACPI)
+	/* Hypervisor ID is only available in ACPI v6+. */
+	if (acpi_gbl_FADT.header.revision < 6)
+		return false;
+	return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0;
+#else
+	return false;
+#endif
+}
+
+static bool hyperv_detect_via_smc(void)
+{
+	struct arm_smccc_res res = {};
+
+	if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC)
+		return false;
+	arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res);
+
+	return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 &&
+		res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 &&
+		res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 &&
+		res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3;
+}
+
  static int __init hyperv_init(void)
  {
  	struct hv_get_vp_registers_output	result;
@@ -35,13 +63,11 @@ static int __init hyperv_init(void)

  	/*
  	 * Allow for a kernel built with CONFIG_HYPERV to be running in
-	 * a non-Hyper-V environment, including on DT instead of ACPI.
+	 * a non-Hyper-V environment.
+	 *
  	 * In such cases, do nothing and return success.
  	 */
-	if (acpi_disabled)
-		return 0;
-
-	if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8))
+	if (!hyperv_detect_via_acpi() && !hyperv_detect_via_smc())
  		return 0;

  	/* Setup the guest ID */
diff --git a/arch/arm64/include/asm/mshyperv.h
b/arch/arm64/include/asm/mshyperv.h
index a975e1a689dd..a7a3586f7cb1 100644
--- a/arch/arm64/include/asm/mshyperv.h
+++ b/arch/arm64/include/asm/mshyperv.h
@@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg)

  #include <asm-generic/mshyperv.h>

+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0	0x7948734d
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1	0x56726570
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2	0
+#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3	0
+

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

The above values don't violate anything in the spec, and I think it would be hard to give an example of what can be broken in the world by using the above values. I do understand what you're saying when you mention the UIDs & the spirit of the spec. Put on the quantitative footing, the Shannon entropy of these values is much lower than that of an UID. A cursory search in the kernel tree does turn up other UIDs that don't look too random.

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

Michael

  #endif
--
2.34.1


--
Thank you,
Roman





[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux