| Message ID | 20250212014321.1108840-2-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | arm64: hyperv: Support Virtual Trust Level Boot | expand |
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
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
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
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
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
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(-)