| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d168a483-b425-4c18-ad71-eb39a0aba06c@linux.microsoft.com>
Date: Mon, 17 Mar 2025 10:11:32 -0700
From: Roman Kisel <romank@...ux.microsoft.com>
To: Mark Rutland <mark.rutland@....com>
Cc: arnd@...db.de, bhelgaas@...gle.com, bp@...en8.de,
catalin.marinas@....com, conor+dt@...nel.org, dan.carpenter@...aro.org,
dave.hansen@...ux.intel.com, decui@...rosoft.com, haiyangz@...rosoft.com,
hpa@...or.com, joey.gouly@....com, krzk+dt@...nel.org, kw@...ux.com,
kys@...rosoft.com, lenb@...nel.org, lpieralisi@...nel.org,
manivannan.sadhasivam@...aro.org, maz@...nel.org, mingo@...hat.com,
oliver.upton@...ux.dev, rafael@...nel.org, robh@...nel.org,
ssengar@...ux.microsoft.com, sudeep.holla@....com, suzuki.poulose@....com,
tglx@...utronix.de, wei.liu@...nel.org, will@...nel.org,
yuzenghui@...wei.com, devicetree@...r.kernel.org, kvmarm@...ts.linux.dev,
linux-acpi@...r.kernel.org, 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,
apais@...rosoft.com, benhill@...rosoft.com, bperkins@...rosoft.com,
sunilmut@...rosoft.com
Subject: Re: [PATCH hyperv-next v6 01/11] arm64: kvm, smccc: Introduce and use
API for detecting hypervisor presence
On 3/17/2025 4:29 AM, Mark Rutland wrote:
> On Fri, Mar 14, 2025 at 05:19:21PM -0700, Roman Kisel wrote:
[...]
>> +}
>
> This use of a statement expression is bizarre, and the function would be
> clearer without it, e.g.
I'll change that to what you're suggesting, thanks for your help!
>
> | bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
> | {
> | struct arm_smccc_res res = {};
> | uuid_t uuid;
> |
> | 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;
> |
> | uuid_t = SMCCC_RES_TO_UUID(res.a0, res.a1, res.a2, res.a3);
> | return uuid_equal(&uuid, hyp_uuid);
> | }
>
> As noted below, I'd prefer if this were renamed to something like
> arm_smccc_hypervisor_has_uuid(), to more clearly indicate what is being
> checked.
>
> [...]
Will update, thanks for your help!
>
>> +/**
>> + * arm_smccc_hyp_present(const uuid_t *hyp_uuid)
>> + *
>> + * Returns `true` if the hypervisor advertises its presence via SMCCC.
>> + *
>> + * When the function returns `false`, the caller shall not assume that
>> + * there is no hypervisor running. Instead, the caller must fall back to
>> + * other approaches if any are available.
>> + */
>> +bool arm_smccc_hyp_present(const uuid_t *hyp_uuid);
>
> I'd prefer if this were:
>
> | /*
> | * Returns whether a specific hypervisor UUID is advertised for the
> | * Vendor Specific Hypervisor Service range.
> | */
> | bool arm_smccc_hypervisor_has_uuid(const uuid_t *uuid);
>
[...]
>
> I think this'd be clearer if we did something similar to what we did for
> the SMCCC SOC_ID name:
>
> https://lore.kernel.org/linux-arm-kernel/20250219005932.3466-1-paul@os.amperecomputing.com/
>
> ... and pack/unpack the bytes explicitly, e.g.
That looks great, thanks for the suggestion!
>
> | static inline uuid smccc_res_to_uuid(u32 r0, u32, r1, u32 r2, u32 r3)
> | {
> | uuid_t uuid = {
> | .b = {
> | [0] = (r0 >> 0) & 0xff,
> | [1] = (r0 >> 8) & 0xff,
> | [2] = (r0 >> 16) & 0xff,
> | [3] = (r0 >> 24) & 0xff,
> |
> | [4] = (r1 >> 0) & 0xff,
> | [5] = (r1 >> 8) & 0xff,
> | [6] = (r1 >> 16) & 0xff,
> | [7] = (r1 >> 24) & 0xff,
> |
> | [8] = (r2 >> 0) & 0xff,
> | [9] = (r2 >> 8) & 0xff,
> | [10] = (r2 >> 16) & 0xff,
> | [11] = (r2 >> 24) & 0xff,
> |
> | [12] = (r3 >> 0) & 0xff,
> | [13] = (r3 >> 8) & 0xff,
> | [14] = (r3 >> 16) & 0xff,
> | [15] = (r3 >> 24) & 0xff,
> | },
> | };
> |
> | return uuid;
> | }
>
> ... which is a bit more verbose, but clearly aligns with what the SMCCC
> spec says w.r.t. packing/unpacking, and should avoid warnings about
> endianness conversions.
>
I believe what you're proposing hits a better trade-off, thanks again!
>> +
>> +#define UUID_TO_SMCCC_RES(uuid_init, regs) do { \
>> + const uuid_t uuid = uuid_init; \
>> + (regs)[0] = le32_to_cpu((u32)uuid.b[0] | (uuid.b[1] << 8) | \
>> + ((uuid.b[2]) << 16) | ((uuid.b[3]) << 24)); \
>> + (regs)[1] = le32_to_cpu((u32)uuid.b[4] | (uuid.b[5] << 8) | \
>> + ((uuid.b[6]) << 16) | ((uuid.b[7]) << 24)); \
>> + (regs)[2] = le32_to_cpu((u32)uuid.b[8] | (uuid.b[9] << 8) | \
>> + ((uuid.b[10]) << 16) | ((uuid.b[11]) << 24)); \
>> + (regs)[3] = le32_to_cpu((u32)uuid.b[12] | (uuid.b[13] << 8) | \
>> + ((uuid.b[14]) << 16) | ((uuid.b[15]) << 24)); \
>> + } while (0)
>> +
>> +#endif /* !__ASSEMBLER__ */
>
> IMO it'd be clearer to initialise a uuid_t beforehand, and then allow
> the helper to unpack the bytes, e.g.
>
> static inline u32 smccc_uuid_to_reg(const uuid_t uuid, int reg)
> {
> u32 val = 0;
>
> val |= (u32)(uuid.b[4 * reg + 0] << 0)
> val |= (u32)(uuid.b[4 * reg + 1] << 8)
> val |= (u32)(uuid.b[4 * reg + 2] << 16)
> val |= (u32)(uuid.b[4 * reg + 3] << 24)
>
> return val:
> }
>
> #define UUID_TO_SMCCC_RES(uuid, regs) \
> do { \
> (regs)[0] = smccc_uuid_to_reg(uuid, 0); \
> (regs)[1] = smccc_uuid_to_reg(uuid, 1); \
> (regs)[2] = smccc_uuid_to_reg(uuid, 2); \
> (regs)[3] = smccc_uuid_to_reg(uuid, 3); \
> } while (0)
>
> ... though arguably at that point you can get rid of the
> UUID_TO_SMCCC_RES() macro and just expand that directly at the callsite.
>
I'll work on that, thanks!!
> Mark.
--
Thank you,
Roman
Powered by blists - more mailing lists