[aes,dv] Allow aes_reseed_vseq resets when passing new sideload key by rswarbrick · Pull Request #29907 · lowRISC/opentitan

rswarbrick · 2026-04-26T19:48:50Z

Now that all the previous PRs have been merged, this one is finally ready for review!

[aes,dv] Allow aes_reseed_vseq resets when passing new sideload key

To do so, we have to pull out the code that passes the new sideload
key and add checks for if reset is asserted, exiting the task early
when that happens.

rswarbrick · 2026-09-17T16:39:58Z

Force-push rebases onto origin/master (it's been a couple of weeks).

martin-velay · 2026-09-18T05:32:30Z

+        end
+        // Detect if the sideload valid bit gets de-asserted while trying to enable sideload.
+        begin
+          while (sideload_valid && !sideload_enabled) begin


Don't we need a !cfg.under_reset here too?

No, because the first process in the fork will set sideload_enabled, which causes things to drop out. But the comment that explains things isn't very obvious: it took me a couple of minutes to figure out! I'm going to add an extra sentence that points in the other direction to make things more obvious.

martin-velay · 2026-09-18T07:27:49Z

All these tasks are looking very similar, would it be possible to "squash" them into something like that and add to all the under_reset thing?

protected virtual task set_ctrl_shadowed_field(uvm_reg_field field, uvm_reg_data_t value); if (field.get_mirrored_value() == value) return; field.set(value); csr_update(.csr(ral.ctrl_shadowed), .en_shadow_wr(1'b1), .blocking(1)); if (cfg.under_reset) return; void'(field.predict(value)); endtask

Good point. Yes, very happily. Honestly, these tasks are a bit bonkers! I'll add a bit of text to the commit message that explains what's going on.

martin-velay · 2026-09-18T07:29:28Z

+    logic  valid;
+
+    if (!uvm_hdl_check_path(sideload_valid_path)) begin
+      `uvm_fatal(`gfn, $sformatf("\n\t ----| PATH NOT FOUND"))


It'd be better to add the sideload_valid_path string to the debug message

I agree. This code was already there, but I'm touching the line so it's an opportunity to improve things :-)

martin-velay · 2026-09-18T07:36:20Z

+        // Detect if the sideload valid bit gets de-asserted while trying to enable sideload.
+        begin
+          while (sideload_valid && !sideload_enabled) begin
+            cfg.clk_rst_vif.wait_clks_or_rst(1);


Before the same thing was done on the negedge of the clk and now I think it's on the posedge. So the read and the write are both on the posedge, which might cause a race condition?

Oops: good point! I'll move it back to the negative edge. Thanks.

martin-velay · 2026-09-18T07:38:18Z

-    end else begin
-      check_no_prng_reseed();
-    end
+    `uvm_info(get_name(),


Wouldn't it be preferable to make all the gfn replacements in one separate commit, or one dedicated PR? As it makes the code heterogeneous.

Oops! Definitely not preferable: this dates back to before I understood how get_full_name works for sequences. Fortunately, I've had the opportunity to learn a bit more about UVM since writing the code :-)

My preference is probably to use get_full_name instead of `gfn in any new code, but I agree that it's a bit silly to make things inconsistent in tasks that already exist. I'll switch back to the magic macro for both reasons!

UPDATE: There were several existing instances of get_name() in the virtual sequence. And it looks like these were all my fault. Boo. I'm switching them across to use gfn too.

martin-velay · 2026-09-18T07:40:10Z


+  // Do a backdoor read to get the value of the keymgr_key_i.valid port, treating 'x and 'z as
+  // invalid.
+  function bit snoop_sideload_valid();


All other helper methods are declared as local except for that one and pass_new_sideload_key. Is it done on purpose?

Not really. The reason is rather embarrassing: the standard Emacs major mode for Verilog wrongly indents non-extern local methods. I have a horrible feeling that I'd forgotten to go through and make things consistent. I'll sort that now.

To do so, we have to pull out the code that passes the new sideload key and add checks for if reset is asserted, exiting the task early when that happens. There are some loops that wait for an event by doing backdoor HDL reads on negative edges of the clock. To do this tidily, I've added a wait_n_clks_or_rst task to clk_rst_if (to match the wait_clks_or_rst task that already existed). One part of the changes in this commit is to add an "early exit on reset" to aes_base_vseq::set_sideload. Since there are a couple of equivalent tasks next to it, I've factored out the common code and taught them about resets too. Honestly, these tasks seem a bit strange to me: most of them have their only call site in aes_base_vseq::setup_dut. This function performs a chain of double-writes to the same register, one for each field. This is a bit inefficient! What's more, these sequence tasks do register prediction (presumably, there was no scoreboard when it was originally written). In both cases, the code is a bit awkward, but further changes to that part of the sequence are probably out of scope for this commit. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>

rswarbrick · 2026-09-19T21:23:56Z

@martin-velay: Thank you very much for the careful review. I think everything should be addressed now and I'd really appreciate another look.

rswarbrick requested review from nasahlpa and vogelpi April 26, 2026 19:48

rswarbrick added Component:DV DV issue: testbench, test case, etc. IP:aes labels Apr 26, 2026

This was referenced Apr 26, 2026

Aes check reseed rate exit on reset #29912

Draft

[aes,dv] Teach parts of the base vseq to allow injected resets #29914

Draft

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from b665440 to 27f9c7e Compare May 19, 2026 17:32

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from 27f9c7e to 3377672 Compare May 26, 2026 19:14

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from 3377672 to 7dea75a Compare August 12, 2026 21:46

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from 7dea75a to 147a518 Compare September 1, 2026 13:32

rswarbrick marked this pull request as ready for review September 1, 2026 13:33

rswarbrick requested a review from a team as a code owner September 1, 2026 13:33

rswarbrick requested review from martin-velay and removed request for a team September 1, 2026 13:33

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from 147a518 to 5053ab2 Compare September 17, 2026 16:39

martin-velay reviewed Sep 18, 2026

View reviewed changes

rswarbrick force-pushed the aes-pass-new-sideload-key-resets branch from 5053ab2 to be9a2d1 Compare September 19, 2026 21:23

rswarbrick requested a review from martin-velay September 19, 2026 21:24

Conversation

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

Uh oh!

Uh oh!

rswarbrick commented Sep 17, 2026

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!

rswarbrick Sep 19, 2026 • 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!

rswarbrick commented Sep 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rswarbrick commented Apr 26, 2026 •

edited

Loading

rswarbrick Sep 19, 2026 •

edited

Loading