Hi Andreas, Copying from review request to have in both places:
I'm wondering why this patch was checked in ( http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', and the review request is only 4 days old. http://reviews.gem5.org/r/3044/ This change also breaks some packet use cases we have in gem5-gpu. Specifically, this change now requires that Requests be translated or otherwise find a valid physical address before a packet can be constructed. I'm not convinced that this is a reasonable requirement, especially since packets can be used between components that operate completely on virtual addresses. Can you please revert this change? Thanks! Joel On Fri, Aug 21, 2015 at 7:15 AM, Andreas Hansson <[email protected]> wrote: > changeset 842f56345a42 in /z/repo/gem5 > details: http://repo.gem5.org/gem5?cmd=changeset;node=842f56345a42 > description: > mem: Reflect that packet address and size are always valid > > This patch simplifies the packet, and removes the possibility of > creating a packet without a valid address and/or size. Under no > circumstances are these fields set at a later point, and thus they > really have to be provided at construction time. > > The patch also fixes a case there the MinorCPU creates a packet > without a valid address and size, only to later delete it. > > diffstat: > > src/cpu/minor/lsq.cc | 6 ++++ > src/mem/packet.hh | 71 > +++++++++++++-------------------------------------- > 2 files changed, 24 insertions(+), 53 deletions(-) > > diffs (148 lines): > > diff -r 54071fd5c397 -r 842f56345a42 src/cpu/minor/lsq.cc > --- a/src/cpu/minor/lsq.cc Fri Aug 21 07:03:25 2015 -0400 > +++ b/src/cpu/minor/lsq.cc Fri Aug 21 07:03:27 2015 -0400 > @@ -1578,6 +1578,12 @@ > if (packet) > return; > > + // if the translation faulted, do not create a packet > + if (fault != NoFault) { > + assert(packet == NULL); > + return; > + } > + > packet = makePacketForRequest(request, isLoad, this, data); > /* Null the ret data so we know not to deallocate it when the > * ret is destroyed. The data now belongs to the ret and > diff -r 54071fd5c397 -r 842f56345a42 src/mem/packet.hh > --- a/src/mem/packet.hh Fri Aug 21 07:03:25 2015 -0400 > +++ b/src/mem/packet.hh Fri Aug 21 07:03:27 2015 -0400 > @@ -255,10 +255,6 @@ > // Snoop response flags > MEM_INHIBIT = 0x00000008, > > - /// Are the 'addr' and 'size' fields valid? > - VALID_ADDR = 0x00000100, > - VALID_SIZE = 0x00000200, > - > /// Is the data pointer set to a value that shouldn't be freed > /// when the packet is destroyed? > STATIC_DATA = 0x00001000, > @@ -523,7 +519,7 @@ > > void copyError(Packet *pkt) { assert(pkt->isError()); cmd = pkt->cmd; > } > > - Addr getAddr() const { assert(flags.isSet(VALID_ADDR)); return addr; } > + Addr getAddr() const { return addr; } > /** > * Update the address of this packet mid-transaction. This is used > * by the address mapper to change an already set address to a new > @@ -531,9 +527,9 @@ > * an existing address, so it asserts that the current address is > * valid. > */ > - void setAddr(Addr _addr) { assert(flags.isSet(VALID_ADDR)); addr = > _addr; } > + void setAddr(Addr _addr) { addr = _addr; } > > - unsigned getSize() const { assert(flags.isSet(VALID_SIZE)); return > size; } > + unsigned getSize() const { return size; } > > Addr getOffset(unsigned int blk_size) const > { > @@ -545,11 +541,7 @@ > return getAddr() & ~(Addr(blk_size - 1)); > } > > - bool isSecure() const > - { > - assert(flags.isSet(VALID_ADDR)); > - return _isSecure; > - } > + bool isSecure() const { return _isSecure; } > > /** > * It has been determined that the SC packet should successfully > update > @@ -577,43 +569,28 @@ > > /** > * Constructor. Note that a Request object must be constructed > - * first, but the Requests's physical address and size fields need > - * not be valid. The command must be supplied. > + * first, and have a valid physical address and size. The command > + * must be supplied. > */ > Packet(const RequestPtr _req, MemCmd _cmd) > - : cmd(_cmd), req(_req), data(nullptr), addr(0), _isSecure(false), > - size(0), headerDelay(0), payloadDelay(0), > + : cmd(_cmd), req(_req), data(nullptr), addr(req->getPaddr()), > + _isSecure(req->isSecure()), size(req->getSize()), > + headerDelay(0), payloadDelay(0), > senderState(NULL) > - { > - if (req->hasPaddr()) { > - addr = req->getPaddr(); > - flags.set(VALID_ADDR); > - _isSecure = req->isSecure(); > - } > - if (req->hasSize()) { > - size = req->getSize(); > - flags.set(VALID_SIZE); > - } > - } > + { } > > /** > - * Alternate constructor if you are trying to create a packet with > - * a request that is for a whole block, not the address from the > - * req. this allows for overriding the size/addr of the req. > + * Alternate constructor when creating a packet that is for a > + * whole block. This allows for overriding the size and addr of > + * the request. > */ > - Packet(const RequestPtr _req, MemCmd _cmd, int _blkSize) > - : cmd(_cmd), req(_req), data(nullptr), addr(0), _isSecure(false), > + Packet(const RequestPtr _req, MemCmd _cmd, unsigned _blkSize) > + : cmd(_cmd), req(_req), data(nullptr), > + addr(_req->getPaddr() & ~Addr(_blkSize - 1)), > + _isSecure(_req->isSecure()), size(_blkSize), > headerDelay(0), payloadDelay(0), > senderState(NULL) > - { > - if (req->hasPaddr()) { > - addr = req->getPaddr() & ~(_blkSize - 1); > - flags.set(VALID_ADDR); > - _isSecure = req->isSecure(); > - } > - size = _blkSize; > - flags.set(VALID_SIZE); > - } > + { } > > /** > * Alternate constructor for copying a packet. Copy all fields > @@ -634,8 +611,6 @@ > if (!clear_flags) > flags.set(pkt->flags & COPY_FLAGS); > > - flags.set(pkt->flags & (VALID_ADDR|VALID_SIZE)); > - > // should we allocate space for data, or not, the express > // snoops do not need to carry any data as they only serve to > // co-ordinate state changes > @@ -757,16 +732,6 @@ > } > } > > - void > - setSize(unsigned size) > - { > - assert(!flags.isSet(VALID_SIZE)); > - > - this->size = size; > - flags.set(VALID_SIZE); > - } > - > - > public: > /** > * @{ > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > -- Joel Hestness PhD Candidate, Computer Architecture Dept. of Computer Science, University of Wisconsin - Madison http://pages.cs.wisc.edu/~hestness/ _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
