Hi Jonathan,

On 16/07/2025 17:21, Jonathan Cameron wrote:

On Fri, 11 Jul 2025 18:36:19 +0000
James Morse [off-list ref] wrote:

quoted

The MPAM table identifies caches by id. The MPAM driver also wants to know
the cache level to determine if the platform is of the shape that can be
managed via resctrl. Cacheinfo has this information, but only for CPUs that
are online.

Waiting for all CPUs to come online is a problem for platforms where
CPUs are brought online late by user-space.

Add a helper that walks every possible cache, until it finds the one
identified by cache-id, then return the level.

acpi_count_levels() expects its levels parameter to be initialised to
zero as it passes it to acpi_find_cache_level() as starting_level.
The existing callers do this. Document it.

A few suggestions inline.  Mostly driven by the number of missing table
puts I've seen in ACPI code. You don't have any missing here but with a
bit of restructuring you can make that easy to see.

Sounds good,

quoted

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 13ca2eee3b98..f53748a5df19 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c

@@ -912,3 +912,76 @@ int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
 	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
 					  ACPI_PPTT_ACPI_IDENTICAL);
 }
+
+/**
+ * find_acpi_cache_level_from_id() - Get the level of the specified cache
+ * @cache_id: The id field of the unified cache
+ *
+ * Determine the level relative to any CPU for the unified cache identified by
+ * cache_id. This allows the property to be found even if the CPUs are offline.
+ *
+ * The returned level can be used to group unified caches that are peers.
+ *
+ * The PPTT table must be rev 3 or later,
+ *
+ * If one CPUs L2 is shared with another as L3, this function will return
+ * an unpredictable value.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cache cannot be found.
+ * Otherwise returns a value which represents the level of the specified cache.
+ */
+int find_acpi_cache_level_from_id(u32 cache_id)
+{
+	u32 acpi_cpu_id;
+	acpi_status status;
+	int level, cpu, num_levels;
+	struct acpi_pptt_cache *cache;
+	struct acpi_table_header *table;
+	struct acpi_pptt_cache_v1 *cache_v1;
+	struct acpi_pptt_processor *cpu_node;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		acpi_pptt_warn_missing();
+		return -ENOENT;
+	}
+
+	if (table->revision < 3) {

Maybe a unified exit path given all paths need to do
acpi_put_table() and return either error or level.

Or maybe it's time for some cleanup.h magic for acpi tables. I've
been thinking about it for a while and mostly stuck on the name ;)

(Isn't that the hard bit?)

(simpler suggestion follows)

static struct acpi_table_header *acpi_get_table_ret(char *signature, u32 instance)
{
	struct acpi_table_header *table;
	int status = acpi_get_table(signature, instance, &table);
	
	if (ACPI_FAILURE(status))
		return ERR_PTR(-ENOENT);
	return table;
}

DEFINE_FREE(acpi_table, struct acpi_table_header *, if (!IS_ERR(_T)) acpi_put_table(_T))

Finally in here and loads of other places we avoid chance of missing an acpi_put_table
and generally simplify the code a little.

int find_acpi_cache_level_from_id(u32 cache_id)
{
	u32 acpi_cpu_id;
	acpi_status status;
	int level, cpu, num_levels;
	struct acpi_pptt_cache *cache;
	struct acpi_pptt_cache_v1 *cache_v1;
	struct acpi_pptt_processor *cpu_node;


	struct acpi_table_header *table __free(acpi_table) =
		acpi_get_table_ret(ACPI_SIG_PPTT, 0);

	if (IS_ERR(table)
		return PTR_ERR(table);

	if (table->revision < 3)
		return -ENOENT;

	/*
	 * If we found the cache first, we'd still need to walk from each CPU
	 * to find the level...
	 */
	for_each_possible_cpu(cpu) {
		acpi_cpu_id = get_acpi_id_for_cpu(cpu);
		cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
		if (!cpu_node)
			return -ENOENT;
		acpi_count_levels(table, cpu_node, &num_levels, NULL);

		/* Start at 1 for L1 */
		for (level = 1; level <= num_levels; level++) {
			cache = acpi_find_cache_node(table, acpi_cpu_id,
						     ACPI_PPTT_CACHE_TYPE_UNIFIED,
						     level, &cpu_node);
			if (!cache)
				continue;

			cache_v1 = ACPI_ADD_PTR(struct acpi_pptt_cache_v1,
						cache,
						sizeof(struct acpi_pptt_cache));

			if (cache->flags & ACPI_PPTT_CACHE_ID_VALID &&
			    cache_v1->cache_id == cache_id) {
				acpi_put_table(table);
				return level;
			}
		}
	}
	return -ENOENT;
}


A less 'fun' alternative is pull some code out as a helper to make put the get and put
near each other with no conditionals to confuse things.

I still find the cleanup stuff slightly sickening ... so lets use it some more.

Added to linux/acpi.h to make it easier to use elsewhere. I think the earlier patches in
this series are simple enough in this area its not worth changing them...


Thanks,

James