diff --git a/doc/icache.rst b/doc/icache.rst index df8c9dfb..16691504 100644 --- a/doc/icache.rst +++ b/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. diff --git a/dv/uvm/icache/doc/ibex_icache_dv_plan.md b/dv/uvm/icache/doc/ibex_icache_dv_plan.md index 53e61895..e6c4877f 100644 --- a/dv/uvm/icache/doc/ibex_icache_dv_plan.md +++ b/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 diff --git a/dv/uvm/icache/dv/env/ibex_icache_env.sv b/dv/uvm/icache/dv/env/ibex_icache_env.sv index 261cddfe..c2469bc3 100644 --- a/dv/uvm/icache/dv/env/ibex_icache_env.sv +++ b/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 diff --git a/dv/uvm/icache/dv/env/seq_lib/ibex_icache_passthru_vseq.sv b/dv/uvm/icache/dv/env/seq_lib/ibex_icache_passthru_vseq.sv index a3089a14..dbaaea3c 100644 --- a/dv/uvm/icache/dv/env/seq_lib/ibex_icache_passthru_vseq.sv +++ b/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 diff --git a/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_driver.sv b/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_driver.sv index 6c1419ee..689145ff 100644 --- a/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_driver.sv +++ b/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 diff --git a/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_req_item.sv b/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_req_item.sv index 3f041d6f..39d49f0e 100644 --- a/dv/uvm/icache/dv/ibex_icache_core_agent/ibex_icache_core_req_item.sv +++ b/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 diff --git a/dv/uvm/icache/dv/ibex_icache_core_agent/seq_lib/ibex_icache_core_base_seq.sv b/dv/uvm/icache/dv/ibex_icache_core_agent/seq_lib/ibex_icache_core_base_seq.sv index 155080db..0c11d1c6 100644 --- a/dv/uvm/icache/dv/ibex_icache_core_agent/seq_lib/ibex_icache_core_base_seq.sv +++ b/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; diff --git a/dv/uvm/icache/dv/ibex_icache_mem_agent/README.md b/dv/uvm/icache/dv/ibex_icache_mem_agent/README.md index 03e57792..c03280dc 100644 --- a/dv/uvm/icache/dv/ibex_icache_mem_agent/README.md +++ b/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. diff --git a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_driver.sv b/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_driver.sv index 8d918f64..21a67429 100644 --- a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_driver.sv +++ b/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 diff --git a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_resp_item.sv b/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_resp_item.sv index ccf641cc..f37ea10c 100644 --- a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_resp_item.sv +++ b/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 diff --git a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_sequencer.sv b/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_sequencer.sv index 8f937329..14c01aca 100644 --- a/dv/uvm/icache/dv/ibex_icache_mem_agent/ibex_icache_mem_sequencer.sv +++ b/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); diff --git a/dv/uvm/icache/dv/ibex_icache_mem_agent/seq_lib/ibex_icache_mem_resp_seq.sv b/dv/uvm/icache/dv/ibex_icache_mem_agent/seq_lib/ibex_icache_mem_resp_seq.sv index de96756b..1f69a133 100644 --- a/dv/uvm/icache/dv/ibex_icache_mem_agent/seq_lib/ibex_icache_mem_resp_seq.sv +++ b/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