[<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
Powered by blists - more mailing lists