Re: [PATCH 2/7] riscv: Implement cmpxchg8/16() using Zabha
From: Nathan Chancellor
Date: Tue May 28 2024 - 15:31:20 EST
Hi Alexandre,
On Tue, May 28, 2024 at 05:10:47PM +0200, Alexandre Ghiti wrote:
> This adds runtime support for Zabha in cmpxchg8/16 operations.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx>
> ---
> arch/riscv/Kconfig | 16 ++++++++++++++++
> arch/riscv/Makefile | 10 ++++++++++
> arch/riscv/include/asm/cmpxchg.h | 26 ++++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b443def70139..05597719bb1c 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -579,6 +579,22 @@ config RISCV_ISA_V_PREEMPTIVE
> preemption. Enabling this config will result in higher memory
> consumption due to the allocation of per-task's kernel Vector context.
>
> +config TOOLCHAIN_HAS_ZABHA
> + bool
> + default y
> + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zabha)
> + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zabha)
This test does not take into account the need for
'-menable-experimental-extensions' and '1p0' in the '-march=' value with
clang 19, so it can never be enabled even if it is available.
I am not really sure how to succinctly account for this though, other
than duplicating and modifying the cc-option checks with a dependency on
either CC_IS_GCC or CC_IS_CLANG. Another option is taking the same
approach as the _SUPPORTS_DYNAMIC_FTRACE symbols and introduce
CLANG_HAS_ZABHA and GCC_HAS_ZABHA? That might not make it too ugly.
I think the ZACAS patch has a similar issue, it just isn't noticeable
with clang 19 but it should be with clang 17 and 18.
> + depends on AS_HAS_OPTION_ARCH
> +
> +config RISCV_ISA_ZABHA
> + bool "Zabha extension support for atomic byte/half-word operations"
> + depends on TOOLCHAIN_HAS_ZABHA
> + default y
> + help
> + Adds support to use atomic byte/half-word operations in the kernel.
> +
> + If you don't know what to do here, say Y.
> +
> config TOOLCHAIN_HAS_ZACAS
> bool
> default y
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index d5b60b87998c..f58ac921dece 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -89,6 +89,16 @@ else
> riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas
> endif
>
> +# Check if the toolchain supports Zabha
> +ifdef CONFIG_AS_IS_LLVM
> +# Support for experimental Zabha was merged in LLVM 19.
> +KBUILD_CFLAGS += -menable-experimental-extensions
> +KBUILD_AFLAGS += -menable-experimental-extensions
> +riscv-march-y := $(riscv-march-y)_zabha1p0
This block should have some dependency on CONFIG_TOOLCHAIN_HAS_ZABHA as
well right? Otherwise, the build breaks with LLVM toolchains that do not
support zabha, like LLVM 18.1.x:
clang: error: invalid arch name 'rv64imac_zihintpause_zacas1p0_zabha1p0', unsupported version number 1.0 for extension 'zabha'
I think the zacas patch has the same bug.
I think that it would be good to consolidate the adding of
'-menable-experimental-extensions' to the compiler and assembler flags
to perhaps having a hidden symbol like CONFIG_EXPERIMENTAL_EXTENSIONS
that is selected by any extension that is experimental for the
particular toolchain version.
config EXPERIMENTAL_EXTENSIONS
bool
config TOOLCHAIN_HAS_ZABHA
def_bool y
select EXPERIMENTAL_EXETNSIONS if CC_IS_CLANG
...
config TOOLCHAIN_HAS_ZACAS
def_bool_y
# ZACAS was experimental until Clang 19: https://github.com/llvm/llvm-project/commit/95aab69c109adf29e183090c25dc95c773215746
select EXPERIMENTAL_EXETNSIONS if CC_IS_CLANG && CLANG_VERSION < 190000
...
Then in the Makefile:
ifdef CONFIG_EXPERIMENTAL_EXTENSIONS
KBUILD_AFLAGS += -menable-experimental-extensions
KBUILD_CFLAGS += -menable-experimental-extensions
endif
> +else
> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZABHA) := $(riscv-march-y)_zabha
> +endif
> +
> # Remove F,D,V from isa string for all. Keep extensions between "fd" and "v" by
> # matching non-v and non-multi-letter extensions out with the filter ([^v_]*)
> KBUILD_CFLAGS += -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 1c50b4821ac8..65de9771078e 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -103,8 +103,14 @@
> * indicated by comparing RETURN with OLD.
> */
>
> -#define __arch_cmpxchg_masked(sc_sfx, prepend, append, r, p, o, n) \
> +#define __arch_cmpxchg_masked(sc_sfx, cas_sfx, prepend, append, r, p, o, n) \
> ({ \
> + __label__ zabha, end; \
> + \
> + asm goto(ALTERNATIVE("nop", "j %[zabha]", 0, \
> + RISCV_ISA_EXT_ZABHA, 1) \
> + : : : : zabha); \
> + \
> u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3); \
> ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE; \
> ulong __mask = GENMASK(((sizeof(*p)) * BITS_PER_BYTE) - 1, 0) \
> @@ -131,6 +137,17 @@
> : "memory"); \
> \
> r = (__typeof__(*(p)))((__retx & __mask) >> __s); \
> + goto end; \
> + \
> +zabha: \
> + __asm__ __volatile__ ( \
> + prepend \
> + " amocas" cas_sfx " %0, %z2, %1\n" \
This should probably have some dependency on CONFIG_RISCV_ISA_ZABHA? I get the
following with GCC 13.2.0:
include/linux/atomic/atomic-arch-fallback.h: Assembler messages:
include/linux/atomic/atomic-arch-fallback.h:2108: Error: unrecognized opcode `amocas.w a4,a3,0(s1)'
> + append \
> + : "+&r" (r), "+A" (*(p)) \
> + : "rJ" (n) \
> + : "memory"); \
> +end: \
I get a lot of warnings from this statement and the one added by the
previous patch for zacas, which is a C23 extension:
include/linux/atomic/atomic-arch-fallback.h:4234:9: warning: label at end of compound statement is a C23 extension [-Wc23-extensions]
include/linux/atomic/atomic-arch-fallback.h:89:29: note: expanded from macro 'raw_cmpxchg_relaxed'
89 | #define raw_cmpxchg_relaxed arch_cmpxchg_relaxed
| ^
arch/riscv/include/asm/cmpxchg.h:219:2: note: expanded from macro 'arch_cmpxchg_relaxed'
219 | _arch_cmpxchg((ptr), (o), (n), "", "", "")
| ^
arch/riscv/include/asm/cmpxchg.h:200:3: note: expanded from macro '_arch_cmpxchg'
200 | __arch_cmpxchg_masked(sc_sfx, ".h" sc_sfx, \
| ^
arch/riscv/include/asm/cmpxchg.h:150:14: note: expanded from macro '__arch_cmpxchg_masked'
150 | end: \
| ^
This resolves it:
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index ba3ffc2fcdd0..57aa4a554278 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -147,7 +147,7 @@ zabha: \
: "+&r" (r), "+A" (*(p)) \
: "rJ" (n) \
: "memory"); \
-end: \
+end:; \
})
#define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n) \
@@ -180,7 +180,7 @@ zacas: \
: "+&r" (r), "+A" (*(p)) \
: "rJ" (n) \
: "memory"); \
-end: \
+end:; \
})
#define _arch_cmpxchg(ptr, old, new, sc_sfx, prepend, append) \
> })
>
> #define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n) \
> @@ -175,8 +192,13 @@ end: \
> \
> switch (sizeof(*__ptr)) { \
> case 1: \
> + __arch_cmpxchg_masked(sc_sfx, ".b" sc_sfx, \
> + prepend, append, \
> + __ret, __ptr, __old, __new); \
> + break; \
> case 2: \
> - __arch_cmpxchg_masked(sc_sfx, prepend, append, \
> + __arch_cmpxchg_masked(sc_sfx, ".h" sc_sfx, \
> + prepend, append, \
> __ret, __ptr, __old, __new); \
> break; \
> case 4: \
> --
> 2.39.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv