| Message ID | 20211125105949.27147-5-huangpei@loongson.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/4] MIPS: rework local_t operation on MIPS64 | expand |
On Thu, Nov 25, 2021 at 06:59:49PM +0800, Huang Pei wrote: > It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under > c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000 > > Fix it by calling "decode_configs" after c->cputype is initialized > > Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG") > Signed-off-by: Huang Pei <huangpei@loongson.cn> > --- > arch/mips/kernel/cpu-probe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c > index ac0e2cfc6d57..24a529c6c4be 100644 > --- a/arch/mips/kernel/cpu-probe.c > +++ b/arch/mips/kernel/cpu-probe.c > @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c) > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > { > - decode_configs(c); > - > /* All Loongson processors covered here define ExcCode 16 as GSExc. */ > c->options |= MIPS_CPU_GSEXCEX; > > @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > panic("Unknown Loongson Processor ID!"); > break; > } > + > + decode_configs(c); > } > #else > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { } > -- > 2.20.1 applied to mips-fixes. Thomas.
On Thu, Nov 25, 2021 at 04:55:28PM +0100, Thomas Bogendoerfer wrote: > On Thu, Nov 25, 2021 at 06:59:49PM +0800, Huang Pei wrote: > > It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under > > c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000 > > > > Fix it by calling "decode_configs" after c->cputype is initialized > > > > Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG") > > Signed-off-by: Huang Pei <huangpei@loongson.cn> > > --- > > arch/mips/kernel/cpu-probe.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c > > index ac0e2cfc6d57..24a529c6c4be 100644 > > --- a/arch/mips/kernel/cpu-probe.c > > +++ b/arch/mips/kernel/cpu-probe.c > > @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c) > > > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > { > > - decode_configs(c); > > - > > /* All Loongson processors covered here define ExcCode 16 as GSExc. */ > > c->options |= MIPS_CPU_GSEXCEX; > > > > @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > panic("Unknown Loongson Processor ID!"); > > break; > > } > > + > > + decode_configs(c); > > } > > #else > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { } > > -- > > 2.20.1 > > applied to mips-fixes. > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ] Hi, Thomas, What about PATCH 1/4, without it, kernel/trace/ring_buffer.i using local_add_return, like this -------------------------------------------------------------------------------- __asm__ __volatile__( " .set push \n" " .set ""arch=r4000"" \n" ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2;.rept 1; sync 0x00; .endr; .set pop; .else; ; .endif" "\n" "1:" "lld " "%1, %2 # local_add_return\n" " addu %0, %1, %3 \n" "scd " "%0, %2 \n" " beqz %0, 1b \n" " addu %0, %1, %3 \n" " .set pop \n" : "=&r" (result), "=&r" (temp), "=m" (l->a.counter) : "Ir" (i), "m" (l->a.counter) : "memory"); } else if (1) { unsigned long temp; __asm__ __volatile__( " .set push \n" " .set ""arch=r4000"" \n" ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2; .rept 1; sync 0x00; .endr; .set pop; .else; ; .endif" " \n" "1:" "lld " "%1, %2 # local_add_return \n" " addu %0, %1, %3 \n" "scd " "%0, %2 \n" " beqz %0, 1b \n" " addu %0, %1, %3 \n" " .set pop \n" : "=&r" (result), "=&r" (temp), "=m" (l->a.counter) : "Ir" (i), "m" (l->a.counter) : "memory"); -------------------------------------------------------------------------------- it is wrong here, "lld" + "addu" with it, like this -------------------------------------------------------------------------------- if (1) { unsigned long temp; __asm__ __volatile__( " .set push \n" " .set ""arch=r4000"" \n" ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2; .rept 1; sync 0x00; .endr; .set pop; .else; ; .endif" " \n" "1:" "lld" " %1, %2 \n" " ""daddu" " %0, %1, %3 \n" " ""scd" " %0, %2 \n" " ""beqz" " %0, 1b \n" " ""daddu" " %0, %1, %3 \n" " .set pop \n" : "=&r" (result), "=&r" (temp), "=m" (l->a.counter) : "Ir" (i), "m" (l->a.counter) : "memory"); } else { MIPS64 needs "lld + daddu" and PATCH 2, any comment?
On Fri, Nov 26, 2021 at 11:13:19AM +0800, Huang Pei wrote: > On Thu, Nov 25, 2021 at 04:55:28PM +0100, Thomas Bogendoerfer wrote: > > On Thu, Nov 25, 2021 at 06:59:49PM +0800, Huang Pei wrote: > > > It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under > > > c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000 > > > > > > Fix it by calling "decode_configs" after c->cputype is initialized > > > > > > Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG") > > > Signed-off-by: Huang Pei <huangpei@loongson.cn> > > > --- > > > arch/mips/kernel/cpu-probe.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c > > > index ac0e2cfc6d57..24a529c6c4be 100644 > > > --- a/arch/mips/kernel/cpu-probe.c > > > +++ b/arch/mips/kernel/cpu-probe.c > > > @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c) > > > > > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > > { > > > - decode_configs(c); > > > - > > > /* All Loongson processors covered here define ExcCode 16 as GSExc. */ > > > c->options |= MIPS_CPU_GSEXCEX; > > > > > > @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > > panic("Unknown Loongson Processor ID!"); > > > break; > > > } > > > + > > > + decode_configs(c); > > > } > > > #else > > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { } > > > -- > > > 2.20.1 > > > > applied to mips-fixes. > > > > Thomas. > > > > -- > > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > > good idea. [ RFC1925, 2.3 ] > Hi, Thomas, > > What about PATCH 1/4, without it, kernel/trace/ring_buffer.i using > local_add_return, like this > -------------------------------------------------------------------------------- > __asm__ __volatile__( > " .set push \n" > " .set ""arch=r4000"" \n" > ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2;.rept 1; sync > 0x00; .endr; .set pop; .else; ; .endif" "\n" > "1:" "lld " "%1, %2 # local_add_return\n" > " addu %0, %1, %3 \n" > "scd " "%0, %2 \n" > " beqz %0, 1b \n" > " addu %0, %1, %3 \n" > " .set pop > \n" > : "=&r" (result), "=&r" (temp), "=m" (l->a.counter) > : "Ir" (i), "m" (l->a.counter) > : "memory"); > } else if (1) { > unsigned long temp; > > __asm__ __volatile__( > " .set push \n" > " .set ""arch=r4000"" \n" > ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set > mips64r2; .rept 1; sync 0x00; .endr; .set pop; .else; ; > .endif" " \n" > "1:" "lld " "%1, %2 # local_add_return > \n" > " addu %0, %1, %3 \n" > "scd " "%0, %2 > \n" > " beqz %0, 1b > \n" > " addu %0, %1, %3 > \n" > " .set pop > \n" > : "=&r" (result), "=&r" (temp), "=m" > (l->a.counter) > : "Ir" (i), "m" (l->a.counter) > : "memory"); > > -------------------------------------------------------------------------------- > it is wrong here, "lld" + "addu" it fixes something, but I didn't see the impact from the commit message. Because there is no Fixes tag, I haven't applied it tomips-fixes. And +#elif MIPS_ISA_REV >= 6 +# define SC_BEQZ beqzc why are you doing this ? > and PATCH 2, any comment? parameter page already contains the virtual address of the page to flush, so there is nothing to fix. I'm not 100% about the cache nature of all TX39 core, so leaving the check for cpu_has_dc_aliases in place is the safer bet. And this platform is nearly dead, so I'm sure sure whether this sort of cosemtics is needed. Thomas.
On Tue, Nov 30, 2021 at 11:34:48AM +0100, Thomas Bogendoerfer wrote: > On Fri, Nov 26, 2021 at 11:13:19AM +0800, Huang Pei wrote: > > On Thu, Nov 25, 2021 at 04:55:28PM +0100, Thomas Bogendoerfer wrote: > > > On Thu, Nov 25, 2021 at 06:59:49PM +0800, Huang Pei wrote: > > > > It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under > > > > c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000 > > > > > > > > Fix it by calling "decode_configs" after c->cputype is initialized > > > > > > > > Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG") > > > > Signed-off-by: Huang Pei <huangpei@loongson.cn> > > > > --- > > > > arch/mips/kernel/cpu-probe.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c > > > > index ac0e2cfc6d57..24a529c6c4be 100644 > > > > --- a/arch/mips/kernel/cpu-probe.c > > > > +++ b/arch/mips/kernel/cpu-probe.c > > > > @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c) > > > > > > > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > > > { > > > > - decode_configs(c); > > > > - > > > > /* All Loongson processors covered here define ExcCode 16 as GSExc. */ > > > > c->options |= MIPS_CPU_GSEXCEX; > > > > > > > > @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) > > > > panic("Unknown Loongson Processor ID!"); > > > > break; > > > > } > > > > + > > > > + decode_configs(c); > > > > } > > > > #else > > > > static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { } > > > > -- > > > > 2.20.1 > > > > > > applied to mips-fixes. > > > > > > Thomas. > > > > > > -- > > > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > > > good idea. [ RFC1925, 2.3 ] > > Hi, Thomas, > > > > What about PATCH 1/4, without it, kernel/trace/ring_buffer.i using > > local_add_return, like this > > -------------------------------------------------------------------------------- > > __asm__ __volatile__( > > " .set push \n" > > " .set ""arch=r4000"" \n" > > ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2;.rept 1; sync > > 0x00; .endr; .set pop; .else; ; .endif" "\n" > > "1:" "lld " "%1, %2 # local_add_return\n" > > " addu %0, %1, %3 \n" > > "scd " "%0, %2 \n" > > " beqz %0, 1b \n" > > " addu %0, %1, %3 \n" > > " .set pop > > \n" > > : "=&r" (result), "=&r" (temp), "=m" (l->a.counter) > > : "Ir" (i), "m" (l->a.counter) > > : "memory"); > > } else if (1) { > > unsigned long temp; > > > > __asm__ __volatile__( > > " .set push \n" > > " .set ""arch=r4000"" \n" > > ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set > > mips64r2; .rept 1; sync 0x00; .endr; .set pop; .else; ; > > .endif" " \n" > > "1:" "lld " "%1, %2 # local_add_return > > \n" > > " addu %0, %1, %3 \n" > > "scd " "%0, %2 > > \n" > > " beqz %0, 1b > > \n" > > " addu %0, %1, %3 > > \n" > > " .set pop > > \n" > > : "=&r" (result), "=&r" (temp), "=m" > > (l->a.counter) > > : "Ir" (i), "m" (l->a.counter) > > : "memory"); > > > > -------------------------------------------------------------------------------- > > it is wrong here, "lld" + "addu" > > it fixes something, but I didn't see the impact from the commit message. > Because there is no Fixes tag, I haven't applied it tomips-fixes. > Sorry, this bug is introduced by 7232311ef14c274d88871212a07557f18f4140d1 > And > > +#elif MIPS_ISA_REV >= 6 > +# define SC_BEQZ beqzc > > why are you doing this ? This copies from arch/mips/include/asm/llsc.h, I had another patch which remove llsc.h. I do not want add that patch into bugfix; > > and PATCH 2, any comment? > > parameter page already contains the virtual address of the page to > flush, so there is nothing to fix. I'm not 100% about the cache > nature of all TX39 core, so leaving the check for cpu_has_dc_aliases > in place is the safer bet. And this platform is nearly dead, so I'm > sure sure whether this sort of cosemtics is needed. > Based on the comment, I can see TX39 want to bypass TLB by Indexed Cache Operation, because with KSEG0 address, Indexed Cache Operation does not pass through TLB; But there are two limitation: +. KSEG0 only cover first 512MB physical address, so do not support High Memory, if not using KSEG0 address ,then it passes through TLB, which is not intended; +. Indexed Cache Operation can overkill cache line with same cache index from different way, but it can not kill Cache Alias, since Cache Alias actually has different cache index; From datasheet of TX39xx, I know the R3900 core of TX39 series has only 4KB direct-mapped Icache and 2KB two-way Dcache, so it has no Cache Alias under 4KB page size, and I can assume it does not support memory beyond 512MB based on Kconfig of TX39, AKA no High Memory; Above all, I think it is OK to both use KSEG0 address and remove cpu_has_dc_alias > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ]
diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c index ac0e2cfc6d57..24a529c6c4be 100644 --- a/arch/mips/kernel/cpu-probe.c +++ b/arch/mips/kernel/cpu-probe.c @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c) static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { - decode_configs(c); - /* All Loongson processors covered here define ExcCode 16 as GSExc. */ c->options |= MIPS_CPU_GSEXCEX; @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) panic("Unknown Loongson Processor ID!"); break; } + + decode_configs(c); } #else static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { }
It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000 Fix it by calling "decode_configs" after c->cputype is initialized Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG") Signed-off-by: Huang Pei <huangpei@loongson.cn> --- arch/mips/kernel/cpu-probe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)