Skip to content

Conversation

glaserf
Copy link
Contributor

@glaserf glaserf commented Oct 8, 2025

This commit addresses a rare dv failure that was caused by turning some assertions too early on again in csrng_intr_vseq.

Prior to the change, stray data items, injected into the data path pipeline through forcing FIFO-wvld signals during fault injection, sometimes triggered the CsrngCmdStageGenbitsFifoPushExpected_A assertions in the command stages several cycles after test_cs_fatal_err() was already completed but before the main vseq routine disabled the CSRNG again. This was happening because test_cs_fatal_err() takes care of disabling the assertions in question, but re-enables them upon returning to the 'main' vseq which then waits for another 50 clock cycles before fully disabling the CSRNG with a CSR/RAL access, thereby preventing any further assertion triggering.

At the same time, the vseq disabled some asserts too early where this really wasn't necessary, as the sub-tests do nothing that would trigger any assertions. I therefore also moved disabling the assertions to a later point in the vseq.

For further details, please see my review comment.

This commit addresses a rare dv failure that was caused by turning some
assertions too early on again in csrng_intr_vseq. At the same time, the
commit reduces the window in which assertions get disabled at all to the
part of csrng_intr_vseq which tests the fatal_err interrupt. This is
the only part that uses fault injection, thereby potentially triggering
the assertions in question.

Signed-off-by: Florian Glaser <[email protected]>
@glaserf glaserf requested a review from a team as a code owner October 8, 2025 14:24
@glaserf glaserf requested review from hcallahan-lowrisc and removed request for a team and hcallahan-lowrisc October 8, 2025 14:24
`DV_ASSERT_CTRL_REQ("CmdStageFifoAsserts", 0)

// Test cs_fatal_err interrupt
test_cs_fatal_err();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fault-injecting test_cs_fatal_err() ends here, where previously the CsrngCmdStageGenbitsFifoPushExpected_A assertions were turned back on already, while the vseq waits for another 50 clock cycles below before disabling the CSRNG, thereby preventing the asserts in question.

During this time, data items in the FIFOs resulting from forced signals during fault injection could propagate through the data path, eventually arriving at the command stages and erroneously triggering CsrngCmdStageGenbitsFifoPushExpected_A assertions.

@glaserf glaserf requested review from h-filali and rswarbrick October 8, 2025 14:36
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

This makes sense, and thanks for figuring out what was going on.

Definitely not urgent, but a nice follow-up might be to tweak the assertions we're disabling so that they pick up a new "unused_assertion_disable" signal and OR it in with the rst_ni argument to the macro. Then the virtual sequence can just set and clear the signal and doesn't need a list of the assertion names.

@rswarbrick rswarbrick added this pull request to the merge queue Oct 10, 2025
Merged via the queue into lowRISC:master with commit 5d2537f Oct 10, 2025
47 checks passed
@glaserf
Copy link
Contributor Author

glaserf commented Oct 10, 2025

This makes sense, and thanks for figuring out what was going on.

Definitely not urgent, but a nice follow-up might be to tweak the assertions we're disabling so that they pick up a new "unused_assertion_disable" signal and OR it in with the rst_ni argument to the macro. Then the virtual sequence can just set and clear the signal and doesn't need a list of the assertion names.

Absolutely, thanks for the suggestion!

@glaserf glaserf deleted the csrng_intr_vseq_assert branch October 15, 2025 20:49
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