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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 13 Jun 2025 13:03:54 +0000
James Morse <james.morse@arm.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.
Hi James,

I'd mention that in the code via a comment, not just the commit message.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

Seems a few unrelated tiny things snuck in here.

Otherwise seems fine to me.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  arch/arm64/include/asm/cache.h | 14 ++++++++++++++
>  arch/arm64/kernel/sleep.S      |  1 +
>  2 files changed, 15 insertions(+)
> 
> 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?
>  #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.

> +
> +	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.

>  /*
>   * Save CPU state in the provided sleep_stack_data area, and publish its
>   * location for cpu_resume()'s use in sleep_save_stash.