Repository navigation
[dv] Turn on the icache ECC checks - #2515
Conversation
andreaskurth
left a comment
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks @kulan-pal, turning these checks on and finding the key-fetch bug with them is a valuable fix. I have a few minor comments; details inline.
Could the ecc entry in dv/uvm/icache/data/ibex_icache_testplan.hjson also mention that ecc_error_o is now checked against the injected errors, and that a line held in two ways is not checked yet?
| `ASSERT(TagValidChk_A, always (& ic_tag_rvalid == (| ic_tag_rvalid)), clk, !rst_n) | ||
| `ASSERT(DataValidChk_A, always (| ic_data_rvalid) == (& ic_data_rvalid), clk, !rst_n) |
There was a problem hiding this comment.
Now that these are live, always makes each clock edge start an attempt that never completes, so open attempts accumulate over the test. Since a concurrent assertion is already evaluated every cycle, could we drop always here?
|
|
||
| `ASSERT(TagValidChk_A, always (& ic_tag_rvalid == (| ic_tag_rvalid)), clk, rst_n) | ||
| `ASSERT(DataValidChk_A, always (| ic_data_rvalid) == (& ic_data_rvalid), clk, rst_n) | ||
| `ASSERT(NoUnexpectedErrChk_A, ecc_err |-> ecc_err_allowed, clk, !rst_n) |
There was a problem hiding this comment.
ecc_err is declared as bit, so an X on ecc_error_o reads as 0 and passes this check silently. Should it be logic, with an `ASSERT_KNOWN on it?
There was a problem hiding this comment.
ecc_err is now logic, and EccErrKnown_A checks that it is known outside reset. A local mutant that drives X instead of 0 fails it and passes the old checks. The error injection had the same problem with X read from the RAMs, so it uses logic now too.
| .ic_scr_key_valid_i ( scramble_key_valid_q ), | ||
| // TODO: Hook up to monitor and appropriate checking | ||
| .ic_scr_key_req_o ( ), | ||
| .ic_scr_key_req_o ( ic_scr_key_req ), |
There was a problem hiding this comment.
The TODO about checking the key request is removed, but nothing checks it yet. If the icache never raised ic_scr_key_req_o, scramble_key_valid_q would stay high and invalidations would complete with the old key unnoticed. Could we keep the TODO, or add an assertion that an invalidate leads to a key fetch?
There was a problem hiding this comment.
Added ScrKeyReqChk_A in tb.sv: the icache must request a key exactly when an invalidation arrives while it is not waiting for one. Local mutants that never request a key, or request one while already waiting, fail it.
| assign ram_if.lookup_valid = dut.lookup_valid_ic1; | ||
| assign ram_if.tag_match = dut.tag_match_ic1; |
There was a problem hiding this comment.
Using tag_match_ic1 as the reference mostly restates the RTL's own ecc_err_ic1 expression, so the "which way counts" decision is not checked independently. Would it be feasible to compute the expected hit in the TB from the uncorrupted ic_tag_rdata_in, the index-based tag tweak and lookup_addr_ic1? For lookup_valid, a sanity check such as lookup_valid |-> (&ic_tag_rvalid) && (&ic_data_rvalid) would help. If that is too much, a comment noting that the reference comes from the DUT would be fine too.
There was a problem hiding this comment.
The checks now find the hit from the uncorrupted tags, using the DUT's tweak and lookup address. I also added your sanity check as LookupReadChk_A, and checks that the lookup valid and the expected hit are known, so an X there cannot switch the checks off. A local mutant where way 1 never hits fails the new checks on some seeds, and the old checks pass on those same seeds.
When an invalidate arrived while the icache was already waiting for a scrambling key, the testbench fetched a second key. That fetch could land in the icache's invalidation sweep. The RAMs reject writes while a key is fetched, so some tags were not cleared and later read back as ECC errors. Fetch a key only when the icache asks for one, as ibex_top does. Check that it asks exactly when an invalidation arrives outside a key wait. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
The ECC assertions in the RAM interface passed rst_n where ASSERT expects an active-high reset, so they were off while the icache ran. Pass !rst_n. On a valid lookup, require an error for an injected tag error, and for corrupted data in the only way that hit, found from the uncorrupted tags. Require known error, lookup and hit signals, and reads of all ways. Reject unexpected errors. Data errors in a line held in two ways stay unchecked. Make the monitor's read data inputs and describe the checks in the testplan. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
TagValidChk_A and DataValidChk_A wrap their conditions in always. Each assertion already starts an attempt on every clock. With always, each attempt stays open until a failure, a reset or the end of the test, so open attempts pile up and none ever succeeds. Remove always so each enabled clock checks the condition once. A mismatch still fails on that clock. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
The RAM interface builds the read data it passes to the icache in two-state variables while errors are injected. Every way's read data goes through them, so an X read from the RAMs reaches the icache as 0. Make them four-state, so the icache sees the same X with and without injection. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
VCS assertion coverage only includes assertions inside the DUT. The checks in tb and in the RAM interface never appeared in the coverage report, so nothing showed whether they ever succeed. Add a bench cover config for those two scopes and pass it after the common ones. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
bc2f83e to
5bc6761
Compare
|
Thanks @andreaskurth. I updated the |
andreaskurth
left a comment
andreaskurth
left a comment
There was a problem hiding this comment.
LGTM, many thanks @kulan-pal!
3 participants
The ECC checks in the icache testbench are switched off while the icache runs. They pass
rst_nas the reset ofASSERT, which expects an active-high reset. With the reset fixed, one check fails on master:That check asks too much. On a lookup, the icache checks the data ECC only of the way that hit. The other ways may never have been written, so their ECC can be wrong. With this PR, the checks require:
The testbench finds the way that hit from the uncorrupted tags, using the icache's lookup address and tag tweak. Data errors in a line stored in two ways stay unchecked, because the icache combines both copies first. #2523 changes that and adds a check for it.
The checks also found a testbench bug, fixed in the first commit. The testbench fetched a new scrambling key on every invalidate, even when the icache was already waiting for one. The extra fetch could start in the middle of the invalidation, and the RAMs ignore writes during a key fetch. Some tags were not cleared and later read back as ECC errors. The testbench now fetches a key only when the icache asks for one, as
ibex_topdoes, and checks that it asks when it should.The error injection now uses
logicinstead ofbit, so an X from the RAMs reaches the icache instead of turning into 0. VCS coverage now includes the key-request and RAM interface checks, so the report shows whether each one ever succeeds. That config comes from #2523.Dependencies: none.
I ran all icache tests with dvsim on VCS U-2023.03-SP2 and the default testbench parameters, on 20 random seeds each and on seed 123. All passed. With coverage on, every assertion this PR adds or changes had real successes and no incomplete attempts on seed 1. To make sure the checks can fail, I ran local fault versions, such as a stuck or X error output, a missing or extra key request, a wrong or unknown hit reference, or missing RAM read valid flags. Each one makes the intended check fail. The one that drops data errors passes some seeds, because those seeds never hit in the cache. I did not run Xcelium.
AI disclosure (CLA §9): written with help from Claude Code and reviewed with OpenAI Codex; I have reviewed and understood every change and take full responsibility for it.