commit d54b8f8475495213f5c2e69aadd7886c69bcd53c
author Adrià Armejach <adria.armejach@bsc.es> Wed Jun 07 17:01:03 2023 +0200
committer Bobby Bruce <bbruce@ucdavis.edu> Fri Jun 16 16:44:49 2023 +0000
tree c6135a407d0cc01b84944e368c083b7b51335347
parent b182b15f93621206c87c6c760cdfc1f5df1877cf

arch-riscv: fix load reserved store conditional

  * According to the manual, load reservations must be cleared on a
    failed or a successful SC attempt.
  * A load reservation can be arbitrarily large. The current
    implementation was reserving something different than cacheBlockSize
    which could lead to problems if snoop addresses are cache block
    aligned. This patch implementation assumes a cacheBlock granularity.
  * Load reservations should also be cleared on faults

Change-Id: I64513534710b5f269260fcb204f717801913e2f5
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/71520
Maintainer: Bobby Bruce <bbruce@ucdavis.edu>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Bobby Bruce <bbruce@ucdavis.edu>

src/arch/generic/isa.hh

diff --git a/src/arch/generic/isa.hh b/src/arch/generic/isa.hh
index e9e4d95..58f66fc 100644
--- a/src/arch/generic/isa.hh
+++ b/src/arch/generic/isa.hh
@@ -70,6 +70,7 @@
   public:
     virtual PCStateBase *newPCState(Addr new_inst_addr=0) const = 0;
     virtual void clear() {}
+    virtual void clearLoadReservation(ContextID cid) {}
 
     virtual RegVal readMiscRegNoEffect(RegIndex idx) const = 0;
     virtual RegVal readMiscReg(RegIndex idx) = 0;

src/arch/riscv/faults.cc

diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index 940f710..8fb8f81 100644
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -153,6 +153,9 @@
             tc->setMiscReg(MISCREG_NMIE, 0);
         }
 
+        // Clear load reservation address
+        tc->getIsaPtr()->clearLoadReservation(tc->contextId());
+
         // Set PC to fault handler address
         Addr addr = mbits(tc->readMiscReg(tvec), 63, 2);
         if (isInterrupt() && bits(tc->readMiscReg(tvec), 1, 0) == 1)

src/arch/riscv/isa.cc

diff --git a/src/arch/riscv/isa.cc b/src/arch/riscv/isa.cc
index d744fe36..94a8239 100644
--- a/src/arch/riscv/isa.cc
+++ b/src/arch/riscv/isa.cc
@@ -672,11 +672,6 @@
     UNSERIALIZE_CONTAINER(miscRegFile);
 }
 
-const int WARN_FAILURE = 10000;
-
-const Addr INVALID_RESERVATION_ADDR = (Addr) -1;
-std::unordered_map<int, Addr> load_reservation_addrs;
-
 void
 ISA::handleLockedSnoop(PacketPtr pkt, Addr cacheBlockMask)
 {
@@ -696,9 +691,9 @@
 {
     Addr& load_reservation_addr = load_reservation_addrs[tc->contextId()];
 
-    load_reservation_addr = req->getPaddr() & ~0xF;
+    load_reservation_addr = req->getPaddr();
     DPRINTF(LLSC, "[cid:%d]: Reserved address %x.\n",
-            req->contextId(), req->getPaddr() & ~0xF);
+            req->contextId(), req->getPaddr());
 }
 
 bool
@@ -717,12 +712,13 @@
             lr_addr_empty ? "yes" : "no");
     if (!lr_addr_empty) {
         DPRINTF(LLSC, "[cid:%d]: addr = %x.\n", req->contextId(),
-                req->getPaddr() & ~0xF);
+                req->getPaddr() & cacheBlockMask);
         DPRINTF(LLSC, "[cid:%d]: last locked addr = %x.\n", req->contextId(),
-                load_reservation_addr);
+                load_reservation_addr & cacheBlockMask);
     }
-    if (lr_addr_empty
-            || load_reservation_addr != ((req->getPaddr() & ~0xF))) {
+    if (lr_addr_empty ||
+            (load_reservation_addr & cacheBlockMask)
+            != ((req->getPaddr() & cacheBlockMask))) {
         req->setExtraData(0);
         int stCondFailures = tc->readStCondFailures();
         tc->setStCondFailures(++stCondFailures);
@@ -730,12 +726,21 @@
             warn("%i: context %d: %d consecutive SC failures.\n",
                     curTick(), tc->contextId(), stCondFailures);
         }
+
+        // Must clear any reservations
+        load_reservation_addr = INVALID_RESERVATION_ADDR;
+
         return false;
     }
     if (req->isUncacheable()) {
         req->setExtraData(2);
     }
 
+    // Must clear any reservations
+    load_reservation_addr = INVALID_RESERVATION_ADDR;
+
+    DPRINTF(LLSC, "[cid:%d]: SC success! Current locked addr = %x.\n",
+            req->contextId(), load_reservation_addr & cacheBlockMask);
     return true;
 }
 
@@ -743,6 +748,8 @@
 ISA::globalClearExclusive()
 {
     tc->getCpuPtr()->wakeup(tc->threadId());
+    Addr& load_reservation_addr = load_reservation_addrs[tc->contextId()];
+    load_reservation_addr = INVALID_RESERVATION_ADDR;
 }
 
 void

src/arch/riscv/isa.hh

diff --git a/src/arch/riscv/isa.hh b/src/arch/riscv/isa.hh
index 5a2a610..7ef5c52 100644
--- a/src/arch/riscv/isa.hh
+++ b/src/arch/riscv/isa.hh
@@ -76,6 +76,11 @@
 
     bool hpmCounterEnabled(int counter) const;
 
+    // Load reserve - store conditional monitor
+    const int WARN_FAILURE = 10000;
+    const Addr INVALID_RESERVATION_ADDR = (Addr)-1;
+    std::unordered_map<int, Addr> load_reservation_addrs;
+
   public:
     using Params = RiscvISAParams;
 
@@ -87,6 +92,13 @@
         return new PCState(new_inst_addr, rv_type);
     }
 
+    void
+    clearLoadReservation(ContextID cid) override
+    {
+        Addr& load_reservation_addr = load_reservation_addrs[cid];
+        load_reservation_addr = INVALID_RESERVATION_ADDR;
+    }
+
   public:
     RegVal readMiscRegNoEffect(RegIndex idx) const override;
     RegVal readMiscReg(RegIndex idx) override;