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 <odanrc@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/34955
Reviewed-by: Jason Lowe-Power <power.jg@gmail.com>
Maintainer: Jason Lowe-Power <power.jg@gmail.com>
Tested-by: kokoro <noreply+kokoro@google.com>

src/mem/cache/cache_blk.cc

@@ -11,6 +11,7 @@
  * unmodified and in its entirety in all distributions of the software,
  * modified or unmodified, in source code or in binary form.
  *
+ * Copyright (c) 2020 Inria
  * Copyright (c) 2007 The Regents of The University of Michigan
  * All rights reserved.
  *
@@ -42,6 +43,12 @@
 
 #include "base/cprintf.hh"
 
+bool
+CacheBlk::matchTag(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 @@ CacheBlk::insert(const Addr tag, const bool is_secure,
     assert(status == 0);
 
     // Set block tag
-    this->tag = tag;
+    setTag(tag);
 
     // Set source requestor ID
     srcRequestorId = src_requestor_ID;

src/mem/cache/cache_blk.hh

@@ -11,6 +11,7 @@
  * unmodified and in its entirety in all distributions of the software,
  * modified or unmodified, in source code or in binary form.
  *
+ * Copyright (c) 2020 Inria
  * Copyright (c) 2003-2005 The Regents of The University of Michigan
  * All rights reserved.
  *
@@ -87,8 +88,6 @@ class CacheBlk : public ReplaceableEntry
     /** 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 +209,7 @@ class CacheBlk : public ReplaceableEntry
      */
     virtual void invalidate()
     {
-        tag = MaxAddr;
+        setTag(MaxAddr);
         task_id = ContextSwitchTaskId::Unknown;
         status = 0;
         whenReady = MaxTick;
@@ -238,6 +237,20 @@ class CacheBlk : public ReplaceableEntry
         return (status & BlkHWPrefetched) != 0;
     }
 
+    /**
+     * 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.
@@ -288,6 +301,14 @@ class CacheBlk : public ReplaceableEntry
         whenReady = tick;
     }
 
+    /**
+     * 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 matchTag(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
@@ -380,9 +401,8 @@ class CacheBlk : public ReplaceableEntry
           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());
     }
 
     /**
@@ -431,6 +451,10 @@ class CacheBlk : public ReplaceableEntry
             return true;
         }
     }
+
+  private:
+    /** Data block tag value. */
+    Addr _tag;
 };
 
 /**

src/mem/cache/tags/base.cc

@@ -86,8 +86,7 @@ BaseTags::findBlock(Addr addr, bool is_secure) const
     // 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->matchTag(tag, is_secure)) {
             return blk;
         }
     }

src/mem/cache/tags/base_set_assoc.hh

@@ -226,7 +226,7 @@ class BaseSetAssoc : public BaseTags
      */
     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 {

src/mem/cache/tags/compressed_tags.cc

@@ -115,8 +115,7 @@ CompressedTags::findVictim(Addr addr, const bool is_secure,
     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->matchTag(tag, is_secure) &&
             !superblock->blks[offset]->isValid() &&
             superblock->isCompressed() &&
             superblock->canCoAllocate(compressed_size))

src/mem/cache/tags/fa_lru.cc

@@ -118,7 +118,7 @@ FALRU::invalidate(CacheBlk *blk)
 {
     // Erase block entry reference in the hash table
     M5_VAR_USED auto num_erased =
-        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 @@ FALRU::findBlock(Addr addr, bool is_secure) const
     }
 
     if (blk && blk->isValid()) {
-        assert(blk->tag == tag);
+        assert(blk->getTag() == tag);
         assert(blk->isSecure() == is_secure);
     }
 
@@ -222,7 +222,7 @@ FALRU::insertBlock(const PacketPtr pkt, CacheBlk *blk)
     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

src/mem/cache/tags/fa_lru.hh

@@ -251,7 +251,7 @@ class FALRU : public BaseTags
      */
     Addr regenerateBlkAddr(const CacheBlk* blk) const override
     {
-        return blk->tag;
+        return blk->getTag();
     }
 
     void forEachBlk(std::function<void(CacheBlk &)> visitor) override {

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 @@ SectorSubBlk::getSectorOffset() const
 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 @@ SectorSubBlk::insert(const Addr tag, const bool is_secure,
                      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->matchTag(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 @@ SectorBlk::invalidateSubBlk()
     // so clear secure bit
     if (--_validCounter == 0) {
         _secureBit = false;
+        setTag(MaxAddr);
     }
 }
 
@@ -180,3 +190,9 @@ SectorBlk::setPosition(const uint32_t set, const uint32_t way)
         blk->setPosition(set, way);
     }
 }
+
+bool
+SectorBlk::matchTag(Addr tag, bool is_secure) const
+{
+    return isValid() && (getTag() == tag) && (isSecure() == is_secure);
+}

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 @@ class SectorSubBlk : public CacheBlk
      */
     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 @@ class SectorBlk : public ReplaceableEntry
      * @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 matchTag(Addr tag, bool is_secure) const;
 };
 
 #endif //__MEM_CACHE_TAGS_SECTOR_BLK_HH__

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 @@ SectorTags::findBlock(Addr addr, bool is_secure) const
     // 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->matchTag(tag, is_secure)) {
             return blk;
         }
     }
@@ -232,8 +231,7 @@ SectorTags::findVictim(Addr addr, const bool is_secure, const std::size_t size,
     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->matchTag(tag, is_secure)) {
             victim_sector = sector_blk;
             break;
         }
@@ -251,8 +249,7 @@ SectorTags::findVictim(Addr addr, const bool is_secure, const std::size_t size,
 
     // 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->matchTag(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 @@ SectorTags::regenerateBlkAddr(const CacheBlk* blk) const
 {
     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);
 }