Re: [PATCH v5 01/34] ACPI / PPTT: Add a helper to fill a cpumask from a processor container

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

Hi Gavin,

On 11/19/25 03:35, Gavin Shan wrote:
> Hi Ben,
> 
> On 11/18/25 2:59 AM, Ben Horgan wrote:
>> From: James Morse <james.morse@arm.com>
>>
>> The ACPI MPAM table uses the UID of a processor container specified in
>> the PPTT to indicate the subset of CPUs and cache topology that can
>> access each MPAM System Component (MSC).
>>
>> This information is not directly useful to the kernel. The equivalent
>> cpumask is needed instead.
>>
>> Add a helper to find the processor container by its id, then walk
>> the possible CPUs to fill a cpumask with the CPUs that have this
>> processor container as a parent.
>>
>> CC: Dave Martin <dave.martin@arm.com>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> Tested-by: Fenghua Yu <fenghuay@nvidia.com>
>> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> Tested-by: Peter Newman <peternewman@google.com>
>> Tested-by: Carl Worth <carl@os.amperecomputing.com>
>> Tested-by: Gavin Shan <gshan@redhat.com>
>> Tested-by: Zeng Heng <zengheng4@huawei.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Changes since v4:
>> Remove blank line
>>
>> Changes since v3:
>> Refer to processor hierarchy in comments (Jonathan)
>> Fix indent (Jonathan)
>> ---
>>   drivers/acpi/pptt.c  | 84 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/acpi.h |  3 ++
>>   2 files changed, 87 insertions(+)
>>
> 
> Since this series may need another respin, a nitpick below...
> 
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 54676e3d82dd..b8248c0092fe 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -817,3 +817,87 @@ int find_acpi_cpu_topology_hetero_id(unsigned int
>> cpu)
>>       return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
>>                         ACPI_PPTT_ACPI_IDENTICAL);
>>   }
>> +
>> +/**
>> + * acpi_pptt_get_child_cpus() - Find all the CPUs below a PPTT
>> + * processor hierarchy node
>> + *
>> + * @table_hdr:        A reference to the PPTT table
>> + * @parent_node:    A pointer to the processor hierarchy node in the
>> + *            table_hdr
>> + * @cpus:        A cpumask to fill with the CPUs below @parent_node
>> + *
>> + * Walks up the PPTT from every possible CPU to find if the provided
>> + * @parent_node is a parent of this CPU.
>> + */
>> +static void acpi_pptt_get_child_cpus(struct acpi_table_header
>> *table_hdr,
>> +                     struct acpi_pptt_processor *parent_node,
>> +                     cpumask_t *cpus)
>> +{
>> +    struct acpi_pptt_processor *cpu_node;
>> +    u32 acpi_id;
>> +    int cpu;
>> +
>> +    cpumask_clear(cpus);
>> +
> 
> The CPU mask has been cleared in its only caller
> acpi_pptt_get_cpus_from_container(),
> no need to clear it again.

I'd rather not change this as it adds extra constraints on the caller
which aren't necessarily expected.

> 
>> +    for_each_possible_cpu(cpu) {
>> +        acpi_id = get_acpi_id_for_cpu(cpu);
>> +        cpu_node = acpi_find_processor_node(table_hdr, acpi_id);
>> +
>> +        while (cpu_node) {
>> +            if (cpu_node == parent_node) {
>> +                cpumask_set_cpu(cpu, cpus);
>> +                break;
>> +            }
>> +            cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
>> +        }
>> +    }
>> +}
>> +
>> +/**
>> + * acpi_pptt_get_cpus_from_container() - Populate a cpumask with all
>> CPUs in a
>> + *                                       processor container
>> + * @acpi_cpu_id:    The UID of the processor container
>> + * @cpus:        The resulting CPU mask
>> + *
>> + * Find the specified Processor Container, and fill @cpus with all
>> the cpus
>> + * below it.
>> + *
>> + * Not all 'Processor Hierarchy' entries in the PPTT are either a CPU
>> + * or a Processor Container, they may exist purely to describe a
>> + * Private resource. CPUs have to be leaves, so a Processor Container
>> + * is a non-leaf that has the 'ACPI Processor ID valid' flag set.
>> + */
>> +void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus)
>> +{
>> +    struct acpi_table_header *table_hdr;
>> +    struct acpi_subtable_header *entry;
>> +    unsigned long table_end;
>> +    u32 proc_sz;
>> +
>> +    cpumask_clear(cpus);
>> +
>> +    table_hdr = acpi_get_pptt();
>> +    if (!table_hdr)
>> +        return;
>> +
>> +    table_end = (unsigned long)table_hdr + table_hdr->length;
>> +    entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>> +                 sizeof(struct acpi_table_pptt));
>> +    proc_sz = sizeof(struct acpi_pptt_processor);
>> +    while ((unsigned long)entry + proc_sz <= table_end) {
>> +        if (entry->type == ACPI_PPTT_TYPE_PROCESSOR) {
>> +            struct acpi_pptt_processor *cpu_node;
>> +
>> +            cpu_node = (struct acpi_pptt_processor *)entry;
>> +            if (cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID &&
>> +                !acpi_pptt_leaf_node(table_hdr, cpu_node) &&
>> +                cpu_node->acpi_processor_id == acpi_cpu_id) {
>> +                acpi_pptt_get_child_cpus(table_hdr, cpu_node, cpus);
>> +                break;
>> +            }
>> +        }
>> +        entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
>> +                     entry->length);
>> +    }
>> +}
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 5ff5d99f6ead..4752ebd48132 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1541,6 +1541,7 @@ int find_acpi_cpu_topology(unsigned int cpu, int
>> level);
>>   int find_acpi_cpu_topology_cluster(unsigned int cpu);
>>   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);
>>   #else
>>   static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
>>   {
>> @@ -1562,6 +1563,8 @@ static inline int
>> find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>>   {
>>       return -EINVAL;
>>   }
>> +static inline void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id,
>> +                             cpumask_t *cpus) { }
>>   #endif
>>     void acpi_arch_init(void);
> 
> Thanks,
> Gavin
> 

Thanks,

Ben