linux-kernel - Re: [PATCH 3/5] arm64: cacheinfo: Provide helper to compress MPIDR value into u32

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: <77874f70-a6e9-4388-b7ba-71bf8b44455b@arm.com>
Date: Fri, 27 Jun 2025 17:39:01 +0100
From: James Morse <james.morse@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J . Wysocki" <rafael@...nel.org>, sudeep.holla@....com,
 Rob Herring <robh@...nel.org>, Ben Horgan <ben.horgan@....com>
Subject: Re: [PATCH 3/5] arm64: cacheinfo: Provide helper to compress MPIDR
 value into u32

Hi Jonathan,

On 17/06/2025 17:14, Jonathan Cameron wrote:
> On Fri, 13 Jun 2025 13:03:54 +0000
> James Morse <james.morse@....com> wrote:
> 
>> Filesystems like resctrl use the cache-id exposed via sysfs to identify
>> groups of CPUs. The value is also used for PCIe cache steering tags. On
>> DT platforms cache-id is not something that is described in the
>> device-tree, but instead generated from the smallest MPIDR of the CPUs
>> associated with that cache. The cache-id exposed to user-space has
>> historically been 32 bits.
>>
>> MPIDR values may be larger than 32 bits.
>>
>> MPIDR only has 32 bits worth of affinity data, but the aff3 field lives
>> above 32bits. The corresponding lower bits are masked out by
>> MPIDR_HWID_BITMASK and contain an SMT flag and Uni-Processor flag.
>>
>> Swizzzle the aff3 field into the bottom 32 bits and using that.
>>
>> In case more affinity fields are added in the future, the upper RES0
>> area should be checked. Returning a value greater than 32 bits from
>> this helper will cause the caller to give up on allocating cache-ids.

> I'd mention that in the code via a comment, not just the commit message.

Sure!


>> Signed-off-by: James Morse <james.morse@....com>
> 
> Seems a few unrelated tiny things snuck in here.
> 
> Otherwise seems fine to me.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>

Thanks!


>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
>> index 99cd6546e72e..f8798dc96364 100644
>> --- a/arch/arm64/include/asm/cache.h
>> +++ b/arch/arm64/include/asm/cache.h
>> @@ -42,6 +42,7 @@
>>  
>>  #include <asm/cputype.h>
>>  #include <asm/mte-def.h>
>> +#include <asm/suspend.h>

> That seems a little random?  Why?

That's a hangover from a much earlier version that tried to use the MPIDR 'hash' that
cpu-suspend already creates. But the advice was to avoid that as if we find platforms with
wildly sparse MPIDR values, we'd need to make that 'hash' smarter to save memory - and
having an ABI hanging from it would be a hindrance.

Hence the swizzle.

>>  #include <asm/sysreg.h>
>>  
>>  #ifdef CONFIG_KASAN_SW_TAGS
>> @@ -87,6 +88,19 @@ int cache_line_size(void);
>>  
>>  #define dma_get_cache_alignment	cache_line_size
>>  
>> +/* Compress a u64 MPIDR value into 32 bits. */
>> +static inline u64 arch_compact_of_hwid(u64 id)
>> +{
>> +	u64 aff3 = MPIDR_AFFINITY_LEVEL(id, 3);
>> +
>> +	/* These bits are expected to be RES0 */
>> +	if (FIELD_GET(GENMASK_ULL(63, 40), id))
>> +		return id;
> 
> I would add a comment that the way this fails is to ensure
> there are bits in the upper bits.  It is a little unusual
> as APIs go but matches the not defined variant so sort of
> makes sense.

Yup, subtle enough that its worth a comment!

|	/*
|	 * These bits are expected to be RES0. If not, return a value with
|	 * the upper 32 bits set to force the caller to give up on 32 bit
|	 * cache ids.
|	 */


>> +
>> +	return (aff3 << 24) | FIELD_GET(GENMASK_ULL(23, 0), id);
>> +}
>> +#define arch_compact_of_hwid	arch_compact_of_hwid
>> +
>>  /*
>>   * Read the effective value of CTR_EL0.
>>   *
>> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
>> index f093cdf71be1..ebc23304d430 100644
>> --- a/arch/arm64/kernel/sleep.S
>> +++ b/arch/arm64/kernel/sleep.S
>> @@ -50,6 +50,7 @@
>>  	lsr	\mask ,\mask, \rs3
>>  	orr	\dst, \dst, \mask		// dst|=(aff3>>rs3)
>>  	.endm
>> +
> 
> Stray change.

This is a left over from trying to use the above 'hash' directly.

All fixed,


Thanks!

James