On Fri, Aug 4, 2023 at 5:06 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote: > > Hey Guo Ren, > > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@xxxxxxxxxx wrote: > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxxxx> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx> > > --- > > 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. -- Best Regards Guo Ren