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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e4685fe-68e9-43bd-96c5-b871edb1b971@linux.microsoft.com>
Date: Fri, 14 Feb 2025 08:47:32 -0800
From: Roman Kisel <romank@...ux.microsoft.com>
To: Arnd Bergmann <arnd@...db.de>, 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
Cc: benhill@...rosoft.com, bperkins@...rosoft.com, sunilmut@...rosoft.com
Subject: Re: [PATCH hyperv-next v4 1/6] arm64: hyperv: Use SMCCC to detect
 hypervisor presence

On 2/14/2025 12:05 AM, Arnd Bergmann wrote:
> On Fri, Feb 14, 2025, at 00:23, Roman Kisel wrote:
>> On 2/11/2025 10:54 PM, Arnd Bergmann wrote:
> 
>> index a74600d9f2d7..86f75f44895f 100644
>> --- a/drivers/firmware/smccc/smccc.c
>> +++ b/drivers/firmware/smccc/smccc.c
>> @@ -67,6 +67,30 @@ s32 arm_smccc_get_soc_id_revision(void)
>>    }
>>    EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
>>
>> +bool arm_smccc_hyp_present(const uuid_t *hyp_uuid)
> 
> The interface looks good to me.

Great :)

> 
>> +{
>> +	struct arm_smccc_res res = {};
>> +	struct {
>> +		u32 dwords[4]
>> +	} __packed res_uuid;
> 
> The structure definition here looks odd because of the
> unexplained __packed attribute and the nonstandard byteorder.
> 

Fair points, thank you, will straighten this out!

> The normal uuid_t is defined as an array of 16 bytes,
> so if you try to represent it in 32-bit words you need to
> decide between __le32 and __be32 representation.
> 
>> +	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;
>> +
>> +	res_uuid.dwords[0] = res.a0;
>> +	res_uuid.dwords[1] = res.a1;
>> +	res_uuid.dwords[2] = res.a2;
>> +	res_uuid.dwords[3] = res.a3;
>> +
>> +	return uuid_equal((uuid_t *)&res_uuid, hyp_uuid);
> 
> The SMCCC standard defines the four words to be little-endian,
> so in order to compare them against a uuid byte array, you'd
> need to declare the array as __le32 and swap the result
> members with cpu_to_le32().
> 
> Alternatively you could pass the four u32 values into the
> function in place of the uuid.
> 
> Since the callers have the same endianess confusion, your
> implementation ends up working correctly even on big-endian,
> but I find it harder to follow when you call uuid_equal() on
> something that is not the actual uuid_t value.
> 

I'll make sure the implementation is clearer, thanks!

>> +
>> +#define ARM_SMCCC_HYP_PRESENT(HYP) 								\
>> +	({															\
>> +		const u32 uuid_as_dwords[4] = {							\
>> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_0,			\
>> +			ARM_SMCCC_VENDOR_HYP_UID_ ## HYP ## _REG_1,			\
> 
> I don't think using a macro is helpful here, it just makes
> it impossible to grep for ARM_SMCCC_VENDOR_HYP_UID_* values when
> reading the source.
> 
> 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).

Will remove the macro and will use UUID_INIT, appreciate taking the
time to review the draft and your suggestions on improving it very much!

> 
>       Arnd

-- 
Thank you,
Roman