[hmac,dv/rtl] Implement DV checks for new digest sizes/key lengths #23108
Conversation
gdessouky
requested review from
marnovandermaas,
martin-velay and
andreaskurth
and removed request for
a team and
marnovandermaas
gdessouky
force-pushed
the
hmac_secret_wipe
branch
from
246d91a
to
7341451
Compare
| @@ -367,19 +367,15 @@ constraint key_digest_c { | |||
| // since HMAC digest size is max 512 bits. | |||
There was a problem hiding this comment.
Do you plan to extend this task later to support 384/512 digest sizes? (remind also to release constraints into hmac_test_vectors_sha_vseq.sv file)
There was a problem hiding this comment.
yes, the compare_digest in hmac_base_vseg.sv is only relevant anyway for hmac_test_vectors_sha_vseq.sv, so we'll have to extend it once we release the constraints in these tests (captured already in issue #22932), but will add a TODO there with link to issue # as well.
hw/ip/hmac/dv/env/hmac_scoreboard.sv
Outdated
| if (digest_idx < 8) begin | ||
| `DV_CHECK_EQ(real_digest_val, exp_digest[digest_idx]); | ||
| end else begin | ||
| `DV_CHECK_EQ(real_digest_val, '0); | ||
| end | ||
| end else if (expected_digest_size == SHA2_512) begin |
There was a problem hiding this comment.
Can you swap the section related to 384 with 512, to be consistent? I can live with that, but still 😅
There was a problem hiding this comment.
They can actually be merged as well.
There was a problem hiding this comment.
RTL and spec changes LGTM, thx @gdessouky. I'll let the DV experts judge the DV changes, though I didn't notice any obvious mistake from a quick glance
gdessouky
force-pushed
the
hmac_secret_wipe
branch
3 times, most recently
from
fb2562e
to
526da58
Compare
gdessouky
force-pushed
the
hmac_secret_wipe
branch
from
526da58
to
fa5729f
Compare
This removes the clearing of the additional digest values for digest sizes 256 and 384, which also simplifies the DV checks across different configuration changes. Signed-off-by: Ghada Dessouky <[email protected]>
This updates the scoreboard for checks for different digest sizes and key lengths, in normal operation and in secure wiping. Signed-off-by: Ghada Dessouky <[email protected]>
This updates the HMAC documentation to correspond with the changes in the commits above as well as commits minor fixes. Signed-off-by: Ghada Dessouky <[email protected]>
gdessouky
force-pushed
the
hmac_secret_wipe
branch
from
fa5729f
to
ae046e7
Compare
|
Thanks @martin-velay for the DV comments, I've addressed them in my last forced push since yesterday. Rebasing and running CI again and hoping for a better CI day today :-) Would be great to merge soon; I have many dependent PRs I'm afraid it will get confusing soon... |
|
The failing test in CI has nothing to do with the changes in this PR, can we please merge :-) @andreaskurth? |
|
Agreed, thanks @gdessouky! |
This PR implements scoreboard checks for the new HMAC digest sizes and key lengths for normal operation and whenever secure wiping is triggered. For ease of DV checks and consistency across configuration changes where previous digests remain held in CSRs until HMAC starts a new operation, this PR also removes the clearing of redundant digest values for the smaller digest sizes in the RTL. It also updates the documentation to clarify that SW should only read the relevant digest CSRs for the digest size configured - @ballifatih (only read digests 0..7 for SHA-2 256 and digests 0..11 for SHA-2 384, i.e., do not assume the redundant digest values in these sizes will be all-zero.)
This should also fix the failing hmac_wipe_secret test (#23053) and close other pending DV TODOS.