[PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey,
This came up due to a report from Kevin @ kernel-ci, who had been
running a mixed configuration of GNU binutils and clang. Their compiler
was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.

Our current checks for extension support only cover the compiler, but it
appears to me that we need to check both the compiler & linker support
in case of "pot-luck" configurations that mix different versions of
LD,AS,CC etc.

Linker support does not seem possible to actually check, since the ISA
string is emitted into the object files - so I put in version checks for
that. The checks have gotten a bit ugly since 32 & 64 bit support need
to be checked independently but ahh well.

As I was going, I fell into the trap of there being duplicated checks
for CC support in both the Makefile and Kconfig, so as part of renaming
the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
symbol for Zihintpause appearing in .config.

I pushed out a version of this that specificly checked for assember
support for LKP to test & it looked /okay/ - but I did some more testing
today and realised that this is redudant & have since dropped the as
check.

I tested locally with a fair few different combinations, to try and
cover each of AS, LD, CC missing support for the extension.

The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
has:

> # 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

I'd like to also move that one to Kconfig rather than the Makefile so
that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
appear to be emitted into the object files it's not a fix and can be a
follow-on patch if my approach here is not entirely off-the-wall.

I am not 100% on the LD/LLD versions that I picked, I went off of a
`git branch -a --contains` of the first commits I found that with
mention of the extension. Please scream if I got it wrong, I'm not
overly familar with where to find this sort of info for the toolchains.

Thanks,
Conor.

Conor Dooley (2):
  riscv: fix detection of toolchain Zicbom support
  riscv: fix detection of toolchain Zihintpause support

 arch/riscv/Kconfig                      | 17 +++++++++++++----
 arch/riscv/Makefile                     |  6 ++----
 arch/riscv/include/asm/vdso/processor.h |  2 +-
 3 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.37.3

[PATCH 1/2] riscv: fix detection of toolchain Zicbom support

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

It is not sufficient to check if a toolchain supports a particular
extension without checking if the linker supports that extension too.
For example, Clang 15 supports Zicbom but GNU bintutils 2.35.2 does
not, leading build errors like so:

riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zicbom1p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zicbom'

Convert CC_HAS_ZICBOM to TOOLCHAIN_HAS_ZICBOM & check if the linker
also supports Zicbom.

Reported-by: Kevin Hilman <khilman@baylibre.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1714
Link: https://storage.kernelci.org/next/master/next-20220920/riscv/defconfig+CONFIG_EFI=n/clang-16/logs/kernel.log
Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/Kconfig  | 10 ++++++----
 arch/riscv/Makefile |  3 +--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d557cc50295d..6da36553158b 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -401,14 +401,16 @@ config RISCV_ISA_SVPBMT
 
 	   If you don't know what to do here, say Y.
 
-config CC_HAS_ZICBOM
+config TOOLCHAIN_HAS_ZICBOM
 	bool
-	default y if 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
-	default y if 32BIT && $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
+	default y
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
+	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
 
 config RISCV_ISA_ZICBOM
 	bool "Zicbom extension support for non-coherent DMA operation"
-	depends on CC_HAS_ZICBOM
+	depends on TOOLCHAIN_HAS_ZICBOM
 	depends on !XIP_KERNEL && MMU
 	select RISCV_DMA_NONCOHERENT
 	select RISCV_ALTERNATIVE
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 3fa8ef336822..3607d38edb4f 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -57,8 +57,7 @@ toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zi
 riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
 
 # Check if the toolchain supports Zicbom extension
-toolchain-supports-zicbom := $(call cc-option-yn, -march=$(riscv-march-y)_zicbom)
-riscv-march-$(toolchain-supports-zicbom) := $(riscv-march-y)_zicbom
+riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
 
 # Check if the toolchain supports Zihintpause extension
 toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
-- 
2.37.3

[PATCH 2/2] riscv: fix detection of toolchain Zihintpause support

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

It is not sufficient to check if a toolchain supports a particular
extension without checking if the linker supports that extension
too. For example, Clang 15 supports Zihintpause but GNU bintutils
2.35.2 does not, leading build errors like so:

riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zihintpause'

Add a TOOLCHAIN_HAS_ZIHINTPAUSE which checks if each of the compiler,
assembler and linker support the extension. Replace the ifdef in the
vdso with one depending on this new symbol.

Fixes: 8eb060e10185 ("arch/riscv: add Zihintpause support")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Palmer:
The VDSO change will conflict with Samuel's one, resolution should be
trivial.. I only made that change as you warned me about checking for
the __riscv_foo stuff if I made the march string depend on the Kconfig
entry rather than on the Makefile's cc-option check.
---
 arch/riscv/Kconfig                      | 7 +++++++
 arch/riscv/Makefile                     | 3 +--
 arch/riscv/include/asm/vdso/processor.h | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 6da36553158b..d7c53896e24f 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -425,6 +425,13 @@ config RISCV_ISA_ZICBOM
 
 	   If you don't know what to do here, say Y.
 
+config TOOLCHAIN_HAS_ZIHINTPAUSE
+	bool
+	default y
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause)
+	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600
+
 config FPU
 	bool "FPU support"
 	default y
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 3607d38edb4f..6651517f3962 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -60,8 +60,7 @@ riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
 riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
 
 # Check if the toolchain supports Zihintpause extension
-toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
-riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
+riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
 
 KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
 KBUILD_AFLAGS += -march=$(riscv-march-y)
diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
index 1e4f8b4aef79..fa70cfe507aa 100644
--- a/arch/riscv/include/asm/vdso/processor.h
+++ b/arch/riscv/include/asm/vdso/processor.h
@@ -21,7 +21,7 @@ static inline void cpu_relax(void)
 		 * Reduce instruction retirement.
 		 * This assumes the PC changes.
 		 */
-#ifdef __riscv_zihintpause
+#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
 		__asm__ __volatile__ ("pause");
 #else
 		/* Encoding of the pause instruction */
-- 
2.37.3

Re: [PATCH 1/2] riscv: fix detection of toolchain Zicbom support

[Heiko Stübner]

Hi Conor,

Am Donnerstag, 6. Oktober 2022, 19:35:20 CEST schrieb Conor Dooley:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> It is not sufficient to check if a toolchain supports a particular
> extension without checking if the linker supports that extension too.
> For example, Clang 15 supports Zicbom but GNU bintutils 2.35.2 does
> not, leading build errors like so:
> 
> riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zicbom1p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zicbom'
> 
> Convert CC_HAS_ZICBOM to TOOLCHAIN_HAS_ZICBOM & check if the linker
> also supports Zicbom.
> 
> Reported-by: Kevin Hilman <khilman@baylibre.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1714
> Link: https://storage.kernelci.org/next/master/next-20220920/riscv/defconfig+CONFIG_EFI=n/clang-16/logs/kernel.log
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/Kconfig  | 10 ++++++----
>  arch/riscv/Makefile |  3 +--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d557cc50295d..6da36553158b 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -401,14 +401,16 @@ config RISCV_ISA_SVPBMT
>  
>  	   If you don't know what to do here, say Y.
>  
> -config CC_HAS_ZICBOM
> +config TOOLCHAIN_HAS_ZICBOM
>  	bool
> -	default y if 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> -	default y if 32BIT && $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> +	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900

I did needed to look quite closely at the cc-option-voodoo, but it really
seems to work out as expected :-)

I can't say much to the specific L(L)D_VERSION values but the change itself
looks good, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

>  
>  config RISCV_ISA_ZICBOM
>  	bool "Zicbom extension support for non-coherent DMA operation"
> -	depends on CC_HAS_ZICBOM
> +	depends on TOOLCHAIN_HAS_ZICBOM
>  	depends on !XIP_KERNEL && MMU
>  	select RISCV_DMA_NONCOHERENT
>  	select RISCV_ALTERNATIVE
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 3fa8ef336822..3607d38edb4f 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -57,8 +57,7 @@ toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zi
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>  
>  # Check if the toolchain supports Zicbom extension
> -toolchain-supports-zicbom := $(call cc-option-yn, -march=$(riscv-march-y)_zicbom)
> -riscv-march-$(toolchain-supports-zicbom) := $(riscv-march-y)_zicbom
> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
>  
>  # Check if the toolchain supports Zihintpause extension
>  toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> 

Re: [PATCH 2/2] riscv: fix detection of toolchain Zihintpause support

[Heiko Stübner]

Am Donnerstag, 6. Oktober 2022, 19:35:21 CEST schrieb Conor Dooley:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> It is not sufficient to check if a toolchain supports a particular
> extension without checking if the linker supports that extension
> too. For example, Clang 15 supports Zihintpause but GNU bintutils
> 2.35.2 does not, leading build errors like so:
> 
> riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zihintpause'
> 
> Add a TOOLCHAIN_HAS_ZIHINTPAUSE which checks if each of the compiler,
> assembler and linker support the extension. Replace the ifdef in the
> vdso with one depending on this new symbol.
> 
> Fixes: 8eb060e10185 ("arch/riscv: add Zihintpause support")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 1/2] riscv: fix detection of toolchain Zicbom support

[Nathan Chancellor]

On Thu, Oct 06, 2022 at 06:35:20PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> It is not sufficient to check if a toolchain supports a particular
> extension without checking if the linker supports that extension too.
> For example, Clang 15 supports Zicbom but GNU bintutils 2.35.2 does
> not, leading build errors like so:
> 
> riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zicbom1p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zicbom'
> 
> Convert CC_HAS_ZICBOM to TOOLCHAIN_HAS_ZICBOM & check if the linker
> also supports Zicbom.
> 
> Reported-by: Kevin Hilman <khilman@baylibre.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1714
> Link: https://storage.kernelci.org/next/master/next-20220920/riscv/defconfig+CONFIG_EFI=n/clang-16/logs/kernel.log
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

The versions look correct to me. I see the LLVM zicbom commit [1] in
llvmorg-15.0.0 and I see the binutils zicbom commit [2] in
binutils-2_39.

FWIW, if we are adding explicit tool versions to the Kconfig, could you
not also drop the cc-option checks? Typically, cc-option is only used
when dynamically checking for a feature, in lieu of statically checking
a version. No strong opinion though.

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

[1]: https://github.com/llvm/llvm-project/commit/4f40ca53cefb725aca6564585d0ec4836a79e21a
[2]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=41d6ac5da655a2e78109848f2db47e53552fd61a

> ---
>  arch/riscv/Kconfig  | 10 ++++++----
>  arch/riscv/Makefile |  3 +--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d557cc50295d..6da36553158b 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -401,14 +401,16 @@ config RISCV_ISA_SVPBMT
>  
>  	   If you don't know what to do here, say Y.
>  
> -config CC_HAS_ZICBOM
> +config TOOLCHAIN_HAS_ZICBOM
>  	bool
> -	default y if 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> -	default y if 32BIT && $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> +	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
>  
>  config RISCV_ISA_ZICBOM
>  	bool "Zicbom extension support for non-coherent DMA operation"
> -	depends on CC_HAS_ZICBOM
> +	depends on TOOLCHAIN_HAS_ZICBOM
>  	depends on !XIP_KERNEL && MMU
>  	select RISCV_DMA_NONCOHERENT
>  	select RISCV_ALTERNATIVE
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 3fa8ef336822..3607d38edb4f 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -57,8 +57,7 @@ toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zi
>  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>  
>  # Check if the toolchain supports Zicbom extension
> -toolchain-supports-zicbom := $(call cc-option-yn, -march=$(riscv-march-y)_zicbom)
> -riscv-march-$(toolchain-supports-zicbom) := $(riscv-march-y)_zicbom
> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
>  
>  # Check if the toolchain supports Zihintpause extension
>  toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> -- 
> 2.37.3
> 
> 

Re: [PATCH 2/2] riscv: fix detection of toolchain Zihintpause support

[Nathan Chancellor]

On Thu, Oct 06, 2022 at 06:35:21PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> It is not sufficient to check if a toolchain supports a particular
> extension without checking if the linker supports that extension
> too. For example, Clang 15 supports Zihintpause but GNU bintutils
> 2.35.2 does not, leading build errors like so:
> 
> riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zihintpause'
> 
> Add a TOOLCHAIN_HAS_ZIHINTPAUSE which checks if each of the compiler,
> assembler and linker support the extension. Replace the ifdef in the
> vdso with one depending on this new symbol.
> 
> Fixes: 8eb060e10185 ("arch/riscv: add Zihintpause support")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Palmer:
> The VDSO change will conflict with Samuel's one, resolution should be
> trivial.. I only made that change as you warned me about checking for
> the __riscv_foo stuff if I made the march string depend on the Kconfig
> entry rather than on the Makefile's cc-option check.

The versions look correct to me. I see the LLVM zihintpause commit [1]
in llvmorg-15.0.0 and I see the binutils zihintpause commit [2] in
binutils-2_36.

[1]: https://github.com/llvm/llvm-project/commit/005fd8aa702edbc532763038365575da96e5787d
[2]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=aa881ecde48c7a0224b92e2cfa43b37ee9ec9fa2

Similar comment as patch 1, I think we can just drop the cc-option
checks. Regardless:

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

> ---
>  arch/riscv/Kconfig                      | 7 +++++++
>  arch/riscv/Makefile                     | 3 +--
>  arch/riscv/include/asm/vdso/processor.h | 2 +-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 6da36553158b..d7c53896e24f 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -425,6 +425,13 @@ config RISCV_ISA_ZICBOM
>  
>  	   If you don't know what to do here, say Y.
>  
> +config TOOLCHAIN_HAS_ZIHINTPAUSE
> +	bool
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause)
> +	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600
> +
>  config FPU
>  	bool "FPU support"
>  	default y
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 3607d38edb4f..6651517f3962 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -60,8 +60,7 @@ riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
>  riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
>  
>  # Check if the toolchain supports Zihintpause extension
> -toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> -riscv-march-$(toolchain-supports-zihintpause) := $(riscv-march-y)_zihintpause
> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
>  
>  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
> diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
> index 1e4f8b4aef79..fa70cfe507aa 100644
> --- a/arch/riscv/include/asm/vdso/processor.h
> +++ b/arch/riscv/include/asm/vdso/processor.h
> @@ -21,7 +21,7 @@ static inline void cpu_relax(void)
>  		 * Reduce instruction retirement.
>  		 * This assumes the PC changes.
>  		 */
> -#ifdef __riscv_zihintpause
> +#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
>  		__asm__ __volatile__ ("pause");
>  #else
>  		/* Encoding of the pause instruction */
> -- 
> 2.37.3
> 
> 

Re: [PATCH 1/2] riscv: fix detection of toolchain Zicbom support

[Conor Dooley]

On Thu, Oct 13, 2022 at 01:22:47PM -0700, Nathan Chancellor wrote:
> On Thu, Oct 06, 2022 at 06:35:20PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > It is not sufficient to check if a toolchain supports a particular
> > extension without checking if the linker supports that extension too.
> > For example, Clang 15 supports Zicbom but GNU bintutils 2.35.2 does
> > not, leading build errors like so:
> > 
> > riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zicbom1p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zicbom'
> > 
> > Convert CC_HAS_ZICBOM to TOOLCHAIN_HAS_ZICBOM & check if the linker
> > also supports Zicbom.
> > 
> > Reported-by: Kevin Hilman <khilman@baylibre.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1714
> > Link: https://storage.kernelci.org/next/master/next-20220920/riscv/defconfig+CONFIG_EFI=n/clang-16/logs/kernel.log
> > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> The versions look correct to me. I see the LLVM zicbom commit [1] in
> llvmorg-15.0.0 and I see the binutils zicbom commit [2] in
> binutils-2_39.
> 
> FWIW, if we are adding explicit tool versions to the Kconfig, could you
> not also drop the cc-option checks? Typically, cc-option is only used
> when dynamically checking for a feature, in lieu of statically checking
> a version. No strong opinion though.

Ehh, the explicit version checks are for linker support, but it could be
that your linker supports it but other things do not. The compilers
support can still be checked with cc-option, no?

This error here happens when the compiler does support it, so the string
containing zicbom is emitted in the object files. That can't be checked
dynamically. For the opposite situation, dynamic checking is possible so
I tried retained them. If it's convention to do one or the other, I can
easily switch.

I'm out of my comfort zone when it comes to kbuild plumbing so please,
please lmk if I'm missing something...

> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> 
> [1]: https://github.com/llvm/llvm-project/commit/4f40ca53cefb725aca6564585d0ec4836a79e21a
> [2]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=41d6ac5da655a2e78109848f2db47e53552fd61a
> 
> > ---
> >  arch/riscv/Kconfig  | 10 ++++++----
> >  arch/riscv/Makefile |  3 +--
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index d557cc50295d..6da36553158b 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -401,14 +401,16 @@ config RISCV_ISA_SVPBMT
> >  
> >  	   If you don't know what to do here, say Y.
> >  
> > -config CC_HAS_ZICBOM
> > +config TOOLCHAIN_HAS_ZICBOM
> >  	bool
> > -	default y if 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> > -	default y if 32BIT && $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> > +	default y
> > +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> > +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> > +	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> >  
> >  config RISCV_ISA_ZICBOM
> >  	bool "Zicbom extension support for non-coherent DMA operation"
> > -	depends on CC_HAS_ZICBOM
> > +	depends on TOOLCHAIN_HAS_ZICBOM
> >  	depends on !XIP_KERNEL && MMU
> >  	select RISCV_DMA_NONCOHERENT
> >  	select RISCV_ALTERNATIVE
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 3fa8ef336822..3607d38edb4f 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -57,8 +57,7 @@ toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zi
> >  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> >  
> >  # Check if the toolchain supports Zicbom extension
> > -toolchain-supports-zicbom := $(call cc-option-yn, -march=$(riscv-march-y)_zicbom)
> > -riscv-march-$(toolchain-supports-zicbom) := $(riscv-march-y)_zicbom
> > +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
> >  
> >  # Check if the toolchain supports Zihintpause extension
> >  toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> > -- 
> > 2.37.3
> > 
> > 

Re: [PATCH 1/2] riscv: fix detection of toolchain Zicbom support

[Nathan Chancellor]

On Thu, Oct 13, 2022 at 09:33:29PM +0100, Conor Dooley wrote:
> On Thu, Oct 13, 2022 at 01:22:47PM -0700, Nathan Chancellor wrote:
> > On Thu, Oct 06, 2022 at 06:35:20PM +0100, Conor Dooley wrote:
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > 
> > > It is not sufficient to check if a toolchain supports a particular
> > > extension without checking if the linker supports that extension too.
> > > For example, Clang 15 supports Zicbom but GNU bintutils 2.35.2 does
> > > not, leading build errors like so:
> > > 
> > > riscv64-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_c2p0_zicbom1p0_zihintpause2p0: Invalid or unknown z ISA extension: 'zicbom'
> > > 
> > > Convert CC_HAS_ZICBOM to TOOLCHAIN_HAS_ZICBOM & check if the linker
> > > also supports Zicbom.
> > > 
> > > Reported-by: Kevin Hilman <khilman@baylibre.com>
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1714
> > > Link: https://storage.kernelci.org/next/master/next-20220920/riscv/defconfig+CONFIG_EFI=n/clang-16/logs/kernel.log
> > > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > The versions look correct to me. I see the LLVM zicbom commit [1] in
> > llvmorg-15.0.0 and I see the binutils zicbom commit [2] in
> > binutils-2_39.
> > 
> > FWIW, if we are adding explicit tool versions to the Kconfig, could you
> > not also drop the cc-option checks? Typically, cc-option is only used
> > when dynamically checking for a feature, in lieu of statically checking
> > a version. No strong opinion though.
> 
> Ehh, the explicit version checks are for linker support, but it could be
> that your linker supports it but other things do not. The compilers
> support can still be checked with cc-option, no?
> 
> This error here happens when the compiler does support it, so the string
> containing zicbom is emitted in the object files. That can't be checked
> dynamically. For the opposite situation, dynamic checking is possible so
> I tried retained them. If it's convention to do one or the other, I can
> easily switch.
> 
> I'm out of my comfort zone when it comes to kbuild plumbing so please,
> please lmk if I'm missing something...

No, you are absolutely correct. That's what I get for reviewing changes
right after being off for a week ;) the cc-option checks should remain
in addition to the new linker checks, so please disregard that feedback
on both changes.

> > 
> > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > 
> > [1]: https://github.com/llvm/llvm-project/commit/4f40ca53cefb725aca6564585d0ec4836a79e21a
> > [2]: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=41d6ac5da655a2e78109848f2db47e53552fd61a
> > 
> > > ---
> > >  arch/riscv/Kconfig  | 10 ++++++----
> > >  arch/riscv/Makefile |  3 +--
> > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index d557cc50295d..6da36553158b 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -401,14 +401,16 @@ config RISCV_ISA_SVPBMT
> > >  
> > >  	   If you don't know what to do here, say Y.
> > >  
> > > -config CC_HAS_ZICBOM
> > > +config TOOLCHAIN_HAS_ZICBOM
> > >  	bool
> > > -	default y if 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> > > -	default y if 32BIT && $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> > > +	default y
> > > +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zicbom)
> > > +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zicbom)
> > > +	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> > >  
> > >  config RISCV_ISA_ZICBOM
> > >  	bool "Zicbom extension support for non-coherent DMA operation"
> > > -	depends on CC_HAS_ZICBOM
> > > +	depends on TOOLCHAIN_HAS_ZICBOM
> > >  	depends on !XIP_KERNEL && MMU
> > >  	select RISCV_DMA_NONCOHERENT
> > >  	select RISCV_ALTERNATIVE
> > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > index 3fa8ef336822..3607d38edb4f 100644
> > > --- a/arch/riscv/Makefile
> > > +++ b/arch/riscv/Makefile
> > > @@ -57,8 +57,7 @@ toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zi
> > >  riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> > >  
> > >  # Check if the toolchain supports Zicbom extension
> > > -toolchain-supports-zicbom := $(call cc-option-yn, -march=$(riscv-march-y)_zicbom)
> > > -riscv-march-$(toolchain-supports-zicbom) := $(riscv-march-y)_zicbom
> > > +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZICBOM) := $(riscv-march-y)_zicbom
> > >  
> > >  # Check if the toolchain supports Zihintpause extension
> > >  toolchain-supports-zihintpause := $(call cc-option-yn, -march=$(riscv-march-y)_zihintpause)
> > > -- 
> > > 2.37.3
> > > 
> > > 

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Andrew Jones]

On Thu, Oct 06, 2022 at 06:35:19PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Hey,
> This came up due to a report from Kevin @ kernel-ci, who had been
> running a mixed configuration of GNU binutils and clang. Their compiler
> was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
> 
> Our current checks for extension support only cover the compiler, but it
> appears to me that we need to check both the compiler & linker support
> in case of "pot-luck" configurations that mix different versions of
> LD,AS,CC etc.
> 
> Linker support does not seem possible to actually check, since the ISA
> string is emitted into the object files - so I put in version checks for
> that. The checks have gotten a bit ugly since 32 & 64 bit support need
> to be checked independently but ahh well.
> 
> As I was going, I fell into the trap of there being duplicated checks
> for CC support in both the Makefile and Kconfig, so as part of renaming
> the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
> the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
> symbol for Zihintpause appearing in .config.
> 
> I pushed out a version of this that specificly checked for assember
> support for LKP to test & it looked /okay/ - but I did some more testing
> today and realised that this is redudant & have since dropped the as
> check.
> 
> I tested locally with a fair few different combinations, to try and
> cover each of AS, LD, CC missing support for the extension.
> 
> The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
> has:
> 
> > # 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
> 
> I'd like to also move that one to Kconfig rather than the Makefile so
> that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
> appear to be emitted into the object files it's not a fix and can be a
> follow-on patch if my approach here is not entirely off-the-wall.
> 
> I am not 100% on the LD/LLD versions that I picked, I went off of a
> `git branch -a --contains` of the first commits I found that with
> mention of the extension. Please scream if I got it wrong, I'm not
> overly familar with where to find this sort of info for the toolchains.
> 
> Thanks,
> Conor.
> 
> Conor Dooley (2):
>   riscv: fix detection of toolchain Zicbom support
>   riscv: fix detection of toolchain Zihintpause support
> 
>  arch/riscv/Kconfig                      | 17 +++++++++++++----
>  arch/riscv/Makefile                     |  6 ++----
>  arch/riscv/include/asm/vdso/processor.h |  2 +-
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> -- 
> 2.37.3
>

This looks good to me, so

For the series

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

However, we could also drop the compiler and linker checking if we
converted our use of cbo.* to the insn-def.h framework (I think Heiko once
mentioned looking at doing that, but I'm not sure.) I'm looking at adding
Zicboz support right now and for starters I've duplicated and modified
these checks. But, I think I'll look into defining the instruction type
needed for cbo.* and using insn-def instead.

Thanks,
drew

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Conor Dooley]

On Mon, Oct 17, 2022 at 05:51:03PM +0200, Andrew Jones wrote:
> On Thu, Oct 06, 2022 at 06:35:19PM +0100, Conor Dooley wrote:
> 
> However, we could also drop the compiler and linker checking if we
> converted our use of cbo.* to the insn-def.h framework (I think Heiko once
> mentioned looking at doing that, but I'm not sure.) I'm looking at adding
> Zicboz support right now and for starters I've duplicated and modified
> these checks. But, I think I'll look into defining the instruction type
> needed for cbo.* and using insn-def instead.

What is the ETA of your zicboz support? Do you think these patches
should be applied to v6.1 & backported before being replaced by insn-def
when your zicboz support arrives? Or just wait for your zicboz series?

Trying to decide what status I should set for this in patchwork.

Thanks,
Conor.

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Andrew Jones]

On Mon, Oct 17, 2022 at 05:03:48PM +0100, Conor Dooley wrote:
> On Mon, Oct 17, 2022 at 05:51:03PM +0200, Andrew Jones wrote:
> > On Thu, Oct 06, 2022 at 06:35:19PM +0100, Conor Dooley wrote:
> > 
> > However, we could also drop the compiler and linker checking if we
> > converted our use of cbo.* to the insn-def.h framework (I think Heiko once
> > mentioned looking at doing that, but I'm not sure.) I'm looking at adding
> > Zicboz support right now and for starters I've duplicated and modified
> > these checks. But, I think I'll look into defining the instruction type
> > needed for cbo.* and using insn-def instead.
> 
> What is the ETA of your zicboz support? Do you think these patches
> should be applied to v6.1 & backported before being replaced by insn-def
> when your zicboz support arrives? Or just wait for your zicboz series?

I hope to have something posted by the end of this week, so if all things
go well, it could land in 6.1. I think it's reasonable to merge your
patches anyway, though, as they fix the current code and we don't know
what rabbit holes I may fall in with my series yet.

Thanks,
drew

> 
> Trying to decide what status I should set for this in patchwork.
> 
> Thanks,
> Conor.
> 

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Palmer Dabbelt]

On Thu, 06 Oct 2022 10:35:19 PDT (-0700), Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> Hey,
> This came up due to a report from Kevin @ kernel-ci, who had been
> running a mixed configuration of GNU binutils and clang. Their compiler
> was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
>
> Our current checks for extension support only cover the compiler, but it
> appears to me that we need to check both the compiler & linker support
> in case of "pot-luck" configurations that mix different versions of
> LD,AS,CC etc.
>
> Linker support does not seem possible to actually check, since the ISA
> string is emitted into the object files - so I put in version checks for
> that. The checks have gotten a bit ugly since 32 & 64 bit support need
> to be checked independently but ahh well.
>
> As I was going, I fell into the trap of there being duplicated checks
> for CC support in both the Makefile and Kconfig, so as part of renaming
> the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
> the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
> symbol for Zihintpause appearing in .config.
>
> I pushed out a version of this that specificly checked for assember
> support for LKP to test & it looked /okay/ - but I did some more testing
> today and realised that this is redudant & have since dropped the as
> check.
>
> I tested locally with a fair few different combinations, to try and
> cover each of AS, LD, CC missing support for the extension.
>
> The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
> has:
>
>> # 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
>
> I'd like to also move that one to Kconfig rather than the Makefile so
> that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
> appear to be emitted into the object files it's not a fix and can be a
> follow-on patch if my approach here is not entirely off-the-wall.

This is actually a different case: for Zicbom and Zihintpause the 
instructions were added to the toolchain at the same time the extensions 
were, for Zifencei and Zicsr the toolchain had the instructions before 
the extension was defined (as they were in previous base ISAs).  So the 
assembler always supports "fence.i", it's just a question of whether it 
also needs another extension in march.

I'm not opposed to adding a Kconfig for it, but it's a slightly 
different thing and I'm not sure the Kconfig really helps any because 
none of the kernel sources need to understand the "the assembler doesn't 
support the 'fence.i' instruction" case.

> I am not 100% on the LD/LLD versions that I picked, I went off of a
> `git branch -a --contains` of the first commits I found that with
> mention of the extension. Please scream if I got it wrong, I'm not
> overly familar with where to find this sort of info for the toolchains.

This LD version check is for the ISA string mismatches between objects?  
IIRC we stopped failing on those in 2.38, from that point on we just 
guess at a mix and allow linking anyway (largely because of that fence.i 
stuff).

>
> Thanks,
> Conor.
>
> Conor Dooley (2):
>   riscv: fix detection of toolchain Zicbom support
>   riscv: fix detection of toolchain Zihintpause support
>
>  arch/riscv/Kconfig                      | 17 +++++++++++++----
>  arch/riscv/Makefile                     |  6 ++----
>  arch/riscv/include/asm/vdso/processor.h |  2 +-
>  3 files changed, 16 insertions(+), 9 deletions(-)

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Conor Dooley]

On Wed, Oct 26, 2022 at 06:48:32AM -0700, Palmer Dabbelt wrote:
> On Thu, 06 Oct 2022 10:35:19 PDT (-0700), Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Hey,
> > This came up due to a report from Kevin @ kernel-ci, who had been
> > running a mixed configuration of GNU binutils and clang. Their compiler
> > was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
> > 
> > Our current checks for extension support only cover the compiler, but it
> > appears to me that we need to check both the compiler & linker support
> > in case of "pot-luck" configurations that mix different versions of
> > LD,AS,CC etc.
> > 
> > Linker support does not seem possible to actually check, since the ISA
> > string is emitted into the object files - so I put in version checks for
> > that. The checks have gotten a bit ugly since 32 & 64 bit support need
> > to be checked independently but ahh well.
> > 
> > As I was going, I fell into the trap of there being duplicated checks
> > for CC support in both the Makefile and Kconfig, so as part of renaming
> > the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
> > the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
> > symbol for Zihintpause appearing in .config.
> > 
> > I pushed out a version of this that specificly checked for assember
> > support for LKP to test & it looked /okay/ - but I did some more testing
> > today and realised that this is redudant & have since dropped the as
> > check.
> > 
> > I tested locally with a fair few different combinations, to try and
> > cover each of AS, LD, CC missing support for the extension.
> > 
> > The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
> > has:
> > 
> > > # 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
> > 
> > I'd like to also move that one to Kconfig rather than the Makefile so
> > that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
> > appear to be emitted into the object files it's not a fix and can be a
> > follow-on patch if my approach here is not entirely off-the-wall.
> 
> This is actually a different case: for Zicbom and Zihintpause the
> instructions were added to the toolchain at the same time the extensions
> were, for Zifencei and Zicsr the toolchain had the instructions before the
> extension was defined (as they were in previous base ISAs).  So the
> assembler always supports "fence.i", it's just a question of whether it also
> needs another extension in march.

Yeah, that one isn't going to break the build. If the extra bit isn't
emitted then the old linker doesn't care. Was more for whether we wanted
it to be consistent with a symbol for everything that we can check
easily after the fact.

> I'm not opposed to adding a Kconfig for it, but it's a slightly different
> thing and I'm not sure the Kconfig really helps any because none of the
> kernel sources need to understand the "the assembler doesn't support the
> 'fence.i' instruction" case.

tbh beyond the OCD thing about being consistent I don't really care :)

> > I am not 100% on the LD/LLD versions that I picked, I went off of a
> > `git branch -a --contains` of the first commits I found that with
> > mention of the extension. Please scream if I got it wrong, I'm not
> > overly familar with where to find this sort of info for the toolchains.
> 
> This LD version check is for the ISA string mismatches between objects?
> IIRC we stopped failing on those in 2.38, from that point on we just guess
> at a mix and allow linking anyway (largely because of that fence.i stuff).

idk, I wouldn't say "mismatches between objects". The
_zicbom_zihintpause ends up in the object files and the linker doesnt
understand that and aborts. The version checks are so that we don't
emitt that string into object files where our linker doesn't support
them. It only really matters for people that've got some sort of heavily
mixed setup of things - which includes some CIs like Kernel CI.

> > Conor Dooley (2):
> >   riscv: fix detection of toolchain Zicbom support
> >   riscv: fix detection of toolchain Zihintpause support
> > 
> >  arch/riscv/Kconfig                      | 17 +++++++++++++----
> >  arch/riscv/Makefile                     |  6 ++----
> >  arch/riscv/include/asm/vdso/processor.h |  2 +-
> >  3 files changed, 16 insertions(+), 9 deletions(-)

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Palmer Dabbelt]

On Wed, 26 Oct 2022 06:59:55 PDT (-0700), Conor Dooley wrote:
> On Wed, Oct 26, 2022 at 06:48:32AM -0700, Palmer Dabbelt wrote:
>> On Thu, 06 Oct 2022 10:35:19 PDT (-0700), Conor Dooley wrote:
>> > From: Conor Dooley <conor.dooley@microchip.com>
>> >
>> > Hey,
>> > This came up due to a report from Kevin @ kernel-ci, who had been
>> > running a mixed configuration of GNU binutils and clang. Their compiler
>> > was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
>> >
>> > Our current checks for extension support only cover the compiler, but it
>> > appears to me that we need to check both the compiler & linker support
>> > in case of "pot-luck" configurations that mix different versions of
>> > LD,AS,CC etc.
>> >
>> > Linker support does not seem possible to actually check, since the ISA
>> > string is emitted into the object files - so I put in version checks for
>> > that. The checks have gotten a bit ugly since 32 & 64 bit support need
>> > to be checked independently but ahh well.
>> >
>> > As I was going, I fell into the trap of there being duplicated checks
>> > for CC support in both the Makefile and Kconfig, so as part of renaming
>> > the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
>> > the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
>> > symbol for Zihintpause appearing in .config.
>> >
>> > I pushed out a version of this that specificly checked for assember
>> > support for LKP to test & it looked /okay/ - but I did some more testing
>> > today and realised that this is redudant & have since dropped the as
>> > check.
>> >
>> > I tested locally with a fair few different combinations, to try and
>> > cover each of AS, LD, CC missing support for the extension.
>> >
>> > The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
>> > has:
>> >
>> > > # 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
>> >
>> > I'd like to also move that one to Kconfig rather than the Makefile so
>> > that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
>> > appear to be emitted into the object files it's not a fix and can be a
>> > follow-on patch if my approach here is not entirely off-the-wall.
>>
>> This is actually a different case: for Zicbom and Zihintpause the
>> instructions were added to the toolchain at the same time the extensions
>> were, for Zifencei and Zicsr the toolchain had the instructions before the
>> extension was defined (as they were in previous base ISAs).  So the
>> assembler always supports "fence.i", it's just a question of whether it also
>> needs another extension in march.
>
> Yeah, that one isn't going to break the build. If the extra bit isn't
> emitted then the old linker doesn't care. Was more for whether we wanted
> it to be consistent with a symbol for everything that we can check
> easily after the fact.
>
>> I'm not opposed to adding a Kconfig for it, but it's a slightly different
>> thing and I'm not sure the Kconfig really helps any because none of the
>> kernel sources need to understand the "the assembler doesn't support the
>> 'fence.i' instruction" case.
>
> tbh beyond the OCD thing about being consistent I don't really care :)
>
>> > I am not 100% on the LD/LLD versions that I picked, I went off of a
>> > `git branch -a --contains` of the first commits I found that with
>> > mention of the extension. Please scream if I got it wrong, I'm not
>> > overly familar with where to find this sort of info for the toolchains.
>>
>> This LD version check is for the ISA string mismatches between objects?
>> IIRC we stopped failing on those in 2.38, from that point on we just guess
>> at a mix and allow linking anyway (largely because of that fence.i stuff).
>
> idk, I wouldn't say "mismatches between objects". The
> _zicbom_zihintpause ends up in the object files and the linker doesnt
> understand that and aborts. The version checks are so that we don't
> emitt that string into object files where our linker doesn't support
> them. It only really matters for people that've got some sort of heavily
> mixed setup of things - which includes some CIs like Kernel CI.

As of 87fdd7ac09b ("RISC-V: Stop reporting warnings for mismatched 
extension versions"), we should be just ignoring these at link time -- 
unless I mixed something up and we're still getting errors, in which 
case we should fix binutils (and then make the check for whatever 
version that is).

Do you have an example of a link that's failing?  I attempted to 
construct one in the test suite, but not sure if I'm just missing 
something...

>
>> > Conor Dooley (2):
>> >   riscv: fix detection of toolchain Zicbom support
>> >   riscv: fix detection of toolchain Zihintpause support
>> >
>> >  arch/riscv/Kconfig                      | 17 +++++++++++++----
>> >  arch/riscv/Makefile                     |  6 ++----
>> >  arch/riscv/include/asm/vdso/processor.h |  2 +-
>> >  3 files changed, 16 insertions(+), 9 deletions(-)

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Conor Dooley]

On Thu, Oct 27, 2022 at 02:32:16PM -0700, Palmer Dabbelt wrote:
> On Wed, 26 Oct 2022 06:59:55 PDT (-0700), Conor Dooley wrote:
> > On Wed, Oct 26, 2022 at 06:48:32AM -0700, Palmer Dabbelt wrote:
> > > On Thu, 06 Oct 2022 10:35:19 PDT (-0700), Conor Dooley wrote:
> > > > From: Conor Dooley <conor.dooley@microchip.com>
> > > >
> > > > Hey,
> > > > This came up due to a report from Kevin @ kernel-ci, who had been
> > > > running a mixed configuration of GNU binutils and clang. Their compiler
> > > > was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
> > > >
> > > > Our current checks for extension support only cover the compiler, but it
> > > > appears to me that we need to check both the compiler & linker support
> > > > in case of "pot-luck" configurations that mix different versions of
> > > > LD,AS,CC etc.
> > > >
> > > > Linker support does not seem possible to actually check, since the ISA
> > > > string is emitted into the object files - so I put in version checks for
> > > > that. The checks have gotten a bit ugly since 32 & 64 bit support need
> > > > to be checked independently but ahh well.
> > > >
> > > > As I was going, I fell into the trap of there being duplicated checks
> > > > for CC support in both the Makefile and Kconfig, so as part of renaming
> > > > the Kconfig symbol to TOOLCHAIN_HAS_FOO, I dropped the extra checks in
> > > > the Makefile. This has the added advantage of the TOOLCHAIN_HAS_FOO
> > > > symbol for Zihintpause appearing in .config.
> > > >
> > > > I pushed out a version of this that specificly checked for assember
> > > > support for LKP to test & it looked /okay/ - but I did some more testing
> > > > today and realised that this is redudant & have since dropped the as
> > > > check.
> > > >
> > > > I tested locally with a fair few different combinations, to try and
> > > > cover each of AS, LD, CC missing support for the extension.
> > > >
> > > > The one that I am left wondering about is Zicsr/Zifencei. Our Makefile
> > > > has:
> > > >
> > > > > # 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
> > > >
> > > > I'd like to also move that one to Kconfig rather than the Makefile so
> > > > that, again, it shows up in .config etc. But as Zicsr/Zifencei do not
> > > > appear to be emitted into the object files it's not a fix and can be a
> > > > follow-on patch if my approach here is not entirely off-the-wall.
> > > 
> > > This is actually a different case: for Zicbom and Zihintpause the
> > > instructions were added to the toolchain at the same time the extensions
> > > were, for Zifencei and Zicsr the toolchain had the instructions before the
> > > extension was defined (as they were in previous base ISAs).  So the
> > > assembler always supports "fence.i", it's just a question of whether it also
> > > needs another extension in march.
> > 
> > Yeah, that one isn't going to break the build. If the extra bit isn't
> > emitted then the old linker doesn't care. Was more for whether we wanted
> > it to be consistent with a symbol for everything that we can check
> > easily after the fact.
> > 
> > > I'm not opposed to adding a Kconfig for it, but it's a slightly different
> > > thing and I'm not sure the Kconfig really helps any because none of the
> > > kernel sources need to understand the "the assembler doesn't support the
> > > 'fence.i' instruction" case.
> > 
> > tbh beyond the OCD thing about being consistent I don't really care :)
> > 
> > > > I am not 100% on the LD/LLD versions that I picked, I went off of a
> > > > `git branch -a --contains` of the first commits I found that with
> > > > mention of the extension. Please scream if I got it wrong, I'm not
> > > > overly familar with where to find this sort of info for the toolchains.
> > > 
> > > This LD version check is for the ISA string mismatches between objects?
> > > IIRC we stopped failing on those in 2.38, from that point on we just guess
> > > at a mix and allow linking anyway (largely because of that fence.i stuff).
> > 
> > idk, I wouldn't say "mismatches between objects". The
> > _zicbom_zihintpause ends up in the object files and the linker doesnt
> > understand that and aborts. The version checks are so that we don't
> > emitt that string into object files where our linker doesn't support
> > them. It only really matters for people that've got some sort of heavily
> > mixed setup of things - which includes some CIs like Kernel CI.
> 
> As of 87fdd7ac09b ("RISC-V: Stop reporting warnings for mismatched extension
> versions"), we should be just ignoring these at link time -- unless I mixed
> something up and we're still getting errors, in which case we should fix
> binutils (and then make the check for whatever version that is).
> 
> Do you have an example of a link that's failing?  I attempted to construct
> one in the test suite, but not sure if I'm just missing something...

Following some discussion on IRC, settling for 2.38 as the gating
version for Zicbom as logic was added there to stop erroring at all for
unrecognised isa strings in objects as per Palmer's suggestion.

Tested 2.38 w/:
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=120100
CONFIG_CLANG_VERSION=0
CONFIG_AS_IS_GNU=y
CONFIG_AS_VERSION=23900
CONFIG_LD_IS_BFD=y
CONFIG_LD_VERSION=23800
CONFIG_LLD_VERSION=0

and it does link properly. Folding in a 23800 instead of 23900 seems
fair enough to me.

Thanks,
Conor.

Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection

[Palmer Dabbelt]

On Thu, 6 Oct 2022 18:35:19 +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> Hey,
> This came up due to a report from Kevin @ kernel-ci, who had been
> running a mixed configuration of GNU binutils and clang. Their compiler
> was relatively recent & supports Zicbom but binutils @ 2.35.2 did not.
> 
> [...]

Applied, thanks!

[1/2] riscv: fix detection of toolchain Zicbom support
      https://git.kernel.org/palmer/c/b8c86872d1dc
[2/2] riscv: fix detection of toolchain Zihintpause support
      https://git.kernel.org/palmer/c/aae538cd03bc

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>