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

[Roman Kisel <romank@linux.microsoft.com>]

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

-- 
Thank you,
Roman