Message-ID: <91570387-4da1-4b26-a274-bed1c59ef12f@ghiti.fr>
Date: Thu, 14 Aug 2025 10:16:05 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: liu.xuemei1@....com.cn, paul.walmsley@...ive.com
Cc: palmer@...belt.com, aou@...s.berkeley.edu, spersvold@...il.com,
 sudeep.holla@....com, mikisabate@...il.com, robh@...nel.org,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] riscv: cacheinfo: init cache levels via fetch_cache_info
 when SMP disabled

Hi Jessica,

On 8/14/25 03:29, liu.xuemei1@....com.cn wrote:
>
> Hi Alex,
>
>
> >> Hi Jessica,
>
> >>
>
> >> On 8/1/25 03:32, liu.xuemei1@....com.cn wrote:
>
> >>>
>
> >>> On 7/31/25 21:29, alex@...ti.fr wrote:
>
> >>>
>
> >>> > > From: Jessica Liu <liu.xuemei1@....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@....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
>
>
>
>
>
>

Powered by blists - more mailing lists