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

["Arnd Bergmann" <arnd@arndb.de>]

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