From: Lawrence Hunter <lawrence.hunter@codethink.co.uk>
To: "Christoph Müllner" <christoph.muellner@vrull.eu>
Cc: qemu-devel@nongnu.org, 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
Subject: Re: [PATCH 00/45] Add RISC-V vector cryptographic instruction set support
Date: Thu, 23 Mar 2023 11:34:49 +0000 [thread overview]
Message-ID: <4b21e3316c50763d0fbe273a59bc985c@codethink.co.uk> (raw)
In-Reply-To: <CAEg0e7iXkPcqAhZH0xxbMyXVP6hnk5vvtUW52qT_2rFDK3PVcQ@mail.gmail.com>
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
next prev parent reply other threads:[~2023-03-23 11:35 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-10 9:11 [PATCH 00/45] Add RISC-V vector cryptographic instruction set support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 01/45] target/riscv: Add zvkb cpu property Lawrence Hunter
2023-03-10 9:11 ` [PATCH 02/45] target/riscv: Refactor some of the generic vector functionality Lawrence Hunter
2023-03-10 9:11 ` [PATCH 03/45] target/riscv: Add vclmul.vv decoding, translation and execution support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 04/45] target/riscv: Refactor some of the generic vector functionality Lawrence Hunter
2023-03-12 23:40 ` Wilfred Mallawa
2023-03-10 9:11 ` [PATCH 05/45] target/riscv: Add vclmul.vx decoding, translation and execution support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 06/45] target/riscv: Add vclmulh.vv " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 07/45] target/riscv: Add vclmulh.vx " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 08/45] target/riscv: Refactor some of the generic vector functionality Lawrence Hunter
2023-03-10 9:11 ` [PATCH 09/45] qemu/bitops.h: Limit rotate amounts Lawrence Hunter
2023-03-10 9:11 ` [PATCH 10/45] target/riscv: Add vrol.[vv, vx] and vror.[vv, vx, vi] decoding, translation and execution support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 11/45] target/riscv: Refactor some of the generic vector functionality Lawrence Hunter
2023-03-10 9:11 ` [PATCH 12/45] target/riscv: Add vbrev8.v decoding, translation and execution support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 13/45] target/riscv: Add vrev8.v " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 14/45] target/riscv: Add vandn.[vv, vx] " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 15/45] target/riscv: Expose zvkb cpu property Lawrence Hunter
2023-03-10 9:11 ` [PATCH 16/45] target/riscv: Add zvkned " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 17/45] target/riscv: Add vaesef.vv decoding, translation and execution support Lawrence Hunter
2023-03-10 9:11 ` [PATCH 18/45] target/riscv: Add vaesef.vs " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 19/45] target/riscv: Add vaesdf.vv " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 20/45] target/riscv: Add vaesdf.vs " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 21/45] target/riscv: Add vaesdm.vv " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 22/45] target/riscv: Add vaesdm.vs " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 23/45] target/riscv: Add vaesz.vs " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 24/45] target/riscv: Add vaesem.vv " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 25/45] target/riscv: Add vaesem.vs " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 26/45] target/riscv: Add vaeskf1.vi " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 27/45] target/riscv: Add vaeskf2.vi " Lawrence Hunter
2023-03-10 9:11 ` [PATCH 28/45] target/riscv: Expose zvkned cpu property Lawrence Hunter
2023-03-10 9:11 ` [PATCH 29/45] target/riscv: Add zvknh cpu properties Lawrence Hunter
2023-03-10 9:12 ` [PATCH 30/45] target/riscv: Add vsha2ms.vv decoding, translation and execution support Lawrence Hunter
2023-03-10 9:12 ` [PATCH 31/45] target/riscv: Add vsha2c[hl].vv " Lawrence Hunter
2023-03-10 9:12 ` [PATCH 32/45] target/riscv: Expose zvknh cpu properties Lawrence Hunter
2023-03-10 9:12 ` [PATCH 33/45] target/riscv: Add zvksh cpu property Lawrence Hunter
2023-03-10 9:12 ` [PATCH 34/45] target/riscv: Add vsm3me.vv decoding, translation and execution support Lawrence Hunter
2023-03-10 9:12 ` [PATCH 35/45] target/riscv: Add vsm3c.vi " Lawrence Hunter
2023-03-10 9:12 ` [PATCH 36/45] target/riscv: Expose zvksh cpu property Lawrence Hunter
2023-03-10 9:12 ` [PATCH 37/45] target/riscv: Add zvkg " Lawrence Hunter
2023-03-10 9:12 ` [PATCH 38/45] target/riscv: Add vgmul.vv decoding, translation and execution support Lawrence Hunter
2023-03-10 9:12 ` [PATCH 39/45] target/riscv: Add vghsh.vv " Lawrence Hunter
2023-03-10 9:12 ` [PATCH 40/45] target/riscv: Expose zvkg cpu property Lawrence Hunter
2023-03-10 9:12 ` [PATCH 41/45] crypto: Create sm4_subword Lawrence Hunter
2023-03-10 9:12 ` [PATCH 42/45] crypto: Add SM4 constant parameter CK Lawrence Hunter
2023-03-10 9:12 ` [PATCH 43/45] target/riscv: Add zvksed cfg property Lawrence Hunter
2023-03-10 9:12 ` [PATCH 44/45] target/riscv: Add Zvksed support Lawrence Hunter
2023-03-10 9:12 ` [PATCH 45/45] target/riscv: Expose Zvksed property Lawrence Hunter
2023-03-21 12:02 ` [PATCH 00/45] Add RISC-V vector cryptographic instruction set support Christoph Müllner
2023-03-23 11:34 ` Lawrence Hunter [this message]
2023-03-23 11:36 ` Christoph Müllner
-- strict thread matches above, loose matches on Subject: below --
2023-03-10 16:03 Lawrence Hunter
2023-03-23 12:51 ` Daniel Henrique Barboza
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4b21e3316c50763d0fbe273a59bc985c@codethink.co.uk \
--to=lawrence.hunter@codethink.co.uk \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=christoph.muellner@vrull.eu \
--cc=dickon.hood@codethink.co.uk \
--cc=frank.chang@sifive.com \
--cc=kiran.ostrolenk@codethink.co.uk \
--cc=kvm@vger.kernel.org \
--cc=nazar.kazakov@codethink.co.uk \
--cc=palmer@dabbelt.com \
--cc=pbonzini@redhat.com \
--cc=philipp.tomsich@vrull.eu \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).