A few of these messages get printed out just before an error. It's
much more helpful for debugging if you see them with the default
verbosity. They only appear when something goes wrong, so let's just
turn them on.
- The testbench probes signals that are unqualified by instr_valid
- This causes events to trigger due to instructions that are not
actually executed, leading to false timeout failures
- Note this fix alone doesn't eliminate such failures due to another
issue which will be addressed separately
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
The agent controls an ibex_icache_ecc_if interface, which is bound
into each prim_badbit_ram_1p module. There's a ton of painful wiring
in the environment to create an agent for each of these interfaces and
connect everything up properly.
By default, these agents don't have associated sequences (so they
don't inject read errors). You can switch them on by setting
enable_ecc_errors on the top-level virtual sequence. The patch adds a
vseq to do so (ibex_icache_ecc_vseq).
Note that we don't currently collect any specific coverage for ECC
checks. We'll probably add some uarch functional coverage points,
which will pick it up in the future, or we'll also pick it up if the
cache gets an alert output.
This does nothing by default, just wrapping up a prim_generic_ram_1p.
But we can bind an interface into it to inject bit errors by forcing
the bad_bit_mask signal.
Note that the icache uses ECC RAMs in a reasonably unusual way (ORing
together inputs and outputs from its data RAMs), so we have to do this
ourselves, rather than piggy-backing on the implementation or testing
done for e.g. OpenTitan's prim_ram_1p_adv.
This signal already got driven (to 1) when signalling a branch with
the interface's branch_to task. This patch now drives the branch_spec
line occasionally even if we don't actually do a branch. (One cycle in
64, for now).
This is manually squashed with a change to import dv_base_reg too, a
new module that was created by Weicai's "csr backdoor support" patch.
It's needed because it is a dependency of dv_lib.
Update code from upstream repository
https://github.com/lowRISC/opentitan to revision
c91b50f357a76dae2ada104e397f6a91f72a33da
* [prim_ram*_adv] Update core files and add prim_util dependency
(Michael Schaffner)
* [prim_ram*_adv] Implement Byte parity in prim_ram*_adv (Michael
Schaffner)
* [dvsim] Run tests in "interleaved" order (Rupert Swarbrick)
* [dvsim] Remove unnecessary getattr/setattr calls from SimCfg.py
(Rupert Swarbrick)
* [dv] Add support for multiple ral models (Srikrishna Iyer)
* [rtl] Fix prim flash dependency (Srikrishna Iyer)
* [prim_fifo_sync] Make FIFO output zero when empty (Noah Moroze)
* [dv] csr backdoor support (Weicai Yang)
* [prim] Add a "clog2 width" function (Philipp Wagner)
* [dvsim] Allow max-parallel to be set in the environment (Rupert
Swarbrick)
* [dvsim] Fix --reseed argument (Rupert Swarbrick)
* [prim_ram/rom*_adv] Break out into individual core files (Michael
Schaffner)
* [prim_rom] Align port naming with prim_ram* (Michael Schaffner)
* [dv] Allow a test to have "simple" timestamps (Rupert Swarbrick)
* [dvsim] Improve --help message (Rupert Swarbrick)
* [dvsim] Remove unused --local argument (Rupert Swarbrick)
* [dvsim] Small tidy-ups to mode selection in SimCfg.py (Rupert
Swarbrick)
* [fpv] formal compile fix required by VC Formal (Cindy Chen)
* [dvsim] Fix error detection logic in Deploy.py (Rupert Swarbrick)
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
- Remove the timing optimisations that delay the factoring-in of ecc
errors into valid_o.
- Optimisations are probably unnecessary here due to the minimal logic
hanging off valid_o, and the optimisations cause protocol checker
violations.
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
This matches the style used in the FF based register file. It gives each
register its own always block with a single enable rather than having
multiple registers with enables in a single always block.
Signed-off-by: Greg Chadwick <gac@lowrisc.org>
Gives the option of a maximum performance configuration without PMP
enabled, which is more of an orthogonal security feature.
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
By giving each register its own always_ff block clock gating is more
obvious to synthesis tools.
This also includes some minor naming tweaks to make use of the _q
convention for flops.
- If an interrupt arrives at the same time as a load/store instruction
is in ID stage, the interrupt must wait until load/store completes.
Without the WB stage this happens naturally as the core stalls. With
the WB stage, we need to allow the load/store to progress to the WB
stage (and clear the ID stage) then hold back the interrupt until it
completes.
- Also cleaned up some lsu related stalling terms and signal naming.
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
- Protocol-wise data_err_i is notionally X when !data_rvalid_i
- In addition, the design does not appear to rely on the asserted
behaviour
- Removing as it is firing in chip-level OT simulations
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
These cover points were extracted by reading down the icache
documentation (icache.rst). There aren't yet cover points to check
that the targets of the testplan were executed properly, nor are there
any uarch coverpoints (which would be bound into the design, rather
than the interface).
The rather elaborate flow of
sequence -> function -> trigger -> task -> covergroup
for cancelled_valid_cg follows a skeleton described in Doug Smith, "A
Practical Look @ SystemVerilog Coverage" (slides from a Doulos
course). I'm not completely convinced it's worth the effort, but I
guess it shows how to extract information from a temporal sequence in
the interface and shove it in a covergroup properly via the monitor.
This should have no functional change - it's still set iff branch is
set - but the logic now lies in the UVM code, rather than the
structural code in tb.sv.
This turns out to be reasonably easy to plumb in: derive from the core
sequence base class, overriding its run_req method (once I've
remembered to make it virtual). Then pick the right core sequence by
adding a factory override in the vseq.
This is an entry in the testplan. Renaming it to "oldval", because
suffixing every class name with "disable_without_invalidation" was
getting ridiculous.
When the existing code in drive_pmp() decided that an error needed
signalling, it waited until the request was dropped, or the address
changed, before clearing the PMP error.
This is fine, unless the memory seed is changed (by magical means!)
under our feet. The monitor spots a new request, but the driver needs
to know to clear the PMP error. This patch forcibly tells the driver
to drop the existing item if a new one comes in.
Without this, you get test failures if there are two back-to-back
branches to the same address that happen at the same time as a seed
update. The problem is that you only see one request transaction (with
the first seed), and the two memory responses both come back with the
first seed, when the second should have had the second seed.
One aspect of (i)cache design that I didn't know about before writing
test code for this block is the problem of multi-way hits. The icache,
as implemented, stores data to parallel ways and it's possible for a
fetch to match more than one way. The data from matching ways all gets
ORed together, which doesn't matter so long as it never
changes (because V | V == V for all V).
Of course, things go poorly if you have two different values, V and W,
at an address which are both stored in the cache. Then the result is V
| W, which isn't necessarily equal to either instruction.
Avoiding this needs priority encoders, which are rather large, so it
seems the usual approach is to disallow branching to modified code
before flushing the cache. This patch teaches the testbench to do this
properly.
Sadly, this means there's now a connection between the core agent and
the memory agent: the memory agent can no longer generate new seeds
whenever it pleases.
The test is the same, but the reordering means that if we see an error
that we weren't expecting, we'll complain about that, rather than
about the instruction data itself.
When a potential exception occurs in ID/EX controller must wait for any
outstanding instruction in WB to complete before resolving it. The
instruction in WB may also have an exception which takes priority over
ID/EX.
Signed-off-by: Greg Chadwick <gac@lowrisc.org>
With the writeback stage enabled the controller can see a load or store
error from the writeback stage whilst seeing some other fault/exception
from ID/EX (e.g. an instruction fetch error). The writeback stage fault
must take priority, however without the writeback stage the
priortisation changes.
This introduces more explicit prioritisation logic for faults/exceptions
and gives the correct prioritisation for configurations both with and
without a writeback stage.
Fixes#912
Signed-off-by: Greg Chadwick <gac@lowrisc.org>
- The speculative branch behaviour causes a performance degradation of
around 3% in the max config. This change enables that behaviour only
the maximum PMP config, which is where it is most needed for timing
closure.
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
- Split out address matching into less than, greater than and equals to
correctly match NAPOT, NA4 and TOR modes.
- Relates to #902
Signed-off-by: Tom Roberts <tomroberts@lowrisc.org>
In practice, this check will only trigger if you constrain your core
to fetch in a tight loop for a while and you don't invalidate the
cache very often.
The check has an assumption about the cache size (at least 1kB), but
that only has an effect on the tightness of the loop needed before we
do any checking.
