From 2d84dc46babdcbd6b6f061d3fe3fec34b999ff5c Mon Sep 17 00:00:00 2001 From: "Daniel R. Carvalho" Date: Thu, 24 Jan 2019 15:00:18 +0100 Subject: [PATCH] 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 Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/17530 Tested-by: kokoro Reviewed-by: Nikos Nikoleris Maintainer: Nikos Nikoleris --- src/mem/cache/base.cc | 8 ++------ src/mem/cache/mshr.cc | 24 ++++++++++++++++++++++ src/mem/cache/mshr.hh | 4 ++++ src/mem/cache/queue.hh | 22 +++++++++++--------- src/mem/cache/queue_entry.hh | 33 +++++++++++++++++++++++++++--- src/mem/cache/write_queue_entry.cc | 24 ++++++++++++++++++++++ src/mem/cache/write_queue_entry.hh | 4 ++++ 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,13 +119,40 @@ 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__ -- 2.30.2