Re: [PATCH 1/7] riscv: Implement cmpxchg32/64() using Zacas

[Alexandre Ghiti]

Hi Conor,

On Tue, May 28, 2024 at 5:34 PM Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >  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.