From f012166bb600ebaeefa48e74f7dd7fdfc9742506 Mon Sep 17 00:00:00 2001 From: Andreas Hansson Date: Tue, 2 Dec 2014 06:07:52 -0500 Subject: [PATCH] mem: Cleanup Packet::checkFunctional and hasData usage This patch cleans up the use of hasData and checkFunctional in the packet. The hasData function is unfortunately suggesting that it checks if the packet has a valid data pointer, when it does in fact only check if the specific packet type is specified to have a data payload. The confusion led to a bug in checkFunctional. The latter function is also tidied up to avoid name overloading. --- src/mem/cache/cache_impl.hh | 8 ++++++- src/mem/packet.cc | 20 ++++++++++------- src/mem/packet.hh | 43 +++++++++++++++++++++++++------------ 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh index 8e8079d58..296f31ebd 100644 --- a/src/mem/cache/cache_impl.hh +++ b/src/mem/cache/cache_impl.hh @@ -1481,6 +1481,10 @@ Cache::handleFill(PacketPtr pkt, BlkType *blk, if (blk == NULL) { // better have read new data... assert(pkt->hasData()); + + // only read reaponses have data + assert(pkt->isRead()); + // need to do a replacement blk = allocateBlock(addr, is_secure, writebacks); if (blk == NULL) { @@ -1538,8 +1542,10 @@ Cache::handleFill(PacketPtr pkt, BlkType *blk, DPRINTF(Cache, "Block addr %x (%s) moving from state %x to %s\n", addr, is_secure ? "s" : "ns", old_state, blk->print()); - // if we got new data, copy it in + // if we got new data, copy it in (checking for a read response + // and a response that has data is the same in the end) if (pkt->isRead()) { + assert(pkt->hasData()); std::memcpy(blk->data, pkt->getConstPtr(), blkSize); } diff --git a/src/mem/packet.cc b/src/mem/packet.cc index 758770824..09b612ad0 100644 --- a/src/mem/packet.cc +++ b/src/mem/packet.cc @@ -174,7 +174,7 @@ MemCmd::commandInfo[] = bool Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, - uint8_t *data) + uint8_t *_data) { Addr func_start = getAddr(); Addr func_end = getAddr() + getSize() - 1; @@ -189,12 +189,14 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, // check print first since it doesn't require data if (isPrint()) { + assert(!_data); safe_cast(senderState)->printObj(obj); return false; } - // if there's no data, there's no need to look further - if (!data) { + // we allow the caller to pass NULL to signify the other packet + // has no data + if (!_data) { return false; } @@ -204,7 +206,9 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, if (isRead()) { if (func_start >= val_start && func_end <= val_end) { - memcpy(getPtr(), data + offset, getSize()); + memcpy(getPtr(), _data + offset, getSize()); + // complete overlap, and as the current packet is a read + // we are done return true; } else { // Offsets and sizes to copy in case of partial overlap @@ -271,7 +275,7 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, // copy the first half uint8_t *dest = getPtr() + func_offset; - uint8_t *src = data + val_offset; + uint8_t *src = _data + val_offset; memcpy(dest, src, (bytesValidStart - func_offset)); // re-calc the offsets and indices to do the copy @@ -293,7 +297,7 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, // copy partial data into the packet's data array uint8_t *dest = getPtr() + func_offset; - uint8_t *src = data + val_offset; + uint8_t *src = _data + val_offset; memcpy(dest, src, overlap_size); // check if we're done filling the functional access @@ -302,11 +306,11 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, } } else if (isWrite()) { if (offset >= 0) { - memcpy(data + offset, getConstPtr(), + memcpy(_data + offset, getConstPtr(), (min(func_end, val_end) - func_start) + 1); } else { // val_start > func_start - memcpy(data, getConstPtr() - offset, + memcpy(_data, getConstPtr() - offset, (min(func_end, val_end) - val_start) + 1); } } else { diff --git a/src/mem/packet.hh b/src/mem/packet.hh index 357134c75..d0b210975 100644 --- a/src/mem/packet.hh +++ b/src/mem/packet.hh @@ -185,6 +185,12 @@ class MemCmd bool needsExclusive() const { return testCmdAttrib(NeedsExclusive); } bool needsResponse() const { return testCmdAttrib(NeedsResponse); } bool isInvalidate() const { return testCmdAttrib(IsInvalidate); } + + /** + * Check if this particular packet type carries payload data. Note + * that this does not reflect if the data pointer of the packet is + * valid or not. + */ bool hasData() const { return testCmdAttrib(HasData); } bool isLLSC() const { return testCmdAttrib(IsLlsc); } bool isSWPrefetch() const { return testCmdAttrib(IsSWPrefetch); } @@ -917,28 +923,37 @@ class Packet : public Printable data = new uint8_t[getSize()]; } - /** - * Check a functional request against a memory value represented - * by a base/size pair and an associated data array. If the - * functional request is a read, it may be satisfied by the memory - * value. If the functional request is a write, it may update the - * memory value. - */ - bool checkFunctional(Printable *obj, Addr base, bool is_secure, int size, - uint8_t *data); - /** * Check a functional request against a memory value stored in - * another packet (i.e. an in-transit request or response). + * another packet (i.e. an in-transit request or + * response). Returns true if the current packet is a read, and + * the other packet provides the data, which is then copied to the + * current packet. If the current packet is a write, and the other + * packet intersects this one, then we update the data + * accordingly. */ bool - checkFunctional(PacketPtr other) + checkFunctional(PacketPtr other) { - uint8_t *data = other->hasData() ? other->getPtr() : NULL; + // all packets that are carrying a payload should have a valid + // data pointer return checkFunctional(other, other->getAddr(), other->isSecure(), - other->getSize(), data); + other->getSize(), + other->hasData() ? + other->getPtr() : NULL); } + /** + * Check a functional request against a memory value represented + * by a base/size pair and an associated data array. If the + * current packet is a read, it may be satisfied by the memory + * value. If the current packet is a write, it may update the + * memory value. + */ + bool + checkFunctional(Printable *obj, Addr base, bool is_secure, int size, + uint8_t *_data); + /** * Push label for PrintReq (safe to call unconditionally). */ -- 2.30.2