[PATCH v4 0/4] cacheinfo: Correctly fallback to using clidr_el1's information

[PATCH v4 0/4] cacheinfo: Correctly fallback to using clidr_el1's information

[Pierre Gondois]

v4:
arch_topology: Remove early cacheinfo error message:
- Only remove the error message if the error code is -ENOENT
cacheinfo: Add use_arch[|_cache]_info field/function:
- Use a static variable instead of a per-leaf 'use_arch_info'
- Reformat the use_arch_cache_info() define

v3:
cacheinfo: Check sib_leaf in cache_leaves_are_shared():
- Reformulate commit message
- Fix rebase issue and move '&&' condition which was in the last patch
  to this patch.
cacheinfo: Add use_arch[|_cache]_info field/function:
- Put the function declaration in one line.
arch_topology: Remove early cacheinfo error message:
- New patch.

v2:
cacheinfo: Check sib_leaf in cache_leaves_are_shared()
- Reformulate commit message
- Add 'Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers [...]'
cacheinfo: Check cache properties are present in DT
- Use of_property_present()
- Add 'Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>'
cacheinfo: Add use_arch[|_cache]_info field/function:
- Make use_arch_cache_info() a static inline function

The cache information can be extracted from either a Device
Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
for arm64).

When the DT is used but no cache properties are advertised,
the current code doesn't correctly fallback to using arch information.

Correct this. Also use the assumption that L1 data/instruction caches
are private and L2/higher caches are shared when the cache information
is coming form clidr_el1.

As suggested by Alexandre, this serie should ideally go to 6.3 fixes.

Pierre Gondois (4):
  cacheinfo: Check sib_leaf in cache_leaves_are_shared()
  cacheinfo: Check cache properties are present in DT
  arch_topology: Remove early cacheinfo error message if -ENOENT
  cacheinfo: Add use_arch[|_cache]_info field/function

 drivers/base/arch_topology.c |  7 +++---
 drivers/base/cacheinfo.c     | 49 ++++++++++++++++++++++++++++++++----
 include/linux/cacheinfo.h    |  6 +++++
 3 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Pierre Gondois]

If there is no ACPI/DT information, it is assumed that L1 caches
are private and L2 (and higher) caches are shared. A cache is
'shared' between two CPUs if it is accessible from these two
CPUs.

Each CPU owns a representation (i.e. has a dedicated cacheinfo struct)
of the caches it has access to. cache_leaves_are_shared() tries to
identify whether two representations are designating the same actual
cache.

In cache_leaves_are_shared(), if 'this_leaf' is a L2 cache (or higher)
and 'sib_leaf' is a L1 cache, the caches are detected as shared as
only this_leaf's cache level is checked.
This is leads to setting sib_leaf as being shared with another CPU,
which is incorrect as this is a L1 cache.

Check 'sib_leaf->level'. Also update the comment as the function is
called when populating 'shared_cpu_map'.

Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/base/cacheinfo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index f3903d002819..c5d2293ac2a6 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -38,11 +38,10 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_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
+	 * system-wide shared caches for all other levels.
 	 */
 	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
-		return !(this_leaf->level == 1);
+		return (this_leaf->level != 1) && (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
 	    (this_leaf->attributes & CACHE_ID))
-- 
2.25.1

[PATCH v4 2/4] cacheinfo: Check cache properties are present in DT

[Pierre Gondois]

If a Device Tree (DT) is used, the presence of cache properties is
assumed. Not finding any is not considered. For arm64 platforms,
cache information can be fetched from the clidr_el1 register.
Checking whether cache information is available in the DT
allows to switch to using clidr_el1.

init_of_cache_level()
\-of_count_cache_leaves()
will assume there a 2 cache leaves (L1 data/instruction caches), which
can be different from clidr_el1 information.

cache_setup_of_node() tries to read cache properties in the DT.
If there are none, this is considered a success. Knowing no
information was available would allow to switch to using clidr_el1.

Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Link: https://lore.kernel.org/all/20230404-hatred-swimmer-6fecdf33b57a@spud/
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/base/cacheinfo.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index c5d2293ac2a6..ea8f416852bd 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -78,6 +78,9 @@ bool last_level_cache_is_shared(unsigned int cpu_x, unsigned int cpu_y)
 }
 
 #ifdef CONFIG_OF
+
+static bool of_check_cache_nodes(struct device_node *np);
+
 /* OF properties to query for a given cache type */
 struct cache_type_info {
 	const char *size_prop;
@@ -205,6 +208,11 @@ static int cache_setup_of_node(unsigned int cpu)
 		return -ENOENT;
 	}
 
+	if (!of_check_cache_nodes(np)) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+
 	prev = np;
 
 	while (index < cache_leaves(cpu)) {
@@ -229,6 +237,25 @@ static int cache_setup_of_node(unsigned int cpu)
 	return 0;
 }
 
+static bool of_check_cache_nodes(struct device_node *np)
+{
+	struct device_node *next;
+
+	if (of_property_present(np, "cache-size")   ||
+	    of_property_present(np, "i-cache-size") ||
+	    of_property_present(np, "d-cache-size") ||
+	    of_property_present(np, "cache-unified"))
+		return true;
+
+	next = of_find_next_cache_node(np);
+	if (next) {
+		of_node_put(next);
+		return true;
+	}
+
+	return false;
+}
+
 static int of_count_cache_leaves(struct device_node *np)
 {
 	unsigned int leaves = 0;
@@ -260,6 +287,11 @@ int init_of_cache_level(unsigned int cpu)
 	struct device_node *prev = NULL;
 	unsigned int levels = 0, leaves, level;
 
+	if (!of_check_cache_nodes(np)) {
+		of_node_put(np);
+		return -ENOENT;
+	}
+
 	leaves = of_count_cache_leaves(np);
 	if (leaves > 0)
 		levels = 1;
-- 
2.25.1

[PATCH v4 3/4] arch_topology: Remove early cacheinfo error message if -ENOENT

[Pierre Gondois]

fetch_cache_info() tries to get the number of cache leaves/levels
for each CPU in order to pre-allocate memory for cacheinfo struct.
Allocating this memory later triggers a:
  'BUG: sleeping function called from invalid context'
in PREEMPT_RT kernels.

If there is no cache related information available in DT or ACPI,
fetch_cache_info() fails and an error message is printed:
  'Early cacheinfo failed, ret = ...'

Not having cache information should be a valid configuration.
Remove the error message if fetch_cache_info() fails with -ENOENT.

Suggested-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/all/20230404-hatred-swimmer-6fecdf33b57a@spud/
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/base/arch_topology.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index b1c1dd38ab01..c4b6198d7461 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -843,10 +843,11 @@ void __init init_cpu_topology(void)
 
 	for_each_possible_cpu(cpu) {
 		ret = fetch_cache_info(cpu);
-		if (ret) {
+		if (!ret)
+			continue;
+		else if (ret != -ENOENT)
 			pr_err("Early cacheinfo failed, ret = %d\n", ret);
-			break;
-		}
+		return;
 	}
 }
 
-- 
2.25.1

[PATCH v4 4/4] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

The cache information can be extracted from either a Device
Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
for arm64).

The clidr_el1 register is used only if DT/ACPI information is not
available. It does not states how caches are shared among CPUs.

Add a use_arch_cache_info field/function to identify when the
DT/ACPI doesn't provide cache information. Use this information
to assume L1 caches are privates and L2 and higher are shared among
all CPUs.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c  | 12 ++++++++++--
 include/linux/cacheinfo.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index ea8f416852bd..8120ac1ddbe4 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -28,6 +28,9 @@ static DEFINE_PER_CPU(struct cpu_cacheinfo, ci_cpu_cacheinfo);
 #define per_cpu_cacheinfo_idx(cpu, idx)		\
 				(per_cpu_cacheinfo(cpu) + (idx))
 
+/* Set if no cache information is found in DT/ACPI. */
+static bool use_arch_info;
+
 struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu)
 {
 	return ci_cacheinfo(cpu);
@@ -40,7 +43,8 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
 	 * For non DT/ACPI systems, assume unique level 1 caches,
 	 * system-wide shared caches for all other levels.
 	 */
-	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)))
+	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
+	    use_arch_info)
 		return (this_leaf->level != 1) && (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
@@ -343,6 +347,10 @@ static int cache_setup_properties(unsigned int cpu)
 	else if (!acpi_disabled)
 		ret = cache_setup_acpi(cpu);
 
+	// Assume there is no cache information available in DT/ACPI from now.
+	if (ret && use_arch_cache_info())
+		use_arch_info = true;
+
 	return ret;
 }
 
@@ -361,7 +369,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	 * to update the shared cpu_map if the cache attributes were
 	 * populated early before all the cpus are brought online
 	 */
-	if (!last_level_cache_is_valid(cpu)) {
+	if (!last_level_cache_is_valid(cpu) && !use_arch_info) {
 		ret = cache_setup_properties(cpu);
 		if (ret)
 			return ret;
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 908e19d17f49..b91cc9991c7c 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -129,4 +129,10 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)
 	return -1;
 }
 
+#ifdef CONFIG_ARM64
+#define use_arch_cache_info()	(true)
+#else
+#define use_arch_cache_info()	(false)
+#endif
+
 #endif /* _LINUX_CACHEINFO_H */
-- 
2.25.1

Re: [PATCH v4 0/4] cacheinfo: Correctly fallback to using clidr_el1's information

[Florian Fainelli]

On 4/14/23 01:14, Pierre Gondois wrote:
> v4:
> arch_topology: Remove early cacheinfo error message:
> - Only remove the error message if the error code is -ENOENT
> cacheinfo: Add use_arch[|_cache]_info field/function:
> - Use a static variable instead of a per-leaf 'use_arch_info'
> - Reformat the use_arch_cache_info() define
> 
> v3:
> cacheinfo: Check sib_leaf in cache_leaves_are_shared():
> - Reformulate commit message
> - Fix rebase issue and move '&&' condition which was in the last patch
>    to this patch.
> cacheinfo: Add use_arch[|_cache]_info field/function:
> - Put the function declaration in one line.
> arch_topology: Remove early cacheinfo error message:
> - New patch.
> 
> v2:
> cacheinfo: Check sib_leaf in cache_leaves_are_shared()
> - Reformulate commit message
> - Add 'Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers [...]'
> cacheinfo: Check cache properties are present in DT
> - Use of_property_present()
> - Add 'Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>'
> cacheinfo: Add use_arch[|_cache]_info field/function:
> - Make use_arch_cache_info() a static inline function
> 
> The cache information can be extracted from either a Device
> Tree (DT), the PPTT ACPI table, or arch registers (clidr_el1
> for arm64).
> 
> When the DT is used but no cache properties are advertised,
> the current code doesn't correctly fallback to using arch information.
> 
> Correct this. Also use the assumption that L1 data/instruction caches
> are private and L2/higher caches are shared when the cache information
> is coming form clidr_el1.
> 
> As suggested by Alexandre, this serie should ideally go to 6.3 fixes.

FWIW:

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian

Re: [PATCH v4 0/4] cacheinfo: Correctly fallback to using clidr_el1's information

[Sudeep Holla]

On Fri, 14 Apr 2023 10:14:48 +0200, Pierre Gondois wrote:
> v4:
> arch_topology: Remove early cacheinfo error message:
> - Only remove the error message if the error code is -ENOENT
> cacheinfo: Add use_arch[|_cache]_info field/function:
> - Use a static variable instead of a per-leaf 'use_arch_info'
> - Reformat the use_arch_cache_info() define
> 
> [...]

Applied to sudeep.holla/linux (for-next/cacheinfo), thanks!

[1/4] cacheinfo: Check sib_leaf in cache_leaves_are_shared()
      https://git.kernel.org/sudeep.holla/c/7a306e3eabf2
[2/4] cacheinfo: Check cache properties are present in DT
      https://git.kernel.org/sudeep.holla/c/cde0fbff07ef
[3/4] arch_topology: Remove early cacheinfo error message if -ENOENT
      https://git.kernel.org/sudeep.holla/c/3522340199cc
[4/4] cacheinfo: Add use_arch[|_cache]_info field/function
      https://git.kernel.org/sudeep.holla/c/ef9f643a9f8b

--
Regards,
Sudeep