Skip to content

Conversation

@glaserf
Copy link
Contributor

@glaserf glaserf commented Nov 21, 2025

Note: This PR will be kept as the current individual commits for eased review and auditing. However, once approved, the merge to master will be done as a single, self-contained commit.

Motivation

While previous PRs (#28428, #28633, #28611) allowed to significantly reduce the circuit area of the CSRNG, there is still quite some area locked in FIFOs that cannot be removed due to circular handshake dependencies between the three modules that together implement the CTR_DRBG algorithm as per NIST SP800-90A. Furthermore, the interconnect and resource sharing between these modules and the block_encrypt unit incur additional area and complexity in csrng_core through the required arbiters and muxes.

Therefore, a larger reorganization of the CTR_DRBG data path is unavoidable to unlock the remaining area and complexity savings.

Main Changes

This PR replaces the csrng_ctr_drbg_{cmd, upd, gen} modules with a single, unified csrng_ctr_drbg module that implements the complete data path as per NIST. It combines all required elements from the aforementioned previous modules (often de-duplicating and/or refactoring/restructuring them) with a newly designed FSM to control everything. More details about the new data path unit can be found in the commit message of the first commit. In addition, ample commentary for better understanding of the inner workings of the CTR_DRBG data path was kept in mind during the creation.

This unlocks the following benefits:

  • Additional 13kGE or 10% of area reduction for a cumulative area reduction of roughly 45%
  • Complexity reduction: Control of the data path by a single instead of four FSMs; uses only a single redundant prim_count for handling v instead of two
  • Code complexity reduction: Reduction of three RTL source files to one, net reduction of about 1000 LoC of RTL
  • DV complexity reduction: Fewer error reporting mechanism to test, especially in regards to fault injection
  • Addresses several open points and TODOs in [csrng] List of requested changes and improvements #28153

This change requires an update to the block diagram and the manual part of the documentation, both of which will be implemented as soon as this PR has seen general approval.

This commit adds the unified csrng_ctr_drbg module which replaces the
csrng_ctr_drbg_{cmd, gen, upd} modules. It combines all required data
path elements from the aforementioned modules and introduces a newly
designed FSM which creates all necessary control- and handshake signals.

This step allows for significant reduction of both circuit area and
design complexity. The unified module splits the response ports up:
Previously, writes to the state db, the genbits FIFOs in the command
stages, and signalling commands as completed to main and command stages
FSM were done on a single handshake channel. This is now done with three
independent channels which allows for a more streamlined data path and
does away with buffering and pipeline stages previously required.

Note that some of the response handshake channels do not feature a ready
signal, since the respective responders (state db, genbits FIFOs) are
always ready by construction.

Furthermore, csrng_ctr_drbg reworks the v-counter logic to perform all
v-increments inside the hardened prim_count. Before, when loading v from
the state db, the first increment was done outside of the prim_count as
part of its `set` value, potentially harming the overall hardening of v.

Signed-off-by: Florian Glaser <[email protected]>
This commit instantiates the unified csrng_ctr_drbg data path module in
csrng_core and removes the csrng_ctr_drbg_{cmd,gen,upd} modules from the
code base. This either requires or enables the following steps as well:

- Remove several assertions that reference removed parts of the design
- Remove/tweak data types from csrng_pkg
- Remove the flopped status_ack interface from the state db
- Remove the no longer needed cmdid FIFO from csrng_block_encrypt
- Remove all arbiters and muxes associated with the previous data path
  organization from csrng_core
- Remove a sizable amount of signals from csrng_core
- Restructure and clean up the handling of the various error signals in
  csrng_core (the amount of error sources is also further reduced)

Signed-off-by: Florian Glaser <[email protected]>
@glaserf
Copy link
Contributor Author

glaserf commented Nov 21, 2025

Regression results:

CSRNG Simulation Results

Friday November 21 2025 16:36:23 UTC

GitHub Revision: b2ba4738d9

Branch: csrng_archi_pr

Testplan

Simulator: XCELIUM

Test Results

Stage Name Tests Max Job Runtime Simulated Time Passing Total Pass Rate
V1 smoke csrng_smoke 3.000s 122.144us 50 50 100.00 %
V1 csr_hw_reset csrng_csr_hw_reset 1.000s 27.677us 5 5 100.00 %
V1 csr_rw csrng_csr_rw 1.000s 29.168us 20 20 100.00 %
V1 csr_bit_bash csrng_csr_bit_bash 8.000s 1.071ms 5 5 100.00 %
V1 csr_aliasing csrng_csr_aliasing 3.000s 442.599us 5 5 100.00 %
V1 csr_mem_rw_with_rand_reset csrng_csr_mem_rw_with_rand_reset 1.000s 97.666us 20 20 100.00 %
V1 regwen_csr_and_corresponding_lockable_csr csrng_csr_rw 1.000s 29.168us 20 20 100.00 %
csrng_csr_aliasing 3.000s 442.599us 5 5 100.00 %
V1 TOTAL 105 105 100.00 %
V2 interrupts csrng_intr 8.000s 1.394ms 200 200 100.00 %
V2 alerts csrng_alert 17.000s 3.392ms 500 500 100.00 %
V2 err csrng_err 3.000s 20.481us 500 500 100.00 %
V2 cmds csrng_cmds 3.333m 18.348ms 50 50 100.00 %
V2 life cycle csrng_cmds 3.333m 18.348ms 50 50 100.00 %
V2 stress_all csrng_stress_all 9.467m 111.730ms 49 50 98.00 %
V2 intr_test csrng_intr_test 1.000s 24.103us 50 50 100.00 %
V2 alert_test csrng_alert_test 2.000s 17.343us 50 50 100.00 %
V2 tl_d_oob_addr_access csrng_tl_errors 5.000s 627.706us 20 20 100.00 %
V2 tl_d_illegal_access csrng_tl_errors 5.000s 627.706us 20 20 100.00 %
V2 tl_d_outstanding_access csrng_csr_hw_reset 1.000s 27.677us 5 5 100.00 %
csrng_csr_rw 1.000s 29.168us 20 20 100.00 %
csrng_csr_aliasing 3.000s 442.599us 5 5 100.00 %
csrng_same_csr_outstanding 2.000s 290.117us 20 20 100.00 %
V2 tl_d_partial_access csrng_csr_hw_reset 1.000s 27.677us 5 5 100.00 %
csrng_csr_rw 1.000s 29.168us 20 20 100.00 %
csrng_csr_aliasing 3.000s 442.599us 5 5 100.00 %
csrng_same_csr_outstanding 2.000s 290.117us 20 20 100.00 %
V2 TOTAL 1439 1440 99.93 %
V2S tl_intg_err csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
csrng_tl_intg_err 8.000s 1.756ms 20 20 100.00 %
V2S sec_cm_config_regwen csrng_regwen 2.000s 78.276us 50 50 100.00 %
csrng_csr_rw 1.000s 29.168us 20 20 100.00 %
V2S sec_cm_config_mubi csrng_alert 17.000s 3.392ms 500 500 100.00 %
V2S sec_cm_intersig_mubi csrng_stress_all 9.467m 111.730ms 49 50 98.00 %
V2S sec_cm_main_sm_fsm_sparse csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_ctr_drbg_fsm_sparse csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_drbg_ctr_redun csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_gen_cmd_ctr_redun csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_ctrl_mubi csrng_alert 17.000s 3.392ms 500 500 100.00 %
V2S sec_cm_main_sm_ctr_local_esc csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
V2S sec_cm_constants_lc_gated csrng_stress_all 9.467m 111.730ms 49 50 98.00 %
V2S sec_cm_sw_genbits_bus_consistency csrng_alert 17.000s 3.392ms 500 500 100.00 %
V2S sec_cm_tile_link_bus_integrity csrng_tl_intg_err 8.000s 1.756ms 20 20 100.00 %
V2S sec_cm_aes_cipher_fsm_sparse csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_aes_cipher_fsm_redun csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
V2S sec_cm_aes_cipher_ctrl_sparse csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
V2S sec_cm_aes_cipher_fsm_local_esc csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
V2S sec_cm_aes_cipher_ctr_redun csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
csrng_sec_cm 5.000s 353.205us 5 5 100.00 %
V2S sec_cm_aes_cipher_data_reg_local_esc csrng_intr 8.000s 1.394ms 200 200 100.00 %
csrng_err 3.000s 20.481us 500 500 100.00 %
V2S TOTAL 75 75 100.00 %
V3 stress_all_with_rand_reset csrng_stress_all_with_rand_reset 2.600m 13.679ms 10 10 100.00 %
V3 TOTAL 10 10 100.00 %
TOTAL 1629 1630 99.94 %

Coverage Results

Coverage Dashboard

Score Block Branch Statement Expression Toggle Fsm Assertion CoverGroup
97.51 % 98.60 % 96.50 % 99.47 % 96.72 % 93.65 % 85.19 % 95.85 % 91.24 %

Failure Buckets

  • UVM_ERROR (csrng_scoreboard.sv:166) [scoreboard] Check failed intr_pins[i] === (intr_en[i] & item.d_data[i]) (* [*] vs * [*]) Interrupt_pin: EntropyReq has 1 failures:
    • Test csrng_stress_all has 1 failures.
      • 41.csrng_stress_all.97134871003220988528998910511999303650552284180187364491311040752061904021854
        Line 148, in log /scratch/glaserf/opentitan/scratch/csrng_archi_pr/csrng-sim-xcelium/41.csrng_stress_all/latest/run.log

          UVM_ERROR @  88108260 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 @  88108260 ps: (uvm_report_catcher.svh:705) [UVM/REPORT/CATCHER]
          --- UVM Report catcher Summary ---
        

INFO: [FlowCfg] [scratch_path]: [csrng] [/scratch/glaserf/opentitan/scratch/csrng_archi_pr/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:01:03  [    build    ]: [Q: 0, D: 0, P: 2, F: 0, K: 0, T: 2] 100%                                                                                                                                                                 
00:19:27  [     run     ]: [Q: 0, D: 0, P: 1629, F: 1, K: 0, T: 1630] 100%                                                                                                                                                           
00:19:57  [  cov_merge  ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%                                                                                                                                                                 
00:20:01  [ cov_report  ]: [Q: 0, D: 0, P: 1, F: 0, K: 0, T: 1] 100%

This commit removes all now unused error reporting bits from the regfile
and documentation as well as the dummy-assigns in csrng_core.

It also changes the misleadingly named `CMD_GEN_CNT_ERR` bit to
`CTR_ERR` which reflects its meaning (collective reporting if _any_
hardened counter in CSRNG has reported an error).

Finally, it aligns the SEC_CM annotations in RTL to match the IP hjson
and moves some of the labels closer to where the actual counter measure
is implemented.

Signed-off-by: Florian Glaser <[email protected]>
This commit removes all dv functionality related to the FIFOs, hardened
counters and FSMs that got removed during the data path simplification.
It also streamlines parts of the dv environment after having removed a
significant amount of the possible errors to probe/test.

It also removes the no longer used `GENU` and `GENB` command codes from
csrng_pkg, which affects a sizable amount of dv files, especially
coverage related ones.

Signed-off-by: Florian Glaser <[email protected]>
@glaserf glaserf marked this pull request as ready for review November 24, 2025 08:16
@glaserf glaserf requested review from a team as code owners November 24, 2025 08:16
@glaserf glaserf requested review from Razer6, andreaskurth, engdoreis, eshapira, johannheyszl, meisnere, moidx, rswarbrick and vogelpi and removed request for a team, engdoreis and eshapira November 24, 2025 08:16
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great @glaserf , thanks a lot for your work on this!

I have a couple of suggestions and questions, but nothing major, well done!

Comment on lines +34 to +35
// Write to state db
output logic state_db_wr_o,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be grouped with the rsp_ signals above? IIRC, state_db_wr_o is always equal to rsp_vld_o except during UPD_BEGIN. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is correct. I can add this to the rsp_ group as suggested.

// (compare NIST specification). Since all app interfaces share a single packer for adata, its
// content can get overwritten after the first block of generated bits, and hence we must buffer
// adata for each app interface locally for the full "lifetime" of a multi-block GENerate command.
// TODO(#28153) Find a smaller solution (giving each app interface an unpacker does not help)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the helpful comment, I agree that moving this packer to the app interface would make sense. This would allow us to save another 384 FFs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about this. However, replacing one packer and 3x 384 'naked' FFs with three packers unfortunately nets basically zero area savings due to the added logic complexity of the packers (I was a bit surprised as this complexity is basically a 1-to-12 router for each of the 32 input bits and did not expect it to be that large).
In any case, it would however decouple the instances from the data path for a cleaner design. These adata registers are the only place where the inst_id is currently required in the CTR_DRBG data path.

As for adata storage in general, I think this is the most substantial circuit area issue left. As described, this is a pure side-effect of the way we handle GENerate commands, but it incurs over 1000 Flops and therefore 10kGE of area overhead, which is now about 15% of the total IP area.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional background, this makes sense.

core_data.pdata = req_data_i.pdata ^ req_entropy_i;
core_data.fips = req_entropy_fips_i;
end
UPD, GEN: /* leave everything as-is */ ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would

UPD, GEN: begin
  // Leave everything as-is
end
UNI: begin
...

be more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe - I tried to keep the order in which the commands are handled the same everywhere, starting with INS and ending with UNI. But I can change it here, no problem.

Copy link
Contributor

@vogelpi vogelpi Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the order but I find

UPD, GEN: ;
UNI: begin
...
end

Just less readable. It looks extremely similar to

UPD, GEN,
UNI: begin
...
end

which would mean something different :-)

Comment on lines +212 to +214
// 1) Write adata to local buffer upon a GENerate command and _vld not being set
// 2) Clear _vld when the last GENerate beat appears on the cmd_rsp port
assign capt_adata = (req_vld_i && (core_data.cmd == GEN) && (core_data.inst_id == i));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments, this is very helpful!

I would suggest to align the capt_adata signal with the command. I.e.

assign capt_adata = (req_vld_i && (core_data.cmd == GEN) && !generate_adata_vld_q[i] && (core_data.inst_id == i));

Then add a second signal clear_adata_vld which is generated as follows:

assign clear_adata_vld = rsp_vld_o && req_glast_i && (core_data.inst_id == i)

This way you more clearly split the controls (the hard part to reason about and where you add your comments) from the actual data path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let me change this.


// SEC_CM: CTR_DRBG.CTR.REDUN
prim_count #(
.Width(CtrLen)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the prim_count has a parameter to define which operations (incr, decr, set etc.) are allowed. This is beneficial for coverage closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. As this was never done for CSRNG, I did not think about it. decr must be disabled.

GEN: begin
v_ctr_load = 1'b1;
// Decide which GENerate sub command to execute
// TODO(#28153) Clarify what the exact condition for skipping the initial UPDate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this here!

Comment on lines +376 to +378
end else if (es_halt_req_i) begin
state_d = ESHaltBenc;
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we'd drop the vld without seeing rdy. I don't think we should do this.

On the other hand, the whole CS AES Halt interface is a bit of a pain without much gain. We should remove that. I'll create an issue and make sure we discuss this in a working group meeting. We should then remove this in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is here: #28819
It's on the agenda for today's Silicon WG.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I always had my doubts about the effectiveness of this whole effort and as you say, it complicates both RTL and dv a lot.

As for dropping vld: I agree it is ugly, but the AES can handle it. However, the lack of a proper clear-signal for the AES is a bit more worrying, as we have to account for the case there the AES has accepted a data item and while it processes it, CSRNG gets disabled. The AES then emits the response at some time in the future where CSRNG is still disabled - or not. So on a disable, we always need to wait for a response first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right. Let's do a follow-up PR to this one that removes the CS AES Halt interface. I can give it a try.

// random bits. In all other cases, the response ends up in the scratch register.
if ((core_data.cmd == GEN) && (gen_subcmd_q == GEN_LOOP)) begin
bits_vld_o = 1'b1;
end else begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're here, does this mean where doing and Update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the sense of the update() function of the NIST algorithm, not the UPDdate command code. Except for UNInstantiate, all command codes call update() once, or in the case of GENerate, possibly twice.

Hence, this whole CTR_DRBG data path basically implements update() with some control wrapping around it to address GENerate, which is like an update() with only one block of data and without XORing the cipher output, wrapped between two regular update() calls at the beginning and end.

end
HndshkGen: begin
unique case (gen_subcmd_q)
UPD_BEGIN: begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we only hand over the data path after doing UPD_BEGIN right? This way, we don't have to track in the data base whether we've generated the first block or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand what you mean, but the first time a GENerate is handed to the data path, we first do an UPD_BEGIN in case of "non-zero" adata, then always a GEN_LOOP, then either flag the command as "completed" in case there are more generate beats to follow (hand over the data path?), or also do an UPD_FINAL.

Tracking of first-block-or-not is done implicitly through ensuring core_data.pdata being all-zero on the consecutive beats to the data path, done externally via clearing the adata packer from the main FSM after the first beat (which confused the heck out of me back when we were checking the KAT coverage).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks. Yes this is what I meant by handing over the data path.

I think we should add a simple bit to track the first-block-or-not once we rework the behavior around all-zero adata vs. zero clen. As you say, it's not intuitive.


// All valid commands except UNInstatiate set the instance state as 'instantiated'.
assign instance_state = (wr_data_i.cmd == INS) || (wr_data_i.cmd == RES) ||
(wr_data_i.cmd == GENU) || (wr_data_i.cmd == UPD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still have other references to GENU in the code base now? If no, we should update the documentation accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed all references to GENU and GENB in the dv-cleanup commit (well, at least I hope so). AFAIK, the doc however always hid these internal opcodes and did not mention them anywhere.

But as mentioned in the PR description, I'll anyways have to update documentation and block diagram. Do you think this should be part of this PR or a separate one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great. We can rework the block diagram and do a general doc update once the refactoring has concluded.

@vogelpi
Copy link
Contributor

vogelpi commented Nov 25, 2025

On a different note, I see that the FSM coverage went down from 97% to 85% with this PR. Do you have an idea of what's missing?

@vogelpi
Copy link
Contributor

vogelpi commented Nov 25, 2025

CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_block_encrypt.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_cmd_stage.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg_cmd.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg_gen.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg_upd.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_main_sm.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_pkg.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_reg_pkg.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_reg_top.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_state_db.sv

This PR is the last one of the big CSRNG restructuring / area optimization effort which has been discussed in various WG meetings. The verification metrics remain at very high levels. This is fine.

@glaserf
Copy link
Contributor Author

glaserf commented Nov 25, 2025

On a different note, I see that the FSM coverage went down from 97% to 85% with this PR. Do you have an idea of what's missing?

Yes, I also saw this and checked what's going on. The transition from ReqSend to ESHaltBenc and therefore the whole ESHaltBenc state gets never hit. Why? Because the block cipher is always ready in the ReqSend state (see https://github.com/lowRISC/opentitan/pull/28804/files#diff-0f17e171ebf502e8b61c2fc999bc8bb9ed340b95263b4487a7621e4461adfd35R370-R377).

So, in theory, we could just remove this state and the check for the block cipher being ready, but I find that a bit un-clean..

That being said, for the response path, the scratch register is also always ready to accept the response from the block cipher, so we could as well simply tie block_encrypt_rsp_rdy_o high instead of controlling it through the FSM..

@vogelpi
Copy link
Contributor

vogelpi commented Nov 27, 2025

On a different note, I see that the FSM coverage went down from 97% to 85% with this PR. Do you have an idea of what's missing?

Yes, I also saw this and checked what's going on. The transition from ReqSend to ESHaltBenc and therefore the whole ESHaltBenc state gets never hit. Why? Because the block cipher is always ready in the ReqSend state (see https://github.com/lowRISC/opentitan/pull/28804/files#diff-0f17e171ebf502e8b61c2fc999bc8bb9ed340b95263b4487a7621e4461adfd35R370-R377).

So, in theory, we could just remove this state and the check for the block cipher being ready, but I find that a bit un-clean..

That being said, for the response path, the scratch register is also always ready to accept the response from the block cipher, so we could as well simply tie block_encrypt_rsp_rdy_o high instead of controlling it through the FSM..

Thanks for the explanation. I would say let's revisit this after removing the CS AES Halt interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants