diff mbox series

[v1,1/2] RISC-V: move zicsr/zifencei spec version check to Kconfig

Message ID 20230223220546.52879-2-conor@kernel.org (mailing list archive)
State Superseded
Headers show
Series RISC-V: avoid build issues for clang/llvm-17 with binutils 2.35 | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Conor Dooley Feb. 23, 2023, 10:05 p.m. UTC 
From: Conor Dooley <conor.dooley@microchip.com>

Checking whether binutils defaults to using a version of the spec
requiring zicsr/zifencei being done in our Makefile is functionally
sufficient at present, but makes it harder to tell after the fact
which extensions are enabled.
By moving it to Kconfig, it's easy to tell from standard build artifacts
what has been done & the road is paved for dealing with this differently
for both binutils and LLVM.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/Kconfig  | 10 ++++++++++
 arch/riscv/Makefile |  5 +----
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Nathan Chancellor Feb. 24, 2023, 4:37 p.m. UTC | #1 
On Thu, Feb 23, 2023 at 10:05:45PM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Checking whether binutils defaults to using a version of the spec
> requiring zicsr/zifencei being done in our Makefile is functionally
> sufficient at present, but makes it harder to tell after the fact
> which extensions are enabled.
> By moving it to Kconfig, it's easy to tell from standard build artifacts
> what has been done & the road is paved for dealing with this differently
> for both binutils and LLVM.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  arch/riscv/Kconfig  | 10 ++++++++++
>  arch/riscv/Makefile |  5 +----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index cebf0c5f8824..4eb0ef8314b3 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -288,6 +288,16 @@ config ARCH_RV64I
>  
>  endchoice
>  
> +config TOOLCHAIN_NEEDS_SPEC_20191213

This symbol's name is a little confusing to me, how does the toolchain
need spec version 20191213. Maybe 'SPEC_20191213' should be something
like 'EXPLICIT_ZICSR_ZIFENCEI'? Otherwise, maybe it should be something
like 'TOOLCHAIN_HAS_DEFAULT_SPEC_20191213'? Sorry for the bikeshed but I
think most Kconfig symbols should be self describing.

> +	bool
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicsr_zifencei)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicsr_zifencei)
> +	help
> +	  Newer binutils versions default to ISA spec version 20191213 which
> +	  moves some instructions from the I extension to the Zicsr and Zifencei
> +	  extensions.
> +
>  # We must be able to map all physical memory into the kernel, but the compiler
>  # is still a bit more efficient when generating code if it's setup in a manner
>  # such that it can only map 2GiB of memory.
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 12d91b0a73d8..7e1267b0e73f 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -53,10 +53,7 @@ riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
>  riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
>  riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>  
> -# Newer binutils versions default to ISA spec version 20191213 which moves some
> -# instructions from the I extension to the Zicsr and Zifencei extensions.
> -toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
> -riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> +riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_SPEC_20191213) := $(riscv-march-y)_zicsr_zifencei
>  
>  # Check if the toolchain supports Zicbom extension
>  riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
> -- 
> 2.39.1
>
Conor Dooley Feb. 24, 2023, 5 p.m. UTC | #2 
On Fri, Feb 24, 2023 at 09:37:28AM -0700, Nathan Chancellor wrote:
> On Thu, Feb 23, 2023 at 10:05:45PM +0000, Conor Dooley wrote:

> > ---
> >  arch/riscv/Kconfig  | 10 ++++++++++
> >  arch/riscv/Makefile |  5 +----
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index cebf0c5f8824..4eb0ef8314b3 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -288,6 +288,16 @@ config ARCH_RV64I
> >  
> >  endchoice
> >  
> > +config TOOLCHAIN_NEEDS_SPEC_20191213
> 
> This symbol's name is a little confusing to me, how does the toolchain
> need spec version 20191213. Maybe 'SPEC_20191213' should be something
> like 'EXPLICIT_ZICSR_ZIFENCEI'? Otherwise, maybe it should be something
> like 'TOOLCHAIN_HAS_DEFAULT_SPEC_20191213'? Sorry for the bikeshed but I
> think most Kconfig symbols should be self describing.

Yah I can do that. I actually had this as the DEFAULT_SPEC variant
before and changed it - I'll switch it to the EXPLICIT one.
Thanks for taking a look Nathan :)

Conor.
Palmer Dabbelt March 6, 2023, 11:52 p.m. UTC | #3 
On Fri, 24 Feb 2023 09:00:09 PST (-0800), Conor Dooley wrote:
> On Fri, Feb 24, 2023 at 09:37:28AM -0700, Nathan Chancellor wrote:
>> On Thu, Feb 23, 2023 at 10:05:45PM +0000, Conor Dooley wrote:
>
>> > ---
>> >  arch/riscv/Kconfig  | 10 ++++++++++
>> >  arch/riscv/Makefile |  5 +----
>> >  2 files changed, 11 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > index cebf0c5f8824..4eb0ef8314b3 100644
>> > --- a/arch/riscv/Kconfig
>> > +++ b/arch/riscv/Kconfig
>> > @@ -288,6 +288,16 @@ config ARCH_RV64I
>> >  
>> >  endchoice
>> >  
>> > +config TOOLCHAIN_NEEDS_SPEC_20191213
>> 
>> This symbol's name is a little confusing to me, how does the toolchain
>> need spec version 20191213. Maybe 'SPEC_20191213' should be something
>> like 'EXPLICIT_ZICSR_ZIFENCEI'? Otherwise, maybe it should be something
>> like 'TOOLCHAIN_HAS_DEFAULT_SPEC_20191213'? Sorry for the bikeshed but I
>> think most Kconfig symbols should be self describing.
>
> Yah I can do that. I actually had this as the DEFAULT_SPEC variant
> before and changed it - I'll switch it to the EXPLICIT one.
> Thanks for taking a look Nathan :)

Another option here would be to use the `-misa-spec` argument for GCC, 
to make it match LLVM's spec version.
Conor Dooley March 7, 2023, 7:14 p.m. UTC | #4 
On Mon, Mar 06, 2023 at 03:52:27PM -0800, Palmer Dabbelt wrote:
> On Fri, 24 Feb 2023 09:00:09 PST (-0800), Conor Dooley wrote:
> > On Fri, Feb 24, 2023 at 09:37:28AM -0700, Nathan Chancellor wrote:
> > > On Thu, Feb 23, 2023 at 10:05:45PM +0000, Conor Dooley wrote:
> > 
> > > > ---
> > > >  arch/riscv/Kconfig  | 10 ++++++++++
> > > >  arch/riscv/Makefile |  5 +----
> > > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index cebf0c5f8824..4eb0ef8314b3 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -288,6 +288,16 @@ config ARCH_RV64I
> > > >  >  endchoice
> > > >  > +config TOOLCHAIN_NEEDS_SPEC_20191213
> > > 
> > > This symbol's name is a little confusing to me, how does the toolchain
> > > need spec version 20191213. Maybe 'SPEC_20191213' should be something
> > > like 'EXPLICIT_ZICSR_ZIFENCEI'? Otherwise, maybe it should be something
> > > like 'TOOLCHAIN_HAS_DEFAULT_SPEC_20191213'? Sorry for the bikeshed but I
> > > think most Kconfig symbols should be self describing.
> > 
> > Yah I can do that. I actually had this as the DEFAULT_SPEC variant
> > before and changed it - I'll switch it to the EXPLICIT one.
> > Thanks for taking a look Nathan :)
> 
> Another option here would be to use the `-misa-spec` argument for GCC, to
> make it match LLVM's spec version.

I'm not entirely sure what you're getting at.
You're the toolchain guy, but my understanding was that gas and gcc have
that option, but not ld, which rules out my initial interpretation. 
Are you suggesting that we always pass -misa-spec=<pre-zicsr_zifencei> to
gcc so that we are using the same misa-spec for both gnu and llvm
toolchain bits?

My instinctive thought was that that is kinda fragile to things
misaligning between gcc/llvm, but given I seem to end up looking at this
every month, we're really fragile to changes in either camp anyway...

I can go spin an alternative v2 of this up if I have understood you
correctly.

Thanks,
Conor.
Palmer Dabbelt March 7, 2023, 7:24 p.m. UTC | #5 
On Tue, 07 Mar 2023 11:14:54 PST (-0800), Conor Dooley wrote:
> On Mon, Mar 06, 2023 at 03:52:27PM -0800, Palmer Dabbelt wrote:
>> On Fri, 24 Feb 2023 09:00:09 PST (-0800), Conor Dooley wrote:
>> > On Fri, Feb 24, 2023 at 09:37:28AM -0700, Nathan Chancellor wrote:
>> > > On Thu, Feb 23, 2023 at 10:05:45PM +0000, Conor Dooley wrote:
>> > 
>> > > > ---
>> > > >  arch/riscv/Kconfig  | 10 ++++++++++
>> > > >  arch/riscv/Makefile |  5 +----
>> > > >  2 files changed, 11 insertions(+), 4 deletions(-)
>> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > > > index cebf0c5f8824..4eb0ef8314b3 100644
>> > > > --- a/arch/riscv/Kconfig
>> > > > +++ b/arch/riscv/Kconfig
>> > > > @@ -288,6 +288,16 @@ config ARCH_RV64I
>> > > >  >  endchoice
>> > > >  > +config TOOLCHAIN_NEEDS_SPEC_20191213
>> > > 
>> > > This symbol's name is a little confusing to me, how does the toolchain
>> > > need spec version 20191213. Maybe 'SPEC_20191213' should be something
>> > > like 'EXPLICIT_ZICSR_ZIFENCEI'? Otherwise, maybe it should be something
>> > > like 'TOOLCHAIN_HAS_DEFAULT_SPEC_20191213'? Sorry for the bikeshed but I
>> > > think most Kconfig symbols should be self describing.
>> > 
>> > Yah I can do that. I actually had this as the DEFAULT_SPEC variant
>> > before and changed it - I'll switch it to the EXPLICIT one.
>> > Thanks for taking a look Nathan :)
>> 
>> Another option here would be to use the `-misa-spec` argument for GCC, to
>> make it match LLVM's spec version.
>
> I'm not entirely sure what you're getting at.
> You're the toolchain guy, but my understanding was that gas and gcc have
> that option, but not ld, which rules out my initial interpretation. 
> Are you suggesting that we always pass -misa-spec=<pre-zicsr_zifencei> to
> gcc so that we are using the same misa-spec for both gnu and llvm
> toolchain bits?

Pretty much, I think just $(cc-option -misa-spec=2.2) should do it.  
That should also cover older GCC versions, as we added the argument long 
before we changed the default.

> My instinctive thought was that that is kinda fragile to things
> misaligning between gcc/llvm, but given I seem to end up looking at this
> every month, we're really fragile to changes in either camp anyway...

Pretty much, but in this case we're essentially just disabling a form of 
change by sticking to an ISA spec version.

> I can go spin an alternative v2 of this up if I have understood you
> correctly.

Thanks

>
> Thanks,
> Conor.
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index cebf0c5f8824..4eb0ef8314b3 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -288,6 +288,16 @@  config ARCH_RV64I
 
 endchoice
 
+config TOOLCHAIN_NEEDS_SPEC_20191213
+	bool
+	default y
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicsr_zifencei)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicsr_zifencei)
+	help
+	  Newer binutils versions default to ISA spec version 20191213 which
+	  moves some instructions from the I extension to the Zicsr and Zifencei
+	  extensions.
+
 # We must be able to map all physical memory into the kernel, but the compiler
 # is still a bit more efficient when generating code if it's setup in a manner
 # such that it can only map 2GiB of memory.
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 12d91b0a73d8..7e1267b0e73f 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -53,10 +53,7 @@  riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
 riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
 riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
 
-# Newer binutils versions default to ISA spec version 20191213 which moves some
-# instructions from the I extension to the Zicsr and Zifencei extensions.
-toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
-riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
+riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_SPEC_20191213) := $(riscv-march-y)_zicsr_zifencei
 
 # Check if the toolchain supports Zicbom extension
 riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom