fix when LoongArch s3 resume, Can't find image information
Signed-off-by: Li Jun <[email protected]>
Signed-off-by: Baoqi Zhang <[email protected]>
Signed-off-by: Jianmin Lv <[email protected]>
Signed-off-by: Biao Dong <[email protected]>
---
arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/arch/loongarch/mm/cache.c b/arch/loongarch/mm/cache.c
index 6be04d36ca07..52872fa0e5d8 100644
--- a/arch/loongarch/mm/cache.c
+++ b/arch/loongarch/mm/cache.c
@@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int leaf)
} while (--nr_nodes > 0);
}
+static void flush_cache_last_level(unsigned int leaf)
+{
+ u64 addr;
+ int i, j, nr_nodes, way_size;
+ struct cache_desc *cdesc = current_cpu_data.cache_leaves + leaf;
+
+ nr_nodes = loongson_sysconf.nr_nodes;
+
+ addr = CSR_DMW1_BASE;
+ iocsr_write32(0x1, 0x280);
+ way_size = cdesc->sets * cdesc->linesz;
+ do {
+ for (i = 0; i < (cdesc->ways * 3); i++) {
+ for (j = 0; j < (cdesc->sets); j++) {
+ *(volatile u32 *)addr;
+ addr += cdesc->linesz;
+ }
+ }
+ addr += 0x100000000000;
+ } while (--nr_nodes > 0);
+}
+
asmlinkage __visible void __flush_cache_all(void)
{
int leaf;
@@ -71,7 +93,7 @@ asmlinkage __visible void __flush_cache_all(void)
leaf = cache_present - 1;
if (cache_inclusive(cdesc + leaf)) {
- flush_cache_leaf(leaf);
+ flush_cache_last_level(leaf);
return;
}
--
2.34.1
On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
> fix when LoongArch s3 resume, Can't find image information
>
> Signed-off-by: Li Jun <[email protected]>
> Signed-off-by: Baoqi Zhang <[email protected]>
> Signed-off-by: Jianmin Lv <[email protected]>
> Signed-off-by: Biao Dong <[email protected]>
> ---
> arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/mm/cache.c b/arch/loongarch/mm/cache.c
> index 6be04d36ca07..52872fa0e5d8 100644
> --- a/arch/loongarch/mm/cache.c
> +++ b/arch/loongarch/mm/cache.c
> @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int leaf)
> } while (--nr_nodes > 0);
> }
>
> +static void flush_cache_last_level(unsigned int leaf)
> +{
> + u64 addr;
> + int i, j, nr_nodes, way_size;
> + struct cache_desc *cdesc = current_cpu_data.cache_leaves +
> leaf;
> +
> + nr_nodes = loongson_sysconf.nr_nodes;
> +
> + addr = CSR_DMW1_BASE;
> + iocsr_write32(0x1, 0x280);
> + way_size = cdesc->sets * cdesc->linesz;
> + do {
> + for (i = 0; i < (cdesc->ways * 3); i++) {
> + for (j = 0; j < (cdesc->sets); j++) {
> + *(volatile u32 *)addr;
??? what does this line do?
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
volatile prevents compiler optimization by allowing the compiler
to reread the address value of addr every time
在 2024/5/6 17:28, Xi Ruoyao 写道:
> On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
>> fix when LoongArch s3 resume, Can't find image information
>>
>> Signed-off-by: Li Jun <[email protected]>
>> Signed-off-by: Baoqi Zhang <[email protected]>
>> Signed-off-by: Jianmin Lv <[email protected]>
>> Signed-off-by: Biao Dong <[email protected]>
>> ---
>> arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/loongarch/mm/cache.c b/arch/loongarch/mm/cache.c
>> index 6be04d36ca07..52872fa0e5d8 100644
>> --- a/arch/loongarch/mm/cache.c
>> +++ b/arch/loongarch/mm/cache.c
>> @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int leaf)
>> } while (--nr_nodes > 0);
>> }
>>
>> +static void flush_cache_last_level(unsigned int leaf)
>> +{
>> + u64 addr;
>> + int i, j, nr_nodes, way_size;
>> + struct cache_desc *cdesc = current_cpu_data.cache_leaves +
>> leaf;
>> +
>> + nr_nodes = loongson_sysconf.nr_nodes;
>> +
>> + addr = CSR_DMW1_BASE;
>> + iocsr_write32(0x1, 0x280);
>> + way_size = cdesc->sets * cdesc->linesz;
>> + do {
>> + for (i = 0; i < (cdesc->ways * 3); i++) {
>> + for (j = 0; j < (cdesc->sets); j++) {
>> + *(volatile u32 *)addr;
> ??? what does this line do?
>
On Mon, 2024-05-06 at 18:08 +0800, lijun wrote:
> volatile prevents compiler optimization by allowing the compiler
>
> to reread the address value of addr every time
But why is this ever needed? What's wrong if the compiler optimizes it?
If the problem is the compiler may optimize it to cdesc->ways * 3 *
cdesc->sets * cdesc->linesz, unknowing cdesc->ways etc may magically
change, you should use READ_ONCE(cdesc->ways) etc.
I.e. use READ_ONCE on the expression which may magically change, instead
of hacking addr. addr won't magically change.
> 在 2024/5/6 17:28, Xi Ruoyao 写道:
> > On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
> > > fix when LoongArch s3 resume, Can't find image information
> > >
> > > Signed-off-by: Li Jun <[email protected]>
> > > Signed-off-by: Baoqi Zhang <[email protected]>
> > > Signed-off-by: Jianmin Lv <[email protected]>
> > > Signed-off-by: Biao Dong <[email protected]>
> > > ---
> > > arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
> > > 1 file changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/loongarch/mm/cache.c b/arch/loongarch/mm/cache.c
> > > index 6be04d36ca07..52872fa0e5d8 100644
> > > --- a/arch/loongarch/mm/cache.c
> > > +++ b/arch/loongarch/mm/cache.c
> > > @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int leaf)
> > > } while (--nr_nodes > 0);
> > > }
> > >
> > > +static void flush_cache_last_level(unsigned int leaf)
> > > +{
> > > + u64 addr;
> > > + int i, j, nr_nodes, way_size;
> > > + struct cache_desc *cdesc = current_cpu_data.cache_leaves
> > > +
> > > leaf;
> > > +
> > > + nr_nodes = loongson_sysconf.nr_nodes;
> > > +
> > > + addr = CSR_DMW1_BASE;
> > > + iocsr_write32(0x1, 0x280);
> > > + way_size = cdesc->sets * cdesc->linesz;
> > > + do {
> > > + for (i = 0; i < (cdesc->ways * 3); i++) {
> > > + for (j = 0; j < (cdesc->sets); j++) {
> > > + *(volatile u32 *)addr;
> > ??? what does this line do?
> >
>
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
The value of addr changes very very quickly, and 'volatile' ensures that
every change can be read
在 2024/5/6 18:17, Xi Ruoyao 写道:
> On Mon, 2024-05-06 at 18:08 +0800, lijun wrote:
>> volatile prevents compiler optimization by allowing the compiler
>>
>> to reread the address value of addr every time
> But why is this ever needed? What's wrong if the compiler optimizes it?
>
> If the problem is the compiler may optimize it to cdesc->ways * 3 *
> cdesc->sets * cdesc->linesz, unknowing cdesc->ways etc may magically
> change, you should use READ_ONCE(cdesc->ways) etc.
>
> I.e. use READ_ONCE on the expression which may magically change, instead
> of hacking addr. addr won't magically change.
>
>> 在 2024/5/6 17:28, Xi Ruoyao 写道:
>>> On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
>>>> fix when LoongArch s3 resume, Can't find image information
>>>>
>>>> Signed-off-by: Li Jun <[email protected]>
>>>> Signed-off-by: Baoqi Zhang <[email protected]>
>>>> Signed-off-by: Jianmin Lv <[email protected]>
>>>> Signed-off-by: Biao Dong <[email protected]>
>>>> ---
>>>> arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
>>>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/loongarch/mm/cache.c b/arch/loongarch/mm/cache.c
>>>> index 6be04d36ca07..52872fa0e5d8 100644
>>>> --- a/arch/loongarch/mm/cache.c
>>>> +++ b/arch/loongarch/mm/cache.c
>>>> @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int leaf)
>>>> } while (--nr_nodes > 0);
>>>> }
>>>>
>>>> +static void flush_cache_last_level(unsigned int leaf)
>>>> +{
>>>> + u64 addr;
>>>> + int i, j, nr_nodes, way_size;
>>>> + struct cache_desc *cdesc = current_cpu_data.cache_leaves
>>>> +
>>>> leaf;
>>>> +
>>>> + nr_nodes = loongson_sysconf.nr_nodes;
>>>> +
>>>> + addr = CSR_DMW1_BASE;
>>>> + iocsr_write32(0x1, 0x280);
>>>> + way_size = cdesc->sets * cdesc->linesz;
>>>> + do {
>>>> + for (i = 0; i < (cdesc->ways * 3); i++) {
>>>> + for (j = 0; j < (cdesc->sets); j++) {
>>>> + *(volatile u32 *)addr;
>>> ??? what does this line do?
>>>
On Tue, 2024-05-07 at 08:53 +0800, lijun wrote:
> The value of addr changes very very quickly, and 'volatile' ensures that
> every change can be read
No, volatile has nothing to do with changing quickly or not.
It's only useful when the compiler cannot know the change, for example
it's changed by the hardware or another thread.
And in the Linux kernel memory model for the hardware change you should
use READ_ONCE/WRITE_ONCE instead (they are actually wrappers of volatile
so in the kernel you should almost never need to directly use volatile),
for the change from another thread using volatile is just wrong and you
should use some atomic or locked operation instead.
See
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html.
In this case I'd like to ask first: why won't a simple addr += cdesc-
>linesz * cdesc->sets * cdesc->ways * 3 work? Which value(s) of addr,
cdesc, or cdesc->{linesz,sets,ways} may change w/o the compiler's
knowledge?
> 在 2024/5/6 18:17, Xi Ruoyao 写道:
> > On Mon, 2024-05-06 at 18:08 +0800, lijun wrote:
> > > volatile prevents compiler optimization by allowing the compiler
> > >
> > > to reread the address value of addr every time
> > But why is this ever needed? What's wrong if the compiler optimizes
> > it?
> >
> > If the problem is the compiler may optimize it to cdesc->ways * 3 *
> > cdesc->sets * cdesc->linesz, unknowing cdesc->ways etc may magically
> > change, you should use READ_ONCE(cdesc->ways) etc.
> >
> > I.e. use READ_ONCE on the expression which may magically change,
> > instead
> > of hacking addr. addr won't magically change.
> >
> > > 在 2024/5/6 17:28, Xi Ruoyao 写道:
> > > > On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
> > > > > fix when LoongArch s3 resume, Can't find image information
> > > > >
> > > > > Signed-off-by: Li Jun <[email protected]>
> > > > > Signed-off-by: Baoqi Zhang <[email protected]>
> > > > > Signed-off-by: Jianmin Lv <[email protected]>
> > > > > Signed-off-by: Biao Dong <[email protected]>
> > > > > ---
> > > > > arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
> > > > > 1 file changed, 23 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/loongarch/mm/cache.c
> > > > > b/arch/loongarch/mm/cache.c
> > > > > index 6be04d36ca07..52872fa0e5d8 100644
> > > > > --- a/arch/loongarch/mm/cache.c
> > > > > +++ b/arch/loongarch/mm/cache.c
> > > > > @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int
> > > > > leaf)
> > > > > } while (--nr_nodes > 0);
> > > > > }
> > > > >
> > > > > +static void flush_cache_last_level(unsigned int leaf)
> > > > > +{
> > > > > + u64 addr;
> > > > > + int i, j, nr_nodes, way_size;
> > > > > + struct cache_desc *cdesc =
> > > > > current_cpu_data.cache_leaves
> > > > > +
> > > > > leaf;
> > > > > +
> > > > > + nr_nodes = loongson_sysconf.nr_nodes;
> > > > > +
> > > > > + addr = CSR_DMW1_BASE;
> > > > > + iocsr_write32(0x1, 0x280);
> > > > > + way_size = cdesc->sets * cdesc->linesz;
> > > > > + do {
> > > > > + for (i = 0; i < (cdesc->ways * 3); i++) {
> > > > > + for (j = 0; j < (cdesc->sets); j++) {
> > > > > + *(volatile u32 *)addr;
> > > > ??? what does this line do?
> > > >
>
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
Hi Li,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc7 next-20240506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Jun/LoongArch-Update-the-flush-cache-policy/20240506-172624
base: linus/master
patch link: https://lore.kernel.org/r/20240506092419.4109941-1-lijun01%40kylinos.cn
patch subject: [PATCH] LoongArch: Update the flush cache policy
config: loongarch-allnoconfig
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
arch/loongarch/mm/cache.c: In function 'flush_cache_last_level':
>> arch/loongarch/mm/cache.c:69:29: warning: variable 'way_size' set but not used [-Wunused-but-set-variable]
69 | int i, j, nr_nodes, way_size;
| ^~~~~~~~
vim +/way_size +69 arch/loongarch/mm/cache.c
65
66 static void flush_cache_last_level(unsigned int leaf)
67 {
68 u64 addr;
> 69 int i, j, nr_nodes, way_size;
70 struct cache_desc *cdesc = current_cpu_data.cache_leaves + leaf;
71
72 nr_nodes = loongson_sysconf.nr_nodes;
73
74 addr = CSR_DMW1_BASE;
75 iocsr_write32(0x1, 0x280);
76 way_size = cdesc->sets * cdesc->linesz;
77 do {
78 for (i = 0; i < (cdesc->ways * 3); i++) {
79 for (j = 0; j < (cdesc->sets); j++) {
80 *(volatile u32 *)addr;
81 addr += cdesc->linesz;
82 }
83 }
84 addr += 0x100000000000;
85 } while (--nr_nodes > 0);
86 }
87
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
I guess, final the value of addr is not important, just all of addr must
read once is very important,
so use two 'for()' and 'volatile' to flush all of addr 's cache,
exactly as the name of the function is
"flush_cache_last_level".
在 2024/5/7 11:55, Xi Ruoyao 写道:
> On Tue, 2024-05-07 at 08:53 +0800, lijun wrote:
>> The value of addr changes very very quickly, and 'volatile' ensures that
>> every change can be read
> No, volatile has nothing to do with changing quickly or not.
>
> It's only useful when the compiler cannot know the change, for example
> it's changed by the hardware or another thread.
>
> And in the Linux kernel memory model for the hardware change you should
> use READ_ONCE/WRITE_ONCE instead (they are actually wrappers of volatile
> so in the kernel you should almost never need to directly use volatile),
> for the change from another thread using volatile is just wrong and you
> should use some atomic or locked operation instead.
>
> See
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html.
>
> In this case I'd like to ask first: why won't a simple addr += cdesc-
>> linesz * cdesc->sets * cdesc->ways * 3 work? Which value(s) of addr,
> cdesc, or cdesc->{linesz,sets,ways} may change w/o the compiler's
> knowledge?
>
>> 在 2024/5/6 18:17, Xi Ruoyao 写道:
>>> On Mon, 2024-05-06 at 18:08 +0800, lijun wrote:
>>>> volatile prevents compiler optimization by allowing the compiler
>>>>
>>>> to reread the address value of addr every time
>>> But why is this ever needed? What's wrong if the compiler optimizes
>>> it?
>>>
>>> If the problem is the compiler may optimize it to cdesc->ways * 3 *
>>> cdesc->sets * cdesc->linesz, unknowing cdesc->ways etc may magically
>>> change, you should use READ_ONCE(cdesc->ways) etc.
>>>
>>> I.e. use READ_ONCE on the expression which may magically change,
>>> instead
>>> of hacking addr. addr won't magically change.
>>>
>>>> 在 2024/5/6 17:28, Xi Ruoyao 写道:
>>>>> On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
>>>>>> fix when LoongArch s3 resume, Can't find image information
>>>>>>
>>>>>> Signed-off-by: Li Jun <[email protected]>
>>>>>> Signed-off-by: Baoqi Zhang <[email protected]>
>>>>>> Signed-off-by: Jianmin Lv <[email protected]>
>>>>>> Signed-off-by: Biao Dong <[email protected]>
>>>>>> ---
>>>>>> arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
>>>>>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/mm/cache.c
>>>>>> b/arch/loongarch/mm/cache.c
>>>>>> index 6be04d36ca07..52872fa0e5d8 100644
>>>>>> --- a/arch/loongarch/mm/cache.c
>>>>>> +++ b/arch/loongarch/mm/cache.c
>>>>>> @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int
>>>>>> leaf)
>>>>>> } while (--nr_nodes > 0);
>>>>>> }
>>>>>>
>>>>>> +static void flush_cache_last_level(unsigned int leaf)
>>>>>> +{
>>>>>> + u64 addr;
>>>>>> + int i, j, nr_nodes, way_size;
>>>>>> + struct cache_desc *cdesc =
>>>>>> current_cpu_data.cache_leaves
>>>>>> +
>>>>>> leaf;
>>>>>> +
>>>>>> + nr_nodes = loongson_sysconf.nr_nodes;
>>>>>> +
>>>>>> + addr = CSR_DMW1_BASE;
>>>>>> + iocsr_write32(0x1, 0x280);
>>>>>> + way_size = cdesc->sets * cdesc->linesz;
>>>>>> + do {
>>>>>> + for (i = 0; i < (cdesc->ways * 3); i++) {
>>>>>> + for (j = 0; j < (cdesc->sets); j++) {
>>>>>> + *(volatile u32 *)addr;
>>>>> ??? what does this line do?
>>>>>
On Tue, 2024-05-07 at 14:38 +0800, lijun wrote:
> I guess, final the value of addr is not important, just all of addr must
> read once is very important,
>
> so use two 'for()' and 'volatile' to flush all of addr 's cache,
> exactly as the name of the function is
>
> "flush_cache_last_level".
AFAIK do a read does not mean flushing the cache. The cacop instruction
is intended to do the cache flushing.
So why the cacop instruction in flush_cache_leaf is not enough to flush
it? Is there something wrong in flush_cache_leaf? It currently reads:
static void flush_cache_leaf(unsigned int leaf)
{
int i, j, nr_nodes;
uint64_t addr = CSR_DMW0_BASE;
struct cache_desc *cdesc = current_cpu_data.cache_leaves + leaf;
nr_nodes = cache_private(cdesc) ? 1 : loongson_sysconf.nr_nodes;
do {
for (i = 0; i < cdesc->sets; i++) {
for (j = 0; j < cdesc->ways; j++) {
flush_cache_line(leaf, addr);
addr++;
}
addr -= cdesc->ways;
addr += cdesc->linesz;
}
addr += (1ULL << NODE_ADDRSPACE_SHIFT);
} while (--nr_nodes > 0);
}
There is something bizarre: the flush_cache_line function uses the cacop
instruction in the Index Invalidate / Invalidate and Writeback mode, and
if I read the LoongArch manual correctly in this mode CSR_DMW0_BASE and
1ULL << NODE_ADDRSPACE_SHIFT are just ignored.
Is this something undocumented or flush_cache_leaf is just "incorrect"?
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University