Rework choosing new seeds in icache UVM memory model

The flow for a memory fetch is:

  1. Cache requests data for a memory address
  2. Agent spots the request, maybe signalling a PMP error
  3. Grant line goes high, at which point the request is granted.
  4. Sometime later (in-order pipeline), agent sends a response

Occasionally, we need to pick a new seed for the backing memory.
Before this patch, we picked these seeds at point (3).

Unfortunately this was wrong in the following case:

  1. We're switching from seed S0 to seed S1.
  2. The request is spotted with seed S0 and doesn't signal a PMP error
  3. The request is granted and we switch to seed S1.
  4. We respond with data from memory based on S1, with no memory
     error either

If S1 would have caused a PMP error, the resulting fetch (no error,
but data from S1) doesn't match any possible seed and the scoreboard
gets confused.

This patch changes to picking new seeds at (2) to solve the problem.
This isn't quite enough by itself, because if a request is granted on
a clock-edge, a new request address might appear and there isn't a
guaranteed ordering in the simulation between the new request and the
old grant (both things happen at the same time). To fix this, the
response sequence now maintains a queue of requests and their
corresponding seeds to make sure that all the checks for a fetch are
done with a single seed.

The patch also gets rid of the seed state in the memory model: it
turns out that this didn't really help: the scoreboard is always
asking "what would I get with this seed?" and now the sequence is
doing something similar.

dv/uvm/icache/dv/env/ibex_icache_scoreboard.sv

@@ -133,14 +133,21 @@ class ibex_icache_scoreboard
       mem_fifo.get(item);
       `uvm_info(`gfn, $sformatf("received mem transaction:\n%0s", item.sprint()), UVM_HIGH)
 
-      if (item.is_response) begin
-        busy_check();
-        assert(mem_trans_count > 0);
-        mem_trans_count -= 1;
-      end else begin
-        mem_trans_count += 1;
-        if (item.seed != 32'd0) mem_seeds.push_back(item.seed);
-      end
+      case (item.trans_type)
+        ICacheMemNewSeed: begin
+          mem_seeds.push_back(item.data);
+        end
+
+        ICacheMemGrant: begin
+          mem_trans_count += 1;
+        end
+
+        ICacheMemResponse: begin
+          busy_check();
+          `DV_CHECK_FATAL(mem_trans_count > 0);
+          mem_trans_count -= 1;
+        end
+      endcase
     end
   endtask
 
@@ -321,12 +328,11 @@ class ibex_icache_scoreboard
     // compressed instruction, in which case we don't care about the top bits anyway.
     lo_bits_to_drop = 8 * (address - addr_lo);
 
-    mem_model.set_seed(seed);
-    rdata = mem_model.read_data(addr_lo) >> lo_bits_to_drop;
+    rdata = mem_model.read_data(seed, addr_lo) >> lo_bits_to_drop;
 
     return is_fetch_compatible_1(seen_insn_data,
                                  seen_err,
-                                 mem_model.is_either_error(addr_lo),
+                                 mem_model.is_either_error(seed, addr_lo),
                                  rdata[31:0],
                                  seed,
                                  chatty);
@@ -365,16 +371,14 @@ class ibex_icache_scoreboard
     lo_bits_to_drop = BusWidth - lo_bits_to_take;
 
     // Do the first read (from the low address) and shift right to drop the bits that we don't need.
-    mem_model.set_seed(seed_lo);
-    exp_err_lo = mem_model.is_either_error(addr_lo);
-    rdata      = mem_model.read_data(addr_lo) >> lo_bits_to_drop;
+    exp_err_lo = mem_model.is_either_error(seed_lo, addr_lo);
+    rdata      = mem_model.read_data(seed_lo, addr_lo) >> lo_bits_to_drop;
     exp_data   = rdata[31:0];
 
     // Now do the second read (from the upper address). Shift the result up by lo_bits_to_take,
     // which will discard some top bits. Then extract 32 bits and OR with what we have so far.
-    mem_model.set_seed(seed_hi);
-    exp_err_hi = mem_model.is_either_error(addr_hi);
-    rdata      = mem_model.read_data(addr_hi) << lo_bits_to_take;
+    exp_err_hi = mem_model.is_either_error(seed_hi, addr_hi);
+    rdata      = mem_model.read_data(seed_hi, addr_hi) << lo_bits_to_take;
     exp_data   = exp_data | rdata[31:0];
 
     return is_fetch_compatible_2(seen_insn_data, seen_err_plus2,

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_agent_pkg.sv

@@ -12,7 +12,11 @@ package ibex_icache_mem_agent_pkg;
   `include "uvm_macros.svh"
   `include "dv_macros.svh"
 
-  // parameters
+  typedef enum {
+    ICacheMemNewSeed,
+    ICacheMemGrant,
+    ICacheMemResponse
+  } ibex_icache_mem_trans_type_e;
 
   // package sources
   `include "ibex_icache_mem_req_item.sv"

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_bus_item.sv

@@ -8,23 +8,20 @@
 
 class ibex_icache_mem_bus_item extends uvm_sequence_item;
 
-  // Is this a request or a response?
-  logic        is_response;
+  // What sort of transaction is this? (new seed, grant or response)
+  ibex_icache_mem_trans_type_e trans_type;
 
-  // Request address and possible new seed (only valid for request transactions)
-  logic [31:0] address;
-  bit   [31:0] seed;
+  // This holds the new seed for a 'new seed' transaction, the request address for a grant
+  // transaction and the returned rdata for a response transaction.
+  logic [31:0] data;
 
-  // Response data and error flags (only valid for response transactions)
-  logic [31:0] rdata;
+  // Response error flag (only valid for response transactions)
   logic        err;
 
   `uvm_object_utils_begin(ibex_icache_mem_bus_item)
-    `uvm_field_int(is_response, UVM_DEFAULT)
-    `uvm_field_int(address,     UVM_DEFAULT | UVM_HEX)
-    `uvm_field_int(seed,        UVM_DEFAULT | UVM_HEX)
-    `uvm_field_int(rdata,       UVM_DEFAULT | UVM_HEX)
-    `uvm_field_int(err,         UVM_DEFAULT)
+    `uvm_field_enum(ibex_icache_mem_trans_type_e, trans_type, UVM_DEFAULT)
+    `uvm_field_int(data, UVM_DEFAULT | UVM_HEX)
+    `uvm_field_int(err,  UVM_DEFAULT)
   `uvm_object_utils_end
 
   `uvm_object_new

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_model.sv

@@ -24,7 +24,6 @@
 class ibex_icache_mem_model #(parameter int unsigned BusWidth = 32)
   extends uvm_object;
 
-  protected bit [31:0] seed = 32'd0;
   protected bit        no_pmp_errs = 0;
   protected bit        no_mem_errs = 0;
 
@@ -34,49 +33,41 @@ class ibex_icache_mem_model #(parameter int unsigned BusWidth = 32)
   endfunction
 
   `uvm_object_utils_begin(ibex_icache_mem_model)
-     `uvm_field_int (seed,        UVM_DEFAULT | UVM_HEX)
      `uvm_field_int (no_pmp_errs, UVM_DEFAULT)
      `uvm_field_int (no_mem_errs, UVM_DEFAULT)
   `uvm_object_utils_end
 
-  function void set_seed(bit [31:0] new_seed);
-    seed = new_seed;
-  endfunction
-
   // Return true if reading from BusWidth bits from address should give an error
-  function automatic logic is_error(logic [31:0] address);
+  protected function automatic logic is_error(bit [31:0] seed, logic [31:0] address);
     logic [31:0] rng_lo, rng_hi, rng_w0, rng_w1;
     rng_lo = seed ^ 32'hdeadbeef;
     rng_w0 = 32'd1 << (32 - 3);
     rng_w1 = (~32'd0) - rng_lo;
     rng_hi = rng_lo + ((rng_w0 < rng_w1) ? rng_w0 : rng_w1);
 
-    return ranges_overlap(address, address + BusWidth / 8, rng_lo, rng_hi);
-  endfunction
+    // Return true iff [address, address + BusWidth / 8) and [rng_lo, rng_hi) have nonempty
+    // intersection.
+    return (address < rng_hi) && (rng_lo < address + BusWidth / 8);
 
-  // Return true iff [lo0, hi0) and [lo1, hi1) have nonempty intersection
-  function automatic logic ranges_overlap(logic [31:0] lo0, logic [31:0] hi0,
-                                          logic [31:0] lo1, logic [31:0] hi1);
-    return (lo0 < hi1) && (lo1 < hi0);
   endfunction
 
   // Return true if reading BusWidth bits from address should give a PMP error
-  function automatic logic is_pmp_error(logic [31:0] address);
-    return (! no_pmp_errs) && is_error(address ^ 32'h12344321);
+  function automatic logic is_pmp_error(bit [31:0] seed, logic [31:0] address);
+    return (! no_pmp_errs) && is_error(seed, address ^ 32'h12344321);
   endfunction
 
   // Return true if reading BusWidth bits from address should give a memory error
-  function automatic logic is_mem_error(logic [31:0] address);
-    return (! no_mem_errs) && is_error(address ^ 32'hf00dbeef);
+  function automatic logic is_mem_error(bit [31:0] seed, logic [31:0] address);
+    return (! no_mem_errs) && is_error(seed, address ^ 32'hf00dbeef);
   endfunction
 
   // Return true if reading BusWidth bits from address should give some sort of error
-  function automatic logic is_either_error(logic [31:0] address);
-    return is_pmp_error(address) || is_mem_error(address);
+  function automatic logic is_either_error(bit [31:0] seed, logic [31:0] address);
+    return is_pmp_error(seed, address) || is_mem_error(seed, address);
   endfunction
 
   // Return BusWidth bits of data from reading at address.
-  function automatic logic [BusWidth-1:0] read_data(logic [31:0] address);
+  function automatic logic [BusWidth-1:0] read_data(bit [31:0] seed, logic [31:0] address);
     logic [BusWidth-1:0] acc, word_data;
     int                  word_count, lo_idx, lo_bit;
     logic [29:0]         word_addr;
@@ -98,7 +89,7 @@ class ibex_icache_mem_model #(parameter int unsigned BusWidth = 32)
       // Note that the address sum might wrap (if we read off the top of memory), but that's not
       // really a problem.
       word_data = 0;
-      word_data[31:0] = read_word(word_addr + i[29:0]);
+      word_data[31:0] = read_word(seed, word_addr + i[29:0]);
 
       // The bottom valid byte in word_data is normally 0, but is positive if i is zero (the bottom)
       // word and the read was misaligned. In that case, it equals i & 3.
@@ -115,7 +106,7 @@ class ibex_icache_mem_model #(parameter int unsigned BusWidth = 32)
   endfunction
 
   // Read 32 bits of data from reading at word_address.
-  function automatic logic [31:0] read_word(logic [29:0] address);
+  function automatic logic [31:0] read_word(bit [31:0] seed, logic [29:0] address);
     return hash({2'b0, address} ^ seed);
   endfunction
 

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_monitor.sv

@@ -76,11 +76,9 @@ class ibex_icache_mem_monitor
     forever begin
       if (cfg.vif.monitor_cb.rvalid) begin
         bus_trans = ibex_icache_mem_bus_item::type_id::create("bus_trans");
-        bus_trans.is_response = 1'b1;
-        bus_trans.address     = '0;
-        bus_trans.seed        = '0;
-        bus_trans.rdata       = cfg.vif.monitor_cb.rdata;
-        bus_trans.err         = cfg.vif.monitor_cb.err;
+        bus_trans.trans_type = ICacheMemResponse;
+        bus_trans.data       = cfg.vif.monitor_cb.rdata;
+        bus_trans.err        = cfg.vif.monitor_cb.err;
         analysis_port.write(bus_trans);
       end
 
@@ -88,33 +86,42 @@ class ibex_icache_mem_monitor
     end
   endtask
 
-  // This is called immediately when an address is requested and is used to drive the PMP response
+  // This is called immediately when an address is requested and is used to drive the PMP response.
+  // If we decide to choose a new seed with this transaction, the seed also gets passed to the
+  // scoreboard.
   function automatic void new_request(logic [31:0] addr);
+    ibex_icache_mem_bus_item bus_trans;
     ibex_icache_mem_req_item item = new("item");
 
     item.is_grant = 1'b0;
     item.address = addr;
-    item.seed = '0;
+    `DV_CHECK_RANDOMIZE_FATAL(item)
     request_port.write(item);
+
+    if (item.seed != 0) begin
+      bus_trans = ibex_icache_mem_bus_item::type_id::create("bus_trans");
+      bus_trans.trans_type = ICacheMemNewSeed;
+      bus_trans.data       = item.seed;
+      bus_trans.err        = '0;
+      analysis_port.write(bus_trans);
+    end
   endfunction
 
   // This is called on a clock edge when an request is granted
   function automatic void new_grant(logic [31:0] addr);
-    ibex_icache_mem_req_item        item;
+    ibex_icache_mem_req_item item;
     ibex_icache_mem_bus_item bus_trans;
 
     item = ibex_icache_mem_req_item::type_id::create("item");
     item.is_grant = 1'b1;
     item.address  = addr;
-    `DV_CHECK_RANDOMIZE_FATAL(item)
+    item.seed     = '0;
     request_port.write(item);
 
     bus_trans = ibex_icache_mem_bus_item::type_id::create("bus_trans");
-    bus_trans.is_response = 1'b0;
-    bus_trans.address     = addr;
-    bus_trans.rdata       = '0;
-    bus_trans.seed        = item.seed;
-    bus_trans.err         = 0;
+    bus_trans.trans_type = ICacheMemGrant;
+    bus_trans.data       = addr;
+    bus_trans.err        = 0;
     analysis_port.write(bus_trans);
   endfunction
 

dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_req_item.sv

@@ -6,11 +6,12 @@
 // memory.
 //
 // When a request first comes in (via a posedge on the req line), it immediately generates a
-// req_item with is_grant = 0. This is used by the driver to decide whether to generate a PMP error.
+// req_item with is_grant = 0. This is used by the sequence to tell the driver whether to generate a
+// PMP error. A req_item with is_grant = 0 may also have a seed update.
 //
 // Assuming that the request wasn't squashed by a PMP error, it will be granted on some later clock
 // edge. At that point, another req_item is generated with is_grant = 1. This is added to a queue in
-// the driver and will be serviced at some later point. This item may also have a seed update.
+// the driver and will be serviced at some later point.
 
 class ibex_icache_mem_req_item extends uvm_sequence_item;
 

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

@@ -17,34 +17,70 @@ class ibex_icache_mem_resp_seq extends ibex_icache_mem_base_seq;
   endtask
 
   task body();
+    // We pick new seeds when we spot a request (rather than when we spot a grant) to ensure that
+    // any given fetch corresponds to exactly one seed. Unfortunately, there's a race if these two
+    // things happen at one clock edge:
+    //
+    //    - Request for new address which gets a new seed
+    //    - Grant of previous request
+    //
+    // If that happens, and the event scheduler happened to schedule the monitor to spot the request
+    // before the grant, we might accidentally handle the grant with the new seed rather than the
+    // old one, which would cause confusion in the scoreboard.
+    //
+    // To avoid this problem, we keep a FIFO of pending addresses along with their corresponding
+    // seeds. When a grant message comes in, we know that we can look up the correct seed there.
+    mailbox #(bit[63:0]) pending_grants = new("pending_grants");
+
     ibex_icache_mem_req_item  req_item  = new("req_item");
     ibex_icache_mem_resp_item resp_item = new("resp_item");
 
+    bit [63:0] gnt_data;
+    bit [31:0] gnt_addr, gnt_seed;
+
+    bit [31:0] cur_seed = 0;
+
     forever begin
       // Wait for a transaction request.
       p_sequencer.request_fifo.get(req_item);
 
       if (!req_item.is_grant) begin
-        // If this is a request (not a grant), check the memory model for a PMP error at this
-        // address. The other fields are ignored.
+        // If this is a request (not a grant), take any new seed and then check the memory model for
+        // a PMP error at this address. Add the address and its corresponding seed to the list of
+        // pending grants.
+        if (req_item.seed != 32'd0) begin
+          `uvm_info(`gfn, $sformatf("New memory seed: 0x%08h", req_item.seed), UVM_HIGH)
+          cur_seed = req_item.seed;
+        end
+
         resp_item.is_grant = 1'b0;
-        resp_item.err      = mem_model.is_pmp_error(req_item.address);
+        resp_item.err      = mem_model.is_pmp_error(cur_seed, req_item.address);
         resp_item.address  = req_item.address;
         resp_item.rdata    = 'X;
 
-      end else begin
-        // If this is a grant, take any new seed then check the memory model for a (non-PMP) error
-        // at this address. On success, look up the memory data too.
+        pending_grants.put({req_item.address, cur_seed});
 
-        if (req_item.seed != 32'd0) begin
-          `uvm_info(`gfn, $sformatf("New memory seed: 0x%08h", req_item.seed), UVM_HIGH)
-          mem_model.set_seed(req_item.seed);
+      end else begin
+        // If this is a grant, search through the pending_grants mailbox to find the seed from the
+        // request that matched.
+        gnt_addr = req_item.address + 1;
+        while (pending_grants.num() > 0) begin
+          pending_grants.get(gnt_data);
+          {gnt_addr, gnt_seed} = gnt_data;
+          if (gnt_addr == req_item.address) break;
         end
 
+        // If nothing went wrong, we should have found the right item and gnt_addr should now
+        // equal req_item.address
+        `DV_CHECK_FATAL(gnt_addr == req_item.address,
+                        $sformatf("No pending grant for address 0x%08h.", req_item.address))
+
+        // Using the seed that we saw for the request, check the memory model for a (non-PMP) error
+        // at this address. On success, look up the memory data too.
         resp_item.is_grant = 1'b1;
-        resp_item.err      = mem_model.is_mem_error(req_item.address);
+        resp_item.err      = mem_model.is_mem_error(gnt_seed, req_item.address);
         resp_item.address  = req_item.address;
-        resp_item.rdata    = resp_item.err ? 'X : mem_model.read_data(req_item.address);
+        resp_item.rdata    = resp_item.err ? 'X : mem_model.read_data(gnt_seed, req_item.address);
       end
 
       // Use the response item as an entry in the sequence, randomising any delay