Re: [PATCH v3 04/29] ACPI / PPTT: Add a helper to fill a cpumask from a cache_id

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

On Wed, 22 Oct 2025 07:58:36 -0500
Jeremy Linton <jeremy.linton@arm.com> wrote:

> Hi,
> 
> This is largely looking pretty solid, but..
> 
> 
> On 10/17/25 1:56 PM, James Morse wrote:
> > MPAM identifies CPUs by the cache_id in the PPTT cache structure.
> > 
> > The driver needs to know which CPUs are associated with the cache.
> > The CPUs may not all be online, so cacheinfo does not have the
> > information.
> > 
> > Add a helper to pull this information out of the PPTT.
> > 
> > CC: Rohit Mathew <Rohit.Mathew@arm.com>
> > Signed-off-by: James Morse <james.morse@arm.com>
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > Tested-by: Fenghua Yu <fenghuay@nvidia.com>
> > ---
> > Changes since v2:
> >   * Removed stray cleanup useage in preference for acpi_get_pptt().
> >   * Removed WARN_ON_ONCE() for symmetry with other helpers.
> >   * Dropped restriction on unified caches.
> > 
> > Changes since v1:
> >   * Added punctuation to the commit message.
> >   * Removed a comment about an alternative implementaion.
> >   * Made the loop continue with a warning if a CPU is missing from the PPTT.
> > 
> > Changes since RFC:
> >   * acpi_count_levels() now returns a value.
> >   * Converted the table-get stuff to use Jonathan's cleanup helper.
> >   * Dropped Sudeep's Review tag due to the cleanup change.
> > ---
> >   drivers/acpi/pptt.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/acpi.h |  6 +++++
> >   2 files changed, 70 insertions(+)
> > 
> > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> > index 50c8f2a3c927..2f86f58699a6 100644
> > --- a/drivers/acpi/pptt.c
> > +++ b/drivers/acpi/pptt.c
> > @@ -985,3 +985,67 @@ int find_acpi_cache_level_from_id(u32 cache_id)
> >   
> >   	return -ENOENT;
> >   }
> > +
> > +/**
> > + * acpi_pptt_get_cpumask_from_cache_id() - Get the cpus associated with the
> > + *					   specified cache
> > + * @cache_id: The id field of the cache
> > + * @cpus: Where to build the cpumask
> > + *
> > + * Determine which CPUs are below this cache in the PPTT. This allows the property
> > + * to be found even if the CPUs are offline.
> > + *
> > + * The PPTT table must be rev 3 or later,
> > + *
> > + * Return: -ENOENT if the PPTT doesn't exist, or the cache cannot be found.
> > + * Otherwise returns 0 and sets the cpus in the provided cpumask.
> > + */
> > +int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id, cpumask_t *cpus)
> > +{
> > +	int level, cpu;
> > +	u32 acpi_cpu_id;
> > +	struct acpi_pptt_cache *cache;
> > +	struct acpi_table_header *table;
> > +	struct acpi_pptt_cache_v1 *cache_v1;
> > +	struct acpi_pptt_processor *cpu_node;
> > +
> > +	cpumask_clear(cpus);
> > +
> > +	table = acpi_get_pptt();
> > +	if (!table)
> > +		return -ENOENT;
> > +
> > +	if (table->revision < 3)
> > +		return -ENOENT;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> > +		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
> > +		if (!cpu_node)
> > +			continue;
> > +
> > +		/* Start at 1 for L1 */
> > +		level = 1;
> > +		cache = acpi_find_any_type_cache_node(table, acpi_cpu_id, level,
> > +						      &cpu_node);
> > +		while (cache) {
> > +			cache_v1 = ACPI_ADD_PTR(struct acpi_pptt_cache_v1,
> > +						cache, sizeof(*cache));  
> 
> Is the core acpi definition in actbl2.h correct? Shouldn't it be 
> something along the lines of:
> 
> struct acpi_pptt_cache_v1 {
>   struct acpi_subtable_header header;
>   u16 reservedd;
>   u32 flags;
>   u32 next_level_of_cache;
>   u32 size;
>   u32 number_of_sets;
>   u8 associativity;
>   u8 attributes;
>   u16 lines_size;
>   u32 cache_id;
> }
> 
> 
> Then that solves the detection of the additional field problem correctly 
> because the length (24 vs 28) of the subtable then tells you which 
> version your dealing with. (and goes back to why much of this is coded 
> to use ACPI_ADD_PTR rather than structure+ logic.)
> 

Do we want to deal with arguing the change in ACPICA? 
I fully agree that it would be much nicer if that didn't use this weird
bits of structures approach :(  

https://github.com/acpica/acpica/blob/master/source/include/actbl2.h#L3497
is where this is coming from.

Maybe can do it in parallel. Rafael, what do you think is best way forwards
with this?

Jonathan

> 
> Thanks,
> 
> 
> 
> 
> 
> 
> > +			if (!cache)
> > +				continue;
> > +
> > +			cache_v1 = ACPI_ADD_PTR(struct acpi_pptt_cache_v1,
> > +						cache, sizeof(*cache));
> > +
> > +			if (cache->flags & ACPI_PPTT_CACHE_ID_VALID &&
> > +			    cache_v1->cache_id == cache_id)
> > +				cpumask_set_cpu(cpu, cpus);
> > +
> > +			level++;
> > +			cache = acpi_find_any_type_cache_node(table, acpi_cpu_id,
> > +							      level, &cpu_node);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index be074bdfd4d1..a9dbacabdf89 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1543,6 +1543,7 @@ int find_acpi_cpu_topology_package(unsigned int cpu);
> >   int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
> >   void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus);
> >   int find_acpi_cache_level_from_id(u32 cache_id);
> > +int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id, cpumask_t *cpus);
> >   #else
> >   static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
> >   {
> > @@ -1570,6 +1571,11 @@ static inline int find_acpi_cache_level_from_id(u32 cache_id)
> >   {
> >   	return -ENOENT;
> >   }
> > +static inline int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id,
> > +						      cpumask_t *cpus)
> > +{
> > +	return -ENOENT;
> > +}
> >   #endif
> >   
> >   void acpi_arch_init(void);  
> 
>