[hmac,dv/rtl] Fix HMAC DV scoreboard synchronization #23220
Conversation
hw/ip/hmac/dv/env/hmac_scoreboard.sv
Outdated
| // TODO: check if we need this block at this point | ||
| // update_err_intr_code(SwInvalidConfig); | ||
| // hash_start is blocked from HMAC because of invalid config | ||
| // hmac_start = 1'b0; |
There was a problem hiding this comment.
Why have you added these lines to keep them commented out? Is there a reason in the future or is it just an oversight?
There was a problem hiding this comment.
I might need them for the future, I have shrunk it down to one TODO line in my upcoming push, this should be fine for the time being while the DV is still stabilizing to help document some WIP.
hw/ip/hmac/dv/env/hmac_scoreboard.sv
Outdated
| if (digest_idx % 2) begin | ||
| `DV_CHECK_NE(real_digest_val, exp_digest[digest_idx-1]) | ||
| // Update new digest data to the exp_digest variable. | ||
| ` uvm_info(`gfn, $sformatf("updating digest to read value after wiping 0x%0h", |
There was a problem hiding this comment.
- On several places you have a back-tick followed by an extra space, which is not needed
- verify alignment
hw/ip/hmac/dv/env/hmac_scoreboard.sv
Outdated
| if (ral.intr_state.hmac_done.get_mirrored_value() == 1) break; | ||
| // break if hmac is done or if invalid config error is triggered | ||
| if (ral.intr_state.hmac_done.get_mirrored_value() == 1 || | ||
| (ral.intr_state.hmac_err.get_mirrored_value() == 1 && |
There was a problem hiding this comment.
Indentation should be reviewed
There was a problem hiding this comment.
RTL change in second-to-last commit LGTM; I'll let others review the DV changes :-) Thx @gdessouky
6650850 to
ecaad66
Compare
|
Thanks @martin-velay and @andreaskurth, addressed the DV cosmetics in my push now. Hoping for green soon! |
72a5c3e to
6499811
Compare
Signed-off-by: Ghada Dessouky <gdessouky@google.com>
|
Please don't merge this one since it constrains the digest sizes again and I want their smoke checks enabled for upcoming RTL PRs. I'll push shortly here to release the constraints while disabling |
This fixes DV synchronization checks for SHA-2 256 such that all CSRs and predictions match. This also fixes FIFO checks according to the FIFO changes implemented to adapt to multiple digest sizes, and disables do_read_check to be able to release the constraints on digest size and key length until synchronization issues are fixed for 384 and 512. Signed-off-by: Ghada Dessouky <gdessouky@google.com>
- this fix will be in a coming PR by Ghada lowRISC#23220 Signed-off-by: Martin Velay <mvelay@lowrisc.org>
|
Just discussed out-of-band with @gdessouky: it's fine to merge now (the comment above this one is not up-to-date anymore). |
This PR will only have 2 commits to review once this PR #23108 is merged. This introduces changes in the HMAC DV to enable
do_read_checkthat was commented out #22578 and achieve synchronization with how the FIFO is verified vs the CSRs being read and compared but only for SHA-2 256. I have disableddo_read_checkin the meantime to keep the constraints unreleased on the digest sizes and key lengths, until I fix the synchronization issues for 384/512. With this PR, the block testhmac_errorandhmac_stress_allstill fail, but have been root-caused and will be fixed in upcoming PRs.