Message-ID: <20250905-prolonged-chip-73be9d74ddb5@spud>
Date: Fri, 5 Sep 2025 16:24:39 +0100
From: Conor Dooley <conor@...nel.org>
To: Asuna <spriteovo@...il.com>
Cc: Jason Montleon <jmontleo@...hat.com>, Han Gao <rabenda.cn@...il.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>,
	Jonathan Corbet <corbet@....net>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, Tejun Heo <tj@...nel.org>,
	Kees Cook <kees@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
	Matthew Maurer <mmaurer@...gle.com>, Jeff Xu <jeffxu@...omium.org>,
	Shakeel Butt <shakeel.butt@...ux.dev>,
	Jan Hendrik Farr <kernel@...rr.cc>,
	Michal Koutný <mkoutny@...e.com>,
	Christian Brauner <brauner@...nel.org>,
	Brian Gerst <brgerst@...il.com>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-kbuild@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH 2/2] RISC-V: re-enable gcc + rust builds

Yo,

On Fri, Sep 05, 2025 at 06:56:35AM +0800, Asuna wrote:
> > One thing - please don't send new versions
> > of patchsets in response to earlier versions or other threads. It
> > doesn't do you any favours with mailbox visibility.
> 
> I apologize for this, I'm pretty much new to mailing lists, so I had
> followed the step "Explicit In-Reply-To headers" [1] in doc. For future
> patches I'll send them alone instead of replying to existing threads.
> 
> [1]: https://www.kernel.org/doc/html/v6.9/process/submitting-patches.html#explicit-in-reply-to-headers
> 
> > Other than Zicsr/Zifencei that may need explicit handling in a dedicated
> > option, the approach here seems kinda backwards.
> > Individually these symbols don't actually mean what they say they do,
> > which is confusing: "recognises" here is true even when it may not be
> > true at all because TOOLCHAIN_HAS_FOO is not set. Why can these options
> > not be removed, and instead the TOOLCHAIN_HAS_FOO options grow a
> > "depends on !RUST || <condition>"?
> 
> Yes, it's kinda "backwards", which is intentional, based on the following
> considerations:
> 
> 1) As mentioned in rust/Makefile, filtering flags for libclang is a hack,
> because currently bindgen only has libclang as backend, and ideally bindgen
> should support GCC so that the passed CC flags are supposed to be fully
> compatible. On the RISC-V side, I tend to think that version checking for
> extensions for libclang is also a hack, which could have been accomplished
> with just the cc-option function, ideally.
> 
> 2) Rust bindgen only "generates" FFI stuff, it is not involved in the final
> assembly stage. In other words, it doesn't matter so much what RISC-V
> extensions to turn on for bindgen (although it does have a little impact,
> like some macro switches), it's more matter to CC.

> Therefore, I chose not to modify the original extension config conditions so
> that if libclang doesn't support the CC flag for an extension, then the Rust
> build is not supported, rather than treating the extension as not supported.

I don't agree with this take, I don't think that any extension should
"blindly" take priority over rust like this. Got two or three main gripes
with how it is being done here.
Firstly, you're lumping every extension into one option even though many
of them will not be even implemented on the target. There's no need to
disable rust if the user has no intention of even making use of the
extension that would block its use. That runs into the second point, in
that you're using TOOLCHAIN_HAS_FOO here, which is only an indicator of
whether the toolchain supports the extension not whether the kernel is
even going to use it. The third problem I have is that the symbol you're
interacting with is not user selectable, and therefore doesn't allow the
user to decide whether or not a particular extension or rust support
with the toolchain they have is the higher priority. If the check moves
to the individual TOOLCHAIN_HAS_FOO options, they could be a

	depends on !RUST || <condition>

which would allow the user to make a decision about which has a greater
priority while also handling the extensions individually.

> Nonetheless, it occurred to me as I was writing this reply that if GCC
> implements a new extension in the future that LLVM/Clang doesn't yet have,
> this could once again lead to a break in GCC+Rust build support if the
> kernel decides to use the new extension. So it's a trade-off, you guys
> decide, I'm fine with both.
> 
> Regarding the name, initially I named it "compatible", and ended up changed
> it to "recognize" before sending the patch. If we continue on this path, I'm
> not sure what name is appropriate to use here, do you guys have any ideas?
> 
> > What does the libclang >= 17 requirement actually do here? Is that the
> > version where llvm starts to require that Zicsr/Zifencei is set in order
> > to use them? I think a comment to that effect is required if so. This
> > doesn't actually need to be blocking either, should just be able to
> > filter it out of march when passing to bindgen, no?
> 
> libclang >= 17 starts recognizing Zicsr/Zifencei in -march, passing them to
> -march doesn't generate an error, and passing them or not doesn't have any
> real difference. (still follows ISA before version 20190608 --
> Zicsr/Zifencei are included in base ISA). I should have written a comment
> there to avoid confusion.
> 
> Reference commit in LLVM/Clang 22e199e6af ("[RISCV] Accept zicsr and
> zifencei command line options")
> https://github.com/llvm/llvm-project/commit/22e199e6afb1263c943c0c0d4498694e15bf8a16
> 
> > What about the case where TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI is not
> > set at all? Currently your patch is going to block rust in that case,
> > when actually nothing needs to be done at all - no part of the toolchain
> > requires understanding Zicsr/Zifencei as standalone extensions in this
> > case.
> 
> This is a bug, I missed this case. So it should be corrected to:
> 
>    config RUST_BINDGEN_LIBCLANG_RECOGNIZES_ZICSR_ZIFENCEI
>         def_bool y
>         depends on TOOLCHAIN_NEEDS_OLD_ISA_SPEC ||
>    !TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI ||
>    RUST_BINDGEN_LIBCLANG_VERSION >= 170000
> 
> 
> > The TOOLCHAIN_NEEDS_OLD_ISA_SPEC handling I don't remember 100% how it
> > works, but if bindgen requires them to be set to use the extension
> > this will return true but do nothing to add the extensions to march?
> > That seems wrong to me.
> > I'd be fairly amenable to disabling rust though when used in combination
> > with gcc < 11.3 and gas >=2.36 since it's such a niche condition, rather
> > doing work to support it. That'd be effectively an inversion of your
> > first condition.
> 
> The current latest version of LLVM/Clang still does not require explicit
> Zicsr/Zifence to enable these two extensions, Clang just accepts them in
> -march and then silently ignores them.
> 
> Checking the usage of CONFIG_TOOLCHAIN_NEEDS_OLD_ISA_SPEC:
> 
>    ifdef CONFIG_TOOLCHAIN_NEEDS_OLD_ISA_SPEC
>    KBUILD_CFLAGS += -Wa,-misa-spec=2.2
>    KBUILD_AFLAGS += -Wa,-misa-spec=2.2
>    else
>    riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI) :=
>    $(riscv-march-y)_zicsr_zifencei
>    endif
> 
> It just uses -Wa to force an older ISA version to GAS. So the
> RUST_BINDGEN_LIBCLANG_RECOGNIZES_ZICSR_ZIFENCEI I corrected above should be
> fine now I guess? Or would you still prefer your idea of blocking Rust if
> TOOLCHAIN_NEEDS_OLD_ISA_SPEC is true?

Nah, if the explicit setting isn't required then it should be fine to
not block on it being used. To be honest, I'm not concerned about
Zicsr/Zifencei being communicated across to bindgen as much as I would
be about other extensions, my motivation here is regarding build
breakages - in particular when things like TOOLCHAIN_NEEDS_OLD_ISA_SPEC
is set, since it's a very niche configuration that if someone told me
they were using I would tell them to stop. As I said, the original
reason for this existing was to support w/e old version of debian linaro
were using that could not do LLVM=1 builds and I think the person who
added to this handle gcc with older binutils was trying to do a gradual
move from an old toolchain in steps to a modern one, so neither were
instances of someone actually wanting to use such a strange mix.

> (To be clear, the breaking changes regarding Zicsr/Zifence are since ISA
> version 20190608, and versions 2.0, 2.1, 2.2 are older than 20190608)
> 
> The only thing I'm confused about is that according to the comment of
> TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI, GCC-12.1.0 bumped the default ISA
> to 20191213, but why doesn't the depends-on have condition || (CC_IS_GCC &&
> GCC_VERSION >= 120100)?

It's probably something along the lines of there being no _C_ code that
produces the Zicsr and Zifencei instructions, and therefore no build
errors produced if they're missing. That's part of why I said my
motivation in this particular case is build breakage, more than anything
else.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists