Mem intg testing #1907

GregAC · 2022-11-04T21:46:47Z

This is a mix @ctopal's work in #1811 and work I've done. It supersedes #1811

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

This will prevent seeing mismatches right at the end of our test. Before this change, mem_error_test could inject error at the store instruction in which we finish up the test, resulting with mismatches with Spike and Ibex on the instructions after finishing. Also do the same prevention for signature_addr as well, since we also don't want to corrupt that memory transaction too. Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

This enables the sequence to corrupt the integrity on write responses should it choose to. Previously the driver would always produce correct integrity.

This enables tests to implement custom test completion handling

github-actions · 2022-11-04T21:47:37Z

rtl/ibex_core.sv

      captured_debug_req <= 1'b0;
    end else  begin
      // Capture when ID stage has emptied out and something occurs that will cause a trap and we
      // haven't yet captured
-      if (~instr_valid_id & (new_debug_req | new_irq | new_nmi) & ~captured_valid) begin
+      if (~instr_valid_id & (new_debug_req | new_irq | new_nmi | new_nmi_int) & ~captured_valid) begin


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
Line length exceeds max: 100; is: 102 [Style: line-length] [line-length]

github-actions · 2022-11-04T21:47:37Z

rtl/ibex_core.sv

      rvfi_ext_stage_debug_req[0] <= '0;
    end else if (if_stage_i.instr_valid_id_d & if_stage_i.instr_new_id_d) begin
      rvfi_ext_stage_mip[0]       <= instr_valid_id | ~captured_valid ? cs_registers_i.mip :
                                                                        captured_mip;
      rvfi_ext_stage_nmi[0]       <= instr_valid_id | ~captured_valid ? irq_nm_i :
                                                                        captured_nmi;
+      rvfi_ext_stage_nmi_int[0]   <= instr_valid_id | ~captured_valid ? id_stage_i.controller_i.irq_nm_int :


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
Line length exceeds max: 100; is: 108 [Style: line-length] [line-length]

github-actions · 2022-11-04T21:47:37Z

dv/uvm/core_ibex/common/ibex_mem_intf_agent/ibex_mem_intf_response_seq_lib.sv

      end
      // Add integrity bits
      {req.intg, req.data} = prim_secded_pkg::prim_secded_inv_39_32_enc(req.data);

      // If data_was_uninitialized is true then we want to force bad integrity bits: invert the
      // correct ones, which we know will break things for the codes we use.
-      if (p_sequencer.cfg.enable_bad_intg_on_uninit_access &&
-          data_was_uninitialized) begin
+      if ((p_sequencer.cfg.enable_bad_intg_on_uninit_access && data_was_uninitialized) || enable_intg_error) begin


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
Line length exceeds max: 100; is: 114 [Style: line-length] [line-length]

Put enable_int_error on new line.

github-actions · 2022-11-04T21:47:38Z

dv/uvm/core_ibex/tb/core_ibex_tb_top.sv

@@ -221,6 +223,9 @@ module core_ibex_tb_top;
  assign dut_if.rf_ren_b         = dut.u_ibex_top.u_ibex_core.rf_ren_b;
  assign dut_if.rf_rd_a_wb_match = dut.u_ibex_top.u_ibex_core.rf_rd_a_wb_match;
  assign dut_if.rf_rd_b_wb_match = dut.u_ibex_top.u_ibex_core.rf_rd_b_wb_match;
+  assign dut_if.sync_exc_seen    = dut.u_ibex_top.u_ibex_core.cs_registers_i.cpuctrlsts_part_q.sync_exc_seen;


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
Line length exceeds max: 100; is: 109 [Style: line-length] [line-length]

This test picks between inserting an integrity error or a bus error to the response in the case of a memory request from Ibex. Introduces a control knob `enable_mem_intg_err` which can control the rate of having integrity errors per request. This commit also disables checking for double fault alerts in the scoreboard because they're expected to be seen while simulating and they don't cause infinite loop problems because every time a memory response is requested the error causing part is just randomized. That means Ibex trying to execute same instruction again would have a chance to succeed this time. Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

This commit is mainly an extension to cosim environment to drive the newly introduced state variable `nmi_int` in Spike. This commit - Extends RVFI interface by a single bit (ext_nmi_int) - Configures cosim to set nmi_int inside Spike Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

When Ibex does a load that receives data with bad integrity it suppresses the write to the destination register. The implements matching functionality for cosim.

ctopal

LGTM

marnovandermaas · 2022-11-07T15:42:46Z

dv/uvm/core_ibex/tests/core_ibex_new_seq_lib.sv

+    stop_seq = 1'b0;
+    seq_finished = 1'b0;


Squash this one and the first commit?

marnovandermaas · 2022-11-07T15:52:49Z

dv/uvm/core_ibex/common/ibex_mem_intf_agent/ibex_mem_intf_response_seq_lib.sv

      end
      // Add integrity bits
      {req.intg, req.data} = prim_secded_pkg::prim_secded_inv_39_32_enc(req.data);

      // If data_was_uninitialized is true then we want to force bad integrity bits: invert the
      // correct ones, which we know will break things for the codes we use.
-      if (p_sequencer.cfg.enable_bad_intg_on_uninit_access &&
-          data_was_uninitialized) begin
+      if ((p_sequencer.cfg.enable_bad_intg_on_uninit_access && data_was_uninitialized) || enable_intg_error) begin


Put enable_int_error on new line.

marnovandermaas · 2022-11-07T15:54:53Z

dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml

+    +instr_cnt=10000
+    +randomize_csr=1
+    +enable_unaligned_load_store=1
+    +suppress_pmp_setup=1


What about the disable_pmp_exception_handler routine?

marnovandermaas · 2022-11-07T16:00:05Z

dv/cosim/spike_cosim.cc

@@ -376,6 +377,12 @@ bool SpikeCosim::check_sync_trap(uint32_t write_reg,
    return false;
  }

+  // If we see an internal NMI, that means we receive an extra memory intf item.
+  // Deleting that is necessary since next Load/Store would fail otherwise.
+  if (processor->get_state()->mcause->read() == 0xFFFFFFE0) {


Why don't we check the nmi_int value instead of mcause?

marnovandermaas · 2022-11-07T16:21:52Z

dv/cosim/spike_cosim.cc

+  if (suppress_reg_write) {
+    // If we suppressed a register write restore the old register state now
+    processor->get_state()->XPR.write(suppressed_write_reg,
+                                      suppressed_write_reg_data);
+  }


Isn't there a way to stop the write in the first place instead of undoing it?

marnovandermaas · 2022-11-07T16:25:56Z

dv/cosim/spike_cosim.cc

+bool SpikeCosim::pc_is_load(uint32_t pc, uint32_t &rd_out) {
+  uint16_t insn_16;
+
+  if (!backdoor_read_mem(pc, 2, reinterpret_cast<uint8_t *>(&insn_16))) {
+    return false;
+  }
+
+  // C.LW
+  if ((insn_16 & 0xE003) == 0x4000) {
+    rd_out = ((insn_16 >> 2) & 0x7) + 8;
+    return true;
+  }
+
+  // C.LWSP
+  if ((insn_16 & 0xE003) == 0x4002) {
+    rd_out = (insn_16 >> 7) & 0x1F;
+    return rd_out != 0;
+  }
+
+  uint16_t insn_32;
+
+  if (!backdoor_read_mem(pc, 4, reinterpret_cast<uint8_t *>(&insn_32))) {
+    return false;
+  }
+
+  // LB/LH/LW/LBU/LHU
+  if ((insn_32 & 0x7F) == 0x3) {
+    uint32_t func = (insn_32 >> 12) & 0x7;
+    if ((func == 0x3) || (func == 0x6) || (func == 0x7)) {
+      // Not valid load encodings
+      return false;
+    }
+
+    rd_out = (insn_32 >> 7) & 0x1F;
+    return true;
+  }
+
+  return false;
+}


I don't like that this replicates decode logic, although I don't have a concrete alternative in mind.

ctopal and others added 6 commits November 4, 2022 20:35

[ibex,dv] Switch to new sequence for memory errors

ec0b034

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

Fix-up new_seq_lib to start after stopping

3c12971

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

[dv] Set rdata on write response in sequence not driver

ec36d5d

This enables the sequence to corrupt the integrity on write responses should it choose to. Previously the driver would always produce correct integrity.

[dv] Add custom test done functionality

cd641e2

This enables tests to implement custom test completion handling

[dv] Add probe and wait functionality for traps

85d3072

GregAC requested review from ctopal and hcallahan-lowrisc November 4, 2022 21:46

github-actions bot reviewed Nov 4, 2022

View reviewed changes

GregAC mentioned this pull request Nov 5, 2022

[dv] Ibex V2 Actions #1901

Closed

15 tasks

GregAC force-pushed the mem_intg_testing branch 3 times, most recently from 7f9e142 to a279618 Compare November 6, 2022 12:07

ctopal added 2 commits November 6, 2022 15:45

Fixup RVFI connection for pc_wdata

64c2bec

Signed-off-by: Canberk Topal <ctopal@lowrisc.org>

GregAC force-pushed the mem_intg_testing branch from a279618 to 213dc1b Compare November 6, 2022 15:46

GregAC added 2 commits November 6, 2022 16:02

[cosim] Add write suppress support

70638e7

When Ibex does a load that receives data with bad integrity it suppresses the write to the destination register. The implements matching functionality for cosim.

[dv] Disable bad integrity on uninit accesses for mem error tests

53ed7bd

GregAC force-pushed the mem_intg_testing branch from 213dc1b to 53ed7bd Compare November 6, 2022 16:02

ctopal approved these changes Nov 7, 2022

View reviewed changes

ctopal mentioned this pull request Nov 7, 2022

[dv] V2S Coverage Implementation #1895

Merged

GregAC merged commit 7592bb9 into lowRISC:master Nov 7, 2022

GregAC deleted the mem_intg_testing branch November 7, 2022 16:24

marnovandermaas reviewed Nov 7, 2022

View reviewed changes

This was referenced Nov 16, 2022

[dv,cosim] Implement internal interrupts in co-sim #1732

Closed

[dv] Add ecc error generation to memory agent #1731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mem intg testing #1907

Mem intg testing #1907

Uh oh!

GregAC commented Nov 4, 2022

Uh oh!

github-actions bot Nov 4, 2022

Uh oh!

github-actions bot Nov 4, 2022

Uh oh!

github-actions bot Nov 4, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

github-actions bot Nov 4, 2022

Uh oh!

ctopal left a comment

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

marnovandermaas Nov 7, 2022

Uh oh!

Uh oh!

Mem intg testing #1907

Mem intg testing #1907

Uh oh!

Conversation

GregAC commented Nov 4, 2022

Uh oh!

github-actions bot Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

ctopal left a comment

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

marnovandermaas Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!