Message-ID: <CAJF2gTRpnaL=K=qGLntG77X61n=Bc011Rq+6045D_HF_raeYYA@mail.gmail.com>
Date: Sat, 20 Sep 2025 14:04:38 +0800
From: Guo Ren <guoren@...nel.org>
To: Xu Lu <luxu.kernel@...edance.com>
Cc: Andrea Parri <parri.andrea@...il.com>, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, paul.walmsley@...ive.com, palmer@...belt.com, 
	aou@...s.berkeley.edu, alex@...ti.fr, ajones@...tanamicro.com, 
	brs@...osinc.com, devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, apw@...onical.com, joe@...ches.com
Subject: Re: [External] Re: [PATCH v2 0/4] riscv: Add Zalasr ISA extension support

On Fri, Sep 19, 2025 at 11:18 AM Xu Lu <luxu.kernel@...edance.com> wrote:
>
> Hi Guo Ren,
>
> Thanks for your advice.
>
> On Fri, Sep 19, 2025 at 12:48 AM Guo Ren <guoren@...nel.org> wrote:
> >
> > On Wed, Sep 17, 2025 at 12:01:34AM -0400, Guo Ren wrote:
> > > On Tue, Sep 02, 2025 at 06:59:15PM +0200, Andrea Parri wrote:
> > > > > Xu Lu (4):
> > > > >   riscv: add ISA extension parsing for Zalasr
> > > > >   dt-bindings: riscv: Add Zalasr ISA extension description
> > > > >   riscv: Instroduce Zalasr instructions
> > > > >   riscv: Use Zalasr for smp_load_acquire/smp_store_release
> > > >
> > > > Informally put, our (Linux) memory consistency model specifies that any
> > > > sequence
> > > >
> > > >   spin_unlock(s);
> > > >   spin_lock(t);
> > > >
> > > > behaves "as it provides at least FENCE.TSO ordering between operations
> > > > which precede the UNLOCK+LOCK sequence and operations which follow the
> > > > sequence".  Unless I missing something, the patch set in question breaks
> > > > such ordering property (on RISC-V): for example, a "release" annotation,
> > > > .RL (as in spin_unlock() -> smp_store_release(), after patch #4) paired
> > > > with an "acquire" fence, FENCE R,RW (as could be found in spin_lock() ->
> > > > atomic_try_cmpxchg_acquire()) do not provide the specified property.
> > > >
> > > > I _think some solutions to the issue above include:
> > > >
> > > >  a) make sure an .RL annotation is always paired with an .AQ annotation
> > > >     and viceversa an .AQ annotation is paired with an .RL annotation
> > > >     (this approach matches the current arm64 approach/implementation);
> > > >
> > > >  b) on the opposite direction, always pair FENCE R,RW (or occasionally
> > > >     FENCE R,R) with FENCE RW,W (this matches the current approach/the
> > > >     current implementation within riscv);
> > > >
> > > >  c) mix the previous two solutions (resp., annotations and fences), but
> > > >     make sure to "upgrade" any releases to provide (insert) a FENCE.TSO.
> > > I prefer option c) at first, it has fewer modification and influence.
> > Another reason is that store-release-to-load-acquire would give out a
> > FENCE rw, rw according to RVWMO PPO 7th rule instead of FENCE.TSO, which
> > is stricter than the Linux requirement you've mentioned.
>
> The existing implementation of spin_unlock, when followed by
> spin_lock, is equal to 'FENCE rw, rw' for operations before
Yes, it's also stricter than option c), the same as ".RL->.AQ" sequence.

You give out another reason for option c).

> spin_unlock
>  and after spin_lock:
>
> spin_unlock:
>     fence rw, w
>     sd
> spin_lock:
>     amocas
>     fence r, rw
>
> The store-release semantics in spin_unlock, is used to ensure that
> when the other cores can watch the store, they must also watch the
> operations before the store, which is a more common case than calling
> spin_unlock immediately followed by spin_lock on the same core. And
> the existing implementation 'fence rw, w' 'fence r, rw' is stricter
> than '.aq' '.rl'. That is why we want to modify it.
>
> I have reimplemented the code and it now looks like the attached text
> file. I will send the patch out later.
>
> Best Regards.
>
> Xu Lu
>
> >
> > >
> > > asm volatile(ALTERNATIVE("fence rw, w;\t\nsb %0, 0(%1)\t\n",  \
> > > -                       SB_RL(%0, %1) "\t\nnop\t\n",          \
> > > +                       SB_RL(%0, %1) "\t\n fence.tso;\t\n",  \
> > >                         0, RISCV_ISA_EXT_ZALASR, 1)           \
> > >                         : : "r" (v), "r" (p) : "memory");     \
> > >
> > > I didn't object option a), and I think it could be done in the future.
> > > Acquire Zalasr extension step by step.
> > >
> > > >
> > > > (a) would align RISC-V and ARM64 (which is a good thing IMO), though it
> > > > is probably the most invasive approach among the three approaches above
> > > > (requiring certain changes to arch/riscv/include/asm/{cmpxchg,atomic}.h,
> > > > which are already relatively messy due to the various ZABHA plus ZACAS
> > > > switches).  Overall, I'm not too exited at the idea of reviewing any of
> > > > those changes, but if the community opts for it, I'll almost definitely
> > > > take a closer look with due calm.  ;-)
> > > >
> > > >   Andrea
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@...ts.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@...ts.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >



-- 
Best Regards
 Guo Ren

Powered by blists - more mailing lists