[hmac] D2(S) Signoff

[msfschaffner]

Description

Ensure D2 signoff criteria are fulfilled after focus area changes have landed.

[andreaskurth]

Commits since Earlgrey-ES tapeout

git rev-parse HEAD

d748311

git log Earlgrey-M2.5.2-RC0..HEAD --oneline hw/ip/hmac

• 77abd32 [spec] Update hmac spec to clarify WIPE_SECRET behavior.
  --> specification clarification
• 24a4034 [hmac,waiver] Update HMAC AscentLint waiver
  85f443c [hmac,data/doc] Add features and update documentation
  e475a92 [hmac,dv] Support new HMAC IP in DV
  6e59b6c [hmac,dv] Add DPI C support for HMAC SHA-384/512
  37a1499 [dv,cryptoc_dpi] Test utility
  b90a8cc [dv,cryptoc_dpi] SHA-384/512 support
  7ef0629 [hmac,core] Update HMAC description
  9b804a3 [hmac,regs] Extend CSRS for wider digest/key length
  136ca2a [hmac,rtl] RTL changes to support configurable wider digest
  --> All those commits are part of [hmac] Implementation of wider digest & configurable key length #21604:
  • Documentation updated except for [hmac,doc] Consolidate documentation changes #21711 and [hmac] Coding style and minor fixes #22247.
  • A couple of TODOs were added that need to be linked to issues and tracked.
  • There is preliminary DV code to verify this. Issue [hmac/dv] Verify wider digest & configurable key length #22102 tracks the verification of this as part of M4.
• dd041b9 [dv,otp_ctrl] Fix issues related to random resets
  --> no RTL nor specification changed
• 7e196e6 [hmac] Change fifo_empty interrupt type to status
  --> Change of interrupt type to status. Documentation updated. One TODO item added, which links the verification of this change ([hmac] Verify FIFO empty status interrupt #21815), which is tracked for M4.
• 06fad37 [hmac] Fix Verilator lint error
  --> RTL changed but no functional difference
• 6a9f12c [hmac,verilator] Remove lint waivers for nonexistent file
  --> no RTL nor specification changed
• 0ebe13d [hmac/doc] Document context save & restore
  d739893 [hmac/dv] Randomly save & restore in wr_msg task
  f151ba8 [hmac/dv] Add randomization knob to exercise save & restore
  064b6dd [hmac] Signal done also when told to stop and hash is complete
  6ceb542 [hmac] Implement hash stop command
  252361c [prim_sha2] Add hash_running_o
  5f5355b [hmac] Remove event_intr signal
  493229a [hmac] Implement hash continue command
  096c457 [hmac] Implement hash continuing in hmac_core
  e97ec53 [hmac/dv] Add functions to stop and continue
  582fb60 [hmac] Add stop and continue bits to CMD CSR
  bba4794 [prim_sha2] Add hash_continue_i
  a62a1b0 [hmac/dv] Add task to write the MSG_LENGTH CSR
  a510490 [hmac/dv] Scoreboard changes for making MSG_LENGTH CSR writable
  1930c0e [hmac] Make MSG_LENGTH CSRs SW-writable
  b2db371 [hmac] Remove assertion that message_length must be stable
  e3b4afe [hmac/dv] Add task that outputs the value read from message length CSR
  d0099ed [hmac/dv] Print digest registers in csr_rd_digest
  5ad9a69 [hmac/dv] Print expected digest when calculating it
  0bb77b0 [hmac/dv] Scoreboard change for making DIGEST CSR SW-writable
  446614e [hmac/dv] Add task to write DIGEST CSR
  acf14b5 [hmac] Connect digest SW outputs to HW
  10e3b27 [hmac] Make DIGEST CSR SW-writable
  778250b [hmac/dv] Exclude CFG.digest_swap CSR field from automated write tests
  4f39118 [hmac/dv] Exclude WIPE_SECRET CSR from automated write tests
  67de537 [prim_sha2] Make digest writable from input while disabled
  --> All those commits are part of [hmac] Initial implementation of save & restore #21307:
  • The documentation has been updated to include the added feature.
  • Two TODO items were added, and these should get resolved during the addition of the SHA-384/512 support.
  • There is preliminary DV code to verify this but it's currently not enabled. Issue [hmac/dv] Verify save & restore #21708 tracks the verification of this feature as part of M4.
• cce79e2 [hmac] Bump version to 2.0.0 and reset development stages
  --> no RTL nor specification changed
• 2290dcc [hw,hmac,rtl] Remove unused SHA2 code
  --> RTL changed but no functional difference
• 3760034 [hmac,syn] Add cfg and constraints to support HMAC synthesis
  --> no RTL nor specification changed
• 8386553 [hmac,dv] Update HMAC UNR-generated coverage exclusions
  --> no RTL nor specification changed
• 58fc788 [hmac/rtl] Update HMAC to instantiate SHA-2 prim
  --> RTL changed but no functional difference
• 61a237e [util/reggen] reverse order of substruct generation
  --> RTL changed but no functional difference
• fc84846 [reggen,hw] Create index parameter for registers windows
  --> RTL changed but no functional difference
• de31bdf [reggen] Remove the devmode input
  --> RTL changed but no functional difference
• 963a500 [doc] Minor tweak to md sanitisation code
  --> no RTL changed; spec docs changed but changes should be structural only
• 1b16ca2 [reggen] Add mubi support SWAccess that sets/clears a reg
  --> RTL changed but no functional difference
• 59f8142 [doc] Moved badges over to using hosted images
  --> no RTL nor specification changed
• dcd6d22 [SiVal] Test plan updates for HMAC
  --> no RTL nor specification changed
• a8da331 [doc] hmac registers and interfaces now use CMDGEN
  --> no RTL changed; spec docs changed but changes should be structural only
• 7688e71 [reggen] Add initial support for version and cip_id hjson fields
  --> no RTL changed
• fbd888e Revert "[reggen] Add CIP_IDs and bump all major versions"
  --> reverts commit below
• 0ba10b3 [reggen] Add CIP_IDs and bump all major versions
  --> reverted by commit tabove

Common to all changes:

• Lint passes.
• CDC/RDC is not done at block level.
• Area is being checked as part of Earlgrey-PROD.M2.
• Timing will be checked later.

Issues closed since the Earlgrey-ES tapeout

https://github.com/lowRISC/opentitan/issues?q=is%3Aissue+is%3Aclosed+closed%3A%3E2023-06-27+hmac

• [hmac] Add support for SHA / HMAC context switch #49
  --> resolved in [hmac] Initial implementation of save & restore #21307; described above
• Add hmac_sha256() #10472
  --> ROM
• [chip] Convert Edge-triggered status interrupt to Level #15378
  --> parent issue of [hmac] Change fifo_empty interrupt from event to status type #21206; described below
• Flow error that dc is unsupported for job runtime extraction #16562
  --> not related to hmac
• [component] Why opentitan need to do a lot of functional safety sms? #17845
  --> not related to hmac
• [crypto] Enforce minimum key size for HMAC. #19321
  --> cryptolib
• [chip-test] chip_sw_hmac_enc #20440
  --> SiVal
• [hmac] Evaluate whether wider SHA2-384 and SHA2-512 modes should be added #20968
  --> resolved in [hmac] Implementation of wider digest & configurable key length #21604; described above
• [hmac] Change fifo_empty interrupt from event to status type #21206
  --> resolved in [hmac] Change fifo_empty interrupt type to status #21809; described above
• [chip-test] chip_sw_hmac_enc #21462
  --> SiVal
• [chip-test] chip_sw_hmac_idle #21463
  --> SiVal
• [chip-test] chip_sw_hmac_sha2_stress #21464
  --> SiVal
• [chip-test] chip_sw_hmac_secure_wipe #21467
  --> SiVal

Currently open issues

https://github.com/lowRISC/opentitan/issues?q=is%3Aissue+is%3Aopen+hmac

• [reggen] Changing multireg structure #270
  --> future release / backlog
• [dv/hmac] enhancement on DV testbench #777
  --> throughput test --> M5
• Credit system in TL-UL to not block the fabric #1644
  --> not directly related to hmac
• [keymgr, kmac] Context Switch (save/restore) #3479
  --> not directly related to hmac
• [sw] polling in ROM #6220
  --> ROM
• [dv, test_vectors] Integrate AES test vectors into existing NIST vector testing package #7143
  --> not related to hmac
• [dv] The #1ps delay in dut_init means we never start a sequence item on the first clock edge #10137
  --> not related to hmac
• [prim] Packer empty signal #12958
  --> future release / backlog
• [dif] Add missing dif to access fault CSRs #14518
  --> SW / M5
• [crypto] Add sideloaded key support to the cryptolib. #15590
  --> cryptolib
• [tlul] Side-channel Hamming weight leakage of data on TL-UL #16767
  --> cryptolib
• [cryptolib] API cleanup #19549
  --> cryptolib
• [bazel, dvsim] Convert dvsim-executed tests to the new rules #19797
  --> build system
• [sival] prod_all_tests #20194
  --> SiVal
• [hmac] D2(S) Signoff #20996
  --> this issue
• [hmac] V1 Signoff #21031
  --> V1 signoff issue
• [sival] raw_all_tests #21075
  --> SiVal
• [sival] test_unlocked_all_tests #21076
  --> SiVal
• [sival] test_locked_all_tests #21083
  --> SiVal
• [chip-test] Incomplete SiVal Stage 1 and 2 tests #21222
  --> SiVal
• [chip-test] chip_sw_hmac_stress #21465
  --> SiVal
• [chip-test] chip_sw_hmac_endianness #21466
  --> SiVal
• [chip-test] chip_sw_hmac_error_conditions #21468
  --> SiVal
• [hmac/rtl] Fix suffixes of hmac_core's port names #21666
  --> to be addressed by [hmac] Coding style and minor fixes #22247
• [hmac/dv] Verify save & restore #21708
  --> V2 & M4
• [hmac,rtl] Handle incomplete message size after cmd.hash_stop #21710
  --> D3 & M4
• [hmac,doc] Consolidate documentation changes #21711
  --> to be completed for M2
• [hmac] Verify FIFO empty status interrupt #21815
  --> V2 & M4
• [bazel] bazelisk.sh unbound variable error when invoked with dvsim.py to run TLT #21823
  --> build system
• [cryptolib] support for EC key generation from provided DRBG #21936
  --> cryptolib
• [silicon_creator] namespace all silicon_creator drivers with scd_ prefix #21937
  --> ROM
• [hmac/dv] Verify wider digest & configurable key length #22102
  --> V2 & M4
• [cryptolib] implement software DRBG #22127
  --> cryptolib

Summary

Re RTL changes, there were three main sets of changes:

• [hmac] Implementation of wider digest & configurable key length #21604: DV tracked for M4 in [hmac/dv] Verify wider digest & configurable key length #22102. Except for the following opens, all D2-relevant checklist items can be checked:
  • [hmac,doc] Consolidate documentation changes #21711
  • [hmac] Coding style and minor fixes #22247
  • Link and track TODOs that were added in the code as part of this PR. --> done in [hmac] Coding style and minor fixes #22247
• [hmac] Initial implementation of save & restore #21307: Documentation updated. DV tracked for M4 in [hmac/dv] Verify save & restore #21708. All D2-relevant checklist items can be checked.
• [hmac] Change fifo_empty interrupt type to status #21809: Documentation updated. DV tracked for M4 in [hmac] Verify FIFO empty status interrupt #21815. All D2-relevant checklist items can be checked.

Re closed design issues since Earlgrey-ES tapeout, all D2-relevant changes are captured in the RTL changes above.

Re open design issues, there is only one: #21710 tracked for D3 & M4.

In conclusion, I think this analysis supports signing off hmac version 2.0.0 at D2 (version 1.0.0, which we forked from after Earlgrey-ES TO, was at D3) once the three opens above are done. Since hmac only features the BUS.INTEGRITY countermeasure, I think we can directly sign off at D2S.

[andreaskurth]

@vogelpi: May I ask you for feedback on my analysis and D2S signoff approval -- conditional on the completion of the three opens above -- if you agree?

[gdessouky]

Thanks @andreaskurth, I filed another issue now for an open design issue to track for D3 & M4 #22312

[vogelpi]

Thanks for preparing this @andreaskurth ! I've reviewed the big RTL PRs as well as the material you've put together and I agree to sign this of at D2S. It's unfortunate that there are still a couple of opens regarding the wider digests and configurable key length feature, including items relevant for D2 (documentation, block diagram). However, this particular change is well motivated and approved by key stakeholders prior to the implementation. So, I believe this is acceptable.

As for the timing checks, the new features are enabled by default and thus also synthesized for FPGA. Timing is still met and the runtime did not horribly increase. So it can be argued that this item of the checklist is met as well.

@gdessouky , I can see that in the PR adding support for wider digests there are about 6 unticked checkboxes (see #21604 (comment)). @andreaskurth noted that issues should be created to track those items. So far, I'v only found the following issues:

• [hmac,doc] Consolidate documentation changes #21711
• [hmac/dv] Verify wider digest & configurable key length #22102
• [hmac,rtl] Flag error for invalid digest size/key length configuration #22312

The other 3 items which include DIFs etc. seem to be untracked which is not ideal. This might get forgotten. Can you please create issues for these items and link them in the original PR message in #21604? Then it's obvious that all relevant follow-up items are tracked. Thanks!

[gdessouky]

Thanks @vogelpi and @andreaskurth!

PR #22247, once merged, would close one of the open issues relevant for D2. It fails CI in ROM tests that don't have to do with HMAC. I've force pushed after Rupert's review, and once you approve it can get merged.

I also put up now PR #22331 to update the documentation modulo the block diagrams (trying right now to find an efficient way to make minor edits to the svg diagrams) as well as the block diagrams. I could elaborate more on the SHA-2 engine and its 32-bit wrapper in the block diagram in a follow-up minor fix. This would close issue #21711.

Regarding the open checkboxes in the PR, the last 3 are covered by issue #22102 (ticked out 1 and consolidated last 2 into 1). I've created another dedicated issue to track the DIFs updates (checkbox 2); I assumed they'd need to be inherently changed anyway for the DV work to proceed at top-level, thanks for catching! I'll link the issues into the PR message.

[andreaskurth]

It's unfortunate that there are still a couple of opens regarding the wider digests and configurable key length feature, including items relevant for D2 (documentation, block diagram). However, this particular change is well motivated and approved by key stakeholders prior to the implementation. So, I believe this is acceptable.

These have now been addressed, and I updated the signoff analysis.

@gdessouky , I can see that in the PR adding support for wider digests there are about 6 unticked checkboxes (see #21604 (comment)). @andreaskurth noted that issues should be created to track those items. So far, I'v only found the following issues:
[...]
The other 3 items which include DIFs etc. seem to be untracked which is not ideal. This might get forgotten. Can you please create issues for these items and link them in the original PR message in #21604? Then it's obvious that all relevant follow-up items are tracked. Thanks!

As mentioned by @gdessouky, two previously untracked items are now complete and the extension of the DIF is tracked in #22332.

With that, I think we're ready to sign HMAC off at D2S. I'll create a PR. Thanks for your review and work on this, @vogelpi and @gdessouky! 👍

[vogelpi]

This sounds good to me, thanks for the feedback @gdessouky and @andreaskurth , great work!