Re: [PATCH 04/33] ACPI / PPTT: Find cache level by cache-id

[Ben Horgan <ben.horgan@arm.com>]

Hi Jonathan,

On 11/10/25 16:02, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:21 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
> 
>> From: James Morse <james.morse@arm.com>
>>
>> The MPAM table identifies caches by id. The MPAM driver also wants to know
>> the cache level to determine if the platform is of the shape that can be
>> managed via resctrl. Cacheinfo has this information, but only for CPUs that
>> are online.
>>
>> Waiting for all CPUs to come online is a problem for platforms where
>> CPUs are brought online late by user-space.
>>
>> Add a helper that walks every possible cache, until it finds the one
>> identified by cache-id, then return the level.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> 
> A few things inline.
> 
>> ---
>> Changes since v3:
>> Tags dropped due to rework
>> Fallout/simplification from adding acpi_pptt_cache_v1_full
>> Look for each cache type before incrementing level
>> ---
>>  drivers/acpi/pptt.c  | 63 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/acpi.h |  5 ++++
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 1ed2099c0d1a..71841c106020 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -918,3 +918,66 @@ void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus)
>>  				     entry->length);
>>  	}
>>  }
>> +
>> +/**
>> + * find_acpi_cache_level_from_id() - Get the level of the specified cache
>> + * @cache_id: The id field of the cache
>> + *
>> + * Determine the level relative to any CPU for the cache identified by
>> + * cache_id. This allows the property to be found even if the CPUs are offline.
>> + *
>> + * The returned level can be used to group caches that are peers.
>> + *
>> + * The PPTT table must be rev 3 or later.
>> + *
>> + * If one CPU's L2 is shared with another CPU as L3, this function will return
>> + * an unpredictable value.
>> + *
>> + * Return: -ENOENT if the PPTT doesn't exist, the revision isn't supported or
>> + * the cache cannot be found.
>> + * Otherwise returns a value which represents the level of the specified cache.
>> + */
>> +int find_acpi_cache_level_from_id(u32 cache_id)
>> +{
>> +	int cpu;
>> +	struct acpi_table_header *table;
>> +
>> +	table = acpi_get_pptt();
>> +	if (!table)
>> +		return -ENOENT;
>> +
>> +	if (table->revision < 3)
>> +		return -ENOENT;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		bool not_empty = true;
>> +		u32 acpi_cpu_id;
>> +		struct acpi_pptt_cache_v1_full *cache;
>> +		struct acpi_pptt_processor *cpu_node;
>> +
>> +		acpi_cpu_id = get_acpi_id_for_cpu(cpu);
> 
> Might as well combine this one with declaration.

Will do.

> 
>> +		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>> +		if (!cpu_node)
>> +			continue;
>> +
>> +		for (int level = 1; not_empty; level++) {
> 
> This smells very much like a while loop rather than a for loop. Make
> it a do/while and you can avoid the somewhat nasty setting not_empty = true
> just to get in for first iteration.
> 
> 		int level = 1;
> 		do {
> 			int cache_type[] = { CACHE_TYPE_INST, CACHE_TYPE_DATA, CACHE_TYPE_UNIFIED };
> 
> 			not_empty = false;
> 			for (int i = 0; i < ARRAY_SIZE(cache_type); i++) {
> 				cache = acpi_find_cache_node(table, acpi_cpu_id, cache_type[i],
> 							     level, &cpu_node);
> 				if (!cache)
> 					continue;
> 
> 				not_empty = true;
> 
> 				if (acpi_pptt_cache_id_is_valid(cache) &&
> 				    cache->extra.cache_id == cache_id)
> 					return level;
> 			}
> 		} while (not_empty);
> 
> Maybe flip sense of that bool to be empty and !empty for the test.

Yes, this is better. I'll stop abusing the for loop in this patch and
the next. Changing not_empty to !empty make sense too.

> 
> 
>> +			int cache_type[] = {CACHE_TYPE_INST, CACHE_TYPE_DATA, CACHE_TYPE_UNIFIED};
>> +
>> +			not_empty = false;
>> +			for (int i = 0; i < ARRAY_SIZE(cache_type); i++) {
>> +				cache = acpi_find_cache_node(table, acpi_cpu_id, cache_type[i],
>> +							     level, &cpu_node);
>> +				if (!cache)
>> +					continue;
>> +
>> +				not_empty = true;
>> +
>> +				if (acpi_pptt_cache_id_is_valid(cache) &&
>> +				    cache->extra.cache_id == cache_id)
>> +					return level;
>> +			}
>> +		}
>> +	}
>> +
>> +	return -ENOENT;
>> +}
> 
> 

Thanks,

Ben