Discussion:
[gem5-dev] Change in gem5/gem5[master]: mem-cache: Add setters to validate and secure block
(too old to reply)
Daniel Carvalho (Gerrit)
2018-11-15 15:45:35 UTC
Permalink
Daniel Carvalho has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/14362


Change subject: mem-cache: Add setters to validate and secure block
......................................................................

mem-cache: Add setters to validate and secure block

In order to allow polymorphism of the block these two
functions have been added, and all direct status
assignments to these bits have been substituted.

We also assert that the block has been invalidated
before insertion. Then the block is validated in
the insertion.

Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Signed-off-by: Daniel R. Carvalho <***@yahoo.com.br>
---
M src/mem/cache/base.cc
M src/mem/cache/cache_blk.cc
M src/mem/cache/cache_blk.hh
M src/mem/cache/tags/sector_tags.cc
4 files changed, 42 insertions(+), 22 deletions(-)



diff --git a/src/mem/cache/base.cc b/src/mem/cache/base.cc
index 497f75c..8afc344 100644
--- a/src/mem/cache/base.cc
+++ b/src/mem/cache/base.cc
@@ -1004,7 +1004,7 @@
return false;
}

- blk->status |= (BlkValid | BlkReadable);
+ blk->status |= BlkReadable;
}
// only mark the block dirty if we got a writeback command,
// and leave it as is for a clean writeback
@@ -1062,7 +1062,7 @@
return false;
}

- blk->status |= (BlkValid | BlkReadable);
+ blk->status |= BlkReadable;
}
}

@@ -1149,26 +1149,22 @@
// No replaceable block or a mostly exclusive
// cache... just use temporary storage to complete the
// current request and then get rid of it
- assert(!tempBlock->isValid());
blk = tempBlock;
tempBlock->insert(addr, is_secure);
DPRINTF(Cache, "using temp block for %#llx (%s)\n", addr,
is_secure ? "s" : "ns");
}
-
- // we should never be overwriting a valid block
- assert(!blk->isValid());
} else {
// existing block... probably an upgrade
- assert(regenerateBlkAddr(blk) == addr);
- assert(blk->isSecure() == is_secure);
- // either we're getting new data or the block should already be
valid
- assert(pkt->hasData() || blk->isValid());
// don't clear block status... if block is already dirty we
// don't want to lose that
}

- blk->status |= BlkValid | BlkReadable;
+ // Block is guaranteed to be valid at this point
+ assert(blk->isValid() && (blk->isSecure() == is_secure) &&
+ (regenerateBlkAddr(blk) == addr));
+
+ blk->status |= BlkReadable;

// sanity check for whole-line writes, which should always be
// marked as writable as part of the fill, and then later marked
diff --git a/src/mem/cache/cache_blk.cc b/src/mem/cache/cache_blk.cc
index a77fc63..59ef11f 100644
--- a/src/mem/cache/cache_blk.cc
+++ b/src/mem/cache/cache_blk.cc
@@ -46,6 +46,9 @@
CacheBlk::insert(const Addr tag, const bool is_secure,
const int src_master_ID, const uint32_t task_ID)
{
+ // Make sure we are not modifying a valid block
+ assert(!isValid());
+
// Set block tag
this->tag = tag;

@@ -63,10 +66,11 @@

// Set secure state
if (is_secure) {
- status = BlkSecure;
- } else {
- status = 0;
+ setSecure();
}
+
+ // Validate block
+ setValid();
}

void
diff --git a/src/mem/cache/cache_blk.hh b/src/mem/cache/cache_blk.hh
index 80983a6..4c9cd1f 100644
--- a/src/mem/cache/cache_blk.hh
+++ b/src/mem/cache/cache_blk.hh
@@ -244,10 +244,27 @@
}

/**
+ * Set valid bit.
+ */
+ virtual void setValid()
+ {
+ assert(!isValid());
+ status |= BlkValid;
+ }
+
+ /**
+ * Set secure bit.
+ */
+ virtual void setSecure()
+ {
+ status |= BlkSecure;
+ }
+
+ /**
* 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
- * current tick. Does not make block valid.
+ * current tick. Validates the block.
*
* @param tag Block address tag.
* @param is_secure Whether the block is in secure space or not.
@@ -425,15 +442,19 @@
void insert(const Addr addr, const bool is_secure,
const int src_master_ID=0, const uint32_t task_ID=0)
override
{
+ // Make sure we are not modifying a valid block
+ assert(!isValid());
+
// Set block address
_addr = addr;

// Set secure state
if (is_secure) {
- status = BlkSecure;
- } else {
- status = 0;
+ setSecure();
}
+
+ // Validate block
+ setValid();
}

/**
diff --git a/src/mem/cache/tags/sector_tags.cc
b/src/mem/cache/tags/sector_tags.cc
index 68440c2..ad374e5 100644
--- a/src/mem/cache/tags/sector_tags.cc
+++ b/src/mem/cache/tags/sector_tags.cc
@@ -171,16 +171,12 @@
const int src_master_ID, const uint32_t task_ID,
CacheBlk *blk)
{
- // Do common block insertion functionality
- BaseTags::insertBlock(addr, is_secure, src_master_ID, task_ID, blk);
-
// Get block's sector
SectorSubBlk* sub_blk = static_cast<SectorSubBlk*>(blk);
const SectorBlk* sector_blk = sub_blk->getSectorBlock();

// When a block is inserted, the tag is only a newly used tag if the
// sector was not previously present in the cache.
- // This assumes BaseTags::insertBlock does not set the valid bit.
if (sector_blk->isValid()) {
// An existing entry's replacement data is just updated
replacementPolicy->touch(sector_blk->replacementData);
@@ -191,6 +187,9 @@
// A new entry resets the replacement data
replacementPolicy->reset(sector_blk->replacementData);
}
+
+ // Do common block insertion functionality
+ BaseTags::insertBlock(addr, is_secure, src_master_ID, task_ID, blk);
}

CacheBlk*
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/14362
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Gerrit-Change-Number: 14362
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Carvalho <***@yahoo.com.br>
Gerrit-MessageType: newchange
Daniel Carvalho (Gerrit)
2018-11-15 15:49:27 UTC
Permalink
Daniel Carvalho has uploaded a new patch set (#2). (
https://gem5-review.googlesource.com/c/public/gem5/+/14362 )

Change subject: mem-cache: Add setters to validate and secure block
......................................................................

mem-cache: Add setters to validate and secure block

In order to allow polymorphism of the block these two
functions have been added, and all direct status
assignments to these bits have been substituted.

We also assert that the block has been invalidated
before insertion. Then the block is validated in
the insertion.

Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Signed-off-by: Daniel R. Carvalho <***@yahoo.com.br>
---
M src/mem/cache/base.cc
M src/mem/cache/cache_blk.cc
M src/mem/cache/cache_blk.hh
M src/mem/cache/tags/sector_tags.cc
4 files changed, 36 insertions(+), 22 deletions(-)
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/14362
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Gerrit-Change-Number: 14362
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Carvalho <***@yahoo.com.br>
Gerrit-MessageType: newpatchset
Daniel Carvalho (Gerrit)
2018-11-16 10:36:46 UTC
Permalink
Daniel Carvalho has uploaded a new patch set (#3). (
https://gem5-review.googlesource.com/c/public/gem5/+/14362 )

Change subject: mem-cache: Add setters to validate and secure block
......................................................................

mem-cache: Add setters to validate and secure block

In order to allow polymorphism of the block these two
functions have been added, and all direct status
assignments to these bits have been substituted.

We also assert that the block has been invalidated
before insertion. Then the block is validated in
the insertion.

Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Signed-off-by: Daniel R. Carvalho <***@yahoo.com.br>
---
M src/mem/cache/base.cc
M src/mem/cache/cache_blk.cc
M src/mem/cache/cache_blk.hh
M src/mem/cache/tags/sector_blk.hh
M src/mem/cache/tags/sector_tags.cc
5 files changed, 44 insertions(+), 23 deletions(-)
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/14362
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Gerrit-Change-Number: 14362
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Carvalho <***@yahoo.com.br>
Gerrit-Reviewer: Daniel Carvalho <***@yahoo.com.br>
Gerrit-CC: Nikos Nikoleris <***@arm.com>
Gerrit-MessageType: newpatchset
Daniel Carvalho (Gerrit)
2018-11-26 12:49:20 UTC
Permalink
Daniel Carvalho has submitted this change and it was merged. (
https://gem5-review.googlesource.com/c/public/gem5/+/14362 )

Change subject: mem-cache: Add setters to validate and secure block
......................................................................

mem-cache: Add setters to validate and secure block

In order to allow polymorphism of the block these two
functions have been added, and all direct status
assignments to these bits have been substituted.

We also assert that the block has been invalidated
before insertion. Then the block is validated in
the insertion.

Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Signed-off-by: Daniel R. Carvalho <***@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/14362
Reviewed-by: Nikos Nikoleris <***@arm.com>
Maintainer: Nikos Nikoleris <***@arm.com>
---
M src/mem/cache/base.cc
M src/mem/cache/cache_blk.cc
M src/mem/cache/cache_blk.hh
M src/mem/cache/tags/sector_blk.hh
M src/mem/cache/tags/sector_tags.cc
5 files changed, 44 insertions(+), 23 deletions(-)

Approvals:
Nikos Nikoleris: Looks good to me, approved; Looks good to me, approved



diff --git a/src/mem/cache/base.cc b/src/mem/cache/base.cc
index 497f75c..244d7ce 100644
--- a/src/mem/cache/base.cc
+++ b/src/mem/cache/base.cc
@@ -1004,7 +1004,7 @@
return false;
}

- blk->status |= (BlkValid | BlkReadable);
+ blk->status |= BlkReadable;
}
// only mark the block dirty if we got a writeback command,
// and leave it as is for a clean writeback
@@ -1062,7 +1062,7 @@
return false;
}

- blk->status |= (BlkValid | BlkReadable);
+ blk->status |= BlkReadable;
}
}

@@ -1149,26 +1149,23 @@
// No replaceable block or a mostly exclusive
// cache... just use temporary storage to complete the
// current request and then get rid of it
- assert(!tempBlock->isValid());
blk = tempBlock;
tempBlock->insert(addr, is_secure);
DPRINTF(Cache, "using temp block for %#llx (%s)\n", addr,
is_secure ? "s" : "ns");
}
-
- // we should never be overwriting a valid block
- assert(!blk->isValid());
} else {
// existing block... probably an upgrade
- assert(regenerateBlkAddr(blk) == addr);
- assert(blk->isSecure() == is_secure);
- // either we're getting new data or the block should already be
valid
- assert(pkt->hasData() || blk->isValid());
// don't clear block status... if block is already dirty we
// don't want to lose that
}

- blk->status |= BlkValid | BlkReadable;
+ // Block is guaranteed to be valid at this point
+ assert(blk->isValid());
+ assert(blk->isSecure() == is_secure);
+ assert(regenerateBlkAddr(blk) == addr);
+
+ blk->status |= BlkReadable;

// sanity check for whole-line writes, which should always be
// marked as writable as part of the fill, and then later marked
diff --git a/src/mem/cache/cache_blk.cc b/src/mem/cache/cache_blk.cc
index a77fc63..c4730ca 100644
--- a/src/mem/cache/cache_blk.cc
+++ b/src/mem/cache/cache_blk.cc
@@ -46,6 +46,9 @@
CacheBlk::insert(const Addr tag, const bool is_secure,
const int src_master_ID, const uint32_t task_ID)
{
+ // Make sure that the block has been properly invalidated
+ assert(status == 0);
+
// Set block tag
this->tag = tag;

@@ -63,10 +66,11 @@

// Set secure state
if (is_secure) {
- status = BlkSecure;
- } else {
- status = 0;
+ setSecure();
}
+
+ // Validate block
+ setValid();
}

void
diff --git a/src/mem/cache/cache_blk.hh b/src/mem/cache/cache_blk.hh
index 80983a6..d7530d6 100644
--- a/src/mem/cache/cache_blk.hh
+++ b/src/mem/cache/cache_blk.hh
@@ -244,10 +244,27 @@
}

/**
+ * Set valid bit.
+ */
+ virtual void setValid()
+ {
+ assert(!isValid());
+ status |= BlkValid;
+ }
+
+ /**
+ * Set secure bit.
+ */
+ virtual void setSecure()
+ {
+ status |= BlkSecure;
+ }
+
+ /**
* 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
- * current tick. Does not make block valid.
+ * current tick. Marks the block valid.
*
* @param tag Block address tag.
* @param is_secure Whether the block is in secure space or not.
@@ -425,15 +442,19 @@
void insert(const Addr addr, const bool is_secure,
const int src_master_ID=0, const uint32_t task_ID=0)
override
{
+ // Make sure that the block has been properly invalidated
+ assert(status == 0);
+
// Set block address
_addr = addr;

// Set secure state
if (is_secure) {
- status = BlkSecure;
- } else {
- status = 0;
+ setSecure();
}
+
+ // Validate block
+ setValid();
}

/**
diff --git a/src/mem/cache/tags/sector_blk.hh
b/src/mem/cache/tags/sector_blk.hh
index 6eed044..12649fc 100644
--- a/src/mem/cache/tags/sector_blk.hh
+++ b/src/mem/cache/tags/sector_blk.hh
@@ -105,7 +105,7 @@
* 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
- * current tick. Does not make block valid.
+ * current tick. Marks the block valid.
*
* @param tag Block address tag.
* @param is_secure Whether the block is in secure space or not.
diff --git a/src/mem/cache/tags/sector_tags.cc
b/src/mem/cache/tags/sector_tags.cc
index 68440c2..ad374e5 100644
--- a/src/mem/cache/tags/sector_tags.cc
+++ b/src/mem/cache/tags/sector_tags.cc
@@ -171,16 +171,12 @@
const int src_master_ID, const uint32_t task_ID,
CacheBlk *blk)
{
- // Do common block insertion functionality
- BaseTags::insertBlock(addr, is_secure, src_master_ID, task_ID, blk);
-
// Get block's sector
SectorSubBlk* sub_blk = static_cast<SectorSubBlk*>(blk);
const SectorBlk* sector_blk = sub_blk->getSectorBlock();

// When a block is inserted, the tag is only a newly used tag if the
// sector was not previously present in the cache.
- // This assumes BaseTags::insertBlock does not set the valid bit.
if (sector_blk->isValid()) {
// An existing entry's replacement data is just updated
replacementPolicy->touch(sector_blk->replacementData);
@@ -191,6 +187,9 @@
// A new entry resets the replacement data
replacementPolicy->reset(sector_blk->replacementData);
}
+
+ // Do common block insertion functionality
+ BaseTags::insertBlock(addr, is_secure, src_master_ID, task_ID, blk);
}

CacheBlk*
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/14362
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: Ie7be42408721ad4c2c9dc880f82a62cb594f8668
Gerrit-Change-Number: 14362
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Carvalho <***@yahoo.com.br>
Gerrit-Reviewer: Daniel Carvalho <***@yahoo.com.br>
Gerrit-Reviewer: Nikos Nikoleris <***@arm.com>
Gerrit-MessageType: merged
Continue reading on narkive:
Loading...