Daniel Carvalho has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/34955 )
Change subject: mem-cache: Protect tag from being mishandled
......................................................................
mem-cache: Protect tag from being mishandled
Make the tag variable private, so that every access to it must pass
through a getter and a setter. This protects it from being incorrectly
updated when there is no direct match between a tag and a data entry,
as is the case of sector, compressed, decoupled, and many other table
layouts.
As a side effect, a block matching function has been created, which
also depends directly on the mapping between tag and data entries.
Change-Id: I848a154404feb5cbcea8d0fd2509bf49e1d73bd0
Signed-off-by: Daniel R. Carvalho <oda...@yahoo.com.br>
---
M src/mem/cache/cache_blk.cc
M src/mem/cache/cache_blk.hh
M src/mem/cache/tags/base.cc
M src/mem/cache/tags/base_set_assoc.hh
M src/mem/cache/tags/compressed_tags.cc
M src/mem/cache/tags/fa_lru.cc
M src/mem/cache/tags/fa_lru.hh
M src/mem/cache/tags/sector_blk.cc
M src/mem/cache/tags/sector_blk.hh
M src/mem/cache/tags/sector_tags.cc
10 files changed, 87 insertions(+), 40 deletions(-)
diff --git a/src/mem/cache/cache_blk.cc b/src/mem/cache/cache_blk.cc
index 4d7e408..0c44210 100644
--- a/src/mem/cache/cache_blk.cc
+++ b/src/mem/cache/cache_blk.cc
@@ -1,4 +1,5 @@
/*
+ * Copyright (c) 2020 Inria
* Copyright (c) 2012-2013 ARM Limited
* All rights reserved
*
@@ -42,6 +43,12 @@
#include "base/cprintf.hh"
+bool
+CacheBlk::matchBlock(Addr tag, bool is_secure) const
+{
+ return isValid() && (getTag() == tag) && (isSecure() == is_secure);
+}
+
void
CacheBlk::insert(const Addr tag, const bool is_secure,
const int src_requestor_ID, const uint32_t task_ID)
@@ -50,7 +57,7 @@
assert(status == 0);
// Set block tag
- this->tag = tag;
+ setTag(tag);
// Set source requestor ID
srcRequestorId = src_requestor_ID;
diff --git a/src/mem/cache/cache_blk.hh b/src/mem/cache/cache_blk.hh
index 99f8545..19a5e58 100644
--- a/src/mem/cache/cache_blk.hh
+++ b/src/mem/cache/cache_blk.hh
@@ -1,4 +1,5 @@
/*
+ * Copyright (c) 2020 Inria
* Copyright (c) 2012-2018 ARM Limited
* All rights reserved.
*
@@ -83,12 +84,14 @@
*/
class CacheBlk : public ReplaceableEntry
{
+ private:
+ /** Data block tag value. */
+ Addr _tag;
+
public:
/** Task Id associated with this block */
uint32_t task_id;
- /** Data block tag value. */
- Addr tag;
/**
* Contains a copy of the data in this block for easy access. This is
used
* for efficient execution when the data could be actually stored in
@@ -210,7 +213,7 @@
*/
virtual void invalidate()
{
- tag = MaxAddr;
+ setTag(MaxAddr);
task_id = ContextSwitchTaskId::Unknown;
status = 0;
whenReady = MaxTick;
@@ -239,6 +242,20 @@
}
/**
+ * Get tag associated to this block.
+ *
+ * @return The tag value.
+ */
+ virtual Addr getTag() const { return _tag; }
+
+ /**
+ * Set tag associated to this block.
+ *
+ * @param The tag value.
+ */
+ virtual void setTag(Addr tag) { _tag = tag; }
+
+ /**
* Check if this block holds data from the secure memory space.
* @return True if the block holds data from the secure memory space.
*/
@@ -289,6 +306,14 @@
}
/**
+ * Checks if the given information corresponds to this block's.
+ *
+ * @param tag The tag value to compare to.
+ * @param is_secure Whether secure bit is set.
+ */
+ virtual bool matchBlock(Addr tag, bool is_secure) const;
+
+ /**
* Set member variables when a block insertion occurs. Resets reference
* count to 1 (the insertion counts as a reference), and touch block if
* it hadn't been touched previously. Sets the insertion tick to the
@@ -380,9 +405,8 @@
default: s = 'T'; break; // @TODO add other types
}
return csprintf("state: %x (%c) valid: %d writable: %d
readable: %d "
- "dirty: %d | tag: %#x %s", status, s,
- isValid(), isWritable(), isReadable(), isDirty(),
tag,
- ReplaceableEntry::print());
+ "dirty: %d | tag: %#x %s", status, s, isValid(), isWritable(),
+ isReadable(), isDirty(), getTag(), ReplaceableEntry::print());
}
/**
diff --git a/src/mem/cache/tags/base.cc b/src/mem/cache/tags/base.cc
index 32c6d29..28b5fcb 100644
--- a/src/mem/cache/tags/base.cc
+++ b/src/mem/cache/tags/base.cc
@@ -86,8 +86,7 @@
// Search for block
for (const auto& location : entries) {
CacheBlk* blk = static_cast<CacheBlk*>(location);
- if ((blk->tag == tag) && blk->isValid() &&
- (blk->isSecure() == is_secure)) {
+ if (blk->matchBlock(tag, is_secure)) {
return blk;
}
}
diff --git a/src/mem/cache/tags/base_set_assoc.hh
b/src/mem/cache/tags/base_set_assoc.hh
index d743df7..d273f1e 100644
--- a/src/mem/cache/tags/base_set_assoc.hh
+++ b/src/mem/cache/tags/base_set_assoc.hh
@@ -226,7 +226,7 @@
*/
Addr regenerateBlkAddr(const CacheBlk* blk) const override
{
- return indexingPolicy->regenerateAddr(blk->tag, blk);
+ return indexingPolicy->regenerateAddr(blk->getTag(), blk);
}
void forEachBlk(std::function<void(CacheBlk &)> visitor) override {
diff --git a/src/mem/cache/tags/compressed_tags.cc
b/src/mem/cache/tags/compressed_tags.cc
index 126f664..1bee93e 100644
--- a/src/mem/cache/tags/compressed_tags.cc
+++ b/src/mem/cache/tags/compressed_tags.cc
@@ -115,8 +115,7 @@
const uint64_t offset = extractSectorOffset(addr);
for (const auto& entry : superblock_entries){
SuperBlk* superblock = static_cast<SuperBlk*>(entry);
- if ((tag == superblock->getTag()) && superblock->isValid() &&
- (is_secure == superblock->isSecure()) &&
+ if (superblock->matchBlock(tag, is_secure) &&
!superblock->blks[offset]->isValid() &&
superblock->isCompressed() &&
superblock->canCoAllocate(compressed_size))
diff --git a/src/mem/cache/tags/fa_lru.cc b/src/mem/cache/tags/fa_lru.cc
index 9574ec9..3b1384a 100644
--- a/src/mem/cache/tags/fa_lru.cc
+++ b/src/mem/cache/tags/fa_lru.cc
@@ -118,7 +118,7 @@
{
// Erase block entry reference in the hash table
auto num_erased M5_VAR_USED =
- tagHash.erase(std::make_pair(blk->tag, blk->isSecure()));
+ tagHash.erase(std::make_pair(blk->getTag(), blk->isSecure()));
// Sanity check; only one block reference should be erased
assert(num_erased == 1);
@@ -177,7 +177,7 @@
}
if (blk && blk->isValid()) {
- assert(blk->tag == tag);
+ assert(blk->getTag() == tag);
assert(blk->isSecure() == is_secure);
}
@@ -222,7 +222,7 @@
moveToHead(falruBlk);
// Insert new block in the hash table
- tagHash[std::make_pair(blk->tag, blk->isSecure())] = falruBlk;
+ tagHash[std::make_pair(blk->getTag(), blk->isSecure())] = falruBlk;
}
void
diff --git a/src/mem/cache/tags/fa_lru.hh b/src/mem/cache/tags/fa_lru.hh
index b7d7a73..5feb518 100644
--- a/src/mem/cache/tags/fa_lru.hh
+++ b/src/mem/cache/tags/fa_lru.hh
@@ -251,7 +251,7 @@
*/
Addr regenerateBlkAddr(const CacheBlk* blk) const override
{
- return blk->tag;
+ return blk->getTag();
}
void forEachBlk(std::function<void(CacheBlk &)> visitor) override {
diff --git a/src/mem/cache/tags/sector_blk.cc
b/src/mem/cache/tags/sector_blk.cc
index e914cef..08fe211 100644
--- a/src/mem/cache/tags/sector_blk.cc
+++ b/src/mem/cache/tags/sector_blk.cc
@@ -1,5 +1,5 @@
/**
- * Copyright (c) 2018 Inria
+ * Copyright (c) 2018, 2020 Inria
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -66,7 +66,21 @@
Addr
SectorSubBlk::getTag() const
{
- return _sectorBlk->getTag();
+ // If the sub-block is valid its tag must match its sector's
+ const Addr tag = _sectorBlk->getTag();
+ assert(!isValid() || (CacheBlk::getTag() == tag));
+ return tag;
+}
+
+void
+SectorSubBlk::setTag(Addr tag)
+{
+ CacheBlk::setTag(tag);
+
+ // The sector block handles its own tag's invalidation
+ if (tag != MaxAddr) {
+ _sectorBlk->setTag(tag);
+ }
}
void
@@ -95,15 +109,10 @@
const int src_requestor_ID, const uint32_t task_ID)
{
// Make sure it is not overwriting another sector
- panic_if((_sectorBlk && _sectorBlk->isValid()) &&
- ((_sectorBlk->getTag() != tag) ||
- (_sectorBlk->isSecure() != is_secure)),
- "Overwriting valid sector!");
+ panic_if(_sectorBlk && _sectorBlk->isValid() &&
+ !_sectorBlk->matchBlock(tag, is_secure), "Overwriting valid
sector!");
CacheBlk::insert(tag, is_secure, src_requestor_ID, task_ID);
-
- // Set sector tag
- _sectorBlk->setTag(tag);
}
std::string
@@ -163,6 +172,7 @@
// so clear secure bit
if (--_validCounter == 0) {
_secureBit = false;
+ setTag(MaxAddr);
}
}
@@ -180,3 +190,9 @@
blk->setPosition(set, way);
}
}
+
+bool
+SectorBlk::matchBlock(Addr tag, bool is_secure) const
+{
+ return isValid() && (getTag() == tag) && (isSecure() == is_secure);
+}
diff --git a/src/mem/cache/tags/sector_blk.hh
b/src/mem/cache/tags/sector_blk.hh
index 5538aa1..3e77c9a 100644
--- a/src/mem/cache/tags/sector_blk.hh
+++ b/src/mem/cache/tags/sector_blk.hh
@@ -1,5 +1,5 @@
/**
- * Copyright (c) 2018 Inria
+ * Copyright (c) 2018, 2020 Inria
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -92,12 +92,8 @@
*/
int getSectorOffset() const;
- /**
- * Get tag associated to this block.
- *
- * @return The tag value.
- */
- Addr getTag() const;
+ Addr getTag() const override;
+ void setTag(Addr tag) override;
/**
* Set valid bit and inform sector block.
@@ -228,6 +224,14 @@
* @param way The way of this entry and sub-entries.
*/
void setPosition(const uint32_t set, const uint32_t way) override;
+
+ /**
+ * Checks if the given information corresponds to this block's.
+ *
+ * @param tag The tag value to compare to.
+ * @param is_secure Whether secure bit is set.
+ */
+ virtual bool matchBlock(Addr tag, bool is_secure) const;
};
#endif //__MEM_CACHE_TAGS_SECTOR_BLK_HH__
diff --git a/src/mem/cache/tags/sector_tags.cc
b/src/mem/cache/tags/sector_tags.cc
index bf40664..8a01740 100644
--- a/src/mem/cache/tags/sector_tags.cc
+++ b/src/mem/cache/tags/sector_tags.cc
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2018 Inria
+ * Copyright (c) 2018, 2020 Inria
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -209,8 +209,7 @@
// Search for block
for (const auto& sector : entries) {
auto blk = static_cast<SectorBlk*>(sector)->blks[offset];
- if (blk->getTag() == tag && blk->isValid() &&
- blk->isSecure() == is_secure) {
+ if (blk->matchBlock(tag, is_secure)) {
return blk;
}
}
@@ -232,8 +231,7 @@
SectorBlk* victim_sector = nullptr;
for (const auto& sector : sector_entries) {
SectorBlk* sector_blk = static_cast<SectorBlk*>(sector);
- if ((tag == sector_blk->getTag()) && sector_blk->isValid() &&
- (is_secure == sector_blk->isSecure())){
+ if (sector_blk->matchBlock(tag, is_secure)) {
victim_sector = sector_blk;
break;
}
@@ -251,8 +249,7 @@
// Get evicted blocks. Blocks are only evicted if the sectors mismatch
and
// the currently existing sector is valid.
- if ((tag == victim_sector->getTag()) &&
- (is_secure == victim_sector->isSecure())){
+ if (victim_sector->matchBlock(tag, is_secure)) {
// It would be a hit if victim was valid, and upgrades do not call
// findVictim, so it cannot happen
assert(!victim->isValid());
@@ -282,7 +279,8 @@
{
const SectorSubBlk* blk_cast = static_cast<const SectorSubBlk*>(blk);
const SectorBlk* sec_blk = blk_cast->getSectorBlock();
- const Addr sec_addr = indexingPolicy->regenerateAddr(blk->tag,
sec_blk);
+ const Addr sec_addr =
+ indexingPolicy->regenerateAddr(blk->getTag(), sec_blk);
return sec_addr | ((Addr)blk_cast->getSectorOffset() << sectorShift);
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/34955
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I848a154404feb5cbcea8d0fd2509bf49e1d73bd0
Gerrit-Change-Number: 34955
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Carvalho <oda...@yahoo.com.br>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
