[otbn,dv] Refactor FSM states in otbn simulator #23966
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rswarbrick
requested review from
hcallahan-lowrisc
and removed request for
a team
rswarbrick
force-pushed
the
otbn-escalate-tidyup
branch
from
39ed6b3 to
33097f5
Compare
rswarbrick
force-pushed
the
otbn-escalate-tidyup
branch
2 times, most recently
from
23124b9 to
e89ff65
Compare
The existing code had assumptions about how the state FSM works that we don't want to be true. Avoid the assumptions by just warping to "after secure wipe" and then stopping when we start the final wipe. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
otbn-escalate-tidyup
branch
from
e89ff65 to
9e87591
Compare
|
The latest force-push should sort out CI. It turns out that the previous version of my tweaking of |
hcallahan-lowrisc
approved these changes
Jul 10, 2024
There was a problem hiding this comment.
The code looks reasonable to me, so I'm going to approve it. However, I'm not familiar enough with the system to confirm the new model matches the spec/intent. Given that this is about closing test corner-cases against the implementation which has been reviewed, discussed and documented, I think it's probably fine to merge.
Just some comment nits. This looked very tricky to get right! Nice job @rswarbrick :)
There was a problem hiding this comment.
Nit. Commit message for 06c9386
hw/ip/otbn/dv/otbnsim/sim/sim.py
Outdated
| is_good = self.state.get_fsm_state() == FsmState.WIPING_GOOD | ||
| locking = self.state.rma_req == LcTx.ON or not is_good | ||
|
|
||
| # If there is ann RMA request, make sure we're in the WIPING_BAD state |
hw/ip/otbn/dv/otbnsim/sim/sim.py
Outdated
Comment on lines
432
to
433
| # an RMA request. If there is an invalid RMA signal or we were | ||
| # in the WIPING_BAD state (so had the locking signal set) then | ||
| # we should lock. Otherwise, jump to IDLE. | ||
| # in the going to lock after wipe (so had the locking signal |
There was a problem hiding this comment.
Nit. grammar here is a bit off
hw/ip/otbn/dv/otbnsim/sim/state.py
Outdated
Comment on lines
57
to
59
| state (ending in updating the STATUS register to show we're done). The | ||
| difference we then goes back to IDLE or go to LOCKED (depending on the | ||
| lock_after_wipe flag). |
hw/ip/otbn/dv/otbnsim/sim/state.py
Outdated
| self.wipe_cycles = -1 | ||
|
|
||
| # This controls which state we move to when the next secure wipe |
hw/ip/otbn/dv/otbnsim/sim/sim.py
Outdated
| # set lock_after_wipe (since we're going to lock when we're done) | ||
| if self.state.rma_req == LcTx.ON and is_good: | ||
| self.state.set_fsm_state(FsmState.WIPING) | ||
| # If there is ann RMA request, ensure lock_after_wipe is set (since |
This was added in commit a491a03. I'm not completely convinced that the change was correct, and it isn't really explained in the commit message. (And it did break something else for me). I will remove the short-circuit for now. If it turns out to matter, I'll put it back in a follow-up commit, with extra notes that explain what it's doing. Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change, but this silences a warning from mypy. Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change, but this avoids some mypy warnings. Signed-off-by: Rupert Swarbrick <[email protected]>
This is to correctly handle a situation where we decide to jump to the locked state in the middle of a secure wipe. Before this change, we would print out ..., U, U, U, U (jump to locked state) V, STALL, STALL, STALL, ... and the trace checker would get upset that the model sent a V (meaning that a wipe had completed) and then a STALL without any corresponding event from the RTL side. With this change, we get ..., U, U, U, U, STALL, STALL, ... and avoid the error. Signed-off-by: Rupert Swarbrick <[email protected]>
A bogus RMA request only matters at specific states in the start/stop controller FSM. Change the model so that it similarly only looks for bad values at those times. Annoyingly, this takes effect "just after" we finish the secure wipe, so we need to model it with a new delayed_lock variable. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
I split these apart when I first handled secure wipe. It seemed like a good idea at the time! But it's not so great, especially if I want to split out the "waiting for URND seed" state (and don't want to duplicate that). Handle the state we're wiping towards with a separate flag. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
otbn-escalate-tidyup
branch
from
9e87591 to
1e99ed8
Compare
|
Thanks for the careful review, and for spotting the slightly spotty grammar from me! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes are to get the
otbn_escalatetest working properly: there was a rather "bodged" model of how secure wipe should work and it didn't match the RTL (or make sense!)The important change is the penultimate one, which splits the "wiping" state into two pieces. This means that we correctly track "waiting for seed" before "performing the wipe", which could end up being tracked backwards in some situations before.
But this doubles the number of "wiping" states and (many years ago) I unwisely differentiated between WIPING_GOOD and WIPING_BAD to distinguish whether we expected to lock when the wipe was complete. Rather than adding two more states, I've merged those two states back together (in the "Merge WIPING_GOOD, WIPING_BAD" commit) so the net change is zero!
Fixes #20835 (phew!)