| Message ID | 20230802164701.192791-8-guoren@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | riscv: Add Native/Paravirt/CNA qspinlock support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
Hey Guo Ren, On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > According to qspinlock requirements, RISC-V gives out a weak LR/SC > forward progress guarantee which does not satisfy qspinlock. But > many vendors could produce stronger forward guarantee LR/SC to > ensure the xchg_tail could be finished in time on any kind of > hart. T-HEAD is the vendor which implements strong forward > guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD > with errata help. > > T-HEAD early version of processors has the merge buffer delay > problem, so we need ERRATA_WRITEONCE to support qspinlock. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/Kconfig.errata | 13 +++++++++++++ > arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ > arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ > arch/riscv/include/asm/vendorid_list.h | 3 ++- > arch/riscv/kernel/cpufeature.c | 3 ++- > 5 files changed, 61 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > index 4745a5c57e7c..eb43677b13cc 100644 > --- a/arch/riscv/Kconfig.errata > +++ b/arch/riscv/Kconfig.errata > @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE > > If you don't know what to do here, say "Y". > > +config ERRATA_THEAD_QSPINLOCK > + bool "Apply T-Head queued spinlock errata" > + depends on ERRATA_THEAD > + default y > + help > + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to > + match the xchg_tail requirement of qspinlock. > + > + This will apply the QSPINLOCK errata to handle the non-standard > + behavior via using qspinlock instead of ticket_lock. Whatever about the acceptability of anything else in this series, having _stronger_ guarantees is not an erratum, is it? We should not abuse the errata stuff for this IMO. > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index f8dbbe1bbd34..d9694fe40a9a 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > * spinlock value, the only way is to change from queued_spinlock to > * ticket_spinlock, but can not be vice. > */ > - if (!force_qspinlock) { > + if (!force_qspinlock && > + !riscv_has_errata_thead_qspinlock()) { > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); Is this a generic vendor extension (lol @ that misnomer) or is it an erratum? Make your mind up please. As has been said on other series, NAK to using march/vendor/imp IDs for feature probing. I've got some thoughts on other parts of this series too, but I'm not going to spend time on it unless the locking people and Palmer ascent to this series. Cheers, Conor.
On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > Hey Guo Ren, > > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > According to qspinlock requirements, RISC-V gives out a weak LR/SC > > forward progress guarantee which does not satisfy qspinlock. But > > many vendors could produce stronger forward guarantee LR/SC to > > ensure the xchg_tail could be finished in time on any kind of > > hart. T-HEAD is the vendor which implements strong forward > > guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD > > with errata help. > > > > T-HEAD early version of processors has the merge buffer delay > > problem, so we need ERRATA_WRITEONCE to support qspinlock. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/Kconfig.errata | 13 +++++++++++++ > > arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ > > arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ > > arch/riscv/include/asm/vendorid_list.h | 3 ++- > > arch/riscv/kernel/cpufeature.c | 3 ++- > > 5 files changed, 61 insertions(+), 2 deletions(-) > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > index 4745a5c57e7c..eb43677b13cc 100644 > > --- a/arch/riscv/Kconfig.errata > > +++ b/arch/riscv/Kconfig.errata > > @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE > > > > If you don't know what to do here, say "Y". > > > > +config ERRATA_THEAD_QSPINLOCK > > + bool "Apply T-Head queued spinlock errata" > > + depends on ERRATA_THEAD > > + default y > > + help > > + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to > > + match the xchg_tail requirement of qspinlock. > > + > > + This will apply the QSPINLOCK errata to handle the non-standard > > + behavior via using qspinlock instead of ticket_lock. > > Whatever about the acceptability of anything else in this series, > having _stronger_ guarantees is not an erratum, is it? We should not > abuse the errata stuff for this IMO. > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index f8dbbe1bbd34..d9694fe40a9a 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > > * spinlock value, the only way is to change from queued_spinlock to > > * ticket_spinlock, but can not be vice. > > */ > > - if (!force_qspinlock) { > > + if (!force_qspinlock && > > + !riscv_has_errata_thead_qspinlock()) { > > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > > Is this a generic vendor extension (lol @ that misnomer) or is it an > erratum? Make your mind up please. As has been said on other series, NAK > to using march/vendor/imp IDs for feature probing. The RISCV_ISA_EXT_XTICKETLOCK is a feature extension number, and it's set by default for forward-compatible. We also define a vendor extension (riscv_has_errata_thead_qspinlock) to force all our processors to use qspinlock; others still stay on ticket_lock. The only possible changing direction is from qspinlock to ticket_lock because ticket_lock would dirty the lock value, which prevents changing to qspinlock next. So startup with qspinlock and change to ticket_lock before smp up. You also could use cmdline to try qspinlock (force_qspinlock). > > I've got some thoughts on other parts of this series too, but I'm not > going to spend time on it unless the locking people and Palmer ascent > to this series. > > Cheers, > Conor.
On Fri, Aug 04, 2023 at 05:53:35PM +0800, Guo Ren wrote: > On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@kernel.org wrote: > > > From: Guo Ren <guoren@linux.alibaba.com> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index f8dbbe1bbd34..d9694fe40a9a 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > > > * spinlock value, the only way is to change from queued_spinlock to > > > * ticket_spinlock, but can not be vice. > > > */ > > > - if (!force_qspinlock) { > > > + if (!force_qspinlock && > > > + !riscv_has_errata_thead_qspinlock()) { > > > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > > > > Is this a generic vendor extension (lol @ that misnomer) or is it an > > erratum? Make your mind up please. As has been said on other series, NAK > > to using march/vendor/imp IDs for feature probing. > > The RISCV_ISA_EXT_XTICKETLOCK is a feature extension number, No, that is not what "ISA_EXT" means, nor what the X in "XTICKETLOCK" would imply. The comment above these reads: These macros represent the logical IDs of each multi-letter RISC-V ISA extension and are used in the ISA bitmap. > and it's > set by default for forward-compatible. We also define a vendor > extension (riscv_has_errata_thead_qspinlock) to force all our > processors to use qspinlock; others still stay on ticket_lock. No, "riscv_has_errata_thead_qspinlock()" would be an _erratum_, not a vendor extension. We need to have a discussion about how to support non-standard extensions etc, not abuse errata. That discussion has been started on the v0.7.1 vector patches, but has not made progress yet. > The only possible changing direction is from qspinlock to ticket_lock > because ticket_lock would dirty the lock value, which prevents > changing to qspinlock next. So startup with qspinlock and change to > ticket_lock before smp up. You also could use cmdline to try qspinlock > (force_qspinlock). I don't see what the relevance of this is, sorry. I am only commenting on how you are deciding that the hardware is capable of using qspinlocks, I don't intend getting into the detail unless the powers that be deem this series worthwhile, as I mentioned: > > I've got some thoughts on other parts of this series too, but I'm not > > going to spend time on it unless the locking people and Palmer ascent > > to this series.
On Fri, Aug 4, 2023 at 6:07 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > On Fri, Aug 04, 2023 at 05:53:35PM +0800, Guo Ren wrote: > > On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > > index f8dbbe1bbd34..d9694fe40a9a 100644 > > > > --- a/arch/riscv/kernel/cpufeature.c > > > > +++ b/arch/riscv/kernel/cpufeature.c > > > > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > > > > * spinlock value, the only way is to change from queued_spinlock to > > > > * ticket_spinlock, but can not be vice. > > > > */ > > > > - if (!force_qspinlock) { > > > > + if (!force_qspinlock && > > > > + !riscv_has_errata_thead_qspinlock()) { > > > > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > > > > > > Is this a generic vendor extension (lol @ that misnomer) or is it an > > > erratum? Make your mind up please. As has been said on other series, NAK > > > to using march/vendor/imp IDs for feature probing. > > > > The RISCV_ISA_EXT_XTICKETLOCK is a feature extension number, > > No, that is not what "ISA_EXT" means, nor what the X in "XTICKETLOCK" > would imply. > > The comment above these reads: > These macros represent the logical IDs of each multi-letter RISC-V ISA > extension and are used in the ISA bitmap. > > > and it's > > set by default for forward-compatible. We also define a vendor > > extension (riscv_has_errata_thead_qspinlock) to force all our > > processors to use qspinlock; others still stay on ticket_lock. > > No, "riscv_has_errata_thead_qspinlock()" would be an _erratum_, not a > vendor extension. We need to have a discussion about how to support > non-standard extensions etc, not abuse errata. That discussion has been > started on the v0.7.1 vector patches, but has not made progress yet. You convinced me, yes, I abuse errata here. I would change to Linux standard static_key mechanism next. > > > The only possible changing direction is from qspinlock to ticket_lock > > because ticket_lock would dirty the lock value, which prevents > > changing to qspinlock next. So startup with qspinlock and change to > > ticket_lock before smp up. You also could use cmdline to try qspinlock > > (force_qspinlock). > > I don't see what the relevance of this is, sorry. I am only commenting > on how you are deciding that the hardware is capable of using qspinlocks, > I don't intend getting into the detail unless the powers that be deem > this series worthwhile, as I mentioned: > > > I've got some thoughts on other parts of this series too, but I'm not > > > going to spend time on it unless the locking people and Palmer ascent > > > to this series. > -- Best Regards Guo Ren
On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > According to qspinlock requirements, RISC-V gives out a weak LR/SC > forward progress guarantee which does not satisfy qspinlock. But > many vendors could produce stronger forward guarantee LR/SC to > ensure the xchg_tail could be finished in time on any kind of > hart. T-HEAD is the vendor which implements strong forward > guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD > with errata help. > > T-HEAD early version of processors has the merge buffer delay > problem, so we need ERRATA_WRITEONCE to support qspinlock. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/Kconfig.errata | 13 +++++++++++++ > arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ > arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ > arch/riscv/include/asm/vendorid_list.h | 3 ++- > arch/riscv/kernel/cpufeature.c | 3 ++- > 5 files changed, 61 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > index 4745a5c57e7c..eb43677b13cc 100644 > --- a/arch/riscv/Kconfig.errata > +++ b/arch/riscv/Kconfig.errata > @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE > > If you don't know what to do here, say "Y". > > +config ERRATA_THEAD_QSPINLOCK > + bool "Apply T-Head queued spinlock errata" > + depends on ERRATA_THEAD > + default y > + help > + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to > + match the xchg_tail requirement of qspinlock. > + > + This will apply the QSPINLOCK errata to handle the non-standard > + behavior via using qspinlock instead of ticket_lock. > + > + If you don't know what to do here, say "Y". If this is to be applied, I would like to see a detailed explanation somewhere, preferably with citations, of: (a) The memory model requirements for qspinlock (b) Why, with arguments, RISC-V does not architecturally meet (a) (c) Why, with arguments, T-HEAD C9xx meets (a) (d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS meets (a) As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops (livelock freedom but no starvation freedom) are exactly the same as those in Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a constrained LR/SC loop with guaranteed eventual success (with -O1). Clearly you disagree; I would like to see your perspective. -s > + > endmenu # "CPU errata selection" > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index 881729746d2e..d560dc45c0e7 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage, > return false; > } > > +static bool errata_probe_qspinlock(unsigned int stage, > + unsigned long arch_id, unsigned long impid) > +{ > + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK)) > + return false; > + > + /* > + * The queued_spinlock torture would get in livelock without > + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD > + * processors. > + */ > + if (arch_id == 0 && impid == 0 && > + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) > + return false; > + > + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > + return true; > + > + return false; > +} > + > static u32 thead_errata_probe(unsigned int stage, > unsigned long archid, unsigned long impid) > { > @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage, > if (errata_probe_write_once(stage, archid, impid)) > cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); > > + if (errata_probe_qspinlock(stage, archid, impid)) > + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK); > + > return cpu_req_errata; > } > > diff --git a/arch/riscv/include/asm/errata_list.h > b/arch/riscv/include/asm/errata_list.h > index fbb2b8d39321..a696d18d1b0d 100644 > --- a/arch/riscv/include/asm/errata_list.h > +++ b/arch/riscv/include/asm/errata_list.h > @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \ > : "=r" (__ovl) : \ > : "memory") > > +static __always_inline bool > +riscv_has_errata_thead_qspinlock(void) > +{ > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + asm_volatile_goto( > + ALTERNATIVE( > + "j %l[l_no]", "nop", > + THEAD_VENDOR_ID, > + ERRATA_THEAD_QSPINLOCK, > + CONFIG_ERRATA_THEAD_QSPINLOCK) > + : : : : l_no); > + } else { > + goto l_no; > + } > + > + return true; > +l_no: > + return false; > +} > + > #endif /* __ASSEMBLY__ */ > > #endif > diff --git a/arch/riscv/include/asm/vendorid_list.h > b/arch/riscv/include/asm/vendorid_list.h > index 73078cfe4029..1f1d03877f5f 100644 > --- a/arch/riscv/include/asm/vendorid_list.h > +++ b/arch/riscv/include/asm/vendorid_list.h > @@ -19,7 +19,8 @@ > #define ERRATA_THEAD_CMO 1 > #define ERRATA_THEAD_PMU 2 > #define ERRATA_THEAD_WRITE_ONCE 3 > -#define ERRATA_THEAD_NUMBER 4 > +#define ERRATA_THEAD_QSPINLOCK 4 > +#define ERRATA_THEAD_NUMBER 5 > #endif > > #endif > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index f8dbbe1bbd34..d9694fe40a9a 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > * spinlock value, the only way is to change from queued_spinlock to > * ticket_spinlock, but can not be vice. > */ > - if (!force_qspinlock) { > + if (!force_qspinlock && > + !riscv_has_errata_thead_qspinlock()) { > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > } > #endif > -- > 2.36.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Aug 07, 2023 at 01:23:34AM -0400, Stefan O'Rear wrote: > On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > According to qspinlock requirements, RISC-V gives out a weak LR/SC > > forward progress guarantee which does not satisfy qspinlock. But > > many vendors could produce stronger forward guarantee LR/SC to > > ensure the xchg_tail could be finished in time on any kind of > > hart. T-HEAD is the vendor which implements strong forward > > guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD > > with errata help. > > > > T-HEAD early version of processors has the merge buffer delay > > problem, so we need ERRATA_WRITEONCE to support qspinlock. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/Kconfig.errata | 13 +++++++++++++ > > arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ > > arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ > > arch/riscv/include/asm/vendorid_list.h | 3 ++- > > arch/riscv/kernel/cpufeature.c | 3 ++- > > 5 files changed, 61 insertions(+), 2 deletions(-) > > > > diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > > index 4745a5c57e7c..eb43677b13cc 100644 > > --- a/arch/riscv/Kconfig.errata > > +++ b/arch/riscv/Kconfig.errata > > @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE > > > > If you don't know what to do here, say "Y". > > > > +config ERRATA_THEAD_QSPINLOCK > > + bool "Apply T-Head queued spinlock errata" > > + depends on ERRATA_THEAD > > + default y > > + help > > + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to > > + match the xchg_tail requirement of qspinlock. > > + > > + This will apply the QSPINLOCK errata to handle the non-standard > > + behavior via using qspinlock instead of ticket_lock. > > + > > + If you don't know what to do here, say "Y". > > If this is to be applied, I would like to see a detailed explanation somewhere, > preferably with citations, of: > > (a) The memory model requirements for qspinlock These were written in commit: a8ad07e5240 ("asm-generic: qspinlock: Indicate the use of mixed-size atomics"). For riscv, the most controversial point is xchg_tail() implementation for native queued spinlock. > (b) Why, with arguments, RISC-V does not architecturally meet (a) In the spec "Eventual Success of Store-Conditional Instructions": "By contrast, if other harts or devices continue to write to that reservation set, it is not guaranteed that any hart will exit its LR/SC loop." 1. The arch_spinlock_t is 32-bit width, and it contains LOCK_PENDING part and IDX_TAIL part. - LOCK: lock holder - PENDING: next waiter (Only once per contended situation) - IDX: nested context (normal, hwirq, softirq, nmi) - TAIL: last contended cpu The xchg_tail operate on IDX_TAIL part, so there is no guarantee on "NO" "other harts or devices continue to write to that reservation set". 2. When you do lock torture test, you may see a long contended ring queue: xchg_tail +-----> CPU4 (big core) | CPU3 (lock holder) -> CPU1 (mcs queued) -> CPU2 (mcs queued) ----+-----> CPU0 (little core) | | | +-----> CPU5 (big core) | | +--locktorture release lock (spin_unlock) and spin_lock again --+-----> CPU3 (big core) If CPU0 doesn't have a strong fwd guarantee, xhg_tail is consistently failed. > (c) Why, with arguments, T-HEAD C9xx meets (a) > (d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS > meets (a) I can't give the C9xx microarch implementation detail. But many open-source riscv cores have provided strong forward progress guarantee LR/SC implementation [1] [2]. But I would say these implementations are too rude, which makes LR send a cacheline unique interconnect request. It satisfies xchg_tail but not cmpxchg & cond_load. CPU vendors should carefully consider your LR/SC fwd guarantee implementation. [1]: https://github.com/riscv-boom/riscv-boom/blob/v3.0.0/src/main/scala/lsu/dcache.scala#L650 [2]: https://github.com/OpenXiangShan/XiangShan/blob/v1.0/src/main/scala/xiangshan/cache/MainPipe.scala#L470 > > As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops > (livelock freedom but no starvation freedom) are exactly the same as those in > Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a > constrained LR/SC loop with guaranteed eventual success (with -O1). Clearly you > disagree; I would like to see your perspective. For Armv8, I would use LSE for the lock-contended scenario. Ref this commit 0ea366f5e1b6: ("arm64: atomics: prefetch the destination word for write prior to stxr"). > > -s > > > + > > endmenu # "CPU errata selection" > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index 881729746d2e..d560dc45c0e7 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage, > > return false; > > } > > > > +static bool errata_probe_qspinlock(unsigned int stage, > > + unsigned long arch_id, unsigned long impid) > > +{ > > + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK)) > > + return false; > > + > > + /* > > + * The queued_spinlock torture would get in livelock without > > + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD > > + * processors. > > + */ > > + if (arch_id == 0 && impid == 0 && > > + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) > > + return false; > > + > > + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > + return true; > > + > > + return false; > > +} > > + > > static u32 thead_errata_probe(unsigned int stage, > > unsigned long archid, unsigned long impid) > > { > > @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage, > > if (errata_probe_write_once(stage, archid, impid)) > > cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); > > > > + if (errata_probe_qspinlock(stage, archid, impid)) > > + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK); > > + > > return cpu_req_errata; > > } > > > > diff --git a/arch/riscv/include/asm/errata_list.h > > b/arch/riscv/include/asm/errata_list.h > > index fbb2b8d39321..a696d18d1b0d 100644 > > --- a/arch/riscv/include/asm/errata_list.h > > +++ b/arch/riscv/include/asm/errata_list.h > > @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \ > > : "=r" (__ovl) : \ > > : "memory") > > > > +static __always_inline bool > > +riscv_has_errata_thead_qspinlock(void) > > +{ > > + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > > + asm_volatile_goto( > > + ALTERNATIVE( > > + "j %l[l_no]", "nop", > > + THEAD_VENDOR_ID, > > + ERRATA_THEAD_QSPINLOCK, > > + CONFIG_ERRATA_THEAD_QSPINLOCK) > > + : : : : l_no); > > + } else { > > + goto l_no; > > + } > > + > > + return true; > > +l_no: > > + return false; > > +} > > + > > #endif /* __ASSEMBLY__ */ > > > > #endif > > diff --git a/arch/riscv/include/asm/vendorid_list.h > > b/arch/riscv/include/asm/vendorid_list.h > > index 73078cfe4029..1f1d03877f5f 100644 > > --- a/arch/riscv/include/asm/vendorid_list.h > > +++ b/arch/riscv/include/asm/vendorid_list.h > > @@ -19,7 +19,8 @@ > > #define ERRATA_THEAD_CMO 1 > > #define ERRATA_THEAD_PMU 2 > > #define ERRATA_THEAD_WRITE_ONCE 3 > > -#define ERRATA_THEAD_NUMBER 4 > > +#define ERRATA_THEAD_QSPINLOCK 4 > > +#define ERRATA_THEAD_NUMBER 5 > > #endif > > > > #endif > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index f8dbbe1bbd34..d9694fe40a9a 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > > * spinlock value, the only way is to change from queued_spinlock to > > * ticket_spinlock, but can not be vice. > > */ > > - if (!force_qspinlock) { > > + if (!force_qspinlock && > > + !riscv_has_errata_thead_qspinlock()) { > > set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > > } > > #endif > > -- > > 2.36.1 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@fastmail.com wrote: > On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote: >> From: Guo Ren <guoren@linux.alibaba.com> >> >> According to qspinlock requirements, RISC-V gives out a weak LR/SC >> forward progress guarantee which does not satisfy qspinlock. But >> many vendors could produce stronger forward guarantee LR/SC to >> ensure the xchg_tail could be finished in time on any kind of >> hart. T-HEAD is the vendor which implements strong forward >> guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD >> with errata help. >> >> T-HEAD early version of processors has the merge buffer delay >> problem, so we need ERRATA_WRITEONCE to support qspinlock. >> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> >> Signed-off-by: Guo Ren <guoren@kernel.org> >> --- >> arch/riscv/Kconfig.errata | 13 +++++++++++++ >> arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ >> arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ >> arch/riscv/include/asm/vendorid_list.h | 3 ++- >> arch/riscv/kernel/cpufeature.c | 3 ++- >> 5 files changed, 61 insertions(+), 2 deletions(-) >> >> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata >> index 4745a5c57e7c..eb43677b13cc 100644 >> --- a/arch/riscv/Kconfig.errata >> +++ b/arch/riscv/Kconfig.errata >> @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE >> >> If you don't know what to do here, say "Y". >> >> +config ERRATA_THEAD_QSPINLOCK >> + bool "Apply T-Head queued spinlock errata" >> + depends on ERRATA_THEAD >> + default y >> + help >> + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to >> + match the xchg_tail requirement of qspinlock. >> + >> + This will apply the QSPINLOCK errata to handle the non-standard >> + behavior via using qspinlock instead of ticket_lock. >> + >> + If you don't know what to do here, say "Y". > > If this is to be applied, I would like to see a detailed explanation somewhere, > preferably with citations, of: > > (a) The memory model requirements for qspinlock > (b) Why, with arguments, RISC-V does not architecturally meet (a) > (c) Why, with arguments, T-HEAD C9xx meets (a) > (d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS > meets (a) I agree. Just having a magic fence that makes qspinlocks stop livelocking on some processors is going to lead to a mess -- I'd argue this means those processors just don't provide the forward progress guarantee, but we'd really need something written down about what this new custom instruction aliasing as a fence does. > As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops > (livelock freedom but no starvation freedom) are exactly the same as those in > Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a > constrained LR/SC loop with guaranteed eventual success (with -O1). Clearly you > disagree; I would like to see your perspective. It sounds to me like this processor might be quite broken: if it's permanently holding stores in a buffer we're going to have more issues than just qspinlock, pretty much anything concurrent is going to have issues -- and that's not just in the kernel, there's concurrent userspace code as well. > -s > >> + >> endmenu # "CPU errata selection" >> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c >> index 881729746d2e..d560dc45c0e7 100644 >> --- a/arch/riscv/errata/thead/errata.c >> +++ b/arch/riscv/errata/thead/errata.c >> @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage, >> return false; >> } >> >> +static bool errata_probe_qspinlock(unsigned int stage, >> + unsigned long arch_id, unsigned long impid) >> +{ >> + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK)) >> + return false; >> + >> + /* >> + * The queued_spinlock torture would get in livelock without >> + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD >> + * processors. >> + */ >> + if (arch_id == 0 && impid == 0 && >> + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) >> + return false; >> + >> + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) >> + return true; >> + >> + return false; >> +} >> + >> static u32 thead_errata_probe(unsigned int stage, >> unsigned long archid, unsigned long impid) >> { >> @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage, >> if (errata_probe_write_once(stage, archid, impid)) >> cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); >> >> + if (errata_probe_qspinlock(stage, archid, impid)) >> + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK); >> + >> return cpu_req_errata; >> } >> >> diff --git a/arch/riscv/include/asm/errata_list.h >> b/arch/riscv/include/asm/errata_list.h >> index fbb2b8d39321..a696d18d1b0d 100644 >> --- a/arch/riscv/include/asm/errata_list.h >> +++ b/arch/riscv/include/asm/errata_list.h >> @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \ >> : "=r" (__ovl) : \ >> : "memory") >> >> +static __always_inline bool >> +riscv_has_errata_thead_qspinlock(void) >> +{ >> + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { >> + asm_volatile_goto( >> + ALTERNATIVE( >> + "j %l[l_no]", "nop", >> + THEAD_VENDOR_ID, >> + ERRATA_THEAD_QSPINLOCK, >> + CONFIG_ERRATA_THEAD_QSPINLOCK) >> + : : : : l_no); >> + } else { >> + goto l_no; >> + } >> + >> + return true; >> +l_no: >> + return false; >> +} >> + >> #endif /* __ASSEMBLY__ */ >> >> #endif >> diff --git a/arch/riscv/include/asm/vendorid_list.h >> b/arch/riscv/include/asm/vendorid_list.h >> index 73078cfe4029..1f1d03877f5f 100644 >> --- a/arch/riscv/include/asm/vendorid_list.h >> +++ b/arch/riscv/include/asm/vendorid_list.h >> @@ -19,7 +19,8 @@ >> #define ERRATA_THEAD_CMO 1 >> #define ERRATA_THEAD_PMU 2 >> #define ERRATA_THEAD_WRITE_ONCE 3 >> -#define ERRATA_THEAD_NUMBER 4 >> +#define ERRATA_THEAD_QSPINLOCK 4 >> +#define ERRATA_THEAD_NUMBER 5 >> #endif >> >> #endif >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index f8dbbe1bbd34..d9694fe40a9a 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) >> * spinlock value, the only way is to change from queued_spinlock to >> * ticket_spinlock, but can not be vice. >> */ >> - if (!force_qspinlock) { >> + if (!force_qspinlock && >> + !riscv_has_errata_thead_qspinlock()) { >> set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); >> } >> #endif >> -- >> 2.36.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 9/13/23 14:54, Palmer Dabbelt wrote: > On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@fastmail.com wrote: >> On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote: >>> From: Guo Ren <guoren@linux.alibaba.com> >>> >>> According to qspinlock requirements, RISC-V gives out a weak LR/SC >>> forward progress guarantee which does not satisfy qspinlock. But >>> many vendors could produce stronger forward guarantee LR/SC to >>> ensure the xchg_tail could be finished in time on any kind of >>> hart. T-HEAD is the vendor which implements strong forward >>> guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD >>> with errata help. >>> >>> T-HEAD early version of processors has the merge buffer delay >>> problem, so we need ERRATA_WRITEONCE to support qspinlock. >>> >>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> >>> Signed-off-by: Guo Ren <guoren@kernel.org> >>> --- >>> arch/riscv/Kconfig.errata | 13 +++++++++++++ >>> arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ >>> arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ >>> arch/riscv/include/asm/vendorid_list.h | 3 ++- >>> arch/riscv/kernel/cpufeature.c | 3 ++- >>> 5 files changed, 61 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata >>> index 4745a5c57e7c..eb43677b13cc 100644 >>> --- a/arch/riscv/Kconfig.errata >>> +++ b/arch/riscv/Kconfig.errata >>> @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE >>> >>> If you don't know what to do here, say "Y". >>> >>> +config ERRATA_THEAD_QSPINLOCK >>> + bool "Apply T-Head queued spinlock errata" >>> + depends on ERRATA_THEAD >>> + default y >>> + help >>> + The T-HEAD C9xx processors implement strong fwd guarantee >>> LR/SC to >>> + match the xchg_tail requirement of qspinlock. >>> + >>> + This will apply the QSPINLOCK errata to handle the non-standard >>> + behavior via using qspinlock instead of ticket_lock. >>> + >>> + If you don't know what to do here, say "Y". >> >> If this is to be applied, I would like to see a detailed explanation >> somewhere, >> preferably with citations, of: >> >> (a) The memory model requirements for qspinlock The part of qspinlock that causes problem with many RISC architectures is its use of a 16-bit xchg() function call which many RISC architectures cannot do it natively and have to be emulated with hopefully some forward progress guarantee. Except that one call, the other atomic operations are all 32 bit in size. Cheers, Longman
On Thu, Sep 14, 2023 at 2:54 AM Palmer Dabbelt <palmer@rivosinc.com> wrote: > > On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@fastmail.com wrote: > > On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote: > >> From: Guo Ren <guoren@linux.alibaba.com> > >> > >> According to qspinlock requirements, RISC-V gives out a weak LR/SC > >> forward progress guarantee which does not satisfy qspinlock. But > >> many vendors could produce stronger forward guarantee LR/SC to > >> ensure the xchg_tail could be finished in time on any kind of > >> hart. T-HEAD is the vendor which implements strong forward > >> guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD > >> with errata help. > >> > >> T-HEAD early version of processors has the merge buffer delay > >> problem, so we need ERRATA_WRITEONCE to support qspinlock. > >> > >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > >> Signed-off-by: Guo Ren <guoren@kernel.org> > >> --- > >> arch/riscv/Kconfig.errata | 13 +++++++++++++ > >> arch/riscv/errata/thead/errata.c | 24 ++++++++++++++++++++++++ > >> arch/riscv/include/asm/errata_list.h | 20 ++++++++++++++++++++ > >> arch/riscv/include/asm/vendorid_list.h | 3 ++- > >> arch/riscv/kernel/cpufeature.c | 3 ++- > >> 5 files changed, 61 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata > >> index 4745a5c57e7c..eb43677b13cc 100644 > >> --- a/arch/riscv/Kconfig.errata > >> +++ b/arch/riscv/Kconfig.errata > >> @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE > >> > >> If you don't know what to do here, say "Y". > >> > >> +config ERRATA_THEAD_QSPINLOCK > >> + bool "Apply T-Head queued spinlock errata" > >> + depends on ERRATA_THEAD > >> + default y > >> + help > >> + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to > >> + match the xchg_tail requirement of qspinlock. > >> + > >> + This will apply the QSPINLOCK errata to handle the non-standard > >> + behavior via using qspinlock instead of ticket_lock. > >> + > >> + If you don't know what to do here, say "Y". > > > > If this is to be applied, I would like to see a detailed explanation somewhere, > > preferably with citations, of: > > > > (a) The memory model requirements for qspinlock > > (b) Why, with arguments, RISC-V does not architecturally meet (a) > > (c) Why, with arguments, T-HEAD C9xx meets (a) > > (d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS > > meets (a) > > I agree. > > Just having a magic fence that makes qspinlocks stop livelocking on some > processors is going to lead to a mess -- I'd argue this means those > processors just don't provide the forward progress guarantee, but we'd > really need something written down about what this new custom > instruction aliasing as a fence does. The "magic fence" is not related to the LR/SC forward progress guarantee, and it's our processors' store buffer hardware problem that needs to be fixed. Not only is qspinlock suffering on it, but also kernel/locking/osq_lock.c. This is about this patch: https://lore.kernel.org/linux-riscv/20230910082911.3378782-10-guoren@kernel.org/ I've written down the root cause of that patch: The "new custom fence instruction" triggers the store buffer flush. Only the normal store instruction has the problem, and atomic stores (including LR/SC) are okay. > > > As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops > > (livelock freedom but no starvation freedom) are exactly the same as those in > > Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a > > constrained LR/SC loop with guaranteed eventual success (with -O1). Clearly you > > disagree; I would like to see your perspective. > > It sounds to me like this processor might be quite broken: if it's > permanently holding stores in a buffer we're going to have more issues > than just qspinlock, pretty much anything concurrent is going to have > issues -- and that's not just in the kernel, there's concurrent > userspace code as well. Yes, the userspace scenarios are our worries because modifying the userspace atomic library is impossible. Now, we are stress-testing various userspace parallel applications, especially userspace queued spinlock, on the 128-core hardware platform. We haven't detected the problem in the userspace, and it could be because of timer interrupts, which breaks the store buffer waiting. > > > -s > > > >> + > >> endmenu # "CPU errata selection" > >> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > >> index 881729746d2e..d560dc45c0e7 100644 > >> --- a/arch/riscv/errata/thead/errata.c > >> +++ b/arch/riscv/errata/thead/errata.c > >> @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage, > >> return false; > >> } > >> > >> +static bool errata_probe_qspinlock(unsigned int stage, > >> + unsigned long arch_id, unsigned long impid) > >> +{ > >> + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK)) > >> + return false; > >> + > >> + /* > >> + * The queued_spinlock torture would get in livelock without > >> + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD > >> + * processors. > >> + */ > >> + if (arch_id == 0 && impid == 0 && > >> + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) > >> + return false; > >> + > >> + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > >> + return true; > >> + > >> + return false; > >> +} > >> + > >> static u32 thead_errata_probe(unsigned int stage, > >> unsigned long archid, unsigned long impid) > >> { > >> @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage, > >> if (errata_probe_write_once(stage, archid, impid)) > >> cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); > >> > >> + if (errata_probe_qspinlock(stage, archid, impid)) > >> + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK); > >> + > >> return cpu_req_errata; > >> } > >> > >> diff --git a/arch/riscv/include/asm/errata_list.h > >> b/arch/riscv/include/asm/errata_list.h > >> index fbb2b8d39321..a696d18d1b0d 100644 > >> --- a/arch/riscv/include/asm/errata_list.h > >> +++ b/arch/riscv/include/asm/errata_list.h > >> @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \ > >> : "=r" (__ovl) : \ > >> : "memory") > >> > >> +static __always_inline bool > >> +riscv_has_errata_thead_qspinlock(void) > >> +{ > >> + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > >> + asm_volatile_goto( > >> + ALTERNATIVE( > >> + "j %l[l_no]", "nop", > >> + THEAD_VENDOR_ID, > >> + ERRATA_THEAD_QSPINLOCK, > >> + CONFIG_ERRATA_THEAD_QSPINLOCK) > >> + : : : : l_no); > >> + } else { > >> + goto l_no; > >> + } > >> + > >> + return true; > >> +l_no: > >> + return false; > >> +} > >> + > >> #endif /* __ASSEMBLY__ */ > >> > >> #endif > >> diff --git a/arch/riscv/include/asm/vendorid_list.h > >> b/arch/riscv/include/asm/vendorid_list.h > >> index 73078cfe4029..1f1d03877f5f 100644 > >> --- a/arch/riscv/include/asm/vendorid_list.h > >> +++ b/arch/riscv/include/asm/vendorid_list.h > >> @@ -19,7 +19,8 @@ > >> #define ERRATA_THEAD_CMO 1 > >> #define ERRATA_THEAD_PMU 2 > >> #define ERRATA_THEAD_WRITE_ONCE 3 > >> -#define ERRATA_THEAD_NUMBER 4 > >> +#define ERRATA_THEAD_QSPINLOCK 4 > >> +#define ERRATA_THEAD_NUMBER 5 > >> #endif > >> > >> #endif > >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > >> index f8dbbe1bbd34..d9694fe40a9a 100644 > >> --- a/arch/riscv/kernel/cpufeature.c > >> +++ b/arch/riscv/kernel/cpufeature.c > >> @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) > >> * spinlock value, the only way is to change from queued_spinlock to > >> * ticket_spinlock, but can not be vice. > >> */ > >> - if (!force_qspinlock) { > >> + if (!force_qspinlock && > >> + !riscv_has_errata_thead_qspinlock()) { > >> set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); > >> } > >> #endif > >> -- > >> 2.36.1 > >> > >> > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata index 4745a5c57e7c..eb43677b13cc 100644 --- a/arch/riscv/Kconfig.errata +++ b/arch/riscv/Kconfig.errata @@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE If you don't know what to do here, say "Y". +config ERRATA_THEAD_QSPINLOCK + bool "Apply T-Head queued spinlock errata" + depends on ERRATA_THEAD + default y + help + The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to + match the xchg_tail requirement of qspinlock. + + This will apply the QSPINLOCK errata to handle the non-standard + behavior via using qspinlock instead of ticket_lock. + + 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 881729746d2e..d560dc45c0e7 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage, return false; } +static bool errata_probe_qspinlock(unsigned int stage, + unsigned long arch_id, unsigned long impid) +{ + if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK)) + return false; + + /* + * The queued_spinlock torture would get in livelock without + * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD + * processors. + */ + if (arch_id == 0 && impid == 0 && + !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE)) + return false; + + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) + return true; + + return false; +} + static u32 thead_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid) { @@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage, if (errata_probe_write_once(stage, archid, impid)) cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE); + if (errata_probe_qspinlock(stage, archid, impid)) + cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK); + return cpu_req_errata; } diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h index fbb2b8d39321..a696d18d1b0d 100644 --- a/arch/riscv/include/asm/errata_list.h +++ b/arch/riscv/include/asm/errata_list.h @@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE( \ : "=r" (__ovl) : \ : "memory") +static __always_inline bool +riscv_has_errata_thead_qspinlock(void) +{ + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + asm_volatile_goto( + ALTERNATIVE( + "j %l[l_no]", "nop", + THEAD_VENDOR_ID, + ERRATA_THEAD_QSPINLOCK, + CONFIG_ERRATA_THEAD_QSPINLOCK) + : : : : l_no); + } else { + goto l_no; + } + + return true; +l_no: + return false; +} + #endif /* __ASSEMBLY__ */ #endif diff --git a/arch/riscv/include/asm/vendorid_list.h b/arch/riscv/include/asm/vendorid_list.h index 73078cfe4029..1f1d03877f5f 100644 --- a/arch/riscv/include/asm/vendorid_list.h +++ b/arch/riscv/include/asm/vendorid_list.h @@ -19,7 +19,8 @@ #define ERRATA_THEAD_CMO 1 #define ERRATA_THEAD_PMU 2 #define ERRATA_THEAD_WRITE_ONCE 3 -#define ERRATA_THEAD_NUMBER 4 +#define ERRATA_THEAD_QSPINLOCK 4 +#define ERRATA_THEAD_NUMBER 5 #endif #endif diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index f8dbbe1bbd34..d9694fe40a9a 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void) * spinlock value, the only way is to change from queued_spinlock to * ticket_spinlock, but can not be vice. */ - if (!force_qspinlock) { + if (!force_qspinlock && + !riscv_has_errata_thead_qspinlock()) { set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa); } #endif