[PATCH v2] RISC-V: remove I-extension ISA spec version dance

Sat Mar 11 02:40:49 PST 2023

On Sat, Mar 11, 2023 at 11:11:57AM +0100, Aurelien Jarno wrote:
> On 2023-03-10 16:40, Conor Dooley wrote:
> > On Fri, Mar 10, 2023 at 11:35:57PM +0800, Bin Meng wrote:
> > > On Fri, Mar 10, 2023 at 11:07 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > > From: Conor Dooley <conor.dooley at microchip.com>
> > > > >
> > > > > The spec folk, in their infinite wisdom, moved both control and status
> > > > > registers & the FENCE.I instructions out of the I extension into their
> > > > > own extensions (Zicsr, Zifencei) in the 20190608 version of the ISA
> > > > > spec [0].
> > > > > The GCC/binutils crew decided [1] to move their default version of the
> > > > > ISA spec to the 20191213 version of the ISA spec, which came into being
> > > > > for version 2.38 of binutils and GCC 12. Building with this toolchain
> > > > > configuration would result in assembler issues:
> > > > >   CC      arch/riscv/kernel/vdso/vgettimeofday.o
> > > > >   <<BUILDDIR>>/arch/riscv/include/asm/vdso/gettimeofday.h: Assembler messages:
> > > > >   <<BUILDDIR>>/arch/riscv/include/asm/vdso/gettimeofday.h:71: Error: unrecognized opcode `csrr a5,0xc01'
> > > > >   <<BUILDDIR>>/arch/riscv/include/asm/vdso/gettimeofday.h:71: Error: unrecognized opcode `csrr a5,0xc01'
> > > > >   <<BUILDDIR>>/arch/riscv/include/asm/vdso/gettimeofday.h:71: Error: unrecognized opcode `csrr a5,0xc01'
> > > > >   <<BUILDDIR>>/arch/riscv/include/asm/vdso/gettimeofday.h:71: Error: unrecognized opcode `csrr a5,0xc01'
> > > > > This was fixed in commit 6df2a016c0c8 ("riscv: fix build with binutils
> > > > > 2.38") by Aurelien Jarno, but has proven fragile.
> > > > >
> > > > > Before LLVM 17, LLVM did not support these extensions and, as such, the
> > > > > cc-option check added by Aurelien worked. Since commit 22e199e6afb1
> > > > > ("[RISCV] Accept zicsr and zifencei command line options") however, LLVM
> > > > > *does* support them and the cc-option check passes.
> > > > >
> > > > > This surfaced as a problem while building the 5.10 stable kernel using
> > > > > the default Tuxmake Debian image [2], as 5.10 did not yet support ld.lld,
> > > > > and uses the Debian provided binutils 2.35.
> > > > > Versions of ld prior to 2.38 will refuse to link if they encounter
> > > > > unknown ISA extensions, and unfortunately Zifencei is not supported by
> > > > > bintuils 2.35.
> > > > >
> > > > > Instead of dancing around with adding these extensions to march, as we
> > > > > currently do, Palmer suggested locking GCC builds to the same version of
> > > > > the ISA spec that is used by LLVM. As far as I can tell, that is 2.2,
> > > > > with, apparently [3], a lack of interest in implementing a flag like
> > > > > GCC's -misa-spec at present.
> > > > >
> > > > > Add {cc,as}-option checks to add -misa-spec to KBUILD_{A,C}FLAGS when
> > > > > GCC is used & remove the march dance.
> > > > >
> > > > > As clang does not accept this argument, I had expected to encounter
> > > > > issues with the assembler, as neither zicsr nor zifencei are present in
> > > > > the ISA string and the spec version *should* be defaulting to a version
> > > > > that requires them to be present. The build passed however and the
> > > > > resulting kernel worked perfectly fine for me on a PolarFire SoC...
> > > > >
> > > > > Link: https://riscv.org/wp-content/uploads/2019/06/riscv-spec.pdf [0]
> > > > > Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aE1ZeHHCYf4 [1]
> > > > > Link: https://lore.kernel.org/all/CA+G9fYt9T=ELCLaB9byxaLW2Qf4pZcDO=huCA0D8ug2V2+irJQ@mail.gmail.com/ [2]
> > > > > Link: https://discourse.llvm.org/t/specifying-unpriviledge-spec-version-misa-spec-gcc-flag-equivalent/66935 [3]
> > > > > CC: stable at vger.kernel.org
> > > > > Suggested-by: Palmer Dabbelt <palmer at rivosinc.com>
> > > > > Reported-by: Naresh Kamboju <naresh.kamboju at linaro.org>
> > > > > Signed-off-by: Conor Dooley <conor.dooley at microchip.com>
> > > > > ---
> > > > > I think Aurelien's original commit message might actually not be quite
> > > > > correct? I found, in my limited testing, that it is not the default
> > > > > behaviour of gas that matters, but rather the toolchain itself?
> > > > > My binutils versions (both those built using the clang-built-linux
> > > > > tc-build scripts which do not set an ISA spec version, and one built
> > > > > using the riscv-gnu-toolchain infra w/ an explicit 20191213 spec version
> > > > > set) do not encounter these issues.
> > > >
> > > > I am unable to reproduce the build failure as reported by commit 6df2a016c0c8
> > > > ("riscv: fix build with binutils 2.38") by Aurelien Jarno using kernel.org
> > > > pre-built GCC 11.3.0 [1] which includes binutils 2.38.
> > > >
> > > > [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.3.0/x86_64-gcc-11.3.0-nolibc-x86_64-linux.tar.xz
> > > >
> > > > The defconfig of v5.16 kernel (commit 6df2a016c0c8 lands in v5.17) builds fine
> > > > for me. Anything I am missing?
> > > >
> > > 
> > > Some further note:
> > > 
> > > After I switched to kernel.org pre-built GCC 12.2.0 [1] which includes
> > > binutils 2.39, I was able to reproduce the exact same build failure of
> > > v5.16 kernel as described in the commit 6df2a016c0c8 ("riscv: fix
> > > build with binutils 2.38") by Aurelien Jarno.
> > > 
> > > To verify the commit message of 6df2a016c0c8 is accurate or not, I
> > > built a GAS from binutils 2.37 and replaced the GAS 2.39 in the
> > > kernel.org package, surprisingly kernel v5.16 did not build with the
> > > same build failure.
> > > 
> > > So it seems that it's GCC that caused the build failure instead of GAS
> > > from binutils??
> > 
> > Right, that's what I was getting at in the bit below the --- line in my
> > patch. I think Aurelien was misled by the failure message and your email
> > ([1] in my links above) which claimed that binutils would default to
> > the 20191213 spec.
> 
> No I was not misled by that, at that time GCC 11.3 and GCC 12 were not
> released.
> 
> binutils definitely defaults to 20191213 if you do not use the
> --with-isa-spec with a different value when configuring it.

I specifically built binutils without that flag, and had no issues
building the kernel with clang, which is the source of my confusion here
really.
I'd have expected that to fail

> > It appears (and I'm not a tc person) that GCC must call GAS with the
> > --misa-spec argument, and in GCC 12 the value used is 20191213.
> > Either GCC 11 must pass --misa-spec=2.2 to binutils, or it passes
> > nothing, leading binutils to be permissive about what -march=rv64i
> > means.
> 
> GCC 11.1 and 11.2 pass nothing to binutils, hence the issue I reported
> and the patch fixing that. Basic support for misa-spec has been
> backported to GCC 11.3, with a default to --misa-spec=2.2, hence the
> "permissive" behaviour you observe.
> 
> > The permissive option would "seem" to be correct, as building with clang
> > (that to my knowledge doesn't pass --misa-spec to GAS) and with
> > -march=rv64i has no issues assembling.
> 
> The "permissive" way only work if we drop support for GCC 11.1 and 11.2,
> which sounds acceptable to me.

I don't think that we can do that.

> > It'd appear to me that binutils is a *player* in this issue, but is not
> > the culprit of the issue Aurelien sought to fix.
> 
> I *disagree*. At the time the commit has been merged, binutils was the
> only culprit.

Okay, fair enough! I was only going off my observations with
current-day, and clearly you remember well the situation that you
encountered this with!

> With more recent versions of GCC 11.3 and GCC 12 released
> in the meantime, the situation is way more complex.

This patch is going to be problematic w/ versions of gcc that do not
understand -misa-spec + binutils 2.38 then, isn't it.
Based on what Jess has said, pursuing this approach seems ill-advised
anyway.

I'm inclined to go back to my approach in v1 & only apply your march
addition when AS_IS_GNU. That still leaves a problem for a configuration
where GAS is 2.38 and LD is 2.35, which I don't think can be handled.
I feel the only option there is just erroring the build if someones
tries it.
Ditto for a future IAS + LD 2.35 combination, if IAS is using a 20190608
or later of the ISA spec.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230311/36bb4a59/attachment.sig>