On Sun, Sep 17, 2023 at 11:15:51PM +0800, Guo Ren wrote: > On Thu, Sep 14, 2023 at 4:32 PM Leonardo Bras <leobras@xxxxxxxxxx> wrote: > > > > On Sun, Sep 10, 2023 at 04:29:03AM -0400, guoren@xxxxxxxxxx wrote: > > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > > > > > > The early version of T-Head C9xx cores has a store merge buffer > > > delay problem. The store merge buffer could improve the store queue > > > performance by merging multi-store requests, but when there are not > > > continued store requests, the prior single store request would be > > > waiting in the store queue for a long time. That would cause > > > significant problems for communication between multi-cores. This > > > problem was found on sg2042 & th1520 platforms with the qspinlock > > > lock torture test. > > > > > > So appending a fence w.o could immediately flush the store merge > > > buffer and let other cores see the write result. > > > > > > This will apply the WRITE_ONCE errata to handle the non-standard > > > behavior via appending a fence w.o instruction for WRITE_ONCE(). > > > > > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx> > > > --- > > > arch/riscv/Kconfig.errata | 19 +++++++++++++++++++ > > > arch/riscv/errata/thead/errata.c | 20 ++++++++++++++++++++ > > > arch/riscv/include/asm/errata_list.h | 13 ------------- > > > arch/riscv/include/asm/rwonce.h | 24 ++++++++++++++++++++++++ > > > arch/riscv/include/asm/vendorid_list.h | 14 ++++++++++++++ > > > include/asm-generic/rwonce.h | 2 ++ > > > 6 files changed, 79 insertions(+), 13 deletions(-) > > > create mode 100644 arch/riscv/include/asm/rwonce.h > > > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > > index 1aa85a427ff3..c919cc3f1a3a 100644 > > > --- a/arch/riscv/Kconfig.errata > > > +++ b/arch/riscv/Kconfig.errata > > > @@ -77,4 +77,23 @@ config ERRATA_THEAD_PMU > > > > > > If you don't know what to do here, say "Y". > > > > > > +config ERRATA_THEAD_WRITE_ONCE > > > + bool "Apply T-Head WRITE_ONCE errata" > > > + depends on ERRATA_THEAD > > > + default y > > > + help > > > + The early version of T-Head C9xx cores has a store merge buffer > > > + delay problem. The store merge buffer could improve the store queue > > > + performance by merging multi-store requests, but when there are no > > > + continued store requests, the prior single store request would be > > > + waiting in the store queue for a long time. That would cause > > > + significant problems for communication between multi-cores. Appending > > > + a fence w.o could immediately flush the store merge buffer and let > > > + other cores see the write result. > > > + > > > + This will apply the WRITE_ONCE errata to handle the non-standard > > > + behavior via appending a fence w.o instruction for WRITE_ONCE(). > > > + > > > + If you don't know what to do here, say "Y". > > > + > > > endmenu # "CPU errata selection" > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > index be84b14f0118..751eb5a7f614 100644 > > > --- a/arch/riscv/errata/thead/errata.c > > > +++ b/arch/riscv/errata/thead/errata.c > > > @@ -69,6 +69,23 @@ static bool errata_probe_pmu(unsigned int stage, > > > return true; > > > } > > > > > > +static bool errata_probe_write_once(unsigned int stage, > > > + unsigned long arch_id, unsigned long impid) > > > +{ > > > + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) > > > + return false; > > > + > > > + /* target-c9xx cores report arch_id and impid as 0 */ > > > + if (arch_id != 0 || impid != 0) > > > + return false; > > > + > > > + if (stage == RISCV_ALTERNATIVES_BOOT || > > > + stage == RISCV_ALTERNATIVES_MODULE) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > static u32 thead_errata_probe(unsigned int stage, > > > unsigned long archid, unsigned long impid) > > > { > > > @@ -83,6 +100,9 @@ static u32 thead_errata_probe(unsigned int stage, > > > if (errata_probe_pmu(stage, archid, impid)) > > > cpu_req_errata |= BIT(ERRATA_THEAD_PMU); > > > > > > + if (errata_probe_write_once(stage, archid, impid)) > > > + cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); > > > + > > > return cpu_req_errata; > > > } > > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > > index 712cab7adffe..fbb2b8d39321 100644 > > > --- a/arch/riscv/include/asm/errata_list.h > > > +++ b/arch/riscv/include/asm/errata_list.h > > > @@ -11,19 +11,6 @@ > > > #include <asm/hwcap.h> > > > #include <asm/vendorid_list.h> > > > > > > -#ifdef CONFIG_ERRATA_SIFIVE > > > -#define ERRATA_SIFIVE_CIP_453 0 > > > -#define ERRATA_SIFIVE_CIP_1200 1 > > > -#define ERRATA_SIFIVE_NUMBER 2 > > > -#endif > > > - > > > -#ifdef CONFIG_ERRATA_THEAD > > > -#define ERRATA_THEAD_PBMT 0 > > > -#define ERRATA_THEAD_CMO 1 > > > -#define ERRATA_THEAD_PMU 2 > > > -#define ERRATA_THEAD_NUMBER 3 > > > -#endif > > > - > > > > Here I understand you are moving stuff from errata_list.h to > > vendorid_list.h. Wouldn't it be better to do this on a separated patch > > before this one? > Okay. > > > > > I understand this is used here, but it looks like it's unrelated. > > > > > #ifdef __ASSEMBLY__ > > > > > > #define ALT_INSN_FAULT(x) \ > > > diff --git a/arch/riscv/include/asm/rwonce.h b/arch/riscv/include/asm/rwonce.h > > > new file mode 100644 > > > index 000000000000..be0b8864969d > > > --- /dev/null > > > +++ b/arch/riscv/include/asm/rwonce.h > > > @@ -0,0 +1,24 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > + > > > +#ifndef __ASM_RWONCE_H > > > +#define __ASM_RWONCE_H > > > + > > > +#include <linux/compiler_types.h> > > > +#include <asm/alternative-macros.h> > > > +#include <asm/vendorid_list.h> > > > + > > > +#define __WRITE_ONCE(x, val) \ > > > +do { \ > > > + *(volatile typeof(x) *)&(x) = (val); \ > > > + asm volatile(ALTERNATIVE( \ > > > + __nops(1), \ > > > + "fence w, o\n\t", \ > > > + THEAD_VENDOR_ID, \ > > > + ERRATA_THEAD_WRITE_ONCE, \ > > > + CONFIG_ERRATA_THEAD_WRITE_ONCE) \ > > > + : : : "memory"); \ > > > +} while (0) > > > + > > > +#include <asm-generic/rwonce.h> > > > + > > > +#endif /* __ASM_RWONCE_H */ > > > > IIUC the idea here is to have an alternative __WRITE_ONCE that replaces the > > asm-generic one. > > > > Honestly, this asm alternative here seems too much information, and too > > cryptic. I mean, yeah in the patch it all makes sense, but I imagine myself > > in the future looking at all this and trying to understand what is going > > on. > > > > Wouldn't it look better to have something like: > > > > ##### > > > > /* Some explanation like the one on Kconfig */ > > > > #define write_once_flush() \ > > do { \ > > asm volatile(ALTERNATIVE( \ > > __nops(1), \ > > "fence w, o\n\t", \ > > THEAD_VENDOR_ID, \ > > ERRATA_THEAD_WRITE_ONCE, \ > > CONFIG_ERRATA_THEAD_WRITE_ONCE) \ > > : : : "memory"); \ > > } while(0) > > > > > > #define __WRITE_ONCE(x, val) \ > > do { \ > > *(volatile typeof(x) *)&(x) = (val); \ > > write_once_flush(); \ > > } while(0) > > > > ##### > > > > > > This way I could quickly see there is a flush after the writting of > > WRITE_ONCE(), and this flush is the above "complicated" asm. > > > > What do you think? > Okay, good point, and I would take it. Thanks! Once you take the above suggestions, please include in your next patch: Reviewed-by: Leonardo Bras <leobras@xxxxxxxxxx> > > > > > > diff --git a/arch/riscv/include/asm/vendorid_list.h b/arch/riscv/include/asm/vendorid_list.h > > > index cb89af3f0704..73078cfe4029 100644 > > > --- a/arch/riscv/include/asm/vendorid_list.h > > > +++ b/arch/riscv/include/asm/vendorid_list.h > > > @@ -8,4 +8,18 @@ > > > #define SIFIVE_VENDOR_ID 0x489 > > > #define THEAD_VENDOR_ID 0x5b7 > > > > > > +#ifdef CONFIG_ERRATA_SIFIVE > > > +#define ERRATA_SIFIVE_CIP_453 0 > > > +#define ERRATA_SIFIVE_CIP_1200 1 > > > +#define ERRATA_SIFIVE_NUMBER 2 > > > +#endif > > > + > > > +#ifdef CONFIG_ERRATA_THEAD > > > +#define ERRATA_THEAD_PBMT 0 > > > +#define ERRATA_THEAD_CMO 1 > > > +#define ERRATA_THEAD_PMU 2 > > > +#define ERRATA_THEAD_WRITE_ONCE 3 > > > +#define ERRATA_THEAD_NUMBER 4 > > > +#endif > > > + > > > #endif > > > diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h > > > index 8d0a6280e982..fb07fe8c6e45 100644 > > > --- a/include/asm-generic/rwonce.h > > > +++ b/include/asm-generic/rwonce.h > > > @@ -50,10 +50,12 @@ > > > __READ_ONCE(x); \ > > > }) > > > > > > +#ifndef __WRITE_ONCE > > > #define __WRITE_ONCE(x, val) \ > > > do { \ > > > *(volatile typeof(x) *)&(x) = (val); \ > > > } while (0) > > > +#endif > > > > > > #define WRITE_ONCE(x, val) \ > > > do { \ > > > -- > > > 2.36.1 > > > > > > > > -- > Best Regards > Guo Ren >