Message-ID: <a96f9469-a22e-43e7-825d-f67ef550898f@linux.microsoft.com>
Date: Tue, 25 Feb 2025 14:25:24 -0800
From: Roman Kisel <romank@...ux.microsoft.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: benhill@...rosoft.com, bperkins@...rosoft.com, sunilmut@...rosoft.com,
 bhelgaas@...gle.com, Borislav Petkov <bp@...en8.de>,
 Catalin Marinas <catalin.marinas@....com>, Conor Dooley
 <conor+dt@...nel.org>, Dave Hansen <dave.hansen@...ux.intel.com>,
 Dexuan Cui <decui@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>,
 "H. Peter Anvin" <hpa@...or.com>, krzk+dt@...nel.org,
 Krzysztof Wilczyński <kw@...ux.com>,
 "K. Y. Srinivasan" <kys@...rosoft.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Ingo Molnar <mingo@...hat.com>, Rob Herring <robh@...nel.org>,
 ssengar@...ux.microsoft.com, Thomas Gleixner <tglx@...utronix.de>,
 Wei Liu <wei.liu@...nel.org>, Will Deacon <will@...nel.org>,
 devicetree@...r.kernel.org, Linux-Arch <linux-arch@...r.kernel.org>,
 linux-arm-kernel@...ts.infradead.org, linux-hyperv@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect
 hypervisor presence



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

Powered by blists - more mailing lists