Added Imperas K Crypto Scalar Tests #172
Conversation
|
Hi there I've done a pass over the tests and have a few comments:
Otherwise, everything looks good to me. It's fantastic to have both the scalar crypto specific instructions, and the various Bitmanip instructions and specific variants thereof - E.g. About the SBox coverage problem, I'll explain it here for people to ask questions about, and add the resulting explanation into the test plan.
Does that explanation make sense? Cheers, |
|
For ase64, you could start with any random (well, in a wider sense of
random than we are used to discussing here) value and add any constant in
which every byte value is coprime with 256, just to make it more
interesting. Whether that makes coverage better is a different issue.
That same approach works for the 32bit versions, of course.
So, the small test would put this all in a loop, but the test methodology
would unroll it to make sure different registers are used.for sources and
destinations. That can still derive the inputs from the original starting
values so they don't have to be reloaded on every test, just recomputed and
moved.
…On Tue, Mar 2, 2021 at 3:52 AM Ben Marshall ***@***.***> wrote:
Hi there
I've done a pass over the tests and have a few comments:
- The xperm.n and xperm.b instructions are missing from the "borrowed
from bitmanip" subset. I assume the reason for this is because until very
recently, there was no toolchain (experimental or otherwise) which
supported xperm.* and the scalar crypto instructions. Such a version
does now exist in the riscv/riscv-crypto repository.
- The RV32 tests seem to use 64-bit literals in various places for
source register inputs, which was a bit confusing? E.g.
riscv-test-suite/rv32i_m/K/src/I-AES32DSI-01.S, line 51.
- The SBox coverage for the AES and SM4 instructions is incomplete.
Given Imperas followed the scalar crypto test plan, I think this is mostly
down to my poor communication of the problem in that document. Everywhere
else, the instruction inputs / value coverage is exactly as I
expected/hoped.
- These tests are for v0.7.2. I remember that the tests for v0.8.1
(the most recent version) were going to follow after about a week, is that
right? I'm fairly sure Imperas/OVPSim is the only remaining implementation
of v0.7.2, all of the hardware implementations I'm aware of, plus
Spike and Sail have moved on to v0.8.1, so rolling them back to
actually run these tests will be difficult. Maybe it would be worth waiting
a week and only merging in the v0.8.1 tests which multiple people can
run and check? I say this without wanting to undermine the effort that went
into generating these tests!
Otherwise, everything looks good to me. It's fantastic to have both the
scalar crypto specific instructions, *and* the various Bitmanip
instructions and specific variants thereof - E.g. zip, rather than all of
shfl.
About the SBox coverage problem, I'll explain it here for people to ask
questions about, and add the resulting explanation into the test plan
<https://github.com/riscv/riscv-crypto/blob/master/tests/compliance/test-plan-scalar.adoc>
.
- For the purposes of scalar crypto, an SBox is an 8-bit to 8-bit
function - other bit widths occur in cryptography more generally, but I'll
limit the scope to what's relevant here. Different algorithms (AES, SM4)
define this function differently, and it is critical to test every possible
input to the SBox. Our aes32*,aes64* and sm4* instructions all use the
SBoxes relevant to their algorithms.
- For the aes32 and sm4 instructions, the value fed into the SBox is
selected by selecting the bs'th byte of GPR[rs2]. Hence the relevant
coverage point is to check for every aes32*/sm4 instruction, that all
256 possible values of 0xFF & (GPR[rs2] >> (8*bs)) are covered.
- For the aes64 instructions, it's a bit different. Looking at line 6
of Figure 3 in v0.8.1 of the scalar crypto specification, we see that
there is a 64-bit value, which is split into bytes, and each byte has the
SBox function applied. We need to check that each of those bytes has all
0..255 values covered. The simplest way to do this would be to start
at 0x0000_0000_0000_0000 and add 0x0101_0101_0101_0101 256 times.
Does that explanation make sense?
Cheers,
Ben
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#172 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJU5MMIVUZW2ZSRIDQLTBTGPTANCNFSM4YIXJW7Q>
.
|
|
I was giong through the aes32dsi test. Now the RV test-case for this is currently written as:
However, the same instruction can exist if one implements just the sub-extension Zkn. Which means in addition to the above you will also need the following macro to enable the same test-cases:
You will need to do the same for all the other instructions as well. @ben-marshall does spike (or a variant) have K support yet ? Would help validate the references. |
|
Thanks for detailed looking. |
|
would we just have different directories with tests in - i.e. duplicate the tests in the different dirs? or would all tests for K be in one dir and all tests for B be in another and those instructions that are shared would have duplicate tests in both K and B dirs? Our current approach is to just duplicate them in different dirs. |
In RISCOF, the RVTEST_CASE will be used to indicate under which conditions should the test be enabled. So in the aes32dsi.S test you would have both the following macros to enable the test when either "K" or "Zkn" exists in the ISA string: Similarly with tests that overlap with "B" and "K" would have another macro which checks for "B" extension in the ISA string as well. All the tests in the repo are written to be forward compatible with RISCOF. My suggestion was to ensure that the same compatibility applies to the K tests as well. If the spec itself is unclear about this do we want to wait until that happens before merging these tests so that there is less re-work later ? The current framework in this repo depends on directory structure to enable/disable suites (not tests) - which obviously doesn't scale well for subsets that overlap. Even if we duplicate the tests in multiple folders (via soft-links to avoid syncing issues) I believe we need to prevent running those tests multiple times. Having a flat-directory structure (with some standard file naming convention) would probably be better. on the topic of file-naming convention, I see that all the K tests have a prefix "I-" in their names. Can you remove that prefix or rename it to "K-" ? |
|
ok thanx for the info. |
|
Well done; good answers, good explanations, good reasoning.
…On Mon, Mar 8, 2021 at 12:04 AM Neel Gala ***@***.***> wrote:
@Imperas <https://github.com/Imperas>
How would we specify that a test was used for several different
intersections and unions of different subsets and extensions - like those
shared by K and B, and K and its subsets?
In RISCOF, the RVTEST_CASE will be used to indicate under which conditions
should the test be enabled. So in the aes32dsi.S test you would have both
the following macros to enable the test when either "K" or "Zkn" exists in
the ISA string:
RVTEST_CASE(0,"//check ISA:=regex(.*32.*);check ISA:=regex(.*I.*K.*);def TEST_CASE_1=True;",aes32dsi)
RVTEST_CASE(0,"//check ISA:=regex(.*32.*);check ISA:=regex(.*I.*ZKn.*);def TEST_CASE_1=True;",aes32dsi)
Similarly with tests that overlap with "B" and "K" would have another
macro which checks for "B" extension in the ISA string as well.
All the tests in the repo are written to be forward compatible with
RISCOF. My suggestion was to ensure that the same compatibility applies to
the K tests as well. If the spec itself is unclear about this do we want to
wait until that happens before merging these tests so that there is less
re-work later ?
The current framework in this repo depends on directory structure to
enable/disable suites (not tests) - which obviously doesn't scale well for
subsets that overlap. Even if we duplicate the tests in multiple folders
(via soft-links to avoid syncing issues) I believe we need to prevent
running those tests multiple times. Having a flat-directory structure (with
some standard file naming convention) would probably be better.
on the topic of file-naming convention, I see that all the K tests have a
prefix "I-" in their names. Can you remove that prefix or rename it to "K-"
?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#172 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJTHE7YURPPJVBYJ5ZLTCSAHLANCNFSM4YIXJW7Q>
.
|
|
In terms of a path forward for these then, it seems like @neelgala has nicely laid out any remaining tweaks to the generated tests. My only remaining concern is that these are for All of this seems like a lot of wrangling to get an old test set merged. Would it be easier for everyone (especially Imperas) if while addressing Neel's comments, the tests were re-generated to fit the latest Does that sound reasonable? I don't want to undermine the effort that has gone into this PR or presume to ask for any more in getting it merged. To me it just seems logical to merge tests for the latest version of the spec. Thanks, |
|
Yes we are taking the input from all to do with data, naming, version, configs, etc. - all good and expected feedback and tweaking and we will issue new pull request/update in due course - and Ben, yes it will be for 0.9.0. thanks. Simon |
|
Can I confirm that this should be closed because it was superseded by pull #177 ? |
As response to the request from the RISC-V K Crypto task group this pull request is Imperas' donation of the Imperas RISC-V K Crypto (Scalar) 0.7.2 Architectural Test suites that are provided under Apache 2.0 open source. They have assertions that have been validated with riscvOVPsim and with signature references generated by riscvOVPsim.
This pull request includes all the source of the tests, the signatures, coverage reports and instructions to reproduce the runs with the riscv-target riscvOVPsim (freeware simulator) available from its github.