diff mbox series

[2/7] riscv: Implement cmpxchg8/16() using Zabha

Message ID 20240528151052.313031-3-alexghiti@rivosinc.com (mailing list archive)
State Superseded
Headers show
Series Zacas/Zabha support and qspinlocks | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Alexandre Ghiti May 28, 2024, 3:10 p.m. UTC
This adds runtime support for Zabha in cmpxchg8/16 operations.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 arch/riscv/Kconfig               | 16 ++++++++++++++++
 arch/riscv/Makefile              | 10 ++++++++++
 arch/riscv/include/asm/cmpxchg.h | 26 ++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 2 deletions(-)

Comments

Nathan Chancellor May 28, 2024, 7:31 p.m. UTC | #1
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@rivosinc.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.

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Andrea Parri May 28, 2024, 11:54 p.m. UTC | #2
> +zabha:									\
> +	__asm__ __volatile__ (						\
> +		prepend							\
> +		"	amocas" cas_sfx " %0, %z2, %1\n"		\
> +		append							\
> +		: "+&r" (r), "+A" (*(p))				\
> +		: "rJ" (n)						\
> +		: "memory");						\

Couldn't a platform have Zabha but not have Zacas?  I don't see how this
asm goto could work in such case, what am I missing?

  Andrea
Alexandre Ghiti May 29, 2024, 12:29 p.m. UTC | #3
On Wed, May 29, 2024 at 1:54 AM Andrea Parri <parri.andrea@gmail.com> wrote:
>
> > +zabha:                                                                       \
> > +     __asm__ __volatile__ (                                          \
> > +             prepend                                                 \
> > +             "       amocas" cas_sfx " %0, %z2, %1\n"                \
> > +             append                                                  \
> > +             : "+&r" (r), "+A" (*(p))                                \
> > +             : "rJ" (n)                                              \
> > +             : "memory");                                            \
>
> Couldn't a platform have Zabha but not have Zacas?  I don't see how this
> asm goto could work in such case, what am I missing?

Zabha amocas.[b|h] instructions are only implemented if Zacas is
present, as the specification states: "If Zacas [2] extension is also
implemented, Zabha further provides the AMOCAS.[B|H] instructions."

But the code you mention is only for 8 and 16bit operations, so I
think we are good anyway?

Thanks,

Alex

>
>   Andrea
Alexandre Ghiti May 29, 2024, 12:49 p.m. UTC | #4
Hi Nathan,

On Tue, May 28, 2024 at 9:31 PM Nathan Chancellor <nathan@kernel.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@rivosinc.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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks for your thorough review!

Alex
Alexandre Ghiti May 29, 2024, 12:55 p.m. UTC | #5
On Wed, May 29, 2024 at 2:29 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> On Wed, May 29, 2024 at 1:54 AM Andrea Parri <parri.andrea@gmail.com> wrote:
> >
> > > +zabha:                                                                       \
> > > +     __asm__ __volatile__ (                                          \
> > > +             prepend                                                 \
> > > +             "       amocas" cas_sfx " %0, %z2, %1\n"                \
> > > +             append                                                  \
> > > +             : "+&r" (r), "+A" (*(p))                                \
> > > +             : "rJ" (n)                                              \
> > > +             : "memory");                                            \
> >
> > Couldn't a platform have Zabha but not have Zacas?  I don't see how this
> > asm goto could work in such case, what am I missing?
>
> Zabha amocas.[b|h] instructions are only implemented if Zacas is
> present, as the specification states: "If Zacas [2] extension is also
> implemented, Zabha further provides the AMOCAS.[B|H] instructions."
>
> But the code you mention is only for 8 and 16bit operations, so I
> think we are good anyway?

And I was wrong like Andrea noted privately. So I'll fix that too, thanks!

>
> Thanks,
>
> Alex
>
> >
> >   Andrea
Nathan Chancellor May 29, 2024, 3:57 p.m. UTC | #6
On Wed, May 29, 2024 at 02:49:58PM +0200, Alexandre Ghiti wrote:
> Then I missed that, I should have checked the generated code. Is the
> extension version "1p0" in '-march=' only required for experimental
> extensions?

I think so, if my understanding of the message is correct.

> 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

Yeah, I tend to agree with Conor on that front. I had not noticed that
part of the message when I was looking at other parts of this thread. I
could see an argument for allowing experimental extensions for
qualification purposes but I think it does create a bit of a support
nightmare, especially when there are breaking changes across revisions.

> > 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

Perhaps with that in mind, maybe EXPERIMENTAL_EXTENSIONS (or whatever)
should be a user selectable option and the TOOLCHAIN values depend on it
when the user has a clang version that does not support the ratified
version.

> 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

FWIW, I think your plan of removing support for the experimental version
of the extension and pushing to remove the experimental status in LLVM
(especially since it seems like it is ratified like zacas?
https://jira.riscv.org/browse/RVS-1685) is probably the best thing going
forward. If the LLVM folks are made aware soon, it should be easy to get
that change into clang-19, which is branching at the end of July I
believe.

> Thanks for your thorough review!

Thanks for taking LLVM support into consideration :)

Cheers,
Nathan
Alexandre Ghiti June 3, 2024, 3:31 p.m. UTC | #7
Hi Conor, Nathan,

On 29/05/2024 17:57, Nathan Chancellor wrote:
> On Wed, May 29, 2024 at 02:49:58PM +0200, Alexandre Ghiti wrote:
>> Then I missed that, I should have checked the generated code. Is the
>> extension version "1p0" in '-march=' only required for experimental
>> extensions?
> I think so, if my understanding of the message is correct.
>
>> 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
> Yeah, I tend to agree with Conor on that front. I had not noticed that
> part of the message when I was looking at other parts of this thread. I
> could see an argument for allowing experimental extensions for
> qualification purposes but I think it does create a bit of a support
> nightmare, especially when there are breaking changes across revisions.
>
>>> 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
> Perhaps with that in mind, maybe EXPERIMENTAL_EXTENSIONS (or whatever)
> should be a user selectable option and the TOOLCHAIN values depend on it
> when the user has a clang version that does not support the ratified
> version.
>
>> 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
> FWIW, I think your plan of removing support for the experimental version
> of the extension and pushing to remove the experimental status in LLVM
> (especially since it seems like it is ratified like zacas?
> https://jira.riscv.org/browse/RVS-1685) is probably the best thing going
> forward. If the LLVM folks are made aware soon, it should be easy to get
> that change into clang-19, which is branching at the end of July I
> believe.


FYI, it was just merged https://github.com/llvm/llvm-project/pull/93831

Thanks again,

Alex


>
>> Thanks for your thorough review!
> Thanks for taking LLVM support into consideration :)
>
> Cheers,
> Nathan
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

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)
+	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
+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"		\
+		append							\
+		: "+&r" (r), "+A" (*(p))				\
+		: "rJ" (n)						\
+		: "memory");						\
+end:									\
 })
 
 #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:								\