Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect hypervisor presence

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

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

-- 
Thank you,
Roman