[dv] Turn on the icache ECC checks

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"]
     }

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,15 +46,15 @@ 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;
   input  ic_data_write;
   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

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

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.

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