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@xxxxxxxxxxxxx> wrote:
> > On Wed, Aug 02, 2023 at 12:46:49PM -0400, guoren@xxxxxxxxxx wrote:
> > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > 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:
signature.asc
Description: PGP signature
