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

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

[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).

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.

Pierre Gondois (3):
  cacheinfo: Check sib_leaf in cache_leaves_are_shared()
  cacheinfo: Check cache properties are present in DT
  cacheinfo: Add use_arch[|_cache]_info field/function

 arch/arm64/kernel/cacheinfo.c |  5 ++++
 drivers/base/cacheinfo.c      | 53 ++++++++++++++++++++++++++++++++---
 include/linux/cacheinfo.h     |  2 ++
 3 files changed, 56 insertions(+), 4 deletions(-)

-- 
2.25.1

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

[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).

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.

Pierre Gondois (3):
  cacheinfo: Check sib_leaf in cache_leaves_are_shared()
  cacheinfo: Check cache properties are present in DT
  cacheinfo: Add use_arch[|_cache]_info field/function

 arch/arm64/kernel/cacheinfo.c |  5 ++++
 drivers/base/cacheinfo.c      | 53 ++++++++++++++++++++++++++++++++---
 include/linux/cacheinfo.h     |  2 ++
 3 files changed, 56 insertions(+), 4 deletions(-)

-- 
2.25.1


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

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

[Pierre Gondois]

If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
the caches are detected as shared.
Indeed, cache_leaves_are_shared() only checks the cache level of
'this_leaf' when 'sib_leaf''s cache level should also be checked.

Also update the comment: the function is called when populating
'shared_cpu_map'.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.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 f6573c335f4c..4ca117574af1 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 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Pierre Gondois]

If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
the caches are detected as shared.
Indeed, cache_leaves_are_shared() only checks the cache level of
'this_leaf' when 'sib_leaf''s cache level should also be checked.

Also update the comment: the function is called when populating
'shared_cpu_map'.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.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 f6573c335f4c..4ca117574af1 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


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

[PATCH 2/3] 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.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 4ca117574af1..5b0edf2d5da8 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_read_bool(np, "cache-size")   ||
+	    of_property_read_bool(np, "i-cache-size") ||
+	    of_property_read_bool(np, "d-cache-size") ||
+	    of_property_read_bool(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,9 @@ 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))
+		goto err_out;
+
 	leaves = of_count_cache_leaves(np);
 	if (leaves > 0)
 		levels = 1;
-- 
2.25.1

[PATCH 2/3] 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.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 4ca117574af1..5b0edf2d5da8 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_read_bool(np, "cache-size")   ||
+	    of_property_read_bool(np, "i-cache-size") ||
+	    of_property_read_bool(np, "d-cache-size") ||
+	    of_property_read_bool(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,9 @@ 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))
+		goto err_out;
+
 	leaves = of_count_cache_leaves(np);
 	if (leaves > 0)
 		levels = 1;
-- 
2.25.1


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

[PATCH 3/3] 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>
---
 arch/arm64/kernel/cacheinfo.c |  5 +++++
 drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
 include/linux/cacheinfo.h     |  2 ++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index c307f69e9b55..b6306cda0fa7 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
 	}
 	return 0;
 }
+
+bool use_arch_cache_info(unsigned int cpu)
+{
+	return true;
+}
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 5b0edf2d5da8..c6266ccc2df5 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -40,8 +40,9 @@ 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)))
-		return (this_leaf->level != 1) || (sib_leaf->level != 1);
+	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
+	    this_leaf->use_arch_info)
+		return (this_leaf->level != 1) && (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
 	    (this_leaf->attributes & CACHE_ID))
@@ -330,6 +331,11 @@ int __weak cache_setup_acpi(unsigned int cpu)
 	return -ENOTSUPP;
 }
 
+bool __weak use_arch_cache_info(unsigned int cpu)
+{
+	return false;
+}
+
 unsigned int coherency_max_size;
 
 static int cache_setup_properties(unsigned int cpu)
@@ -349,6 +355,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
 	struct cacheinfo *this_leaf, *sib_leaf;
 	unsigned int index, sib_index;
+	bool use_arch_info = false;
 	int ret = 0;
 
 	if (this_cpu_ci->cpu_map_populated)
@@ -361,6 +368,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	 */
 	if (!last_level_cache_is_valid(cpu)) {
 		ret = cache_setup_properties(cpu);
+		// Possibility to rely on arch specific information.
+		if (ret && use_arch_cache_info(cpu)) {
+			use_arch_info = true;
+			ret = 0;
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -370,6 +383,9 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 
 		this_leaf = per_cpu_cacheinfo_idx(cpu, index);
 
+		if (use_arch_info)
+			this_leaf->use_arch_info = true;
+
 		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
 		for_each_online_cpu(i) {
 			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 908e19d17f49..bcbb8bd5759a 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -66,6 +66,7 @@ struct cacheinfo {
 #define CACHE_ALLOCATE_POLICY_MASK	\
 	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
 #define CACHE_ID		BIT(4)
+	bool use_arch_info;
 	void *fw_token;
 	bool disable_sysfs;
 	void *priv;
@@ -82,6 +83,7 @@ struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
 int init_cache_level(unsigned int cpu);
 int init_of_cache_level(unsigned int cpu);
 int populate_cache_leaves(unsigned int cpu);
+bool use_arch_cache_info(unsigned int cpu);
 int cache_setup_acpi(unsigned int cpu);
 bool last_level_cache_is_valid(unsigned int cpu);
 bool last_level_cache_is_shared(unsigned int cpu_x, unsigned int cpu_y);
-- 
2.25.1

[PATCH 3/3] 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>
---
 arch/arm64/kernel/cacheinfo.c |  5 +++++
 drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
 include/linux/cacheinfo.h     |  2 ++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index c307f69e9b55..b6306cda0fa7 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
 	}
 	return 0;
 }
+
+bool use_arch_cache_info(unsigned int cpu)
+{
+	return true;
+}
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 5b0edf2d5da8..c6266ccc2df5 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -40,8 +40,9 @@ 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)))
-		return (this_leaf->level != 1) || (sib_leaf->level != 1);
+	if (!(IS_ENABLED(CONFIG_OF) || IS_ENABLED(CONFIG_ACPI)) ||
+	    this_leaf->use_arch_info)
+		return (this_leaf->level != 1) && (sib_leaf->level != 1);
 
 	if ((sib_leaf->attributes & CACHE_ID) &&
 	    (this_leaf->attributes & CACHE_ID))
@@ -330,6 +331,11 @@ int __weak cache_setup_acpi(unsigned int cpu)
 	return -ENOTSUPP;
 }
 
+bool __weak use_arch_cache_info(unsigned int cpu)
+{
+	return false;
+}
+
 unsigned int coherency_max_size;
 
 static int cache_setup_properties(unsigned int cpu)
@@ -349,6 +355,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
 	struct cacheinfo *this_leaf, *sib_leaf;
 	unsigned int index, sib_index;
+	bool use_arch_info = false;
 	int ret = 0;
 
 	if (this_cpu_ci->cpu_map_populated)
@@ -361,6 +368,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	 */
 	if (!last_level_cache_is_valid(cpu)) {
 		ret = cache_setup_properties(cpu);
+		// Possibility to rely on arch specific information.
+		if (ret && use_arch_cache_info(cpu)) {
+			use_arch_info = true;
+			ret = 0;
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -370,6 +383,9 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 
 		this_leaf = per_cpu_cacheinfo_idx(cpu, index);
 
+		if (use_arch_info)
+			this_leaf->use_arch_info = true;
+
 		cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
 		for_each_online_cpu(i) {
 			struct cpu_cacheinfo *sib_cpu_ci = get_cpu_cacheinfo(i);
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 908e19d17f49..bcbb8bd5759a 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -66,6 +66,7 @@ struct cacheinfo {
 #define CACHE_ALLOCATE_POLICY_MASK	\
 	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
 #define CACHE_ID		BIT(4)
+	bool use_arch_info;
 	void *fw_token;
 	bool disable_sysfs;
 	void *priv;
@@ -82,6 +83,7 @@ struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
 int init_cache_level(unsigned int cpu);
 int init_of_cache_level(unsigned int cpu);
 int populate_cache_leaves(unsigned int cpu);
+bool use_arch_cache_info(unsigned int cpu);
 int cache_setup_acpi(unsigned int cpu);
 bool last_level_cache_is_valid(unsigned int cpu);
 bool last_level_cache_is_shared(unsigned int cpu_x, unsigned int cpu_y);
-- 
2.25.1


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

Re: [PATCH 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Will Deacon]

On Mon, Mar 27, 2023 at 01:59:51PM +0200, Pierre Gondois wrote:
> 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>
> ---
>  arch/arm64/kernel/cacheinfo.c |  5 +++++
>  drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
>  include/linux/cacheinfo.h     |  2 ++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
> index c307f69e9b55..b6306cda0fa7 100644
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
>  	}
>  	return 0;
>  }
> +
> +bool use_arch_cache_info(unsigned int cpu)
> +{
> +	return true;
> +}

It would be a lot nicer if this was a static inline function in a header
rather than a weak symbol.

Will

Re: [PATCH 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Will Deacon]

On Mon, Mar 27, 2023 at 01:59:51PM +0200, Pierre Gondois wrote:
> 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>
> ---
>  arch/arm64/kernel/cacheinfo.c |  5 +++++
>  drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
>  include/linux/cacheinfo.h     |  2 ++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
> index c307f69e9b55..b6306cda0fa7 100644
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
>  	}
>  	return 0;
>  }
> +
> +bool use_arch_cache_info(unsigned int cpu)
> +{
> +	return true;
> +}

It would be a lot nicer if this was a static inline function in a header
rather than a weak symbol.

Will

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

Re: [PATCH 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

On Mon, Mar 27, 2023 at 01:59:49PM +0200, Pierre Gondois wrote:
> If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
> the caches are detected as shared.
> Indeed, cache_leaves_are_shared() only checks the cache level of
> 'this_leaf' when 'sib_leaf''s cache level should also be checked.

nit: this commit message reads quite weirdly as there's a missing "do
foo" statement, followed by "also do bar".

> 
> Also update the comment: the function is called when populating
> 'shared_cpu_map'.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.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 f6573c335f4c..4ca117574af1 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);

So this is
Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
then?

Cheers,
Conor.

>  
>  	if ((sib_leaf->attributes & CACHE_ID) &&
>  	    (this_leaf->attributes & CACHE_ID))
> -- 
> 2.25.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared()

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1680 bytes --]

On Mon, Mar 27, 2023 at 01:59:49PM +0200, Pierre Gondois wrote:
> If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
> the caches are detected as shared.
> Indeed, cache_leaves_are_shared() only checks the cache level of
> 'this_leaf' when 'sib_leaf''s cache level should also be checked.

nit: this commit message reads quite weirdly as there's a missing "do
foo" statement, followed by "also do bar".

> 
> Also update the comment: the function is called when populating
> 'shared_cpu_map'.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.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 f6573c335f4c..4ca117574af1 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);

So this is
Fixes: f16d1becf96f ("cacheinfo: Use cache identifiers to check if the caches are shared if available")
then?

Cheers,
Conor.

>  
>  	if ((sib_leaf->attributes & CACHE_ID) &&
>  	    (this_leaf->attributes & CACHE_ID))
> -- 
> 2.25.1
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
> 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.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>

> +static bool of_check_cache_nodes(struct device_node *np)
> +{
> +	struct device_node *next;
> +
> +	if (of_property_read_bool(np, "cache-size")   ||
> +	    of_property_read_bool(np, "i-cache-size") ||
> +	    of_property_read_bool(np, "d-cache-size") ||
> +	    of_property_read_bool(np, "cache-unified"))
> +		return true;
> +

Rob's been purging use of the of_property_read family of functions
recently [1], should this use of_property_present() instead?

Cheers,
Conor.

1 - https://lore.kernel.org/all/?q=Use+of_property_present%28%29+for+testing+DT+property+presence+f%3Arob

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1420 bytes --]

On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
> 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.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>

> +static bool of_check_cache_nodes(struct device_node *np)
> +{
> +	struct device_node *next;
> +
> +	if (of_property_read_bool(np, "cache-size")   ||
> +	    of_property_read_bool(np, "i-cache-size") ||
> +	    of_property_read_bool(np, "d-cache-size") ||
> +	    of_property_read_bool(np, "cache-unified"))
> +		return true;
> +

Rob's been purging use of the of_property_read family of functions
recently [1], should this use of_property_present() instead?

Cheers,
Conor.

1 - https://lore.kernel.org/all/?q=Use+of_property_present%28%29+for+testing+DT+property+presence+f%3Arob

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4224 bytes --]

Hey Pierre,

On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
> 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.
>

Alex reported seeing a bunch of messages in his boot log in QEMU since
-rc1 which appears to be the fault of, as far as I can tell, e0df442ee49
("cacheinfo: Check 'cache-unified' property to count cache leaves")
like:
cacheinfo: Unable to detect cache hierarchy for CPU N

The RISC-V QEMU virt machine doesn't define any cache properties of any
sort in the dtb, and unlike the arm64 virt machine I tried (a72) doesn't
have some registers that cache info is discoverable from.
When we call of_count_cache_leaves() from init_of_cache_level() and
there are of course no reasons to increment leaves, we hit the return 2
case you mention above, setting num_leaves to 2.

As you mention, when we hit cache_setup_of_node(), levels is not going
to be set to one, so we trigger the condition (this_leaf->level != 1)
and, as there are no cache nodes, break out of the loop without
incrementing index. Index is therefore less than 2, and thus we return
-ENOENT.
This is of course propagated back out to detect_cache_attributes() and
triggers the "Unable to detect..." printout :(

With this patch(set), the spurious error prints go away, but we are left
with a "Early cacheinfo failed, ret = -22" which will need to be fixed.

So I think this also needs to be:
Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")

Probably also needs a:
Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
since he's found an actual, rather than theoretical, problem!

Cheers,
Conor.

> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 4ca117574af1..5b0edf2d5da8 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_read_bool(np, "cache-size")   ||
> +	    of_property_read_bool(np, "i-cache-size") ||
> +	    of_property_read_bool(np, "d-cache-size") ||
> +	    of_property_read_bool(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,9 @@ 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))
> +		goto err_out;
> +
>  	leaves = of_count_cache_leaves(np);
>  	if (leaves > 0)
>  		levels = 1;
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4224 bytes --]

Hey Pierre,

On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
> 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.
>

Alex reported seeing a bunch of messages in his boot log in QEMU since
-rc1 which appears to be the fault of, as far as I can tell, e0df442ee49
("cacheinfo: Check 'cache-unified' property to count cache leaves")
like:
cacheinfo: Unable to detect cache hierarchy for CPU N

The RISC-V QEMU virt machine doesn't define any cache properties of any
sort in the dtb, and unlike the arm64 virt machine I tried (a72) doesn't
have some registers that cache info is discoverable from.
When we call of_count_cache_leaves() from init_of_cache_level() and
there are of course no reasons to increment leaves, we hit the return 2
case you mention above, setting num_leaves to 2.

As you mention, when we hit cache_setup_of_node(), levels is not going
to be set to one, so we trigger the condition (this_leaf->level != 1)
and, as there are no cache nodes, break out of the loop without
incrementing index. Index is therefore less than 2, and thus we return
-ENOENT.
This is of course propagated back out to detect_cache_attributes() and
triggers the "Unable to detect..." printout :(

With this patch(set), the spurious error prints go away, but we are left
with a "Early cacheinfo failed, ret = -22" which will need to be fixed.

So I think this also needs to be:
Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")

Probably also needs a:
Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
since he's found an actual, rather than theoretical, problem!

Cheers,
Conor.

> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/base/cacheinfo.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 4ca117574af1..5b0edf2d5da8 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_read_bool(np, "cache-size")   ||
> +	    of_property_read_bool(np, "i-cache-size") ||
> +	    of_property_read_bool(np, "d-cache-size") ||
> +	    of_property_read_bool(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,9 @@ 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))
> +		goto err_out;
> +
>  	leaves = of_count_cache_leaves(np);
>  	if (leaves > 0)
>  		levels = 1;
> -- 
> 2.25.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

Hello Will,

On 3/27/23 14:17, Will Deacon wrote:
> On Mon, Mar 27, 2023 at 01:59:51PM +0200, Pierre Gondois wrote:
>> 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>
>> ---
>>   arch/arm64/kernel/cacheinfo.c |  5 +++++
>>   drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
>>   include/linux/cacheinfo.h     |  2 ++
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
>> index c307f69e9b55..b6306cda0fa7 100644
>> --- a/arch/arm64/kernel/cacheinfo.c
>> +++ b/arch/arm64/kernel/cacheinfo.c
>> @@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
>>   	}
>>   	return 0;
>>   }
>> +
>> +bool use_arch_cache_info(unsigned int cpu)
>> +{
>> +	return true;
>> +}
> 
> It would be a lot nicer if this was a static inline function in a header
> rather than a weak symbol.

I am not sure I see where the static inline function should be added.
Do you prefer to have function like the following in
include/linux/cacheinfo.h ?

static inline bool use_arch_cache_info(unsigned int cpu)
{
#if defined(CONFIG_ARM64)
	return true;
#else
	return false;
#endif
}

Regards,
Pierre

> 
> Will

Re: [PATCH 3/3] cacheinfo: Add use_arch[|_cache]_info field/function

[Pierre Gondois]

Hello Will,

On 3/27/23 14:17, Will Deacon wrote:
> On Mon, Mar 27, 2023 at 01:59:51PM +0200, Pierre Gondois wrote:
>> 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>
>> ---
>>   arch/arm64/kernel/cacheinfo.c |  5 +++++
>>   drivers/base/cacheinfo.c      | 20 ++++++++++++++++++--
>>   include/linux/cacheinfo.h     |  2 ++
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
>> index c307f69e9b55..b6306cda0fa7 100644
>> --- a/arch/arm64/kernel/cacheinfo.c
>> +++ b/arch/arm64/kernel/cacheinfo.c
>> @@ -96,3 +96,8 @@ int populate_cache_leaves(unsigned int cpu)
>>   	}
>>   	return 0;
>>   }
>> +
>> +bool use_arch_cache_info(unsigned int cpu)
>> +{
>> +	return true;
>> +}
> 
> It would be a lot nicer if this was a static inline function in a header
> rather than a weak symbol.

I am not sure I see where the static inline function should be added.
Do you prefer to have function like the following in
include/linux/cacheinfo.h ?

static inline bool use_arch_cache_info(unsigned int cpu)
{
#if defined(CONFIG_ARM64)
	return true;
#else
	return false;
#endif
}

Regards,
Pierre

> 
> Will

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

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Pierre Gondois]

Hello Conor,

On 4/4/23 21:29, Conor Dooley wrote:
> Hey Pierre,
> 
> On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
>> 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.
>>
> 
> Alex reported seeing a bunch of messages in his boot log in QEMU since
> -rc1 which appears to be the fault of, as far as I can tell, e0df442ee49
> ("cacheinfo: Check 'cache-unified' property to count cache leaves")
> like:
> cacheinfo: Unable to detect cache hierarchy for CPU N
> 
> The RISC-V QEMU virt machine doesn't define any cache properties of any
> sort in the dtb, and unlike the arm64 virt machine I tried (a72) doesn't
> have some registers that cache info is discoverable from.
> When we call of_count_cache_leaves() from init_of_cache_level() and
> there are of course no reasons to increment leaves, we hit the return 2
> case you mention above, setting num_leaves to 2.
> 
> As you mention, when we hit cache_setup_of_node(), levels is not going
> to be set to one, so we trigger the condition (this_leaf->level != 1)
> and, as there are no cache nodes, break out of the loop without
> incrementing index. Index is therefore less than 2, and thus we return
> -ENOENT.
> This is of course propagated back out to detect_cache_attributes() and
> triggers the "Unable to detect..." printout :(
> 
> With this patch(set), the spurious error prints go away, but we are left
> with a "Early cacheinfo failed, ret = -22" which will need to be fixed.
> 
> So I think this also needs to be:
> Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
> 
> Probably also needs a:
> Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> since he's found an actual, rather than theoretical, problem!

Ok yes indeed, I will do this and the other comments you made,

Regards,
Pierre

> 
> Cheers,
> Conor.
> 

Re: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

[Pierre Gondois]

Hello Conor,

On 4/4/23 21:29, Conor Dooley wrote:
> Hey Pierre,
> 
> On Mon, Mar 27, 2023 at 01:59:50PM +0200, Pierre Gondois wrote:
>> 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.
>>
> 
> Alex reported seeing a bunch of messages in his boot log in QEMU since
> -rc1 which appears to be the fault of, as far as I can tell, e0df442ee49
> ("cacheinfo: Check 'cache-unified' property to count cache leaves")
> like:
> cacheinfo: Unable to detect cache hierarchy for CPU N
> 
> The RISC-V QEMU virt machine doesn't define any cache properties of any
> sort in the dtb, and unlike the arm64 virt machine I tried (a72) doesn't
> have some registers that cache info is discoverable from.
> When we call of_count_cache_leaves() from init_of_cache_level() and
> there are of course no reasons to increment leaves, we hit the return 2
> case you mention above, setting num_leaves to 2.
> 
> As you mention, when we hit cache_setup_of_node(), levels is not going
> to be set to one, so we trigger the condition (this_leaf->level != 1)
> and, as there are no cache nodes, break out of the loop without
> incrementing index. Index is therefore less than 2, and thus we return
> -ENOENT.
> This is of course propagated back out to detect_cache_attributes() and
> triggers the "Unable to detect..." printout :(
> 
> With this patch(set), the spurious error prints go away, but we are left
> with a "Early cacheinfo failed, ret = -22" which will need to be fixed.
> 
> So I think this also needs to be:
> Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
> 
> Probably also needs a:
> Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> since he's found an actual, rather than theoretical, problem!

Ok yes indeed, I will do this and the other comments you made,

Regards,
Pierre

> 
> Cheers,
> Conor.
> 

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