| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 May 2024 14:49:58 +0200
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>, Arnd Bergmann <arnd@...db.de>,
Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-arch@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH 2/7] riscv: Implement cmpxchg8/16() using Zabha
Hi Nathan,
On Tue, May 28, 2024 at 9:31 PM Nathan Chancellor <nathan@...nel.org> 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@...osinc.com>
> > ---
> > 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@rivosinc.com/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@rivosinc.com/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@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Thanks for your thorough review!
Alex
Powered by blists - more mailing lists