mem-cache: Add match functions to QueueEntry

Having the caller decide the matching logic is error-prone, and
frequently ends up with the secure bit being forgotten. This
change adds matching functions to the QueueEntry to avoid this
problem.

As a side effect the signature of findPending has been changed.

Change-Id: I6e494a821c1e6e841ab103ec69632c0e1b269a08
Signed-off-by: Daniel R. Carvalho <odanrc@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/17530
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Maintainer: Nikos Nikoleris <nikos.nikoleris@arm.com>

7 files changed, 100 insertions, 19 deletions

diff --git a/src/mem/cache/base.cc b/src/mem/cache/base.cc
index ddcfd80a7..2a6bc2a56 100644
--- a/src/mem/cache/base.cc
+++ b/src/mem/cache/base.cc
@@ -729,9 +729,7 @@ BaseCache::getNextQueueEntry()
     // full write buffer, otherwise we favour the miss requests
     if (wq_entry && (writeBuffer.isFull() || !miss_mshr)) {
         // need to search MSHR queue for conflicting earlier miss.
-        MSHR *conflict_mshr =
-            mshrQueue.findPending(wq_entry->blkAddr,
-                                  wq_entry->isSecure);
+        MSHR *conflict_mshr = mshrQueue.findPending(wq_entry);
 
         if (conflict_mshr && conflict_mshr->order < wq_entry->order) {
             // Service misses in order until conflict is cleared.
@@ -744,9 +742,7 @@ BaseCache::getNextQueueEntry()
         return wq_entry;
     } else if (miss_mshr) {
         // need to check for conflicting earlier writeback
-        WriteQueueEntry *conflict_mshr =
-            writeBuffer.findPending(miss_mshr->blkAddr,
-                                    miss_mshr->isSecure);
+        WriteQueueEntry *conflict_mshr = writeBuffer.findPending(miss_mshr);
         if (conflict_mshr) {
             // not sure why we don't check order here... it was in the
             // original code but commented out.
diff --git a/src/mem/cache/mshr.cc b/src/mem/cache/mshr.cc
index 8db7c2997..13a50169c 100644
--- a/src/mem/cache/mshr.cc
+++ b/src/mem/cache/mshr.cc
@@ -273,6 +273,9 @@ MSHR::allocate(Addr blk_addr, unsigned blk_size, PacketPtr target,
     Target::Source source = (target->cmd == MemCmd::HardPFReq) ?
         Target::FromPrefetcher : Target::FromCPU;
     targets.add(target, when_ready, _order, source, true, alloc_on_fill);
+
+    // All targets must refer to the same block
+    assert(target->matchBlockAddr(targets.front().pkt, blkSize));
 }
 
 
@@ -685,3 +688,24 @@ MSHR::print() const
     print(str);
     return str.str();
 }
+
+bool
+MSHR::matchBlockAddr(const Addr addr, const bool is_secure) const
+{
+    assert(hasTargets());
+    return (blkAddr == addr) && (isSecure == is_secure);
+}
+
+bool
+MSHR::matchBlockAddr(const PacketPtr pkt) const
+{
+    assert(hasTargets());
+    return pkt->matchBlockAddr(blkAddr, isSecure, blkSize);
+}
+
+bool
+MSHR::conflictAddr(const QueueEntry* entry) const
+{
+    assert(hasTargets());
+    return entry->matchBlockAddr(blkAddr, isSecure);
+}
diff --git a/src/mem/cache/mshr.hh b/src/mem/cache/mshr.hh
index e9505ebdc..4b054894d 100644
--- a/src/mem/cache/mshr.hh
+++ b/src/mem/cache/mshr.hh
@@ -531,6 +531,10 @@ class MSHR : public QueueEntry, public Printable
      * @return string with mshr fields + [deferred]targets
      */
     std::string print() const;
+
+    bool matchBlockAddr(const Addr addr, const bool is_secure) const override;
+    bool matchBlockAddr(const PacketPtr pkt) const override;
+    bool conflictAddr(const QueueEntry* entry) const override;
 };
 
 #endif // __MEM_CACHE_MSHR_HH__
diff --git a/src/mem/cache/queue.hh b/src/mem/cache/queue.hh
index 6c8a19265..30fe4bad6 100644
--- a/src/mem/cache/queue.hh
+++ b/src/mem/cache/queue.hh
@@ -174,7 +174,7 @@ class Queue : public Drainable
             // cacheable accesses being added to an WriteQueueEntry
             // serving an uncacheable access
             if (!(ignore_uncacheable && entry->isUncacheable()) &&
-                entry->blkAddr == blk_addr && entry->isSecure == is_secure) {
+                entry->matchBlockAddr(blk_addr, is_secure)) {
                 return entry;
             }
         }
@@ -185,7 +185,8 @@ class Queue : public Drainable
     {
         pkt->pushLabel(label);
         for (const auto& entry : allocatedList) {
-            if (entry->blkAddr == blk_addr && entry->trySatisfyFunctional(pkt)) {
+            if (entry->matchBlockAddr(blk_addr, pkt->isSecure()) &&
+                entry->trySatisfyFunctional(pkt)) {
                 pkt->popLabel();
                 return true;
             }
@@ -195,16 +196,17 @@ class Queue : public Drainable
     }
 
     /**
-     * Find any pending requests that overlap the given request.
-     * @param blk_addr Block address.
-     * @param is_secure True if the target memory space is secure.
-     * @return A pointer to the earliest matching WriteQueueEntry.
+     * Find any pending requests that overlap the given request of a
+     * different queue.
+     *
+     * @param entry The entry to be compared against.
+     * @return A pointer to the earliest matching entry.
      */
-    Entry* findPending(Addr blk_addr, bool is_secure) const
+    Entry* findPending(const QueueEntry* entry) const
     {
-        for (const auto& entry : readyList) {
-            if (entry->blkAddr == blk_addr && entry->isSecure == is_secure) {
-                return entry;
+        for (const auto& ready_entry : readyList) {
+            if (ready_entry->conflictAddr(entry)) {
+                return ready_entry;
             }
         }
         return nullptr;
diff --git a/src/mem/cache/queue_entry.hh b/src/mem/cache/queue_entry.hh
index 39ee0a0b9..61c898dd4 100644
--- a/src/mem/cache/queue_entry.hh
+++ b/src/mem/cache/queue_entry.hh
@@ -119,14 +119,41 @@ class QueueEntry : public Packet::SenderState
     /** True if the entry targets the secure memory space. */
     bool isSecure;
 
-    QueueEntry() : readyTime(0), _isUncacheable(false),
-                   inService(false), order(0), blkAddr(0), blkSize(0),
-                   isSecure(false)
+    QueueEntry()
+        : readyTime(0), _isUncacheable(false),
+          inService(false), order(0), blkAddr(0), blkSize(0), isSecure(false)
     {}
 
     bool isUncacheable() const { return _isUncacheable; }
 
     /**
+     * Check if entry corresponds to the one being looked for.
+     *
+     * @param addr Address to match against.
+     * @param is_secure Whether the target should be in secure space or not.
+     * @return True if entry matches given information.
+     */
+    virtual bool matchBlockAddr(const Addr addr, const bool is_secure)
+                                                            const = 0;
+
+    /**
+     * Check if entry contains a packet that corresponds to the one being
+     * looked for.
+     *
+     * @param pkt The packet to search for.
+     * @return True if any of its targets' packets matches the given one.
+     */
+    virtual bool matchBlockAddr(const PacketPtr pkt) const = 0;
+
+    /**
+     * Check if given entry's packets conflict with this' entries packets.
+     *
+     * @param entry Other entry to compare against.
+     * @return True if entry matches given information.
+     */
+    virtual bool conflictAddr(const QueueEntry* entry) const = 0;
+
+    /**
      * Send this queue entry as a downstream packet, with the exact
      * behaviour depending on the specific entry type.
      */
diff --git a/src/mem/cache/write_queue_entry.cc b/src/mem/cache/write_queue_entry.cc
index 6c8387bcb..f3106e4bc 100644
--- a/src/mem/cache/write_queue_entry.cc
+++ b/src/mem/cache/write_queue_entry.cc
@@ -112,6 +112,9 @@ WriteQueueEntry::allocate(Addr blk_addr, unsigned blk_size, PacketPtr target,
              "a cacheable eviction or a writeclean");
 
     targets.add(target, when_ready, _order);
+
+    // All targets must refer to the same block
+    assert(target->matchBlockAddr(targets.front().pkt, blkSize));
 }
 
 void
@@ -141,6 +144,27 @@ WriteQueueEntry::sendPacket(BaseCache &cache)
     return cache.sendWriteQueuePacket(this);
 }
 
+bool
+WriteQueueEntry::matchBlockAddr(const Addr addr, const bool is_secure) const
+{
+    assert(hasTargets());
+    return (blkAddr == addr) && (isSecure == is_secure);
+}
+
+bool
+WriteQueueEntry::matchBlockAddr(const PacketPtr pkt) const
+{
+    assert(hasTargets());
+    return pkt->matchBlockAddr(blkAddr, isSecure, blkSize);
+}
+
+bool
+WriteQueueEntry::conflictAddr(const QueueEntry* entry) const
+{
+    assert(hasTargets());
+    return entry->matchBlockAddr(blkAddr, isSecure);
+}
+
 void
 WriteQueueEntry::print(std::ostream &os, int verbosity,
                        const std::string &prefix) const
diff --git a/src/mem/cache/write_queue_entry.hh b/src/mem/cache/write_queue_entry.hh
index 59714d948..9aaac493c 100644
--- a/src/mem/cache/write_queue_entry.hh
+++ b/src/mem/cache/write_queue_entry.hh
@@ -179,6 +179,10 @@ class WriteQueueEntry : public QueueEntry, public Printable
      * @return string with mshr fields
      */
     std::string print() const;
+
+    bool matchBlockAddr(const Addr addr, const bool is_secure) const override;
+    bool matchBlockAddr(const PacketPtr pkt) const override;
+    bool conflictAddr(const QueueEntry* entry) const override;
 };
 
 #endif // __MEM_CACHE_WRITE_QUEUE_ENTRY_HH__