Update trigger behavior for memory accesses to match recommended debug specification behavior

According to the debug specification (select bit description):

0 (address): There is at least one compare value and it contains
the lowest virtual address of the access. In addition, it is recommended
that there are additional compare values for the other accessed
virtual addresses match. (E.g. on a 32-bit read from 0x4000,
the lowest address is 0x4000 and the other addresses
are 0x4001, 0x4002, and 0x4003.)

1 (data): There is exactly one compare value and it contains
the data value loaded or stored, or the instruction executed.
Any bits beyond the size of the data access will contain 0.

Previously, when select bit was 0, Spike did not follow
the recommendation and provided only 1 matching value to the trigger module.
This change modifies the behavior to the recommended one.

The implementation follows the debug specification recommendation.

Signed-off-by: Farid Khaidari <[email protected]>

Author: fkhaidari

riscv/mmu.cc

@@ -109,7 +109,7 @@ mmu_t::insn_parcel_t mmu_t::fetch_slow_path(reg_t vaddr)
   auto access_info = generate_access_info(vaddr, FETCH, {});
 
   if (check_triggers_fetch)
-    check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt);
+    check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt, sizeof(insn_parcel_t));
 
   if (!tlb_hit) {
     paddr = translate(access_info, sizeof(insn_parcel_t));
@@ -129,7 +129,7 @@ mmu_t::insn_parcel_t mmu_t::fetch_slow_path(reg_t vaddr)
   auto res = perform_intrapage_fetch(vaddr, host_addr, paddr);
 
   if (check_triggers_fetch)
-    check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt, from_le(res));
+    check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt, sizeof(insn_parcel_t), from_le(res));
 
   return res;
 }
@@ -211,12 +211,12 @@ bool mmu_t::mmio(reg_t paddr, size_t len, uint8_t* bytes, access_type type)
   return true;
 }
 
-void mmu_t::check_triggers(triggers::operation_t operation, reg_t address, bool virt, reg_t tval, std::optional<reg_t> data)
+void mmu_t::check_triggers(triggers::operation_t operation, reg_t address, bool virt, reg_t tval, size_t len, std::optional<reg_t> data)
 {
   if (matched_trigger || !proc)
     return;
 
-  auto match = proc->TM.detect_memory_access_match(operation, address, data);
+  auto match = proc->TM.detect_memory_access_match(operation, address, len, data);
 
   if (match.has_value())
     switch (match->timing) {
@@ -289,7 +289,7 @@ void mmu_t::load_slow_path(reg_t original_addr, reg_t len, uint8_t* bytes, xlate
   reg_t transformed_addr = access_info.transformed_vaddr;
 
   if (check_triggers_load)
-    check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt);
+    check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt, len);
 
   if ((transformed_addr & (len - 1)) == 0) {
     load_slow_path_intrapage(len, bytes, access_info);
@@ -309,16 +309,21 @@ void mmu_t::load_slow_path(reg_t original_addr, reg_t len, uint8_t* bytes, xlate
     }
   }
 
+  if (!proc)
+    return;
+
   if (check_triggers_load) {
-    while (len > sizeof(reg_t)) {
-        check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt, reg_from_bytes(sizeof(reg_t), bytes));
-      len -= sizeof(reg_t);
-      bytes += sizeof(reg_t);
+    auto xlen_bytes = proc->get_xlen();
+    while (len > xlen_bytes) {
+        check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt, xlen_bytes, reg_from_bytes(xlen_bytes, bytes));
+      len -= xlen_bytes;
+      bytes += xlen_bytes;
+    transformed_addr += xlen_bytes;
     }
-    check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt, reg_from_bytes(len, bytes));
+    check_triggers(triggers::OPERATION_LOAD, transformed_addr, access_info.effective_virt, len, reg_from_bytes(len, bytes));
   }
 
-  if (proc && unlikely(proc->get_log_commits_enabled()))
+  if (unlikely(proc->get_log_commits_enabled()))
     proc->state.log_mem_read.push_back(std::make_tuple(original_addr, 0, len));
 }
 
@@ -377,15 +382,18 @@ void mmu_t::store_slow_path(reg_t original_addr, reg_t len, const uint8_t* bytes
   auto access_info = generate_access_info(original_addr, STORE, xlate_flags);
   reg_t transformed_addr = access_info.transformed_vaddr;
 
-  if (actually_store && check_triggers_store) {
+  if (actually_store && check_triggers_store && proc) {
     reg_t trig_len = len;
     const uint8_t* trig_bytes = bytes;
-    while (trig_len > sizeof(reg_t)) {
-      check_triggers(triggers::OPERATION_STORE, transformed_addr, access_info.effective_virt, reg_from_bytes(sizeof(reg_t), trig_bytes));
-      trig_len -= sizeof(reg_t);
-      trig_bytes += sizeof(reg_t);
+    auto trig_addr = transformed_addr;
+    auto xlen_bytes = proc->get_xlen();
+    while (trig_len > xlen_bytes) {
+      check_triggers(triggers::OPERATION_STORE, trig_addr, access_info.effective_virt, xlen_bytes, reg_from_bytes(sizeof(reg_t), trig_bytes));
+      trig_len -= xlen_bytes;
+      trig_bytes += xlen_bytes;
+      trig_addr += xlen_bytes;
     }
-    check_triggers(triggers::OPERATION_STORE, transformed_addr, access_info.effective_virt, reg_from_bytes(trig_len, trig_bytes));
+    check_triggers(triggers::OPERATION_STORE, trig_addr, access_info.effective_virt, trig_len, reg_from_bytes(trig_len, trig_bytes));
   }
 
   if (transformed_addr & (len - 1)) {

riscv/mmu.h

@@ -14,7 +14,6 @@
 #include "triggers.h"
 #include "cfg.h"
 #include <stdlib.h>
-#include <vector>
 
 // virtual memory configuration
 #define PGSHIFT 12
@@ -449,10 +448,10 @@ class mmu_t
   bool mmio_store(reg_t paddr, size_t len, const uint8_t* bytes);
   bool mmio(reg_t paddr, size_t len, uint8_t* bytes, access_type type);
   bool mmio_ok(reg_t paddr, access_type type);
-  void check_triggers(triggers::operation_t operation, reg_t address, bool virt, std::optional<reg_t> data = std::nullopt) {
-    check_triggers(operation, address, virt, address, data);
+  void check_triggers(triggers::operation_t operation, reg_t address, bool virt, size_t len, std::optional<reg_t> data = std::nullopt) {
+    check_triggers(operation, address, virt, address, len, data);
   }
-  void check_triggers(triggers::operation_t operation, reg_t address, bool virt, reg_t tval, std::optional<reg_t> data);
+  void check_triggers(triggers::operation_t operation, reg_t address, bool virt, reg_t tval, size_t len, std::optional<reg_t> data);
   bool check_svukte_qualified(reg_t addr, reg_t mode, bool forced_virt);
   reg_t translate(mem_access_info_t access_info, reg_t len);
 

riscv/triggers.cc

@@ -238,7 +238,7 @@ bool mcontrol_common_t::simple_match(unsigned xlen, reg_t value) const {
   assert(0);
 }
 
-std::optional<match_result_t> mcontrol_common_t::detect_memory_access_match(processor_t * const proc, operation_t operation, reg_t address, std::optional<reg_t> data) noexcept {
+std::optional<match_result_t> mcontrol_common_t::detect_memory_access_match(processor_t * const proc, operation_t operation, reg_t address, size_t len, std::optional<reg_t> data) noexcept {
   if ((operation == triggers::OPERATION_EXECUTE && !execute) ||
       (operation == triggers::OPERATION_STORE && !store) ||
       (operation == triggers::OPERATION_LOAD && !load) ||
@@ -251,23 +251,25 @@ std::optional<match_result_t> mcontrol_common_t::detect_memory_access_match(proc
     if (!data.has_value())
       return std::nullopt;
     value = *data;
+    len = 1;
   } else {
     value = address;
   }
 
   // We need this because in 32-bit mode sometimes the PC bits get sign
   // extended.
   auto xlen = proc->get_xlen();
-  if (xlen == 32) {
-    value &= 0xffffffff;
-  }
+  auto mask = make_mask64(0, xlen);
+  value &= mask;
+
+  for (std::size_t i = 0; i < len; ++i)
+    if (simple_match(xlen, (value + i) & mask)) {
+      /* This is OK because this function is only called if the trigger was not
+       * inhibited by the previous trigger in the chain. */
+      set_hit(timing ? HIT_IMMEDIATELY_AFTER : HIT_BEFORE);
+      return match_result_t(timing_t(timing), action);
+    }
 
-  if (simple_match(xlen, value)) {
-    /* This is OK because this function is only called if the trigger was not
-     * inhibited by the previous trigger in the chain. */
-    set_hit(timing ? HIT_IMMEDIATELY_AFTER : HIT_BEFORE);
-    return match_result_t(timing_t(timing), action);
-  }
   return std::nullopt;
 }
 
@@ -600,7 +602,7 @@ bool module_t::tdata3_write(unsigned index, const reg_t val) noexcept
   return true;
 }
 
-std::optional<match_result_t> module_t::detect_memory_access_match(operation_t operation, reg_t address, std::optional<reg_t> data) noexcept
+std::optional<match_result_t> module_t::detect_memory_access_match(operation_t operation, reg_t address, size_t len, std::optional<reg_t> data) noexcept
 {
   state_t * const state = proc->get_state();
   if (state->debug_mode)
@@ -621,7 +623,7 @@ std::optional<match_result_t> module_t::detect_memory_access_match(operation_t o
      * entire chain did not match. This is allowed by the spec, because the final
      * trigger in the chain will never get `hit` set unless the entire chain
      * matches. */
-    auto result = trigger->detect_memory_access_match(proc, operation, address, data);
+    auto result = trigger->detect_memory_access_match(proc, operation, address, len, data);
     if (result.has_value() && !trigger->get_chain() && (!ret.has_value() || ret->action < result->action))
       ret = result;
 

riscv/triggers.h

@@ -91,7 +91,7 @@ class trigger_t {
   virtual void stash_read_values() {}
 
   virtual std::optional<match_result_t> detect_memory_access_match(processor_t UNUSED * const proc,
-      operation_t UNUSED operation, reg_t UNUSED address, std::optional<reg_t> UNUSED data) noexcept { return std::nullopt; }
+      operation_t UNUSED operation, reg_t UNUSED address, size_t UNUSED len, std::optional<reg_t> UNUSED data) noexcept { return std::nullopt; }
   virtual std::optional<match_result_t> detect_icount_fire(processor_t UNUSED * const proc) { return std::nullopt; }
   virtual void detect_icount_decrement(processor_t UNUSED * const proc) {}
   virtual std::optional<match_result_t> detect_trap_match(processor_t UNUSED * const proc, const trap_t UNUSED & t) noexcept { return std::nullopt; }
@@ -214,7 +214,7 @@ class mcontrol_common_t : public trigger_t {
   virtual void set_hit(hit_t val) = 0;
 
   virtual std::optional<match_result_t> detect_memory_access_match(processor_t * const proc,
-      operation_t operation, reg_t address, std::optional<reg_t> data) noexcept override;
+      operation_t operation, reg_t address, size_t len, std::optional<reg_t> data) noexcept override;
 
 private:
   bool simple_match(unsigned xlen, reg_t value) const;
@@ -292,7 +292,7 @@ class module_t {
 
   unsigned count() const { return triggers.size(); }
 
-  std::optional<match_result_t> detect_memory_access_match(operation_t operation, reg_t address, std::optional<reg_t> data) noexcept;
+  std::optional<match_result_t> detect_memory_access_match(operation_t operation, reg_t address, size_t len, std::optional<reg_t> data) noexcept;
   std::optional<match_result_t> detect_icount_match() noexcept;
   std::optional<match_result_t> detect_trap_match(const trap_t& t) noexcept;