Make sure we don't see multi-way hits in icache testbench

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.

doc/icache.rst

@@ -269,3 +269,9 @@ While doing this, the cache behaves as if ``icache_enable_i`` is false and will
 
 The ``busy_o`` signal is guaranteed to be high while the cache is invalidating its internal memories or whenever it has a pending fetch on the instruction bus.
 When the ``busy_o`` signal is low, it is safe to clock gate the cache.
+
+The cache doesn't have circuitry to avoid inconsistent multi-way hits.
+As such, the core must never fetch from an address with the cache enabled after modifying the data at that address, without first starting a cache invalidation.
+
+.. note::
+   This is a constraint on *software*, not just on the core.

dv/uvm/icache/doc/ibex_icache_dv_plan.md

@@ -57,6 +57,11 @@ These transactions are:
 
 The monitor for the core agent reports cache invalidations, when the cache is enabled/disabled, branches and instruction reads (including errors).
 
+As well as restrictions on how the core can communicate, the cache also assumes that the core will never modify memory and then perform a cached fetch from that memory without an invalidation.
+This means that the cache doesn't need to avoid multi-way hits with inconsistent data.
+In order that the testbench can guarantee this doesn't happen, the core agent must be in charge of deciding when to update the contents of memory with a new seed (see the Memory Agent, below).
+A new seed is picked on every invalidation (since this gives the maximum test sensitivity) and is picked occasionally when the cache is fetching when disabled.
+
 #### Memory Agent
 
 The memory agent emulates the instruction bus, supplying data to the cache.
@@ -67,9 +72,6 @@ The architectural state of the instruction bus and memory (contents at each addr
 The precise functions can be found in [`dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_model.sv`](https://github.com/lowRISC/ibex/blob/master/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_model.sv).
 
 The memory agent is an active slave, responding to instruction fetches from the cache with either a PMP error (on the same cycle as the request) or instruction data (with an in-order request pipeline).
-As well as the instruction fetch requests, its sequence contains occasional transactions that change the underlying seed.
-These transactions are reported to the scoreboard (so it can know the set of possible seeds for any given fetch).
-To avoid too many valid seeds in the scoreboard, this should happen at a similar rate to cache invalidations in the core agent.
 
 ### Top level testbench
 

dv/uvm/icache/dv/env/ibex_icache_env.sv

@@ -39,8 +39,12 @@ class ibex_icache_env extends dv_base_env #(
     if (cfg.en_scb) begin
       core_agent.monitor.analysis_port.connect(scoreboard.core_fifo.analysis_export);
       mem_agent.monitor.analysis_port.connect(scoreboard.mem_fifo.analysis_export);
-      mem_agent.driver.analysis_port.connect(scoreboard.seed_fifo.analysis_export);
+      core_agent.driver.analysis_port.connect(scoreboard.seed_fifo.analysis_export);
     end
+    if (cfg.is_active && cfg.mem_agent_cfg.is_active && cfg.core_agent_cfg.is_active) begin
+      core_agent.driver.analysis_port.connect(mem_agent.sequencer.seed_fifo.analysis_export);
+    end
+
     if (cfg.is_active && cfg.core_agent_cfg.is_active) begin
       virtual_sequencer.core_sequencer_h = core_agent.sequencer;
     end

dv/uvm/icache/dv/env/seq_lib/ibex_icache_passthru_vseq.sv

@@ -14,9 +14,7 @@ class ibex_icache_passthru_vseq extends ibex_icache_base_vseq;
     core_seq.constrain_branches = 1'b1;
     core_seq.initial_enable     = 1'b0;
     core_seq.const_enable       = 1'b1;
-
-    // Increase the frequency of seed updates
-    mem_seq.gap_between_seeds = 49;
+    core_seq.gap_between_seeds  = 1;
 
   endtask : pre_start
 

dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_driver.sv

@@ -9,7 +9,16 @@ class ibex_icache_core_driver
   `uvm_component_utils(ibex_icache_core_driver)
   `uvm_component_new
 
-  // reset signals
+  // An analysis port that gets hooked up to the scoreboard and the memory agent. This isn't used to
+  // report on actual bus traffic (that's done by the monitor, as usual) but is used to report on
+  // new memory seeds, which don't in themselves cause any bus traffic.
+  uvm_analysis_port #(bit [31:0]) analysis_port;
+
+  function void build_phase(uvm_phase phase);
+    super.build_phase(phase);
+    analysis_port = new("analysis_port", this);
+  endfunction
+
   virtual task automatic reset_signals();
     cfg.vif.reset();
   endtask
@@ -53,6 +62,7 @@ class ibex_icache_core_driver
     fork
         cfg.vif.branch_to(req.branch_addr);
         if (req.invalidate) invalidate();
+        if (req.new_seed != 0) drive_new_seed(req.new_seed);
         read_insns(rsp, req.num_insns);
     join
   endtask
@@ -82,6 +92,7 @@ class ibex_icache_core_driver
     fork
         if (req_low_cycles > 0) lower_req(req_low_cycles);
         if (req.invalidate) invalidate();
+        if (req.new_seed != 0) drive_new_seed(req.new_seed);
     join
     read_insns(rsp, req.num_insns);
   endtask
@@ -138,4 +149,10 @@ class ibex_icache_core_driver
     cfg.vif.invalidate_pulse(num_cycles);
   endtask
 
+  // If this item specifies a new seed, tell the memory agent and the scoreboard.
+  task automatic drive_new_seed(bit [31:0] new_seed);
+    `DV_CHECK_FATAL(new_seed != 0);
+    analysis_port.write(new_seed);
+  endtask
+
 endclass

dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_req_item.sv

@@ -20,6 +20,10 @@ class ibex_icache_core_req_item extends uvm_sequence_item;
   // The number of instructions to read (always non-negative, but may be zero)
   rand int                           num_insns;
 
+  // A possible new memory seed. This has no effect when zero. When nonzero, the driver will pass
+  // the seed through a "backdoor" to the memory agent.
+  rand bit [31:0]                    new_seed;
+
   constraint c_non_branch_trans_addr {
     // If the transaction type is ICacheCoreTransTypeBranch then branch_addr can be anything. To
     // make reading debug logs a bit easier, we force it to be zero otherwise.
@@ -38,12 +42,6 @@ class ibex_icache_core_req_item extends uvm_sequence_item;
     !branch_addr[0];
   }
 
-  constraint c_invalidate_dist {
-    // Poke the cache invalidate line one time in 50. This takes ages and we don't want to
-    // accidentally spend most of the test waiting for invalidation.
-    invalidate dist { 0 :/ 49, 1 :/ 1 };
-  }
-
   constraint c_num_insns_dist {
     // For branch transactions, we want to read zero instructions reasonably frequently. For req
     // transactions, much less so. Also, we don't bother with long sequences for req transactions:
@@ -55,6 +53,13 @@ class ibex_icache_core_req_item extends uvm_sequence_item;
       num_insns dist { 0 :/ 1, [1:20] :/ 20 };
   }
 
+  constraint c_new_seed_dist {
+    // We always want to pick a new seed when we start an invalidation (for maximum test
+    // sensitivity). If we aren't starting an invalidation, we mustn't pick a new seed if the cache
+    // is enabled (because that could cause multi-way hits). If the cache isn't enabled, pick a new
+    // seed sometimes.
+    new_seed dist { 0 :/ 1, [1:32'hffffffff] :/ (invalidate ? 1000 : enable ? 0 : 1) };
+  }
 
   `uvm_object_utils_begin(ibex_icache_core_req_item)
     `uvm_field_enum(ibex_icache_core_trans_type_e, trans_type, UVM_DEFAULT)
@@ -62,6 +67,7 @@ class ibex_icache_core_req_item extends uvm_sequence_item;
     `uvm_field_int (enable,      UVM_DEFAULT)
     `uvm_field_int (invalidate,  UVM_DEFAULT)
     `uvm_field_int (num_insns,   UVM_DEFAULT)
+    `uvm_field_int (new_seed,    UVM_DEFAULT | UVM_HEX)
   `uvm_object_utils_end
 
   `uvm_object_new

dv/uvm/icache/dv/ibex_icache_core_agent/seq_lib/ibex_icache_core_base_seq.sv

@@ -27,6 +27,17 @@ class ibex_icache_core_base_seq extends dv_base_seq #(
   // If this bit is set, we will never invalidate the cache (useful for hit ratio tracking)
   bit no_invalidate = 1'b0;
 
+  // The expected number of items between each new memory seed when the cache is disabled. A new
+  // memory seed when the cache is disabled implies that the next time the cache is enabled, we must
+  // start an invalidation. As such, this shouldn't normally be set too low.
+  int unsigned gap_between_seeds = 49;
+
+  // The expected number of items between each invalidation. This shouldn't be too small because an
+  // invalidation takes ages and we don't want to accidentally spend most of the test waiting for
+  // invalidation.
+  int unsigned gap_between_invalidations = 49;
+
+
   // Number of test items (note that a single test item may contain many instruction fetches)
   protected rand int count;
   constraint c_count { count inside {[800:1000]}; }
@@ -47,6 +58,10 @@ class ibex_icache_core_base_seq extends dv_base_seq #(
   // Whether the cache is enabled at the moment
   protected bit cache_enabled;
 
+  // Whether the cache must be invalidated next time it is enabled (because we changed the seed
+  // under its feet while it was disabled)
+  protected bit stale_seed = 0;
+
   virtual task body();
     // Set cache_enabled from initial_enable here (rather than at the declaration). This way, a user
     // can set initial_enable after constructing the sequence, but before running it.
@@ -91,6 +106,13 @@ class ibex_icache_core_base_seq extends dv_base_seq #(
 
        // If no_invalidate is set, we shouldn't ever touch the invalidate line.
        no_invalidate -> invalidate == 1'b0;
+
+       // Start an invalidation every 1+gap_between_invalidations items.
+       invalidate dist { 1'b0 :/ gap_between_invalidations, 1'b1 :/ 1 };
+
+       // If we have seen a new seed since the last invalidation (which must have happened while the
+       // cache was disabled) and this item is enabled, force the cache to invalidate.
+       stale_seed && enable -> invalidate == 1'b1;
     )
 
     finish_item(req);
@@ -99,6 +121,11 @@ class ibex_icache_core_base_seq extends dv_base_seq #(
     // Update whether we think the cache is enabled
     cache_enabled = req.enable;
 
+    // Update the stale_seed flag. It is cleared if we just invalidated and should be set if we
+    // didn't invalidate and picked a new seed.
+    if (req.invalidate) stale_seed = 0;
+    else if (req.new_seed != 0) stale_seed = 1;
+
     // The next transaction must start with a branch if this one ended with an error
     force_branch = rsp.saw_error;
 

dv/uvm/icache/dv/ibex_icache_mem_agent/README.md

@@ -20,3 +20,6 @@ The entire dance is as follows:
   1. The sequence uses its memory model to decide the data to be fetched and whether the response should have an error. The results get written into a sequence item with `is_grant` field true.
   1. When the driver picks up this sequence item, it adds it to a response queue.
   1. Responses get popped from the response queue in order. Each causes a delay of zero or more cycles and then the response is finally driven onto the bus with `rvalid`, `rdata` and `err`.
+
+The underlying memory seed can be updated by pushing a new value to the sequencer's `seed_fifo`.
+New seeds are generated by the core model, which ensures we only get a new seed when either the cache is disabled or on an invalidation.

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_driver.sv

@@ -20,11 +20,6 @@ class ibex_icache_mem_driver
   mailbox #(ibex_icache_mem_resp_item) rdata_queue;
   mailbox #(bit [31:0])                pmp_queue;
 
-  // An analysis port that gets hooked up to the scoreboard. This isn't used to report on actual bus
-  // traffic (that's done by the monitor, as usual) but is used to report on new memory seeds, which
-  // the scoreboard needs to know about, but don't in themselves cause any bus traffic.
-  uvm_analysis_port #(bit [31:0]) analysis_port;
-
   `uvm_component_utils(ibex_icache_mem_driver)
   `uvm_component_new
 
@@ -32,7 +27,6 @@ class ibex_icache_mem_driver
     super.build_phase(phase);
     rdata_queue   = new("rdata_queue");
     pmp_queue     = new("pmp_queue");
-    analysis_port = new("analysis_port", this);
   endfunction
 
   virtual task reset_signals();
@@ -79,15 +73,8 @@ class ibex_icache_mem_driver
 
       // Is this a request or a grant?
       if (!req.is_grant) begin
-        // A request has two effects.
-        //
-        //   1. If this is a new seed then we need to tell the scoreboard about it.
-        //
-        //   2. If it causes a PMP error, we push the address onto pmp_queue (which will be handled
-        //      by drive_pmp).
-        if (req.seed != 32'd0) begin
-          analysis_port.write(req.seed);
-        end
+        // If a request causes a PMP error, we push the address onto pmp_queue (which will be
+        // handled by drive_pmp).
         if (req.err) begin
           pmp_queue.put(req.address);
         end

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_resp_item.sv

@@ -9,7 +9,6 @@ class ibex_icache_mem_resp_item extends uvm_sequence_item;
   int unsigned min_response_delay = 0;
   int unsigned mid_response_delay = 5;
   int unsigned max_response_delay = 50;
-  int unsigned gap_between_seeds = 499;
 
   // True if this is a granted request. Otherwise, this is the first time we've seen an address (and
   // we might need to drive the PMP line).
@@ -28,9 +27,6 @@ class ibex_icache_mem_resp_item extends uvm_sequence_item;
   // Only has an effect if is_grant. The memory data to reply with (will be 'X if err is set)
   logic [31:0]      rdata;
 
-  // A new memory seed if non-zero. Only has an effect if !is_grant.
-  rand bit [31:0]   seed;
-
   constraint c_delay_dist {
     delay dist {
       min_response_delay                        :/ 5,
@@ -39,31 +35,18 @@ class ibex_icache_mem_resp_item extends uvm_sequence_item;
     };
   }
 
-  constraint c_seed_dist {
-    seed dist {
-      32'd0            :/ gap_between_seeds,
-      [1:32'hffffffff] :/ 1
-    };
-  }
-
   // The delay field has no effect for requests (i.e. if is_grant is false). Force it to zero rather
   // than leave mysterious numbers in the logs.
   constraint c_no_delay_for_req {
     (!is_grant) -> delay == 0;
   }
 
-  // Similarly, the seed should only be nonzero if is_grant is false.
-  constraint c_no_new_seed_for_gnt {
-    is_grant -> seed == 0;
-  }
-
   `uvm_object_utils_begin(ibex_icache_mem_resp_item)
     `uvm_field_int (is_grant, UVM_DEFAULT)
     `uvm_field_int (err,      UVM_DEFAULT)
     `uvm_field_int (address,  UVM_DEFAULT | UVM_HEX)
     `uvm_field_int (delay,    UVM_DEFAULT)
     `uvm_field_int (rdata,    UVM_DEFAULT | UVM_HEX)
-    `uvm_field_int (seed,     UVM_DEFAULT | UVM_HEX)
   `uvm_object_utils_end
 
   `uvm_object_new

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_sequencer.sv

@@ -12,13 +12,15 @@ class ibex_icache_mem_sequencer
   `uvm_component_new
 
   uvm_tlm_analysis_fifo #(ibex_icache_mem_req_item) request_fifo;
+  uvm_tlm_analysis_fifo #(bit [31:0])               seed_fifo;
 
   // An objection used for heartbeat tracking. Set with register_hb.
   uvm_callbacks_objection hb_objection;
 
   function void build_phase(uvm_phase phase);
     super.build_phase(phase);
-    request_fifo   = new("request_fifo", this);
+    request_fifo = new("request_fifo", this);
+    seed_fifo    = new("seed_fifo", this);
   endfunction
 
   function void register_hb (uvm_callbacks_objection obj);

dv/uvm/icache/dv/ibex_icache_mem_agent/seq_lib/ibex_icache_mem_resp_seq.sv

@@ -7,9 +7,6 @@
 class ibex_icache_mem_resp_seq extends ibex_icache_mem_base_seq;
 
   // Knobs
-  //
-  // gap_between_seeds is the expected number of memory fetches between each seed update.
-  int unsigned gap_between_seeds = 499;
 
   protected ibex_icache_mem_model #(.BusWidth (32)) mem_model;
 
@@ -42,9 +39,6 @@ class ibex_icache_mem_resp_seq extends ibex_icache_mem_base_seq;
     ibex_icache_mem_req_item  req_item  = new("req_item");
     ibex_icache_mem_resp_item resp_item = new("resp_item");
 
-    // Set knob to control randomization
-    resp_item.gap_between_seeds = gap_between_seeds;
-
     forever begin
       // Wait for a transaction request.
       p_sequencer.request_fifo.get(req_item);
@@ -65,27 +59,26 @@ class ibex_icache_mem_resp_seq extends ibex_icache_mem_base_seq;
   //
   task automatic take_req(ibex_icache_mem_resp_item resp_item,
                           ibex_icache_mem_req_item  req_item);
+    bit [31:0] tmp_seed;
 
-    resp_item.is_grant = 1'b0;
-    resp_item.address  = req_item.address;
-    resp_item.rdata    = 'X;
-
-    start_item(resp_item);
-    `DV_CHECK_RANDOMIZE_FATAL(resp_item)
-
-    // Take any new seed (so it will apply to this request and all future requests)
-    if (resp_item.seed != 32'd0) begin
-      `uvm_info(`gfn, $sformatf("New memory seed: 0x%08h", resp_item.seed), UVM_HIGH)
-      cur_seed = resp_item.seed;
+    // Consume any new seeds. These will have been pushed into the sequencer's seed_fifo by the core
+    // agent. Any new seed will apply to this request and all future requests (we need tmp_seed here
+    // because otherwise the first failed call to try_get will trash the value we got).
+    while(p_sequencer.seed_fifo.try_get(tmp_seed)) begin
+      cur_seed = tmp_seed;
+      `uvm_info(`gfn, $sformatf("New memory seed: 0x%08h", cur_seed), UVM_HIGH)
     end
 
-    // Finally, fill in the err signal
-    resp_item.err = mem_model.is_pmp_error(cur_seed, req_item.address);
-
     // Warn the "grant side" to expect this fetch
     pending_grants.push_back({req_item.address, cur_seed});
 
-    // And finish the item
+    resp_item.is_grant = 1'b0;
+    resp_item.address  = req_item.address;
+    resp_item.rdata    = 'X;
+    resp_item.err = mem_model.is_pmp_error(cur_seed, req_item.address);
+
+    start_item(resp_item);
+    `DV_CHECK_RANDOMIZE_FATAL(resp_item)
     finish_item(resp_item);
 
   endtask : take_req