| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250814092936030rQLylo3a7HXUWKIniqFy1@zte.com.cn>
Date: Thu, 14 Aug 2025 09:29:36 +0800 (CST)
From: <liu.xuemei1@....com.cn>
To: <alex@...ti.fr>, <paul.walmsley@...ive.com>
Cc: <palmer@...belt.com>, <aou@...s.berkeley.edu>, <spersvold@...il.com>,
<sudeep.holla@....com>, <mikisabate@...il.com>, <robh@...nel.org>,
<liu.xuemei1@....com.cn>, <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 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.
Best regards,
Jessica
<div class="zcontentRow"><p>Hi Alex,</p><p><br></p><p>>> Hi Jessica,</p><p>>></p><p>>> On 8/1/25 03:32, liu.xuemei1@....com.cn wrote:</p><p>>>></p><p>>>> On 7/31/25 21:29, alex@...ti.fr wrote:</p><p>>>></p><p>>>> > > From: Jessica Liu <liu.xuemei1@....com.cn></p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > As described in commit 1845d381f280 ("riscv: cacheinfo: Add back</p><p>>>></p><p>>>> > > init_cache_level() function"), when CONFIG_SMP is undefined, the </p><p>>>> cache</p><p>>>></p><p>>>> > > hierarchy detection needs to be performed through the </p><p>>>> init_cache_level(),</p><p>>>></p><p>>>> > > whereas when CONFIG_SMP is defined, this detection is handled </p><p>>>> during the</p><p>>>></p><p>>>> > > init_cpu_topology() process.</p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > Furthermore, while commit 66381d36771e ("RISC-V: Select ACPI PPTT </p><p>>>> drivers")</p><p>>>></p><p>>>> > > enables cache information retrieval through the ACPI PPTT table, the</p><p>>>></p><p>>>> > > init_of_cache_level() called within init_cache_level() cannot </p><p>>>> support cache</p><p>>>></p><p>>>> > > hierarchy detection through ACPI PPTT. Therefore, when CONFIG_SMP is</p><p>>>></p><p>>>> > > undefined, we directly invoke the fetch_cache_info function to </p><p>>>> initialize</p><p>>>></p><p>>>> > > the cache levels.</p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > Signed-off-by: Jessica Liu <liu.xuemei1@....com.cn></p><p>>>></p><p>>>> > > ---</p><p>>>></p><p>>>> > > arch/riscv/kernel/cacheinfo.c | 6 +++++-</p><p>>>></p><p>>>> > > 1 file changed, 5 insertions(+), 1 deletion(-)</p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > diff --git a/arch/riscv/kernel/cacheinfo.c </p><p>>>> b/arch/riscv/kernel/cacheinfo.c</p><p>>>></p><p>>>> > > index 26b085dbdd07..f81ca963d177 100644</p><p>>>></p><p>>>> > > --- a/arch/riscv/kernel/cacheinfo.c</p><p>>>></p><p>>>> > > +++ b/arch/riscv/kernel/cacheinfo.c</p><p>>>></p><p>>>> > > @@ -73,7 +73,11 @@ static void ci_leaf_init(struct cacheinfo </p><p>>>> *this_leaf,</p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > int init_cache_level(unsigned int cpu)</p><p>>>></p><p>>>> > > {</p><p>>>></p><p>>>> > > - return init_of_cache_level(cpu);</p><p>>>></p><p>>>> > > +#ifdef CONFIG_SMP</p><p>>>></p><p>>>> > > + return 0;</p><p>>>></p><p>>>> > > +#endif</p><p>>>></p><p>>>> > > +</p><p>>>></p><p>>>> > > + return fetch_cache_info(cpu);</p><p>>>></p><p>>>> > > }</p><p>>>></p><p>>>> > ></p><p>>>></p><p>>>> > > int populate_cache_leaves(unsigned int cpu)</p><p>>>></p><p>>>> ></p><p>>>></p><p>>>> ></p><p>>>></p><p>>>> > Is the current behaviour wrong or just redundant? If wrong, I'll add a</p><p>>>></p><p>>>> > Fixes tag to backport, otherwise I won't.</p><p>>>></p><p>>>> ></p><p>>>></p><p>>>> > Thanks,</p><p>>>></p><p>>>> ></p><p>>>></p><p>>>> > Alex</p><p>>>></p><p>>>></p><p>>>> Hi Alex,</p><p>>>></p><p>>>></p><p>>>> The current behavior is actually wrong when using ACPI on !CONFIG_SMP</p><p>>>></p><p>>>> systems. The original init_of_cache_level() cannot detect cache </p><p>>>> hierarchy</p><p>>>></p><p>>>> through ACPI PPTT table, which means cache information would be missing</p><p>>>></p><p>>>> in this configuration.</p><p>>>></p><p>>>></p><p>>>> The patch fixes this by directly calling fetch_cache_info() when</p><p>>>></p><p>>>> CONFIG_SMP is undefined, which properly handles both DT and ACPI cases..</p><p>>>></p><p>>>></p><p>>>> So yes, it would be appropriate to add a Fixes tag. The commit being</p><p>>>></p><p>>>> fixed is 1845d381f280 ("riscv: cacheinfo: Add back init_cache_level() </p><p>>>> function").</p><p>>>></p><p>>>></p><p>>>> Please let me know if you need any additional information.</p><p>>>></p><p>>></p><p>>> I'm about to send my first PR for 6.17 so I'll delay merging this one </p><p>>> for the first rc.</p><p>></p><p>></p><p>>So I took the time this morning to look into this, and I don't really </p><p>>like the different treatment for smp, can't we just move </p><p>>init_cpu_topology() call to setup_arch() (or else) for both !smp and smp?</p><p>></p><p>>Thanks,</p><p>></p><p>>Alex</p><p><br></p><p>Thank you for your feedback and suggestion. I understand your desire</p><p>to have a unified approach for both SMP and !SMP. However, after</p><p>careful consideration, I still believe that handling them separately</p><p>is the more appropriate solution.</p><p><br></p><p>The current method of obtaining cache information in</p><p>`init_cpu_topology()` is specific to RISC-V and ARM64. If we move</p><p>`init_cpu_topology()` to cover both SMP and !SMP, it may require</p><p>modifying the generic boot sequence. This could inadvertently affect</p><p>other architectures that do not rely on `init_cpu_topology()` for</p><p>cache initialization, leading to potential regressions and maintenance</p><p>issues.</p><p><br></p><p>The `setup_arch()` function is called early in the boot process,</p><p>and at this stage, the ACPI subsystem has not been fully initialized.</p><p>Specifically, the ACPI tables (including PPTT) are not yet parsed.</p><p>Therefore, if we call `init_cpu_topology()` from `setup_arch()`, it</p><p>would not be able to retrieve cache information from the ACPI PPTT table.</p><p><br></p><p>I hope this clarifies my train of thought. I'm open to further discussion and</p><p>alternative suggestions that can address the issue properly.</p><p><br></p><p>Best regards,</p><p>Jessica</p><p style="font-size:14px;font-family:微软雅黑,Microsoft YaHei;"><br></p><p style="font-size:14px;font-family:微软雅黑,Microsoft YaHei;"><br></p><p style="font-size:14px;font-family:微软雅黑,Microsoft YaHei;"><br></p><p style="font-size:14px;font-family:微软雅黑,Microsoft YaHei;"><br></p><p><br></p></div>
Powered by blists - more mailing lists