linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support
@ 2021-12-16 23:31 Rob Herring
  2021-12-16 23:31 ` [PATCH 1/6] cacheinfo: Allow for >32-bit cache 'id' Rob Herring
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

For upcoming Arm MPAM support in resctrl, it is necessary to have the
cacheinfo 'id' for MPAM enabled caches. The 'id' is part of the resctrl
ABI. While this support already exists for ACPI based systems, it is
missing for DT. This series adds the support.

The 'id' value used is the smallest CPU h/w id value associated with a
cache. This requires walking the cache hierarchy from every CPU node to
get all CPUs associated with a cache. As MPAM also needs to know this,
the CPU affinity is also saved to avoid reimplementing and walking the
firmware tables again.

Patches 1 and 2 are v2 from the prior series[1]. The rest are new.

Tested on arm64 with DT. ACPI changes are untested. I don't have a
system with an appropriate PPTT nor do I know how to modify ACPI tables.

Rob

[1] https://lore.kernel.org/all/20211006164332.1981454-1-robh@kernel.org/

Rob Herring (6):
  cacheinfo: Allow for >32-bit cache 'id'
  cacheinfo: Set cache 'id' based on DT data
  cacheinfo: Add cpu_affinity_map to store affinity for all CPUs
  ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map
  cacheinfo: Use cpu_affinity_map for populating shared_cpu_map
  cacheinfo: Add cacheinfo_get_cache_affinity() function

 drivers/acpi/pptt.c       | 29 +++++++++++++++--
 drivers/base/cacheinfo.c  | 65 ++++++++++++++++++++++++++-------------
 include/linux/cacheinfo.h | 29 +++++++++++++++--
 3 files changed, 97 insertions(+), 26 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/6] cacheinfo: Allow for >32-bit cache 'id'
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-16 23:31 ` [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data Rob Herring
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

In preparation to set the cache 'id' based on the CPU h/w ids which are
64-bit on arm64, allow for a 64-bit bit 'id' value. The only case that
needs this is arm64, so unsigned long is sufficient.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/base/cacheinfo.c  | 8 +++++++-
 include/linux/cacheinfo.h | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index dad296229161..66d10bdb863b 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -366,13 +366,19 @@ static ssize_t file_name##_show(struct device *dev,		\
 	return sysfs_emit(buf, "%u\n", this_leaf->object);	\
 }
 
-show_one(id, id);
 show_one(level, level);
 show_one(coherency_line_size, coherency_line_size);
 show_one(number_of_sets, number_of_sets);
 show_one(physical_line_partition, physical_line_partition);
 show_one(ways_of_associativity, ways_of_associativity);
 
+static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct cacheinfo *this_leaf = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%lu\n", this_leaf->id);
+}
+
 static ssize_t size_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 2f909ed084c6..b2e7f3e40204 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -48,7 +48,7 @@ extern unsigned int coherency_max_size;
  * keeping, the remaining members form the core properties of the cache
  */
 struct cacheinfo {
-	unsigned int id;
+	unsigned long id;
 	enum cache_type type;
 	unsigned int level;
 	unsigned int coherency_line_size;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
  2021-12-16 23:31 ` [PATCH 1/6] cacheinfo: Allow for >32-bit cache 'id' Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-17 16:57   ` Robin Murphy
  2021-12-16 23:31 ` [PATCH 3/6] cacheinfo: Add cpu_affinity_map to store affinity for all CPUs Rob Herring
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

Use the minimum CPU h/w id of the CPUs associated with the cache for the
cache 'id'. This will provide a stable id value for a given system. As
we need to check all possible CPUs, we can't use the shared_cpu_map
which is just online CPUs. There's not a cache to CPUs mapping in DT, so
we have to walk all CPU nodes and then walk cache levels.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
 - Loop with for_each_possible_cpu instead of for_each_of_cpu_node as
   we will need the logical cpu numbers.
---
 drivers/base/cacheinfo.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 66d10bdb863b..21accddf8f5f 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -136,6 +136,36 @@ static bool cache_node_is_unified(struct cacheinfo *this_leaf,
 	return of_property_read_bool(np, "cache-unified");
 }
 
+static void cache_of_set_id(struct cacheinfo *this_leaf, struct device_node *np)
+{
+	int cpu;
+	unsigned long min_id = ~0UL;
+
+	for_each_possible_cpu(cpu) {
+		u64 id;
+		struct device_node *cache_node, *cpu_node;
+
+		cache_node = cpu_node = of_get_cpu_node(cpu, NULL);
+		id = of_get_cpu_hwid(cpu_node, 0);
+		while ((cache_node = of_find_next_cache_node(cache_node))) {
+			if (cache_node == np) {
+				if (id < min_id) {
+					min_id = id;
+					of_node_put(cache_node);
+					break;
+				}
+			}
+			of_node_put(cache_node);
+		}
+		of_node_put(cpu_node);
+	}
+
+	if (min_id != ~0UL) {
+		this_leaf->id = min_id;
+		this_leaf->attributes |= CACHE_ID;
+	}
+}
+
 static void cache_of_set_props(struct cacheinfo *this_leaf,
 			       struct device_node *np)
 {
@@ -151,6 +181,7 @@ static void cache_of_set_props(struct cacheinfo *this_leaf,
 	cache_get_line_size(this_leaf, np);
 	cache_nr_sets(this_leaf, np);
 	cache_associativity(this_leaf);
+	cache_of_set_id(this_leaf, np);
 }
 
 static int cache_setup_of_node(unsigned int cpu)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/6] cacheinfo: Add cpu_affinity_map to store affinity for all CPUs
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
  2021-12-16 23:31 ` [PATCH 1/6] cacheinfo: Allow for >32-bit cache 'id' Rob Herring
  2021-12-16 23:31 ` [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-16 23:31 ` [PATCH 4/6] ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map Rob Herring
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

Currently, getting the cache CPU affinity for all possible CPUs requires
walking the DT or ACPI tables. As that is already done once (for each
CPU online event), let's save the affinity for possible CPUs so it can be
retrieved later.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/base/cacheinfo.c  | 1 +
 include/linux/cacheinfo.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 21accddf8f5f..c9e5b48fac42 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -149,6 +149,7 @@ static void cache_of_set_id(struct cacheinfo *this_leaf, struct device_node *np)
 		id = of_get_cpu_hwid(cpu_node, 0);
 		while ((cache_node = of_find_next_cache_node(cache_node))) {
 			if (cache_node == np) {
+				cpumask_set_cpu(cpu, &this_leaf->cpu_affinity_map);
 				if (id < min_id) {
 					min_id = id;
 					of_node_put(cache_node);
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index b2e7f3e40204..37652cfdd8dc 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -56,7 +56,8 @@ struct cacheinfo {
 	unsigned int ways_of_associativity;
 	unsigned int physical_line_partition;
 	unsigned int size;
-	cpumask_t shared_cpu_map;
+	cpumask_t cpu_affinity_map;	/* possible CPUs */
+	cpumask_t shared_cpu_map;	/* online CPUs */
 	unsigned int attributes;
 #define CACHE_WRITE_THROUGH	BIT(0)
 #define CACHE_WRITE_BACK	BIT(1)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/6] ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
                   ` (2 preceding siblings ...)
  2021-12-16 23:31 ` [PATCH 3/6] cacheinfo: Add cpu_affinity_map to store affinity for all CPUs Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-16 23:31 ` [PATCH 5/6] cacheinfo: Use cpu_affinity_map for populating shared_cpu_map Rob Herring
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

The cacheinfo.cpu_affinity_map was added to list the possible CPUs a
cache is associated with. Populate the cpu_affinity_map when parsing the
ACPI PPTT for cacheinfo. With this, the cache CPU affinities are
represented in the same way for DT and ACPI.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/acpi/pptt.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 701f61c01359..686018f4e7ed 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -418,6 +418,30 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
 	}
 }
 
+static void acpi_set_cache_cpumask(struct acpi_table_header *table,
+				   struct cacheinfo *this_leaf,
+				   struct acpi_pptt_processor *match_cpu_node)
+{
+	int cpu;
+
+	/*
+	 * If we found the cache first, we'd still need to walk from each cpu.
+	 */
+	for_each_possible_cpu(cpu) {
+		u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+		struct acpi_pptt_processor *cpu_node;
+		struct acpi_pptt_cache *cache;
+
+		cache = acpi_find_cache_node(table, acpi_cpu_id,
+					     this_leaf->type,
+					     this_leaf->level, &cpu_node);
+		if (!cache || cpu_node != match_cpu_node)
+			continue;
+
+		cpumask_set_cpu(cpu, &this_leaf->cpu_affinity_map);
+	}
+}
+
 static void cache_setup_acpi_cpu(struct acpi_table_header *table,
 				 unsigned int cpu)
 {
@@ -435,10 +459,11 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
 						   this_leaf->level,
 						   &cpu_node);
 		pr_debug("found = %p %p\n", found_cache, cpu_node);
-		if (found_cache)
+		if (found_cache) {
 			update_cache_properties(this_leaf, found_cache,
 			                        cpu_node, table->revision);
-
+			acpi_set_cache_cpumask(table, this_leaf, cpu_node);
+		}
 		index++;
 	}
 }
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/6] cacheinfo: Use cpu_affinity_map for populating shared_cpu_map
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
                   ` (3 preceding siblings ...)
  2021-12-16 23:31 ` [PATCH 4/6] ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-16 23:31 ` [PATCH 6/6] cacheinfo: Add cacheinfo_get_cache_affinity() function Rob Herring
  2021-12-21  9:31 ` [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Greg Kroah-Hartman
  6 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

Now that we have a full map of possible shared CPUs, we can iterate over
just the cache's cpu_affinity_map instead of all online CPUs to populate
the shared_cpu_map.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/base/cacheinfo.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index c9e5b48fac42..d7129b2fa9dc 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -32,12 +32,6 @@ struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu)
 }
 
 #ifdef CONFIG_OF
-static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
-					   struct cacheinfo *sib_leaf)
-{
-	return sib_leaf->fw_token == this_leaf->fw_token;
-}
-
 /* OF properties to query for a given cache type */
 struct cache_type_info {
 	const char *size_prop;
@@ -228,16 +222,6 @@ static int cache_setup_of_node(unsigned int cpu)
 }
 #else
 static inline int cache_setup_of_node(unsigned int cpu) { return 0; }
-static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
-					   struct cacheinfo *sib_leaf)
-{
-	/*
-	 * For non-DT/ACPI systems, assume unique level 1 caches, system-wide
-	 * shared caches for all other levels. This will be used only if
-	 * arch specific code has not populated shared_cpu_map
-	 */
-	return !(this_leaf->level == 1);
-}
 #endif
 
 int __weak cache_setup_acpi(unsigned int cpu)
@@ -274,16 +258,15 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 			continue;
 
 		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
-		for_each_online_cpu(i) {
+		for_each_cpu(i, &this_leaf->cpu_affinity_map) {
 			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);
 
 			if (i == cpu || !sib_cpu_ci->info_list)
 				continue;/* skip if itself or no cacheinfo */
+
 			sib_leaf = sib_cpu_ci->info_list + index;
-			if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
-				cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
-				cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
-			}
+			cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
+			cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
 		}
 		/* record the maximum cache line size */
 		if (this_leaf->coherency_line_size > coherency_max_size)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 6/6] cacheinfo: Add cacheinfo_get_cache_affinity() function
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
                   ` (4 preceding siblings ...)
  2021-12-16 23:31 ` [PATCH 5/6] cacheinfo: Use cpu_affinity_map for populating shared_cpu_map Rob Herring
@ 2021-12-16 23:31 ` Rob Herring
  2021-12-21  9:31 ` [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Greg Kroah-Hartman
  6 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-16 23:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

Add a cacheinfo_get_cache_affinity() function to retrieve the CPU affinity
mask for a given cache identified by level and cache id.

This is needed by Arm MPAM to get the CPU affinity of MPAM enabled caches.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 include/linux/cacheinfo.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 37652cfdd8dc..5e72420cdc75 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -123,4 +123,28 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
 	return -1;
 }
 
+/*
+ * Get the CPU affinity of the cache associated with @cpu at level @level and
+ * with identifier @id.
+ * cpuhp lock must be held.
+ */
+static inline int cacheinfo_get_cache_affinity(int cpu, int level, int id,
+					       cpumask_t *mask)
+{
+	struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
+	int i;
+
+	for (i = 0; ci->info_list && i < ci->num_leaves; i++) {
+		if ((ci->info_list[i].level == level) &&
+		    (ci->info_list[i].attributes & CACHE_ID) &&
+		    (ci->info_list[i].id == id)) {
+			cpumask_copy(mask, &ci->info_list[i].cpu_affinity_map);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+
 #endif /* _LINUX_CACHEINFO_H */
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-16 23:31 ` [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data Rob Herring
@ 2021-12-17 16:57   ` Robin Murphy
  2021-12-17 18:14     ` Rob Herring
  0 siblings, 1 reply; 18+ messages in thread
From: Robin Murphy @ 2021-12-17 16:57 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, James Morse, Jeremy Linton
  Cc: linux-arm-kernel, linux-kernel, Sudeep Holla, Rafael J. Wysocki,
	Len Brown, devicetree, linux-acpi

Hi Rob,

On 2021-12-16 23:31, Rob Herring wrote:
> Use the minimum CPU h/w id of the CPUs associated with the cache for the
> cache 'id'. This will provide a stable id value for a given system. As
> we need to check all possible CPUs, we can't use the shared_cpu_map
> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> we have to walk all CPU nodes and then walk cache levels.

I believe another expected use of the cache ID exposed in sysfs is to 
program steering tags for cache stashing (typically in VFIO-based 
userspace drivers like DPDK so we can't realistically mediate it any 
other way). There were plans afoot last year to ensure that ACPI PPTT 
could provide the necessary ID values for arm64 systems which will 
typically be fairly arbitrary (but unique) due to reflecting underlying 
interconnect routing IDs. Assuming that there will eventually be some 
interest in cache stashing on DT-based systems too, we probably want to 
allow for an explicit ID property on DT cache nodes in a similar manner.

That said, I think it does make sense to have some kind of 
auto-generated fallback scheme *as well*, since I'm sure there will be 
plenty systems which care about MPAM but don't support stashing, and 
therefore wouldn't have a meaningful set of IDs to populate their DT 
with. Conversely I think that might also matter for ACPI too - one point 
I remember from previous discussions is that PPTT may use a compact 
representation where a single entry represents all equivalent caches at 
that level, so I'm not sure we can necessarily rely on IDs out of that 
path being unique either.

Robin.

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2:
>   - Loop with for_each_possible_cpu instead of for_each_of_cpu_node as
>     we will need the logical cpu numbers.
> ---
>   drivers/base/cacheinfo.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 66d10bdb863b..21accddf8f5f 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -136,6 +136,36 @@ static bool cache_node_is_unified(struct cacheinfo *this_leaf,
>   	return of_property_read_bool(np, "cache-unified");
>   }
>   
> +static void cache_of_set_id(struct cacheinfo *this_leaf, struct device_node *np)
> +{
> +	int cpu;
> +	unsigned long min_id = ~0UL;
> +
> +	for_each_possible_cpu(cpu) {
> +		u64 id;
> +		struct device_node *cache_node, *cpu_node;
> +
> +		cache_node = cpu_node = of_get_cpu_node(cpu, NULL);
> +		id = of_get_cpu_hwid(cpu_node, 0);
> +		while ((cache_node = of_find_next_cache_node(cache_node))) {
> +			if (cache_node == np) {
> +				if (id < min_id) {
> +					min_id = id;
> +					of_node_put(cache_node);
> +					break;
> +				}
> +			}
> +			of_node_put(cache_node);
> +		}
> +		of_node_put(cpu_node);
> +	}
> +
> +	if (min_id != ~0UL) {
> +		this_leaf->id = min_id;
> +		this_leaf->attributes |= CACHE_ID;
> +	}
> +}
> +
>   static void cache_of_set_props(struct cacheinfo *this_leaf,
>   			       struct device_node *np)
>   {
> @@ -151,6 +181,7 @@ static void cache_of_set_props(struct cacheinfo *this_leaf,
>   	cache_get_line_size(this_leaf, np);
>   	cache_nr_sets(this_leaf, np);
>   	cache_associativity(this_leaf);
> +	cache_of_set_id(this_leaf, np);
>   }
>   
>   static int cache_setup_of_node(unsigned int cpu)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 16:57   ` Robin Murphy
@ 2021-12-17 18:14     ` Rob Herring
  2021-12-17 19:03       ` Sudeep Holla
  2021-12-17 19:08       ` Robin Murphy
  0 siblings, 2 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-17 18:14 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Greg Kroah-Hartman, James Morse, Jeremy Linton, linux-arm-kernel,
	linux-kernel, Sudeep Holla, Rafael J. Wysocki, Len Brown,
	devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Hi Rob,
>
> On 2021-12-16 23:31, Rob Herring wrote:
> > Use the minimum CPU h/w id of the CPUs associated with the cache for the
> > cache 'id'. This will provide a stable id value for a given system. As
> > we need to check all possible CPUs, we can't use the shared_cpu_map
> > which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> > we have to walk all CPU nodes and then walk cache levels.
>
> I believe another expected use of the cache ID exposed in sysfs is to
> program steering tags for cache stashing (typically in VFIO-based
> userspace drivers like DPDK so we can't realistically mediate it any
> other way). There were plans afoot last year to ensure that ACPI PPTT
> could provide the necessary ID values for arm64 systems which will
> typically be fairly arbitrary (but unique) due to reflecting underlying
> interconnect routing IDs. Assuming that there will eventually be some
> interest in cache stashing on DT-based systems too, we probably want to
> allow for an explicit ID property on DT cache nodes in a similar manner.

If you have a suggestion for ID values that correspond to the h/w,
then we can add them. I'd like a bit more than just trusting that ID
is something real.

While the ACPI folks may be willing to take an arbitrary index, it's
something we (mostly) avoid for DT.

> That said, I think it does make sense to have some kind of
> auto-generated fallback scheme *as well*, since I'm sure there will be
> plenty systems which care about MPAM but don't support stashing, and
> therefore wouldn't have a meaningful set of IDs to populate their DT
> with. Conversely I think that might also matter for ACPI too - one point
> I remember from previous discussions is that PPTT may use a compact
> representation where a single entry represents all equivalent caches at
> that level, so I'm not sure we can necessarily rely on IDs out of that
> path being unique either.

AIUI, cache ids break the compact representation.

Rob

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 18:14     ` Rob Herring
@ 2021-12-17 19:03       ` Sudeep Holla
  2021-12-17 19:08         ` Sudeep Holla
  2021-12-17 19:26         ` Rob Herring
  2021-12-17 19:08       ` Robin Murphy
  1 sibling, 2 replies; 18+ messages in thread
From: Sudeep Holla @ 2021-12-17 19:03 UTC (permalink / raw)
  To: Rob Herring
  Cc: Robin Murphy, Greg Kroah-Hartman, James Morse, Sudeep Holla,
	Jeremy Linton, linux-arm-kernel, linux-kernel, Rafael J. Wysocki,
	Len Brown, devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On Fri, Dec 17, 2021 at 12:14:22PM -0600, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > Hi Rob,
> >
> > On 2021-12-16 23:31, Rob Herring wrote:
> > > Use the minimum CPU h/w id of the CPUs associated with the cache for the
> > > cache 'id'. This will provide a stable id value for a given system. As

I am trying to follow the code. IIUC, the level one(I$ and D$) are skipped
in this logic and the private unified cache if any will get the cpu hwid as
the cache id which is all fine. But what happens if there are 2 levels of
unified private cache ? I am assuming we only care about shared caches for
MPAM and ignore private caches which sounds OK but I just wanted to confirm.

> > > we need to check all possible CPUs, we can't use the shared_cpu_map
> > > which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> > > we have to walk all CPU nodes and then walk cache levels.

I would have preferred to add the cache IDs in DT similar to ACPI but I see
you have certain concerns with that which are valid as well.

> >
> > I believe another expected use of the cache ID exposed in sysfs is to
> > program steering tags for cache stashing (typically in VFIO-based
> > userspace drivers like DPDK so we can't realistically mediate it any
> > other way). There were plans afoot last year to ensure that ACPI PPTT
> > could provide the necessary ID values for arm64 systems which will
> > typically be fairly arbitrary (but unique) due to reflecting underlying
> > interconnect routing IDs. Assuming that there will eventually be some
> > interest in cache stashing on DT-based systems too, we probably want to
> > allow for an explicit ID property on DT cache nodes in a similar manner.
> 
> If you have a suggestion for ID values that correspond to the h/w,
> then we can add them. I'd like a bit more than just trusting that ID
> is something real.
>

I agree, probably architecture must do better job at defining these. But
generated IDs IMO might cause issues especial if we have to change the
logic without breaking the backward compatibility.

> While the ACPI folks may be willing to take an arbitrary index, it's
> something we (mostly) avoid for DT.
>

Not sure if we can call that *arbitrary* 😄, in that case we can imagine
the same at several places in the firmware.

> > That said, I think it does make sense to have some kind of
> > auto-generated fallback scheme *as well*, since I'm sure there will be
> > plenty systems which care about MPAM but don't support stashing, and
> > therefore wouldn't have a meaningful set of IDs to populate their DT
> > with. Conversely I think that might also matter for ACPI too - one point
> > I remember from previous discussions is that PPTT may use a compact
> > representation where a single entry represents all equivalent caches at
> > that level, so I'm not sure we can necessarily rely on IDs out of that
> > path being unique either.
> 
> AIUI, cache ids break the compact representation.
>

IIRC, a note was added to avoid compaction if an implementation requires
any cache instance to be referenced uniquely.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:03       ` Sudeep Holla
@ 2021-12-17 19:08         ` Sudeep Holla
  2021-12-17 19:26         ` Rob Herring
  1 sibling, 0 replies; 18+ messages in thread
From: Sudeep Holla @ 2021-12-17 19:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Robin Murphy, Greg Kroah-Hartman, James Morse, Sudeep Holla,
	Jeremy Linton, linux-arm-kernel, linux-kernel, Rafael J. Wysocki,
	Len Brown, devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On Fri, Dec 17, 2021 at 07:03:45PM +0000, Sudeep Holla wrote:
> On Fri, Dec 17, 2021 at 12:14:22PM -0600, Rob Herring wrote:
> > On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > On 2021-12-16 23:31, Rob Herring wrote:
> > > > Use the minimum CPU h/w id of the CPUs associated with the cache for the
> > > > cache 'id'. This will provide a stable id value for a given system. As
> 
> I am trying to follow the code. IIUC, the level one(I$ and D$) are skipped
> in this logic and the private unified cache if any will get the cpu hwid as
> the cache id which is all fine. But what happens if there are 2 levels of
> unified private cache ? I am assuming we only care about shared caches for
> MPAM and ignore private caches which sounds OK but I just wanted to confirm.
> 
> > > > we need to check all possible CPUs, we can't use the shared_cpu_map
> > > > which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> > > > we have to walk all CPU nodes and then walk cache levels.
> 
> I would have preferred to add the cache IDs in DT similar to ACPI but I see
> you have certain concerns with that which are valid as well.

One thing I forgot to add is for some weird reasons, some platform supports
both DT and ACPI, this will force the ID generated here to be used in ACPI as
well to ensure same userspace scripts can be used to manage both. That doesn't
sound so great to me.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 18:14     ` Rob Herring
  2021-12-17 19:03       ` Sudeep Holla
@ 2021-12-17 19:08       ` Robin Murphy
  2021-12-17 19:35         ` Rob Herring
  1 sibling, 1 reply; 18+ messages in thread
From: Robin Murphy @ 2021-12-17 19:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Greg Kroah-Hartman, James Morse, Jeremy Linton, linux-arm-kernel,
	linux-kernel, Sudeep Holla, Rafael J. Wysocki, Len Brown,
	devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On 2021-12-17 18:14, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> Hi Rob,
>>
>> On 2021-12-16 23:31, Rob Herring wrote:
>>> Use the minimum CPU h/w id of the CPUs associated with the cache for the
>>> cache 'id'. This will provide a stable id value for a given system. As
>>> we need to check all possible CPUs, we can't use the shared_cpu_map
>>> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
>>> we have to walk all CPU nodes and then walk cache levels.
>>
>> I believe another expected use of the cache ID exposed in sysfs is to
>> program steering tags for cache stashing (typically in VFIO-based
>> userspace drivers like DPDK so we can't realistically mediate it any
>> other way). There were plans afoot last year to ensure that ACPI PPTT
>> could provide the necessary ID values for arm64 systems which will
>> typically be fairly arbitrary (but unique) due to reflecting underlying
>> interconnect routing IDs. Assuming that there will eventually be some
>> interest in cache stashing on DT-based systems too, we probably want to
>> allow for an explicit ID property on DT cache nodes in a similar manner.
> 
> If you have a suggestion for ID values that correspond to the h/w,
> then we can add them. I'd like a bit more than just trusting that ID
> is something real.
> 
> While the ACPI folks may be willing to take an arbitrary index, it's
> something we (mostly) avoid for DT.

Not really. On the CHI side there are two fields - StashNID, which could 
be any node ID value depending on the interconnect layout, plus 
(optionally) StashLPID to address a specific cache within that node if 
it's something like a CPU cluster. However, how a PCIe TLP steering tag 
translates to those fields in the resulting CHI flit is largely up to 
the root complex.

I think it's going to be more like a "reg" property than a nice 
validatable index.

>> That said, I think it does make sense to have some kind of
>> auto-generated fallback scheme *as well*, since I'm sure there will be
>> plenty systems which care about MPAM but don't support stashing, and
>> therefore wouldn't have a meaningful set of IDs to populate their DT
>> with. Conversely I think that might also matter for ACPI too - one point
>> I remember from previous discussions is that PPTT may use a compact
>> representation where a single entry represents all equivalent caches at
>> that level, so I'm not sure we can necessarily rely on IDs out of that
>> path being unique either.
> 
> AIUI, cache ids break the compact representation.

Right, firmware authors can't use it if they do want to specify IDs, but 
that also means that if we find we *are* consuming a compact PPTT, then 
chances are we're not getting meaningful IDs out of it for MPAM to rely on.

Robin.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:03       ` Sudeep Holla
  2021-12-17 19:08         ` Sudeep Holla
@ 2021-12-17 19:26         ` Rob Herring
  2021-12-17 20:28           ` Jeremy Linton
  1 sibling, 1 reply; 18+ messages in thread
From: Rob Herring @ 2021-12-17 19:26 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Robin Murphy, Greg Kroah-Hartman, James Morse, Jeremy Linton,
	linux-arm-kernel, linux-kernel, Rafael J. Wysocki, Len Brown,
	devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On Fri, Dec 17, 2021 at 1:03 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Dec 17, 2021 at 12:14:22PM -0600, Rob Herring wrote:
> > On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > On 2021-12-16 23:31, Rob Herring wrote:
> > > > Use the minimum CPU h/w id of the CPUs associated with the cache for the
> > > > cache 'id'. This will provide a stable id value for a given system. As
>
> I am trying to follow the code. IIUC, the level one(I$ and D$) are skipped
> in this logic and the private unified cache if any will get the cpu hwid as
> the cache id which is all fine. But what happens if there are 2 levels of
> unified private cache ? I am assuming we only care about shared caches for
> MPAM and ignore private caches which sounds OK but I just wanted to confirm.

The cacheinfo 'id' is only unique to the level and type. It's the
type, level, and ID that gives a unique identifier:

 * struct cacheinfo - represent a cache leaf node
 * @id: This cache's id. It is unique among caches with the same (type, level).

Maybe ACPI's ID expects/allows globally unique cache IDs?

> > > > we need to check all possible CPUs, we can't use the shared_cpu_map
> > > > which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> > > > we have to walk all CPU nodes and then walk cache levels.
>
> I would have preferred to add the cache IDs in DT similar to ACPI but I see
> you have certain concerns with that which are valid as well.
>
> > >
> > > I believe another expected use of the cache ID exposed in sysfs is to
> > > program steering tags for cache stashing (typically in VFIO-based
> > > userspace drivers like DPDK so we can't realistically mediate it any
> > > other way). There were plans afoot last year to ensure that ACPI PPTT
> > > could provide the necessary ID values for arm64 systems which will
> > > typically be fairly arbitrary (but unique) due to reflecting underlying
> > > interconnect routing IDs. Assuming that there will eventually be some
> > > interest in cache stashing on DT-based systems too, we probably want to
> > > allow for an explicit ID property on DT cache nodes in a similar manner.
> >
> > If you have a suggestion for ID values that correspond to the h/w,
> > then we can add them. I'd like a bit more than just trusting that ID
> > is something real.
> >
>
> I agree, probably architecture must do better job at defining these. But
> generated IDs IMO might cause issues especial if we have to change the
> logic without breaking the backward compatibility.
>
> > While the ACPI folks may be willing to take an arbitrary index, it's
> > something we (mostly) avoid for DT.
> >
>
> Not sure if we can call that *arbitrary* 😄, in that case we can imagine
> the same at several places in the firmware.

By arbitrary, I mean made up by the binding/dts author or
documentation convention (UART0, UART1, etc.). Certainly things like
clock IDs are often made up number spaces, but I don't see how we
avoid that. DT had 'cell-index' which I still see attempted. But that
property traces back to h/w having a single power ctrl register and
cell-index was the bit index for the register. If only h/w was still
that simple.

Rob

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:08       ` Robin Murphy
@ 2021-12-17 19:35         ` Rob Herring
  2021-12-17 20:22           ` Jeremy Linton
  2021-12-17 21:13           ` Robin Murphy
  0 siblings, 2 replies; 18+ messages in thread
From: Rob Herring @ 2021-12-17 19:35 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Greg Kroah-Hartman, James Morse, Jeremy Linton, linux-arm-kernel,
	linux-kernel, Sudeep Holla, Rafael J. Wysocki, Len Brown,
	devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On Fri, Dec 17, 2021 at 1:08 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-12-17 18:14, Rob Herring wrote:
> > On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> Hi Rob,
> >>
> >> On 2021-12-16 23:31, Rob Herring wrote:
> >>> Use the minimum CPU h/w id of the CPUs associated with the cache for the
> >>> cache 'id'. This will provide a stable id value for a given system. As
> >>> we need to check all possible CPUs, we can't use the shared_cpu_map
> >>> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
> >>> we have to walk all CPU nodes and then walk cache levels.
> >>
> >> I believe another expected use of the cache ID exposed in sysfs is to
> >> program steering tags for cache stashing (typically in VFIO-based
> >> userspace drivers like DPDK so we can't realistically mediate it any
> >> other way). There were plans afoot last year to ensure that ACPI PPTT
> >> could provide the necessary ID values for arm64 systems which will
> >> typically be fairly arbitrary (but unique) due to reflecting underlying
> >> interconnect routing IDs. Assuming that there will eventually be some
> >> interest in cache stashing on DT-based systems too, we probably want to
> >> allow for an explicit ID property on DT cache nodes in a similar manner.
> >
> > If you have a suggestion for ID values that correspond to the h/w,
> > then we can add them. I'd like a bit more than just trusting that ID
> > is something real.
> >
> > While the ACPI folks may be willing to take an arbitrary index, it's
> > something we (mostly) avoid for DT.
>
> Not really. On the CHI side there are two fields - StashNID, which could
> be any node ID value depending on the interconnect layout, plus
> (optionally) StashLPID to address a specific cache within that node if
> it's something like a CPU cluster. However, how a PCIe TLP steering tag
> translates to those fields in the resulting CHI flit is largely up to
> the root complex.

Knowing next to nothing about CHI, this means pretty much nothing to me. :(

I would guess there is a bit more to supporting CHI in DT systems than
just a cache ID.

> I think it's going to be more like a "reg" property than a nice
> validatable index.
>
> >> That said, I think it does make sense to have some kind of
> >> auto-generated fallback scheme *as well*, since I'm sure there will be
> >> plenty systems which care about MPAM but don't support stashing, and
> >> therefore wouldn't have a meaningful set of IDs to populate their DT
> >> with. Conversely I think that might also matter for ACPI too - one point
> >> I remember from previous discussions is that PPTT may use a compact
> >> representation where a single entry represents all equivalent caches at
> >> that level, so I'm not sure we can necessarily rely on IDs out of that
> >> path being unique either.
> >
> > AIUI, cache ids break the compact representation.
>
> Right, firmware authors can't use it if they do want to specify IDs, but
> that also means that if we find we *are* consuming a compact PPTT, then
> chances are we're not getting meaningful IDs out of it for MPAM to rely on.

Sounds like broken firmware is in our future. ;) Or ACPI can default
to the same id scheme.

Rob

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:35         ` Rob Herring
@ 2021-12-17 20:22           ` Jeremy Linton
  2021-12-17 21:13           ` Robin Murphy
  1 sibling, 0 replies; 18+ messages in thread
From: Jeremy Linton @ 2021-12-17 20:22 UTC (permalink / raw)
  To: Rob Herring, Robin Murphy
  Cc: Greg Kroah-Hartman, James Morse, linux-arm-kernel, linux-kernel,
	Sudeep Holla, Rafael J. Wysocki, Len Brown, devicetree,
	open list:ACPI FOR ARM64 (ACPI/arm64)

Hi,

On 12/17/21 13:35, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 1:08 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-12-17 18:14, Rob Herring wrote:
>>> On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> Hi Rob,
>>>>
>>>> On 2021-12-16 23:31, Rob Herring wrote:
>>>>> Use the minimum CPU h/w id of the CPUs associated with the cache for the
>>>>> cache 'id'. This will provide a stable id value for a given system. As
>>>>> we need to check all possible CPUs, we can't use the shared_cpu_map
>>>>> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
>>>>> we have to walk all CPU nodes and then walk cache levels.
>>>>
>>>> I believe another expected use of the cache ID exposed in sysfs is to
>>>> program steering tags for cache stashing (typically in VFIO-based
>>>> userspace drivers like DPDK so we can't realistically mediate it any
>>>> other way). There were plans afoot last year to ensure that ACPI PPTT
>>>> could provide the necessary ID values for arm64 systems which will
>>>> typically be fairly arbitrary (but unique) due to reflecting underlying
>>>> interconnect routing IDs. Assuming that there will eventually be some
>>>> interest in cache stashing on DT-based systems too, we probably want to
>>>> allow for an explicit ID property on DT cache nodes in a similar manner.
>>>
>>> If you have a suggestion for ID values that correspond to the h/w,
>>> then we can add them. I'd like a bit more than just trusting that ID
>>> is something real.
>>>
>>> While the ACPI folks may be willing to take an arbitrary index, it's
>>> something we (mostly) avoid for DT.
>>
>> Not really. On the CHI side there are two fields - StashNID, which could
>> be any node ID value depending on the interconnect layout, plus
>> (optionally) StashLPID to address a specific cache within that node if
>> it's something like a CPU cluster. However, how a PCIe TLP steering tag
>> translates to those fields in the resulting CHI flit is largely up to
>> the root complex.
> 
> Knowing next to nothing about CHI, this means pretty much nothing to me. :(
> 
> I would guess there is a bit more to supporting CHI in DT systems than
> just a cache ID.
> 
>> I think it's going to be more like a "reg" property than a nice
>> validatable index.
>>
>>>> That said, I think it does make sense to have some kind of
>>>> auto-generated fallback scheme *as well*, since I'm sure there will be
>>>> plenty systems which care about MPAM but don't support stashing, and
>>>> therefore wouldn't have a meaningful set of IDs to populate their DT
>>>> with. Conversely I think that might also matter for ACPI too - one point
>>>> I remember from previous discussions is that PPTT may use a compact
>>>> representation where a single entry represents all equivalent caches at
>>>> that level, so I'm not sure we can necessarily rely on IDs out of that
>>>> path being unique either.
>>>
>>> AIUI, cache ids break the compact representation.
>>
>> Right, firmware authors can't use it if they do want to specify IDs, but
>> that also means that if we find we *are* consuming a compact PPTT, then
>> chances are we're not getting meaningful IDs out of it for MPAM to rely on.
> 
> Sounds like broken firmware is in our future. ;) Or ACPI can default
> to the same id scheme.
> 

Yah, that is a problem. The ID's provided by the ACPI cache ID field are 
as officially meaningless as the ones we can generate from the existing 
fw_token mechanism. Given that, they don't really add anything beyond 
what we can achieve simply by encoding the level somewhere in the 
fw_token currently in use if we want something that is globally unique 
rather than just unique for a given cache level+I/D. Their one advantage 
though is that they can be more human readable at the cost of 2-3X the 
size of the table, with the additional problem of having to worry about 
them being populated in all the cache structures in the table. Its 
almost easier to revisit some of the earlier discussion and generate a 
uniq id, and then renumber them at the end.


If you want to encode some kind of routing ID in them, then that will 
have to be standardized, and I would guess it might be easier to add the 
routing ID's to the structure than retroactively add meaning to the ID 
field if anyone is actually using it. Or just create yet another lookup 
table to translate the id to something meaningful.








^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:26         ` Rob Herring
@ 2021-12-17 20:28           ` Jeremy Linton
  0 siblings, 0 replies; 18+ messages in thread
From: Jeremy Linton @ 2021-12-17 20:28 UTC (permalink / raw)
  To: Rob Herring, Sudeep Holla
  Cc: Robin Murphy, Greg Kroah-Hartman, James Morse, linux-arm-kernel,
	linux-kernel, Rafael J. Wysocki, Len Brown, devicetree,
	open list:ACPI FOR ARM64 (ACPI/arm64)

Hi,

On 12/17/21 13:26, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 1:03 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On Fri, Dec 17, 2021 at 12:14:22PM -0600, Rob Herring wrote:
>>> On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> Hi Rob,
>>>>
>>>> On 2021-12-16 23:31, Rob Herring wrote:
>>>>> Use the minimum CPU h/w id of the CPUs associated with the cache for the
>>>>> cache 'id'. This will provide a stable id value for a given system. As
>>
>> I am trying to follow the code. IIUC, the level one(I$ and D$) are skipped
>> in this logic and the private unified cache if any will get the cpu hwid as
>> the cache id which is all fine. But what happens if there are 2 levels of
>> unified private cache ? I am assuming we only care about shared caches for
>> MPAM and ignore private caches which sounds OK but I just wanted to confirm.
> 
> The cacheinfo 'id' is only unique to the level and type. It's the
> type, level, and ID that gives a unique identifier:
> 
>   * struct cacheinfo - represent a cache leaf node
>   * @id: This cache's id. It is unique among caches with the same (type, level).
> 
> Maybe ACPI's ID expects/allows globally unique cache IDs?

Yes, but the spec is IMHO written in a way that they may only be unique 
for a subset of the caches! The rest might not have an ID at all, 
particularly for !arm machines.


> 
>>>>> we need to check all possible CPUs, we can't use the shared_cpu_map
>>>>> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
>>>>> we have to walk all CPU nodes and then walk cache levels.
>>
>> I would have preferred to add the cache IDs in DT similar to ACPI but I see
>> you have certain concerns with that which are valid as well.
>>
>>>>
>>>> I believe another expected use of the cache ID exposed in sysfs is to
>>>> program steering tags for cache stashing (typically in VFIO-based
>>>> userspace drivers like DPDK so we can't realistically mediate it any
>>>> other way). There were plans afoot last year to ensure that ACPI PPTT
>>>> could provide the necessary ID values for arm64 systems which will
>>>> typically be fairly arbitrary (but unique) due to reflecting underlying
>>>> interconnect routing IDs. Assuming that there will eventually be some
>>>> interest in cache stashing on DT-based systems too, we probably want to
>>>> allow for an explicit ID property on DT cache nodes in a similar manner.
>>>
>>> If you have a suggestion for ID values that correspond to the h/w,
>>> then we can add them. I'd like a bit more than just trusting that ID
>>> is something real.
>>>
>>
>> I agree, probably architecture must do better job at defining these. But
>> generated IDs IMO might cause issues especial if we have to change the
>> logic without breaking the backward compatibility.
>>
>>> While the ACPI folks may be willing to take an arbitrary index, it's
>>> something we (mostly) avoid for DT.
>>>
>>
>> Not sure if we can call that *arbitrary* 😄, in that case we can imagine
>> the same at several places in the firmware.
> 
> By arbitrary, I mean made up by the binding/dts author or
> documentation convention (UART0, UART1, etc.). Certainly things like
> clock IDs are often made up number spaces, but I don't see how we
> avoid that. DT had 'cell-index' which I still see attempted. But that
> property traces back to h/w having a single power ctrl register and
> cell-index was the bit index for the register. If only h/w was still
> that simple.
> 
> Rob
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data
  2021-12-17 19:35         ` Rob Herring
  2021-12-17 20:22           ` Jeremy Linton
@ 2021-12-17 21:13           ` Robin Murphy
  1 sibling, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2021-12-17 21:13 UTC (permalink / raw)
  To: Rob Herring
  Cc: Greg Kroah-Hartman, James Morse, Jeremy Linton, linux-arm-kernel,
	linux-kernel, Sudeep Holla, Rafael J. Wysocki, Len Brown,
	devicetree, open list:ACPI FOR ARM64 (ACPI/arm64)

On 2021-12-17 19:35, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 1:08 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-12-17 18:14, Rob Herring wrote:
>>> On Fri, Dec 17, 2021 at 10:57 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> Hi Rob,
>>>>
>>>> On 2021-12-16 23:31, Rob Herring wrote:
>>>>> Use the minimum CPU h/w id of the CPUs associated with the cache for the
>>>>> cache 'id'. This will provide a stable id value for a given system. As
>>>>> we need to check all possible CPUs, we can't use the shared_cpu_map
>>>>> which is just online CPUs. There's not a cache to CPUs mapping in DT, so
>>>>> we have to walk all CPU nodes and then walk cache levels.
>>>>
>>>> I believe another expected use of the cache ID exposed in sysfs is to
>>>> program steering tags for cache stashing (typically in VFIO-based
>>>> userspace drivers like DPDK so we can't realistically mediate it any
>>>> other way). There were plans afoot last year to ensure that ACPI PPTT
>>>> could provide the necessary ID values for arm64 systems which will
>>>> typically be fairly arbitrary (but unique) due to reflecting underlying
>>>> interconnect routing IDs. Assuming that there will eventually be some
>>>> interest in cache stashing on DT-based systems too, we probably want to
>>>> allow for an explicit ID property on DT cache nodes in a similar manner.
>>>
>>> If you have a suggestion for ID values that correspond to the h/w,
>>> then we can add them. I'd like a bit more than just trusting that ID
>>> is something real.
>>>
>>> While the ACPI folks may be willing to take an arbitrary index, it's
>>> something we (mostly) avoid for DT.
>>
>> Not really. On the CHI side there are two fields - StashNID, which could
>> be any node ID value depending on the interconnect layout, plus
>> (optionally) StashLPID to address a specific cache within that node if
>> it's something like a CPU cluster. However, how a PCIe TLP steering tag
>> translates to those fields in the resulting CHI flit is largely up to
>> the root complex.
> 
> Knowing next to nothing about CHI, this means pretty much nothing to me. :(
> 
> I would guess there is a bit more to supporting CHI in DT systems than
> just a cache ID.

I use CHI as an example because it's what I'm familiar with, and my 
involvement in cache stashing discussions has been in the context of Arm 
CMN interconnects which are CHI-based. Other folks who build their own 
interconnects may have different details of how exactly they support 
cache stashing, but the overall point is that the required IDs are 
typically going to boil down to some amount (likely around 8-16 bits or 
so) of address-like information in a system-specific format which can't 
be reasoned about beyond that.

>> I think it's going to be more like a "reg" property than a nice
>> validatable index.
>>
>>>> That said, I think it does make sense to have some kind of
>>>> auto-generated fallback scheme *as well*, since I'm sure there will be
>>>> plenty systems which care about MPAM but don't support stashing, and
>>>> therefore wouldn't have a meaningful set of IDs to populate their DT
>>>> with. Conversely I think that might also matter for ACPI too - one point
>>>> I remember from previous discussions is that PPTT may use a compact
>>>> representation where a single entry represents all equivalent caches at
>>>> that level, so I'm not sure we can necessarily rely on IDs out of that
>>>> path being unique either.
>>>
>>> AIUI, cache ids break the compact representation.
>>
>> Right, firmware authors can't use it if they do want to specify IDs, but
>> that also means that if we find we *are* consuming a compact PPTT, then
>> chances are we're not getting meaningful IDs out of it for MPAM to rely on.
> 
> Sounds like broken firmware is in our future. ;) Or ACPI can default
> to the same id scheme.

I don't really see this being an opportunity for firmware to be any more 
broken than usual. Systems that support cache stashing will need to 
provide the correct hardware IDs for targetable caches via their 
firmware tables, which it seems that MPAM's notion of cache IDs will 
have to coexist with. Systems that do not support cache stashing may not 
even have a meaningful notion of hardware IDs for caches, and thus 
cannot be expected to provide any in firmware. Linux will need to cope 
with both situations.

Thanks,
Robin.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support
  2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
                   ` (5 preceding siblings ...)
  2021-12-16 23:31 ` [PATCH 6/6] cacheinfo: Add cacheinfo_get_cache_affinity() function Rob Herring
@ 2021-12-21  9:31 ` Greg Kroah-Hartman
  6 siblings, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-21  9:31 UTC (permalink / raw)
  To: Rob Herring
  Cc: James Morse, Jeremy Linton, linux-arm-kernel, linux-kernel,
	Sudeep Holla, Rafael J. Wysocki, Len Brown, devicetree,
	linux-acpi

On Thu, Dec 16, 2021 at 05:31:19PM -0600, Rob Herring wrote:
> For upcoming Arm MPAM support in resctrl, it is necessary to have the
> cacheinfo 'id' for MPAM enabled caches. The 'id' is part of the resctrl
> ABI. While this support already exists for ACPI based systems, it is
> missing for DT. This series adds the support.
> 
> The 'id' value used is the smallest CPU h/w id value associated with a
> cache. This requires walking the cache hierarchy from every CPU node to
> get all CPUs associated with a cache. As MPAM also needs to know this,
> the CPU affinity is also saved to avoid reimplementing and walking the
> firmware tables again.
> 
> Patches 1 and 2 are v2 from the prior series[1]. The rest are new.
> 
> Tested on arm64 with DT. ACPI changes are untested. I don't have a
> system with an appropriate PPTT nor do I know how to modify ACPI tables.
> 
> Rob
> 
> [1] https://lore.kernel.org/all/20211006164332.1981454-1-robh@kernel.org/
> 
> Rob Herring (6):
>   cacheinfo: Allow for >32-bit cache 'id'
>   cacheinfo: Set cache 'id' based on DT data
>   cacheinfo: Add cpu_affinity_map to store affinity for all CPUs
>   ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map
>   cacheinfo: Use cpu_affinity_map for populating shared_cpu_map
>   cacheinfo: Add cacheinfo_get_cache_affinity() function
> 
>  drivers/acpi/pptt.c       | 29 +++++++++++++++--
>  drivers/base/cacheinfo.c  | 65 ++++++++++++++++++++++++++-------------
>  include/linux/cacheinfo.h | 29 +++++++++++++++--
>  3 files changed, 97 insertions(+), 26 deletions(-)
> 
> -- 
> 2.32.0
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-12-21  9:31 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 23:31 [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Rob Herring
2021-12-16 23:31 ` [PATCH 1/6] cacheinfo: Allow for >32-bit cache 'id' Rob Herring
2021-12-16 23:31 ` [PATCH 2/6] cacheinfo: Set cache 'id' based on DT data Rob Herring
2021-12-17 16:57   ` Robin Murphy
2021-12-17 18:14     ` Rob Herring
2021-12-17 19:03       ` Sudeep Holla
2021-12-17 19:08         ` Sudeep Holla
2021-12-17 19:26         ` Rob Herring
2021-12-17 20:28           ` Jeremy Linton
2021-12-17 19:08       ` Robin Murphy
2021-12-17 19:35         ` Rob Herring
2021-12-17 20:22           ` Jeremy Linton
2021-12-17 21:13           ` Robin Murphy
2021-12-16 23:31 ` [PATCH 3/6] cacheinfo: Add cpu_affinity_map to store affinity for all CPUs Rob Herring
2021-12-16 23:31 ` [PATCH 4/6] ACPI / PPTT: Populate the cacheinfo.cpu_affinity_map Rob Herring
2021-12-16 23:31 ` [PATCH 5/6] cacheinfo: Use cpu_affinity_map for populating shared_cpu_map Rob Herring
2021-12-16 23:31 ` [PATCH 6/6] cacheinfo: Add cacheinfo_get_cache_affinity() function Rob Herring
2021-12-21  9:31 ` [PATCH 0/6] cacheinfo: CPU affinity and Devicetree 'id' support Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).