Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dv] Add spurious responses to memory agent #2185

Merged
merged 1 commit into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions dv/uvm/core_ibex/common/ibex_mem_intf_agent/ibex_mem_intf.sv
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ interface ibex_mem_intf#(
wire misaligned_second;
wire misaligned_first_saw_error;
wire m_mode_access;
wire spurious_response;

clocking request_driver_cb @(posedge clk);
input reset;
Expand All @@ -40,6 +41,7 @@ interface ibex_mem_intf#(
input rdata;
input rintg;
input error;
input spurious_response;
endclocking

clocking response_driver_cb @(posedge clk);
Expand All @@ -55,6 +57,7 @@ interface ibex_mem_intf#(
output rdata;
output rintg;
output error;
output spurious_response;
endclocking

clocking monitor_cb @(posedge clk);
Expand All @@ -74,6 +77,7 @@ interface ibex_mem_intf#(
input misaligned_second;
input misaligned_first_saw_error;
input m_mode_access;
input spurious_response;
endclocking

task automatic wait_clks(input int num);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,28 @@ class ibex_mem_intf_monitor extends uvm_monitor;

protected virtual ibex_mem_intf vif;

// The monitor tick event fires every clock cycle once any writes to
// outstanding_access_port and addr_ph_ports have occurred.
event monitor_tick;
rswarbrick marked this conversation as resolved.
Show resolved Hide resolved

mailbox #(ibex_mem_intf_seq_item) collect_response_queue;
uvm_analysis_port#(ibex_mem_intf_seq_item) item_collected_port;
uvm_analysis_port#(ibex_mem_intf_seq_item) addr_ph_port;
// The number of outstanding accesses is written to this port every clock cycle
uvm_analysis_port#(int) outstanding_accesses_port;
rswarbrick marked this conversation as resolved.
Show resolved Hide resolved

`uvm_component_utils(ibex_mem_intf_monitor)
`uvm_component_new

int outstanding_accesses = 0;

function void build_phase(uvm_phase phase);
super.build_phase(phase);
item_collected_port = new("item_collected_port", this);
addr_ph_port = new("addr_ph_port_monitor", this);
collect_response_queue = new();
outstanding_accesses_port = new("outstanding_accesses_port", this);

if(!uvm_config_db#(virtual ibex_mem_intf)::get(this, "", "vif", vif)) begin
`uvm_fatal("NOVIF",{"virtual interface must be set for: ",get_full_name(),".vif"});
end
Expand Down Expand Up @@ -52,9 +62,23 @@ class ibex_mem_intf_monitor extends uvm_monitor;

virtual protected task collect_address_phase();
ibex_mem_intf_seq_item trans_collected;


forever begin
@(vif.monitor_cb);

trans_collected = ibex_mem_intf_seq_item::type_id::create("trans_collected");
while(!(vif.monitor_cb.request && vif.monitor_cb.grant)) @(vif.monitor_cb);

while (!(vif.monitor_cb.request && vif.monitor_cb.grant)) begin
if (vif.monitor_cb.rvalid && !vif.monitor_cb.spurious_response) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !spurious_response bit seems slightly surprising here. Should the monitor really care about whether the response was the one we were waiting for? I guess the problem is the outstanding_accesses counter, but maybe that really belongs in the driver or the sequence rather than the monitor, since those components know whether they have anything "in flight".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is partially down to laziness on my part but pre this change spurious responses didn't exist and downstream users of the monitor (i.e. those things connecting to the item_collected_port) were expecting everything they receive to represent an actual transaction that occurred (in particular the co-sim scoreboard). If we output spurious responses here we'll need all downstream users of the monitor to filter out spurious responses. This felt like a cleaner design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that feels reasonable. Maybe an example of where "the principled solution" is also the wrong one :-)

outstanding_accesses--;
end
outstanding_accesses_port.write(outstanding_accesses);

-> monitor_tick;
@(vif.monitor_cb);
end

trans_collected.addr = vif.monitor_cb.addr;
trans_collected.be = vif.monitor_cb.be;
trans_collected.misaligned_first = vif.monitor_cb.misaligned_first;
Expand All @@ -63,7 +87,7 @@ class ibex_mem_intf_monitor extends uvm_monitor;
trans_collected.m_mode_access = vif.monitor_cb.m_mode_access;
`uvm_info(get_full_name(), $sformatf("Detect request with address: %0x",
trans_collected.addr), UVM_HIGH)
if(vif.monitor_cb.we) begin
if (vif.monitor_cb.we) begin
trans_collected.read_write = WRITE;
trans_collected.data = vif.monitor_cb.wdata;
trans_collected.intg = vif.monitor_cb.wintg;
Expand All @@ -73,7 +97,14 @@ class ibex_mem_intf_monitor extends uvm_monitor;
addr_ph_port.write(trans_collected);
`uvm_info(get_full_name(),"Send through addr_ph_port", UVM_HIGH)
collect_response_queue.put(trans_collected);
@(vif.monitor_cb);

outstanding_accesses++;
if (vif.monitor_cb.rvalid && !vif.monitor_cb.spurious_response) begin
outstanding_accesses--;
end
outstanding_accesses_port.write(outstanding_accesses);

-> monitor_tick;
end
endtask : collect_address_phase

Expand All @@ -83,7 +114,7 @@ class ibex_mem_intf_monitor extends uvm_monitor;
collect_response_queue.get(trans_collected);
do
@(vif.monitor_cb);
while(vif.monitor_cb.rvalid === 0);
while(vif.monitor_cb.rvalid === 0 || vif.monitor_cb.spurious_response === 1);

if (trans_collected.read_write == READ) begin
trans_collected.data = vif.monitor_cb.rdata;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class ibex_mem_intf_request_driver extends uvm_driver #(ibex_mem_intf_seq_item);
forever begin
rdata_queue.get(tr);
vif.wait_clks(1);
while(vif.rvalid !== 1'b1) vif.wait_clks(1);
while((vif.rvalid !== 1'b1 || vif.spurious_response === 1'b1)) vif.wait_clks(1);
if(tr.read_write == READ)
tr.data = vif.request_driver_cb.rdata;
tr.intg = vif.request_driver_cb.rintg;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,12 @@ class ibex_mem_intf_response_agent extends uvm_agent;
if(get_is_active() == UVM_ACTIVE) begin
driver.seq_item_port.connect(sequencer.seq_item_export);
monitor.addr_ph_port.connect(sequencer.addr_ph_port.analysis_export);
monitor.outstanding_accesses_port.connect(sequencer.outstanding_accesses_imp);
end
driver.cfg = cfg;
sequencer.cfg = cfg;

sequencer.monitor_tick = monitor.monitor_tick;
endfunction : connect_phase

function void reset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,25 @@ class ibex_mem_intf_response_agent_cfg extends uvm_object;
// CONTROL_KNOB : enable/disable to generation of bad integrity upon uninit accesses
bit enable_bad_intg_on_uninit_access = 0;

int unsigned spurious_response_delay_min = 0;
int unsigned spurious_response_delay_max = 100;

constraint zero_delays_c {
zero_delays dist {1 :/ zero_delay_pct,
0 :/ 100 - zero_delay_pct};
}

`uvm_object_utils_begin(ibex_mem_intf_response_agent_cfg)
`uvm_field_int(fixed_data_write_response, UVM_DEFAULT)
`uvm_field_int(gnt_delay_min, UVM_DEFAULT)
`uvm_field_int(gnt_delay_max, UVM_DEFAULT)
`uvm_field_int(valid_delay_min, UVM_DEFAULT)
`uvm_field_int(valid_delay_max, UVM_DEFAULT)
`uvm_field_int(zero_delays, UVM_DEFAULT)
`uvm_field_int(zero_delay_pct, UVM_DEFAULT)
`uvm_field_int(fixed_data_write_response, UVM_DEFAULT)
`uvm_field_int(gnt_delay_min, UVM_DEFAULT)
`uvm_field_int(gnt_delay_max, UVM_DEFAULT)
`uvm_field_int(valid_delay_min, UVM_DEFAULT)
`uvm_field_int(valid_delay_max, UVM_DEFAULT)
`uvm_field_int(zero_delays, UVM_DEFAULT)
`uvm_field_int(zero_delay_pct, UVM_DEFAULT)
`uvm_field_int(spurious_response_delay_min, UVM_DEFAULT)
`uvm_field_int(spurious_response_delay_max, UVM_DEFAULT)

`uvm_object_utils_end

function new(string name = "");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class ibex_mem_intf_response_driver extends uvm_driver #(ibex_mem_intf_seq_item)

virtual protected task get_and_drive();
wait (cfg.vif.response_driver_cb.reset === 1'b0);

fork
begin
forever begin
Expand Down Expand Up @@ -113,18 +114,33 @@ class ibex_mem_intf_response_driver extends uvm_driver #(ibex_mem_intf_seq_item)
ibex_mem_intf_seq_item tr;
forever begin
@(cfg.vif.response_driver_cb);
cfg.vif.response_driver_cb.rvalid <= 1'b0;
cfg.vif.response_driver_cb.rdata <= 'x;
cfg.vif.response_driver_cb.rintg <= 'x;
cfg.vif.response_driver_cb.error <= 'x;
cfg.vif.response_driver_cb.rvalid <= 1'b0;
cfg.vif.response_driver_cb.spurious_response <= 1'b0;
cfg.vif.response_driver_cb.rdata <= 'x;
cfg.vif.response_driver_cb.rintg <= 'x;
cfg.vif.response_driver_cb.error <= 'x;

rdata_queue.get(tr);

`uvm_info(`gfn, $sformatf("Got response for addr %x", tr.addr), UVM_HIGH)

if(cfg.vif.response_driver_cb.reset) continue;

repeat (tr.rvalid_delay) @(cfg.vif.response_driver_cb);

if(~cfg.vif.response_driver_cb.reset) begin
if (tr.spurious_response) begin
`uvm_info(`gfn, $sformatf("Driving spurious response"), UVM_HIGH)
end else begin
`uvm_info(`gfn, $sformatf("Driving response for addr %x", tr.addr), UVM_HIGH)
end

cfg.vif.response_driver_cb.rvalid <= 1'b1;
cfg.vif.response_driver_cb.error <= tr.error;
// A spurious response is not associated with any request. This is flagged in the vif
// signals so other components in the testbench can ignore them if needed.
cfg.vif.response_driver_cb.spurious_response <= tr.spurious_response;

if (tr.read_write == READ) begin
cfg.vif.response_driver_cb.rdata <= tr.data;
cfg.vif.response_driver_cb.rintg <= tr.intg;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,20 @@ class ibex_mem_intf_response_seq extends uvm_sequence #(ibex_mem_intf_seq_item);
bit error_synch = 1'b1;
bit is_dmem_seq = 1'b0;
bit suppress_error_on_exc = 1'b0;
bit enable_spurious_response = 1'b0;


`uvm_object_utils(ibex_mem_intf_response_seq)
`uvm_declare_p_sequencer(ibex_mem_intf_response_sequencer)
`uvm_object_new

rand int unsigned spurious_response_delay_cycles;

constraint spurious_response_delay_cycles_c {
spurious_response_delay_cycles inside {[p_sequencer.cfg.spurious_response_delay_min :
p_sequencer.cfg.spurious_response_delay_max]};
}

virtual task body();
virtual core_ibex_dut_probe_if ibex_dut_vif;

Expand All @@ -33,6 +42,9 @@ class ibex_mem_intf_response_seq extends uvm_sequence #(ibex_mem_intf_seq_item);

if (m_mem == null) `uvm_fatal(get_full_name(), "Cannot get memory model")
`uvm_info(`gfn, $sformatf("is_dmem_seq: 0x%0x", is_dmem_seq), UVM_LOW)

`DV_CHECK_MEMBER_RANDOMIZE_FATAL(spurious_response_delay_cycles)

forever
begin
bit [ADDR_WIDTH-1:0] aligned_addr;
Expand All @@ -41,12 +53,48 @@ class ibex_mem_intf_response_seq extends uvm_sequence #(ibex_mem_intf_seq_item);
bit [INTG_WIDTH-1:0] read_intg;
bit data_was_uninitialized = 1'b0;

p_sequencer.addr_ph_port.get(item);
if (enable_spurious_response) begin
// When spurious responses are enabled we wake every monitor tick to decide whether to
// insert a spurious response.
while (1) begin
@p_sequencer.monitor_tick;

if (p_sequencer.addr_ph_port.try_get(item)) begin
// If we have a new request proceed as normal.
break;
end

if ((spurious_response_delay_cycles == 0)
&& (p_sequencer.outstanding_accesses == 0)) begin

// If we've hit the time generate a new spurious responses and there's no outstanding
// responses (we must only generate a spurious response when the interface is idle)
// send one to the driver.
req = ibex_mem_intf_seq_item::type_id::create("req");

`DV_CHECK_RANDOMIZE_WITH_FATAL(req, rvalid_delay == 0;)

req.spurious_response = 1'b1;
{req.intg, req.data} = prim_secded_pkg::prim_secded_inv_39_32_enc(req.data);

`uvm_info(`gfn, $sformatf("Generated spurious response:\n%0s", req.sprint()), UVM_HIGH)
start_item(req);
finish_item(req);

`DV_CHECK_MEMBER_RANDOMIZE_FATAL(spurious_response_delay_cycles)
rswarbrick marked this conversation as resolved.
Show resolved Hide resolved
end else if (spurious_response_delay_cycles > 0) begin
spurious_response_delay_cycles = spurious_response_delay_cycles - 1;
end
end
end else begin
// Without spurious responses just wait for the monitor to report a new request
p_sequencer.addr_ph_port.get(item);
end

aligned_addr = {item.addr[DATA_WIDTH-1:2], 2'b0};

req = ibex_mem_intf_seq_item::type_id::create("req");
error_synch = 1'b0;

if (suppress_error_on_exc &&
(ibex_dut_vif.dut_cb.sync_exc_seen || ibex_dut_vif.dut_cb.irq_exc_seen)) begin
enable_error = 1'b0;
Expand Down Expand Up @@ -75,6 +123,7 @@ class ibex_mem_intf_response_seq extends uvm_sequence #(ibex_mem_intf_seq_item);
}) begin
`uvm_fatal(`gfn, "Cannot randomize response request")
end

error_synch = 1'b1;
enable_error = 1'b0; // Disable after single inserted error.
aligned_addr = {req.addr[DATA_WIDTH-1:2], 2'b0};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,27 @@ class ibex_mem_intf_response_sequencer extends uvm_sequencer #(ibex_mem_intf_seq

// TLM port to peek the address phase from the response monitor
uvm_tlm_analysis_fifo #(ibex_mem_intf_seq_item) addr_ph_port;
uvm_analysis_imp #(int, ibex_mem_intf_response_sequencer) outstanding_accesses_imp;
ibex_mem_intf_response_agent_cfg cfg;

event monitor_tick = null;
int outstanding_accesses = 0;

`uvm_component_utils(ibex_mem_intf_response_sequencer)

function new (string name, uvm_component parent);
super.new(name, parent);
addr_ph_port = new("addr_ph_port_sequencer", this);
outstanding_accesses_imp = new("outstanding_access_imp", this);
endfunction : new

// On reset, empty the tlm fifo
function void reset();
addr_ph_port.flush();
endfunction

function void write(int x);
outstanding_accesses = x;
endfunction

endclass : ibex_mem_intf_response_sequencer
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class ibex_mem_intf_seq_item extends uvm_sequence_item;
rand bit [3:0] req_delay;
rand bit [5:0] rvalid_delay;
rand bit error;
bit spurious_response;
bit misaligned_first;
bit misaligned_second;
bit misaligned_first_saw_error;
Expand All @@ -35,6 +36,7 @@ class ibex_mem_intf_seq_item extends uvm_sequence_item;
`uvm_field_int (misaligned_second, UVM_DEFAULT)
`uvm_field_int (misaligned_first_saw_error, UVM_DEFAULT)
`uvm_field_int (m_mode_access, UVM_DEFAULT)
`uvm_field_int (spurious_response, UVM_DEFAULT)
`uvm_object_utils_end

`uvm_object_new
Expand Down
8 changes: 8 additions & 0 deletions dv/uvm/core_ibex/env/core_ibex_env_cfg.sv
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ class core_ibex_env_cfg extends uvm_object;
// If '1', reaching the timeout in seconds fatally ends the test.
// If '0', we end the test with a pass.
bit is_timeout_s_fatal = 1;
// If '1' core_ibex_vseq will randomly choose to enable spurious responses in the data side memory
// agent. This will also disable assertions that check the memory interface protocol as spurious
// responses violate them.
bit enable_spurious_dside_responses = 1;

// If spurious responses are enabled what percentage of tests will enable them
int unsigned spurious_response_pct = 20;


`uvm_object_utils_begin(core_ibex_env_cfg)
`uvm_field_int(enable_double_fault_detector, UVM_DEFAULT)
Expand Down
13 changes: 13 additions & 0 deletions dv/uvm/core_ibex/tb/core_ibex_tb_top.sv
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ module core_ibex_tb_top;
`DV_ASSERT_CTRL("tb_rf_rd_mux_b_onehot",
`IBEX_RF_PATH.gen_rdata_mux_check.u_rdata_b_mux.SelIsOnehot_A)

`DV_ASSERT_CTRL("tb_no_spurious_response",
core_ibex_tb_top.dut.u_ibex_top.u_ibex_core.NoMemResponseWithoutPendingAccess)
`DV_ASSERT_CTRL("tb_no_spurious_response",
core_ibex_tb_top.dut.u_ibex_top.MaxOutstandingDSideAccessesCorrect)
`DV_ASSERT_CTRL("tb_no_spurious_response",
core_ibex_tb_top.dut.u_ibex_top.PendingAccessTrackingCorrect)

if (SecureIbex) begin : g_lockstep_assert_ctrl
`define IBEX_LOCKSTEP_PATH core_ibex_tb_top.dut.u_ibex_top.gen_lockstep.u_ibex_lockstep
`DV_ASSERT_CTRL("tb_no_spurious_response",
`IBEX_LOCKSTEP_PATH.u_shadow_core.NoMemResponseWithoutPendingAccess)
end

assign dut.u_ibex_top.u_ibex_core.u_fcov_bind.rf_we_glitch_err =
dut.u_ibex_top.rf_alert_major_internal;

Expand Down
Loading
Loading