riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

[PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by liu.xuemei1@zte.com.cn 2 months, 1 week ago

From: Jessica Liu <liu.xuemei1@zte.com.cn>

As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
init_cache_level() function"), when CONFIG_SMP is undefined, the cache
hierarchy detection needs to be performed through the init_cache_level(),
whereas when CONFIG_SMP is defined, this detection is handled during the
init_cpu_topology() process.

Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT drivers")
enables cache information retrieval through the ACPI PPTT table, the
init_of_cache_level() called within init_cache_level() cannot support cache
hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
undefined, we directly invoke the fetch_cache_info function to initialize
the cache levels.

Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
---
 arch/riscv/kernel/cacheinfo.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 26b085dbdd07..f81ca963d177 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,

 int init_cache_level(unsigned int cpu)
 {
-	return init_of_cache_level(cpu);
+#ifdef CONFIG_SMP
+	return 0;
+#endif
+
+	return fetch_cache_info(cpu);
 }

 int populate_cache_leaves(unsigned int cpu)
-- 
2.25.1

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by Alexandre Ghiti 2 months ago

Hi Jessica,

On 7/25/25 04:44, liu.xuemei1@zte.com.cn wrote:
> From: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
> init_cache_level() function"), when CONFIG_SMP is undefined, the cache
> hierarchy detection needs to be performed through the init_cache_level(),
> whereas when CONFIG_SMP is defined, this detection is handled during the
> init_cpu_topology() process.
>
> Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT drivers")
> enables cache information retrieval through the ACPI PPTT table, the
> init_of_cache_level() called within init_cache_level() cannot support cache
> hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
> undefined, we directly invoke the fetch_cache_info function to initialize
> the cache levels.
>
> Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
> ---
>   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 26b085dbdd07..f81ca963d177 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
>
>   int init_cache_level(unsigned int cpu)
>   {
> -	return init_of_cache_level(cpu);
> +#ifdef CONFIG_SMP
> +	return 0;
> +#endif
> +
> +	return fetch_cache_info(cpu);
>   }
>
>   int populate_cache_leaves(unsigned int cpu)


Is the current behaviour wrong or just redundant? If wrong, I'll add a 
Fixes tag to backport, otherwise I won't.

Thanks,

Alex

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by liu.xuemei1@zte.com.cn 2 months ago

On 7/31/25 21:29, alex@ghiti.fr wrote:
> > From: Jessica Liu <liu.xuemei1@zte.com.cn>
> >
> > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
> > init_cache_level() function"), when CONFIG_SMP is undefined, the cache
> > hierarchy detection needs to be performed through the init_cache_level(),
> > whereas when CONFIG_SMP is defined, this detection is handled during the
> > init_cpu_topology() process.
> >
> > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT drivers")
> > enables cache information retrieval through the ACPI PPTT table, the
> > init_of_cache_level() called within init_cache_level() cannot support cache
> > hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
> > undefined, we directly invoke the fetch_cache_info function to initialize
> > the cache levels.
> >
> > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
> > ---
> >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> > index 26b085dbdd07..f81ca963d177 100644
> > --- a/arch/riscv/kernel/cacheinfo.c
> > +++ b/arch/riscv/kernel/cacheinfo.c
> > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
> >
> >   int init_cache_level(unsigned int cpu)
> >   {
> > -    return init_of_cache_level(cpu);
> > +#ifdef CONFIG_SMP
> > +    return 0;
> > +#endif
> > +
> > +    return fetch_cache_info(cpu);
> >   }
> >
> >   int populate_cache_leaves(unsigned int cpu)
> 
> 
> Is the current behaviour wrong or just redundant? If wrong, I'll add a 
> Fixes tag to backport, otherwise I won't.
> 
> Thanks,
> 
> Alex

Hi Alex,

The current behavior is actually wrong when using ACPI on !CONFIG_SMP
systems. The original init_of_cache_level() cannot detect cache hierarchy
through ACPI PPTT table, which means cache information would be missing
in this configuration.

The patch fixes this by directly calling fetch_cache_info() when
CONFIG_SMP is undefined, which properly handles both DT and ACPI cases.

So yes, it would be appropriate to add a Fixes tag. The commit being
fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level() function").

Please let me know if you need any additional information.

Best regards,
Jessica

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by Alexandre Ghiti 2 months ago

Hi Jessica,

On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>
> On 7/31/25 21:29, alex@ghiti.fr wrote:
>
> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> > >
>
> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>
> > > init_cache_level() function"), when CONFIG_SMP is undefined, the cache
>
> > > hierarchy detection needs to be performed through the 
> init_cache_level(),
>
> > > whereas when CONFIG_SMP is defined, this detection is handled 
> during the
>
> > > init_cpu_topology() process.
>
> > >
>
> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT 
> drivers")
>
> > > enables cache information retrieval through the ACPI PPTT table, the
>
> > > init_of_cache_level() called within init_cache_level() cannot 
> support cache
>
> > > hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
>
> > > undefined, we directly invoke the fetch_cache_info function to 
> initialize
>
> > > the cache levels.
>
> > >
>
> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> > > ---
>
> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>
> > >
>
> > > diff --git a/arch/riscv/kernel/cacheinfo.c 
> b/arch/riscv/kernel/cacheinfo.c
>
> > > index 26b085dbdd07..f81ca963d177 100644
>
> > > --- a/arch/riscv/kernel/cacheinfo.c
>
> > > +++ b/arch/riscv/kernel/cacheinfo.c
>
> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo 
> *this_leaf,
>
> > >
>
> > >   int init_cache_level(unsigned int cpu)
>
> > >   {
>
> > > -    return init_of_cache_level(cpu);
>
> > > +#ifdef CONFIG_SMP
>
> > > +    return 0;
>
> > > +#endif
>
> > > +
>
> > > +    return fetch_cache_info(cpu);
>
> > >   }
>
> > >
>
> > >   int populate_cache_leaves(unsigned int cpu)
>
> >
>
> >
>
> > Is the current behaviour wrong or just redundant? If wrong, I'll add a
>
> > Fixes tag to backport, otherwise I won't.
>
> >
>
> > Thanks,
>
> >
>
> > Alex
>
>
> Hi Alex,
>
>
> The current behavior is actually wrong when using ACPI on !CONFIG_SMP
>
> systems. The original init_of_cache_level() cannot detect cache hierarchy
>
> through ACPI PPTT table, which means cache information would be missing
>
> in this configuration.
>
>
> The patch fixes this by directly calling fetch_cache_info() when
>
> CONFIG_SMP is undefined, which properly handles both DT and ACPI cases.
>
>
> So yes, it would be appropriate to add a Fixes tag. The commit being
>
> fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level() 
> function").
>
>
> Please let me know if you need any additional information.
>

I'm about to send my first PR for 6.17 so I'll delay merging this one 
for the first rc.

Thanks for the explanation,

Alex


>
> Best regards,
>
> Jessica
>
>
>
>
>
>

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by Alexandre Ghiti 2 months ago

Hi Jessica,

On 8/1/25 13:45, Alexandre Ghiti wrote:
> Hi Jessica,
>
> On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>>
>> On 7/31/25 21:29, alex@ghiti.fr wrote:
>>
>> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>>
>> > >
>>
>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>>
>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the 
>> cache
>>
>> > > hierarchy detection needs to be performed through the 
>> init_cache_level(),
>>
>> > > whereas when CONFIG_SMP is defined, this detection is handled 
>> during the
>>
>> > > init_cpu_topology() process.
>>
>> > >
>>
>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT 
>> drivers")
>>
>> > > enables cache information retrieval through the ACPI PPTT table, the
>>
>> > > init_of_cache_level() called within init_cache_level() cannot 
>> support cache
>>
>> > > hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
>>
>> > > undefined, we directly invoke the fetch_cache_info function to 
>> initialize
>>
>> > > the cache levels.
>>
>> > >
>>
>> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>>
>> > > ---
>>
>> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>>
>> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> > >
>>
>> > > diff --git a/arch/riscv/kernel/cacheinfo.c 
>> b/arch/riscv/kernel/cacheinfo.c
>>
>> > > index 26b085dbdd07..f81ca963d177 100644
>>
>> > > --- a/arch/riscv/kernel/cacheinfo.c
>>
>> > > +++ b/arch/riscv/kernel/cacheinfo.c
>>
>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo 
>> *this_leaf,
>>
>> > >
>>
>> > >   int init_cache_level(unsigned int cpu)
>>
>> > >   {
>>
>> > > -    return init_of_cache_level(cpu);
>>
>> > > +#ifdef CONFIG_SMP
>>
>> > > +    return 0;
>>
>> > > +#endif
>>
>> > > +
>>
>> > > +    return fetch_cache_info(cpu);
>>
>> > >   }
>>
>> > >
>>
>> > >   int populate_cache_leaves(unsigned int cpu)
>>
>> >
>>
>> >
>>
>> > Is the current behaviour wrong or just redundant? If wrong, I'll add a
>>
>> > Fixes tag to backport, otherwise I won't.
>>
>> >
>>
>> > Thanks,
>>
>> >
>>
>> > Alex
>>
>>
>> Hi Alex,
>>
>>
>> The current behavior is actually wrong when using ACPI on !CONFIG_SMP
>>
>> systems. The original init_of_cache_level() cannot detect cache 
>> hierarchy
>>
>> through ACPI PPTT table, which means cache information would be missing
>>
>> in this configuration.
>>
>>
>> The patch fixes this by directly calling fetch_cache_info() when
>>
>> CONFIG_SMP is undefined, which properly handles both DT and ACPI cases.
>>
>>
>> So yes, it would be appropriate to add a Fixes tag. The commit being
>>
>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level() 
>> function").
>>
>>
>> Please let me know if you need any additional information.
>>
>
> I'm about to send my first PR for 6.17 so I'll delay merging this one 
> for the first rc.


So I took the time this morning to look into this, and I don't really 
like the different treatment for smp, can't we just move 
init_cpu_topology() call to setup_arch() (or else) for both !smp and smp?

Thanks,

Alex


>
> Thanks for the explanation,
>
> Alex
>
>
>>
>> Best regards,
>>
>> Jessica
>>
>>
>>
>>
>>
>>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by liu.xuemei1@zte.com.cn 1 month, 3 weeks ago

Hi Alex,

>> Hi Jessica,
>>
>> On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>>>
>>> On 7/31/25 21:29, alex@ghiti.fr wrote:
>>>
>>> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>>>
>>> > >
>>>
>>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>>>
>>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the 
>>> cache
>>>
>>> > > hierarchy detection needs to be performed through the 
>>> init_cache_level(),
>>>
>>> > > whereas when CONFIG_SMP is defined, this detection is handled 
>>> during the
>>>
>>> > > init_cpu_topology() process.
>>>
>>> > >
>>>
>>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT 
>>> drivers")
>>>
>>> > > enables cache information retrieval through the ACPI PPTT table, the
>>>
>>> > > init_of_cache_level() called within init_cache_level() cannot 
>>> support cache
>>>
>>> > > hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is
>>>
>>> > > undefined, we directly invoke the fetch_cache_info function to 
>>> initialize
>>>
>>> > > the cache levels.
>>>
>>> > >
>>>
>>> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>>>
>>> > > ---
>>>
>>> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>>>
>>> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> > >
>>>
>>> > > diff --git a/arch/riscv/kernel/cacheinfo.c 
>>> b/arch/riscv/kernel/cacheinfo.c
>>>
>>> > > index 26b085dbdd07..f81ca963d177 100644
>>>
>>> > > --- a/arch/riscv/kernel/cacheinfo.c
>>>
>>> > > +++ b/arch/riscv/kernel/cacheinfo.c
>>>
>>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo 
>>> *this_leaf,
>>>
>>> > >
>>>
>>> > >   int init_cache_level(unsigned int cpu)
>>>
>>> > >   {
>>>
>>> > > -    return init_of_cache_level(cpu);
>>>
>>> > > +#ifdef CONFIG_SMP
>>>
>>> > > +    return 0;
>>>
>>> > > +#endif
>>>
>>> > > +
>>>
>>> > > +    return fetch_cache_info(cpu);
>>>
>>> > >   }
>>>
>>> > >
>>>
>>> > >   int populate_cache_leaves(unsigned int cpu)
>>>
>>> >
>>>
>>> >
>>>
>>> > Is the current behaviour wrong or just redundant? If wrong, I'll add a
>>>
>>> > Fixes tag to backport, otherwise I won't.
>>>
>>> >
>>>
>>> > Thanks,
>>>
>>> >
>>>
>>> > Alex
>>>
>>>
>>> Hi Alex,
>>>
>>>
>>> The current behavior is actually wrong when using ACPI on !CONFIG_SMP
>>>
>>> systems. The original init_of_cache_level() cannot detect cache 
>>> hierarchy
>>>
>>> through ACPI PPTT table, which means cache information would be missing
>>>
>>> in this configuration.
>>>
>>>
>>> The patch fixes this by directly calling fetch_cache_info() when
>>>
>>> CONFIG_SMP is undefined, which properly handles both DT and ACPI cases..
>>>
>>>
>>> So yes, it would be appropriate to add a Fixes tag. The commit being
>>>
>>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level() 
>>> function").
>>>
>>>
>>> Please let me know if you need any additional information.
>>>
>>
>> I'm about to send my first PR for 6.17 so I'll delay merging this one 
>> for the first rc.
>
>
>So I took the time this morning to look into this, and I don't really 
>like the different treatment for smp, can't we just move 
>init_cpu_topology() call to setup_arch() (or else) for both !smp and smp?
>
>Thanks,
>
>Alex

Thank you for your feedback and suggestion. I understand your desire
to have a unified approach for both SMP and !SMP. However, after
careful consideration, I still believe that handling them separately
is the more appropriate solution.

The current method of obtaining cache information in
`init_cpu_topology()` is specific to RISC-V and ARM64. If we move
`init_cpu_topology()` to cover both SMP and !SMP, it may require
modifying the generic boot sequence. This could inadvertently affect
other architectures that do not rely on `init_cpu_topology()` for
cache initialization, leading to potential regressions and maintenance
issues.

The `setup_arch()` function is called early in the boot process,
and at this stage, the ACPI subsystem has not been fully initialized.
Specifically, the ACPI tables (including PPTT) are not yet parsed.
Therefore, if we call `init_cpu_topology()` from `setup_arch()`, it
would not be able to retrieve cache information from the ACPI PPTT table.

I hope this clarifies my train of thought. I'm open to further discussion and
alternative suggestions that can address the issue properly.

Best regards,
Jessica

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by Alexandre Ghiti 1 month, 3 weeks ago

Hi Jessica,

On 8/14/25 03:29, liu.xuemei1@zte.com.cn wrote:
>
> Hi Alex,
>
>
> >> Hi Jessica,
>
> >>
>
> >> On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>
> >>>
>
> >>> On 7/31/25 21:29, alex@ghiti.fr wrote:
>
> >>>
>
> >>> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>
> >>>
>
> >>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the
>
> >>> cache
>
> >>>
>
> >>> > > hierarchy detection needs to be performed through the
>
> >>> init_cache_level(),
>
> >>>
>
> >>> > > whereas when CONFIG_SMP is defined, this detection is handled
>
> >>> during the
>
> >>>
>
> >>> > > init_cpu_topology() process.
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT
>
> >>> drivers")
>
> >>>
>
> >>> > > enables cache information retrieval through the ACPI PPTT 
> table, the
>
> >>>
>
> >>> > > init_of_cache_level() called within init_cache_level() cannot
>
> >>> support cache
>
> >>>
>
> >>> > > hierarchy detection through ACPI PPTT. Therefore, when 
> CONFIG_SMP is
>
> >>>
>
> >>> > > undefined, we directly invoke the fetch_cache_info function to
>
> >>> initialize
>
> >>>
>
> >>> > > the cache levels.
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>
> >>>
>
> >>> > > ---
>
> >>>
>
> >>> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>
> >>>
>
> >>> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > > diff --git a/arch/riscv/kernel/cacheinfo.c
>
> >>> b/arch/riscv/kernel/cacheinfo.c
>
> >>>
>
> >>> > > index 26b085dbdd07..f81ca963d177 100644
>
> >>>
>
> >>> > > --- a/arch/riscv/kernel/cacheinfo.c
>
> >>>
>
> >>> > > +++ b/arch/riscv/kernel/cacheinfo.c
>
> >>>
>
> >>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo
>
> >>> *this_leaf,
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > >   int init_cache_level(unsigned int cpu)
>
> >>>
>
> >>> > >   {
>
> >>>
>
> >>> > > -    return init_of_cache_level(cpu);
>
> >>>
>
> >>> > > +#ifdef CONFIG_SMP
>
> >>>
>
> >>> > > +    return 0;
>
> >>>
>
> >>> > > +#endif
>
> >>>
>
> >>> > > +
>
> >>>
>
> >>> > > +    return fetch_cache_info(cpu);
>
> >>>
>
> >>> > >   }
>
> >>>
>
> >>> > >
>
> >>>
>
> >>> > >   int populate_cache_leaves(unsigned int cpu)
>
> >>>
>
> >>> >
>
> >>>
>
> >>> >
>
> >>>
>
> >>> > Is the current behaviour wrong or just redundant? If wrong, I'll 
> add a
>
> >>>
>
> >>> > Fixes tag to backport, otherwise I won't.
>
> >>>
>
> >>> >
>
> >>>
>
> >>> > Thanks,
>
> >>>
>
> >>> >
>
> >>>
>
> >>> > Alex
>
> >>>
>
> >>>
>
> >>> Hi Alex,
>
> >>>
>
> >>>
>
> >>> The current behavior is actually wrong when using ACPI on !CONFIG_SMP
>
> >>>
>
> >>> systems. The original init_of_cache_level() cannot detect cache
>
> >>> hierarchy
>
> >>>
>
> >>> through ACPI PPTT table, which means cache information would be 
> missing
>
> >>>
>
> >>> in this configuration.
>
> >>>
>
> >>>
>
> >>> The patch fixes this by directly calling fetch_cache_info() when
>
> >>>
>
> >>> CONFIG_SMP is undefined, which properly handles both DT and ACPI 
> cases..
>
> >>>
>
> >>>
>
> >>> So yes, it would be appropriate to add a Fixes tag. The commit being
>
> >>>
>
> >>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level()
>
> >>> function").
>
> >>>
>
> >>>
>
> >>> Please let me know if you need any additional information.
>
> >>>
>
> >>
>
> >> I'm about to send my first PR for 6.17 so I'll delay merging this one
>
> >> for the first rc.
>
> >
>
> >
>
> >So I took the time this morning to look into this, and I don't really
>
> >like the different treatment for smp, can't we just move
>
> >init_cpu_topology() call to setup_arch() (or else) for both !smp and smp?
>
> >
>
> >Thanks,
>
> >
>
> >Alex
>
>
> Thank you for your feedback and suggestion. I understand your desire
>
> to have a unified approach for both SMP and !SMP. However, after
>
> careful consideration, I still believe that handling them separately
>
> is the more appropriate solution.
>
>
> The current method of obtaining cache information in
>
> `init_cpu_topology()` is specific to RISC-V and ARM64. If we move
>
> `init_cpu_topology()` to cover both SMP and !SMP, it may require
>
> modifying the generic boot sequence. This could inadvertently affect
>
> other architectures that do not rely on `init_cpu_topology()` for
>
> cache initialization, leading to potential regressions and maintenance
>
> issues.
>
>
> The `setup_arch()` function is called early in the boot process,
>
> and at this stage, the ACPI subsystem has not been fully initialized.
>
> Specifically, the ACPI tables (including PPTT) are not yet parsed.
>
> Therefore, if we call `init_cpu_topology()` from `setup_arch()`, it
>
> would not be able to retrieve cache information from the ACPI PPTT table.
>
>
> I hope this clarifies my train of thought. I'm open to further 
> discussion and
>
> alternative suggestions that can address the issue properly.
>

To me it does not make sense to retrieve the cache info at 2 different 
points in time if the system is smp or not. I still think we should find 
a common place where init_cpu_topology() can be called for both smp and 
up, setup_arch() could not be the right place for the reasons you gave, 
but we just need to find the right one :)

Thanks for working on this,

Alex


>
> Best regards,
>
> Jessica
>
>
>
>
>
>

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by Alexandre Ghiti 1 month ago

Hi Jessica,

On 8/14/25 10:16, Alexandre Ghiti wrote:
> Hi Jessica,
>
> On 8/14/25 03:29, liu.xuemei1@zte.com.cn wrote:
>>
>> Hi Alex,
>>
>>
>> >> Hi Jessica,
>>
>> >>
>>
>> >> On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>>
>> >>>
>>
>> >>> On 7/31/25 21:29, alex@ghiti.fr wrote:
>>
>> >>>
>>
>> >>> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>>
>> >>>
>>
>> >>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the
>>
>> >>> cache
>>
>> >>>
>>
>> >>> > > hierarchy detection needs to be performed through the
>>
>> >>> init_cache_level(),
>>
>> >>>
>>
>> >>> > > whereas when CONFIG_SMP is defined, this detection is handled
>>
>> >>> during the
>>
>> >>>
>>
>> >>> > > init_cpu_topology() process.
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI 
>> PPTT
>>
>> >>> drivers")
>>
>> >>>
>>
>> >>> > > enables cache information retrieval through the ACPI PPTT 
>> table, the
>>
>> >>>
>>
>> >>> > > init_of_cache_level() called within init_cache_level() cannot
>>
>> >>> support cache
>>
>> >>>
>>
>> >>> > > hierarchy detection through ACPI PPTT. Therefore, when 
>> CONFIG_SMP is
>>
>> >>>
>>
>> >>> > > undefined, we directly invoke the fetch_cache_info function to
>>
>> >>> initialize
>>
>> >>>
>>
>> >>> > > the cache levels.
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>>
>> >>>
>>
>> >>> > > ---
>>
>> >>>
>>
>> >>> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>>
>> >>>
>>
>> >>> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > > diff --git a/arch/riscv/kernel/cacheinfo.c
>>
>> >>> b/arch/riscv/kernel/cacheinfo.c
>>
>> >>>
>>
>> >>> > > index 26b085dbdd07..f81ca963d177 100644
>>
>> >>>
>>
>> >>> > > --- a/arch/riscv/kernel/cacheinfo.c
>>
>> >>>
>>
>> >>> > > +++ b/arch/riscv/kernel/cacheinfo.c
>>
>> >>>
>>
>> >>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo
>>
>> >>> *this_leaf,
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > >   int init_cache_level(unsigned int cpu)
>>
>> >>>
>>
>> >>> > >   {
>>
>> >>>
>>
>> >>> > > -    return init_of_cache_level(cpu);
>>
>> >>>
>>
>> >>> > > +#ifdef CONFIG_SMP
>>
>> >>>
>>
>> >>> > > +    return 0;
>>
>> >>>
>>
>> >>> > > +#endif
>>
>> >>>
>>
>> >>> > > +
>>
>> >>>
>>
>> >>> > > +    return fetch_cache_info(cpu);
>>
>> >>>
>>
>> >>> > >   }
>>
>> >>>
>>
>> >>> > >
>>
>> >>>
>>
>> >>> > >   int populate_cache_leaves(unsigned int cpu)
>>
>> >>>
>>
>> >>> >
>>
>> >>>
>>
>> >>> >
>>
>> >>>
>>
>> >>> > Is the current behaviour wrong or just redundant? If wrong, 
>> I'll add a
>>
>> >>>
>>
>> >>> > Fixes tag to backport, otherwise I won't.
>>
>> >>>
>>
>> >>> >
>>
>> >>>
>>
>> >>> > Thanks,
>>
>> >>>
>>
>> >>> >
>>
>> >>>
>>
>> >>> > Alex
>>
>> >>>
>>
>> >>>
>>
>> >>> Hi Alex,
>>
>> >>>
>>
>> >>>
>>
>> >>> The current behavior is actually wrong when using ACPI on 
>> !CONFIG_SMP
>>
>> >>>
>>
>> >>> systems. The original init_of_cache_level() cannot detect cache
>>
>> >>> hierarchy
>>
>> >>>
>>
>> >>> through ACPI PPTT table, which means cache information would be 
>> missing
>>
>> >>>
>>
>> >>> in this configuration.
>>
>> >>>
>>
>> >>>
>>
>> >>> The patch fixes this by directly calling fetch_cache_info() when
>>
>> >>>
>>
>> >>> CONFIG_SMP is undefined, which properly handles both DT and ACPI 
>> cases..
>>
>> >>>
>>
>> >>>
>>
>> >>> So yes, it would be appropriate to add a Fixes tag. The commit being
>>
>> >>>
>>
>> >>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back 
>> init_cache_level()
>>
>> >>> function").
>>
>> >>>
>>
>> >>>
>>
>> >>> Please let me know if you need any additional information.
>>
>> >>>
>>
>> >>
>>
>> >> I'm about to send my first PR for 6.17 so I'll delay merging this one
>>
>> >> for the first rc.
>>
>> >
>>
>> >
>>
>> >So I took the time this morning to look into this, and I don't really
>>
>> >like the different treatment for smp, can't we just move
>>
>> >init_cpu_topology() call to setup_arch() (or else) for both !smp and 
>> smp?
>>
>> >
>>
>> >Thanks,
>>
>> >
>>
>> >Alex
>>
>>
>> Thank you for your feedback and suggestion. I understand your desire
>>
>> to have a unified approach for both SMP and !SMP. However, after
>>
>> careful consideration, I still believe that handling them separately
>>
>> is the more appropriate solution.
>>
>>
>> The current method of obtaining cache information in
>>
>> `init_cpu_topology()` is specific to RISC-V and ARM64. If we move
>>
>> `init_cpu_topology()` to cover both SMP and !SMP, it may require
>>
>> modifying the generic boot sequence. This could inadvertently affect
>>
>> other architectures that do not rely on `init_cpu_topology()` for
>>
>> cache initialization, leading to potential regressions and maintenance
>>
>> issues.
>>
>>
>> The `setup_arch()` function is called early in the boot process,
>>
>> and at this stage, the ACPI subsystem has not been fully initialized.
>>
>> Specifically, the ACPI tables (including PPTT) are not yet parsed.
>>
>> Therefore, if we call `init_cpu_topology()` from `setup_arch()`, it
>>
>> would not be able to retrieve cache information from the ACPI PPTT 
>> table.
>>
>>
>> I hope this clarifies my train of thought. I'm open to further 
>> discussion and
>>
>> alternative suggestions that can address the issue properly.
>>
>
> To me it does not make sense to retrieve the cache info at 2 different 
> points in time if the system is smp or not. I still think we should 
> find a common place where init_cpu_topology() can be called for both 
> smp and up, setup_arch() could not be the right place for the reasons 
> you gave, but we just need to find the right one :)
>
> Thanks for working on this,


I don't mean to pressure you, I know it's the end of summer and people 
are still on vacations or just back from vacation.

I just wanted to know if you had time to look into what I asked above?

Thanks,

Alex


>
> Alex
>
>
>>
>> Best regards,
>>
>> Jessica
>>
>>
>>
>>
>>
>>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info when SMP disabled

Posted by liu.xuemei1@zte.com.cn 1 month ago

>> Hi Jessica,
>>
>> On 8/14/25 03:29, liu.xuemei1@zte.com.cn wrote:
>>>
>>> Hi Alex,
>>>
>>>
>>> >> Hi Jessica,
>>>
>>> >>
>>>
>>> >> On 8/1/25 03:32, liu.xuemei1@zte.com.cn wrote:
>>>
>>> >>>
>>>
>>> >>> On 7/31/25 21:29, alex@ghiti.fr wrote:
>>>
>>> >>>
>>>
>>> >>> > > From: Jessica Liu <liu.xuemei1@zte.com.cn>
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back
>>>
>>> >>>
>>>
>>> >>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the
>>>
>>> >>> cache
>>>
>>> >>>
>>>
>>> >>> > > hierarchy detection needs to be performed through the
>>>
>>> >>> init_cache_level(),
>>>
>>> >>>
>>>
>>> >>> > > whereas when CONFIG_SMP is defined, this detection is handled
>>>
>>> >>> during the
>>>
>>> >>>
>>>
>>> >>> > > init_cpu_topology() process.
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI 
>>> PPTT
>>>
>>> >>> drivers")
>>>
>>> >>>
>>>
>>> >>> > > enables cache information retrieval through the ACPI PPTT 
>>> table, the
>>>
>>> >>>
>>>
>>> >>> > > init_of_cache_level() called within init_cache_level() cannot
>>>
>>> >>> support cache
>>>
>>> >>>
>>>
>>> >>> > > hierarchy detection through ACPI PPTT. Therefore, when 
>>> CONFIG_SMP is
>>>
>>> >>>
>>>
>>> >>> > > undefined, we directly invoke the fetch_cache_info function to
>>>
>>> >>> initialize
>>>
>>> >>>
>>>
>>> >>> > > the cache levels.
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > > Signed-off-by: Jessica Liu <liu.xuemei1@zte.com.cn>
>>>
>>> >>>
>>>
>>> >>> > > ---
>>>
>>> >>>
>>>
>>> >>> > >   arch/riscv/kernel/cacheinfo.c | 6 +++++-
>>>
>>> >>>
>>>
>>> >>> > >   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > > diff --git a/arch/riscv/kernel/cacheinfo.c
>>>
>>> >>> b/arch/riscv/kernel/cacheinfo.c
>>>
>>> >>>
>>>
>>> >>> > > index 26b085dbdd07..f81ca963d177 100644
>>>
>>> >>>
>>>
>>> >>> > > --- a/arch/riscv/kernel/cacheinfo.c
>>>
>>> >>>
>>>
>>> >>> > > +++ b/arch/riscv/kernel/cacheinfo.c
>>>
>>> >>>
>>>
>>> >>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo
>>>
>>> >>> *this_leaf,
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > >   int init_cache_level(unsigned int cpu)
>>>
>>> >>>
>>>
>>> >>> > >   {
>>>
>>> >>>
>>>
>>> >>> > > -    return init_of_cache_level(cpu);
>>>
>>> >>>
>>>
>>> >>> > > +#ifdef CONFIG_SMP
>>>
>>> >>>
>>>
>>> >>> > > +    return 0;
>>>
>>> >>>
>>>
>>> >>> > > +#endif
>>>
>>> >>>
>>>
>>> >>> > > +
>>>
>>> >>>
>>>
>>> >>> > > +    return fetch_cache_info(cpu);
>>>
>>> >>>
>>>
>>> >>> > >   }
>>>
>>> >>>
>>>
>>> >>> > >
>>>
>>> >>>
>>>
>>> >>> > >   int populate_cache_leaves(unsigned int cpu)
>>>
>>> >>>
>>>
>>> >>> >
>>>
>>> >>>
>>>
>>> >>> >
>>>
>>> >>>
>>>
>>> >>> > Is the current behaviour wrong or just redundant? If wrong, 
>>> I'll add a
>>>
>>> >>>
>>>
>>> >>> > Fixes tag to backport, otherwise I won't.
>>>
>>> >>>
>>>
>>> >>> >
>>>
>>> >>>
>>>
>>> >>> > Thanks,
>>>
>>> >>>
>>>
>>> >>> >
>>>
>>> >>>
>>>
>>> >>> > Alex
>>>
>>> >>>
>>>
>>> >>>
>>>
>>> >>> Hi Alex,
>>>
>>> >>>
>>>
>>> >>>
>>>
>>> >>> The current behavior is actually wrong when using ACPI on 
>>> !CONFIG_SMP
>>>
>>> >>>
>>>
>>> >>> systems. The original init_of_cache_level() cannot detect cache
>>>
>>> >>> hierarchy
>>>
>>> >>>
>>>
>>> >>> through ACPI PPTT table, which means cache information would be 
>>> missing
>>>
>>> >>>
>>>
>>> >>> in this configuration.
>>>
>>> >>>
>>>
>>> >>>
>>>
>>> >>> The patch fixes this by directly calling fetch_cache_info() when
>>>
>>> >>>
>>>
>>> >>> CONFIG_SMP is undefined, which properly handles both DT and ACPI 
>>> cases..
>>>
>>> >>>
>>>
>>> >>>
>>>
>>> >>> So yes, it would be appropriate to add a Fixes tag. The commit being
>>>
>>> >>>
>>>
>>> >>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back 
>>> init_cache_level()
>>>
>>> >>> function").
>>>
>>> >>>
>>>
>>> >>>
>>>
>>> >>> Please let me know if you need any additional information.
>>>
>>> >>>
>>>
>>> >>
>>>
>>> >> I'm about to send my first PR for 6.17 so I'll delay merging this one
>>>
>>> >> for the first rc.
>>>
>>> >
>>>
>>> >
>>>
>>> >So I took the time this morning to look into this, and I don't really
>>>
>>> >like the different treatment for smp, can't we just move
>>>
>>> >init_cpu_topology() call to setup_arch() (or else) for both !smp and 
>>> smp?
>>>
>>> >
>>>
>>> >Thanks,
>>>
>>> >
>>>
>>> >Alex
>>>
>>>
>>> Thank you for your feedback and suggestion. I understand your desire
>>>
>>> to have a unified approach for both SMP and !SMP. However, after
>>>
>>> careful consideration, I still believe that handling them separately
>>>
>>> is the more appropriate solution.
>>>
>>>
>>> The current method of obtaining cache information in
>>>
>>> `init_cpu_topology()` is specific to RISC-V and ARM64. If we move
>>>
>>> `init_cpu_topology()` to cover both SMP and !SMP, it may require
>>>
>>> modifying the generic boot sequence. This could inadvertently affect
>>>
>>> other architectures that do not rely on `init_cpu_topology()` for
>>>
>>> cache initialization, leading to potential regressions and maintenance
>>>
>>> issues.
>>>
>>>
>>> The `setup_arch()` function is called early in the boot process,
>>>
>>> and at this stage, the ACPI subsystem has not been fully initialized.
>>>
>>> Specifically, the ACPI tables (including PPTT) are not yet parsed.
>>>
>>> Therefore, if we call `init_cpu_topology()` from `setup_arch()`, it
>>>
>>> would not be able to retrieve cache information from the ACPI PPTT 
>>> table.
>>>
>>>
>>> I hope this clarifies my train of thought. I'm open to further 
>>> discussion and
>>>
>>> alternative suggestions that can address the issue properly.
>>>
>>
>> To me it does not make sense to retrieve the cache info at 2 different 
>> points in time if the system is smp or not. I still think we should 
>> find a common place where init_cpu_topology() can be called for both 
>> smp and up, setup_arch() could not be the right place for the reasons 
>> you gave, but we just need to find the right one :)
>>
>> Thanks for working on this,
>
>
>I don't mean to pressure you, I know it's the end of summer and people 
>are still on vacations or just back from vacation.
>
>I just wanted to know if you had time to look into what I asked above?
>
>Thanks,
>
>Alex

Hi Alex,

Thank you for the reminder, and I hope you had a good summer. I did just
returned from vacation.

I understand your concern about having a unified approach for cache
initialization in both SMP and !SMP configurations. I will look into your
suggestion carefully. Once I have a solution, I will test it thoroughly
and send a revised patch.

I appreciate your patience and guidance.

Best regards,
Jessica
>
>
>>
>> Alex
>>
>>
>>>
>>> Best regards,
>>>
>>> Jessica