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

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

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

-- 
Thank you,
Roman