The fully ordered versions of xchg[8|16]() using LR/SC lack the
necessary memory barriers to guarantee the order.
Fix this by matching what is already implemented in the fully ordered
versions of cmpxchg() using LR/SC.
Suggested-by: Andrea Parri <[email protected]>
Reported-by: Andrea Parri <[email protected]>
Closes: https://lore.kernel.org/linux-riscv/ZlYbupL5XgzgA0MX@andrea/T/#u
Fixes: a8ed2b7a2c13 ("riscv/cmpxchg: Implement xchg for variables of size 1 and 2")
Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/riscv/include/asm/cmpxchg.h | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index ddb002ed89de..e1e564f5dc7b 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -10,7 +10,7 @@
#include <asm/fence.h>
-#define __arch_xchg_masked(prepend, append, r, p, n) \
+#define __arch_xchg_masked(sc_sfx, prepend, append, r, p, n) \
({ \
u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3); \
ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE; \
@@ -25,7 +25,7 @@
"0: lr.w %0, %2\n" \
" and %1, %0, %z4\n" \
" or %1, %1, %z3\n" \
- " sc.w %1, %1, %2\n" \
+ " sc.w" sc_sfx " %1, %1, %2\n" \
" bnez %1, 0b\n" \
append \
: "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b)) \
@@ -46,7 +46,7 @@
: "memory"); \
})
-#define _arch_xchg(ptr, new, sfx, prepend, append) \
+#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, append) \
({ \
__typeof__(ptr) __ptr = (ptr); \
__typeof__(*(__ptr)) __new = (new); \
@@ -55,15 +55,15 @@
switch (sizeof(*__ptr)) { \
case 1: \
case 2: \
- __arch_xchg_masked(prepend, append, \
+ __arch_xchg_masked(sc_sfx, prepend, append, \
__ret, __ptr, __new); \
break; \
case 4: \
- __arch_xchg(".w" sfx, prepend, append, \
+ __arch_xchg(".w" swap_sfx, prepend, append, \
__ret, __ptr, __new); \
break; \
case 8: \
- __arch_xchg(".d" sfx, prepend, append, \
+ __arch_xchg(".d" swap_sfx, prepend, append, \
__ret, __ptr, __new); \
break; \
default: \
@@ -73,16 +73,16 @@
})
#define arch_xchg_relaxed(ptr, x) \
- _arch_xchg(ptr, x, "", "", "")
+ _arch_xchg(ptr, x, "", "", "", "")
#define arch_xchg_acquire(ptr, x) \
- _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER)
+ _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
#define arch_xchg_release(ptr, x) \
- _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "")
+ _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
#define arch_xchg(ptr, x) \
- _arch_xchg(ptr, x, ".aqrl", "", "")
+ _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
#define xchg32(ptr, x) \
({ \
--
2.39.2
> -#define _arch_xchg(ptr, new, sfx, prepend, append) \
> +#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, append) \
> ({ \
> __typeof__(ptr) __ptr = (ptr); \
> __typeof__(*(__ptr)) __new = (new); \
> @@ -55,15 +55,15 @@
> switch (sizeof(*__ptr)) { \
> case 1: \
> case 2: \
> - __arch_xchg_masked(prepend, append, \
> + __arch_xchg_masked(sc_sfx, prepend, append, \
> __ret, __ptr, __new); \
> break; \
> case 4: \
> - __arch_xchg(".w" sfx, prepend, append, \
> + __arch_xchg(".w" swap_sfx, prepend, append, \
> __ret, __ptr, __new); \
> break; \
> case 8: \
> - __arch_xchg(".d" sfx, prepend, append, \
> + __arch_xchg(".d" swap_sfx, prepend, append, \
> __ret, __ptr, __new); \
> break; \
> default: \
> @@ -73,16 +73,16 @@
> })
>
> #define arch_xchg_relaxed(ptr, x) \
> - _arch_xchg(ptr, x, "", "", "")
> + _arch_xchg(ptr, x, "", "", "", "")
>
> #define arch_xchg_acquire(ptr, x) \
> - _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER)
> + _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
>
> #define arch_xchg_release(ptr, x) \
> - _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "")
> + _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
>
> #define arch_xchg(ptr, x) \
> - _arch_xchg(ptr, x, ".aqrl", "", "")
> + _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
This does indeed fix the fully-ordered variant of xchg8/16(). But this
also changes the fully-ordered xchg32() to
amoswap.w.aqrl a4,a5,(s1)
fence rw,rw
(and similarly for xchg64()); we should be able to restore the original
mapping with the diff below on top of this patch.
Andrea
P.S. Perhaps expand the width of the macros to avoid newlines (I didn't
do it keep the diff smaller).
P.S. With Zabha, we'd probably like to pass swap_sfx and swap_append as
well to __arch_xchg_masked().
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index e1e564f5dc7ba..88c8bb7ec1c34 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -46,7 +46,8 @@
: "memory"); \
})
-#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, append) \
+#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, \
+ sc_append, swap_append) \
({ \
__typeof__(ptr) __ptr = (ptr); \
__typeof__(*(__ptr)) __new = (new); \
@@ -55,15 +56,15 @@
switch (sizeof(*__ptr)) { \
case 1: \
case 2: \
- __arch_xchg_masked(sc_sfx, prepend, append, \
+ __arch_xchg_masked(sc_sfx, prepend, sc_append, \
__ret, __ptr, __new); \
break; \
case 4: \
- __arch_xchg(".w" swap_sfx, prepend, append, \
+ __arch_xchg(".w" swap_sfx, prepend, swap_append, \
__ret, __ptr, __new); \
break; \
case 8: \
- __arch_xchg(".d" swap_sfx, prepend, append, \
+ __arch_xchg(".d" swap_sfx, prepend, swap_append, \
__ret, __ptr, __new); \
break; \
default: \
@@ -73,16 +74,16 @@
})
#define arch_xchg_relaxed(ptr, x) \
- _arch_xchg(ptr, x, "", "", "", "")
+ _arch_xchg(ptr, x, "", "", "", "", "")
#define arch_xchg_acquire(ptr, x) \
- _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
+ _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER, RISCV_ACQUIRE_BARRIER)
#define arch_xchg_release(ptr, x) \
- _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
+ _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "", "")
#define arch_xchg(ptr, x) \
- _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
+ _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n", "")
#define xchg32(ptr, x) \
({ \
Andrea,
On Thu, May 30, 2024 at 1:54 PM Andrea Parri <[email protected]> wrote:
>
> > -#define _arch_xchg(ptr, new, sfx, prepend, append) \
> > +#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, append) \
> > ({ \
> > __typeof__(ptr) __ptr = (ptr); \
> > __typeof__(*(__ptr)) __new = (new); \
> > @@ -55,15 +55,15 @@
> > switch (sizeof(*__ptr)) { \
> > case 1: \
> > case 2: \
> > - __arch_xchg_masked(prepend, append, \
> > + __arch_xchg_masked(sc_sfx, prepend, append, \
> > __ret, __ptr, __new); \
> > break; \
> > case 4: \
> > - __arch_xchg(".w" sfx, prepend, append, \
> > + __arch_xchg(".w" swap_sfx, prepend, append, \
> > __ret, __ptr, __new); \
> > break; \
> > case 8: \
> > - __arch_xchg(".d" sfx, prepend, append, \
> > + __arch_xchg(".d" swap_sfx, prepend, append, \
> > __ret, __ptr, __new); \
> > break; \
> > default: \
> > @@ -73,16 +73,16 @@
> > })
> >
> > #define arch_xchg_relaxed(ptr, x) \
> > - _arch_xchg(ptr, x, "", "", "")
> > + _arch_xchg(ptr, x, "", "", "", "")
> >
> > #define arch_xchg_acquire(ptr, x) \
> > - _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER)
> > + _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
> >
> > #define arch_xchg_release(ptr, x) \
> > - _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "")
> > + _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
> >
> > #define arch_xchg(ptr, x) \
> > - _arch_xchg(ptr, x, ".aqrl", "", "")
> > + _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
>
> This does indeed fix the fully-ordered variant of xchg8/16(). But this
> also changes the fully-ordered xchg32() to
>
> amoswap.w.aqrl a4,a5,(s1)
> fence rw,rw
>
> (and similarly for xchg64()); we should be able to restore the original
> mapping with the diff below on top of this patch.
And you already told me that privately...Sorry, my mind has been
elsewhere lately...I'll fix that right now.
Sorry again and thanks,
Alex
>
> Andrea
>
> P.S. Perhaps expand the width of the macros to avoid newlines (I didn't
> do it keep the diff smaller).
>
> P.S. With Zabha, we'd probably like to pass swap_sfx and swap_append as
> well to __arch_xchg_masked().
>
>
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index e1e564f5dc7ba..88c8bb7ec1c34 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -46,7 +46,8 @@
> : "memory"); \
> })
>
> -#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, append) \
> +#define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend, \
> + sc_append, swap_append) \
> ({ \
> __typeof__(ptr) __ptr = (ptr); \
> __typeof__(*(__ptr)) __new = (new); \
> @@ -55,15 +56,15 @@
> switch (sizeof(*__ptr)) { \
> case 1: \
> case 2: \
> - __arch_xchg_masked(sc_sfx, prepend, append, \
> + __arch_xchg_masked(sc_sfx, prepend, sc_append, \
> __ret, __ptr, __new); \
> break; \
> case 4: \
> - __arch_xchg(".w" swap_sfx, prepend, append, \
> + __arch_xchg(".w" swap_sfx, prepend, swap_append, \
> __ret, __ptr, __new); \
> break; \
> case 8: \
> - __arch_xchg(".d" swap_sfx, prepend, append, \
> + __arch_xchg(".d" swap_sfx, prepend, swap_append, \
> __ret, __ptr, __new); \
> break; \
> default: \
> @@ -73,16 +74,16 @@
> })
>
> #define arch_xchg_relaxed(ptr, x) \
> - _arch_xchg(ptr, x, "", "", "", "")
> + _arch_xchg(ptr, x, "", "", "", "", "")
>
> #define arch_xchg_acquire(ptr, x) \
> - _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
> + _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER, RISCV_ACQUIRE_BARRIER)
>
> #define arch_xchg_release(ptr, x) \
> - _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
> + _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "", "")
>
> #define arch_xchg(ptr, x) \
> - _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
> + _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n", "")
>
> #define xchg32(ptr, x) \
> ({ \
>
> And you already told me that privately...Sorry, my mind has been
> elsewhere lately...I'll fix that right now.
Np. While at it, one nit below.
> > #define arch_xchg_relaxed(ptr, x) \
> > - _arch_xchg(ptr, x, "", "", "", "")
> > + _arch_xchg(ptr, x, "", "", "", "", "")
> >
> > #define arch_xchg_acquire(ptr, x) \
> > - _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER)
> > + _arch_xchg(ptr, x, "", "", "", RISCV_ACQUIRE_BARRIER, RISCV_ACQUIRE_BARRIER)
> >
> > #define arch_xchg_release(ptr, x) \
> > - _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "")
> > + _arch_xchg(ptr, x, "", "", RISCV_RELEASE_BARRIER, "", "")
> >
> > #define arch_xchg(ptr, x) \
> > - _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n")
> > + _arch_xchg(ptr, x, ".rl", ".aqrl", "", " fence rw, rw\n", "")
The plain string can be replaced with RISCV_FULL_BARRIER (cf. asm/fence.h)
to match the style/approach used elsewhere in this file.
Andrea