2023-03-27 12:04:06

by Pierre Gondois

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

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


2023-03-27 12:04:07

by Pierre Gondois

[permalink] [raw]
Subject: [PATCH 2/3] cacheinfo: Check cache properties are present in DT

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 <[email protected]>
---
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

2023-03-27 14:08:24

by Conor Dooley

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

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 <[email protected]>

> +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


Attachments:
(No filename) (1.39 kB)
signature.asc (235.00 B)
Download all attachments

2023-04-04 19:32:55

by Conor Dooley

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

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 <[email protected]>
since he's found an actual, rather than theoretical, problem!

Cheers,
Conor.

> Signed-off-by: Pierre Gondois <[email protected]>
> ---
> 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
>


Attachments:
(No filename) (4.12 kB)
signature.asc (235.00 B)
Download all attachments

2023-04-06 07:40:02

by Pierre Gondois

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

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 <[email protected]>
> 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.
>