| Message ID | 20221006173520.1785507-1-conor@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | (attempt to) Fix RISC-V toolchain extension support detection | expand |
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
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.
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. >
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(-)
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(-)
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(-)
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.
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,