-
Notifications
You must be signed in to change notification settings - Fork 762
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
[kmac/dv] initial version of KMAC smoke test #4674
Conversation
csr_wr(.csr(ral.prefix_7), .value(prefix_arr[7])); | ||
csr_wr(.csr(ral.prefix_8), .value(prefix_arr[8])); | ||
csr_wr(.csr(ral.prefix_9), .value(prefix_arr[9])); | ||
csr_wr(.csr(ral.prefix_10), .value(prefix_arr[10])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider to use loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, I'll update it to use a similar loop to what you linked to below
csr_wr(.csr(ral.key_share0_0), .value(key_share0[0])); | ||
csr_wr(.csr(ral.key_share0_1), .value(key_share0[1])); | ||
csr_wr(.csr(ral.key_share0_2), .value(key_share0[2])); | ||
csr_wr(.csr(ral.key_share0_3), .value(key_share0[3])); | ||
csr_wr(.csr(ral.key_share0_4), .value(key_share0[4])); | ||
csr_wr(.csr(ral.key_share0_5), .value(key_share0[5])); | ||
csr_wr(.csr(ral.key_share0_6), .value(key_share0[6])); | ||
csr_wr(.csr(ral.key_share0_7), .value(key_share0[7])); | ||
csr_wr(.csr(ral.key_share0_8), .value(key_share0[8])); | ||
csr_wr(.csr(ral.key_share0_9), .value(key_share0[9])); | ||
csr_wr(.csr(ral.key_share0_10), .value(key_share0[10])); | ||
csr_wr(.csr(ral.key_share0_11), .value(key_share0[11])); | ||
csr_wr(.csr(ral.key_share0_12), .value(key_share0[12])); | ||
csr_wr(.csr(ral.key_share0_13), .value(key_share0[13])); | ||
csr_wr(.csr(ral.key_share0_14), .value(key_share0[14])); | ||
csr_wr(.csr(ral.key_share0_15), .value(key_share0[15])); | ||
// share 0 | ||
csr_wr(.csr(ral.key_share1_0), .value(key_share1[0])); | ||
csr_wr(.csr(ral.key_share1_1), .value(key_share1[1])); | ||
csr_wr(.csr(ral.key_share1_2), .value(key_share1[2])); | ||
csr_wr(.csr(ral.key_share1_3), .value(key_share1[3])); | ||
csr_wr(.csr(ral.key_share1_4), .value(key_share1[4])); | ||
csr_wr(.csr(ral.key_share1_5), .value(key_share1[5])); | ||
csr_wr(.csr(ral.key_share1_6), .value(key_share1[6])); | ||
csr_wr(.csr(ral.key_share1_7), .value(key_share1[7])); | ||
csr_wr(.csr(ral.key_share1_8), .value(key_share1[8])); | ||
csr_wr(.csr(ral.key_share1_9), .value(key_share1[9])); | ||
csr_wr(.csr(ral.key_share1_10), .value(key_share1[10])); | ||
csr_wr(.csr(ral.key_share1_11), .value(key_share1[11])); | ||
csr_wr(.csr(ral.key_share1_12), .value(key_share1[12])); | ||
csr_wr(.csr(ral.key_share1_13), .value(key_share1[13])); | ||
csr_wr(.csr(ral.key_share1_14), .value(key_share1[14])); | ||
csr_wr(.csr(ral.key_share1_15), .value(key_share1[15])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use loop like this
opentitan/hw/ip/keymgr/dv/env/seq_lib/keymgr_base_vseq.sv
Lines 170 to 176 in c00e37f
foreach (sw_share_output[i, j]) begin | |
string csr_name = $sformatf("sw_share%0d_output_%0d", i, j); | |
uvm_reg csr = ral.get_reg_by_name(csr_name); | |
csr_rd(.ptr(csr), .value(sw_share_output[i][j])); | |
`uvm_info(`gfn, $sformatf("%0s: 0x%0h", csr_name, sw_share_output[i][j]), UVM_HIGH) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh that's a very good point, I was trying to come up with some macro-based approach for this, but looping like what you do is definitely the best approach here.
Thanks for the suggestion!
// check that there is actually room in the fifo before we start writing anything | ||
wait_fifo_has_capacity(); | ||
// can't use `DV_CHECK_RANDOMIZE_FATAL, so need a workaround | ||
`DV_CHECK_FATAL(randomize(fifo_addr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we should use DV_CHECK_MEMBER_RANDOMIZE_FATAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will update this accordingly.
// Set `left_right` to 1'b1 for left encoding, and 1'b0 for right encoding | ||
// | ||
// Input `q` is assumed to be empty | ||
function void encode(bit [2039:0] x, bit left_right, ref bit [7:0] q[$]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, maybe make 2040 a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I can do that (would probably be clearer that way) ! thanks for the suggestion
// wait for all msgfifo accesses to complete | ||
// | ||
// TODO: is there a better way of doing this? | ||
wait_no_outstanding_access(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment - I think the tl_access has a non_blocking option, https://github.com/lowRISC/opentitan/blob/master/hw/dv/sv/cip_lib/cip_base_vseq.sv#L102
Right now it defaults to blocking, so i believe there is no need to wait no outstanding.
But would be necessary if you set the non-blocking to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup you're absolutely right!
I intend to randomize blocking/nonblocking accesses in a future PR, so the call to wait_no_outstanding_access
would become "relevant" then.
Let me comment out this function call for now with a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
latest force push fixes failures in |
hw/ip/kmac/dv/env/kmac_env_pkg.sv
Outdated
// number of supported interrupts | ||
parameter int KMAC_NUM_INTR = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: An alternate approach is to add KmacNumIntrs
as the last literal of kmac_intr_e
(see UART for example), which has a couple of advantages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are some benefits of doing this? i'm not sure I fully understand how this would work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Enum literal is an immutable const, hence logically equivalent to a localparam
- Looping through an enum is easy (use this literal as an exit condition). Without it, you will need to add a loop counter
- No need to adjust the value of the parameter when interrupts are added / removed (forgetting to do that adds a small debug overhead)
- Adds the ability to set an 'illegal' value to an enum variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense!
I updated the enum to add this value at the very end.
`uvm_info(`gfn, $sformatf("kmac_en: %0b", kmac_en), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("xof_en: %0b", xof_en), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("strength: %0s", strength.name()), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("keccak_block_size: %0d", keccak_block_size), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("hash_mode: %0s", hash_mode.name()), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("msg_endianness: %0b", msg_endian), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("state_endianness: %0b", state_endian), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("en_sideload: %0b", en_sideload), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("key_len: %0s", key_len.name()), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("output_len: %0d", output_len), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("fname_arr: %0p", fname_arr), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("fname: %0s", str_utils_pkg::bytes_to_str(fname_arr)), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("custom_str_arr: %0p", custom_str_arr), UVM_HIGH) | ||
`uvm_info(`gfn, $sformatf("custom_str: %0s", str_utils_pkg::bytes_to_str(custom_str_arr)), UVM_HIGH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend adding a convert2string
function and call it here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, thanks!
custom_str_arr.size() == custom_str_len; | ||
} | ||
|
||
// constrains N and S to only be valid ASCII characters [A-Z,a-z] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// constrains N and S to only be valid ASCII characters [A-Z,a-z] | |
// constrains N and S to only be valid alphabet letters & space [A-Za-z ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
// general function to swap endianness of a byte array (such as msg/digest). | ||
// | ||
// This function preserves word ordering, and only endian-swaps the bytes in each word. | ||
// | ||
// e.g. if `arr[] = '{'h00, 'h01, 'h02, 'h03, 'h10, 'h11, 'h12, 'h13}`, the function will produce: | ||
// `'{'h03, 'h02, 'h01, 'h00, 'h13, 'h12, 'h11, 'h10}` | ||
function void endian_swap(ref bit [7:0] arr[]); | ||
arr = {<< byte {arr}}; | ||
arr = {<< 32 {arr}}; | ||
endfunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like we could move this to dv_utils_pkg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will do so. it seems like the dv_utils_pkg
already has some endian functionality contained, so would be good to aggregate them all.
private CI failing is due to CSR test failures, #4515 resolves these issues so will need to wait until that gets merged. |
This commit adds a `bytes_to_str` function in the `str_utils_pkg` that allows us to convert a bytestring back into string representation. Signed-off-by: Udi Jonnalagadda <udij@google.com>
This commit updates the KMAC testplan wording. Previously, it stated that we only manually squeeze more output data if the desired output length is greater than 1600 bits. This has been fixed to reflect that we only manually squeeze output data if the desired output length is greater than the keccak block size (168 or 136 bytes). Signed-off-by: Udi Jonnalagadda <udij@google.com>
This commit updates the KMAC DPI package to add function name and customization string inputs to the CSHAKE and KMAC functions. Signed-off-by: Udi Jonnalagadda <udij@google.com>
This commit adds the initial version of KMAC smoke test. There are some TODO items remaining that will be handled in the future, such as adding random sideload key injections, and moving most of the checking logic into the scoreboard once it is fleshed out. Signed-off-by: Udi Jonnalagadda <udij@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can revisit the str bytes conversion later. The rest LGTM.
sgtm, thanks for the review! |
This PR adds in the initial draft of the KMAC smoke test, with some additional enhancement commits needed to support it.
The first commit adds a function to the
str_utils_pkg
to convert a bytestring into a string representation.The second commit updates some wording in the testplan on the condition under which we manually squeeze more output data.
The third commit updates the kmac DPI package to add function name and customization string inputs to the CSHAKE and KMAC functions.
Finally, the last commit adds the actual testbench code to implement the smoke test.