Hi Shameer,

On 17/06/2019 09:28, Shameerali Kolothum Thodi wrote:

quoted

-----Original Message-----

quoted

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.

quoted

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@arm.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel