| Message ID | 20240726225910.1912537-2-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: hyperv: Support Virtual Trust Level Boot | expand |
On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: > The arm64 Hyper-V startup path relies on ACPI to detect > running under a Hyper-V compatible hypervisor. That > doesn't work on non-ACPI systems. > > Hoist the ACPI detection logic into a separate function, > use the new SMC added recently to Hyper-V to use in the > non-ACPI case. > > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> The change looks sensible. Within one minor comment fixed below: Acked-by: Wei Liu <wei.liu@kernel.org> However I would also like to get an Acked-by or reivewed-by from someone who works on the ARM64 side of things -- Saurabh, Boqun, Srivatsa, and Jinank? > --- > arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- > arch/arm64/include/asm/mshyperv.h | 5 +++++ > 2 files changed, 36 insertions(+), 5 deletions(-) > [...] > diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h > index a975e1a689dd..a7a3586f7cb1 100644 > --- a/arch/arm64/include/asm/mshyperv.h > +++ b/arch/arm64/include/asm/mshyperv.h > @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) > > #include <asm-generic/mshyperv.h> > > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 Presumably these are ASCII codes for an identifier string, please provide comments to explain what they are. > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 > + > #endif > -- > 2.34.1 >
From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM > > The arm64 Hyper-V startup path relies on ACPI to detect > running under a Hyper-V compatible hypervisor. That > doesn't work on non-ACPI systems. > > Hoist the ACPI detection logic into a separate function, > use the new SMC added recently to Hyper-V to use in the > non-ACPI case. Wording seems slightly messed up. Perhaps: Hoist the ACPI detection logic into a separate function. Then use the new SMC added recently to Hyper-V in the non-ACPI case. Also, the phrase "the new SMC" seems a bit off to me. The "Terms and Abbreviations" section of the SMCCC specification defines "SMC" as an instruction: Secure Monitor Call. An Arm assembler instruction that causes an exception that is taken synchronously into EL3. More precisely, I think you mean a SMC "function identifier" that is newly implemented by Hyper-V. And the function identifier itself isn't new; it's the Hyper-V implementation that's new. Similar comment applies in the cover letter for this patch set, and perhaps to the Subject line of this patch. > > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > --- > arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- > arch/arm64/include/asm/mshyperv.h | 5 +++++ > 2 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c > index b1a4de4eee29..341f98312667 100644 > --- a/arch/arm64/hyperv/mshyperv.c > +++ b/arch/arm64/hyperv/mshyperv.c > @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) > return 0; > } > > +static bool hyperv_detect_via_acpi(void) > +{ > + if (acpi_disabled) > + return false; > +#if IS_ENABLED(CONFIG_ACPI) > + /* Hypervisor ID is only available in ACPI v6+. */ > + if (acpi_gbl_FADT.header.revision < 6) > + return false; > + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; > +#else > + return false; > +#endif > +} > + > +static bool hyperv_detect_via_smc(void) > +{ > + struct arm_smccc_res res = {}; > + > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) > + return false; > + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); > + > + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && > + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && > + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && > + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; > +} > + > static int __init hyperv_init(void) > { > struct hv_get_vp_registers_output result; > @@ -35,13 +63,11 @@ static int __init hyperv_init(void) > > /* > * Allow for a kernel built with CONFIG_HYPERV to be running in > - * a non-Hyper-V environment, including on DT instead of ACPI. > + * a non-Hyper-V environment. > + * > * In such cases, do nothing and return success. > */ > - if (acpi_disabled) > - return 0; > - > - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8)) > + if (!hyperv_detect_via_acpi() && !hyperv_detect_via_smc()) > return 0; > > /* Setup the guest ID */ > diff --git a/arch/arm64/include/asm/mshyperv.h > b/arch/arm64/include/asm/mshyperv.h > index a975e1a689dd..a7a3586f7cb1 100644 > --- a/arch/arm64/include/asm/mshyperv.h > +++ b/arch/arm64/include/asm/mshyperv.h > @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) > > #include <asm-generic/mshyperv.h> > > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 > +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 > + Section 6.2 of the SMCCC specification says that the "Call UID Query" returns a UUID. The above #defines look like an ASCII string is being returned. Arguably the ASCII string can be treated as a set of 128 bits just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V be changed to return a real UUID? While the distinction probably won't make a material difference here, we've had problems in the past with Hyper-V doing slightly weird things that later caused unexpected trouble. Please just get it right. :-) Michael > #endif > -- > 2.34.1 >
On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: > The arm64 Hyper-V startup path relies on ACPI to detect > running under a Hyper-V compatible hypervisor. That > doesn't work on non-ACPI systems. > > Hoist the ACPI detection logic into a separate function, > use the new SMC added recently to Hyper-V to use in the > non-ACPI case. > > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > --- > arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- > arch/arm64/include/asm/mshyperv.h | 5 +++++ > 2 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c > index b1a4de4eee29..341f98312667 100644 > --- a/arch/arm64/hyperv/mshyperv.c > +++ b/arch/arm64/hyperv/mshyperv.c > @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) > return 0; > } > > +static bool hyperv_detect_via_acpi(void) > +{ > + if (acpi_disabled) > + return false; > +#if IS_ENABLED(CONFIG_ACPI) > + /* Hypervisor ID is only available in ACPI v6+. */ > + if (acpi_gbl_FADT.header.revision < 6) > + return false; > + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; > +#else > + return false; > +#endif > +} > + > +static bool hyperv_detect_via_smc(void) > +{ > + struct arm_smccc_res res = {}; > + > + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) > + return false; > + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); > + > + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && > + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && > + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && > + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; > +} As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor, can we add a comment about it, specifying the exact version in it would be great. If someone attempts to build non-ACPI kernel on older Hyper-V what is the behaviour of this function, do we need to safeguard or handle that case ? - Saurabh
On 8/2/2024 6:21 PM, Wei Liu wrote: > On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: >> The arm64 Hyper-V startup path relies on ACPI to detect >> running under a Hyper-V compatible hypervisor. That >> doesn't work on non-ACPI systems. >> >> Hoist the ACPI detection logic into a separate function, >> use the new SMC added recently to Hyper-V to use in the >> non-ACPI case. >> >> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > > The change looks sensible. > > Within one minor comment fixed below: > > Acked-by: Wei Liu <wei.liu@kernel.org> > > However I would also like to get an Acked-by or reivewed-by from someone > who works on the ARM64 side of things -- Saurabh, Boqun, Srivatsa, and > Jinank? > >> --- >> arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- >> arch/arm64/include/asm/mshyperv.h | 5 +++++ >> 2 files changed, 36 insertions(+), 5 deletions(-) >> > [...] >> diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h >> index a975e1a689dd..a7a3586f7cb1 100644 >> --- a/arch/arm64/include/asm/mshyperv.h >> +++ b/arch/arm64/include/asm/mshyperv.h >> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) >> >> #include <asm-generic/mshyperv.h> >> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 > > Presumably these are ASCII codes for an identifier string, please > provide comments to explain what they are. > Michael posted a suggestion to change that altogether; will elaborate on the topic in the thread with Michael. >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 >> + >> #endif >> -- >> 2.34.1 >>
On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote: > On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: >> The arm64 Hyper-V startup path relies on ACPI to detect >> running under a Hyper-V compatible hypervisor. That >> doesn't work on non-ACPI systems. >> >> Hoist the ACPI detection logic into a separate function, >> use the new SMC added recently to Hyper-V to use in the >> non-ACPI case. >> >> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> >> --- >> arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- >> arch/arm64/include/asm/mshyperv.h | 5 +++++ >> 2 files changed, 36 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c >> index b1a4de4eee29..341f98312667 100644 >> --- a/arch/arm64/hyperv/mshyperv.c >> +++ b/arch/arm64/hyperv/mshyperv.c >> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) >> return 0; >> } >> >> +static bool hyperv_detect_via_acpi(void) >> +{ >> + if (acpi_disabled) >> + return false; >> +#if IS_ENABLED(CONFIG_ACPI) >> + /* Hypervisor ID is only available in ACPI v6+. */ >> + if (acpi_gbl_FADT.header.revision < 6) >> + return false; >> + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; >> +#else >> + return false; >> +#endif >> +} >> + >> +static bool hyperv_detect_via_smc(void) >> +{ >> + struct arm_smccc_res res = {}; >> + >> + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) >> + return false; >> + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); >> + >> + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && >> + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && >> + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && >> + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; >> +} > > As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor, > can we add a comment about it, specifying the exact version in it would be great. > I can add a comment about that, thought that would look as too much detail to refer to a version of the Windows insiders build in the comments in this code. Another option would be to entrench the logic in if statements which felt gross as there is a fallback. > If someone attempts to build non-ACPI kernel on older Hyper-V what is the > behaviour of this function, do we need to safeguard or handle that case ? The function won't panic if that's what you're asking about, i.e. safe for runtime. That won't break the build either as it relies on the SMCCC spec, and that uses the smc or hvc instructions (the code does expect hvc to be the conduit and checks for that being the case). The hypervisor doesn't inject the exception in the guest for the unknown call, just returns SMCCC_RET_NOT_SUPPORTED in the first output register (the hypervisor got a unit-test for that, too). That said, I think the logic is solid. Appreciate your question and your help! Will be glad to discuss other concerns should you have any. > > - Saurabh
On Mon, Aug 05, 2024 at 08:17:05AM -0700, Roman Kisel wrote: > > > On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote: > >On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: > >>The arm64 Hyper-V startup path relies on ACPI to detect > >>running under a Hyper-V compatible hypervisor. That > >>doesn't work on non-ACPI systems. > >> > >>Hoist the ACPI detection logic into a separate function, > >>use the new SMC added recently to Hyper-V to use in the > >>non-ACPI case. > >> > >>Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > >>--- > >> arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- > >> arch/arm64/include/asm/mshyperv.h | 5 +++++ > >> 2 files changed, 36 insertions(+), 5 deletions(-) > >> > >>diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c > >>index b1a4de4eee29..341f98312667 100644 > >>--- a/arch/arm64/hyperv/mshyperv.c > >>+++ b/arch/arm64/hyperv/mshyperv.c > >>@@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) > >> return 0; > >> } > >>+static bool hyperv_detect_via_acpi(void) > >>+{ > >>+ if (acpi_disabled) > >>+ return false; > >>+#if IS_ENABLED(CONFIG_ACPI) > >>+ /* Hypervisor ID is only available in ACPI v6+. */ > >>+ if (acpi_gbl_FADT.header.revision < 6) > >>+ return false; > >>+ return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; > >>+#else > >>+ return false; > >>+#endif > >>+} > >>+ > >>+static bool hyperv_detect_via_smc(void) > >>+{ > >>+ struct arm_smccc_res res = {}; > >>+ > >>+ if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) > >>+ return false; > >>+ arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); > >>+ > >>+ return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && > >>+ res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && > >>+ res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && > >>+ res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; > >>+} > > > >As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor, > >can we add a comment about it, specifying the exact version in it would be great. > > > I can add a comment about that, thought that would look as too much > detail to refer to a version of the Windows insiders build in the > comments in this code. Another option would be to entrench the logic > in if statements which felt gross as there is a fallback. I'll leave the decision to your judgment. > > >If someone attempts to build non-ACPI kernel on older Hyper-V what is the > >behaviour of this function, do we need to safeguard or handle that case ? > The function won't panic if that's what you're asking about, i.e. > safe for runtime. That won't break the build either as it relies on > the SMCCC spec, and that uses the smc or hvc instructions (the code > does expect hvc to be the conduit and checks for that being the > case). The hypervisor doesn't inject the exception in the guest for > the unknown call, just returns SMCCC_RET_NOT_SUPPORTED in the first > output register (the hypervisor got a unit-test for that, too). Looks good, have you considered checking for SMCCC_RET_NOT_SUPPORTED ? - Saurabh
On 8/5/2024 8:46 AM, Saurabh Singh Sengar wrote: > On Mon, Aug 05, 2024 at 08:17:05AM -0700, Roman Kisel wrote: >> >> >> On 8/4/2024 8:53 PM, Saurabh Singh Sengar wrote: >>> On Fri, Jul 26, 2024 at 03:59:04PM -0700, Roman Kisel wrote: >>>> The arm64 Hyper-V startup path relies on ACPI to detect >>>> running under a Hyper-V compatible hypervisor. That >>>> doesn't work on non-ACPI systems. >>>> >>>> Hoist the ACPI detection logic into a separate function, >>>> use the new SMC added recently to Hyper-V to use in the >>>> non-ACPI case. >>>> >>>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> >>>> --- >>>> arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- >>>> arch/arm64/include/asm/mshyperv.h | 5 +++++ >>>> 2 files changed, 36 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c >>>> index b1a4de4eee29..341f98312667 100644 >>>> --- a/arch/arm64/hyperv/mshyperv.c >>>> +++ b/arch/arm64/hyperv/mshyperv.c >>>> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) >>>> return 0; >>>> } >>>> +static bool hyperv_detect_via_acpi(void) >>>> +{ >>>> + if (acpi_disabled) >>>> + return false; >>>> +#if IS_ENABLED(CONFIG_ACPI) >>>> + /* Hypervisor ID is only available in ACPI v6+. */ >>>> + if (acpi_gbl_FADT.header.revision < 6) >>>> + return false; >>>> + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; >>>> +#else >>>> + return false; >>>> +#endif >>>> +} >>>> + >>>> +static bool hyperv_detect_via_smc(void) >>>> +{ >>>> + struct arm_smccc_res res = {}; >>>> + >>>> + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) >>>> + return false; >>>> + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); >>>> + >>>> + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && >>>> + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && >>>> + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && >>>> + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; >>>> +} >>> >>> As you mentioned in the cover letter this is supported in latest Hyper-V hypervisor, >>> can we add a comment about it, specifying the exact version in it would be great. >>> >> I can add a comment about that, thought that would look as too much >> detail to refer to a version of the Windows insiders build in the >> comments in this code. Another option would be to entrench the logic >> in if statements which felt gross as there is a fallback. > > I'll leave the decision to your judgment. > >> >>> If someone attempts to build non-ACPI kernel on older Hyper-V what is the >>> behaviour of this function, do we need to safeguard or handle that case ? >> The function won't panic if that's what you're asking about, i.e. >> safe for runtime. That won't break the build either as it relies on >> the SMCCC spec, and that uses the smc or hvc instructions (the code >> does expect hvc to be the conduit and checks for that being the >> case). The hypervisor doesn't inject the exception in the guest for >> the unknown call, just returns SMCCC_RET_NOT_SUPPORTED in the first >> output register (the hypervisor got a unit-test for that, too). > > Looks good, have you considered checking for SMCCC_RET_NOT_SUPPORTED ? > No, I have not. Let me think out loud here... `a0` is compared to what must be return from the hypervisor the UID. That constant is an all-1 32 or 64 bit pattern, high unlikely to see that as a part of the UID due to low entropy as I understand. I might've added the check though for the better code readability, and because we have this e-mail thread going on, looks like I must :) Let me do that in v4, thanks! > - Saurabh
On 8/4/2024 8:01 PM, Michael Kelley wrote: > From: Roman Kisel <romank@linux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM >> >> The arm64 Hyper-V startup path relies on ACPI to detect >> running under a Hyper-V compatible hypervisor. That >> doesn't work on non-ACPI systems. >> >> Hoist the ACPI detection logic into a separate function, >> use the new SMC added recently to Hyper-V to use in the >> non-ACPI case. > > Wording seems slightly messed up. Perhaps: > > Hoist the ACPI detection logic into a separate function. Then > use the new SMC added recently to Hyper-V in the non-ACPI > case. > > Also, the phrase "the new SMC" seems a bit off to me. The "Terms and > Abbreviations" section of the SMCCC specification defines "SMC" as > an instruction: > > Secure Monitor Call. An Arm assembler instruction that causes an > exception that is taken synchronously into EL3. > > More precisely, I think you mean a SMC "function identifier" that is > newly implemented by Hyper-V. And the function identifier itself isn't > new; it's the Hyper-V implementation that's new. > > Similar comment applies in the cover letter for this patch set, and > perhaps to the Subject line of this patch. > Will fix the wording, appreciate your help with bringing this into the best shape possible! >> >> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> >> --- >> arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- >> arch/arm64/include/asm/mshyperv.h | 5 +++++ >> 2 files changed, 36 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c >> index b1a4de4eee29..341f98312667 100644 >> --- a/arch/arm64/hyperv/mshyperv.c >> +++ b/arch/arm64/hyperv/mshyperv.c >> @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) >> return 0; >> } >> >> +static bool hyperv_detect_via_acpi(void) >> +{ >> + if (acpi_disabled) >> + return false; >> +#if IS_ENABLED(CONFIG_ACPI) >> + /* Hypervisor ID is only available in ACPI v6+. */ >> + if (acpi_gbl_FADT.header.revision < 6) >> + return false; >> + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; >> +#else >> + return false; >> +#endif >> +} >> + >> +static bool hyperv_detect_via_smc(void) >> +{ >> + struct arm_smccc_res res = {}; >> + >> + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) >> + return false; >> + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); >> + >> + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && >> + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && >> + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && >> + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; >> +} >> + >> static int __init hyperv_init(void) >> { >> struct hv_get_vp_registers_output result; >> @@ -35,13 +63,11 @@ static int __init hyperv_init(void) >> >> /* >> * Allow for a kernel built with CONFIG_HYPERV to be running in >> - * a non-Hyper-V environment, including on DT instead of ACPI. >> + * a non-Hyper-V environment. >> + * >> * In such cases, do nothing and return success. >> */ >> - if (acpi_disabled) >> - return 0; >> - >> - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8)) >> + if (!hyperv_detect_via_acpi() && !hyperv_detect_via_smc()) >> return 0; >> >> /* Setup the guest ID */ >> diff --git a/arch/arm64/include/asm/mshyperv.h >> b/arch/arm64/include/asm/mshyperv.h >> index a975e1a689dd..a7a3586f7cb1 100644 >> --- a/arch/arm64/include/asm/mshyperv.h >> +++ b/arch/arm64/include/asm/mshyperv.h >> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) >> >> #include <asm-generic/mshyperv.h> >> >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 >> + > > Section 6.2 of the SMCCC specification says that the "Call UID Query" > returns a UUID. The above #defines look like an ASCII string is being > returned. Arguably the ASCII string can be treated as a set of 128 bits > just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V > be changed to return a real UUID? While the distinction probably > won't make a material difference here, we've had problems in the past > with Hyper-V doing slightly weird things that later caused unexpected > trouble. Please just get it right. :-) > The above values don't violate anything in the spec, and I think it would be hard to give an example of what can be broken in the world by using the above values. I do understand what you're saying when you mention the UIDs & the spirit of the spec. Put on the quantitative footing, the Shannon entropy of these values is much lower than that of an UID. A cursory search in the kernel tree does turn up other UIDs that don't look too random. As that is implemented only in the non-release versions, hardly someone has taken a dependency on the specific values in their production code. I guess that can be changed without causing any disturbance to the customers, will report of any concerns though. > Michael > >> #endif >> -- >> 2.34.1 >>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:51 AM [snip] > >> diff --git a/arch/arm64/include/asm/mshyperv.h > >> b/arch/arm64/include/asm/mshyperv.h > >> index a975e1a689dd..a7a3586f7cb1 100644 > >> --- a/arch/arm64/include/asm/mshyperv.h > >> +++ b/arch/arm64/include/asm/mshyperv.h > >> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) > >> > >> #include <asm-generic/mshyperv.h> > >> > >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d > >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 > >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 > >> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 > >> + > > > > Section 6.2 of the SMCCC specification says that the "Call UID Query" > > returns a UUID. The above #defines look like an ASCII string is being > > returned. Arguably the ASCII string can be treated as a set of 128 bits > > just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V > > be changed to return a real UUID? While the distinction probably > > won't make a material difference here, we've had problems in the past > > with Hyper-V doing slightly weird things that later caused unexpected > > trouble. Please just get it right. :-) > > > The above values don't violate anything in the spec, and I think it > would be hard to give an example of what can be broken in the world by > using the above values. Agreed. However, note that UUIDs *do* have some internal structure. See https://www.uuidtools.com/decode, for example. The UUID above is: 4d734879-7065-7256-0000-000000000000 It has a version digit of "7", which is "unknown". I'm no expert on UUIDs, but apparently the "7" isn't necessarily invalid, though perhaps a bit unexpected. > I do understand what you're saying when you > mention the UIDs & the spirit of the spec. Put on the quantitative > footing, the Shannon entropy of these values is much lower than that of > an UID. A cursory search in the kernel tree does turn up other UIDs that > don't look too random. > > As that is implemented only in the non-release versions, hardly someone > has taken a dependency on the specific values in their production code. > I guess that can be changed without causing any disturbance to the > customers, will report of any concerns though. My last thought is that when dealing with the open source world, and with published standards, it's usually best to do what's normal and expected, rather than trying to make the case that something unexpected is allowed by the spec. Doing the latter can come back to bite you in completely unexpected ways. :-) With that, I won't make any further comments on the topic. You do whatever you can do in working with Hyper-V. Either way, it won't be a blocker to my giving "Reviewed-by" on the next version of the patch. Michael
On 8/5/2024 1:30 PM, Michael Kelley wrote: > From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, August 5, 2024 9:51 AM > > [snip] > >>>> diff --git a/arch/arm64/include/asm/mshyperv.h >>>> b/arch/arm64/include/asm/mshyperv.h >>>> index a975e1a689dd..a7a3586f7cb1 100644 >>>> --- a/arch/arm64/include/asm/mshyperv.h >>>> +++ b/arch/arm64/include/asm/mshyperv.h >>>> @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) >>>> >>>> #include <asm-generic/mshyperv.h> >>>> >>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d >>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 >>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 >>>> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 >>>> + >>> >>> Section 6.2 of the SMCCC specification says that the "Call UID Query" >>> returns a UUID. The above #defines look like an ASCII string is being >>> returned. Arguably the ASCII string can be treated as a set of 128 bits >>> just like a UUID, but it doesn't meet the spirit of the spec. Can Hyper-V >>> be changed to return a real UUID? While the distinction probably >>> won't make a material difference here, we've had problems in the past >>> with Hyper-V doing slightly weird things that later caused unexpected >>> trouble. Please just get it right. :-) >>> >> The above values don't violate anything in the spec, and I think it >> would be hard to give an example of what can be broken in the world by >> using the above values. > > Agreed. However, note that UUIDs *do* have some internal structure. > See https://www.uuidtools.com/decode, for example. The UUID above > is: > > 4d734879-7065-7256-0000-000000000000 > > It has a version digit of "7", which is "unknown". I'm no expert on UUIDs, > but apparently the "7" isn't necessarily invalid, though perhaps a bit unexpected. > Understood! I made an assumption that's just an array of random bytes. Thank you very much for explaining that to me :) >> I do understand what you're saying when you >> mention the UIDs & the spirit of the spec. Put on the quantitative >> footing, the Shannon entropy of these values is much lower than that of >> an UID. A cursory search in the kernel tree does turn up other UIDs that >> don't look too random. >> >> As that is implemented only in the non-release versions, hardly someone >> has taken a dependency on the specific values in their production code. >> I guess that can be changed without causing any disturbance to the >> customers, will report of any concerns though. > > My last thought is that when dealing with the open source world, and > with published standards, it's usually best to do what's normal and > expected, rather than trying to make the case that something unexpected > is allowed by the spec. Doing the latter can come back to bite you in > completely unexpected ways. :-) > Agreed, thanks for the discussion! I've discussed changing the values, and a pull request to the hypervisor has been put up for that. So far, going well. > With that, I won't make any further comments on the topic. You do > whatever you can do in working with Hyper-V. Either way, it won't be > a blocker to my giving "Reviewed-by" on the next version of the > patch. > Will be a badge of honor to me! Again, appreciate your words of advise and caution, and helping in bringing these patches into the best shape possible! > Michael
diff --git a/arch/arm64/hyperv/mshyperv.c b/arch/arm64/hyperv/mshyperv.c index b1a4de4eee29..341f98312667 100644 --- a/arch/arm64/hyperv/mshyperv.c +++ b/arch/arm64/hyperv/mshyperv.c @@ -27,6 +27,34 @@ int hv_get_hypervisor_version(union hv_hypervisor_version_info *info) return 0; } +static bool hyperv_detect_via_acpi(void) +{ + if (acpi_disabled) + return false; +#if IS_ENABLED(CONFIG_ACPI) + /* Hypervisor ID is only available in ACPI v6+. */ + if (acpi_gbl_FADT.header.revision < 6) + return false; + return strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8) == 0; +#else + return false; +#endif +} + +static bool hyperv_detect_via_smc(void) +{ + struct arm_smccc_res res = {}; + + if (arm_smccc_1_1_get_conduit() != SMCCC_CONDUIT_HVC) + return false; + arm_smccc_1_1_hvc(ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID, &res); + + return res.a0 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 && + res.a1 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 && + res.a2 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 && + res.a3 == ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3; +} + static int __init hyperv_init(void) { struct hv_get_vp_registers_output result; @@ -35,13 +63,11 @@ static int __init hyperv_init(void) /* * Allow for a kernel built with CONFIG_HYPERV to be running in - * a non-Hyper-V environment, including on DT instead of ACPI. + * a non-Hyper-V environment. + * * In such cases, do nothing and return success. */ - if (acpi_disabled) - return 0; - - if (strncmp((char *)&acpi_gbl_FADT.hypervisor_id, "MsHyperV", 8)) + if (!hyperv_detect_via_acpi() && !hyperv_detect_via_smc()) return 0; /* Setup the guest ID */ diff --git a/arch/arm64/include/asm/mshyperv.h b/arch/arm64/include/asm/mshyperv.h index a975e1a689dd..a7a3586f7cb1 100644 --- a/arch/arm64/include/asm/mshyperv.h +++ b/arch/arm64/include/asm/mshyperv.h @@ -51,4 +51,9 @@ static inline u64 hv_get_msr(unsigned int reg) #include <asm-generic/mshyperv.h> +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_0 0x7948734d +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_1 0x56726570 +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_2 0 +#define ARM_SMCCC_VENDOR_HYP_UID_HYPERV_REG_3 0 + #endif
The arm64 Hyper-V startup path relies on ACPI to detect running under a Hyper-V compatible hypervisor. That doesn't work on non-ACPI systems. Hoist the ACPI detection logic into a separate function, use the new SMC added recently to Hyper-V to use in the non-ACPI case. Signed-off-by: Roman Kisel <romank@linux.microsoft.com> --- arch/arm64/hyperv/mshyperv.c | 36 ++++++++++++++++++++++++++----- arch/arm64/include/asm/mshyperv.h | 5 +++++ 2 files changed, 36 insertions(+), 5 deletions(-)