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

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

On 2/24/2025 11:24 PM, Arnd Bergmann wrote:
> On Tue, Feb 25, 2025, at 00:22, Roman Kisel wrote:
>> Hi Arnd,

[...]

> 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.

I'd gladly stick to that provided I have your support of touching
KVM's code! As the SMCCC document states, there shall be an UUID,
and in the kernel, there would be

#define ARM_SMCCC_VENDOR_KVM_UID UUID_INIT(.......)
#define ARM_SMCCC_VENDOR_HYP_UID UUID_INIT(.......)

Hence, the ARM_SMCCC_VENDOR_HYP_UID_*_REG_{0,1,2,3} can be removed as
you're suggesting.

That looks enticing enough semantically as though we're building layers
from the SMCCC spec down to the "on-wire format" -- the only part that
needs "deserializing" the UUID from `struct arm_smccc_res` the
hypervisor returns.

To add to that, anyone who wishes to implement a hypervisor for arm64
will have to use some RFC 9562-compliant UUID generating facility. Thus,
the UUID predates these 4 dwords. Using UUIDs in the kernel code will
relieve of the chore of figuring out the 4 dwords from the UUID.

Also, for the Gunyah folks will be pretty easy to use this infra:
define the UUID in the header (1 line), call the new function (1 line),
done.

> 
> 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.

IMO, that approach provides a simplicity where anyone can see if the
code is wrong from a quick glance: just compare 4 dwords. The fact that
the 4 dwords form an UUID is bypassed though (as it is in the existing
code). Somehow feels not spec-accurate imo. Also when I remove the UID
part from the names, I'm going to have a rather weak justification as
to why this is a benefit.

Likely, there are two levels of improvement here:

1. Just refactor the common parts out and have
    `bool arm_smccc_hyp_present(u32 reg0, u32 reg1, u32 reg2, u32 reg2);`

2. Introduce the UUID usage throughout and have a spec-accurate
    prototype of
    `bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);`

and would be great to go for the second one :)

> 
> 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.

For hyperv case, `uuidgen` produced the UUID and that is used.
Likely the same for kvm.

> 
>       Arnd

-- 
Thank you,
Roman