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..93aaf8a55e 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; -bit [TagSizeECC-1:0] ic_tag_rdata_err [IC_NUM_WAYS]; -bit [LineSizeECC-1:0] ic_data_rdata_err [IC_NUM_WAYS]; +logic ecc_err; +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; 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, (& ic_tag_rvalid) == (| ic_tag_rvalid), clk, !rst_n) +`ASSERT(DataValidChk_A, (| ic_data_rvalid) == (& ic_data_rvalid), clk, !rst_n) endinterface 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. diff --git a/dv/uvm/icache/dv/tb/tb.sv b/dv/uvm/icache/dv/tb/tb.sv index 51ebfbd197..186e955589 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,17 +94,20 @@ 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 ) ); + // 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 : - 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 +130,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