[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

d7483116b80bf56ce2b8e665c679872de3f3794d

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

• 77abd32245 [spec] Update hmac spec to clarify WIPE_SECRET behavior. --> specification clarification
• 24a4034b7d [hmac,waiver] Update HMAC AscentLint waiver
  85f443c6ca [hmac,data/doc] Add features and update documentation
  e475a921cf [hmac,dv] Support new HMAC IP in DV
  6e59b6c26d [hmac,dv] Add DPI C support for HMAC SHA-384/512
  37a1499bcc [dv,cryptoc_dpi] Test utility
  b90a8cc016 [dv,cryptoc_dpi] SHA-384/512 support
  7ef06291d3 [hmac,core] Update HMAC description
  9b804a3c14 [hmac,regs] Extend CSRS for wider digest/key length
  136ca2a0e6 [hmac,rtl] RTL changes to support configurable wider digest --> All those commits are part of #21604:
  • Documentation updated except for #21711 and #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 #22102 tracks the verification of this as part of M4.
• dd041b9180 [dv,otp_ctrl] Fix issues related to random resets --> no RTL nor specification changed
• 7e196e6f07 [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 (#21815), which is tracked for M4.
• 06fad37a11 [hmac] Fix Verilator lint error --> RTL changed but no functional difference
• 6a9f12ca40 [hmac,verilator] Remove lint waivers for nonexistent file --> no RTL nor specification changed
• 0ebe13daa5 [hmac/doc] Document context save & restore
  d739893419 [hmac/dv] Randomly save & restore in wr_msg task
  f151ba86b9 [hmac/dv] Add randomization knob to exercise save & restore
  064b6dd623 [hmac] Signal done also when told to stop and hash is complete
  6ceb5424c1 [hmac] Implement hash stop command
  252361c75b [prim_sha2] Add hash_running_o
  5f5355b049 [hmac] Remove event_intr signal
  493229ae92 [hmac] Implement hash continue command
  096c4570d7 [hmac] Implement hash continuing in hmac_core
  e97ec534d3 [hmac/dv] Add functions to stop and continue
  582fb60573 [hmac] Add stop and continue bits to CMD CSR
  bba47944c2 [prim_sha2] Add hash_continue_i
  a62a1b0c31 [hmac/dv] Add task to write the MSG_LENGTH CSR
  a510490f68 [hmac/dv] Scoreboard changes for making MSG_LENGTH CSR writable
  1930c0eede [hmac] Make MSG_LENGTH CSRs SW-writable
  b2db371776 [hmac] Remove assertion that message_length must be stable
  e3b4afe66f [hmac/dv] Add task that outputs the value read from message length CSR
  d0099ed5d7 [hmac/dv] Print digest registers in csr_rd_digest
  5ad9a69809 [hmac/dv] Print expected digest when calculating it
  0bb77b08df [hmac/dv] Scoreboard change for making DIGEST CSR SW-writable
  446614ee49 [hmac/dv] Add task to write DIGEST CSR
  acf14b5f11 [hmac] Connect digest SW outputs to HW
  10e3b278ee [hmac] Make DIGEST CSR SW-writable
  778250b9b4 [hmac/dv] Exclude CFG.digest_swap CSR field from automated write tests
  4f391184b1 [hmac/dv] Exclude WIPE_SECRET CSR from automated write tests
  67de537fac [prim_sha2] Make digest writable from input while disabled
  --> All those commits are part of #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 #21708 tracks the verification of this feature as part of M4.
• cce79e2819 [hmac] Bump version to 2.0.0 and reset development stages
  --> no RTL nor specification changed
• 2290dcc119 [hw,hmac,rtl] Remove unused SHA2 code --> RTL changed but no functional difference
• 37600340d1 [hmac,syn] Add cfg and constraints to support HMAC synthesis
  --> no RTL nor specification changed
• 8386553dc3 [hmac,dv] Update HMAC UNR-generated coverage exclusions --> no RTL nor specification changed
• 58fc788547 [hmac/rtl] Update HMAC to instantiate SHA-2 prim --> RTL changed but no functional difference
• 61a237e197 [util/reggen] reverse order of substruct generation --> RTL changed but no functional difference
• fc8484601e [reggen,hw] Create index parameter for registers windows --> RTL changed but no functional difference
• de31bdf1c2 [reggen] Remove the devmode input --> RTL changed but no functional difference
• 963a5006cc [doc] Minor tweak to md sanitisation code --> no RTL changed; spec docs changed but changes should be structural only
• 1b16ca2122 [reggen] Add mubi support SWAccess that sets/clears a reg --> RTL changed but no functional difference
• 59f8142826 [doc] Moved badges over to using hosted images --> no RTL nor specification changed
• dcd6d22553 [SiVal] Test plan updates for HMAC --> no RTL nor specification changed
• a8da33132e [doc] hmac registers and interfaces now use CMDGEN --> no RTL changed; spec docs changed but changes should be structural only
• 7688e714e8 [reggen] Add initial support for version and cip_id hjson fields
  --> no RTL changed
• fbd888eea8 Revert "[reggen] Add CIP_IDs and bump all major versions"
  --> reverts commit below
• 0ba10b3cd3 [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

• 49

  --> resolved in #21307; described above

• 10472

  --> ROM

• 15378

  --> parent issue of #21206; described below

• 16562

  --> not related to hmac

• 17845

  --> not related to hmac

• 19321

  --> cryptolib

• 20440

  --> SiVal

• 20968

  --> resolved in #21604; described above

• 21206

  --> resolved in #21809; described above

• 21462

  --> SiVal

• 21463

  --> SiVal

• 21464

  --> SiVal

• 21467

  --> SiVal

Currently open issues

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

• 270

  --> future release / backlog

• 777

  --> throughput test --> M5

• 1644

  --> not directly related to hmac

• 3479

  --> not directly related to hmac

• 6220

  --> ROM

• 7143

  --> not related to hmac

• 10137

  --> not related to hmac

• 12958

  --> future release / backlog

• 14518

  --> SW / M5

• 15590

  --> cryptolib

• 16767

  --> cryptolib

• 19549

  --> cryptolib

• 19797

  --> build system

• 20194

  --> SiVal

• 20996

  --> this issue

• 21031

  --> V1 signoff issue

• 21075

  --> SiVal

• 21076

  --> SiVal

• 21083

  --> SiVal

• 21222

  --> SiVal

• 21465

  --> SiVal

• 21466

  --> SiVal

• 21468

  --> SiVal

• 21666

  --> to be addressed by #22247

• 21708

  --> V2 & M4

• 21710

  --> D3 & M4

• 21711

  --> to be completed for M2

• 21815

  --> V2 & M4

• 21823

  --> build system

• 21936

  --> cryptolib

• 21937

  --> ROM

• 22102

  --> V2 & M4

• 22127

  --> cryptolib

Summary

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

• 21604: DV tracked for M4 in #22102. Except for the following opens, all D2-relevant checklist items can be checked:

  • [x] #21711
  • [x] #22247
  • [x] Link and track TODOs that were added in the code as part of this PR. --> done in #22247

• 21307: Documentation updated. DV tracked for M4 in #21708. All D2-relevant checklist items can be checked.

• 21809: Documentation updated. DV tracked for M4 in #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 https://github.com/lowRISC/opentitan/issues/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 https://github.com/lowRISC/opentitan/pull/21604#issue-2147283075). @andreaskurth noted that issues should be created to track those items. So far, I'v only found the following issues:

• https://github.com/lowRISC/opentitan/issues/21711
• https://github.com/lowRISC/opentitan/issues/22102
• https://github.com/lowRISC/opentitan/issues/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 https://github.com/lowRISC/opentitan/pull/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 https://github.com/lowRISC/opentitan/pull/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 https://github.com/lowRISC/opentitan/issues/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 https://github.com/lowRISC/opentitan/issues/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! :+1:

[vogelpi]

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