| Message ID | 20240528151052.313031-2-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Zacas/Zabha support and qspinlocks | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Tue, May 28, 2024 at 05:10:46PM +0200, Alexandre Ghiti wrote: > This adds runtime support for Zacas in cmpxchg operations. > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > arch/riscv/Kconfig | 17 +++++++++++++++++ > arch/riscv/Makefile | 11 +++++++++++ > arch/riscv/include/asm/cmpxchg.h | 23 ++++++++++++++++++++--- > 3 files changed, 48 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 8a0f403432e8..b443def70139 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -579,6 +579,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_ZACAS > + bool > + default y > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zacas) > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zacas) > + depends on AS_HAS_OPTION_ARCH > + > +config RISCV_ISA_ZACAS > + bool "Zacas extension support for atomic CAS" > + depends on TOOLCHAIN_HAS_ZACAS > + default y > + help > + Adds support to use atomic CAS instead of LR/SC to implement kernel > + atomic cmpxchg operation. If you were a person compiling a kernel, would you be able to read this and realise that this is safe to enable when their system does not support atomic CAS? Please take a look at other how other extensions handle this, or the patch that I have been sending that tries to make things clearer: https://patchwork.kernel.org/project/linux-riscv/patch/20240528-varnish-status-9c22973093a0@spud/ > + > + If you don't know what to do here, say Y. > + > config TOOLCHAIN_HAS_ZBB > bool > default y > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile > index 5b3115a19852..d5b60b87998c 100644 > --- a/arch/riscv/Makefile > +++ b/arch/riscv/Makefile > @@ -78,6 +78,17 @@ endif > # Check if the toolchain supports Zihintpause extension > riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause > > +# Check if the toolchain supports Zacas > +ifdef CONFIG_AS_IS_LLVM > +# Support for experimental Zacas was merged in LLVM 17, but the removal of > +# the "experimental" was merged in LLVM 19. > +KBUILD_CFLAGS += -menable-experimental-extensions > +KBUILD_AFLAGS += -menable-experimental-extensions > +riscv-march-y := $(riscv-march-y)_zacas1p0 > +else > +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas > +endif I'm almost certain that we discussed this before for vector and it was decided to not enable experimental extensions (particularly as it is a global option), and instead require the non-experimental versions. This isn't even consistent with your TOOLCHAIN_HAS_ZACAS checks, that will only enable the option for the ratified version. I think we should continue to avoid enabling experimental extensions, even if that imposes a requirement of having a bleeding edge toolchain to actually use the extension. Thanks, Conor.
> + asm goto(ALTERNATIVE("nop", "j %[zacas]", 0, \ > + RISCV_ISA_EXT_ZACAS, 1) \ > + : : : : zacas); \ > + \ > __asm__ __volatile__ ( \ > prepend \ > "0: lr" lr_sfx " %0, %2\n" \ > " bne %0, %z3, 1f\n" \ > - " sc" sc_sfx " %1, %z4, %2\n" \ > + " sc" sc_cas_sfx " %1, %z4, %2\n" \ > " bnez %1, 0b\n" \ > append \ > "1:\n" \ > : "=&r" (r), "=&r" (__rc), "+A" (*(p)) \ > : "rJ" (co o), "rJ" (n) \ > : "memory"); \ > + goto end; \ > + \ > +zacas: \ > + __asm__ __volatile__ ( \ > + prepend \ > + " amocas" sc_cas_sfx " %0, %z2, %1\n" \ > + append \ > + : "+&r" (r), "+A" (*(p)) \ > + : "rJ" (n) \ > + : "memory"); \ With this, a cmpxchg32() will result in something like amocas.w.rl a5,a4,(s1) fence rw,rw (cf. my remarks in patch #4); this will/should provide enough sync, but you might want to try the alternative and currently more common mapping for "fully-ordered AMO sequences", aka amocas.w.aqrl a5,a4,(s1) Similarly for cmpxchg64 and other sizes. Andrea
Hi Conor, On Tue, May 28, 2024 at 5:34 PM Conor Dooley <conor@kernel.org> wrote: > > On Tue, May 28, 2024 at 05:10:46PM +0200, Alexandre Ghiti wrote: > > This adds runtime support for Zacas in cmpxchg operations. > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > --- > > arch/riscv/Kconfig | 17 +++++++++++++++++ > > arch/riscv/Makefile | 11 +++++++++++ > > arch/riscv/include/asm/cmpxchg.h | 23 ++++++++++++++++++++--- > > 3 files changed, 48 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index 8a0f403432e8..b443def70139 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -579,6 +579,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_ZACAS > > + bool > > + default y > > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zacas) > > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zacas) > > + depends on AS_HAS_OPTION_ARCH > > + > > +config RISCV_ISA_ZACAS > > + bool "Zacas extension support for atomic CAS" > > + depends on TOOLCHAIN_HAS_ZACAS > > + default y > > + help > > + Adds support to use atomic CAS instead of LR/SC to implement kernel > > + atomic cmpxchg operation. > > If you were a person compiling a kernel, would you be able to read this > and realise that this is safe to enable when their system does not > support atomic CAS? Please take a look at other how other extensions > handle this, or the patch that I have been sending that tries to make > things clearer: > https://patchwork.kernel.org/project/linux-riscv/patch/20240528-varnish-status-9c22973093a0@spud/ Ok, I will go for: "Enable the use of the Zacas ISA-extension to implement atomic cmpxchg operations when it is detected at boot." And I will do the same for Zabha. > > > + > > + If you don't know what to do here, say Y. > > + > > config TOOLCHAIN_HAS_ZBB > > bool > > default y > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile > > index 5b3115a19852..d5b60b87998c 100644 > > --- a/arch/riscv/Makefile > > +++ b/arch/riscv/Makefile > > @@ -78,6 +78,17 @@ endif > > # Check if the toolchain supports Zihintpause extension > > riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause > > > > +# Check if the toolchain supports Zacas > > +ifdef CONFIG_AS_IS_LLVM > > +# Support for experimental Zacas was merged in LLVM 17, but the removal of > > +# the "experimental" was merged in LLVM 19. > > +KBUILD_CFLAGS += -menable-experimental-extensions > > +KBUILD_AFLAGS += -menable-experimental-extensions > > +riscv-march-y := $(riscv-march-y)_zacas1p0 > > +else > > +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas > > +endif > > I'm almost certain that we discussed this before for vector and it was > decided to not enable experimental extensions (particularly as it is a > global option), and instead require the non-experimental versions. > This isn't even consistent with your TOOLCHAIN_HAS_ZACAS checks, that > will only enable the option for the ratified version. Zacas was ratified, hence the removal of "experimental" in LLVM 19. But unfortunately Zabha lacks such changes in LLVM, so that will make this inconsistent (ratified extension but still experimental). I'll remove the enablement of the experimental extensions then so that will fail for LLVM < 19. And for Zabha, I'll try to push the removal of experimental from LLVM. > I think we should > continue to avoid enabling experimental extensions, even if that imposes > a requirement of having a bleeding edge toolchain to actually use the > extension. Would it make sense to have a CONFIG_RISCV_LLVM_ENABLE_EXPERIMENTAL_EXTENSIONS or similar? So that people who want to play with those extensions will still be able to do so without patching the kernel? Thanks, Alex > > Thanks, > Conor.
On Wed, May 29, 2024 at 02:20:33PM +0200, Alexandre Ghiti wrote: > On Tue, May 28, 2024 at 5:34 PM Conor Dooley <conor@kernel.org> wrote: > > On Tue, May 28, 2024 at 05:10:46PM +0200, Alexandre Ghiti wrote: > > > config TOOLCHAIN_HAS_ZBB > > > bool > > > default y > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile > > > index 5b3115a19852..d5b60b87998c 100644 > > > --- a/arch/riscv/Makefile > > > +++ b/arch/riscv/Makefile > > > @@ -78,6 +78,17 @@ endif > > > # Check if the toolchain supports Zihintpause extension > > > riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause > > > > > > +# Check if the toolchain supports Zacas > > > +ifdef CONFIG_AS_IS_LLVM > > > +# Support for experimental Zacas was merged in LLVM 17, but the removal of > > > +# the "experimental" was merged in LLVM 19. > > > +KBUILD_CFLAGS += -menable-experimental-extensions > > > +KBUILD_AFLAGS += -menable-experimental-extensions > > > +riscv-march-y := $(riscv-march-y)_zacas1p0 > > > +else > > > +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas > > > +endif > > > > I'm almost certain that we discussed this before for vector and it was > > decided to not enable experimental extensions (particularly as it is a > > global option), and instead require the non-experimental versions. > > This isn't even consistent with your TOOLCHAIN_HAS_ZACAS checks, that > > will only enable the option for the ratified version. > > Zacas was ratified, hence the removal of "experimental" in LLVM 19. > But unfortunately Zabha lacks such changes in LLVM, so that will make > this inconsistent (ratified extension but still experimental). > > I'll remove the enablement of the experimental extensions then so that > will fail for LLVM < 19. And for Zabha, I'll try to push the removal > of experimental from LLVM. Ye, as Nathan mentioned there may yet be some time. It'd be great if you could. > > I think we should > > continue to avoid enabling experimental extensions, even if that imposes > > a requirement of having a bleeding edge toolchain to actually use the > > extension. > > Would it make sense to have a > CONFIG_RISCV_LLVM_ENABLE_EXPERIMENTAL_EXTENSIONS or similar? So that > people who want to play with those extensions will still be able to do > so without patching the kernel? Maybe, but I think something like that should depend on BROKEN and only be done when the extension hasn't had its experimental status removed by a released version of LLVM and is not supported by a release of GCC. Given we only allow frozen extensions into the kernel, actually having to do this would be rather rare. I think we should also reserve the right to drop support for the experimental version as soon as it does get its status changed, and depending on BROKEN would let us do that without any regressions in terms of toolchain version support. Yes, making the option depend on BROKEN would require patching a Kconfig file but this would be a facility for kernel developers to test prior to the release of a toolchain that actually supports the extension, and the "hard" part in the Makefile to hook it up would already be done. I think if you're capable of messing about with experimental extensions in the kernel you're capable of also modifying a Kconfig file locally ;) Cheers, Conor.
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 8a0f403432e8..b443def70139 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -579,6 +579,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_ZACAS + bool + default y + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zacas) + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zacas) + depends on AS_HAS_OPTION_ARCH + +config RISCV_ISA_ZACAS + bool "Zacas extension support for atomic CAS" + depends on TOOLCHAIN_HAS_ZACAS + default y + help + Adds support to use atomic CAS instead of LR/SC to implement kernel + atomic cmpxchg operation. + + If you don't know what to do here, say Y. + config TOOLCHAIN_HAS_ZBB bool default y diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile index 5b3115a19852..d5b60b87998c 100644 --- a/arch/riscv/Makefile +++ b/arch/riscv/Makefile @@ -78,6 +78,17 @@ endif # Check if the toolchain supports Zihintpause extension riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause +# Check if the toolchain supports Zacas +ifdef CONFIG_AS_IS_LLVM +# Support for experimental Zacas was merged in LLVM 17, but the removal of +# the "experimental" was merged in LLVM 19. +KBUILD_CFLAGS += -menable-experimental-extensions +KBUILD_AFLAGS += -menable-experimental-extensions +riscv-march-y := $(riscv-march-y)_zacas1p0 +else +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas +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 4d23f0c35b94..1c50b4821ac8 100644 --- a/arch/riscv/include/asm/cmpxchg.h +++ b/arch/riscv/include/asm/cmpxchg.h @@ -9,6 +9,7 @@ #include <linux/bug.h> #include <asm/fence.h> +#include <asm/alternative.h> #define __arch_xchg_masked(prepend, append, r, p, n) \ ({ \ @@ -132,21 +133,37 @@ r = (__typeof__(*(p)))((__retx & __mask) >> __s); \ }) -#define __arch_cmpxchg(lr_sfx, sc_sfx, prepend, append, r, p, co, o, n) \ +#define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n) \ ({ \ + __label__ zacas, end; \ register unsigned int __rc; \ \ + asm goto(ALTERNATIVE("nop", "j %[zacas]", 0, \ + RISCV_ISA_EXT_ZACAS, 1) \ + : : : : zacas); \ + \ __asm__ __volatile__ ( \ prepend \ "0: lr" lr_sfx " %0, %2\n" \ " bne %0, %z3, 1f\n" \ - " sc" sc_sfx " %1, %z4, %2\n" \ + " sc" sc_cas_sfx " %1, %z4, %2\n" \ " bnez %1, 0b\n" \ append \ "1:\n" \ : "=&r" (r), "=&r" (__rc), "+A" (*(p)) \ : "rJ" (co o), "rJ" (n) \ : "memory"); \ + goto end; \ + \ +zacas: \ + __asm__ __volatile__ ( \ + prepend \ + " amocas" sc_cas_sfx " %0, %z2, %1\n" \ + append \ + : "+&r" (r), "+A" (*(p)) \ + : "rJ" (n) \ + : "memory"); \ +end: \ }) #define _arch_cmpxchg(ptr, old, new, sc_sfx, prepend, append) \ @@ -154,7 +171,7 @@ __typeof__(ptr) __ptr = (ptr); \ __typeof__(*(__ptr)) __old = (old); \ __typeof__(*(__ptr)) __new = (new); \ - __typeof__(*(__ptr)) __ret; \ + __typeof__(*(__ptr)) __ret = (old); \ \ switch (sizeof(*__ptr)) { \ case 1: \
This adds runtime support for Zacas in cmpxchg operations. Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- arch/riscv/Kconfig | 17 +++++++++++++++++ arch/riscv/Makefile | 11 +++++++++++ arch/riscv/include/asm/cmpxchg.h | 23 ++++++++++++++++++++--- 3 files changed, 48 insertions(+), 3 deletions(-)