linux-kernel - Re: [External] Re: [PATCH v2 0/4] riscv: Add Zalasr ISA extension support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPYmKFvLSri1-PKSHfgG03w3zLbp9w2FaY7v5-jM__LufqPf2Q@mail.gmail.com>
Date: Wed, 3 Sep 2025 19:41:57 +0800
From: Xu Lu <luxu.kernel@...edance.com>
To: Andrea Parri <parri.andrea@...il.com>
Cc: 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

Hi Andrea,

Great catch! Thanks a lot for your review.

The problem comes from the mixed use of acquire/release semantics via
fence and via real ld.aq/sd.rl. I would prefer your method (a). The
existing atomic acquire/release functions' implementation can be
further modified to amocas.sq/amocas.rl/lr.aq/sc.rl. I will send the
next version after I finish it and hope you can help with review then.

Best regards,
Xu Lu

On Wed, Sep 3, 2025 at 12:59 AM Andrea Parri <parri.andrea@...il.com> 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.
>
> (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