Hi James,
On Thu, Aug 28, 2025 at 04:57:06PM +0100, James Morse wrote:
> Hi Dave,
>
> On 27/08/2025 11:48, Dave Martin wrote:
> > On Fri, Aug 22, 2025 at 03:29:44PM +0000, James Morse wrote:
> >> The PPTT describes CPUs and caches, as well as processor containers.
> >> The ACPI table for MPAM describes the set of CPUs that can access an MSC
> >> with the UID of a processor container.
> >>
> >> 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.
>
> > Nit: The motivation for the change is not clear here.
> >
> > I guess this boils down to the need to map the MSC topology information
> > in the the ACPI MPAM table to a cpumask for each MSC.
> >
> > If so, a possible rearrangement and rewording might be, say:
> >
> > --8<--
> >
> > The ACPI MPAM table uses the UID of a processor container specified in
> > the PPTT, to indicate the subset of CPUs and upstream cache topology
> > that can access each MPAM Memory 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 [...]
> >
> > -->8--
>
> Thanks, that is clearer!
Thanks
> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
[...]
> >> @@ -298,6 +298,92 @@ static struct acpi_pptt_processor *acpi_find_processor_node(struct acpi_table_he
[...]
> >> +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);
> >> +
> >> + for_each_possible_cpu(cpu) {
> >> + acpi_id = get_acpi_id_for_cpu(cpu);
>
> > ^ Presumably this can't fail?
>
> It'll return something! This could only be a problem if this raced with a CPU becoming
> impossible, and there is no mechanism to do that.
Yep, now I go and look more closely at that function, my question looks
misguided.
[...]
> >> +void acpi_pptt_get_cpus_from_container(u32 acpi_cpu_id, cpumask_t *cpus)
> >> +{
> >> + struct acpi_pptt_processor *cpu_node;
> >> + struct acpi_table_header *table_hdr;
> >> + struct acpi_subtable_header *entry;
> >> + unsigned long table_end;
> >> + acpi_status status;
> >> + bool leaf_flag;
> >> + u32 proc_sz;
> >> +
> >> + cpumask_clear(cpus);
> >> +
> >> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table_hdr);
> >> + if (ACPI_FAILURE(status))
> >> + return;
>
> > Is acpi_get_pptt() applicable here?
>
> Oh, that is new, and would let me chuck the reference counting.
> I guess this replaces Jonthan's magic table free'ing cleanup thing!
Ah, rightho.
> > (That function is not thread-safe, but then, perhaps most/all of these
> > functions are not thread safe. If we are still on the boot CPU at this
> > point (?) then this wouldn't be a concern.)
>
> I think that relies on the first caller being from somewhere that can't race.
> In this case its the architecture's smp_prepare_cpus() call to setup the acpi topology.
> That is sufficiently early its not a concern.
I guess so.
[...]
> >> + cpu_node = (struct acpi_pptt_processor *)entry;
> >> + if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
> >> + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
> >> + leaf_flag = acpi_pptt_leaf_node(table_hdr, cpu_node);
> >> + if (!leaf_flag) {
> >> + if (cpu_node->acpi_processor_id == acpi_cpu_id)
>
>
> > Is there any need to distinguish processor containers from (leaf) CPU
> > nodes, here? If not, dropping the distinction might simplify the code
> > here (even if callers do not care).
>
> In the namespace the object types are different, so I assumed they have their own UID
> space. The PPTT holds both - hence the check for which kind of thing it is. The risk is
> looking for processor-container-4 and finding CPU-4 instead...
>
> The relevant ACPI bit is "8.4.2.1 Processor Container Device", its says:
> | A processor container declaration must supply a _UID method returning an ID that is
> | unique in the processor container hierarchy.
>
> Which doesn't quite let me combine them here.
I was going by the PPTT spec, where the types are not distinct --
you're probably right, though.
According to that, isn't it the "ACPI Processor ID valid" flag, not the
"Node is a Leaf" flag, that says whether this field is meaningful?
It's reasonable not to bother to try to enumerate the children of a
node that claims to be a leaf (even if there actually are children),
but I wonder what happens if acpi_processor_id is not declared to be
valid and matches by accident. That's probably not a valid table (?)
but does anything bad happen on the kernel side?
> > Otherwise, maybe eliminate leaf_flag and collapse these into a single
> > if(), as suggested by Ben [1].
> >
> >> + acpi_pptt_get_child_cpus(table_hdr, cpu_node, cpus);
> >
> > Can there ever be multiple matches?
> >
> > The possibility of duplicate processor IDs in the PPTT sounds weird to
> > me, but then I'm not an ACPI expert.
>
> Multiple processor-containers with the same ID? That would be a corrupt table.
> acpi_pptt_get_child_cpus() then walks the tree again to find the CPUs below this
> processor-container - those have a different kind of id.
Does anything bad happen if we encounter duplicates?
(Other then the MPAM driver never getting enabled, or not working as
advertised, that is.)
I haven't tried to think through all the implications, here.
> > If there can only be a single match, though, then we may as well break
> > out of the loop here, unless we want to be paranoid and report
> > duplicates as an error -- but that would require extra implementation,
> > so I'm not sure that would be worth it.
>
> Hmmm, the PPTT node should map to only one processor or processor-container.
> I'll chuck the break in.
Ack
Cheers
---Dave
