[PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()

[PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()

[Yunhui Cui]

The implementation of the ci_leaf_init() function body and the caller
do not use the input parameter (struct device_node *node), so remove it.

Fixes: 6a24915145c9 ("Revert "riscv: Set more data to cacheinfo"")
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/kernel/cacheinfo.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 09e9b88110d1..30a6878287ad 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -64,7 +64,6 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
 }
 
 static void ci_leaf_init(struct cacheinfo *this_leaf,
-			 struct device_node *node,
 			 enum cache_type type, unsigned int level)
 {
 	this_leaf->level = level;
@@ -80,11 +79,11 @@ int populate_cache_leaves(unsigned int cpu)
 	int levels = 1, level = 1;
 
 	if (of_property_read_bool(np, "cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 	if (of_property_read_bool(np, "i-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 	if (of_property_read_bool(np, "d-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 
 	prev = np;
 	while ((np = of_find_next_cache_node(np))) {
@@ -97,11 +96,11 @@ int populate_cache_leaves(unsigned int cpu)
 		if (level <= levels)
 			break;
 		if (of_property_read_bool(np, "cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 		if (of_property_read_bool(np, "i-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 		if (of_property_read_bool(np, "d-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 		levels = level;
 	}
 	of_node_put(np);
-- 
2.20.1

[PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()

[Yunhui Cui]

The implementation of the ci_leaf_init() function body and the caller
do not use the input parameter (struct device_node *node), so remove it.

Fixes: 6a24915145c9 ("Revert "riscv: Set more data to cacheinfo"")
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/kernel/cacheinfo.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 09e9b88110d1..30a6878287ad 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -64,7 +64,6 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
 }
 
 static void ci_leaf_init(struct cacheinfo *this_leaf,
-			 struct device_node *node,
 			 enum cache_type type, unsigned int level)
 {
 	this_leaf->level = level;
@@ -80,11 +79,11 @@ int populate_cache_leaves(unsigned int cpu)
 	int levels = 1, level = 1;
 
 	if (of_property_read_bool(np, "cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 	if (of_property_read_bool(np, "i-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 	if (of_property_read_bool(np, "d-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 
 	prev = np;
 	while ((np = of_find_next_cache_node(np))) {
@@ -97,11 +96,11 @@ int populate_cache_leaves(unsigned int cpu)
 		if (level <= levels)
 			break;
 		if (of_property_read_bool(np, "cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 		if (of_property_read_bool(np, "i-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 		if (of_property_read_bool(np, "d-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 		levels = level;
 	}
 	of_node_put(np);
-- 
2.20.1


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

[PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Yunhui Cui]

Before cacheinfo can be built correctly, we need to initialize level
and type. Since RSIC-V currently does not have a register group that
describes cache-related attributes like ARM64, we cannot obtain them
directly, so now we obtain cache leaves from the ACPI PPTT table
(acpi_get_cache_info()) and set the cache type through split_levels.

Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 30a6878287ad..dc5fb70362f1 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -6,6 +6,7 @@
 #include <linux/cpu.h>
 #include <linux/of.h>
 #include <asm/cacheinfo.h>
+#include <linux/acpi.h>
 
 static struct riscv_cacheinfo_ops *rv_cache_ops;
 
@@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
 	struct device_node *prev = NULL;
 	int levels = 1, level = 1;
 
+	if (!acpi_disabled) {
+		int ret, idx, fw_levels, split_levels;
+
+		ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
+		if (ret)
+			return ret;
+
+		for (idx = 0; level <= this_cpu_ci->num_levels &&
+		     idx < this_cpu_ci->num_leaves; idx++, level++) {
+			if (level <= split_levels) {
+				ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
+				ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
+			} else {
+				ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
+			}
+		}
+		return 0;
+	}
+
 	if (of_property_read_bool(np, "cache-size"))
 		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 	if (of_property_read_bool(np, "i-cache-size"))
-- 
2.20.1

[PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Yunhui Cui]

Before cacheinfo can be built correctly, we need to initialize level
and type. Since RSIC-V currently does not have a register group that
describes cache-related attributes like ARM64, we cannot obtain them
directly, so now we obtain cache leaves from the ACPI PPTT table
(acpi_get_cache_info()) and set the cache type through split_levels.

Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 30a6878287ad..dc5fb70362f1 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -6,6 +6,7 @@
 #include <linux/cpu.h>
 #include <linux/of.h>
 #include <asm/cacheinfo.h>
+#include <linux/acpi.h>
 
 static struct riscv_cacheinfo_ops *rv_cache_ops;
 
@@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
 	struct device_node *prev = NULL;
 	int levels = 1, level = 1;
 
+	if (!acpi_disabled) {
+		int ret, idx, fw_levels, split_levels;
+
+		ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
+		if (ret)
+			return ret;
+
+		for (idx = 0; level <= this_cpu_ci->num_levels &&
+		     idx < this_cpu_ci->num_leaves; idx++, level++) {
+			if (level <= split_levels) {
+				ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
+				ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
+			} else {
+				ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
+			}
+		}
+		return 0;
+	}
+
 	if (of_property_read_bool(np, "cache-size"))
 		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 	if (of_property_read_bool(np, "i-cache-size"))
-- 
2.20.1


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

[PATCH v3 3/3] RISC-V: Select ACPI PPTT drivers

[Yunhui Cui]

After adding ACPI support to populate_cache_leaves(), RISC-V can build
cacheinfo through the ACPI PPTT table, thus enabling the ACPI_PPTT
configuration.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 6d64888134ba..5d73fcaf9136 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -14,6 +14,7 @@ config RISCV
 	def_bool y
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
+	select ACPI_PPTT if ACPI
 	select ARCH_DMA_DEFAULT_COHERENT
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
-- 
2.20.1

[PATCH v3 3/3] RISC-V: Select ACPI PPTT drivers

[Yunhui Cui]

After adding ACPI support to populate_cache_leaves(), RISC-V can build
cacheinfo through the ACPI PPTT table, thus enabling the ACPI_PPTT
configuration.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 arch/riscv/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 6d64888134ba..5d73fcaf9136 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -14,6 +14,7 @@ config RISCV
 	def_bool y
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
+	select ACPI_PPTT if ACPI
 	select ARCH_DMA_DEFAULT_COHERENT
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
-- 
2.20.1


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

Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Sudeep Holla]

On Tue, Apr 16, 2024 at 11:14:37AM +0800, Yunhui Cui wrote:
> Before cacheinfo can be built correctly, we need to initialize level
> and type. Since RSIC-V currently does not have a register group that
> describes cache-related attributes like ARM64, we cannot obtain them
> directly, so now we obtain cache leaves from the ACPI PPTT table
> (acpi_get_cache_info()) and set the cache type through split_levels.
>
> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>

I had already given the reviewed-by for the series, anyways here we go again:

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Sudeep Holla]

On Tue, Apr 16, 2024 at 11:14:37AM +0800, Yunhui Cui wrote:
> Before cacheinfo can be built correctly, we need to initialize level
> and type. Since RSIC-V currently does not have a register group that
> describes cache-related attributes like ARM64, we cannot obtain them
> directly, so now we obtain cache leaves from the ACPI PPTT table
> (acpi_get_cache_info()) and set the cache type through split_levels.
>
> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>

I had already given the reviewed-by for the series, anyways here we go again:

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

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

Re: [PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()

[Jeremy Linton]

Hi,

Other than a comment in 2/3 this all (patches 1-3) looks sane to me. So:

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks,


On 4/15/24 22:14, Yunhui Cui wrote:
> The implementation of the ci_leaf_init() function body and the caller
> do not use the input parameter (struct device_node *node), so remove it.
> 
> Fixes: 6a24915145c9 ("Revert "riscv: Set more data to cacheinfo"")
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>   arch/riscv/kernel/cacheinfo.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 09e9b88110d1..30a6878287ad 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -64,7 +64,6 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
>   }
>   
>   static void ci_leaf_init(struct cacheinfo *this_leaf,
> -			 struct device_node *node,
>   			 enum cache_type type, unsigned int level)
>   {
>   	this_leaf->level = level;
> @@ -80,11 +79,11 @@ int populate_cache_leaves(unsigned int cpu)
>   	int levels = 1, level = 1;
>   
>   	if (of_property_read_bool(np, "cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   	if (of_property_read_bool(np, "i-cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>   	if (of_property_read_bool(np, "d-cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>   
>   	prev = np;
>   	while ((np = of_find_next_cache_node(np))) {
> @@ -97,11 +96,11 @@ int populate_cache_leaves(unsigned int cpu)
>   		if (level <= levels)
>   			break;
>   		if (of_property_read_bool(np, "cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   		if (of_property_read_bool(np, "i-cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>   		if (of_property_read_bool(np, "d-cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>   		levels = level;
>   	}
>   	of_node_put(np);

Re: [PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()

[Jeremy Linton]

Hi,

Other than a comment in 2/3 this all (patches 1-3) looks sane to me. So:

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks,


On 4/15/24 22:14, Yunhui Cui wrote:
> The implementation of the ci_leaf_init() function body and the caller
> do not use the input parameter (struct device_node *node), so remove it.
> 
> Fixes: 6a24915145c9 ("Revert "riscv: Set more data to cacheinfo"")
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>   arch/riscv/kernel/cacheinfo.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 09e9b88110d1..30a6878287ad 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -64,7 +64,6 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
>   }
>   
>   static void ci_leaf_init(struct cacheinfo *this_leaf,
> -			 struct device_node *node,
>   			 enum cache_type type, unsigned int level)
>   {
>   	this_leaf->level = level;
> @@ -80,11 +79,11 @@ int populate_cache_leaves(unsigned int cpu)
>   	int levels = 1, level = 1;
>   
>   	if (of_property_read_bool(np, "cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   	if (of_property_read_bool(np, "i-cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>   	if (of_property_read_bool(np, "d-cache-size"))
> -		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
> +		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>   
>   	prev = np;
>   	while ((np = of_find_next_cache_node(np))) {
> @@ -97,11 +96,11 @@ int populate_cache_leaves(unsigned int cpu)
>   		if (level <= levels)
>   			break;
>   		if (of_property_read_bool(np, "cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   		if (of_property_read_bool(np, "i-cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>   		if (of_property_read_bool(np, "d-cache-size"))
> -			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>   		levels = level;
>   	}
>   	of_node_put(np);


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

Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Jeremy Linton]

Hi,


On 4/15/24 22:14, Yunhui Cui wrote:
> Before cacheinfo can be built correctly, we need to initialize level
> and type. Since RSIC-V currently does not have a register group that
> describes cache-related attributes like ARM64, we cannot obtain them
> directly, so now we obtain cache leaves from the ACPI PPTT table
> (acpi_get_cache_info()) and set the cache type through split_levels.
> 
> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>   arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 30a6878287ad..dc5fb70362f1 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -6,6 +6,7 @@
>   #include <linux/cpu.h>
>   #include <linux/of.h>
>   #include <asm/cacheinfo.h>
> +#include <linux/acpi.h>
>   
>   static struct riscv_cacheinfo_ops *rv_cache_ops;
>   
> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
>   	struct device_node *prev = NULL;
>   	int levels = 1, level = 1;
>   
> +	if (!acpi_disabled) {
> +		int ret, idx, fw_levels, split_levels;
> +
> +		ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> +		if (ret)
> +			return ret;
> +
> +		for (idx = 0; level <= this_cpu_ci->num_levels &&
> +		     idx < this_cpu_ci->num_leaves; idx++, level++) {

AFAIK the purpose of idx here it to assure that the number of cache 
leaves is not overflowing. But right below we are utilizing two of them 
at once, so this check isn't correct. OTOH, since its allocated as 
levels + split_levels I don't think its actually possible for this to 
cause a problem. Might be worthwhile to just hoist it before the loop 
and revalidate the total leaves about to be utilized.


> +			if (level <= split_levels) {
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> +			} else {
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> +			}
> +		}
> +		return 0;
> +	}
> +
>   	if (of_property_read_bool(np, "cache-size"))
>   		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   	if (of_property_read_bool(np, "i-cache-size"))

Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Jeremy Linton]

Hi,


On 4/15/24 22:14, Yunhui Cui wrote:
> Before cacheinfo can be built correctly, we need to initialize level
> and type. Since RSIC-V currently does not have a register group that
> describes cache-related attributes like ARM64, we cannot obtain them
> directly, so now we obtain cache leaves from the ACPI PPTT table
> (acpi_get_cache_info()) and set the cache type through split_levels.
> 
> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>   arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 30a6878287ad..dc5fb70362f1 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -6,6 +6,7 @@
>   #include <linux/cpu.h>
>   #include <linux/of.h>
>   #include <asm/cacheinfo.h>
> +#include <linux/acpi.h>
>   
>   static struct riscv_cacheinfo_ops *rv_cache_ops;
>   
> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
>   	struct device_node *prev = NULL;
>   	int levels = 1, level = 1;
>   
> +	if (!acpi_disabled) {
> +		int ret, idx, fw_levels, split_levels;
> +
> +		ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> +		if (ret)
> +			return ret;
> +
> +		for (idx = 0; level <= this_cpu_ci->num_levels &&
> +		     idx < this_cpu_ci->num_leaves; idx++, level++) {

AFAIK the purpose of idx here it to assure that the number of cache 
leaves is not overflowing. But right below we are utilizing two of them 
at once, so this check isn't correct. OTOH, since its allocated as 
levels + split_levels I don't think its actually possible for this to 
cause a problem. Might be worthwhile to just hoist it before the loop 
and revalidate the total leaves about to be utilized.


> +			if (level <= split_levels) {
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> +			} else {
> +				ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> +			}
> +		}
> +		return 0;
> +	}
> +
>   	if (of_property_read_bool(np, "cache-size"))
>   		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>   	if (of_property_read_bool(np, "i-cache-size"))


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

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[yunhui cui]

Hi Jeremy,

On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
>
> On 4/15/24 22:14, Yunhui Cui wrote:
> > Before cacheinfo can be built correctly, we need to initialize level
> > and type. Since RSIC-V currently does not have a register group that
> > describes cache-related attributes like ARM64, we cannot obtain them
> > directly, so now we obtain cache leaves from the ACPI PPTT table
> > (acpi_get_cache_info()) and set the cache type through split_levels.
> >
> > Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >   arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> > index 30a6878287ad..dc5fb70362f1 100644
> > --- a/arch/riscv/kernel/cacheinfo.c
> > +++ b/arch/riscv/kernel/cacheinfo.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/cpu.h>
> >   #include <linux/of.h>
> >   #include <asm/cacheinfo.h>
> > +#include <linux/acpi.h>
> >
> >   static struct riscv_cacheinfo_ops *rv_cache_ops;
> >
> > @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
> >       struct device_node *prev = NULL;
> >       int levels = 1, level = 1;
> >
> > +     if (!acpi_disabled) {
> > +             int ret, idx, fw_levels, split_levels;
> > +
> > +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             for (idx = 0; level <= this_cpu_ci->num_levels &&
> > +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
>
> AFAIK the purpose of idx here it to assure that the number of cache
> leaves is not overflowing. But right below we are utilizing two of them
> at once, so this check isn't correct. OTOH, since its allocated as
> levels + split_levels I don't think its actually possible for this to
> cause a problem. Might be worthwhile to just hoist it before the loop
> and revalidate the total leaves about to be utilized.
>

Do you mean to modify the logic as follows to make it more complete?
for (idx = 0; level <= this_cpu_ci->num_levels &&
      idx < this_cpu_ci->num_leaves; level++) {
        if (level <= split_levels) {
               ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
               idx++;
               ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
               idx++;
       } else {
               ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
               idx++;
      }
}


Thanks,
Yunhui

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[yunhui cui]

Hi Jeremy,

On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
>
> On 4/15/24 22:14, Yunhui Cui wrote:
> > Before cacheinfo can be built correctly, we need to initialize level
> > and type. Since RSIC-V currently does not have a register group that
> > describes cache-related attributes like ARM64, we cannot obtain them
> > directly, so now we obtain cache leaves from the ACPI PPTT table
> > (acpi_get_cache_info()) and set the cache type through split_levels.
> >
> > Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >   arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> > index 30a6878287ad..dc5fb70362f1 100644
> > --- a/arch/riscv/kernel/cacheinfo.c
> > +++ b/arch/riscv/kernel/cacheinfo.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/cpu.h>
> >   #include <linux/of.h>
> >   #include <asm/cacheinfo.h>
> > +#include <linux/acpi.h>
> >
> >   static struct riscv_cacheinfo_ops *rv_cache_ops;
> >
> > @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
> >       struct device_node *prev = NULL;
> >       int levels = 1, level = 1;
> >
> > +     if (!acpi_disabled) {
> > +             int ret, idx, fw_levels, split_levels;
> > +
> > +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             for (idx = 0; level <= this_cpu_ci->num_levels &&
> > +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
>
> AFAIK the purpose of idx here it to assure that the number of cache
> leaves is not overflowing. But right below we are utilizing two of them
> at once, so this check isn't correct. OTOH, since its allocated as
> levels + split_levels I don't think its actually possible for this to
> cause a problem. Might be worthwhile to just hoist it before the loop
> and revalidate the total leaves about to be utilized.
>

Do you mean to modify the logic as follows to make it more complete?
for (idx = 0; level <= this_cpu_ci->num_levels &&
      idx < this_cpu_ci->num_leaves; level++) {
        if (level <= split_levels) {
               ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
               idx++;
               ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
               idx++;
       } else {
               ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
               idx++;
      }
}


Thanks,
Yunhui

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

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Jeremy Linton]

Hi,

On 4/16/24 22:15, yunhui cui wrote:
> Hi Jeremy,
> 
> On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> Hi,
>>
>>
>> On 4/15/24 22:14, Yunhui Cui wrote:
>>> Before cacheinfo can be built correctly, we need to initialize level
>>> and type. Since RSIC-V currently does not have a register group that
>>> describes cache-related attributes like ARM64, we cannot obtain them
>>> directly, so now we obtain cache leaves from the ACPI PPTT table
>>> (acpi_get_cache_info()) and set the cache type through split_levels.
>>>
>>> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
>>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
>>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>>> ---
>>>    arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
>>> index 30a6878287ad..dc5fb70362f1 100644
>>> --- a/arch/riscv/kernel/cacheinfo.c
>>> +++ b/arch/riscv/kernel/cacheinfo.c
>>> @@ -6,6 +6,7 @@
>>>    #include <linux/cpu.h>
>>>    #include <linux/of.h>
>>>    #include <asm/cacheinfo.h>
>>> +#include <linux/acpi.h>
>>>
>>>    static struct riscv_cacheinfo_ops *rv_cache_ops;
>>>
>>> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
>>>        struct device_node *prev = NULL;
>>>        int levels = 1, level = 1;
>>>
>>> +     if (!acpi_disabled) {
>>> +             int ret, idx, fw_levels, split_levels;
>>> +
>>> +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             for (idx = 0; level <= this_cpu_ci->num_levels &&
>>> +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
>>
>> AFAIK the purpose of idx here it to assure that the number of cache
>> leaves is not overflowing. But right below we are utilizing two of them
>> at once, so this check isn't correct. OTOH, since its allocated as
>> levels + split_levels I don't think its actually possible for this to
>> cause a problem. Might be worthwhile to just hoist it before the loop
>> and revalidate the total leaves about to be utilized.
>>

I think I was suggesting something along the lines of:

BUG_ON((split_levels > fw_levels) || (split_levels + fw_levels > 
this_cpu_ci->num_leaves));

Then removing idx entirely. ex:

for (; level <= this_cpu_ci->num_levels; level++)
...
> 
> Do you mean to modify the logic as follows to make it more complete?
Sure that is one way to do it, but then you need to probably repeat the 
idx check:
> for (idx = 0; level <= this_cpu_ci->num_levels &&
>        idx < this_cpu_ci->num_leaves; level++) {
>          if (level <= split_levels) {
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>                 idx++;
if (idx >= this_cpu_ci->num_leaves) break;
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>                 idx++;
>         } else {
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>                 idx++;
>        }
> }

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[Jeremy Linton]

Hi,

On 4/16/24 22:15, yunhui cui wrote:
> Hi Jeremy,
> 
> On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> Hi,
>>
>>
>> On 4/15/24 22:14, Yunhui Cui wrote:
>>> Before cacheinfo can be built correctly, we need to initialize level
>>> and type. Since RSIC-V currently does not have a register group that
>>> describes cache-related attributes like ARM64, we cannot obtain them
>>> directly, so now we obtain cache leaves from the ACPI PPTT table
>>> (acpi_get_cache_info()) and set the cache type through split_levels.
>>>
>>> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
>>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
>>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>>> ---
>>>    arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
>>> index 30a6878287ad..dc5fb70362f1 100644
>>> --- a/arch/riscv/kernel/cacheinfo.c
>>> +++ b/arch/riscv/kernel/cacheinfo.c
>>> @@ -6,6 +6,7 @@
>>>    #include <linux/cpu.h>
>>>    #include <linux/of.h>
>>>    #include <asm/cacheinfo.h>
>>> +#include <linux/acpi.h>
>>>
>>>    static struct riscv_cacheinfo_ops *rv_cache_ops;
>>>
>>> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
>>>        struct device_node *prev = NULL;
>>>        int levels = 1, level = 1;
>>>
>>> +     if (!acpi_disabled) {
>>> +             int ret, idx, fw_levels, split_levels;
>>> +
>>> +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             for (idx = 0; level <= this_cpu_ci->num_levels &&
>>> +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
>>
>> AFAIK the purpose of idx here it to assure that the number of cache
>> leaves is not overflowing. But right below we are utilizing two of them
>> at once, so this check isn't correct. OTOH, since its allocated as
>> levels + split_levels I don't think its actually possible for this to
>> cause a problem. Might be worthwhile to just hoist it before the loop
>> and revalidate the total leaves about to be utilized.
>>

I think I was suggesting something along the lines of:

BUG_ON((split_levels > fw_levels) || (split_levels + fw_levels > 
this_cpu_ci->num_leaves));

Then removing idx entirely. ex:

for (; level <= this_cpu_ci->num_levels; level++)
...
> 
> Do you mean to modify the logic as follows to make it more complete?
Sure that is one way to do it, but then you need to probably repeat the 
idx check:
> for (idx = 0; level <= this_cpu_ci->num_levels &&
>        idx < this_cpu_ci->num_leaves; level++) {
>          if (level <= split_levels) {
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>                 idx++;
if (idx >= this_cpu_ci->num_leaves) break;
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>                 idx++;
>         } else {
>                 ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
>                 idx++;
>        }
> }




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

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[yunhui cui]

Hi Jeremy,

On Wed, Apr 17, 2024 at 10:00 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 4/16/24 22:15, yunhui cui wrote:
> > Hi Jeremy,
> >
> > On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> Hi,
> >>
> >>
> >> On 4/15/24 22:14, Yunhui Cui wrote:
> >>> Before cacheinfo can be built correctly, we need to initialize level
> >>> and type. Since RSIC-V currently does not have a register group that
> >>> describes cache-related attributes like ARM64, we cannot obtain them
> >>> directly, so now we obtain cache leaves from the ACPI PPTT table
> >>> (acpi_get_cache_info()) and set the cache type through split_levels.
> >>>
> >>> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> >>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >>> ---
> >>>    arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
> >>>    1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> >>> index 30a6878287ad..dc5fb70362f1 100644
> >>> --- a/arch/riscv/kernel/cacheinfo.c
> >>> +++ b/arch/riscv/kernel/cacheinfo.c
> >>> @@ -6,6 +6,7 @@
> >>>    #include <linux/cpu.h>
> >>>    #include <linux/of.h>
> >>>    #include <asm/cacheinfo.h>
> >>> +#include <linux/acpi.h>
> >>>
> >>>    static struct riscv_cacheinfo_ops *rv_cache_ops;
> >>>
> >>> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
> >>>        struct device_node *prev = NULL;
> >>>        int levels = 1, level = 1;
> >>>
> >>> +     if (!acpi_disabled) {
> >>> +             int ret, idx, fw_levels, split_levels;
> >>> +
> >>> +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> >>> +             if (ret)
> >>> +                     return ret;
> >>> +
> >>> +             for (idx = 0; level <= this_cpu_ci->num_levels &&
> >>> +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
> >>
> >> AFAIK the purpose of idx here it to assure that the number of cache
> >> leaves is not overflowing. But right below we are utilizing two of them
> >> at once, so this check isn't correct. OTOH, since its allocated as
> >> levels + split_levels I don't think its actually possible for this to
> >> cause a problem. Might be worthwhile to just hoist it before the loop
> >> and revalidate the total leaves about to be utilized.
> >>
>
> I think I was suggesting something along the lines of:
>
> BUG_ON((split_levels > fw_levels) || (split_levels + fw_levels >
> this_cpu_ci->num_leaves));
>
> Then removing idx entirely. ex:
Okay, I'll follow yours and update v4.


> for (; level <= this_cpu_ci->num_levels; level++)
> ...
> >
> > Do you mean to modify the logic as follows to make it more complete?
> Sure that is one way to do it, but then you need to probably repeat the
> idx check:
> > for (idx = 0; level <= this_cpu_ci->num_levels &&
> >        idx < this_cpu_ci->num_leaves; level++) {
> >          if (level <= split_levels) {
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> >                 idx++;
> if (idx >= this_cpu_ci->num_leaves) break;
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> >                 idx++;
> >         } else {
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> >                 idx++;
> >        }
> > }
>
>
>

Thanks,
Yunhui

Re: [External] Re: [PATCH v3 2/3] riscv: cacheinfo: initialize cacheinfo's level and type from ACPI PPTT

[yunhui cui]

Hi Jeremy,

On Wed, Apr 17, 2024 at 10:00 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Hi,
>
> On 4/16/24 22:15, yunhui cui wrote:
> > Hi Jeremy,
> >
> > On Wed, Apr 17, 2024 at 4:04 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
> >>
> >> Hi,
> >>
> >>
> >> On 4/15/24 22:14, Yunhui Cui wrote:
> >>> Before cacheinfo can be built correctly, we need to initialize level
> >>> and type. Since RSIC-V currently does not have a register group that
> >>> describes cache-related attributes like ARM64, we cannot obtain them
> >>> directly, so now we obtain cache leaves from the ACPI PPTT table
> >>> (acpi_get_cache_info()) and set the cache type through split_levels.
> >>>
> >>> Suggested-by: Jeremy Linton <jeremy.linton@arm.com>
> >>> Suggested-by: Sudeep Holla <sudeep.holla@arm.com>
> >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >>> ---
> >>>    arch/riscv/kernel/cacheinfo.c | 20 ++++++++++++++++++++
> >>>    1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> >>> index 30a6878287ad..dc5fb70362f1 100644
> >>> --- a/arch/riscv/kernel/cacheinfo.c
> >>> +++ b/arch/riscv/kernel/cacheinfo.c
> >>> @@ -6,6 +6,7 @@
> >>>    #include <linux/cpu.h>
> >>>    #include <linux/of.h>
> >>>    #include <asm/cacheinfo.h>
> >>> +#include <linux/acpi.h>
> >>>
> >>>    static struct riscv_cacheinfo_ops *rv_cache_ops;
> >>>
> >>> @@ -78,6 +79,25 @@ int populate_cache_leaves(unsigned int cpu)
> >>>        struct device_node *prev = NULL;
> >>>        int levels = 1, level = 1;
> >>>
> >>> +     if (!acpi_disabled) {
> >>> +             int ret, idx, fw_levels, split_levels;
> >>> +
> >>> +             ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels);
> >>> +             if (ret)
> >>> +                     return ret;
> >>> +
> >>> +             for (idx = 0; level <= this_cpu_ci->num_levels &&
> >>> +                  idx < this_cpu_ci->num_leaves; idx++, level++) {
> >>
> >> AFAIK the purpose of idx here it to assure that the number of cache
> >> leaves is not overflowing. But right below we are utilizing two of them
> >> at once, so this check isn't correct. OTOH, since its allocated as
> >> levels + split_levels I don't think its actually possible for this to
> >> cause a problem. Might be worthwhile to just hoist it before the loop
> >> and revalidate the total leaves about to be utilized.
> >>
>
> I think I was suggesting something along the lines of:
>
> BUG_ON((split_levels > fw_levels) || (split_levels + fw_levels >
> this_cpu_ci->num_leaves));
>
> Then removing idx entirely. ex:
Okay, I'll follow yours and update v4.


> for (; level <= this_cpu_ci->num_levels; level++)
> ...
> >
> > Do you mean to modify the logic as follows to make it more complete?
> Sure that is one way to do it, but then you need to probably repeat the
> idx check:
> > for (idx = 0; level <= this_cpu_ci->num_levels &&
> >        idx < this_cpu_ci->num_leaves; level++) {
> >          if (level <= split_levels) {
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> >                 idx++;
> if (idx >= this_cpu_ci->num_leaves) break;
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> >                 idx++;
> >         } else {
> >                 ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> >                 idx++;
> >        }
> > }
>
>
>

Thanks,
Yunhui

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