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
