[csrng,dv] Fix instantiation of test in tb by rswarbrick · Pull Request #30038 · lowRISC/opentitan

rswarbrick · 2026-05-05T19:52:54Z

This fixes #30034, and also teaches the environment to be more agnostic about the number of HW apps supported by CSRNG.

rswarbrick · 2026-05-06T10:31:27Z

Force push replaces a call to super.body that I accidentally dropped in the last commit.

rswarbrick · 2026-05-06T10:34:05Z

Gah! And I should have looked more closely at the failing lint check before pushing. The latest force-push fixes that too.

glaserf

Thanks a lot for fixing this and cleaning it up @rswarbrick! Having the number of HW interfaces set separately in dv and rtl surely wasn't elegant at all.

I only have some very small remarks, LGTM. Some of the uvm related changes are outside my realm of knowledge, so it would probably be good if someone else also takes a peek at it.

Sorry for the forgotten parameter in the tb. I think I ran into the issue with VCS myself once but then somehow must have forgotten to fix this. And I agree that it is very odd that Xcelium just jugs along and compiles fine.

glaserf · 2026-05-08T07:47:23Z

+    cs_item_q     = new[cfg.m_num_hw_apps + 1];
+    uninstantiate = new[cfg.m_num_hw_apps + 1];


Nit: Since these data structures are not specific to the HW interfaces, I think using m_num_apps here would be a bit better. Also, a new reader must be aware of the +1 relationship between m_num_apps and m_num_hw_apps to fully understand this part of the code.

I agree! (I think I might have written the first version of this before m_num_apps existed). Thanks for pointing it out.

glaserf · 2026-05-08T07:48:46Z

-  rand bit [NUM_HW_APPS:0] int_state_read_enable;
-  bit [NUM_HW_APPS:0] chk_int_state_read_enable;
+  rand bit [MaxNumHwApps:0] int_state_read_enable;
+  bit [MaxNumHwApps:0]      chk_int_state_read_enable;


Nit: Should we right-align the vector sizes here?

I'm not sure. The problem is that you end up with

rand bit [MaxNumHwApps:0] int_state_read_enable; bit [MaxNumHwApps:0] chk_int_state_read_enable;

which is a bit weird because it's treating "rand bit" and "bit" as being the same sort of thing.

Something like this would be consistent

rand bit [MaxNumHwApps:0] int_state_read_enable; bit [MaxNumHwApps:0] chk_int_state_read_enable;

but it's extremely ugly!

Do you have any better ideas? (Maybe I'm missing something obvious...(

glaserf · 2026-05-08T07:51:37Z

+    cs_item       = new[cfg.m_num_hw_apps + 1];
+    es_item       = new[cfg.m_num_hw_apps + 1];
+    es_item_q     = new[cfg.m_num_hw_apps + 1];
+    prd_genbits_q = new[cfg.m_num_hw_apps + 1];
+    cs_data       = new[cfg.m_num_hw_apps + 1];
+    es_data       = new[cfg.m_num_hw_apps + 1];
+    fips          = new[cfg.m_num_hw_apps + 1];
+    cmd_sts       = new[cfg.m_num_hw_apps + 1]('{default: CMD_STS_SUCCESS});


Maybe also better use m_num_apps here.

I agree! Done.

glaserf · 2026-05-08T08:31:35Z

+    .csrng_cmd_i                (csrng_cmd_req[dut.NumHwApps-1:0]),
+    .csrng_cmd_o                (csrng_cmd_rsp[dut.NumHwApps-1:0]),


I think the slice select here is not necessary and we can just reference the whole vector?

You're completely right! I originally wrote that code before I'd imported csrng_reg_pkg::NumApps, which meant I had some horrible "max footprint" thing.

Looking closely, I see that I was also making a generate loop depend on on dut.NumHwApps. I'll fix that too.

Thanks for spotting the bit I hadn't cleaned up :-)

glaserf · 2026-05-08T08:32:10Z

+  csrng_pkg::csrng_req_t[NumApps-2:0] csrng_cmd_req;
+  csrng_pkg::csrng_rsp_t[NumApps-2:0] csrng_cmd_rsp;


Could we use dut.NumHwApps here?

Hmm. I'm not sure. After getting rid of the silly slices (that you pointed out in the previous note), we don't need to look inside the dut at all (no occurrence of "dut." being added to the testbench).

I'm sort of enthusiastic about only depending on the "outside" (or the auto-generated bits). Does that sound reasonable to you?

glaserf · 2026-05-08T14:28:21Z

Just came to my mind: Did you get a chance to run a full regression for csrng to make sure that nothing got accidentally broken? The dv for edn and entropy_src might also be affected.

The interface is only instantiated by binding it into an instance of csrng (which has a NumHwApps localparam). Use that value as we get built. The only tricky item was defining cp_hw_inst_exc. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

This can be extracted from the instantiated dut in the tb, which passes it to the test through uvm_config_db. The test can then configure the csrng_env_cfg object before calling initialize. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

The environment config gets randomized in dv_base_test::build_phase anyway. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

This is mostly pretty mechanical, but these notes describe the changes in a bit more detail. - In the environment (including in virtual sequences), the number of HW apps is available in cfg.m_num_hw_apps. - The various arrays in csrng_scoreboard can be made dynamic and built at runtime (by which time cfg.m_num_hw_apps will have been set up) - The same trick can be used in the (copy-paste?) code that creates m_edn_push_seq in the various virtual sequences and also in csrng_cmds_vseq. - In csrng_regwen_vseq, there are two packed bit vectors that needed to be given a max footprint, but the extra stray bits won't be read anywhere. - In tb.sv, we can make the only "parameter dependence" come from csrng_reg_pkg (which was auto-generated from the hjson description). Importing csrng_reg_pkg::NumApps means we don't have to "look inside" the dut with e.g. dut.NumHwApps. - (Finally) We can get rid of the invalid NHwApps parameter given to the dut instance. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

rswarbrick · 2026-05-09T20:40:45Z

First force-push addressed the notes from @glaserf and the second one just rebases onto master (to pick up some clean-ups in the agent drivers). If looking at the "compare" links, I suggest the first one :-)

rswarbrick · 2026-05-09T21:17:49Z

Just came to my mind: Did you get a chance to run a full regression for csrng to make sure that nothing got accidentally broken? The dv for edn and entropy_src might also be affected.

I've just run the csrng tests with the current version of master (2ee0202) plus this PR and get the following results:

The numbers (both pass rate and coverage) are essentially the same as the weekly run from last weekend.

I don't think this change can affect the DV for edn or entropy_src: this PR only affects the block-level environment for the CSRNG block (not csrng_agent).

rswarbrick requested a review from glaserf May 5, 2026 19:52

rswarbrick requested a review from a team as a code owner May 5, 2026 19:52

rswarbrick requested review from marnovandermaas and removed request for a team May 5, 2026 19:52

rswarbrick added Component:DV DV issue: testbench, test case, etc. IP:csrng labels May 5, 2026

rswarbrick force-pushed the csrng-num-hw-apps branch from 375d501 to 79b8e5d Compare May 6, 2026 10:31

rswarbrick force-pushed the csrng-num-hw-apps branch from 79b8e5d to c52bdb7 Compare May 6, 2026 10:33

glaserf reviewed May 8, 2026

View reviewed changes

rswarbrick force-pushed the csrng-num-hw-apps branch from c52bdb7 to 8f77de4 Compare May 9, 2026 20:38

rswarbrick added 5 commits May 9, 2026 21:39

[dv] Allow a test to configure an env_cfg before cfg.initialize()

a9dad7e

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

[csrng,dv] Remove pointless double-randomization from tests

b8d6e0a

The environment config gets randomized in dv_base_test::build_phase anyway. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

rswarbrick force-pushed the csrng-num-hw-apps branch from 8f77de4 to b18ed93 Compare May 9, 2026 20:39

rswarbrick requested a review from glaserf May 9, 2026 21:20

		cs_item_q = new[cfg.m_num_hw_apps + 1];
		uninstantiate = new[cfg.m_num_hw_apps + 1];

		.csrng_cmd_i (csrng_cmd_req[dut.NumHwApps-1:0]),
		.csrng_cmd_o (csrng_cmd_rsp[dut.NumHwApps-1:0]),

		csrng_pkg::csrng_req_t[NumApps-2:0] csrng_cmd_req;
		csrng_pkg::csrng_rsp_t[NumApps-2:0] csrng_cmd_rsp;

Conversation

rswarbrick commented May 5, 2026

Uh oh!

rswarbrick commented May 6, 2026

Uh oh!

rswarbrick commented May 6, 2026

Uh oh!

glaserf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glaserf commented May 8, 2026

Uh oh!

rswarbrick commented May 9, 2026

Uh oh!

rswarbrick commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

glaserf left a comment •

edited

Loading

rswarbrick commented May 9, 2026 •

edited

Loading