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

Conor Dooley
2023-08-04T10:07:41Z

[-- Attachment #1.1: Type: text/plain, Size: 2714 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]