Zvkned: add infrastructure for Zvkned #752

KotorinMinami · 2025-02-20T19:34:10Z

Implements the Zvkned (NIST Suite: Vector AES Block Cipher) extension, as of version Release 1.0.0.

The following instructions are included:

vaesef.[vv,vs]
vaesem.[vv,vs]
vaesdf.[vv,vs]
vaesdm.[vv,vs]
vaeskf1.vi
vaeskf2.vi
vaesz.vs

All instructions were tested with VLEN=128 and results were compared with QEMU results of each instruction.

Note
This PR was originally developed by @charmitro , and now the Vector AES Block Cipher has been released.

github-actions · 2025-02-20T19:41:32Z

Test Results

392 tests ±0 392 ✅ ±0 1m 19s ⏱️ -1s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 3161d3c. ± Comparison against base commit 80c4928.

♻️ This comment has been updated with latest results.

Timmmm

A few random low-level Sail comments (I know nothing about the actual spec).

Also how was it tested against QEMU exactly? It would be nice to integrate tests like that (in future; we don't have the infrastructure for it yet).

Timmmm · 2025-02-20T20:44:00Z

model/riscv_insts_zvkned.sail

+  let LMUL     = if LMUL_pow < 0 then 0 else LMUL_pow;
+  let num_elem = get_num_elem(LMUL_pow, SEW);
+
+  if (zvk_check_elements(VLEN, num_elem, LMUL, SEW) == false)


Suggested change

if (zvk_check_elements(VLEN, num_elem, LMUL, SEW) == false)

if not(zvk_check_elements(VLEN, num_elem, LMUL, SEW))

(And in the other places.)

Timmmm · 2025-02-20T20:45:37Z

model/riscv_insts_zvkned.sail

+
+val aes_rotword : bits(32) -> bits(32)
+function aes_rotword(x) = {
+  let a0 : bits (8) = x[ 7.. 0];


I think I would just simplify this to

function aes_rotword(x) = x[7 .. 0] @ x[15 .. 8] @ x[23 .. 16] @ x[31 .. 24]

Timmmm · 2025-02-20T20:46:44Z

model/riscv_insts_zvkned.sail

+  } else {
+    let 'n = num_elem;
+    let 'm = SEW;
+    assert('m == 32);


Is this necessarily true? Definitely worth a comment explaining why at least.

the spec declared that it should be 32 bits, and without that, the Sail compiler will chuck an error, because it won't know how many bits 'm has.

Hmm the spec says it is reserved. I think we should maybe raise an illegal instruction exception in that case (though we still need a proper policy on how to handle reserved behaviour).

I haven't looked at the spec closely enough yet, but if it is reserved it seems like it would be best to catch it at the encdec stage if possible and just not decode it as this instruction. Then it would fall through to the existing illegal instruction catch-all.

Yeah that probably makes sense.

Yes, but then bits('m) would need to be bits(32) for Sail to prove it.

You could do something like this:

let 'SEW = get_sew(); let LMUL_pow = get_lmul_pow(); let LMUL = if LMUL_pow < 0 then 0 else LMUL_pow; let 'num_elem = get_num_elem(LMUL_pow, SEW); if 'SEW != 32 | not(zvk_check_elements(VLEN, num_elem, LMUL, SEW)) then { handle_illegal(); RETIRE_FAIL } else { let vs2_val = read_vreg(num_elem, SEW, LMUL_pow, vs2); let vd_val = read_vreg(num_elem, SEW, LMUL_pow, vd); let eg_len = (unsigned(vl) / 'num_elem); ...

I think that will compile. Not 100% sure though.

That works, thank you!

Timmmm · 2025-02-20T20:48:54Z

model/riscv_insts_zvkned.sail

+
+    let eg_len = (unsigned(vl) / 'n);
+    let eg_start = (unsigned(vstart) / 'n);
+    let keyelem = if suffix == "vv" then 1 else 0;


I think we should avoid having behaviour encoded in strings. suffix should be an enum, not strings.

Timmmm · 2025-02-20T20:50:13Z

model/riscv_insts_zvkned.sail

+      assert(0 <= ((i * 4) + 3) & ((i * 4) + 3) < 'n);
+
+      let vd_state = get_velem(vd_val, i);
+      let vs2_key = get_velem(vs2_val, keyelem * i);


So this would be something like:

let vs2_key = get_velem(vs2_val, match suffix { something_VV => i, something_VS => 0 });

(I'll leave you to come up with suitable names for whatever VV and VS mean!)

Timmmm · 2025-02-20T20:51:17Z

model/riscv_insts_zvkned.sail

+      let vs2_key = get_velem(vs2_val, keyelem * i);
+      let sb       : bitvector(128) = aes_subbytes_fwd(vd_state);
+      let sr       : bitvector(128) = aes_shift_rows_fwd(sb);
+      let ark      : bitvector(128) = sr ^ vs2_key;


bits? Maybe before my time; is bitvector an old version of bits?

Timmmm · 2025-02-20T20:51:46Z

model/riscv_insts_zvkned.sail

+      let sb       : bitvector(128, dec) = aes_subbytes_fwd(vd_state);
+      let sr       : bitvector(128, dec) = aes_shift_rows_fwd(sb);
+      let mix      : bitvector(128, dec) = aes_mixcolumns_fwd(sr);
+      let ark      : bitvector(128, dec) = mix ^ vs2_key;


bits(128). You could just delete all these, though it may be appropriate for readability.

Timmmm · 2025-02-20T20:53:03Z

model/riscv_insts_zvkned.sail

+    assert('m == 32);
+
+    let vs2_val : vector('n, dec, bits('m)) = read_vreg(num_elem, SEW, LMUL_pow, vs2);
+    let vd_val  : vector('n, dec, bits('m)) = read_vreg(num_elem, SEW, LMUL_pow, vd);


Remove , dec - Sail doesn't support per-vector orders now.

You could just delete the type annotations (I would, but up to you).

charmitro · 2025-02-20T21:38:37Z

model/riscv_insts_zvkned.sail

+/* VAESDM.VV */
+
+mapping vaesdm_mnemonic : bits(7) <-> string = {
+  0b1010001 <-> "vaesem.vv",
+  0b1010011 <-> "vaesem.vs",
+}


Comment and mapping name states vaesdm but the mapping itself is happening for vaesem.[vv, vs] mnemonics.

Also the rest of the model maps ast to assembly; not the bits directly.

jordancarlin

Still need to take a look at the spec, but here are some initial comments.

jordancarlin · 2025-02-21T08:01:39Z

model/riscv_insts_zvkned.sail

+function get_velem(v, i) = v[4 * i + 3] @ v[4 * i + 2] @ v[4 * i + 1] @ v[4 * i]
+
+val      zvk_check_elements : (int, int, int, int) -> bool
+function zvk_check_elements(VLEN, num_elem, LMUL, SEW) = {


With simple function declarations like this, it’s usually easier to eliminate the val line and directly integrate the types into the function line.

jordancarlin · 2025-02-21T08:02:10Z

model/riscv_insts_zvkned.sail

@@ -0,0 +1,428 @@
+/*


Missing copyright header

jordancarlin · 2025-02-21T08:07:54Z

model/riscv_extensions.sail

@@ -23,6 +23,7 @@ enum clause extension = Ext_C
 enum clause extension = Ext_B
 // Vector Operations
 enum clause extension = Ext_V
+enum clause extension = Ext_Zvkned


Missing comment with long form name of the extension. This should also be moved down to be with the rest of the Z* extensions in a new Zv* section.

charmitro · 2025-02-22T21:24:59Z

model/riscv_insts_zvkned.sail

+      let current_round_key = get_velem(vs2_val, i);
+
+      let round_key_b = get_velem(vs2_val, i);


Specification states:

let CurrentRoundKey[3:0] : bits(128) = get_velem(vs2, EGW=128, i); let RoundKeyB[3:0] : bits(32) = get_velem(vd, EGW=128, i); // Previous round key

To support the implementation of Zvkned extension in SAIL, this creates the necessary infrastructure(i.e., a file to hold it, and the extension macro), preparing the tree for the Zvkned implementation. Signed-off-by: Charalampos Mitrodimas <[email protected]> Co-authored-by: KotorinMinami <[email protected]>

Timmmm reviewed Feb 20, 2025

View reviewed changes

charmitro reviewed Feb 20, 2025

View reviewed changes

jordancarlin reviewed Feb 21, 2025

View reviewed changes

trdthg mentioned this pull request Feb 21, 2025

Track Cryptography Extensions: Vector Instructions #742

Open

charmitro reviewed Feb 22, 2025

View reviewed changes

KotorinMinami force-pushed the zvkned branch 2 times, most recently from b470e17 to 9826514 Compare February 24, 2025 07:09

KotorinMinami force-pushed the zvkned branch from 9826514 to 3161d3c Compare February 24, 2025 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zvkned: add infrastructure for Zvkned #752

Zvkned: add infrastructure for Zvkned #752

KotorinMinami commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 •

edited

Loading

Timmmm left a comment

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

KotorinMinami Feb 24, 2025

Timmmm Feb 24, 2025

jordancarlin Feb 24, 2025

Timmmm Feb 24, 2025

KotorinMinami Feb 24, 2025

Timmmm Feb 24, 2025

KotorinMinami Feb 24, 2025

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

Timmmm Feb 20, 2025

charmitro Feb 20, 2025

Timmmm Feb 21, 2025

jordancarlin left a comment

jordancarlin Feb 21, 2025

jordancarlin Feb 21, 2025

jordancarlin Feb 21, 2025

charmitro Feb 22, 2025

	if (zvk_check_elements(VLEN, num_elem, LMUL, SEW) == false)
	if not(zvk_check_elements(VLEN, num_elem, LMUL, SEW))

		let current_round_key = get_velem(vs2_val, i);

		let round_key_b = get_velem(vs2_val, i);

Zvkned: add infrastructure for Zvkned #752

Are you sure you want to change the base?

Zvkned: add infrastructure for Zvkned #752

Conversation

KotorinMinami commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 • edited Loading

Test Results

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordancarlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2025 •

edited

Loading