Hi Shameer,
On 17/06/2019 09:28, Shameerali Kolothum Thodi wrote:
>> -----Original Message-----
>> The resctrl ABI requires caches to have a unique id. This number must be
>> unique across all caches at this level, but doesn't need to be contiguous. (there
>> may be gaps, it may not start at 0).
>> See Documentation/x86/intel_rdt_ui.txt::Cache IDs
>>
>> We want a value that is the same over reboots, and should be the same on
>> identical hardware, even if the PPTT is generated in a different order. The
>> hardware doesn't give us any indication of which caches are shared, so this
>> information must come from firmware tables.
>>
>> Starting with a cacheinfo's fw_token, we walk the table to find all CPUs that
>> share this cpu_node (and thus cache), and take the lowest physical id to use as
>> the id for the cache. On arm64 this value corresponds to the MPIDR.
>>
>> This is only done for unified caches, as instruction/data caches would generate
>> the same id using this scheme.
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index
>> d1e26cb599bf..9478f8c28158 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -341,6 +341,84 @@ static struct acpi_pptt_cache
>> +/**
>> + * acpi_pptt_min_physid_from_cpu_node() - Recursivly find @min_physid
>> +for all
>> + * leaf CPUs below @cpu_node.
>> + * @table_hdr: Pointer to the head of the PPTT table
>> + * @cpu_node: The point in the toplogy to start the walk
>> + * @min_physid: The min_physid to update with leaf CPUs.
>> + */
>> +void acpi_pptt_min_physid_from_cpu_node(struct acpi_table_header
>> *table_hdr,
>> + struct acpi_pptt_processor *cpu_node,
>> + phys_cpuid_t *min_physid)
>> +{
>> + bool leaf = true;
>> + u32 acpi_processor_id;
>> + phys_cpuid_t cpu_node_phys_id;
>> + struct acpi_subtable_header *iter;
>> + struct acpi_pptt_processor *iter_node;
>> + u32 target_node = ACPI_PTR_DIFF(cpu_node, table_hdr);
>> + u32 proc_sz = sizeof(struct acpi_pptt_processor *);
>> + unsigned long table_end = (unsigned long)table_hdr +
>> +table_hdr->length;
>> +
>> + /*
>> + * Walk the PPTT, looking for nodes that reference cpu_node
>> + * as parent.
>> + */
>> + iter = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
>> + sizeof(struct acpi_table_pptt));
>> +
>> + while ((unsigned long)iter + proc_sz < table_end) {
>> + iter_node = (struct acpi_pptt_processor *)iter;
>> +
>> + if (iter->type == ACPI_PPTT_TYPE_PROCESSOR &&
>> + iter_node->parent == target_node) {
>> + leaf = false;
>> + acpi_pptt_min_physid_from_cpu_node(table_hdr, iter_node,
>> + min_physid);
>> + }
>> +
>> + if (iter->length == 0)
>> + return;
>> + iter = ACPI_ADD_PTR(struct acpi_subtable_header, iter,
>> + iter->length);
>> + }
>> +
>> + if (leaf && cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) {
>> + acpi_processor_id = cpu_node->acpi_processor_id;
>> + cpu_node_phys_id = acpi_id_to_phys_cpuid(acpi_processor_id);
>> + *min_physid = min(*min_physid, cpu_node_phys_id);
>> + }
>> +}
> I was just trying out the latest public MPAM branch available here[1]
Great!
> and noted that
> on our HiSilicon platform all the L3 cache were labeled with the same Id. Debugging> revealed that the above leaf node check was removed in this branch[2] which makes
> the min_physid calculation going wrong.
Thanks for debugging this,
> Just wondering is there any particular reason
> for removing the check or the branch is not carrying the latest patch?
Nope, that's a bug.
Jeremy Linton's review feedback[0] was that that PROCESSOR_ID_VALID flag can't be relied
on. It looks like I over-zealously removed the whole if(), and this doesn't cause a
problem with my pptt so I didn't notice.
I've fixed it locally, I've also pushed a fix to those branches, but it will get folded in
next time I push a branch.
Thanks!
James
[0] lore.kernel.org/r/a68abfd2-1e28-d9e7-919a-8b3133db4d20@xxxxxxx
