[aes, pre-dv] Pre-dv Verilator testbench for AES #11
Conversation
andrea-caforio
force-pushed
the
aes-pre-dv-tb
branch
2 times, most recently
from
dfa1ce6
to
26b1dd7
Compare
This commit contains a comprehensive pre-dv testbench for the AES IP block, invoking all the possible modes of operations (+GCM) with different key and inputs sizes. By default, the communication with the IP happens over the TLUL bus, but if a TLUL/shim adapter is configured, messages can be relayed through the shim first. There are two generic modules `tlul_adapter` and `tlul_delayer` that are agnostic to the actual testbench and can be reused in other testbenches that require a robust TLUL handler. This testbench mimics already existing pre-dv suites for Ascon and KMAC. Signed-off-by: Andrea Caforio <[email protected]>
andrea-caforio
force-pushed
the
aes-pre-dv-tb
branch
from
26b1dd7
to
ca13f34
Compare
There was a problem hiding this comment.
Thanks @andrea-caforio for your PR. I have some comments and questions but it's all minor. This looks nice!
| This directory contains the Verilator testbench for the TLUL shim adapter that is attached to the AES IP block. | ||
| Out of the box, the testbench contains test vectors for most of the salient use cases, nonetheless extending the testbench with further tests is straightforward as detailed below. | ||
| This directory contains the Verilator testbench for the AES IP block. | ||
| Out of the box, the testbench contains test vectors for most of the salient use cases, nonetheless extending the testbench with further tests is straightforward as detailed below. By default, communication with the IP happens over the TLUL bus. If a TLUL/shim adapter is available, messages can optionally be relayed by the shim. |
There was a problem hiding this comment.
Nit: line break after first sentence
| @@ -1,7 +1,7 @@ | |||
| # TLUL/Shim Verilator Testbench | |||
|
|
|||
| This directory contains the Verilator testbench for the TLUL shim adapter that is attached to the AES IP block. | |||
There was a problem hiding this comment.
Would it be possible to have simlink in the old position of the README.md pointing to the new position for now please? Because we have shared a reference with people. When merging this into master, we can remove the simlink (it won't be needed anymore then).
| @@ -18,7 +18,8 @@ module aes_tlul_shim_tb | |||
| localparam bit SecSkipPRNGReseeding = 0, | |||
| localparam bit EnableDataIntgGen = 1, | |||
| localparam bit EnableRspDataIntgCheck = 1, | |||
| localparam bit DelayerEnable = 0 | |||
| localparam bit DelayerEnable = 0, | |||
| localparam bit ShimEnable = 0 | |||
There was a problem hiding this comment.
Maybe enable the Shim by default to make it easier for people because that's what they currently need.
| .rst_ni ( rst_ni ), | ||
| .tl_i ( tl_d2h_delayed ), | ||
| .tl_o ( tl_h2d ), | ||
| .dv_i ( ~bus_req.c_dpi_load ), |
There was a problem hiding this comment.
I would create a separate signal for this valid bit to make it more obvious what it is. And then comment on how the valid depends on the c_dpi_load. I think this would help people to understand how the shim is meant to be used.
| .id_i ( shim_req.id ) | ||
| ); | ||
| if (ShimEnable) begin : gen_tlul_adapter_shim | ||
| tlul_adapter_shim #( |
There was a problem hiding this comment.
Maybe add a comment here saying that the shim expects the valid-hold protocol input and converts this to TL-UL internally.
|
|
||
| `include "prim_assert.sv" | ||
|
|
||
| module tlul_adapter |
There was a problem hiding this comment.
I am not fully happy with the name here but I don't have a better one. Basically this is just the thing to convert the generic TB requests to TLUL. And the other one converts valid-hold protocol requests to TLUL. I wouldn't rename the other one but maybe this one here should just be a tlul_adapter_tb_reqs or aes_tb_reqs_tlul_adapter? I don't really know. What is your view on this?
| // A valid request is indicated with `tlul_en_i` and is acknowledged once `tlul_wait_o` is unset. | ||
| input logic tlul_en_i, | ||
| output logic tlul_wait_o, | ||
|
|
||
| // Generic read/write request. | ||
| input logic [top_pkg::TL_AW-1:0] tlul_addr_i, | ||
| input logic tlul_write_i, | ||
| input logic [top_pkg::TL_DW-1:0] tlul_wdata_i, | ||
| output logic [top_pkg::TL_DW-1:0] tlul_rdata_o, | ||
| output logic tlul_error_o |
There was a problem hiding this comment.
Since this is the generic TB reqs "interface", should we maybe drop the tlul prefix?
| logic pending_d, pending_q; | ||
| logic req_ack, resp_ack; | ||
|
|
||
| assign req_ack = tlul_en_i & tl_i.a_ready & tl_o.a_valid; |
There was a problem hiding this comment.
The whole logic in here is very similar to the tlul_adapter_shim but that's probably because the TB requests have been designed with the tlul_adapter_shim in mind. I think this is okay and still appreciate having this very simple option available, too.
| .id_i ( '0 ) | ||
| ); | ||
| end else begin : gen_tlul_adapter | ||
| tlul_adapter #( |
There was a problem hiding this comment.
Question: what happens for the final read_caliptra(CALIPTRA_NAME_0_OFFSET), calls in case the shim is not there?
This commit contains a comprehensive pre-dv testbench for the AES IP block, invoking all the possible modes of operations (+GCM) with different key and inputs sizes. By default, the communication with the IP happens over the TLUL bus, but if a TLUL/shim adapter is configured, messages can be relayed through the shim first.
There are two generic modules
tlul_adapterandtlul_delayerthat are agnostic to the actual testbench and can be reused in other testbenches that require a robust TLUL handler.This testbench mimics already existing pre-dv suites for Ascon and KMAC.
I'm opening this PR here instead of upstream due to the GCM dependency