Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[csrng] V2S Signoff #22467

Closed
andreaskurth opened this issue Apr 8, 2024 · 6 comments · Fixed by #23933
Closed

[csrng] V2S Signoff #22467

andreaskurth opened this issue Apr 8, 2024 · 6 comments · Fixed by #23933
Assignees
Labels
Component:DV DV issue: testbench, test case, etc. IP:csrng Type:Signoff

Comments

@andreaskurth
Copy link
Contributor

No description provided.

@andreaskurth andreaskurth added Component:DV DV issue: testbench, test case, etc. IP:csrng labels Apr 8, 2024
@andreaskurth andreaskurth added this to the Earlgrey-PROD.M4 milestone Apr 8, 2024
@moidx
Copy link
Contributor

moidx commented Jun 11, 2024

Discussed moving as P1 to M5 due to RTL risk analysis performed during last issue triage.

@vogelpi
Copy link
Contributor

vogelpi commented Jun 11, 2024

I will collect the required information and update the issue on THU. In short, we almost signed of CSRNG at V2S as part of M2 but had to signoff at V1 instead for the following reason:

  • The branch and expression coverage dropped to below 90%.

This drop occurred a long time ago and unrelated to the M2 work. It's tracked in #19033 and @h-filali is investigating this (he'll return tomorrow).

@vogelpi vogelpi self-assigned this Jun 18, 2024
@vogelpi
Copy link
Contributor

vogelpi commented Jun 25, 2024

Commits since Earlgrey-PROD.M2

For Commits since Earlgrey-ES tapeout, see #20976 (comment) .

git rev-parse --short HEAD 5655312

git log 9ddf276..HEAD --oneline hw/ip/csrng
fc05272 [csrng/dv] Add checks for the cmd sts signal to the scoreboard
-> Adds DV for command status signals, see #22219
1ffb8ef [csrng/doc] Correct reference to NIST spec term
b9213f5 [csrng/dv] Extend coverage definition for glen
-> More fine grained cover point for generate_length command parameter, see #18350
b633a21 [csrng/dv] Exclude AES core from coverage analysis
-> Changes to the AES core for reducing CSRNG area prior to M2 made it necessary to adjust the coverage exclusions.
66e3f8e [csrng/dv] Fix sfifo_gadstage_err and sfifo_gbencack_err tests
e1f3467 [csrng/dv] Fix syntax causing vcs to fail
763bf42 [dv] Reduce number of seeds for stress_all_with_rand_reset tests
b447bf7 [csrng/rtl] Make the main SM error test signal fatal
-> Fix a minor discrepancy compared to ENTROPY_SRC and EDN (see also 01551bc)
afd59fc [csrng/dv] Add the right err_test codes to fatal errors
6381071 [csrng] Flush command FIFO after signaling ACK errors
-> RTL bug fix to respect valid/ready handshake protocol in error case and fix error signaling between EDN and CSRNG (see #23526)
89d7f21 [csrng/dv] Correct prediction of FIPS/CC compliance flag for UNI cmd
01551bc [csrng] Locally escalate upon FSM errors
-> Aligning error / local escalation behavior with ENTROPY_SRC and EDN
16d7604 [csrng/dv] Disable some SVAs during fatal error injection in csrng_intr
3a4c42f [csrng] Add per-instance internal state read enable
c903d65 [csrng] Make reseed counters readable at any point in time
-> RTL fixes related to #23539 , DV tracked in #23826 and #23827
3b47465 [csrng/dv] Add DV for forced fips flag
baa7e6a [csrng/doc] Correct documentation around INVALID_ACMD errors
eecef82 [csrng/doc] Use enum type for SW_CMD_STS.CMD_STS field
8577cce [csrng] Add option to force FIPS/CC compliance flag
-> RTL fix for #23324, DV added with (#23349)
3931c32 [csrng/rtl] Check for GENU instead of GEN for CMD_STS_INVALID_GEN_CMD
-> RTL bug fix
604d2b2 [csrng/doc] Correct the documentation for invalid application commands
bb02ff0 [csrng/dv] Add DV for the cmd status responses
73b62e5 [csrng/dv] Add command status checks to send_cmd_req
b9e0388 [csrng/rtl] Move cmd checks to the cmd stage
7f84f7c [csrng/rtl] Add reseed interval status error
-> RTL changes to improve error handling and reporting
28ddb84 [csrng/dv] Add missing type declaration
fa7af7a [csrng/dv] Fix forcing of ack_sts signals in csrng_intr vseq
da67f46 [csrng/dv] Remove disabling/enabling of reworked and renamed SVA
0c397d7 [csrng] Add SVAs to check for unexpected pushes to the genbits FIFO
b65b9ef [csrng/rtl] Fix lint error
1d12ff7 [csrng/rtl] Remove zeroize state after uni error from ctr drbg cmd
-> Remove an uneeded RTL check that should rather be checked using an SVA
903b319 [csrng/rtl] Add command out of sequence cmd_sts error
042e0b9 [csrng/rtl] Change and assign cmd_sts signal
-> RTL changes to improve error handling and reporting
8295903 [bazel,autogen_hjson] Split C and rust header generation
30d7e78 (tag: Earlgrey-PROD-M2-RC5, tag: Earlgrey-PROD-M2) Add the project name to the copyright header
c2dd19d [csrng] Increase version number, revert verification stage to V1

Issues closed since Earlgrey-PROD.M2

For issues closed since Earlgrey-ES tapeout, see #20976 (comment)

Currently open issues

Coverage report from 2024-07-03

m5_v2s_signoff_csrng_2

Two V2 sequences (csrng_intr_vseq, csrng_err_vseq, csrng_cmds) have a pass rate below 100% and thus show as failing in the overview. However, all tests have a pass rate above the 90% threshold required for V2S. The only exception is the V3 stress_all_with_rand_reset test.

All coverage metrics are above the 90% V2(S) threshold.

Summary

Between M2 and now, there have been some RTL and DV changes to implement missing but important features and improve the error handling as well as for increasing coverage metrics. The most important RTL PRs were

All relevant DV work for V2S has been done and I suggest to sign off CSRNG at V2S. Do you share this view @andreaskurth and @h-filali ?

@vogelpi
Copy link
Contributor

vogelpi commented Jul 4, 2024

@h-filali , I've now updated the sign-off issue / comment above to reflect the latest changes. Would mind reviewing this please?

@h-filali
Copy link

h-filali commented Jul 5, 2024

@vogelpi I double checked the dashboard and I agree. The coverage metrics are all above 90% and the failing tests consist of mainly stress_all_with_rand_reset which is V3 and some rare failures on other tests (<6%). This has been consistently the case for multiple weeks (apart from where we had some general regression issues).
I double checked the commits and they are the same for me.
I double checked if the relevant CSRNG issues that I could find (closed or open) are all mentioned here.
I double checked that the issues that are mentioned here all are relevant for CSRNG.
This signoff issue LGTM! Great work ^^

vogelpi added a commit to vogelpi/opentitan that referenced this issue Jul 5, 2024
This resolves lowRISC#22467.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
@vogelpi
Copy link
Contributor

vogelpi commented Jul 5, 2024

Thanks for your review @h-filali ! I've now filed PR #23933 to move CSRNG to the V2S and close this issue.

@vogelpi vogelpi closed this as completed in 5b9568d Jul 5, 2024
nbdd0121 added a commit to nbdd0121/caliptra-rtl that referenced this issue Dec 11, 2024
Upstream OpenTitan commit 5b9568d058bed157328a1564a7f11d1ac45bec9b

This resolves lowRISC/opentitan#22467.

The following files are changed in OpenTitan commit but not included:
- hw/ip/csrng/doc/checklist.md

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
Co-authored-by: Gary Guo <gary.guo@lowrisc.org>
Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:DV DV issue: testbench, test case, etc. IP:csrng Type:Signoff
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants