Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zvkned: add infrastructure for Zvkned #752

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KotorinMinami
Copy link
Contributor

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.

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 <charalampos.mitrodimas@vrull.eu>

Co-authored-by: KotorinMinami <98281165+KotorinMinami@users.noreply.github.com>
Copy link

Test Results

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

Results for commit 4adf486. ± Comparison against base commit 80c4928.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would just simplify this to

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

} else {
let 'n = num_elem;
let 'm = SEW;
assert('m == 32);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines +201 to +206
/* VAESDM.VV */

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@jordancarlin jordancarlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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) = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,428 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants