| Message ID | 20250212014321.1108840-2-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| 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
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 >
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 >>
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
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
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
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
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(-)