On Wed, Nov 01, 2023 at 09:03:33PM -0700, Eric Biggers wrote:
> >
> > There is no public assembler supports the vector-crypto asm mnemonics.
> > We should still use `opcode` for vector-crypto instructions. But we might
> > use asm for standard rvv parts.
> > In order to reuse the codes in OpenSSL as much as possible, we still use
> > the `riscv.pm` for all standard rvv and vector-crypto instructions. If the asm
> > mnemonic is still a better approach, I will `rewrite` all standard rvv parts
> > with asm mnemonics in next patch.
>
> Tip-of-tree gcc + binutils seems to support them. Building some of the sample
> code from the riscv-crypto repository:
>
> $ riscv64-linux-gnu-as --version
> GNU assembler (GNU Binutils) 2.41.50.20231021
> $ riscv64-linux-gnu-gcc --version
> riscv64-linux-gnu-gcc (GCC) 14.0.0 20231021 (experimental)
> $ riscv64-linux-gnu-gcc -march=rv64ivzvkned -c riscv-crypto/doc/vector/code-samples/zvkned.s
>
> And tip-of-tree clang supports them experimentally:
>
> $ clang --version
> clang version 18.0.0 (https://github.com/llvm/llvm-project 30416f39be326b403e19f23da387009736483119)
> $ clang -menable-experimental-extensions -target riscv64-linux-gnu -march=rv64ivzvkned1 -c riscv-crypto/doc/vector/code-samples/zvkned.s
>
> 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).
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?
- Eric
On Nov 22, 2023, at 07:51, Eric Biggers <[email protected]> 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`.
> 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`.
And we could move this discussion to this thread.
https://lore.kernel.org/all/[email protected]/
-Jerry
On Wed, Nov 22, 2023 at 03:58:17PM +0800, Jerry Shih wrote:
> On Nov 22, 2023, at 07:51, Eric Biggers <[email protected]> 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?
> > 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