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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 May 2024 14:20:33 +0200
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: Conor Dooley <conor@...nel.org>
Cc: Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>, 
	Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>, Arnd Bergmann <arnd@...db.de>, 
	Leonardo Bras <leobras@...hat.com>, Guo Ren <guoren@...nel.org>, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-arch@...r.kernel.org
Subject: Re: [PATCH 1/7] riscv: Implement cmpxchg32/64() using Zacas

Hi Conor,

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