Message-ID: <40b19590-d97c-472b-a577-0b637f6507ef@arm.com>
Date: Thu, 6 Nov 2025 16:09:29 +0000
From: Ben Horgan <ben.horgan@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>,
 James Morse <james.morse@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Dave Martin <dave.martin@....com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com, Rob Herring <robh@...nel.org>,
 Rohit Mathew <rohit.mathew@....com>, Rafael Wysocki <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Hanjun Guo <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>, Jeremy Linton <jeremy.linton@....com>,
 Gavin Shan <gshan@...hat.com>
Subject: Re: [PATCH v3 01/29] ACPI / PPTT: Add a helper to fill a cpumask from
 a processor container

Hi Jonathan,

Replying for James as he's busy and I've been tasked with posting the
next version of this series.

On 10/24/25 12:26, Jonathan Cameron wrote:
> On Fri, 17 Oct 2025 18:56:17 +0000
> James Morse <james.morse@....com> wrote:
> 
>> 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@....com>
>> Reviewed-by: Sudeep Holla <sudeep.holla@....com>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>
>> Reviewed-by: Fenghua Yu <fenghuay@...dia.com>
>> Reviewed-by: Gavin Shan <gshan@...hat.com>
>> Tested-by: Fenghua Yu <fenghuay@...dia.com>
>> Signed-off-by: James Morse <james.morse@....com>
> Hi James,
> 
> V2 has dropped out of my memory more or less so to get this back in
> I'll do a fresh review (even of ones I've already given an RB on).
> Nothing here to change that tag. Just some naming / comment suggestions
> that I think would help a little with readability.
> 
> Some of these comments come from me forgetting how the spec named
> things and so going to take a look.  The spec isn't consistent with the
> naming (e.g. ACPI PROCESSOR ID is not necessarily a processor
> ID) but I think keeping closer to spec names will help readers. 
> 
> Note this may all be in the category of perfect being the enemy of
> good + upstream so I don't mind if you ignore.
> 
>> ---
>> Changes since v2:
>>  * Grouped two nested if clauses differently to reduce scope of cpu_node.
>>  * Removed stale comment refering to the return value.
>>
>> Changes since v1:
>>  * Replaced commit message with wording from Dave.
>>  * Fixed a stray plural.
>>  * Moved further down in the file to make use of get_pptt() helper.
>>  * Added a break to exit the loop early.
>>
>> Changes since RFC:
>>  * Removed leaf_flag local variable from acpi_pptt_get_cpus_from_container()
>>
>> Changes since RFC:
>>  * Dropped has_leaf_flag dodging of acpi_pptt_leaf_node()
>>  * Added missing : in kernel-doc
>>  * Made helper return void as this never actually returns an error.
>> ---
>>  drivers/acpi/pptt.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/acpi.h |  3 ++
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 54676e3d82dd..58cfa3916a13 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -817,3 +817,85 @@ 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 node
> 
> The spec calls these Processor Hierarchy Node Structures. I think the
> addition of the Hierarchy word will help people understand this isn't
> finding things below a node specific to a processor but to some higher
> level hierarchy structure.

I've taken your suggestions around adding hierarchy in the comments but
for the variable and function names I've kept them as they are as adding
hierarchy in made them more unwieldy.

> 
>> + * @table_hdr:		A reference to the PPTT table.
>> + * @parent_node:	A pointer to the processor node in the @table_hdr.
> 
> Likewise, calling this a "processor hierarchy node" would make things
> clearer.
> 
>> + * @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;
> 
> This is definitely a processor hierarchy node.  To my mind
> cpu_node doesn't convey this.  See below for more, but perhaps just renaming
> it to include hierarchy in the name would help.
> 
>> +	u32 acpi_id;
>> +	int cpu;
>> +
>> +	cpumask_clear(cpus);
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		acpi_id = get_acpi_id_for_cpu(cpu);
>> +		cpu_node = acpi_find_processor_node(table_hdr, acpi_id);
> 
> Here it is indeed a CPU.
> 
>> +
>> +		while (cpu_node) {
>> +			if (cpu_node == parent_node) {
>> +				cpumask_set_cpu(cpu, cpus);
>> +				break;
>> +			}
>> +			cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
> 
> But here it is a processor container or a private node.  So
> cpu_hieriarchy_node or something along those lines would be more appropriate.
> 
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * 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' entries in the PPTT are either a CPU or a Processor
> 
> I'd go with 'Processor Hierarchy' here
>  
>> + * 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;
> similar naming thing here. 
>> +
>> +			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);
> The double tab indent here is odd.
> 			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);
> 
> Isn't find I think for readability.  I could understand the bonus tab if it was
> close to aligning with the line above but it isn't.

Indent fixed.

> 
>> +					break;
>> +			}
>> +		}
>> +		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
>> +				     entry->length);
>> +	}
>> +}
> 
> 
> 

Thanks,

Ben

Powered by blists - more mailing lists