Re: [PATCH 00/45] Add RISC-V vector cryptographic instruction set support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 21/03/2023 12:02, Christoph Müllner wrote:
On Fri, Mar 10, 2023 at 10:16 AM Lawrence Hunter
<lawrence.hunter@xxxxxxxxxxxxxxx> wrote:

This patchset provides an implementation for Zvkb, Zvkned, Zvknh, Zvksh, Zvkg, and Zvksed of the draft RISC-V vector cryptography extensions as per the 20230303 version of the specification(1) (1fcbb30). Please note that the Zvkt data-independent execution latency extension has not been implemented, and we would recommend not using these patches in an environment where timing attacks are an issue.

Work performed by Dickon, Lawrence, Nazar, Kiran, and William from Codethink sponsored by SiFive, as well as Max Chou and Frank Chang from SiFive.

For convenience we have created a git repo with our patches on top of a recent master. https://github.com/CodethinkLabs/qemu-ct

I did test and review this patchset.
Since most of my comments affect multiple patches I have summarized
them here in one email.
Observations that only affect a single patch will be sent in response
to the corresponding email.

I have tested this series with the OpenSSL PR for Zvk that can be found here:
   https://github.com/openssl/openssl/pull/20149
I ran with all Zvk* extensions enabled (using Zvkg for GCM) and with
Zvkb only (using Zvkb for GCM).
All tests succeed. Note, however, that the test coverage is limited
(e.g. no .vv instructions, vstart is always zero).

When sending out a follow-up version (even if it just introduces a minimal fix), then consider using patchset versioning (e.g. git format-patch -v2 ...).

Ok, will do

It might be a matter of taste, but I would prefer a series that groups
and orders the commits differently:
   a) independent changes to the existing code (refactoring only, but
no new features) - one commit per topic
   b) introduction of new functionality - one commit per extension
A series using such a commit granularity and order would be easier to
maintain and review (and not result in 45 patches).
Also, the refactoring changes could land before Zvk freezes if
maintainers decide to do so.

Makes sense, will do

So far all translation files in target/riscv/insn_trans/* contain
multiple extensions if they are related.
I think we should follow this pattern and use a common trans_zvk.c.inc file.

Agree, will do

All patches to insn32.decode have comments of the form "RV64 Zvk*
vector crypto extension".
What is the point of the "RV64"? I would simply remove that.

Ok, will remove it

All instructions set "env->vstart = 0;" at the end.
I don't think that this is correct (the specification does not require this).

That's from vector spec: "All vector instructions are defined to begin execution with the element number given in the vstart CSR, leaving earlier elements in the destination vector undisturbed, and to reset the vstart CSR to zero at the end of execution." - from "3.7. Vector Start Index CSR vstart"

The tests of the reserved encodings are not consistent:
* Zvknh does a dynamic test (query tcg_gen_*())
* Zvkned does a dynamic test (tcg_gen_*())
* Zvkg does not test for (vl%EGS == 0)

Zvkg also does dynamic test, by calling macros GEN_V_UNMASKED_TRANS and GEN_VV_UNMASKED_TRANS

The vl CSR can only be updated by the vset{i}vl{i} instructions.
The same applies to the vstart CSR and the vtype CSR that holds vsew,
vlmul and other fields.
The current code tests the VSTART/SEW value using "s->vstart % 4 ==
0"/"s->sew == MO_32".
Why is it not possible to do the same with VL, i.e. "s->vl % 4 == 0"
(after adding it to DisasContext)?

vl can also be updated by another instruction - from vector spec "3.5. Vector Length Register vl" - "The XLEN-bit-wide read-only vl CSR can only be updated by the vset{i}vl{i} instructions, and the fault-only-first vector load instruction variants." So just because of that fault-only-first instruction we need dynamic checks.

vstart is just another CSR -- software can write to it, but probably shouldn't. Whether that's ever going to be useful outside testing ISA conformance tests or not I don't know, but it's clearly read-write (also section 3.7).

Also, I would introduce named constants or macros for the EGS values
to avoid magic constants in the code
(some extensions do that - e.g. ZVKSED_EGS).

Makes sense, will do

Best,
Lawrence



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux