Message-ID: <CAEg0e7iPGnaMKR9bT01-bGOUUToUG0o3Q7NBS_m8CbZdYWeBXA@mail.gmail.com>
Date:   Thu, 23 Nov 2023 01:36:34 +0100
From:   Christoph Müllner <christoph.muellner@...ll.eu>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Jerry Shih <jerry.shih@...ive.com>,
        Charlie Jenkins <charlie@...osinc.com>,
        Heiko Stuebner <heiko@...ech.de>, palmer@...belt.com,
        paul.walmsley@...ive.com, aou@...s.berkeley.edu,
        herbert@...dor.apana.org.au, davem@...emloft.net,
        conor.dooley@...rochip.com, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        Heiko Stuebner <heiko.stuebner@...ll.eu>
Subject: Re: [PATCH v4 00/12] RISC-V: support some cryptography accelerations

On Thu, Nov 23, 2023 at 12:43 AM Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Wed, Nov 22, 2023 at 03:58:17PM +0800, Jerry Shih wrote:
> > On Nov 22, 2023, at 07:51, Eric Biggers <ebiggers@...nel.org> wrote:
> > > On Wed, Nov 01, 2023 at 09:03:33PM -0700, Eric Biggers wrote:
> > >>
> > >> It would be nice to use a real assembler, so that people won't have to worry
> > >> about potential mistakes or inconsistencies in the perl-based "assembler".  Also
> > >> keep in mind that if we allow people to compile this code without the real
> > >> assembler support from the beginning, it might end up staying that way for quite
> > >> a while in order to avoid breaking the build for people.
> > >>
> > >> Ultimately it's up to you though; I think that you and others who have been
> > >> working on RISC-V crypto can make the best decision about what to do here.  I
> > >> also don't want this patchset to be delayed waiting for other projects, so maybe
> > >> that indeed means the perl-based "assembler" needs to be used for now.
> > >
> > > Just wanted to bump up this discussion again.  In binutils, the vector crypto
> > > v1.0.0 support was released 4 months ago in 2.41.  See the NEWS file at
> > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob_plain;f=binutils/NEWS;hb=refs/heads/binutils-2_41-branch
> > >
> > >    * The RISC-V port now supports the following new standard extensions:
> > >      - Zicond (conditional zero instructions)
> > >      - Zfa (additional floating-point instructions)
> > >      - Zvbb, Zvbc, Zvkg, Zvkned, Zvknh[ab], Zvksed, Zvksh, Zvkn, Zvknc, Zvkng,
> > >        Zvks, Zvksc, Zvkg, Zvkt (vector crypto instructions)
> > >
> > > That's every extension listed in the vector crypto v1.0.0 specification
> > > (https://github.com/riscv/riscv-crypto/releases/download/v1.0.0/riscv-crypto-spec-vector.pdf).
> >
> > It doesn't fit all v1.0 spec.
> > The `Zvkb` is missed in binutils. It's the subset of `Zvbb`. We needs some extra
> > works if user just try to use `Zvkb`.
> > https://github.com/riscv/riscv-crypto/blob/main/doc/vector/riscv-crypto-vector-zvkb.adoc
> > Some crypto algorithms are already checking for `Zvkb` instead of `Zvbb`.
>
> Yeah, that's unfortunate that Zvkb got missed in binutils.  However, since all
> Zvkb instructions are part of Zvbb, which is supported, assembling Zvkb
> instructions should still work --- right?

Not forgotten, but the Zvkb extension did not exist when the patchset
was merged.
RISC-V extension support is typically merged when specifications are "frozen".
This means a high bar for changes, but they are possible until the
spec is ratified.
Often nothing is changed until ratification, but here Zvkb has been
(re-)introduced.

I was not aware of this untils I read this email, so I just wrote a
patch that fills the gap:
  https://sourceware.org/pipermail/binutils/2023-November/130762.html

Thanks for reporting!

BR
Christoph

>
> > > LLVM still has the vector crypto extensions marked as "experimental" extensions.
> > > However, there is an open pull request to mark them non-experimental:
> > > https://github.com/llvm/llvm-project/pull/69000
> > >
> > > Could we just go ahead and remove riscv.pm, develop with binutils for now, and
> > > prioritize getting the LLVM support finished?
> >
> > Yes, we could.
> > But we need to handle the extensions checking for toolchains like:
> > https://github.com/torvalds/linux/commit/b6fcdb191e36f82336f9b5e126d51c02e7323480
> > I could do that, but I think I need some times to test the builds. And it will introduce
> > more dependency patch which is not related to actual crypto algorithms and the
> > gluing code in kernel. I will send another patch for toolchain part after the v2 patch.
> > And I am working for v2 patch with your new review comments. The v2 will still
> > use `perlasm`.
>
> Note that perlasm (.pl) vs assembly (.S), and ".inst" vs real assembler
> instructions, are actually separate concerns.  We could use real assembler
> instructions while still using perlasm.  Or we could use assembly while still
> using macros that generate the instructions as .inst.
>
> My preference is indeed both: assembly (.S) with real assembler instructions.
> That would keep things more straightforward.
>
> We do not necessarily need to do both before merging the code, though.  It will
> be beneficial to get this code merged sooner rather than later, so that other
> people can work on improving it.
>
> I would prioritize the change to use real assembler instructions.  I do think
> it's worth thinking about getting that change in from the beginning, so that the
> toolchain prerequisites are properly in place from the beginning and people can
> properly account for them and prioritize the toolchain work as needed.
>
> - Eric

Powered by blists - more mailing lists