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@xxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> 
> [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
> > 
> >