Re: [PATCH v3 03/11] riscv: Implement cmpxchg8/16() using Zabha

Alexandre Ghiti
2024-07-18T12:50:40Z

Hi Drew,

On 17/07/2024 17:26, Andrew Jones wrote:
> On Wed, Jul 17, 2024 at 08:19:49AM GMT, Alexandre Ghiti wrote:
>> This adds runtime support for Zabha in cmpxchg8/16() operations.
>>
>> Note that in the absence of Zacas support in the toolchain, CAS
>> instructions from Zabha won't be used.
>>
>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>> ---
>> arch/riscv/Kconfig | 17 ++++++++++++++++
>> arch/riscv/Makefile | 3 +++
>> arch/riscv/include/asm/cmpxchg.h | 33 ++++++++++++++++++++++++++++++--
>> arch/riscv/include/asm/hwcap.h | 1 +
>> arch/riscv/kernel/cpufeature.c | 1 +
>> 5 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 1caaedec88c7..d3b0f92f92da 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -596,6 +596,23 @@ 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/halfword operations"
>> + depends on TOOLCHAIN_HAS_ZABHA
>> + default y
>> + help
>> + Enable the use of the Zabha ISA-extension to implement kernel
>> + byte/halfword atomic memory operations when it is detected at boot.
>> +
>> + 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 9fd13d7a9cc6..78dcaaeebf4e 100644
>> --- a/arch/riscv/Makefile
>> +++ b/arch/riscv/Makefile
>> @@ -88,6 +88,9 @@ riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
>> # Check if the toolchain supports Zacas
>> riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas
>>
>> +# Check if the toolchain supports Zabha
>> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZABHA) := $(riscv-march-y)_zabha
>> +
>> # 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 5d38153e2f13..c86722a101d0 100644
>> --- a/arch/riscv/include/asm/cmpxchg.h
>> +++ b/arch/riscv/include/asm/cmpxchg.h
>> @@ -105,8 +105,30 @@
>> * 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__ no_zabha_zacas, end; \
>> + \
>> + if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) && \
>> + IS_ENABLED(CONFIG_RISCV_ISA_ZACAS)) { \
>> + asm goto(ALTERNATIVE("j %[no_zabha_zacas]", "nop", 0, \
>> + RISCV_ISA_EXT_ZABHA, 1) \
>> + : : : : no_zabha_zacas); \
>> + asm goto(ALTERNATIVE("j %[no_zabha_zacas]", "nop", 0, \
>> + RISCV_ISA_EXT_ZACAS, 1) \
>> + : : : : no_zabha_zacas); \
> I came late to the call, but I guess trying to get rid of these asm gotos
> was the topic of the discussion. The proposal was to try and use static
> branches, but keep in mind that we've had trouble with static branches
> inside macros in the past when those macros are used in many places[1]
>
> [1] commit 0b1d60d6dd9e ("riscv: Fix build with CONFIG_CC_OPTIMIZE_FOR_SIZE=y")

Thanks for the pointer, I was not aware of this. I came up with a
solution with preprocessor guards, not the prettiest solution but at
least it does not create more problems :)

>
>> + \
>> + __asm__ __volatile__ ( \
>> + prepend \
>> + " amocas" cas_sfx " %0, %z2, %1\n" \
>> + append \
>> + : "+&r" (r), "+A" (*(p)) \
>> + : "rJ" (n) \
>> + : "memory"); \
>> + goto end; \
>> + } \
>> + \
>> +no_zabha_zacas:; \
> unnecessary ;

Actually it is, it fixes a warning encountered on llvm:
https://lore.kernel.org/linux-riscv/20240528193110.GA2196855@thelio-3990X/

Thanks,

Alex

>
>> 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) \
>> @@ -133,6 +155,8 @@
>> : "memory"); \
>> \
>> r = (__typeof__(*(p)))((__retx & __mask) >> __s); \
>> + \
>> +end:; \
>> })
>>
>> #define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n) \
>> @@ -180,8 +204,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: \
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index e17d0078a651..f71ddd2ca163 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -81,6 +81,7 @@
>> #define RISCV_ISA_EXT_ZTSO 72
>> #define RISCV_ISA_EXT_ZACAS 73
>> #define RISCV_ISA_EXT_XANDESPMU 74
>> +#define RISCV_ISA_EXT_ZABHA 75
>>
>> #define RISCV_ISA_EXT_XLINUXENVCFG 127
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 5ef48cb20ee1..c125d82c894b 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -257,6 +257,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>> __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>> __RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
>> __RISCV_ISA_EXT_DATA(zacas, RISCV_ISA_EXT_ZACAS),
>> + __RISCV_ISA_EXT_DATA(zabha, RISCV_ISA_EXT_ZABHA),
>> __RISCV_ISA_EXT_DATA(zfa, RISCV_ISA_EXT_ZFA),
>> __RISCV_ISA_EXT_DATA(zfh, RISCV_ISA_EXT_ZFH),
>> __RISCV_ISA_EXT_DATA(zfhmin, RISCV_ISA_EXT_ZFHMIN),
>> --
>> 2.39.2
>>
> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv