Hi Nathan, On Tue, May 28, 2024 at 9:31 PM Nathan Chancellor <nathan@xxxxxxxxxx> wrote: > > 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. Then I missed that, I should have checked the generated code. Is the extension version "1p0" in '-march=' only required for experimental extensions? > > 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. But from Conor comment here [1], we should not enable extensions that are only experimental. In that case, we should be good with this. [1] https://lore.kernel.org/linux-riscv/20240528151052.313031-1-alexghiti@xxxxxxxxxxxx/T/#mefb283477bce852f3713cbbb4ff002252281c9d5 > > > + 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. Ok, I will fix that, thanks. > > 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 That's a good idea to me, let's see what Conor thinks [2] [2] https://lore.kernel.org/linux-riscv/20240528151052.313031-1-alexghiti@xxxxxxxxxxxx/T/#m1d798dfc4c27e5b6d9e14117d81b577ace123322 > > > +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)' Indeed, my test setup lacks a few things apparently, I will fix that, thanks. > > > + 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) \ Weird, I missed this too, I will fix that, thanks. > > > }) > > > > #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 Thanks for your thorough review! Alex
