From cbfe914f885cc54a3c8763c0ed6aed8e425d6357 Mon Sep 17 00:00:00 2001 From: Robert Kovacsics Date: Fri, 13 Jul 2018 17:28:11 +0100 Subject: mem: Fix off-by-one error in checkFunctional, and simplify it There was an off-by-one error in the isRead() case, as `val_end` and `func_end` pointed to the last byte to write to (not one past the last byte), and thus `*_end - *_start` was not the length of the data to memcpy. This was correct in the case of val_start >= func_start && val_end <= func_end where `overlap_size = size`, but if it were (as the other cases suggest) `overlap_size = val_end - val_start`, then it would also be off by one. Also, the isWrite() case catered for this. I simplified the four ifs into one case which uses min/max (this is how I spotted the inconsistency). Change-Id: Ib5c5da084652e752f6baf1eec56b51b4f0f5c95c Reviewed-on: https://gem5-review.googlesource.com/11750 Reviewed-by: Daniel Carvalho Reviewed-by: Nikos Nikoleris Maintainer: Nikos Nikoleris --- src/mem/packet.cc | 116 ++++++++++++++++-------------------------------------- 1 file changed, 34 insertions(+), 82 deletions(-) diff --git a/src/mem/packet.cc b/src/mem/packet.cc index 7a81cdbb7..f2f7fd9b8 100644 --- a/src/mem/packet.cc +++ b/src/mem/packet.cc @@ -50,6 +50,7 @@ #include "mem/packet.hh" +#include #include #include @@ -227,10 +228,10 @@ bool Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, uint8_t *_data) { - Addr func_start = getAddr(); - Addr func_end = getAddr() + getSize() - 1; - Addr val_start = addr; - Addr val_end = val_start + size - 1; + const Addr func_start = getAddr(); + const Addr func_end = getAddr() + getSize() - 1; + const Addr val_start = addr; + const Addr val_end = val_start + size - 1; if (is_secure != _isSecure || func_start > val_end || val_start > func_end) { @@ -251,94 +252,45 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size, return false; } - // offset of functional request into supplied value (could be - // negative if partial overlap) - int offset = func_start - val_start; + const Addr val_offset = func_start > val_start ? + func_start - val_start : 0; + const Addr func_offset = func_start < val_start ? + val_start - func_start : 0; + const Addr overlap_size = std::min(val_end, func_end)+1 - + std::max(val_start, func_start); if (isRead()) { - if (func_start >= val_start && func_end <= val_end) { - memcpy(getPtr(), _data + offset, getSize()); - if (bytesValid.empty()) - bytesValid.resize(getSize(), true); - // 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 - int func_offset; - int val_offset; - int overlap_size; - - // calculate offsets and copy sizes for the two byte arrays - if (val_start < func_start && val_end <= func_end) { - // the one we are checking against starts before and - // ends before or the same - val_offset = func_start - val_start; - func_offset = 0; - overlap_size = val_end - func_start; - } else if (val_start >= func_start && val_end > func_end) { - // the one we are checking against starts after or the - // same, and ends after - val_offset = 0; - func_offset = val_start - func_start; - overlap_size = func_end - val_start; - } else if (val_start >= func_start && val_end <= func_end) { - // the one we are checking against is completely - // subsumed in the current packet, possibly starting - // and ending at the same address - val_offset = 0; - func_offset = val_start - func_start; - overlap_size = size; - } else if (val_start < func_start && val_end > func_end) { - // the current packet is completely subsumed in the - // one we are checking against - val_offset = func_start - val_start; - func_offset = 0; - overlap_size = func_end - func_start; - } else { - panic("Missed a case for checkFunctional with " - " %s 0x%x size %d, against 0x%x size %d\n", - cmdString(), getAddr(), getSize(), addr, size); - } - - // copy partial data into the packet's data array - uint8_t *dest = getPtr() + func_offset; - uint8_t *src = _data + val_offset; - memcpy(dest, src, overlap_size); + memcpy(getPtr() + func_offset, + _data + val_offset, + overlap_size); - // initialise the tracking of valid bytes if we have not - // used it already - if (bytesValid.empty()) - bytesValid.resize(getSize(), false); + // initialise the tracking of valid bytes if we have not + // used it already + if (bytesValid.empty()) + bytesValid.resize(getSize(), false); - // track if we are done filling the functional access - bool all_bytes_valid = true; + // track if we are done filling the functional access + bool all_bytes_valid = true; - int i = 0; + int i = 0; - // check up to func_offset - for (; all_bytes_valid && i < func_offset; ++i) - all_bytes_valid &= bytesValid[i]; + // check up to func_offset + for (; all_bytes_valid && i < func_offset; ++i) + all_bytes_valid &= bytesValid[i]; - // update the valid bytes - for (i = func_offset; i < func_offset + overlap_size; ++i) - bytesValid[i] = true; + // update the valid bytes + for (i = func_offset; i < func_offset + overlap_size; ++i) + bytesValid[i] = true; - // check the bit after the update we just made - for (; all_bytes_valid && i < getSize(); ++i) - all_bytes_valid &= bytesValid[i]; + // check the bit after the update we just made + for (; all_bytes_valid && i < getSize(); ++i) + all_bytes_valid &= bytesValid[i]; - return all_bytes_valid; - } + return all_bytes_valid; } else if (isWrite()) { - if (offset >= 0) { - memcpy(_data + offset, getConstPtr(), - (min(func_end, val_end) - func_start) + 1); - } else { - // val_start > func_start - memcpy(_data, getConstPtr() - offset, - (min(func_end, val_end) - val_start) + 1); - } + memcpy(_data + val_offset, + getConstPtr() + func_offset, + overlap_size); } else { panic("Don't know how to handle command %s\n", cmdString()); } -- cgit v1.2.3