Various DV fixes

ci/vars.env

@@ -6,7 +6,7 @@
 # Quote values to ensure they are parsed as string (version numbers might
 # end up as float otherwise).
 VERILATOR_VERSION=v4.210
-IBEX_COSIM_VERSION=15fbd568
+IBEX_COSIM_VERSION=39612f9
 RISCV_TOOLCHAIN_TAR_VERSION=20220210-1
 RISCV_TOOLCHAIN_TAR_VARIANT=lowrisc-toolchain-gcc-rv32imcb
 RISCV_COMPLIANCE_GIT_VERSION=844c6660ef3f0d9b96957991109dfd80cc4938e2

ci/vars.yml

@@ -7,7 +7,7 @@
 # end up as float otherwise).
 variables:
   VERILATOR_VERSION: "v4.210"
-  IBEX_COSIM_VERSION: "15fbd568"
+  IBEX_COSIM_VERSION: "39612f9"
   RISCV_TOOLCHAIN_TAR_VERSION: "20220210-1"
   RISCV_TOOLCHAIN_TAR_VARIANT: "lowrisc-toolchain-gcc-rv32imcb"
   RISCV_COMPLIANCE_GIT_VERSION: "844c6660ef3f0d9b96957991109dfd80cc4938e2"

dv/cosim/cosim.h

@@ -35,6 +35,10 @@ struct DSideAccessInfo {
   // `misaligned_first` set to true, there is no second half.
   bool misaligned_first;
   bool misaligned_second;
+
+  bool misaligned_first_saw_error;
+
+  bool m_mode_access;
 };
 
 class Cosim {
@@ -83,7 +87,7 @@ class Cosim {
   //
   // At the next call of `step`, the MIP value will take effect (i.e. if it's a
   // new interrupt that is enabled it will step straight to that handler).
-  virtual void set_mip(uint32_t mip) = 0;
+  virtual void set_mip(uint32_t pre_mip, uint32_t post_mip) = 0;
 
   // Set the state of the NMI (non-maskable interrupt) line.
   //

dv/cosim/cosim_dpi.cc

@@ -2,11 +2,13 @@
 // Licensed under the Apache License, Version 2.0, see LICENSE for details.
 // SPDX-License-Identifier: Apache-2.0
 
+#include "cosim_dpi.h"
+
 #include <svdpi.h>
+
 #include <cassert>
 
 #include "cosim.h"
-#include "cosim_dpi.h"
 
 int riscv_cosim_step(Cosim *cosim, const svBitVecVal *write_reg,
                      const svBitVecVal *write_reg_data, const svBitVecVal *pc,
@@ -19,10 +21,11 @@ int riscv_cosim_step(Cosim *cosim, const svBitVecVal *write_reg,
              : 0;
 }
 
-void riscv_cosim_set_mip(Cosim *cosim, const svBitVecVal *mip) {
+void riscv_cosim_set_mip(Cosim *cosim, const svBitVecVal *pre_mip,
+                         const svBitVecVal *post_mip) {
   assert(cosim);
 
-  cosim->set_mip(mip[0]);
+  cosim->set_mip(pre_mip[0], post_mip[0]);
 }
 
 void riscv_cosim_set_nmi(Cosim *cosim, svBit nmi) {
@@ -66,17 +69,21 @@ void riscv_cosim_notify_dside_access(Cosim *cosim, svBit store,
                                      svBitVecVal *addr, svBitVecVal *data,
                                      svBitVecVal *be, svBit error,
                                      svBit misaligned_first,
-                                     svBit misaligned_second) {
+                                     svBit misaligned_second,
+                                     svBit misaligned_first_saw_error,
+                                     svBit m_mode_access) {
   assert(cosim);
 
-  cosim->notify_dside_access(
-      DSideAccessInfo{.store = store != 0,
-                      .data = data[0],
-                      .addr = addr[0],
-                      .be = be[0],
-                      .error = error != 0,
-                      .misaligned_first = misaligned_first != 0,
-                      .misaligned_second = misaligned_second != 0});
+  cosim->notify_dside_access(DSideAccessInfo{
+      .store = store != 0,
+      .data = data[0],
+      .addr = addr[0],
+      .be = be[0],
+      .error = error != 0,
+      .misaligned_first = misaligned_first != 0,
+      .misaligned_second = misaligned_second != 0,
+      .misaligned_first_saw_error = misaligned_first_saw_error != 0,
+      .m_mode_access = m_mode_access != 0});
 }
 
 void riscv_cosim_set_iside_error(Cosim *cosim, svBitVecVal *addr) {

dv/cosim/cosim_dpi.h

@@ -8,14 +8,17 @@
 #include <stdint.h>
 #include <svdpi.h>
 
+#include "cosim.h"
+
 // This adapts the C++ interface of the `Cosim` class to be used via DPI. See
 // the documentation in cosim.h for further details
 
 extern "C" {
 int riscv_cosim_step(Cosim *cosim, const svBitVecVal *write_reg,
                      const svBitVecVal *write_reg_data, const svBitVecVal *pc,
                      svBit sync_trap, svBit suppress_reg_write);
-void riscv_cosim_set_mip(Cosim *cosim, const svBitVecVal *mip);
+void riscv_cosim_set_mip(Cosim *cosim, const svBitVecVal *pre_mip,
+                         const svBitVecVal *post_mip);
 void riscv_cosim_set_nmi(Cosim *cosim, svBit nmi);
 void riscv_cosim_set_nmi_int(Cosim *cosim, svBit nmi_int);
 void riscv_cosim_set_debug_req(Cosim *cosim, svBit debug_req);
@@ -27,7 +30,9 @@ void riscv_cosim_notify_dside_access(Cosim *cosim, svBit store,
                                      svBitVecVal *addr, svBitVecVal *data,
                                      svBitVecVal *be, svBit error,
                                      svBit misaligned_first,
-                                     svBit misaligned_second);
+                                     svBit misaligned_second,
+                                     svBit misaligned_first_saw_error,
+                                     svBit m_mode_access);
 void riscv_cosim_set_iside_error(Cosim *cosim, svBitVecVal *addr);
 int riscv_cosim_get_num_errors(Cosim *cosim);
 const char *riscv_cosim_get_error(Cosim *cosim, int index);

dv/cosim/cosim_dpi.svh

@@ -12,7 +12,8 @@
 
 import "DPI-C" function int riscv_cosim_step(chandle cosim_handle, bit [4:0] write_reg,
   bit [31:0] write_reg_data, bit [31:0] pc, bit sync_trap, bit suppress_reg_write);
-import "DPI-C" function void riscv_cosim_set_mip(chandle cosim_handle, bit [31:0] mip);
+import "DPI-C" function void riscv_cosim_set_mip(chandle cosim_handle, bit [31:0] pre_mip,
+  bit [31:0] post_mip);
 import "DPI-C" function void riscv_cosim_set_nmi(chandle cosim_handle, bit nmi);
 import "DPI-C" function void riscv_cosim_set_nmi_int(chandle cosim_handle, bit nmi_int);
 import "DPI-C" function void riscv_cosim_set_debug_req(chandle cosim_handle, bit debug_req);
@@ -22,7 +23,7 @@ import "DPI-C" function void riscv_cosim_set_csr(chandle cosim_handle, int csr_i
 import "DPI-C" function void riscv_cosim_set_ic_scr_key_valid(chandle cosim_handle, bit valid);
 import "DPI-C" function void riscv_cosim_notify_dside_access(chandle cosim_handle, bit store,
   bit [31:0] addr, bit [31:0] data, bit [3:0] be, bit error, bit misaligned_first,
-  bit misaligned_second);
+  bit misaligned_second, bit misaligned_first_saw_error, bit m_mode_access);
 import "DPI-C" function int riscv_cosim_set_iside_error(chandle cosim_handle, bit [31:0] addr);
 import "DPI-C" function int riscv_cosim_get_num_errors(chandle cosim_handle);
 import "DPI-C" function string riscv_cosim_get_error(chandle cosim_handle, int index);

dv/cosim/spike_cosim.cc

@@ -3,17 +3,19 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include "spike_cosim.h"
+
+#include <cassert>
+#include <iostream>
+#include <sstream>
+
 #include "riscv/config.h"
 #include "riscv/decode.h"
 #include "riscv/devices.h"
 #include "riscv/log_file.h"
+#include "riscv/mmu.h"
 #include "riscv/processor.h"
 #include "riscv/simif.h"
 
-#include <cassert>
-#include <iostream>
-#include <sstream>
-
 // For a short time, we're going to support building against version
 // ibex-cosim-v0.2 (20a886c) and also ibex-cosim-v0.3 (9af9730). Unfortunately,
 // they've got different APIs and spike doesn't expose a version string.
@@ -83,26 +85,24 @@ bool SpikeCosim::mmio_load(reg_t addr, size_t len, uint8_t *bytes) {
   bool dut_error = false;
 
   // Incoming access may be an iside or dside access. Use PC to help determine
-  // which.
-  uint32_t pc = processor->get_state()->pc;
+  // which. PC is 64 bits in spike, we only care about the bottom 32-bit so mask
+  // off the top bits.
+  uint64_t pc = processor->get_state()->pc & 0xffffffff;
   uint32_t aligned_addr = addr & 0xfffffffc;
 
   if (pending_iside_error && (aligned_addr == pending_iside_err_addr)) {
     // Check if the incoming access is subject to an iside error, in which case
     // assume it's an iside access and produce an error.
     pending_iside_error = false;
     dut_error = true;
-  } else if (addr < pc || addr >= (pc + 8)) {
+  } else {
     // Spike may attempt to access up to 8-bytes from the PC when fetching, so
-    // only check as a dside access when it falls outside that range.
-
-    // Otherwise check if the aligned PC matches with the aligned address or an
-    // incremented aligned PC (to capture the unaligned 4-byte instruction
-    // case). Assume a successful iside access if either of these checks are
-    // true, otherwise assume a dside access and check against DUT dside
-    // accesses.  If the RTL produced a bus error for the access, or the
-    // checking failed produce a memory fault in spike.
-    dut_error = (check_mem_access(false, addr, len, bytes) != kCheckMemOk);
+    // only check as a dside access when it falls outside that range
+    bool in_iside_range = (addr >= pc && addr < pc + 8);
+
+    if (!in_iside_range) {
+      dut_error = (check_mem_access(false, addr, len, bytes) != kCheckMemOk);
+    }
   }
 
   return !(bus_error || dut_error);
@@ -261,8 +261,8 @@ bool SpikeCosim::step(uint32_t write_reg, uint32_t write_reg_data, uint32_t pc,
     if (pending_sync_exception) {
       if (!sync_trap) {
         std::stringstream err_str;
-        err_str << "Synchronous trap was expected at ISS PC: "
-                << std::hex << processor->get_state()->pc
+        err_str << "Synchronous trap was expected at ISS PC: " << std::hex
+                << processor->get_state()->pc
                 << " but the DUT didn't report one at PC " << pc;
         errors.emplace_back(err_str.str());
         return false;
@@ -294,9 +294,8 @@ bool SpikeCosim::step(uint32_t write_reg, uint32_t write_reg_data, uint32_t pc,
 
   if (pending_iside_error) {
     std::stringstream err_str;
-    err_str << "DUT generated an iside error for address: "
-            << std::hex << pending_iside_err_addr
-            << " but the ISS didn't produce one";
+    err_str << "DUT generated an iside error for address: " << std::hex
+            << pending_iside_err_addr << " but the ISS didn't produce one";
     errors.emplace_back(err_str.str());
     return false;
   }
@@ -329,8 +328,8 @@ bool SpikeCosim::check_retired_instr(uint32_t write_reg,
   if ((processor->get_state()->last_inst_pc & 0xffffffff) != dut_pc) {
     std::stringstream err_str;
     err_str << "PC mismatch, DUT retired : " << std::hex << dut_pc
-            << " , but the ISS retired: "
-            << std::hex << processor->get_state()->last_inst_pc;
+            << " , but the ISS retired: " << std::hex
+            << processor->get_state()->last_inst_pc;
     errors.emplace_back(err_str.str());
     return false;
   }
@@ -385,18 +384,17 @@ bool SpikeCosim::check_retired_instr(uint32_t write_reg,
   return true;
 }
 
-bool SpikeCosim::check_sync_trap(uint32_t write_reg,
-                                 uint32_t dut_pc, uint32_t initial_spike_pc) {
+bool SpikeCosim::check_sync_trap(uint32_t write_reg, uint32_t dut_pc,
+                                 uint32_t initial_spike_pc) {
   // Check if an synchronously-trapping instruction matches
   // between Spike and the DUT.
 
   // Check that both spike and DUT trapped on the same pc
   if (initial_spike_pc != dut_pc) {
     std::stringstream err_str;
-    err_str << "PC mismatch at synchronous trap, DUT at pc: "
-            << std::hex << dut_pc
-            << "while ISS pc is at : "
-            << std::hex << initial_spike_pc;
+    err_str << "PC mismatch at synchronous trap, DUT at pc: " << std::hex
+            << dut_pc << "while ISS pc is at : " << std::hex
+            << initial_spike_pc;
     errors.emplace_back(err_str.str());
     return false;
   }
@@ -411,6 +409,12 @@ bool SpikeCosim::check_sync_trap(uint32_t write_reg,
     return false;
   }
 
+  if ((processor->get_state()->mcause->read() == 0x5) ||
+      (processor->get_state()->mcause->read() == 0x7)) {
+    // We have a load or store access fault, apply fixup for misaligned accesses
+    misaligned_pmp_fixup();
+  }
+
   // 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) {
@@ -577,11 +581,12 @@ void SpikeCosim::initial_proc_setup(uint32_t start_pc, uint32_t start_mtvec,
   }
 }
 
-void SpikeCosim::set_mip(uint32_t mip) {
-  uint32_t new_mip = mip;
+void SpikeCosim::set_mip(uint32_t pre_mip, uint32_t post_mip) {
+  uint32_t new_mip = pre_mip;
   uint32_t old_mip = processor->get_state()->mip->read();
 
-  processor->get_state()->mip->write_with_mask(0xffffffff, mip);
+  processor->get_state()->mip->write_with_mask(0xffffffff, post_mip);
+  processor->get_state()->mip->write_pre_val(pre_mip);
 
   if (processor->get_state()->debug_mode ||
       (processor->halt_request == processor_t::HR_REGULAR) ||
@@ -619,6 +624,62 @@ void SpikeCosim::early_interrupt_handle() {
   }
 }
 
+// Ibex splits misaligned accesses into two separate requests. They
+// independently undergo PMP access checks. It is possible for one to fail (so
+// no request produced for that half of the access) whilst the other successed
+// (producing a request for that half of the access).
+//
+// Spike splits misaligned accesses up into bytes and will apply PMP access
+// checks byte by byte in a linear order. As soon as a byte sees a PMP
+// permission failure the rest of the misaligned access is aborted.
+//
+// This results in mismatches as in some misaligned access cases Ibex will
+// produce a request and spike will not.
+//
+// This fixup detects this condition and removes the Ibex access from
+// pending_dside_accesses to avoid a mismatch. This removed access is checked
+// against PMP using the spike MMU to check spike agrees it passes PMP checks.
+//
+// There may be a better way to handle this (e.g. altering spike behaviour to
+// match Ibex) so for now a warning is generated in fixup cases so they can be
+// easily identified.
+void SpikeCosim::misaligned_pmp_fixup() {
+  if (pending_dside_accesses.size() != 0) {
+    auto &top_pending_access = pending_dside_accesses.front();
+    auto &top_pending_access_info = top_pending_access.dut_access_info;
+
+    // If top access is the second half of a misaligned access where the first
+    // half saw an error we have the PMP fixup case
+    if (top_pending_access_info.misaligned_second &&
+        top_pending_access_info.misaligned_first_saw_error) {
+      mmu_t *mmu = processor->get_mmu();
+
+      // Check if the second half of the access (which Ibex produces a request
+      // for and spike does not) passes PMP
+      if (!mmu->pmp_ok(top_pending_access_info.addr, 4,
+                       top_pending_access_info.store ? STORE : LOAD,
+                       top_pending_access_info.m_mode_access ? PRV_M : PRV_U)) {
+        // Raise an error if the second half shouldn't have passed PMP
+        std::stringstream err_str;
+        err_str << "Saw second half of a misaligned access which not have "
+                << "generated a memory request as it does not pass a PMP check,"
+                << " address: " << std::hex << top_pending_access_info.addr;
+        errors.emplace_back(err_str.str());
+      } else {
+        // Output warning on stdout so we're aware which tests this is happening
+        // in
+        std::cout << "WARNING: Cosim dropping second half of misaligned access "
+                  << "as first half saw an error and second half passed PMP "
+                  << "check, address: " << std::hex
+                  << top_pending_access_info.addr << std::endl;
+        std::cout << std::dec;
+
+        pending_dside_accesses.erase(pending_dside_accesses.begin());
+      }
+    }
+  }
+}
+
 void SpikeCosim::set_nmi(bool nmi) {
   if (nmi && !nmi_mode && !processor->get_state()->debug_mode &&
       processor->halt_request != processor_t::HR_REGULAR) {

dv/cosim/spike_cosim.h

@@ -95,6 +95,8 @@ class SpikeCosim : public simif_t, public Cosim {
 
   void early_interrupt_handle();
 
+  void misaligned_pmp_fixup();
+
   unsigned int insn_cnt;
 
  public:
@@ -123,7 +125,7 @@ class SpikeCosim : public simif_t, public Cosim {
                            uint32_t dut_pc, bool suppress_reg_write);
   bool check_sync_trap(uint32_t write_reg, uint32_t pc,
                        uint32_t initial_spike_pc);
-  void set_mip(uint32_t mip) override;
+  void set_mip(uint32_t pre_mip, uint32_t post_mip) override;
   void set_nmi(bool nmi) override;
   void set_nmi_int(bool nmi_int) override;
   void set_debug_req(bool debug_req) override;