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

[James Morse <james.morse@arm.com>]

Hi Fenghua,

On 02/10/2025 06:03, Fenghua Yu wrote:
> On 9/10/25 13:42, 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.

>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index c5f2a51d280b..c379a9952b00 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -966,3 +966,62 @@ int find_acpi_cache_level_from_id(u32 cache_id)
>> +/**
>> + * acpi_pptt_get_cpumask_from_cache_id() - Get the cpus associated with the
>> + *                       specified cache
>> + * @cache_id: The id field of the unified 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)
>> +{
>> +    u32 acpi_cpu_id;
>> +    int level, cpu, num_levels;
>> +    struct acpi_pptt_cache *cache;
>> +    struct acpi_pptt_cache_v1 *cache_v1;
>> +    struct acpi_pptt_processor *cpu_node;
>> +    struct acpi_table_header *table __free(acpi_table) =
>> acpi_get_table_ret(ACPI_SIG_PPTT, 0);
>> +
>> +    cpumask_clear(cpus);
>> +
>> +    if (IS_ERR(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 (WARN_ON_ONCE(!cpu_node))
>> +            continue;
>> +        num_levels = acpi_count_levels(table, cpu_node, NULL);
>> +
>> +        /* Start at 1 for L1 */
>> +        for (level = 1; level <= num_levels; level++) {
>> +            cache = acpi_find_cache_node(table, acpi_cpu_id,
>> +                             ACPI_PPTT_CACHE_TYPE_UNIFIED,
>> +                             level, &cpu_node);
>> +            if (!cache)
>> +                continue;
>> +
>> +            cache_v1 = ACPI_ADD_PTR(struct acpi_pptt_cache_v1,
>> +                        cache,
>> +                        sizeof(struct acpi_pptt_cache));
>> +
>> +            if (cache->flags & ACPI_PPTT_CACHE_ID_VALID &&
>> +                cache_v1->cache_id == cache_id)
>> +                cpumask_set_cpu(cpu, cpus);
> 
> This function is almost identical to find_acpi_cache_level_from_id() defined in patch #3.

Yes - there is already a lot of repetition in this file.

I'd previously suggested to Jeremy L to ahve a walker with callbacks, but he felt that
made it harder to read.
Jonathan suggested a for_each_acpi_pptt_entry() helper:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=pptt/for_each_pptt_entry/v0&id=353ceeba3d39c6b6a10eeb1a59c49649cdf719d8

I'm avoiding including that here as its ~30 patches already!


> To reduce code size and complexity, it's better to define a common helper to server both
> of the two functions.
> 
> e.g. define a helper acpi_pptt_get_level_cpumask_from_cache_id(u32 cache_id, int *lvl,
> cpu_mask_t *cpus)
> 
> This helper has the same code body to traverse the cache levels for all CPUs as
> find_acpi_cache_level_from_id() and acpi_pptt_get_cpumask_from_cache_id(). The major
> difference is here:
> 
> +            if (cache->flags & ACPI_PPTT_CACHE_ID_VALID &&
> +                cache_v1->cache_id == cache_id) {
> +                if (cpus)
> +                    cpumask_set_cpu(cpu, cpus);
> +                if ((level) {
> +                    *lvl = level;
> +                    return 0;
> +                }
> 
> Then simplify the two callers as follows:
> int find_acpi_cache_level_from_id(u32 cache_id)
> {
>     int level;
>     int err = acpi_pptt_get_level_cpumask_from_cache_id(cache_id, &level, NULL);
>     if (err)
>         return err;
> 
>     return level;
> }
> 
> int acpi_pptt_get_cpumask_from_cache_id(u32 cache_id, cpumask_t *cpus)
> {
>     return acpi_pptt_get_level_cpumask_from_cache_id(cache_id, NULL, cpus);
> }
> 

You've combined two functions that both walk the table (there are quiet a few more in this
file) - but they look for very different things. Your common helper is going to be much
more complex than either of these standalone.

I think Jonathan's for-each helper is the best path forward, that reduces the boiler plate
leaving the relevant differences.



Thanks,

James