Re: [PATCH v2 0/4] riscv: Add Zalasr ISA extension support

[Andrea Parri <parri.andrea@gmail.com>]

> 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