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

Message ID	20230310091215.931644-1-lawrence.hunter@codethink.co.uk (mailing list archive)
Headers	show Return-Path: <kvm-owner@vger.kernel.org> From: Lawrence Hunter <lawrence.hunter@codethink.co.uk> To: qemu-devel@nongnu.org Cc: dickon.hood@codethink.co.uk, nazar.kazakov@codethink.co.uk, kiran.ostrolenk@codethink.co.uk, frank.chang@sifive.com, palmer@dabbelt.com, alistair.francis@wdc.com, bin.meng@windriver.com, pbonzini@redhat.com, philipp.tomsich@vrull.eu, kvm@vger.kernel.org, Lawrence Hunter <lawrence.hunter@codethink.co.uk> Subject: [PATCH 00/45] Add RISC-V vector cryptographic instruction set support Date: Fri, 10 Mar 2023 09:11:30 +0000 Message-Id: <20230310091215.931644-1-lawrence.hunter@codethink.co.uk> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Add RISC-V vector cryptographic instruction set support \| expand [00/45] Add RISC-V vector cryptographic instruction set support [01/45] target/riscv: Add zvkb cpu property [02/45] target/riscv: Refactor some of the generic vector functionality [03/45] target/riscv: Add vclmul.vv decoding, translation and execution support [04/45] target/riscv: Refactor some of the generic vector functionality [05/45] target/riscv: Add vclmul.vx decoding, translation and execution support [06/45] target/riscv: Add vclmulh.vv decoding, translation and execution support [07/45] target/riscv: Add vclmulh.vx decoding, translation and execution support [08/45] target/riscv: Refactor some of the generic vector functionality [09/45] qemu/bitops.h: Limit rotate amounts [10/45] target/riscv: Add vrol.[vv,vx] and vror.[vv,vx,vi] decoding, translation and execution supp… [11/45] target/riscv: Refactor some of the generic vector functionality [12/45] target/riscv: Add vbrev8.v decoding, translation and execution support [13/45] target/riscv: Add vrev8.v decoding, translation and execution support [14/45] target/riscv: Add vandn.[vv,vx] decoding, translation and execution support [15/45] target/riscv: Expose zvkb cpu property [16/45] target/riscv: Add zvkned cpu property [17/45] target/riscv: Add vaesef.vv decoding, translation and execution support [18/45] target/riscv: Add vaesef.vs decoding, translation and execution support [19/45] target/riscv: Add vaesdf.vv decoding, translation and execution support [20/45] target/riscv: Add vaesdf.vs decoding, translation and execution support [21/45] target/riscv: Add vaesdm.vv decoding, translation and execution support [22/45] target/riscv: Add vaesdm.vs decoding, translation and execution support [23/45] target/riscv: Add vaesz.vs decoding, translation and execution support [24/45] target/riscv: Add vaesem.vv decoding, translation and execution support [25/45] target/riscv: Add vaesem.vs decoding, translation and execution support [26/45] target/riscv: Add vaeskf1.vi decoding, translation and execution support [27/45] target/riscv: Add vaeskf2.vi decoding, translation and execution support [28/45] target/riscv: Expose zvkned cpu property [29/45] target/riscv: Add zvknh cpu properties [30/45] target/riscv: Add vsha2ms.vv decoding, translation and execution support [31/45] target/riscv: Add vsha2c[hl].vv decoding, translation and execution support [32/45] target/riscv: Expose zvknh cpu properties [33/45] target/riscv: Add zvksh cpu property [34/45] target/riscv: Add vsm3me.vv decoding, translation and execution support [35/45] target/riscv: Add vsm3c.vi decoding, translation and execution support [36/45] target/riscv: Expose zvksh cpu property [37/45] target/riscv: Add zvkg cpu property [38/45] target/riscv: Add vgmul.vv decoding, translation and execution support [39/45] target/riscv: Add vghsh.vv decoding, translation and execution support [40/45] target/riscv: Expose zvkg cpu property [41/45] crypto: Create sm4_subword [42/45] crypto: Add SM4 constant parameter CK [43/45] target/riscv: Add zvksed cfg property [44/45] target/riscv: Add Zvksed support [45/45] target/riscv: Expose Zvksed property

Message ID

20230310091215.931644-1-lawrence.hunter@codethink.co.uk (mailing list archive)

Headers

From: Lawrence Hunter <lawrence.hunter@codethink.co.uk>
To: qemu-devel@nongnu.org
Cc: dickon.hood@codethink.co.uk, nazar.kazakov@codethink.co.uk,
        kiran.ostrolenk@codethink.co.uk, frank.chang@sifive.com,
        palmer@dabbelt.com, alistair.francis@wdc.com,
        bin.meng@windriver.com, pbonzini@redhat.com,
        philipp.tomsich@vrull.eu, kvm@vger.kernel.org,
        Lawrence Hunter <lawrence.hunter@codethink.co.uk>
Subject: [PATCH 00/45] Add RISC-V vector cryptographic instruction set support
Date: Fri, 10 Mar 2023 09:11:30 +0000
Message-Id: <20230310091215.931644-1-lawrence.hunter@codethink.co.uk>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

Add RISC-V vector cryptographic instruction set support | expand

Message

Lawrence Hunter March 10, 2023, 9:11 a.m. UTC

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

1. https://github.com/riscv/riscv-crypto/releases


Dickon Hood (2):
  qemu/bitops.h: Limit rotate amounts
  target/riscv: Add vrol.[vv,vx] and vror.[vv,vx,vi] decoding,
    translation and execution support

Kiran Ostrolenk (8):
  target/riscv: Refactor some of the generic vector functionality
  target/riscv: Refactor some of the generic vector functionality
  target/riscv: Refactor some of the generic vector functionality
  target/riscv: Refactor some of the generic vector functionality
  target/riscv: Add vsha2ms.vv decoding, translation and execution
    support
  target/riscv: Add zvksh cpu property
  target/riscv: Add vsm3c.vi decoding, translation and execution support
  target/riscv: Expose zvksh cpu property

Lawrence Hunter (17):
  target/riscv: Add vclmul.vv decoding, translation and execution
    support
  target/riscv: Add vclmul.vx decoding, translation and execution
    support
  target/riscv: Add vclmulh.vv decoding, translation and execution
    support
  target/riscv: Add vclmulh.vx decoding, translation and execution
    support
  target/riscv: Add vaesef.vv decoding, translation and execution
    support
  target/riscv: Add vaesef.vs decoding, translation and execution
    support
  target/riscv: Add vaesdf.vv decoding, translation and execution
    support
  target/riscv: Add vaesdf.vs decoding, translation and execution
    support
  target/riscv: Add vaesdm.vv decoding, translation and execution
    support
  target/riscv: Add vaesdm.vs decoding, translation and execution
    support
  target/riscv: Add vaesz.vs decoding, translation and execution support
  target/riscv: Add vsha2c[hl].vv decoding, translation and execution
    support
  target/riscv: Add vsm3me.vv decoding, translation and execution
    support
  target/riscv: Add zvkg cpu property
  target/riscv: Add vgmul.vv decoding, translation and execution support
  target/riscv: Add vghsh.vv decoding, translation and execution support
  target/riscv: Expose zvkg cpu property

Max Chou (5):
  crypto: Create sm4_subword
  crypto: Add SM4 constant parameter CK
  target/riscv: Add zvksed cfg property
  target/riscv: Add Zvksed support
  target/riscv: Expose Zvksed property

Nazar Kazakov (10):
  target/riscv: Add zvkb cpu property
  target/riscv: Add vrev8.v decoding, translation and execution support
  target/riscv: Add vandn.[vv,vx] decoding, translation and execution
    support
  target/riscv: Expose zvkb cpu property
  target/riscv: Add zvkned cpu property
  target/riscv: Add vaeskf1.vi decoding, translation and execution
    support
  target/riscv: Add vaeskf2.vi decoding, translation and execution
    support
  target/riscv: Expose zvkned cpu property
  target/riscv: Add zvknh cpu properties
  target/riscv: Expose zvknh cpu properties

William Salmon (3):
  target/riscv: Add vbrev8.v decoding, translation and execution support
  target/riscv: Add vaesem.vv decoding, translation and execution
    support
  target/riscv: Add vaesem.vs decoding, translation and execution
    support

 accel/tcg/tcg-runtime-gvec.c                 |   11 +
 accel/tcg/tcg-runtime.h                      |    1 +
 crypto/sm4.c                                 |   10 +
 include/crypto/sm4.h                         |    9 +
 include/qemu/bitops.h                        |   24 +-
 target/arm/tcg/crypto_helper.c               |   10 +-
 target/riscv/cpu.c                           |   36 +
 target/riscv/cpu.h                           |    7 +
 target/riscv/helper.h                        |   71 ++
 target/riscv/insn32.decode                   |   49 +
 target/riscv/insn_trans/trans_rvv.c.inc      |   93 +-
 target/riscv/insn_trans/trans_rvzvkb.c.inc   |  220 ++++
 target/riscv/insn_trans/trans_rvzvkg.c.inc   |   40 +
 target/riscv/insn_trans/trans_rvzvkned.c.inc |  170 +++
 target/riscv/insn_trans/trans_rvzvknh.c.inc  |   84 ++
 target/riscv/insn_trans/trans_rvzvksed.c.inc |   57 +
 target/riscv/insn_trans/trans_rvzvksh.c.inc  |   43 +
 target/riscv/meson.build                     |    4 +-
 target/riscv/op_helper.c                     |    5 +
 target/riscv/translate.c                     |    6 +
 target/riscv/vcrypto_helper.c                | 1001 ++++++++++++++++++
 target/riscv/vector_helper.c                 |  240 +----
 target/riscv/vector_internals.c              |   81 ++
 target/riscv/vector_internals.h              |  222 ++++
 24 files changed, 2192 insertions(+), 302 deletions(-)
 create mode 100644 target/riscv/insn_trans/trans_rvzvkb.c.inc
 create mode 100644 target/riscv/insn_trans/trans_rvzvkg.c.inc
 create mode 100644 target/riscv/insn_trans/trans_rvzvkned.c.inc
 create mode 100644 target/riscv/insn_trans/trans_rvzvknh.c.inc
 create mode 100644 target/riscv/insn_trans/trans_rvzvksed.c.inc
 create mode 100644 target/riscv/insn_trans/trans_rvzvksh.c.inc
 create mode 100644 target/riscv/vcrypto_helper.c
 create mode 100644 target/riscv/vector_internals.c
 create mode 100644 target/riscv/vector_internals.h

Comments

Christoph Müllner March 21, 2023, 12:02 p.m. UTC | #1

On Fri, Mar 10, 2023 at 10:16 AM Lawrence Hunter
<lawrence.hunter@codethink.co.uk> 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 ...).

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.

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.

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.

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

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)
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)?
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).

BR
Christoph


>
> 1. https://github.com/riscv/riscv-crypto/releases
>
>
> Dickon Hood (2):
>   qemu/bitops.h: Limit rotate amounts
>   target/riscv: Add vrol.[vv,vx] and vror.[vv,vx,vi] decoding,
>     translation and execution support
>
> Kiran Ostrolenk (8):
>   target/riscv: Refactor some of the generic vector functionality
>   target/riscv: Refactor some of the generic vector functionality
>   target/riscv: Refactor some of the generic vector functionality
>   target/riscv: Refactor some of the generic vector functionality
>   target/riscv: Add vsha2ms.vv decoding, translation and execution
>     support
>   target/riscv: Add zvksh cpu property
>   target/riscv: Add vsm3c.vi decoding, translation and execution support
>   target/riscv: Expose zvksh cpu property
>
> Lawrence Hunter (17):
>   target/riscv: Add vclmul.vv decoding, translation and execution
>     support
>   target/riscv: Add vclmul.vx decoding, translation and execution
>     support
>   target/riscv: Add vclmulh.vv decoding, translation and execution
>     support
>   target/riscv: Add vclmulh.vx decoding, translation and execution
>     support
>   target/riscv: Add vaesef.vv decoding, translation and execution
>     support
>   target/riscv: Add vaesef.vs decoding, translation and execution
>     support
>   target/riscv: Add vaesdf.vv decoding, translation and execution
>     support
>   target/riscv: Add vaesdf.vs decoding, translation and execution
>     support
>   target/riscv: Add vaesdm.vv decoding, translation and execution
>     support
>   target/riscv: Add vaesdm.vs decoding, translation and execution
>     support
>   target/riscv: Add vaesz.vs decoding, translation and execution support
>   target/riscv: Add vsha2c[hl].vv decoding, translation and execution
>     support
>   target/riscv: Add vsm3me.vv decoding, translation and execution
>     support
>   target/riscv: Add zvkg cpu property
>   target/riscv: Add vgmul.vv decoding, translation and execution support
>   target/riscv: Add vghsh.vv decoding, translation and execution support
>   target/riscv: Expose zvkg cpu property
>
> Max Chou (5):
>   crypto: Create sm4_subword
>   crypto: Add SM4 constant parameter CK
>   target/riscv: Add zvksed cfg property
>   target/riscv: Add Zvksed support
>   target/riscv: Expose Zvksed property
>
> Nazar Kazakov (10):
>   target/riscv: Add zvkb cpu property
>   target/riscv: Add vrev8.v decoding, translation and execution support
>   target/riscv: Add vandn.[vv,vx] decoding, translation and execution
>     support
>   target/riscv: Expose zvkb cpu property
>   target/riscv: Add zvkned cpu property
>   target/riscv: Add vaeskf1.vi decoding, translation and execution
>     support
>   target/riscv: Add vaeskf2.vi decoding, translation and execution
>     support
>   target/riscv: Expose zvkned cpu property
>   target/riscv: Add zvknh cpu properties
>   target/riscv: Expose zvknh cpu properties
>
> William Salmon (3):
>   target/riscv: Add vbrev8.v decoding, translation and execution support
>   target/riscv: Add vaesem.vv decoding, translation and execution
>     support
>   target/riscv: Add vaesem.vs decoding, translation and execution
>     support
>
>  accel/tcg/tcg-runtime-gvec.c                 |   11 +
>  accel/tcg/tcg-runtime.h                      |    1 +
>  crypto/sm4.c                                 |   10 +
>  include/crypto/sm4.h                         |    9 +
>  include/qemu/bitops.h                        |   24 +-
>  target/arm/tcg/crypto_helper.c               |   10 +-
>  target/riscv/cpu.c                           |   36 +
>  target/riscv/cpu.h                           |    7 +
>  target/riscv/helper.h                        |   71 ++
>  target/riscv/insn32.decode                   |   49 +
>  target/riscv/insn_trans/trans_rvv.c.inc      |   93 +-
>  target/riscv/insn_trans/trans_rvzvkb.c.inc   |  220 ++++
>  target/riscv/insn_trans/trans_rvzvkg.c.inc   |   40 +
>  target/riscv/insn_trans/trans_rvzvkned.c.inc |  170 +++
>  target/riscv/insn_trans/trans_rvzvknh.c.inc  |   84 ++
>  target/riscv/insn_trans/trans_rvzvksed.c.inc |   57 +
>  target/riscv/insn_trans/trans_rvzvksh.c.inc  |   43 +
>  target/riscv/meson.build                     |    4 +-
>  target/riscv/op_helper.c                     |    5 +
>  target/riscv/translate.c                     |    6 +
>  target/riscv/vcrypto_helper.c                | 1001 ++++++++++++++++++
>  target/riscv/vector_helper.c                 |  240 +----
>  target/riscv/vector_internals.c              |   81 ++
>  target/riscv/vector_internals.h              |  222 ++++
>  24 files changed, 2192 insertions(+), 302 deletions(-)
>  create mode 100644 target/riscv/insn_trans/trans_rvzvkb.c.inc
>  create mode 100644 target/riscv/insn_trans/trans_rvzvkg.c.inc
>  create mode 100644 target/riscv/insn_trans/trans_rvzvkned.c.inc
>  create mode 100644 target/riscv/insn_trans/trans_rvzvknh.c.inc
>  create mode 100644 target/riscv/insn_trans/trans_rvzvksed.c.inc
>  create mode 100644 target/riscv/insn_trans/trans_rvzvksh.c.inc
>  create mode 100644 target/riscv/vcrypto_helper.c
>  create mode 100644 target/riscv/vector_internals.c
>  create mode 100644 target/riscv/vector_internals.h
>
> --
> 2.39.2
>
>

Lawrence Hunter March 23, 2023, 11:34 a.m. UTC | #2

On 21/03/2023 12:02, Christoph Müllner wrote:
> On Fri, Mar 10, 2023 at 10:16 AM Lawrence Hunter
> <lawrence.hunter@codethink.co.uk> 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

Christoph Müllner March 23, 2023, 11:36 a.m. UTC | #3

On Thu, Mar 23, 2023 at 12:34 PM Lawrence Hunter
<lawrence.hunter@codethink.co.uk> wrote:
>
> On 21/03/2023 12:02, Christoph Müllner wrote:
> > On Fri, Mar 10, 2023 at 10:16 AM Lawrence Hunter
> > <lawrence.hunter@codethink.co.uk> 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"

Yes, you are right.
I just created a PR for the Zvk* spec to clarify this:
  https://github.com/riscv/riscv-crypto/pull/308

>
> > 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