Re: [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK

[Guo Ren <guoren@kernel.org>]

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.



-- 
Best Regards
 Guo Ren