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
