From cbf530c33809803ac3eabc56176c521047a0882d Mon Sep 17 00:00:00 2001 From: "Daniel R. Carvalho" Date: Thu, 20 Feb 2020 16:23:16 +0100 Subject: [PATCH] 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 Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/34955 Reviewed-by: Jason Lowe-Power Maintainer: Jason Lowe-Power Tested-by: kokoro --- src/mem/cache/cache_blk.cc | 9 ++++++- src/mem/cache/cache_blk.hh | 36 ++++++++++++++++++++++----- src/mem/cache/tags/base.cc | 3 +-- src/mem/cache/tags/base_set_assoc.hh | 2 +- src/mem/cache/tags/compressed_tags.cc | 3 +-- src/mem/cache/tags/fa_lru.cc | 6 ++--- src/mem/cache/tags/fa_lru.hh | 2 +- src/mem/cache/tags/sector_blk.cc | 34 ++++++++++++++++++------- src/mem/cache/tags/sector_blk.hh | 18 ++++++++------ src/mem/cache/tags/sector_tags.cc | 14 +++++------ 10 files changed, 87 insertions(+), 40 deletions(-) diff --git a/src/mem/cache/cache_blk.cc b/src/mem/cache/cache_blk.cc index 4d7e4086b..4b3325ca6 100644 --- a/src/mem/cache/cache_blk.cc +++ b/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; diff --git a/src/mem/cache/cache_blk.hh b/src/mem/cache/cache_blk.hh index 99f854594..1de5a818a 100644 --- a/src/mem/cache/cache_blk.hh +++ b/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; }; /** diff --git a/src/mem/cache/tags/base.cc b/src/mem/cache/tags/base.cc index 32c6d2951..985a1eb4a 100644 --- a/src/mem/cache/tags/base.cc +++ b/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(location); - if ((blk->tag == tag) && blk->isValid() && - (blk->isSecure() == is_secure)) { + if (blk->matchTag(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 d743df758..d273f1e38 100644 --- a/src/mem/cache/tags/base_set_assoc.hh +++ b/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 visitor) override { diff --git a/src/mem/cache/tags/compressed_tags.cc b/src/mem/cache/tags/compressed_tags.cc index 126f664e2..f86d1a5db 100644 --- a/src/mem/cache/tags/compressed_tags.cc +++ b/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(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)) diff --git a/src/mem/cache/tags/fa_lru.cc b/src/mem/cache/tags/fa_lru.cc index 962c9fde7..82f951970 100644 --- a/src/mem/cache/tags/fa_lru.cc +++ b/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 diff --git a/src/mem/cache/tags/fa_lru.hh b/src/mem/cache/tags/fa_lru.hh index b7d7a737a..5feb518f4 100644 --- a/src/mem/cache/tags/fa_lru.hh +++ b/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 visitor) override { diff --git a/src/mem/cache/tags/sector_blk.cc b/src/mem/cache/tags/sector_blk.cc index e914cef1a..81c9de2a3 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 @@ 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); +} diff --git a/src/mem/cache/tags/sector_blk.hh b/src/mem/cache/tags/sector_blk.hh index 5538aa117..c5c4fc8ca 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 @@ 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__ diff --git a/src/mem/cache/tags/sector_tags.cc b/src/mem/cache/tags/sector_tags.cc index bf4066458..e43237ea2 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 @@ SectorTags::findBlock(Addr addr, bool is_secure) const // Search for block for (const auto& sector : entries) { auto blk = static_cast(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(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(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); } -- 2.30.2