2022-10-06 17:47:39

by Conor Dooley

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

From: Conor Dooley <[email protected]>

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 <[email protected]>
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 <[email protected]>
---
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

2022-10-06 18:32:13

by Heiko Stuebner

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

Hi Conor,

Am Donnerstag, 6. Oktober 2022, 19:35:20 CEST schrieb Conor Dooley:
> From: Conor Dooley <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>

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

2022-10-13 20:24:31

by Nathan Chancellor

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

On Thu, Oct 06, 2022 at 06:35:20PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>

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 <[email protected]>

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

2022-10-13 21:10:05

by Nathan Chancellor

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

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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > 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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > [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
> > >
> > >

2022-10-13 21:34:34

by Conor Dooley

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

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 <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
>
> 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 <[email protected]>
>
> [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
> >
> >