-
Notifications
You must be signed in to change notification settings - Fork 907
[csrng/rtl] Remove four prim_fifo_sync from the data path #28428
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
base: master
Are you sure you want to change the base?
Conversation
d8f932a to
1012a42
Compare
|
Here are the results of a full regression run: CSRNG Simulation ResultsTuesday October 21 2025 21:18:06 UTCGitHub Revision:
|
| Stage | Name | Tests | Max Job Runtime | Simulated Time | Passing | Total | Pass Rate |
|---|---|---|---|---|---|---|---|
| V1 | smoke | csrng_smoke | 2.000s | 126.360us | 50 | 50 | 100.00 % |
| V1 | csr_hw_reset | csrng_csr_hw_reset | 1.000s | 33.332us | 5 | 5 | 100.00 % |
| V1 | csr_rw | csrng_csr_rw | 1.000s | 64.890us | 20 | 20 | 100.00 % |
| V1 | csr_bit_bash | csrng_csr_bit_bash | 8.000s | 546.373us | 5 | 5 | 100.00 % |
| V1 | csr_aliasing | csrng_csr_aliasing | 4.000s | 732.336us | 5 | 5 | 100.00 % |
| V1 | csr_mem_rw_with_rand_reset | csrng_csr_mem_rw_with_rand_reset | 2.000s | 267.462us | 20 | 20 | 100.00 % |
| V1 | regwen_csr_and_corresponding_lockable_csr | csrng_csr_rw | 1.000s | 64.890us | 20 | 20 | 100.00 % |
| csrng_csr_aliasing | 4.000s | 732.336us | 5 | 5 | 100.00 % | ||
| V1 | TOTAL | 105 | 105 | 100.00 % | |||
| V2 | interrupts | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| V2 | alerts | csrng_alert | 37.000s | 6.936ms | 500 | 500 | 100.00 % |
| V2 | err | csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % |
| V2 | cmds | csrng_cmds | 4.817m | 59.997ms | 50 | 50 | 100.00 % |
| V2 | life cycle | csrng_cmds | 4.817m | 59.997ms | 50 | 50 | 100.00 % |
| V2 | stress_all | csrng_stress_all | 7.550m | 36.446ms | 47 | 50 | 94.00 % |
| V2 | intr_test | csrng_intr_test | 1.000s | 21.046us | 50 | 50 | 100.00 % |
| V2 | alert_test | csrng_alert_test | 2.000s | 46.987us | 50 | 50 | 100.00 % |
| V2 | tl_d_oob_addr_access | csrng_tl_errors | 9.000s | 1.794ms | 20 | 20 | 100.00 % |
| V2 | tl_d_illegal_access | csrng_tl_errors | 9.000s | 1.794ms | 20 | 20 | 100.00 % |
| V2 | tl_d_outstanding_access | csrng_csr_hw_reset | 1.000s | 33.332us | 5 | 5 | 100.00 % |
| csrng_csr_rw | 1.000s | 64.890us | 20 | 20 | 100.00 % | ||
| csrng_csr_aliasing | 4.000s | 732.336us | 5 | 5 | 100.00 % | ||
| csrng_same_csr_outstanding | 2.000s | 154.184us | 20 | 20 | 100.00 % | ||
| V2 | tl_d_partial_access | csrng_csr_hw_reset | 1.000s | 33.332us | 5 | 5 | 100.00 % |
| csrng_csr_rw | 1.000s | 64.890us | 20 | 20 | 100.00 % | ||
| csrng_csr_aliasing | 4.000s | 732.336us | 5 | 5 | 100.00 % | ||
| csrng_same_csr_outstanding | 2.000s | 154.184us | 20 | 20 | 100.00 % | ||
| V2 | TOTAL | 1437 | 1440 | 99.79 % | |||
| V2S | tl_intg_err | csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % |
| csrng_tl_intg_err | 5.000s | 1.007ms | 20 | 20 | 100.00 % | ||
| V2S | sec_cm_config_regwen | csrng_regwen | 3.000s | 15.670us | 50 | 50 | 100.00 % |
| csrng_csr_rw | 1.000s | 64.890us | 20 | 20 | 100.00 % | ||
| V2S | sec_cm_config_mubi | csrng_alert | 37.000s | 6.936ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_intersig_mubi | csrng_stress_all | 7.550m | 36.446ms | 47 | 50 | 94.00 % |
| V2S | sec_cm_main_sm_fsm_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_update_fsm_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_blk_enc_fsm_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_outblk_fsm_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_gen_cmd_ctr_redun | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_drbg_upd_ctr_redun | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_drbg_gen_ctr_redun | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_ctrl_mubi | csrng_alert | 37.000s | 6.936ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_main_sm_ctr_local_esc | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_constants_lc_gated | csrng_stress_all | 7.550m | 36.446ms | 47 | 50 | 94.00 % |
| V2S | sec_cm_sw_genbits_bus_consistency | csrng_alert | 37.000s | 6.936ms | 500 | 500 | 100.00 % |
| V2S | sec_cm_tile_link_bus_integrity | csrng_tl_intg_err | 5.000s | 1.007ms | 20 | 20 | 100.00 % |
| V2S | sec_cm_aes_cipher_fsm_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_fsm_redun | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_ctrl_sparse | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_fsm_local_esc | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_ctr_redun | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| csrng_sec_cm | 2.000s | 125.764us | 5 | 5 | 100.00 % | ||
| V2S | sec_cm_aes_cipher_data_reg_local_esc | csrng_intr | 7.000s | 886.714us | 200 | 200 | 100.00 % |
| csrng_err | 2.000s | 22.431us | 500 | 500 | 100.00 % | ||
| V2S | TOTAL | 75 | 75 | 100.00 % | |||
| V3 | stress_all_with_rand_reset | csrng_stress_all_with_rand_reset | 2.767m | 22.685ms | 10 | 10 | 100.00 % |
| V3 | TOTAL | 10 | 10 | 100.00 % | |||
| TOTAL | 1627 | 1630 | 99.82 % |
Coverage Results
Coverage Dashboard
| Score | Block | Branch | Statement | Expression | Toggle | Fsm | Assertion | CoverGroup |
|---|---|---|---|---|---|---|---|---|
| 97.65 % | 98.68 % | 96.74 % | 99.94 % | 97.02 % | 92.08 % | 100.00 % | 95.81 % | 90.36 % |
Failure Buckets
UVM_ERROR (csrng_scoreboard.sv:166) [scoreboard] Check failed intr_pins[i] === (intr_en[i] & item.d_data[i]) (* [*] vs * [*]) Interrupt_pin: EntropyReqhas 3 failures:- Test csrng_stress_all has 3 failures.
-
12.csrng_stress_all.16855465880732006052574500111896078071668320754285630673622661893706837231358
Line 166, in log /scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/csrng-sim-xcelium/12.csrng_stress_all/latest/run.logUVM_ERROR @ 4864276116 ps: (csrng_scoreboard.sv:166) [uvm_test_top.env.scoreboard] Check failed intr_pins[i] === (intr_en[i] & item.d_data[i]) (0x1 [1] vs 0x0 [0]) Interrupt_pin: EntropyReq UVM_INFO @ 4864276116 ps: (uvm_report_catcher.svh:705) [UVM/REPORT/CATCHER] --- UVM Report catcher Summary --- -
15.csrng_stress_all.111338181391474760543831925593840933333007280464523098505164842877179332551434
Line 167, in log /scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/csrng-sim-xcelium/15.csrng_stress_all/latest/run.logUVM_ERROR @ 672697298 ps: (csrng_scoreboard.sv:166) [uvm_test_top.env.scoreboard] Check failed intr_pins[i] === (intr_en[i] & item.d_data[i]) (0x1 [1] vs 0x0 [0]) Interrupt_pin: EntropyReq UVM_INFO @ 672697298 ps: (uvm_report_catcher.svh:705) [UVM/REPORT/CATCHER] --- UVM Report catcher Summary --- -
... and 1 more failures.
-
- Test csrng_stress_all has 3 failures.
INFO: [FlowCfg] [scratch_path]: [csrng] [/scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/csrng-sim-xcelium]
ERROR: [dvsim] Errors were encountered in this run.
[ legend ]: [Q: queued, D: dispatched, P: passed, F: failed, K: killed, T: total]
00:00:43 [ build ]: [Q: 0, D: 0, P: 2, F: 0, K: 0, T: 2] 100%
00:16:45 [ run ]: [Q: 0, D: 0, P: 1627, F: 3, K: 0, T: 1630] 100%
00:18:40 [ cov_merge ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
00:18:44 [ cov_report ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
Major version bump, since the following commits introduce breaking changes. Even though the intention is to keep the D2S and V2S stages, the slight required changes to the register map break binary compatibility, which requires a reset to D1 and V1. Signed-off-by: Florian Glaser <[email protected]>
Remove the redundant states of the main state machine which only produced outputs that were OR-reduced in csrng_core without any security counter measures. Furthermore, change the interaction between main sm, the cmd stages and the ctr_drbg_cmd module to be based on proper valid/ready handshakes. Signed-off-by: Florian Glaser <[email protected]>
1012a42 to
de408f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @glaserf for the PR, this is very nice and clean work!
I have one question. And I think it would be good also run the csrng_kat software test (I am not sure if this is run in CI, too) or just in the regressions.
I think before merging this PR, we should re-assure partners are aware of the version increment and the changes to the software interface.
| fields: [ | ||
| { bits: "7:0", | ||
| { bits: "5:0", | ||
| name: "MAIN_SM_STATE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please grep for MAIN_SM_STATE in the repo and see if there is still software reading this register for properly using CSRNG? I am sure we did this in the past, but I am not sure if this software is run in CI. It might just get run in the nightly / weekly regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, good point! The only thing I could find was a HAL function in the CSRNG dif and a call to in the the dif unit test.
However, the expected value of '42' (hex 0x2A) there does not match the old reset value, and the unit test just after this one, which reads the HW exception status register, also expects 42, so I lean towards the option that this test does not expect the actual reset value of the hardware but just tests the dif itself?
| assign gen_adata_null_d = !enable_i ? '0 : | ||
| (req_vld_i ? prep_gen_adata_null : gen_adata_null_q); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure but should we use req_vld_i & req_rdy_o here? Otherwise we might clear gen_adata_null_q when seeing a new vld but not yet having finished the last command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I updated the commit in question and also tightened one more handshake condition. Both in theory could lead to problems; however, in reality nothing breaks since the command arbiter/main FSM won't push a new command into the data path before the old one has finished at the state_db side.
I re-ran the full regression and updated the results in the comment above.
|
@phani-lowrisc for visibility. |
|
Thanks for the positive feedback, @vogelpi ! Regarding the KAT test, I just browsed the CI jobs that were ran as part of this PR and found the CSRNG KAT in several jobs included and passing, e.g., here: It also passes when ran locally: CHIP Simulation ResultsTuesday October 21 2025 12:46:46 UTCGitHub Revision:
|
| Stage | Name | Tests | Max Job Runtime | Simulated Time | Passing | Total | Pass Rate |
|---|---|---|---|---|---|---|---|
| V2 | chip_sw_csrng_known_answer_tests | chip_sw_csrng_kat_test | 59.000s | 2.592ms | 1 | 1 | 100.00 % |
| V2 | TOTAL | 1 | 1 | 100.00 % | |||
| TOTAL | 1 | 1 | 100.00 % |
INFO: [FlowCfg] [scratch_path]: [chip] [/scratch/glaserf/opentitan/scratch/csrng_sfifo_removal/chip_earlgrey_asic-sim-xcelium]
[ legend ]: [Q: queued, D: dispatched, P: passed, F: failed, K: killed, T: total]
00:00:37 [ build ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
00:01:56 [ run ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%
Remove the cmdreq FIFO in csrng_ctr_drbg_cmd and instead use data from the state db and the adata unpacker. The FIFO in question is the largest in the design, measuring around 9kGE. This commit can be seen as a blueprint for the ones that follow in regards to the removal of the respective error reporting circuitry and bits in the register map, dv, and dif. Signed-off-by: Florian Glaser <[email protected]>
This commit removes the pdata FIFO from the csrng_ctr_drbg_upd stage. As a consquence, pdata for the final step is now taken from the request fifo right at the request port of this module. This requires the internal control logic to wait for the block_encrypt module to process all three requests until the upstream req_rdy_o can be asserted. This change, however, does not reduce performance/throughput of the CSRNG. Signed-off-by: Florian Glaser <[email protected]>
This commit bridges the bencreq FIFO and instead feeds the read data from the updreq FIFO to the block_encrypt. This has neither functional nor timing impacts. Signed-off-by: Florian Glaser <[email protected]>
The input data is now taken directly from either the output FIFO in ctr_drbg_gen or the state db and the adata unpacker, depending on the origin of the request. Signed-off-by: Florian Glaser <[email protected]>
This commit addresses a rare dv failure caused by an incorrect counter width assumed in csrng_err_vseq. Instead, move the counter width definition to csrng_pkg and use use the value from there. Signed-off-by: Florian Glaser <[email protected]>
de408f5 to
89786d4
Compare
thanks @vogelpi for adding me will review them as well. |
This PR marks the beginning of the FIFO removal efforts for the CSRNG data path. In order to make this easier, this PR first simplifies the main FSM by removing states that effectively were identical (without providing any security countermeasures or hardening) and streamlining handshaking with the command stages.
Each of the following commits removes one FIFO; they are all quite similar in their structure/set of files that are affected. Since each FIFO is connected to several error reporting bits in the regfile, documentation,
dif, and dv, these parts of the IP also have to get touched every time a FIFO is removed.Since both the FSM simplification and removal of the FIFO error bits affect the register map, these changes are breaking. The first commit therefore bumps the version to 3.0.0 and resets the design and verification stages to D1 and V1, respectively, even though the intent here is to keep the design at the current D2S and V2S stages.
Once all breaking changes have been implemented and merged (will be distributed over several PRs), we'll hence have to sign-off the IP again.
The proposed changes have no timing impact on the IP; the critical path remains within the AES cipher.
Part of #28153.