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

Sat Mar 11 05:09:07 PST 2023

On Sat, Mar 11, 2023 at 6:49 PM Aurelien Jarno <aurelien at aurel32.net> wrote:
>
> On 2023-03-11 13:41, Bin Meng wrote:
> > On Sat, Mar 11, 2023 at 12:40 AM Conor Dooley <conor at kernel.org> 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.
> > > 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.
> >
> > I verified that "-misa-spec" is a new option introduced in GCC 12 and
> > the default value is set to 20191213 which is unfortunately backward
> > incompatible.
> >
> > GCC 11 does not have the "-misa-spec" option, so I assume it produces
> > backward compatible codes.
> >
> > IOW, what commit 6df2a016c0c8 was trying to fix has nothing to do with
> > binutils 2.38+. It's the GCC changes that is the culprit.
>
> I disagree with that statement. Your tests seems to have been done using
> the toolchains from [1], which changes two parameters, i.e. both the GCC
> and binutils version.
>
> The original issue can be demonstrated the following way:
>
> - Revert commit 6df2a016c0c8
> - Get toolchain from:
>   https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/x86_64-gcc-11.1.0-nolibc-riscv64-linux.tar.xz
> - Get toolchain from:
>   https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.3.0/x86_64-gcc-11.3.0-nolibc-riscv64-linux.tar.xz
> - Replace the as version in the 11.1.0 toolchain (2.36.1) with the one
>   from the 1.3.0 one (2.38), for instance using:
>   ln -sf ../../../../gcc-11.3.0-nolibc/riscv64-linux/riscv64-linux/bin/as gcc-11.1.0-nolibc/riscv64-linux/riscv64-linux/bin/as
> - Build a defconfig kernel using the 11.1.0 toolchain, so using GCC 11.1
>   with binutils 2.38
>
> [1] https://mirrors.edge.kernel.org/pub/tools/crosstool/
>

Okay, thanks for sharing the info of binutils's configuration of "-misa-spec".

Indeed I checked the "-misa-spec" default value was changed from 2.2
to 20191213 in binutils 2.38 via the following commit:

commit aed44286efa8ae8717a77d94b51ac3614e2ca6dc
Author: Nelson Chu <nelson.chu at sifive.com>
Date:   Thu Dec 30 23:23:46 2021 +0800

    RISC-V: Updated the default ISA spec to 20191213.

    Update the default ISA spec from 2.2 to 20191213 will change the default
    version of i from 2.0 to 2.1.  Since zicsr and zifencei are separated
    from i 2.1, users need to add them in the architecture string if they need
    fence.i and csr instructions.  Besides, we also allow old ISA spec can
    recognize zicsr and zifencei, but we won't output them since they are
    already included in the i extension when i's version is less than 2.1.

So the original commit message of 6df2a016c0c8 was correct at the time
of writing.

As you mentioned, now GCC also brings the "-misa-spec" starting from
GCC 12. This makes things complicated ...

Regards,
Bin