Introduce cmpxchg128() -- aka. the demise of cmpxchg_double()

As seems to be the common (majority) usage in crypto, use __uint128_t instead of u128.

This frees up u128 for definition in linux/types.h.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 lib/crypto/curve25519-hacl64.c |  142 ++++++++++++++++++++---------------------
 lib/crypto/poly1305-donna64.c  |   22 ++----
 2 files changed, 80 insertions(+), 84 deletions(-)

lib/crypto/curve25519-hacl64.c +0 -2

@@ -14,8 +14,6 @@
 #include <crypto/curve25519.h>
 #include <linux/string.h>
 
-typedef __uint128_t u128;
-
 static __always_inline u64 u64_eq_mask(u64 a, u64 b)
 {
 	u64 x = a ^ b;

Why not just use u128 from types.h in this file?

Quoted 1 line · Jason A. Donenfeld

Why not just use u128 from types.h in this file?

Ordering, I can't very well introduce it in types.h while other definitions exist in the tree. So I first have to clean up the u128 namespace.

Is there a patch at the end of the series that adds it back in to use u128?

Quoted 2 lines · Jason A. Donenfeld

Is there a patch at the end of the series that adds it back in to use u128?

Could we do some ifdef trickery to reduce the amount of code churn please? Changing everything away from u128 and then back to it seems silly.

Thanks,

Seems like "merging common code snippets" is something we at least used to do with single patches...

Even though x86 is firmly little endian, use be128 because le128 is in fact the wrong way around :/ The actual code is already using be128 in ghash_setkey() so this shouldn't be more confusing.

This frees up the u128 name for a real u128 type.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

cyrpto/b128ops: Remove struct u128

Per git-grep u128_xor() and its related struct u128 are unused except to implement {be,le}128_xor(). Remove them to free up the namespace.

Introduce [us]128 (when available). Unlike [us]64, ensure they are always naturally aligned.

This also enables 128bit wide atomics (which require natural alignment) such as cmpxchg128().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/types.h      |    5 +++++
 include/uapi/linux/types.h |    4 ++++
 2 files changed, 9 insertions(+)

Introduce [us]128 (when available). Unlike [us]64, ensure they are always naturally aligned.

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +17 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\

arch/arm64/include/asm/atomic_ll_sc.h +11 -0

+	: "r" (o.low), "r" (o.high), "r" (n.low), "r" (n.high)		\
+	: cl);								\
+									\
+	return r.full;							\
+}
+
+__CMPXCHG128(   ,        ,  ,         )
+__CMPXCHG128(_mb, dmb ish, l, "memory")
+
+#undef __CMPXCHG128
+
 #undef K
 
 #endif	/* __ASM_ATOMIC_LL_SC_H */

Quoted 43 lines · Peter Zijlstra

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +23 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\
+	"1:	ldxp	%0, %1, %2\n"					\
+	"	eor	%3, %0, %3\n"					\
+	"	eor	%4, %1, %4\n"					\
+	"	orr	%3, %4, %3\n"					\
+	"	cbnz	%3, 2f\n"					\
+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\

arch/arm64/include/asm/atomic_ll_sc.h __CMPXCHG_DBL( , , , ) · line 355

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h +4 -0

+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

Quoted 4 lines · Boqun Feng

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

In this I've copied the existing cmpxchg_double() code; I'll have to let the arch folks speak here, I've no clue.

Quoted 7 lines · Peter Zijlstra

Quoted 4 lines · Boqun Feng

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

Quoted 2 lines

In this I've copied the existing cmpxchg_double() code; I'll have to let the arch folks speak here, I've no clue.

It does sound like the right thing to do. I doubt it ends up making a difference in practice, but yes, the asm doesn't have a memory clobber, so the input/output types should be the right ones for the compiler to not possibly do something odd and cache the part that it doesn't see as being accessed.

Quoted 44 lines · Boqun Feng

Quoted 43 lines · Peter Zijlstra

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +23 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\
+	"1:	ldxp	%0, %1, %2\n"					\
+	"	eor	%3, %0, %3\n"					\
+	"	eor	%4, %1, %4\n"					\
+	"	orr	%3, %4, %3\n"					\
+	"	cbnz	%3, 2f\n"					\
+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\

Quoted 8 lines

arch/arm64/include/asm/atomic_ll_sc.h __CMPXCHG_DBL( , , , ) · line 355

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h +4 -0

+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

In this I've copied the existing cmpxchg_double() code; I'll have to let the arch folks speak here, I've no clue.

Quoted 54 lines · Peter Zijlstra

Quoted 44 lines · Boqun Feng

Quoted 43 lines · Peter Zijlstra

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +23 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\
+	"1:	ldxp	%0, %1, %2\n"					\
+	"	eor	%3, %0, %3\n"					\
+	"	eor	%4, %1, %4\n"					\
+	"	orr	%3, %4, %3\n"					\
+	"	cbnz	%3, 2f\n"					\
+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\

Quoted 8 lines

arch/arm64/include/asm/atomic_ll_sc.h __CMPXCHG_DBL( , , , ) · line 355

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h +4 -0

+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

Quoted 2 lines

In this I've copied the existing cmpxchg_double() code; I'll have to let the arch folks speak here, I've no clue.

We definitely need to ensure the compiler sees we poke the whole thing, or it can get this horribly wrong, so that is a latent bug.

See commit:

fee960bed5e857eb ("arm64: xchg: hazard against entire exchange variable")

... for examples of GCC being clever, where I overlooked the *_double() cases.

Quoted 58 lines · Mark Rutland

Quoted 54 lines · Peter Zijlstra

Quoted 44 lines · Boqun Feng

Quoted 43 lines · Peter Zijlstra

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +23 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\
+	"1:	ldxp	%0, %1, %2\n"					\
+	"	eor	%3, %0, %3\n"					\
+	"	eor	%4, %1, %4\n"					\
+	"	orr	%3, %4, %3\n"					\
+	"	cbnz	%3, 2f\n"					\
+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\

Quoted 8 lines

arch/arm64/include/asm/atomic_ll_sc.h __CMPXCHG_DBL( , , , ) · line 355

Quoted 4 lines

arch/arm64/include/asm/atomic_ll_sc.h +4 -0

+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\

I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned long *) ptr)"? Because compilers may think only 64bit value pointed by "ptr" gets modified, and they are allowed to do "useful" optimization.

Quoted 2 lines

In this I've copied the existing cmpxchg_double() code; I'll have to let the arch folks speak here, I've no clue.

Quoted 8 lines

We definitely need to ensure the compiler sees we poke the whole thing, or it can get this horribly wrong, so that is a latent bug.

See commit:

fee960bed5e857eb ("arm64: xchg: hazard against entire exchange variable")

... for examples of GCC being clever, where I overlooked the *_double() cases.

Using __uint128_t instead, e.g.

arch/arm64/include/asm/atomic_ll_sc.h +1 -1

index 0890e4f568fb7..cbb3d961123b1 100644
@@ -315,7 +315,7 @@ __ll_sc__cmpxchg_double##name(unsigned long old1,                   \
        "       cbnz    %w0, 1b\n"                                      \
        "       " #mb "\n"                                              \
        "2:"                                                            \
-       : "=&r" (tmp), "=&r" (ret), "+Q" (*(unsigned long *)ptr)        \
+       : "=&r" (tmp), "=&r" (ret), "+Q" (*(__uint128_t *)ptr)          \
        : "r" (old1), "r" (old2), "r" (new1), "r" (new2)                \
        : cl);                                                          \
                                                                        \

arch/arm64/include/asm/atomic_lse.h +1 -1

index 52075e93de6c0..a94d6dacc0292 100644
@@ -311,7 +311,7 @@ __lse__cmpxchg_double##name(unsigned long old1,                             \
        "       eor     %[old2], %[old2], %[oldval2]\n"                 \
        "       orr     %[old1], %[old1], %[old2]"                      \
        : [old1] "+&r" (x0), [old2] "+&r" (x1),                         \
-         [v] "+Q" (*(unsigned long *)ptr)                              \
+         [v] "+Q" (*(__uint128_t *)ptr)                                \
        : [new1] "r" (x2), [new2] "r" (x3), [ptr] "r" (x4),             \
          [oldval1] "r" (oldval1), [oldval2] "r" (oldval2)              \
        : cl);                                                          \

... makes GCC much happier:

... I'll go check whether clang is happy with that, and how far back that can go, otherwise we'll need to blat the high half with a separate constaint that (ideally) doesn't end up allocating a pointless address register.

Quoted 4 lines · Mark Rutland

Quoted 1 line · Mark Rutland

On Tue, Dec 20, 2022 at 12:08:16PM +0100, Peter Zijlstra wrote:

... makes GCC much happier:

Quoted 3 lines

... I'll go check whether clang is happy with that, and how far back that can go, otherwise we'll need to blat the high half with a separate constaint that (ideally) doesn't end up allocating a pointless address register.

Hmm... from the commit history it looks like GCC prior to 5.1 might not be happy with that, but that *might* just be if we actually do arithmetic on the value, and we might be ok just using it for memroy effects. I can't currently get such an old GCC to run on my machines so I haven't been able to check.

Quoted 6 lines · Mark Rutland

Quoted 4 lines · Mark Rutland

Quoted 1 line · Mark Rutland

On Tue, Dec 20, 2022 at 12:08:16PM +0100, Peter Zijlstra wrote:

... makes GCC much happier:

Quoted 8 lines

Quoted 3 lines

... I'll go check whether clang is happy with that, and how far back that can go, otherwise we'll need to blat the high half with a separate constaint that (ideally) doesn't end up allocating a pointless address register.

Hmm... from the commit history it looks like GCC prior to 5.1 might not be happy with that, but that *might* just be if we actually do arithmetic on the value, and we might be ok just using it for memroy effects. I can't currently get such an old GCC to run on my machines so I haven't been able to check.

gcc-5.1 is the oldest (barely) supported compiler, the minimum was last raised from gcc-4.9 in linux-5.15. If only gcc-4.9 and older are affected, we're good on mainline but may still want a fix for stable kernels.

I checked that the cross-compiler binaries from [1] still work, but I noticed that this version is missing the native aarch64-to-aarch64 compiler (x86 to aarch64 and vice versa are there), and you need to install libmpfr4 [2] as a dependency. The newer compilers (6.5.0 and up) don't have these problems.

Arnd

[1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/arm64/5.5.0/
[2] http://ftp.uk.debian.org/debian/pool/main/m/mpfr4/libmpfr4_3.1.5-1_arm64.deb

Quoted 16 lines · Arnd Bergmann

Quoted 14 lines · Mark Rutland

Quoted 8 lines · Mark Rutland

Quoted 1 line · Mark Rutland

On Tue, Dec 20, 2022 at 12:08:16PM +0100, Peter Zijlstra wrote:

... makes GCC much happier:

... I'll go check whether clang is happy with that, and how far back that can go, otherwise we'll need to blat the high half with a separate constaint that (ideally) doesn't end up allocating a pointless address register.

Hmm... from the commit history it looks like GCC prior to 5.1 might not be happy with that, but that *might* just be if we actually do arithmetic on the value, and we might be ok just using it for memroy effects. I can't currently get such an old GCC to run on my machines so I haven't been able to check.

Quoted 4 lines

gcc-5.1 is the oldest (barely) supported compiler, the minimum was last raised from gcc-4.9 in linux-5.15. If only gcc-4.9 and older are affected, we're good on mainline but may still want a fix for stable kernels.

Yup; I just wanted something that would easily backport to stable, at least as far as linux-4.9.y (where I couldn't find the minimum GCC version when I looked yesterday).

Quoted 4 lines

I checked that the cross-compiler binaries from [1] still work, but I noticed that this version is missing the native aarch64-to-aarch64 compiler (x86 to aarch64 and vice versa are there), and you need to install libmpfr4 [2] as a dependency. The newer compilers (6.5.0 and up) don't have these problems.

I was trying the old kernel.org crosstool binaries, but I was either missing a library (or I have an incompatible version) on my x86_64 host. I'll have another look today -- thanks for the pointers!

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +38 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+									\
+	asm volatile("// __cmpxchg128" #name "\n"			\
+	"	prfm	pstl1strm, %2\n"				\
+	"1:	ldxp	%0, %1, %2\n"					\
+	"	eor	%3, %0, %3\n"					\
+	"	eor	%4, %1, %4\n"					\
+	"	orr	%3, %4, %3\n"					\
+	"	cbnz	%3, 2f\n"					\
+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\
+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\
+	: "r" (o.low), "r" (o.high), "r" (n.low), "r" (n.high)		\
+	: cl);								\
+									\
+	return r.full;							\
+}
+
+__CMPXCHG128(   ,        ,  ,         )
+__CMPXCHG128(_mb, dmb ish, l, "memory")
+
+#undef __CMPXCHG128
+
 #undef K
 
 #endif	/* __ASM_ATOMIC_LL_SC_H */

arch/arm64/include/asm/atomic_lse.h +9 -1

@@ -151,7 +151,7 @@ __lse_atomic64_fetch_##op##name(s64 i, a
 	"	" #asm_op #mb "	%[i], %[old], %[v]"			\
 	: [v] "+Q" (v->counter),					\
 	  [old] "=r" (old)						\
-	: [i] "r" (i) 							\
+	: [i] "r" (i)							\
 	: cl);								\
 									\
 	return old;							\

@@ -324,4 +324,35 @@ __CMPXCHG_DBL(_mb, al, "memory")
 
 #undef __CMPXCHG_DBL
 
+#define __CMPXCHG128(name, mb, cl...)					\
+static __always_inline u128						\
+__lse__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)		\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+	register unsigned long x0 asm ("x0") = o.low;			\
+	register unsigned long x1 asm ("x1") = o.high;			\

arch/arm64/include/asm/atomic_lse.h +12 -0

+	: cl);								\
+									\
+	r.low = x0; r.high = x1;					\
+									\
+	return r.full;							\
+}
+
+__CMPXCHG128(   ,   )
+__CMPXCHG128(_mb, al, "memory")
+
+#undef __CMPXCHG128
+
 #endif	/* __ASM_ATOMIC_LSE_H */

arch/arm64/include/asm/cmpxchg.h +24 -0

+				      u128 old, u128 new)		\
+{									\
+	return __lse_ll_sc_body(_cmpxchg128##name,			\
+				ptr, old, new);				\
+}
+
+__CMPXCHG128(   )
+__CMPXCHG128(_mb)
+
+#undef __CMPXCHG128
+
 #define __CMPXCHG_GEN(sfx)						\
 static __always_inline unsigned long __cmpxchg##sfx(volatile void *ptr,	\
 					   unsigned long old,		\

@@ -229,6 +242,19 @@ __CMPXCHG_GEN(_mb)
 	__ret;									\
 })
 
+/* cmpxchg128 */
+#define system_has_cmpxchg128()		1
+
+#define arch_cmpxchg128(ptr, o, n)						\
+({										\
+	__cmpxchg128_mb((ptr), (o), (n));					\
+})
+
+#define arch_cmpxchg128_local(ptr, o, n)					\
+({										\
+	__cmpxchg128((ptr), (o), (n));						\
+})
+
 #define __CMPWAIT_CASE(w, sfx, sz)					\
 static inline void __cmpwait_case_##sz(volatile void *ptr,		\
 				       unsigned long val)		\

arch/s390/include/asm/cmpxchg.h +33 -0

@@ -201,4 +201,37 @@ static __always_inline int __cmpxchg_dou
 			 (unsigned long)(n1), (unsigned long)(n2));	\
 })
 
+#define system_has_cmpxchg128()		1
+
+static __always_inline u128 arch_cmpxchg128(volatile u128 *ptr, u128 old, u128 new)
+{
+	asm volatile(
+		"	cdsg	%[old],%[new],%[ptr]\n"
+		: [old] "+&d" (old)
+		: [new] "d" (new),
+		  [ptr] "QS" (*(unsigned long *)ptr)
+		: "memory", "cc");
+	return old;
+}
+
+static __always_inline bool arch_try_cmpxchg128(volatile u128 *ptr, u128 *oldp, u128 new)
+{
+	u128 old = *oldp;
+	int cc;
+
+	asm volatile(
+		"	cdsg	%[old],%[new],%[ptr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [cc] "=&d" (cc), [old] "+&d" (old)
+		: [new] "d" (new),
+		  [ptr] "QS" (*(unsigned long *)ptr)
+		: "memory", "cc");
+
+	if (unlikely(!cc))
+		*oldp = old;
+
+	return likely(cc);
+}
+
 #endif /* __ASM_CMPXCHG_H */

arch/x86/include/asm/cmpxchg_32.h +2 -1

@@ -103,6 +103,7 @@ static inline bool __try_cmpxchg64(volat
 
 #endif
 
-#define system_has_cmpxchg_double() boot_cpu_has(X86_FEATURE_CX8)
+#define system_has_cmpxchg_double()	boot_cpu_has(X86_FEATURE_CX8)
+#define system_has_cmpxchg64()		boot_cpu_has(X86_FEATURE_CX8)
 
 #endif /* _ASM_X86_CMPXCHG_32_H */

arch/x86/include/asm/cmpxchg_64.h +54 -1

@@ -20,6 +20,59 @@
 	arch_try_cmpxchg((ptr), (po), (n));				\
 })
 
-#define system_has_cmpxchg_double() boot_cpu_has(X86_FEATURE_CX16)
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+static __always_inline u128 arch_cmpxchg128(volatile u128 *ptr, u128 old, u128 new)
+{
+	union __u128_halves o = { .full = old, }, n = { .full = new, };
+
+	asm volatile(LOCK_PREFIX "cmpxchg16b %[ptr]"
+		     : [ptr] "+m" (*ptr),
+		       "+a" (o.low), "+d" (o.high)
+		     : "b" (n.low), "c" (n.high)
+		     : "memory");
+
+	return o.full;
+}
+
+static __always_inline u128 arch_cmpxchg128_local(volatile u128 *ptr, u128 old, u128 new)
+{
+	union __u128_halves o = { .full = old, }, n = { .full = new, };
+
+	asm volatile("cmpxchg16b %[ptr]"
+		     : [ptr] "+m" (*ptr),
+		       "+a" (o.low), "+d" (o.high)
+		     : "b" (n.low), "c" (n.high)
+		     : "memory");
+
+	return o.full;
+}
+
+static __always_inline bool arch_try_cmpxchg128(volatile u128 *ptr, u128 *old, u128 new)
+{
+	union __u128_halves o = { .full = *old, }, n = { .full = new, };
+	bool ret;
+
+	asm volatile(LOCK_PREFIX "cmpxchg16b %[ptr]"
+		     CC_SET(e)
+		     : CC_OUT(e) (ret),
+		       [ptr] "+m" (*ptr),
+		       "+a" (o.low), "+d" (o.high)
+		     : "b" (n.low), "c" (n.high)
+		     : "memory");
+
+	if (unlikely(!ret))
+		*old = o.full;
+
+	return likely(ret);
+}
+
+#define system_has_cmpxchg_double()	boot_cpu_has(X86_FEATURE_CX16)
+#define system_has_cmpxchg128()		boot_cpu_has(X86_FEATURE_CX16)
 
 #endif /* _ASM_X86_CMPXCHG_64_H */

Quoted 8 lines

arch/arm64/include/asm/atomic_lse.h +8 -0

+#define __CMPXCHG128(name, mb, cl...)					\
+static __always_inline u128						\
+__lse__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)		\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+	register unsigned long x0 asm ("x0") = o.low;			\
+	register unsigned long x1 asm ("x1") = o.high;			\

arch/arm64/include/asm/atomic_lse.h __CMPXCHG_DBL(_mb, al, "memory") · line 344

Quoted 10 lines

arch/arm64/include/asm/atomic_lse.h +10 -0

+	register unsigned long x2 asm ("x2") = n.low;			\
+	register unsigned long x3 asm ("x3") = n.high;			\
+	register unsigned long x4 asm ("x4") = (unsigned long)ptr;	\
+									\
+	asm volatile(							\
+	__LSE_PREAMBLE							\
+	"	casp" #mb "\t%[old1], %[old2], %[new1], %[new2], %[v]\n"\
+	: [old1] "+&r" (x0), [old2] "+&r" (x1),				\
+	  [v] "+Q" (*(unsigned long *)ptr)				\
+	: [new1] "r" (x2), [new2] "r" (x3), [ptr] "r" (x4),		\

Issue #1: the line below can be removed, otherwise..

arch/arm64/include/asm/atomic_lse.h __CMPXCHG_DBL(_mb, al, "memory") · line 345

Quoted 1 line

arch/arm64/include/asm/atomic_lse.h +1 -0

+	  [oldval1] "r" (r.low), [oldval2] "r" (r.high)			\

warning:

	./arch/arm64/include/asm/atomic_lse.h: In function '__lse__cmpxchg128_mb':
	./arch/arm64/include/asm/atomic_lse.h:309:27: warning: 'r.<U97b8>.low' is used uninitialized [-Wuninitialized]
	  309 |           [oldval1] "r" (r.low), [oldval2] "r" (r.high)

Quoted 19 lines

arch/arm64/include/asm/atomic_lse.h +12 -0

+	: cl);								\
+									\
+	r.low = x0; r.high = x1;					\
+									\
+	return r.full;							\
+}
+
+__CMPXCHG128(   ,   )
+__CMPXCHG128(_mb, al, "memory")
+
+#undef __CMPXCHG128
+
 #endif	/* __ASM_ATOMIC_LSE_H */

arch/arm64/include/asm/cmpxchg.h +0 -0

@@ -147,6 +147,19 @@ __CMPXCHG_DBL(_mb)
 
 #undef __CMPXCHG_DBL
 

arch/arm64/include/asm/cmpxchg.h __CMPXCHG_DBL(_mb) · line 151

Quoted 2 lines

arch/arm64/include/asm/cmpxchg.h +2 -0

+#define __CMPXCHG128(name)						\
+static inline long __cmpxchg128##name(volatile u128 *ptr,		\

Issue #2: this should be

static inline u128 __cmpxchg128##name(..)

because cmpxchg* needs to return the old value.

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/s390/include/asm/cmpxchg.h +23 -0

@@ -201,4 +201,37 @@ static __always_inline int __cmpxchg_dou
 			 (unsigned long)(n1), (unsigned long)(n2));	\
 })
 
+#define system_has_cmpxchg128()		1
+
+static __always_inline u128 arch_cmpxchg128(volatile u128 *ptr, u128 old, u128 new)
+{
+	asm volatile(
+		"	cdsg	%[old],%[new],%[ptr]\n"
+		: [old] "+&d" (old)
+		: [new] "d" (new),
+		  [ptr] "QS" (*(unsigned long *)ptr)
+		: "memory", "cc");
+	return old;
+}
+
+static __always_inline bool arch_try_cmpxchg128(volatile u128 *ptr, u128 *oldp, u128 new)
+{
+	u128 old = *oldp;
+	int cc;
+
+	asm volatile(
+		"	cdsg	%[old],%[new],%[ptr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [cc] "=&d" (cc), [old] "+&d" (old)

For all architectures that currently support cmpxchg_double() implement the cmpxchg128() family of functions that is basically the same but with a saner interface.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/arm64/include/asm/atomic_ll_sc.h |   38 +++++++++++++++++++++++
 arch/arm64/include/asm/atomic_lse.h   |   33 +++++++++++++++++++-
 arch/arm64/include/asm/cmpxchg.h      |   26 ++++++++++++++++
 arch/s390/include/asm/cmpxchg.h       |   33 ++++++++++++++++++++
 arch/x86/include/asm/cmpxchg_32.h     |    3 +
 arch/x86/include/asm/cmpxchg_64.h     |   55 +++++++++++++++++++++++++++++++++-
 6 files changed, 185 insertions(+), 3 deletions(-)

arch/arm64/include/asm/atomic_ll_sc.h +12 -0

@@ -326,6 +326,44 @@ __CMPXCHG_DBL(   ,        ,  ,         )
 __CMPXCHG_DBL(_mb, dmb ish, l, "memory")
 
 #undef __CMPXCHG_DBL
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __CMPXCHG128(name, mb, rel, cl)					\
+static __always_inline u128						\
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)	\
+{									\

arch/arm64/include/asm/atomic_ll_sc.h +5 -0

+	"	st" #rel "xp	%w3, %5, %6, %2\n"			\
+	"	cbnz	%w3, 1b\n"					\
+	"	" #mb "\n"						\
+	"2:"								\
+	: "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr)	\

arch/arm64/include/asm/atomic_ll_sc.h +1 -0

+
 #undef K
 
 #endif	/* __ASM_ATOMIC_LL_SC_H */

arch/arm64/include/asm/atomic_lse.h +20 -0

@@ -324,4 +324,35 @@ __CMPXCHG_DBL(_mb, al, "memory")
 
 #undef __CMPXCHG_DBL
 
+#define __CMPXCHG128(name, mb, cl...)					\
+static __always_inline u128						\
+__lse__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)		\
+{									\
+	union __u128_halves r, o = { .full = (old) },			\
+			       n = { .full = (new) };			\
+	register unsigned long x0 asm ("x0") = o.low;			\
+	register unsigned long x1 asm ("x1") = o.high;			\
+	register unsigned long x2 asm ("x2") = n.low;			\
+	register unsigned long x3 asm ("x3") = n.high;			\
+	register unsigned long x4 asm ("x4") = (unsigned long)ptr;	\
+									\
+	asm volatile(							\
+	__LSE_PREAMBLE							\
+	"	casp" #mb "\t%[old1], %[old2], %[new1], %[new2], %[v]\n"\
+	: [old1] "+&r" (x0), [old2] "+&r" (x1),				\
+	  [v] "+Q" (*(unsigned long *)ptr)				\
+	: [new1] "r" (x2), [new2] "r" (x3), [ptr] "r" (x4),		\
+	  [oldval1] "r" (r.low), [oldval2] "r" (r.high)			\
+	: cl);								\

Similarly, I'd suggest:

| #define __CMPXCHG128(name, mb, cl...)                                   \
| static __always_inline u128                                             \
| __lse__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new)         \
| {                                                                       \
|         asm volatile(                                                   \
|         __LSE_PREAMBLE                                                  \
|         "       casp" #mb "\t%[old], %H[old], %[new], %H[new], %[v]\n"  \
|         : [old] "+&r" (old),                                            \
|           [v] "+Q" (*(u128 *)ptr)                                       \
|         : [new] "r" (new)                                               \
|         : cl);                                                          \

|                                                                         \
|         return old;                                                     \
| }
| 
| __CMPXCHG128(   ,   )   
| __CMPXCHG128(_mb, al, "memory")
| 
| #undef __CMPXCHG128

In order to replace cmpxchg_double() with the newly minted cmpxchg128() family of functions, wire it up in this_cpu_cmpxchg().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

In order to replace cmpxchg_double() with the newly minted cmpxchg128() family of functions, wire it up in this_cpu_cmpxchg().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Quoted 4 lines · Peter Zijlstra

In order to replace cmpxchg_double() with the newly minted cmpxchg128() family of functions, wire it up in this_cpu_cmpxchg().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

...

arch/s390/include/asm/percpu.h

Quoted 6 lines

arch/s390/include/asm/percpu.h +4 -0

+#define this_cpu_cmpxchg_16(pcp, oval, nval)				\
+({									\
+	u128 old__ = __pcpu_cast_128((nval), (nval));			\
+	u128 new__ = __pcpu_cast_128((oval), (oval));			\

spot the bug... please merge the below into this patch.

In order to depricate cmpxchg_double(), replace all its usage with cmpxchg128().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/s390/include/asm/cpu_mf.h  |   29 ++++++++++++-----
 arch/s390/kernel/perf_cpum_sf.c |   65 +++++++++++++++++++++++++---------------
 2 files changed, 63 insertions(+), 31 deletions(-)

So, Alexander Gordeev reported that this code was already prior to your changes potentially broken with respect to missing READ_ONCE() within the cmpxchg_double() loops.

arch/s390/kernel/perf_cpum_sf.c +0 -0

@@ -1294,12 +1306,16 @@ static void hw_perf_event_update(struct perf_event *event, int flush_all)
 		num_sdb++;
 
 		/* Reset trailer (using compare-double-and-swap) */

arch/s390/kernel/perf_cpum_sf.c +3 -0

+		/* READ_ONCE() 16 byte header */
+		prev.val = __cdsg(&te->header.val, 0, 0);
 		do {
+			old.val = prev.val;

arch/s390/kernel/perf_cpum_sf.c +6 -0

+	/* READ_ONCE() 16 byte header */
+	prev.val = __cdsg(&te->header.val, 0, 0);
 	do {
+		old.val = prev.val;
+		new.val = prev.val;
+		*overflow = old.overflow;
+		if (old.f) {
 			/*
 			 * SDB is already set by hardware.
 			 * Abort and try to set somewhere

Quoted 3 lines · Heiko Carstens

So, Alexander Gordeev reported that this code was already prior to your changes potentially broken with respect to missing READ_ONCE() within the cmpxchg_double() loops.

Unless there's an early exit, that shouldn't matter. If you managed to read garbage the cmpxchg itself will simply fail and the loop retries.

Quoted 3 lines · Heiko Carstens

So, Alexander Gordeev reported that this code was already prior to your changes potentially broken with respect to missing READ_ONCE() within the cmpxchg_double() loops.

Unless there's an early exit, that shouldn't matter. If you managed to read garbage the cmpxchg itself will simply fail and the loop retries.

Quoted 4 lines

arch/s390/kernel/perf_cpum_sf.c +0 -0

@@ -1294,12 +1306,16 @@ static void hw_perf_event_update(struct perf_event *event, int flush_all)
 		num_sdb++;
 
 		/* Reset trailer (using compare-double-and-swap) */

arch/s390/kernel/perf_cpum_sf.c static void hw_perf_event_update(struct perf_event *event, int flush_all) · line 1318

Quoted 10 lines

arch/s390/kernel/perf_cpum_sf.c +9 -0

+		/* READ_ONCE() 16 byte header */
+		prev.val = __cdsg(&te->header.val, 0, 0);
 		do {
+			old.val = prev.val;
+			new.val = prev.val;
+			new.f = 0;
+			new.a = 1;
+			new.overflow = 0;
+			prev.val = __cdsg(&te->header.val, old.val, new.val);
+		} while (prev.val != old.val);

So this, and

this case are just silly and expensive. If that initial read is split and manages to read gibberish the cmpxchg will fail and we retry anyway.

Quoted 14 lines

arch/s390/kernel/perf_cpum_sf.c +6 -0

+	/* READ_ONCE() 16 byte header */
+	prev.val = __cdsg(&te->header.val, 0, 0);
 	do {
+		old.val = prev.val;
+		new.val = prev.val;
+		*overflow = old.overflow;
+		if (old.f) {
 			/*
 			 * SDB is already set by hardware.
 			 * Abort and try to set somewhere

@@ -1490,10 +1509,10 @@ static bool aux_set_alert(struct aux_buffer *aux, unsigned long alert_index,
 			 */
 			return false;
 		}

arch/s390/kernel/perf_cpum_sf.c static void hw_perf_event_update(struct perf_event *event, int flush_all) · line 1318

Quoted 4 lines

arch/s390/kernel/perf_cpum_sf.c +4 -0

+		new.a = 1;
+		new.overflow = 0;
+		prev.val = __cdsg(&te->header.val, old.val, new.val);
+	} while (prev.val != old.val);

And while this case has an early exit, it only cares about a single bit (although you made it a full word) and so also shouldn't care. If aux_reset_buffer() returns false, @overflow isn't consumed.

So I really don't see the point of this patch.

Quoted 6 lines

arch/s390/kernel/perf_cpum_sf.c static int aux_output_begin(struct perf_output_handle *handle, · line 1503

Quoted 6 lines

arch/s390/kernel/perf_cpum_sf.c +5 -0

+	/* READ_ONCE() 16 byte header */
+	prev.val = __cdsg(&te->header.val, 0, 0);
 	do {
+		old.val = prev.val;
+		new.val = prev.val;
+		*overflow = old.overflow;

Quoted 8 lines

Quoted 8 lines

arch/s390/kernel/perf_cpum_sf.c +1 -0

+		if (old.f) {
 			/*
 			 * SDB is already set by hardware.
 			 * Abort and try to set somewhere

@@ -1490,10 +1509,10 @@ static bool aux_set_alert(struct aux_buffer *aux, unsigned long alert_index,
 			 */
 			return false;
 		}

Quoted 8 lines

arch/s390/kernel/perf_cpum_sf.c static void hw_perf_event_update(struct perf_event *event, int flush_all) · line 1318

Quoted 4 lines

arch/s390/kernel/perf_cpum_sf.c +4 -0

+		new.a = 1;
+		new.overflow = 0;
+		prev.val = __cdsg(&te->header.val, old.val, new.val);
+	} while (prev.val != old.val);

And while this case has an early exit, it only cares about a single bit (although you made it a full word) and so also shouldn't care. If aux_reset_buffer() returns false, @overflow isn't consumed.

Yes, except that it is anything but obvious that @overflow isn't consumed.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/iommu/amd/amd_iommu_types.h |    9 +++++++--
 drivers/iommu/amd/iommu.c           |   10 ++++------
 2 files changed, 11 insertions(+), 8 deletions(-)

drivers/iommu/amd/amd_iommu_types.h +7 -2

@@ -979,8 +979,13 @@ union irte_ga_hi {
 };
 
 struct irte_ga {
-	union irte_ga_lo lo;
-	union irte_ga_hi hi;
+	union {
+		struct {
+			union irte_ga_lo lo;
+			union irte_ga_hi hi;
+		};
+		u128 irte;
+	};
 };
 
 struct irq_2_irte {

drivers/iommu/amd/iommu.c +2 -2

@@ -2992,10 +2992,10 @@ static int alloc_irq_index(struct amd_io
 static int modify_irte_ga(struct amd_iommu *iommu, u16 devid, int index,
 			  struct irte_ga *irte, struct amd_ir_data *data)
 {
-	bool ret;
 	struct irq_remap_table *table;
-	unsigned long flags;
 	struct irte_ga *entry;
+	unsigned long flags;
+	u128 old;
 
 	table = get_irq_table(iommu, devid);
 	if (!table)

@@ -3006,16 +3006,14 @@ static int modify_irte_ga(struct amd_iom
 	entry = (struct irte_ga *)table->table;

drivers/iommu/amd/iommu.c +2 -1

 	 */
-	WARN_ON(!ret);
+	old = entry->irte;
+	WARN_ON(!try_cmpxchg128(&entry->irte, &old, irte->irte));
 
 	if (data)
 		data->ref = entry;

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/iommu/amd/amd_iommu_types.h |    9 +++++++--
 drivers/iommu/amd/iommu.c           |   10 ++++------
 2 files changed, 11 insertions(+), 8 deletions(-)

drivers/iommu/amd/amd_iommu_types.h +7 -2

@@ -979,8 +979,13 @@ union irte_ga_hi {
 };
 
 struct irte_ga {
-	union irte_ga_lo lo;
-	union irte_ga_hi hi;
+	union {
+		struct {
+			union irte_ga_lo lo;
+			union irte_ga_hi hi;
+		};
+		u128 irte;
+	};
 };
 
 struct irq_2_irte {

drivers/iommu/amd/iommu.c +2 -4

@@ -2992,10 +2992,10 @@ static int alloc_irq_index(struct amd_io
 static int modify_irte_ga(struct amd_iommu *iommu, u16 devid, int index,
 			  struct irte_ga *irte, struct amd_ir_data *data)
 {
-	bool ret;
 	struct irq_remap_table *table;
-	unsigned long flags;
 	struct irte_ga *entry;
+	unsigned long flags;
+	u128 old;
 
 	table = get_irq_table(iommu, devid);
 	if (!table)

@@ -3006,16 +3006,14 @@ static int modify_irte_ga(struct amd_iom
 	entry = (struct irte_ga *)table->table;
 	entry = &entry[index];
 
-	ret = cmpxchg_double(&entry->lo.val, &entry->hi.val,
-			     entry->lo.val, entry->hi.val,

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/slub_def.h |   12 ++-
 mm/slab.h                |   41 +++++++++++--
 mm/slub.c                |  146 ++++++++++++++++++++++++++++-------------------
 3 files changed, 135 insertions(+), 64 deletions(-)

Quoted 7 lines · Peter Zijlstra

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/slub_def.h |   12 ++-
 mm/slab.h                |   41 +++++++++++--
 mm/slub.c                |  146 ++++++++++++++++++++++++++++-------------------
 3 files changed, 135 insertions(+), 64 deletions(-)

Does this actually work? Just wondering since I end up with an instant list corruption on s390. Might be endianness related, but I can't see anything obvious at a first glance.

Quoted 13 lines · Heiko Carstens

Quoted 7 lines · Peter Zijlstra

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/slub_def.h |   12 ++-
 mm/slab.h                |   41 +++++++++++--
 mm/slub.c                |  146 ++++++++++++++++++++++++++++-------------------
 3 files changed, 135 insertions(+), 64 deletions(-)

Does this actually work? Just wondering since I end up with an instant list corruption on s390. Might be endianness related, but I can't see anything obvious at a first glance.

the right thing for a 128-bit value. And I have to admit that all those games with __pcpu_cast_128() make no sense to me. Why isn't it just using "u128" everywhere without any odd _Generic() games?

But I *do* note that this patch seems to be the only one that depends on the new this_cpu_cmpxchg() updates to make it just automatically do the right thing for a 128-bit value. And I have to admit that all those games with __pcpu_cast_128() make no sense to me. Why isn't it just using "u128" everywhere without any odd _Generic() games?

I ran into a ton of casting trouble when compiling kernel/fork.c which uses this_cpu_cmpxchg() on a pointer type and the compiler hates casting pointers to an integer that is not the exact same size.

Quoted 4 lines · Peter Zijlstra

I ran into a ton of casting trouble when compiling kernel/fork.c which uses this_cpu_cmpxchg() on a pointer type and the compiler hates casting pointers to an integer that is not the exact same size.

Ahh. Yeah - not because that code needs or wants the 128-bit case, but because the macro expands to all sizes in a switch statement, so the compiler sees all the cases even if only one is then statically picked.

So the silly casts are for all the cases that never matter.

Annoying.

I wonder if the "this_cpu_cmpxchg()" macro could be made to use _Generic() to pick out the pointer case first, and then only use 'sizeof()' for the integer types, so that we don't have this kind of "every architecture needs to deal with the nasty situation" code.

Ok, it's not actually the this_cpu_cmpxchg() macro, it's __pcpu_size_call_return() and friends, but whatever.

Another alternative is to try to avoid casting to "u64" as long as humanly possible, and use only "typeof((*ptr))" everywhere. Then when the type actually *is* 128-bit, it all works out fine, because it won't be a pointer. That's the approach the uaccess macros tend to take, and then they hit the reverse issue on clang, where using the "byte register" constraints would cause warnings for non-byte accesses, and we had to do

               unsigned char x_u8__;
               __get_user_asm(x_u8__, ptr, "b", "=q", label);
               (x) = x_u8__;

because using '(x)' directly would then warn when 'x' wasn't a char-sized thing - even if that asm case never actually was _used_ for that case, since it was all inside a "switch (sizeof) case 1:" statement.

Linus

Quoted 18 lines

Another alternative is to try to avoid casting to "u64" as long as humanly possible, and use only "typeof((*ptr))" everywhere. Then when the type actually *is* 128-bit, it all works out fine, because it won't be a pointer. That's the approach the uaccess macros tend to take, and then they hit the reverse issue on clang, where using the "byte register" constraints would cause warnings for non-byte accesses, and we had to do

                unsigned char x_u8__;
                __get_user_asm(x_u8__, ptr, "b", "=q", label);
                (x) = x_u8__;

because using '(x)' directly would then warn when 'x' wasn't a char-sized thing - even if that asm case never actually was _used_ for that case, since it was all inside a "switch (sizeof) case 1:" statement.

Quoted 1 line

Linus

I wrote a crazy macro for dealing with exactly this at one point, basically producing the "right type" to cast to. It would need to have 128-bit support added to it, but that should be trivial. It is called something like int_type() ... not in front of a computer right now so can't double check.

Quoted 4 lines · Peter Zijlstra

I ran into a ton of casting trouble when compiling kernel/fork.c which uses this_cpu_cmpxchg() on a pointer type and the compiler hates casting pointers to an integer that is not the exact same size.

Ahh. Yeah - not because that code needs or wants the 128-bit case, but because the macro expands to all sizes in a switch statement, so the compiler sees all the cases even if only one is then statically picked.

So the silly casts are for all the cases that never matter.

Annoying.

Hi,

Since Linus hated on cmpxchg_double(), a few patches to get rid of it, as proposed here:

https://lkml.kernel.org/r/[email protected]

based on tip/master because Linus' tree is moving a wee bit fast at the moment.

0day robot is all green for building, very limited testing on arm64/s390 for obvious raisins -- I tried to get the asm right, but please, double check.