From c15a2c7a4493edbed7c576f067009a67d25e9ea2 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Fri, 25 Sep 2026 12:46:10 -0700 Subject: [PATCH 1/5] [dv] Only fetch a key when the icache asks 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 --- dv/uvm/icache/dv/tb/tb.sv | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/dv/uvm/icache/dv/tb/tb.sv b/dv/uvm/icache/dv/tb/tb.sv index 51ebfbd197..550da18bab 100644 --- a/dv/uvm/icache/dv/tb/tb.sv +++ b/dv/uvm/icache/dv/tb/tb.sv @@ -16,6 +16,7 @@ module tb #( // macro includes `include "uvm_macros.svh" `include "dv_macros.svh" + `include "prim_assert.sv" wire clk, rst_n; @@ -47,6 +48,7 @@ module tb #( logic [SCRAMBLE_NONCE_W-1:0] scramble_nonce_q, scramble_nonce_d; logic scramble_key_valid_d, scramble_key_valid_q; logic scramble_req_d, scramble_req_q; + logic ic_scr_key_req; // DUT ibex_icache #( @@ -92,8 +94,7 @@ module tb #( .ic_data_wdata_o ( ram_if.ic_data_wdata ), .ic_data_rdata_i ( ram_if.ic_data_rdata_o ), .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 ), // TODO: Probe this and verify functionality .ecc_error_o ( ram_if.ecc_err ) @@ -102,7 +103,7 @@ module tb #( // Scramble key valid starts with OTP returning new valid key and stays high // until we request a new valid key. assign scramble_key_valid_d = scramble_req_q ? scramble_key_valid : - core_if.invalidate ? 1'b0 : + ic_scr_key_req ? 1'b0 : scramble_key_valid_q; always_ff @(posedge clk or negedge rst_n) begin @@ -125,11 +126,30 @@ module tb #( end end - // Scramble key request starts with invalidate signal from ICache and stays high + // Scramble key request starts with the key request from ICache and stays high // until we got a valid key. - assign scramble_req_d = scramble_req_q ? ~scramble_key_valid : core_if.invalidate; + assign scramble_req_d = scramble_req_q ? ~scramble_key_valid : ic_scr_key_req; assign scramble_req = scramble_req_q; + // The icache waits for a key out of reset and after each key request, until it sees a valid key. + // It must request a new key exactly when an invalidation arrives outside that wait. + logic out_of_reset_q, await_key_q; + always_ff @(posedge clk or negedge rst_n) begin + if (!rst_n) begin + out_of_reset_q <= 1'b1; + await_key_q <= 1'b1; + end else begin + out_of_reset_q <= 1'b0; + if (ic_scr_key_req) begin + await_key_q <= 1'b1; + end else if (!out_of_reset_q && scramble_key_valid_q) begin + await_key_q <= 1'b0; + end + end + end + + `ASSERT(ScrKeyReqChk_A, ic_scr_key_req == (core_if.invalidate && !await_key_q), clk, !rst_n) + // RAMs for (genvar way = 0; way < IC_NUM_WAYS; way++) begin : gen_rams // Tag RAM instantiation From c287f2c21be53c56a5c9f7da28a07eb858f2d4a4 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Fri, 16 Oct 2026 10:27:27 -0700 Subject: [PATCH 2/5] [dv] Fix the icache ECC assertions 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 --- dv/uvm/icache/data/ibex_icache_testplan.hjson | 13 +++-- dv/uvm/icache/dv/env/ibex_icache_ram_if.sv | 47 ++++++++++++++----- dv/uvm/icache/dv/tb/tb.sv | 6 ++- 3 files changed, 50 insertions(+), 16 deletions(-) diff --git a/dv/uvm/icache/data/ibex_icache_testplan.hjson b/dv/uvm/icache/data/ibex_icache_testplan.hjson index bc22ec196d..c7dcf8683a 100644 --- a/dv/uvm/icache/data/ibex_icache_testplan.hjson +++ b/dv/uvm/icache/data/ibex_icache_testplan.hjson @@ -111,12 +111,17 @@ { name: ecc - desc: '''Insert single-bit errors into cached data + desc: '''Insert one- or two-bit errors into cached data With the cache (as in the caching test), interpose a fake - memory for the cache RAMs, which occasionally inserts a single - bit error. Check that the invalid cached data is correctly - ignored.''' + memory for the cache RAMs, which occasionally flips one or two + bits. Check that the invalid cached data is correctly ignored. + + On a valid lookup, check that ecc_error_o is raised for an + injected tag error, or for corrupted data in the only way that + hit. It may also be raised for corrupted data in a line held in + two ways, which is not checked yet. Check that it is raised for + no other reason and is known outside reset.''' stage: V2 tests: ["ibex_icache_ecc"] } diff --git a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv index f5d28616f2..8ab4b1ac55 100644 --- a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv +++ b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv @@ -27,13 +27,17 @@ logic [IC_INDEX_W-1:0] ic_data_addr; logic [LineSizeECC-1:0] ic_data_wdata; logic [LineSizeECC-1:0] ic_data_rdata_in [IC_NUM_WAYS]; logic [LineSizeECC-1:0] ic_data_rdata_o [IC_NUM_WAYS]; -bit ecc_err; +logic ecc_err; bit [TagSizeECC-1:0] ic_tag_rdata_err [IC_NUM_WAYS]; bit [LineSizeECC-1:0] ic_data_rdata_err [IC_NUM_WAYS]; bit enable_ecc_errors; bit [IC_NUM_WAYS-1 : 0] data_sel_line; bit [IC_NUM_WAYS-1 : 0] tag_sel_line; int unsigned dis_err_pct = 99; +// The DUT's lookup state and tag tweak, connected in tb.sv +logic lookup_valid; +logic [ADDR_W-1:IC_INDEX_HI+1] lookup_addr; +logic [TagSizeECC-1:0] tag_tweak; clocking monitor_cb @(posedge clk); input ic_tag_addr; @@ -42,7 +46,7 @@ clocking monitor_cb @(posedge clk); input ic_tag_wdata; input ic_tag_rvalid; input ic_tag_rdata_in; - output ic_tag_rdata_o; + input ic_tag_rdata_o; input ic_data_req; input ic_data_gnt; input ic_data_rvalid; @@ -50,7 +54,7 @@ clocking monitor_cb @(posedge clk); input ic_data_addr; input ic_data_wdata; input ic_data_rdata_in; - output ic_data_rdata_o; + input ic_data_rdata_o; input ecc_err; input enable_ecc_errors; endclocking @@ -96,17 +100,38 @@ always @(negedge clk) begin end end -`ASSERT(TagErrChk_A, (& ic_tag_rvalid) && enable_ecc_errors && (tag_sel_line != 0) |-> ecc_err, - clk, rst_n) +// The ways that hit, from the tags as the RAMs hold them, before any injected error +logic [IC_NUM_WAYS-1:0] tag_match; +for (genvar i = 0; i < IC_NUM_WAYS; i++) begin : g_tag_match + assign tag_match[i] = (ic_tag_rdata_in[i][IC_TAG_SIZE-1:0] ^ tag_tweak[IC_TAG_SIZE-1:0]) == + {1'b1, lookup_addr}; +end + +// The DUT flags an ECC error on a valid lookup that reads a corrupted tag in any way, or corrupted +// data in the way that hit. +logic [IC_NUM_WAYS-1:0] tag_err_inj, data_err_inj; +logic ecc_err_allowed; +assign tag_err_inj = (enable_ecc_errors && (& ic_tag_rvalid)) ? tag_sel_line : '0; +assign data_err_inj = (enable_ecc_errors && (& ic_data_rvalid)) ? data_sel_line : '0; +assign ecc_err_allowed = lookup_valid && ((|tag_err_inj) || (|(data_err_inj & tag_match))); +`ASSERT(TagErrChk_A, lookup_valid && (|tag_err_inj) |-> ecc_err, clk, !rst_n) + +// Require the error only when the corrupted way is the only hit. A line held in two ways is +// merged before the data ECC check, so that case is not checked here. for (genvar i = 0; i < IC_NUM_WAYS; i++) begin : g_data_assertion `ASSERT(DataErrChk_A, - (& ic_data_rvalid) && !$isunknown(ic_data_rdata_in[i]) && (data_sel_line[i]) && - enable_ecc_errors |-> ecc_err, - clk, - rst_n) + lookup_valid && data_err_inj[i] && (tag_match == IC_NUM_WAYS'(1) << i) |-> ecc_err, + clk, !rst_n) end -`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) +`ASSERT_KNOWN(EccErrKnown_A, ecc_err, clk, !rst_n) +// An unknown lookup or hit would let the checks above pass without checking +`ASSERT_KNOWN(LookupValidKnown_A, lookup_valid, clk, !rst_n) +`ASSERT(TagMatchKnown_A, lookup_valid |-> !$isunknown(tag_match), clk, !rst_n) +`ASSERT(LookupReadChk_A, lookup_valid |-> (& ic_tag_rvalid) && (& ic_data_rvalid), clk, !rst_n) + +`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) endinterface diff --git a/dv/uvm/icache/dv/tb/tb.sv b/dv/uvm/icache/dv/tb/tb.sv index 550da18bab..186e955589 100644 --- a/dv/uvm/icache/dv/tb/tb.sv +++ b/dv/uvm/icache/dv/tb/tb.sv @@ -96,10 +96,14 @@ module tb #( .ic_scr_key_valid_i ( scramble_key_valid_q ), .ic_scr_key_req_o ( ic_scr_key_req ), - // TODO: Probe this and verify functionality .ecc_error_o ( ram_if.ecc_err ) ); + // Lookup state used by the ECC checks in ram_if + assign ram_if.lookup_valid = dut.lookup_valid_ic1; + assign ram_if.lookup_addr = dut.lookup_addr_ic1; + assign ram_if.tag_tweak = dut.tag_tweak_lw_ic1; + // Scramble key valid starts with OTP returning new valid key and stays high // until we request a new valid key. assign scramble_key_valid_d = scramble_req_q ? scramble_key_valid : From 29eb493202fb19c8c5dc6ca569262b88504a6275 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Tue, 13 Oct 2026 22:15:57 -0700 Subject: [PATCH 3/5] [dv] Drop always from the icache valid checks 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 --- dv/uvm/icache/dv/env/ibex_icache_ram_if.sv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv index 8ab4b1ac55..d5f9bf955b 100644 --- a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv +++ b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv @@ -132,6 +132,6 @@ end `ASSERT(TagMatchKnown_A, lookup_valid |-> !$isunknown(tag_match), clk, !rst_n) `ASSERT(LookupReadChk_A, lookup_valid |-> (& ic_tag_rvalid) && (& ic_data_rvalid), clk, !rst_n) -`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(TagValidChk_A, (& ic_tag_rvalid) == (| ic_tag_rvalid), clk, !rst_n) +`ASSERT(DataValidChk_A, (| ic_data_rvalid) == (& ic_data_rvalid), clk, !rst_n) endinterface From 9c8486ae79ed24c9e64d69fa79f806d0aa02602e Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Fri, 16 Oct 2026 10:08:30 -0700 Subject: [PATCH 4/5] [dv] Keep X in the icache ECC error injection 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 --- dv/uvm/icache/dv/env/ibex_icache_ram_if.sv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv index d5f9bf955b..93aaf8a55e 100644 --- a/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv +++ b/dv/uvm/icache/dv/env/ibex_icache_ram_if.sv @@ -28,8 +28,8 @@ logic [LineSizeECC-1:0] ic_data_wdata; logic [LineSizeECC-1:0] ic_data_rdata_in [IC_NUM_WAYS]; logic [LineSizeECC-1:0] ic_data_rdata_o [IC_NUM_WAYS]; logic ecc_err; -bit [TagSizeECC-1:0] ic_tag_rdata_err [IC_NUM_WAYS]; -bit [LineSizeECC-1:0] ic_data_rdata_err [IC_NUM_WAYS]; +logic [TagSizeECC-1:0] ic_tag_rdata_err [IC_NUM_WAYS]; +logic [LineSizeECC-1:0] ic_data_rdata_err [IC_NUM_WAYS]; bit enable_ecc_errors; bit [IC_NUM_WAYS-1 : 0] data_sel_line; bit [IC_NUM_WAYS-1 : 0] tag_sel_line; From 5bc67614a30c1e8701f47df291c3adc867becc67 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Fri, 16 Oct 2026 10:27:27 -0700 Subject: [PATCH 5/5] [dv] Add the bench checks to icache coverage 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 --- dv/uvm/icache/dv/ibex_icache_cover.cfg | 9 +++++++++ dv/uvm/icache/dv/ibex_icache_sim_cfg.hjson | 6 ++++++ 2 files changed, 15 insertions(+) create mode 100644 dv/uvm/icache/dv/ibex_icache_cover.cfg diff --git a/dv/uvm/icache/dv/ibex_icache_cover.cfg b/dv/uvm/icache/dv/ibex_icache_cover.cfg new file mode 100644 index 0000000000..43c68ef906 --- /dev/null +++ b/dv/uvm/icache/dv/ibex_icache_cover.cfg @@ -0,0 +1,9 @@ +// Copyright lowRISC contributors. +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +// Add the key-request check in tb and the checks in the RAM interface +begin assert + +tree tb 1 + +tree tb.ram_if +end diff --git a/dv/uvm/icache/dv/ibex_icache_sim_cfg.hjson b/dv/uvm/icache/dv/ibex_icache_sim_cfg.hjson index e6617cc2c9..ab14951895 100644 --- a/dv/uvm/icache/dv/ibex_icache_sim_cfg.hjson +++ b/dv/uvm/icache/dv/ibex_icache_sim_cfg.hjson @@ -43,6 +43,12 @@ name: fusesoc_cores_root_dirs value: ["--cores-root {proj_root}"] } + // Add the bench's own coverage config to the common ones. The first two files must stay in + // sync with default_vcs_cov_cfg_file in vendor/lowrisc_ip/dv/tools/dvsim/common_sim_cfg.hjson. + { + name: default_vcs_cov_cfg_file + value: "-cm_hier {dv_root}/tools/vcs/cover.cfg+{dv_root}/tools/vcs/common_cov_excl.cfg+{proj_root}/dv/uvm/icache/dv/ibex_icache_cover.cfg" + } ] // Default iterations for all tests - each test entry can override this.