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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]