| Message ID | 20240416031438.7637-2-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3,1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init() | expand |
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>
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"))
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
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++; > } > }
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
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"))
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(+)