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? 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? > 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 >