Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Oct 2022 06:48:32 -0700 (PDT)
Subject:     Re: [PATCH 0/2] (attempt to) Fix RISC-V toolchain extension support detection
In-Reply-To: <20221006173520.1785507-1-conor@kernel.org>
CC:     nathan@kernel.org, ndesaulniers@google.com, trix@redhat.com,
        conor.dooley@microchip.com, daolu@rivosinc.com, heiko@sntech.de,
        guoren@kernel.org, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     Conor Dooley <conor@kernel.org>
Message-ID: <mhng-eb49b867-c5d3-45f9-904c-1726b1ce6158@palmer-ri-x1c9>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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