scalar-crypto: Initial commit of 1.0.0-rc2 spec work. #99

ben-marshall · 2021-07-30T12:33:45Z

Brief: (PR information duplicated from commit message)

This commit adds initial support for the scalar cryptography extension to RISC-V, synced with version v1.0.0-rc2 of the draft proposals.
The riscv-crypto repository (https://github.com/riscv/riscv-crypto) contains all of the work so far on the extension. See the releases tab for the latest spec release.

Details:

There are two main classes of functionality being added. The instructions and the entropy source.
There is a considerable amount of common code (which will eventually also be used by the vector crypto extension) which lives in model/riscv_types_kext.sail. This consists of things like SBox definitions, block cipher operations and common structs/enums.
The entropy source is a CSR based interface (note this is not the same as a "random number generator") which can be used to seed a DRBG. It's been added in line with the existing ISA CSRs, with an additional platform function plat_get_16_random_bits, which calls out to actually source entropy for the source.

Additional:

I'm sorry this is such an enormous PR, and so hard to review. I didn't see a better way to break it down into smaller incremental pieces, but am of course happy to help explain things as needed. I'm also happy maintaining this code (with the help of the repo owners) during the rest of the standardisation process.
The scalar cryptography extension is unique in that it borrows instructions from the Bit-manipulation extension. Since there is not yet any support for Bitmanip within sail-riscv, I have only included instructions unique to the scalar crypto extension. As and when Bitmanip support is added, I/we can work to make sure the right instructions are available to both extensions. Details of the relevant instructions are found in the scalar crypto specification.

Like I say, I'm sure there will be a certain amount of iteration before this can be merged. Please let me know what I can do to help or make things clearer. I also know that the sail-riscv repository is in the the process of being migrated to the riscv organisation. I wanted to open this PR now so it doesn't get forgotten or missed. I'm happy re-opening it against a new repo if need be.

Many thanks,
Ben

cetola · 2021-09-09T17:15:50Z

Ben is still working on some fixes, but once complete we will need review. Added @martinberger and @jrtc27.

martinberger · 2021-09-09T17:52:42Z

Just so that I am up-to-date. What is the relationship of the Verilog to the Sail code, have they already been verified against each other, or is the RTL only as a kind of comment? @ben-marshall

model/riscv_insts_zkn.sail

model/riscv_insts_zkn_rv32.sail

model/riscv_types_kext.sail

model/riscv_zkr.sail

model/riscv_types_kext.sail

ben-marshall · 2021-09-10T08:29:56Z

@martinberger - The example RTL in our riscv/riscv-crypto repository has been a little out of date for a while now if that's the RTL you meant? It's on my list to bring it back in line now things are frozen. We haven't done any formal checking against the Sail code - I didn't know there was a flow for that? If you can point me to it, I'd love to give it a go.

@jrtc27 - Thanks for all of your comments, I realise I should have marked this WiP. Now everything is frozen, I can update the entropy source (since that changed 3 times in three weeks) and fix the editorial issues. Since I seem to be so goodbad at flunking the existing code practices, would it help if I collated your comments into a style-guide doc of sorts to use going forward?

martinberger · 2021-09-10T09:03:54Z

@ben-marshall I am not aware that there is an official flow for for comparing Sail with RTL. It would be great to have one. But I'm not sure we should have RTL in the official Sail model, unless it is proven equivalent.

martinberger · 2021-09-10T09:07:50Z

would it help if I collated your comments into a style-guide doc of sorts to use going forward?

@ben-marshall That would be lovely. I was thinking about something like this too. In the long run, I think an automatic formatter, like gofmt is the way to go.

ben-marshall · 2021-09-10T09:42:30Z

In the long run, I think an automatic formatter, like gofmt is the way to go.

I absolutely agree! Okay, as I'm implementing all of my fixes then, I'll try to record them.

But I'm not sure we should have RTL in the official Sail model, unless it is proven equivalent.

Also agreed. I think keeping RTL out of the Sail model repo completely is sensible.

model/riscv_sys_regs.sail

model/riscv_ext_regs.sail

model/riscv_sys_regs.sail

model/riscv_insts_zkn.sail

model/prelude.sail

- Updated with arch review comments: - Post arch review, we decided to not overlap aes32/aes64 opcodes. - Post arch-review entropy source updates. - Cleanup of common code and removal of un-used structures. - Express AES/SM4 sboxes all in the same way. - Adds polymorphic rotation support after some help during review: #99 (comment) - Adds seed entropy source CSR in line with existing base ISA csrs, rather than using the out-of-tree extensions interface as previous versions did. Changes to be committed: modified: Makefile modified: c_emulator/riscv_platform.c modified: c_emulator/riscv_platform.h modified: c_emulator/riscv_platform_impl.c modified: c_emulator/riscv_platform_impl.h modified: handwritten_support/0.11/riscv_extras.lem modified: handwritten_support/riscv_extras.lem modified: model/prelude.sail modified: model/riscv_csr_map.sail modified: model/riscv_insts_zicsr.sail new file: model/riscv_insts_zkn.sail new file: model/riscv_insts_zks.sail modified: model/riscv_sys_control.sail modified: model/riscv_sys_regs.sail new file: model/riscv_types_kext.sail modified: ocaml_emulator/platform.ml

ben-marshall · 2021-09-17T10:46:47Z

Hi @martinberger and @jrtc27

I think I've addressed the things you have pointed out / asked for so far. I don't doubt there is room for improvement, but I'm not confident enough to judge what that looks like now.

Some known issues:

Access control for the entropy source is dependent on the mseccfg CSR, which is just now in public review. I've noted the TODO items where this code will need updating.
Because the riscv-config (or it's latest iteration) isn't final, the gating of instructions based on whether extensions are implemented or not is done with some stub functions which currently always return true. They will need changing to support dynamic configuration of extensions and runtime changes of XLEN, depending on what the implementation of that looks like.

Otherwise:

I've added Jessica's polymorphic rotation implementation and changed the code I've written to use it.
There are still parts of the scalar crypto extensions (Zbkb/c/x) which this PR doesn't add. I'd like to get these instructions merged in first, then I'll turn to the Zbkb/c/x instructions. There is some overlap with Bitmanip there which I need to coordinate. I hope submitting things in pieces like this is still okay.

I'm sorry we jumped the gun a bit in asking for a review. Everything you said was still very relevant / would have been necessary if I'd had the PR in a finished state already, so it was all useful. Please let me know anything else you need from me and I'll get on it. I don't want to take up any more of your time than I have, and I'm very grateful for your patience!

Cheers,
Ben

jjscheel · 2021-10-05T12:39:00Z

@martinberger, would appreciate some insight into next steps for this PR for @ben-marshall. Thanks!

martinberger · 2021-10-05T13:24:22Z

@martinberger, would appreciate some insight into next steps for this PR for @ben-marshall. Thanks!

@jjscheel @jrtc27 I've looked at the current version and it looks good to me.

jjscheel · 2021-10-11T12:50:30Z

@martinberger it sounds like you can mark your review complete.

@jrtc27, anything left for you? if not, can we mark your review complete and get this merged?

model/riscv_sys_regs.sail

jrtc27 · 2021-10-05T12:43:03Z

model/riscv_csr_map.sail

I'll commit this as a separate commit to keep this PR clean

jrtc27 · 2021-10-05T12:43:48Z

model/riscv_insts_zkn.sail

+
+function clause execute (SHA512SIG0H(rs2, rs1, rd)) = {
+  X(rd) = EXTS((X(rs1) >>  1) ^ (X(rs1) >>  7) ^ (X(rs1) >>  8) ^
+               (X(rs2) << 31)                  ^ (X(rs2) << 24) );


The spaces at the ends of these lines are a bit odd, they don't see to be to line up with anything?

Erm. Maybe I didn't want line 192 to end with 8)^ and then wanted the final bracket to match up? I don't know I'm afraid, but can change it as you like.

Yes, please delete the space before the final close paren in all instances of this, it looks odd and isn't consistent with the rest of the code base. If you like you can think of the semicolon as lining up with the ^ instead :)

model/riscv_sys_control.sail

model/riscv_insts_zks.sail

model/riscv_sys_regs.sail

jrtc27 · 2021-10-11T13:27:56Z

model/riscv_types_kext.sail

+/* Auxiliary function for performing GF multiplicaiton */
+val xt2 : bits(8) -> bits(8)
+function xt2(x) = {
+  (x << 1) ^ ( match (bit_to_bool(x[7]) ) {


Weird spacing here around the parens (which then also is a bit misleading as it looks like the ones with spaces around them are a pair, but they're not, and that wouldn't make sense), also just use if/else instead of matching on a bool.

Yeah this was a cursed use of match. Changed to if.

model/riscv_types_kext.sail

Makefile

jrtc27 · 2021-10-11T13:32:11Z

Makefile

+SAIL_DEFAULT_INST += riscv_insts_zkn.sail
+SAIL_DEFAULT_INST += riscv_insts_zks.sail


Maybe put these after D just below so D is kept with F?

Yep, good shout.

martinberger · 2021-10-11T13:52:53Z

@martinberger it sounds like you can mark your review complete.

@jjscheel Done

Co-authored-by: Jessica Clarke <jrtc27@jrtc27.com> - Apply suggestions from code review - Re-order files in makefile to keep things grouped sensibly. - Fix weird spacing etc. - Rename have<extension>() functions and inline XLEN checks. - Remove uneccessary wrapper function around plat_get_16_random_bits and rename to get_16_random_bits Changes to be committed: modified: Makefile modified: model/riscv_insts_zkn.sail modified: model/riscv_insts_zks.sail modified: model/riscv_sys_control.sail modified: model/riscv_sys_regs.sail modified: model/riscv_types_kext.sail

On branch scalar-crypto Changes to be committed: modified: model/riscv_insts_zkn.sail

#99 (comment) On branch scalar-crypto Changes to be committed: modified: model/riscv_insts_zkn.sail

ben-marshall · 2021-10-13T10:52:15Z

@jrtc27, @martinberger, @jjscheel - Hopefully I've addressed all of the remaining comments, apologies it took me a while to get round to.

Cheers,
Ben

jrtc27 · 2021-10-18T11:28:34Z

I did not approve this yet

martinberger · 2021-10-19T16:43:05Z

I did not approve this yet

I'm sorry, I lost track what was still open. What is left to check / change? @jrtc27

jrtc27 · 2021-10-22T13:50:40Z

model/riscv_insts_zkn.sail

+union clause ast = SHA512SUM0R : (regidx, regidx, regidx)
+union clause ast = SHA512SUM1R : (regidx, regidx, regidx)
+
+mapping clause encdec = SHA512SUM0R (rs2, rs1, rd) if haveZknh() & sizeof(xlen)==32


Missing spaces around == introduced in the latest version that got merged (I count 10 instances across two files)

jrtc27 · 2021-10-22T13:52:49Z

model/riscv_insts_zkn.sail

+union clause ast = AES64DSM  : (regidx, regidx, regidx)
+union clause ast = AES64DS   : (regidx, regidx, regidx)
+
+mapping clause encdec = AES64KS1I (rcon, rs1, rd) if (haveZkne() | haveZknd()) & (sizeof(xlen) == 64) & (rcon <_u 0xB)


A bunch of these saw behavioural changes in the latest version that got merged, not just non-behavioural changes inlining the implementations of the misleadingly-named functions. This one was previously just Zkne (on RV64), but is now enabled for both Zkne and Zknd (on RV64). Based on the name of the instruction I assume this is correct, but could you please double-check / confirm every single change in https://github.com/riscv/sail-riscv/compare/33aaaa04eaeaa9d2ed69023735ad6ebb93f71775..b83f68b3ade2d5444fd7dfe952e441824bb19dd8 is what you intended?

jrtc27 · 2021-10-22T13:54:42Z

model/riscv_insts_zkn.sail

+
+function clause execute (SHA512SIG0H(rs2, rs1, rd)) = {
+  X(rd) = EXTS((X(rs1) >>  1) ^ (X(rs1) >>  7) ^ (X(rs1) >>  8) ^
+               (X(rs2) << 31)                  ^ (X(rs2) << 24) );


Yes, please delete the space before the final close paren in all instances of this, it looks odd and isn't consistent with the rest of the code base. If you like you can think of the semicolon as lining up with the ^ instead :)

jrtc27 · 2021-10-22T14:19:19Z

model/riscv_insts_zkn.sail

+  let rc   : bits(32) = aes_decode_rcon(rcon);
+  let tmp2 : bits(32) = if (rcon == 0xA) then tmp1 else (tmp1 >>> 8);
+  let tmp3 : bits(32) = aes_subword_fwd(tmp2);
+  X(rd)    = (tmp3 ^ rc) @ (tmp3 ^ rc);


In that example use, the output of AES64KS1I is only ever fed into AES64KS2's rs1 input, which only ever reads the upper half of the output, so the second copy is unused. I do not believe any current Zkn-using code makes use of the low 32 bits of the instruction. For AESNI, they return RotWord (SubWord (X1)) XOR RCON, SubWord (X1) (since RotWord and SubWord commute, due to SubWord being a mapping over bytes, that's entirely equivalent to tmp3 here, Intel commuted them compared with the spec, I guess because then SubWord (X1) is a common sub-expression), and note that the lower 32 bits are only needed for AES-256. This is presumably for the

else if (Nk > 6 and i mod Nk = 4) temp = SubWord(temp)

part of KeyExpansion as defined by the AES spec.

But I now see this is the point of the rcon == 0xA check; you're exploiting the fact that the only standardised AES variants never need more than 10 different round constants (1-10 in the spec, 0-9 in your encoding here), and then using the 11th round constant as a way to bodge in the special >192-bit behaviour for every 4 + x * Nkth word, treating that as a sentinel value to skip the RotWord (and you define that rcon as 0, so the XOR is skipped, in a non-obvious manner). I'd probably rather that be specified in a more obvious manner, with aes_decode_rcon only accepting legal inputs instead of silently mapping reserved ones to 0, and AES64KS1I having a very clear special case for 0xA, rather than trying to shoe-horn it into the normal case. If it were up to me I also wouldn't have chosen 0xA but a different instruction just for this (aes64ks1s perhaps, with the s being for SubWord), or at least 0xF as a clearer sentinel value. I also don't know whether it's plausible there will ever be future variants needing more round constants, especially given the

However, future reaffirmations of this standard could include changes or additions to the allowed values for those parameters. Therefore, implementers may choose to design their AES implementations with future flexibility in mind.

in the NIST spec, and so putting it at the end of the encoding space gets it more out of the way, though you still only have 4 bits for the field rather than the theoretical maximum of 8 so maybe in practice you'd need to carve out space for a new instruction anyway.

jrtc27 · 2021-10-22T15:15:21Z

I did not approve this yet

I'm sorry, I lost track what was still open. What is left to check / change? @jrtc27

See the comments I just posted re-reviewing it after the latest changes (and digging into how exactly AES64KS1I ends up working for AES-256)

martinberger · 2021-10-22T16:58:50Z

@jrtc27 Thanks for the thorough reviews.

I think the PR reviews could be made more lightweight if all issues regarding syntact code formatting were handled by a code formatter. The Go community have been very successful in pushing gofmt. Sail already has the -output_sail option, but that runs a bit too late. I wonder of difficult it would be to tweak the code for -output_sail so it became an automatic code formatter.

jrtc27 · 2021-10-22T20:00:49Z

@jrtc27 Thanks for the thorough reviews.

I think the PR reviews could be made more lightweight if all issues regarding syntact code formatting were handled by a code formatter. The Go community have been very successful in pushing gofmt. Sail already has the -output_sail option, but that runs a bit too late. I wonder of difficult it would be to tweak the code for -output_sail so it became an automatic code formatter.

For the common case, probably not very (though note that -output_sail also has the issue that it does no line wrapping), but it needs quite a lot of work to get something that works reliably enough that it isn't a complete pain; even if it's right 95% of the time, that 5% of fighting against it to manually undo strange formatting decisions can be more effort than just writing properly-formatted code in the first place. If you or someone you know wants to invest the effort into making that work then please go ahead, but I don't think it's just a weekend job.

Also there are a load of places where redundant whitespace is inserted into the model's code deliberately in order to line things up and make it more readable (this PR has some examples for lining up shifts, but there are also places where we pad variable names in match clauses), but not in all cases; that's a very subjective human decision, so I think you'd either have to annotate those as not auto-formatted or sacrifice their readability.

jrtc27 · 2021-11-01T16:01:40Z

@ben-marshall will you be fixing the outstanding issues highlighted above with your changes?

ben-marshall · 2021-11-01T16:11:12Z

Hi @jrtc27 - Yes they are on my todo list. Sorry I should have replied earlier saying so. I'm hoping to get to it on Friday.

ben-marshall mentioned this pull request Aug 6, 2021

SAIL Support: Scalar Instructions riscv/riscv-crypto#20

Closed

16 tasks

ben-marshall mentioned this pull request Sep 8, 2021

Lem and adding the plat_get_16_random_bits entropy source function #106

Closed

ben-marshall changed the title ~~scalar-crypto: Initial commit of 0.9.4 spec work~~ scalar-crypto: Initial commit of 1.0.0 spec work. Sep 8, 2021

cetola requested review from martinberger and jrtc27 September 9, 2021 17:14

jrtc27 reviewed Sep 9, 2021

View reviewed changes

ben-marshall commented Sep 15, 2021

View reviewed changes

model/riscv_sys_regs.sail Show resolved Hide resolved

ben-marshall commented Sep 15, 2021

View reviewed changes

model/riscv_ext_regs.sail Outdated Show resolved Hide resolved

ben-marshall commented Sep 15, 2021

View reviewed changes

model/riscv_sys_regs.sail Show resolved Hide resolved

ben-marshall commented Sep 15, 2021

View reviewed changes

model/riscv_insts_zkn.sail Outdated Show resolved Hide resolved

jrtc27 reviewed Sep 15, 2021

View reviewed changes

model/prelude.sail Outdated Show resolved Hide resolved

ben-marshall changed the title ~~scalar-crypto: Initial commit of 1.0.0 spec work.~~ scalar-crypto: Initial commit of 1.0.0-rc2 spec work. Sep 17, 2021

ben-marshall requested a review from jrtc27 September 24, 2021 07:58

jrtc27 reviewed Oct 11, 2021

View reviewed changes

martinberger approved these changes Oct 11, 2021

View reviewed changes

ben-marshall and others added 3 commits October 13, 2021 10:29

scalar-crypto: fix decode of aesks1i

dbda26b

On branch scalar-crypto Changes to be committed: modified: model/riscv_insts_zkn.sail

scalar-crypto: better names for aes64ks1i intermediates

4feadb7

#99 (comment) On branch scalar-crypto Changes to be committed: modified: model/riscv_insts_zkn.sail

martinberger approved these changes Oct 14, 2021

View reviewed changes

martinberger merged commit 1c7584b into riscv:master Oct 18, 2021

jrtc27 reviewed Oct 22, 2021

View reviewed changes

ben-marshall mentioned this pull request Nov 3, 2021

Scalar crypto - post code review improvments #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scalar-crypto: Initial commit of 1.0.0-rc2 spec work. #99

scalar-crypto: Initial commit of 1.0.0-rc2 spec work. #99

ben-marshall commented Jul 30, 2021 •

edited

cetola commented Sep 9, 2021

martinberger commented Sep 9, 2021 •

edited

ben-marshall commented Sep 10, 2021

martinberger commented Sep 10, 2021

martinberger commented Sep 10, 2021

ben-marshall commented Sep 10, 2021

ben-marshall commented Sep 17, 2021

jjscheel commented Oct 5, 2021

martinberger commented Oct 5, 2021 •

edited

jjscheel commented Oct 11, 2021

jrtc27 Oct 5, 2021

jrtc27 Oct 5, 2021

ben-marshall Oct 13, 2021

jrtc27 Oct 22, 2021

jrtc27 Oct 11, 2021

ben-marshall Oct 13, 2021

jrtc27 Oct 11, 2021

ben-marshall Oct 13, 2021

martinberger commented Oct 11, 2021

ben-marshall commented Oct 13, 2021

jrtc27 commented Oct 18, 2021

martinberger commented Oct 19, 2021

jrtc27 Oct 22, 2021

jrtc27 Oct 22, 2021 •

edited

jrtc27 Oct 22, 2021

jrtc27 Oct 22, 2021

jrtc27 commented Oct 22, 2021

martinberger commented Oct 22, 2021 •

edited

jrtc27 commented Oct 22, 2021

jrtc27 commented Nov 1, 2021

ben-marshall commented Nov 1, 2021

		SAIL_DEFAULT_INST += riscv_insts_zkn.sail
		SAIL_DEFAULT_INST += riscv_insts_zks.sail

scalar-crypto: Initial commit of 1.0.0-rc2 spec work. #99

scalar-crypto: Initial commit of 1.0.0-rc2 spec work. #99

Conversation

ben-marshall commented Jul 30, 2021 • edited

cetola commented Sep 9, 2021

martinberger commented Sep 9, 2021 • edited

ben-marshall commented Sep 10, 2021

martinberger commented Sep 10, 2021

martinberger commented Sep 10, 2021

ben-marshall commented Sep 10, 2021

ben-marshall commented Sep 17, 2021

jjscheel commented Oct 5, 2021

martinberger commented Oct 5, 2021 • edited

jjscheel commented Oct 11, 2021

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

martinberger commented Oct 11, 2021

ben-marshall commented Oct 13, 2021

jrtc27 commented Oct 18, 2021

martinberger commented Oct 19, 2021

Choose a reason for hiding this comment

jrtc27 Oct 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 commented Oct 22, 2021

martinberger commented Oct 22, 2021 • edited

jrtc27 commented Oct 22, 2021

jrtc27 commented Nov 1, 2021

ben-marshall commented Nov 1, 2021

ben-marshall commented Jul 30, 2021 •

edited

martinberger commented Sep 9, 2021 •

edited

martinberger commented Oct 5, 2021 •

edited

jrtc27 Oct 22, 2021 •

edited

martinberger commented Oct 22, 2021 •

edited