[hw] OTBN Sync for Integrated #22993
Closed
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
This package is now referenced in edn_pkg.sv. Signed-off-by: Pirmin Vogel <[email protected]>
According to the lc_ctrl spec, all values of non-escalation lc signals other than ON must be intrepreted as OFF. Also, no detection of invalid values should be performed as these signals might experience staggered transitions due to CDCs naturally leading to invalid values. This commit changes the design accordingly to match this spec. This resolves lowRISC#19050. Signed-off-by: Pirmin Vogel <[email protected]>
Fixes part of lowRISC#20887 Signed-off-by: Michael Schaffner <[email protected]>
sameo
requested review from
hcallahan-lowrisc and
engdoreis
and removed request for
a team
This commit adds a little bit of security hardening around signals used for zeroing parts of the OTBN state during secure wipe. To this end, the secure_wipe_running_o signal is registered inside the start stop controller (to get distinct driving cells) and the critical signals are compared against the secure_wipe_running signal at the point of consumption. This resolves lowRISC#12061. Co-authored-by: Greg Chadwick <[email protected]> Signed-off-by: Pirmin Vogel <[email protected]>
This PR fixes a typo in the Develop OTBN section of the OTBN documentation and various dead links referencing OTBN instructions in isa.html Signed-off-by: Pascal Nasahl <[email protected]>
Signed-off-by: Hugo McNally <[email protected]>
To enable easy re-use of this smoke test in a SiVal environment, loads from `RND` and `KEY_*` WSRs are removed when the deterministic symbol is given. A register dump has also been added. Signed-off-by: Hugo McNally <[email protected]>
Signed-off-by: Hugo McNally <[email protected]>
ImemSizeByte is set to a default value in the interface, so where another value is being used reference to the otbn_model_if type without the parameter are referring to the wrong type. Signed-off-by: Greg Chadwick <[email protected]>
These waits are bounds on assertions in otbn_core.sv. When CDC is enabled, they aren't long enough (because we also have to wait for data from the EDN). Bump them to match better, and add a note about why this isn't going to mask bugs. Signed-off-by: Rupert Swarbrick <[email protected]>
The RTL change changes the hierarchical IDs of some assertions so that
they all start with a particular scope. Once that is done, we can turn
them off with
$assertoff(0, "tb.dut.g_secure_wipe_assertions");
This is needed to avoid some spurious test failures in the
otbn_ctrl_redun test.
Although this changes some RTL code (hw/ip/otbn/rtl/otbn.sv), it
should be completely safe: the only change is to the hierarchical IDs
of some assertion statements.
Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change, except that the lists of CSRs/WSRs in the documentation are now auto-generated from a big YAML file. The plan is to use that YAML file for the other lists that you see mentioned in e.g. otbn/README.md. But I grabbed the initial list from the documentation, so "generating itself" seemed like the obvious first step. This should be the first step to sorting out issue lowRISC#3323, which tracks whether the toolchain can support named CSRs and WSRs in OTBN assembly code. If reviewing this PR: - You can mostly ignore the .yml files: the contents get used to generate README.md, which is also checked in. - The script in isr.py is the thing that does the easy job of parsing the yaml. - The script in md_isrs.py is the thing that converts the parsed yaml into a markdown table. - README.md is now auto-generated, and is intended to be mostly unchanged. Note that I haven't quite managed to auto-generate the same README.md as was there before. The changes are: - Using *bold* instead of <strong>bold</strong> - Avoiding some stray capitals in Flag Group - Indenting everything more uniformly. For example, see the RND WSR. - Adding anchor tags to all WSRs. We needed this for WSRs like KEY_S0_L, and I thought it seemed cleaner to do it uniformly instead of adding a flag to the YML to determine whether the WSR needed an anchor tag. Signed-off-by: Rupert Swarbrick <[email protected]>
The incorrect indentation causes a flake8 error. Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change. Most of this is re-wrapping comments. Signed-off-by: Rupert Swarbrick <[email protected]>
Most of the work in this patch is loading up the lists of CSRs and WSRs and then passing them through the code that translates between source code and an executable binary. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Santiago Arranz Olmos <[email protected]>
When I added the possibility of double-flopped (external) registers, I
split up ExtRegChange and TraceExtRegChange. An ExtRegChange contains
the actual change to a register ("Set value to 123"). A
TraceExtRegChange contains the ExtRegChange plus the register's actual
name (so it now says "Set INSN_CNT to 123").
I messed up the type diagram a bit, though. In particular, I left
ExtRegChange as a descendent of Trace which is not right at all! This
commit changes things so that it's no longer a subclass. We also
change a field name from "trace" to changes, since its value is a list
of changes, rather than a list of trace items.
This should cause no functional change.
Signed-off-by: Rupert Swarbrick <[email protected]>
This has no effect except to make it more obvious that these fields are intended to be private within the URNDWSR class. Signed-off-by: Rupert Swarbrick <[email protected]>
This doesn't actually get used as a field: it's just a name that we use locally when we're computing the next value in step(). Get rid of the field to make the class a bit simpler to understand. Signed-off-by: Rupert Swarbrick <[email protected]>
This is a representation of a 256-bit number as four 64-bit numbers.
It gets updated "without crossover", in that the limbs don't
interact, except that it "rolls" across by one limb each time.
So
A,B,C,D
becomes
f(B),f(C),f(D),f(A).
The existing Python did this by updating an array at position "i+1"
each time and then copying the last limb back to the start.
The new code updates at position "1 + i mod 4" which is hopefully a
little easier to understand, and avoids the Python model having an
extra element in the array.
Signed-off-by: Rupert Swarbrick <[email protected]>
This function is supposed to discard the changes to the model that are caused by an instruction that has been aborted. The URND state doesn't really update like that, and setting its value to zero would definitely be wrong! Signed-off-by: Rupert Swarbrick <[email protected]>
These were originally added as a way to give a method a nonempty body. The methods gained some code since, so we can now drop the spurious 'return' statements. Signed-off-by: Rupert Swarbrick <[email protected]>
There's no real need to use an option type here. The code is now slightly simpler. The only thing that we really lose is an internal assertion that makes sure we don't try to read URND before we've given it a value. Signed-off-by: Rupert Swarbrick <[email protected]>
This is probably slightly clearer in the code than reporting that there haven't been any changes on a cycle. Signed-off-by: Rupert Swarbrick <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
The problem is that the design RTL contains assertions like InitSecWipeNonZeroBaseRegs_A, which assert that each register has been initialised to a nonzero value during secure wipe. This is reasonable, but isn't true in this test: that's kind of the point! Signed-off-by: Rupert Swarbrick <[email protected]>
Avoid hierarchical references from classes. This causes VCS to fail, and can be easily avoided to enable it. Fixes lowRISC#19815 Signed-off-by: Guillermo Maturana <[email protected]>
In the Verilator-based pre-DV code, we instantiated otbn_core instead of the (bigger) otbn module. This is reasonable, but it means that we have to correctly model the gap between "writing the START command" and "raising the start_i signal". This got a cycle slower at some point (possibly with 3b58f20, but I haven't checked carefully), and the timing between the model and the RTL got unstuck. Add the corresponding cycle in the pre-DV wrapper to make everything match again. Signed-off-by: Rupert Swarbrick <[email protected]>
The otbn_ctrl_redun test wants to run a well-behaved binary. While the binary is running, the vseq wants to poke something to cause an error. Unfortunately, this doesn't work if the test binary happens to stop really quickly. The "safe mode" is designed to try to avoid this happening. Signed-off-by: Rupert Swarbrick <[email protected]>
In this vseq, we look at flags in the design and then do something once they become true. Factor out the call to DV_SPINWAIT that we used each time, and add a DV_CHECK_FATAL to assert that the calls to uvm_hdl_read succeed (because the path is indeed valid). Signed-off-by: Rupert Swarbrick <[email protected]>
For some reason, the code that was running before would set flag to X after a read. I don't really understand why that is. Once that happened, the while loop would instantly terminate. The test then failed because we're in the err_type=0 case. We try to inject an error, which doesn't actually do anything because the signal isn't being used. But we also tell the model that there should be an error (because we think there should!) and see a mismatch. Signed-off-by: Rupert Swarbrick <[email protected]>
This test was passing if a run happened not to inject an error. It's probably easier to reason about the feature if we fail the test in this case. (Obviously, we'll need to tweak things to ensure that we *do* inject an error, but that's a different problem). Signed-off-by: Rupert Swarbrick <[email protected]>
Since XOR works on each bit independently, we can do the XOR before rotating the inputs by 45 bits. This way, we only have to rotate one thing (the XOR'd value) and the result is somewhat faster. Standalone otbnsim gets a little bit faster (~2.5%) with this change. Signed-off-by: Rupert Swarbrick <[email protected]>
This shouldn't make any difference to behaviour, but it means that the "dirty" flag no longer gets set every instruction. That avoids hunting through some ext_regs and committing them unnecessarily. A silly test example that I put together for profiling gets about 25% faster, so this should avoid a bit of wasted energy in CI, if nothing else. Signed-off-by: Rupert Swarbrick <[email protected]>
It seems that recent versions of mypy complain about "yield" on its own because they doesn't infer that this is the same as an explicit "yield None". Tweak the code to match what the tool expects. Signed-off-by: Rupert Swarbrick <[email protected]>
When CDC is enabled, secure wipe can take a bit longer than otherwise, because the EDN takes longer to send new entropy. Lengthen this wait to give it time to complete. Signed-off-by: Rupert Swarbrick <[email protected]>
No functional change: this is mostly breaking long lines. But it avoids wiggly red lines from flake8 in the editor when trying to do other stuff... Signed-off-by: Rupert Swarbrick <[email protected]>
It seems that the compiler complains about lines of code of the form "A | |B" (presumably because they look a bit like "A || B" with a typo). Adding a parenthesis fixes things. Signed-off-by: Rupert Swarbrick <[email protected]>
It seems that I latched the error (in df151f9), but forgot to use that latched error signal in the calculation of err_latched. Signed-off-by: Rupert Swarbrick <[email protected]>
These "warps" are to bodge the internal loop counters in the RTL so that they match the warping in the model. The translation that they do doesn't work any more once the FSM in the RTL has stopped, because the value of prefetch_loop_iterations_o becomes 32'ffffffff. Fortunately, we don't really care: we know we're stopping anyway. Avoid bodging the loop counts in the RTL in that case: they'll never have any more effect anyway. Signed-off-by: Rupert Swarbrick <[email protected]>
The time taken for a secure wipe isn't constant, because it involves waiting for randomness from the EDN. And this takes longer when CDC is enabled, causing some tests to fail. This change adds a "wait_secure_wipe()" task to otbn_base_vseq, which makes sure to wait long enough for the secure wipe to finish. We can probably replace some other waits with this, but I didn't want to rewrite lots in this commit, so we just switch to it in otbn_rf_base_intg_err_vseq (the test that was failing for me when I wrote this). Signed-off-by: Rupert Swarbrick <[email protected]>
These would fail if the path was invalid for the design. There's some promising repetition that looks like it's worth factoring out, but this isn't as simple as one might hope and the (hard!) macro usage that was needed was much more complicated and not much shorter. Signed-off-by: Rupert Swarbrick <[email protected]>
These need adding to some tasks that are defined in a parent. Xcelium issues a warning if the parent class's task is marked protected but the task with the same name in the derived class is not. Get things in sync again. Signed-off-by: Rupert Swarbrick <[email protected]>
This vseq wants to start an OTBN run and then, at some time after the run has started, corrupt the next read of the base register file. Of course, this needs a bit of a wait: we need to wait until the base register file is being read. This wait is usually very short (we read from the RF with most instructions) but is rather longer if we happen to be stalled reading from RND! The stall for a RND read is longer when CDC is enabled, and pushed us over the 2000 cycle max wait. Increasing by another multiple of 10 seems to fix things. Signed-off-by: Rupert Swarbrick <[email protected]>
The `-purge` argument will remove dangling wires in the final netlist introduced due to the keep[_hierarchy] constraints and the flattening step after removing these constraints. Removing these wires reduces the size of netlist considerably thereby speeding up analysis using tools like PROLEAD. Signed-off-by: Pirmin Vogel <[email protected]>
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.
A
hw/ip/otbnsync between themasterandintegrated_devbranches.Missing commits:
[otbn, rtl] Increase imem size to 8 KiB][sram_ctrl,prim,rtl] Fix tlul_we -> ready timing path.[prim, rom_ctrl] Remove S&P layer from data scrambling[reggen] Remove the devmode input