Mem: Make errors in the memory system be responses, not requests. Fixes cache handling of error responses.

--HG--
extra : convert_revision : 22309fc61bd1be0f8d31a3926f290290789a37e5

src/cpu/o3/fetch_impl.hh

@@ -364,6 +364,8 @@ DefaultFetch<Impl>::processCacheCompletion(PacketPtr pkt)
 
     DPRINTF(Fetch, "[tid:%u] Waking up from cache miss.\n",tid);
 
+    assert(!pkt->wasNacked());
+
     // Only change the status if it's still waiting on the icache access
     // to return.
     if (fetchStatus[tid] != IcacheWaitResponse ||

src/cpu/o3/lsq_impl.hh

@@ -80,6 +80,8 @@ template <class Impl>
 bool
 LSQ<Impl>::DcachePort::recvTiming(PacketPtr pkt)
 {
+    if (pkt->isError())
+        DPRINTF(LSQ, "Got error packet back for address: %#X\n", pkt->getAddr());
     if (pkt->isResponse()) {
         lsq->thread[pkt->req->getThreadNum()].completeDataAccess(pkt);
     }

src/cpu/o3/lsq_unit_impl.hh

@@ -83,6 +83,8 @@ LSQUnit<Impl>::completeDataAccess(PacketPtr pkt)
 
     //iewStage->ldstQueue.removeMSHR(inst->threadNumber,inst->seqNum);
 
+    assert(!pkt->wasNacked());
+
     if (isSwitchedOut() || inst->isSquashed()) {
         iewStage->decrWb(inst->seqNum);
     } else {

src/cpu/simple/atomic.cc

@@ -311,6 +311,7 @@ AtomicSimpleCPU::read(Addr addr, T &data, unsigned flags)
                 dcache_latency = dcachePort.sendAtomic(&pkt);
         }
         dcache_access = true;
+
         assert(!pkt.isError());
 
         data = gtoh(data);
@@ -536,6 +537,7 @@ AtomicSimpleCPU::tick()
                 else
                     icache_latency = icachePort.sendAtomic(&ifetch_pkt);
 
+                assert(!ifetch_pkt.isError());
 
                 // ifetch_req is initialized to read the instruction directly
                 // into the CPU object's inst field.

src/cpu/simple/timing.cc

@@ -564,7 +564,7 @@ TimingSimpleCPU::IcachePort::ITickEvent::process()
 bool
 TimingSimpleCPU::IcachePort::recvTiming(PacketPtr pkt)
 {
-    if (pkt->isResponse()) {
+    if (pkt->isResponse() && !pkt->wasNacked()) {
         // delay processing of returned data until next CPU clock edge
         Tick next_tick = cpu->nextCycle(curTick);
 
@@ -658,7 +658,7 @@ TimingSimpleCPU::DcachePort::setPeer(Port *port)
 bool
 TimingSimpleCPU::DcachePort::recvTiming(PacketPtr pkt)
 {
-    if (pkt->isResponse()) {
+    if (pkt->isResponse() && !pkt->wasNacked()) {
         // delay processing of returned data until next CPU clock edge
         Tick next_tick = cpu->nextCycle(curTick);
 

src/dev/alpha/console.cc

@@ -107,6 +107,7 @@ AlphaConsole::read(PacketPtr pkt)
     Addr daddr = pkt->getAddr() - pioAddr;
 
     pkt->allocate();
+    pkt->makeAtomicResponse();
 
     switch (pkt->getSize())
     {
@@ -188,7 +189,6 @@ AlphaConsole::read(PacketPtr pkt)
         default:
           pkt->setBadAddress();
     }
-    pkt->makeAtomicResponse();
     return pioDelay;
 }
 

src/dev/isa_fake.cc

@@ -56,6 +56,7 @@ Tick
 IsaFake::read(PacketPtr pkt)
 {
 
+    pkt->makeAtomicResponse();
     if (params()->warn_access != "")
         warn("Device %s accessed by read to address %#x size=%d\n",
                 name(), pkt->getAddr(), pkt->getSize());
@@ -83,7 +84,6 @@ IsaFake::read(PacketPtr pkt)
           default:
             panic("invalid access size!\n");
         }
-        pkt->makeAtomicResponse();
     }
     return pioDelay;
 }
@@ -91,6 +91,7 @@ IsaFake::read(PacketPtr pkt)
 Tick
 IsaFake::write(PacketPtr pkt)
 {
+    pkt->makeAtomicResponse();
     if (params()->warn_access != "") {
         uint64_t data;
         switch (pkt->getSize()) {
@@ -138,7 +139,6 @@ IsaFake::write(PacketPtr pkt)
                 panic("invalid access size!\n");
             }
         }
-        pkt->makeAtomicResponse();
     }
     return pioDelay;
 }

src/mem/bridge.cc

@@ -119,17 +119,17 @@ Bridge::BridgePort::recvTiming(PacketPtr pkt)
 
     DPRINTF(BusBridge, "Local queue size: %d outreq: %d outresp: %d\n",
                     sendQueue.size(), queuedRequests, outstandingResponses);
-    DPRINTF(BusBridge, "Remove queue size: %d outreq: %d outresp: %d\n",
+    DPRINTF(BusBridge, "Remote queue size: %d outreq: %d outresp: %d\n",
                     otherPort->sendQueue.size(), otherPort->queuedRequests,
                     otherPort->outstandingResponses);
 
-    if (pkt->isRequest() && otherPort->reqQueueFull() && !pkt->wasNacked()) {
+    if (pkt->isRequest() && otherPort->reqQueueFull()) {
         DPRINTF(BusBridge, "Remote queue full, nacking\n");
         nackRequest(pkt);
         return true;
     }
 
-    if (pkt->needsResponse() && !pkt->wasNacked())
+    if (pkt->needsResponse())
         if (respQueueFull()) {
             DPRINTF(BusBridge, "Local queue full, no space for response, nacking\n");
             DPRINTF(BusBridge, "queue size: %d outreq: %d outstanding resp: %d\n",
@@ -150,8 +150,8 @@ void
 Bridge::BridgePort::nackRequest(PacketPtr pkt)
 {
     // Nack the packet
+    pkt->makeTimingResponse();
     pkt->setNacked();
-    pkt->setDest(pkt->getSrc());
 
     //put it on the list to send
     Tick readyTime = curTick + nackDelay;
@@ -195,27 +195,23 @@ Bridge::BridgePort::nackRequest(PacketPtr pkt)
 void
 Bridge::BridgePort::queueForSendTiming(PacketPtr pkt)
 {
-    if (pkt->isResponse() || pkt->wasNacked()) {
+    if (pkt->isResponse()) {
         // This is a response for a request we forwarded earlier.  The
         // corresponding PacketBuffer should be stored in the packet's
         // senderState field.
+
         PacketBuffer *buf = dynamic_cast<PacketBuffer*>(pkt->senderState);
         assert(buf != NULL);
         // set up new packet dest & senderState based on values saved
         // from original request
         buf->fixResponse(pkt);
 
-        // Check if this packet was expecting a response and it's a nacked
-        // packet, in which case we will never being seeing it
-        if (buf->expectResponse && pkt->wasNacked())
-            --outstandingResponses;
-
         DPRINTF(BusBridge, "response, new dest %d\n", pkt->getDest());
         delete buf;
     }
 
 
-    if (pkt->isRequest() && !pkt->wasNacked()) {
+    if (pkt->isRequest()) {
         ++queuedRequests;
     }
 
@@ -249,7 +245,15 @@ Bridge::BridgePort::trySend()
             buf->origSrc, pkt->getDest(), pkt->getAddr());
 
     bool wasReq = pkt->isRequest();
-    bool wasNacked = pkt->wasNacked();
+    bool was_nacked_here = buf->nackedHere;
+
+    // If the send was successful, make sure sender state was set to NULL
+    // otherwise we could get a NACK back of a packet that didn't expect a
+    // response and we would try to use freed memory.
+
+    Packet::SenderState *old_sender_state = pkt->senderState;
+    if (pkt->isRequest() && !buf->expectResponse)
+        pkt->senderState = NULL;
 
     if (sendTiming(pkt)) {
         // send successful
@@ -266,12 +270,10 @@ Bridge::BridgePort::trySend()
             delete buf;
         }
 
-        if (!wasNacked) {
+        if (wasReq)
-            if (wasReq)
+            --queuedRequests;
-                --queuedRequests;
+        else if (!was_nacked_here)
-            else
+            --outstandingResponses;
-                --outstandingResponses;
-        }
 
         // If there are more packets to send, schedule event to try again.
         if (!sendQueue.empty()) {
@@ -281,8 +283,10 @@ Bridge::BridgePort::trySend()
         }
     } else {
         DPRINTF(BusBridge, "  unsuccessful\n");
+        pkt->senderState = old_sender_state;
         inRetry = true;
     }
+
     DPRINTF(BusBridge, "trySend: queue size: %d outreq: %d outstanding resp: %d\n",
                     sendQueue.size(), queuedRequests, outstandingResponses);
 }

src/mem/bridge.hh

@@ -78,17 +78,19 @@ class Bridge : public MemObject
           public:
             Tick ready;
             PacketPtr pkt;
+            bool nackedHere;
             Packet::SenderState *origSenderState;
             short origSrc;
             bool expectResponse;
 
             PacketBuffer(PacketPtr _pkt, Tick t, bool nack = false)
-                : ready(t), pkt(_pkt),
+                : ready(t), pkt(_pkt), nackedHere(nack),
-                  origSenderState(_pkt->senderState), origSrc(_pkt->getSrc()),
+                  origSenderState(_pkt->senderState),
+                  origSrc(nack ? _pkt->getDest() : _pkt->getSrc() ),
                   expectResponse(_pkt->needsResponse() && !nack)
 
             {
-                if (!pkt->isResponse() && !nack && !pkt->wasNacked())
+                if (!pkt->isResponse() && !nack)
                     pkt->senderState = this;
             }
 

src/mem/bus.cc

@@ -237,6 +237,7 @@ Bus::recvTiming(PacketPtr pkt)
     if (dest_port_id == src) {
         // Must be forwarded snoop up from below...
         assert(dest == Packet::Broadcast);
+        assert(src != defaultId); // catch infinite loops
     } else {
         // send to actual target
         if (!dest_port->sendTiming(pkt))  {

src/mem/cache/cache_impl.hh

@@ -606,7 +606,13 @@ Cache<TagStore>::atomicAccess(PacketPtr pkt)
         DPRINTF(Cache, "Receive response: %s for addr %x in state %i\n",
                 busPkt->cmdString(), busPkt->getAddr(), old_state);
 
-        if (isCacheFill) {
+        bool is_error = busPkt->isError();
+        assert(!busPkt->wasNacked());
+
+        if (is_error && pkt->needsResponse()) {
+            pkt->makeAtomicResponse();
+            pkt->copyError(busPkt);
+        } else if (isCacheFill && !is_error) {
             PacketList writebacks;
             blk = handleFill(busPkt, blk, writebacks);
             satisfyCpuSideRequest(pkt, blk);
@@ -667,6 +673,8 @@ Cache<TagStore>::handleResponse(PacketPtr pkt)
 {
     Tick time = curTick + hitLatency;
     MSHR *mshr = dynamic_cast<MSHR*>(pkt->senderState);
+    bool is_error = pkt->isError();
+
     assert(mshr);
 
     if (pkt->wasNacked()) {
@@ -675,7 +683,11 @@ Cache<TagStore>::handleResponse(PacketPtr pkt)
              "not implemented\n");
         return;
     }
-    assert(!pkt->isError());
+    if (is_error) {
+        DPRINTF(Cache, "Cache received packet with error for address %x, "
+                "cmd: %s\n", pkt->getAddr(), pkt->cmdString());
+    }
+
     DPRINTF(Cache, "Handling response to %x\n", pkt->getAddr());
 
     MSHRQueue *mq = mshr->queue;
@@ -702,7 +714,7 @@ Cache<TagStore>::handleResponse(PacketPtr pkt)
             miss_latency;
     }
 
-    if (mshr->isCacheFill) {
+    if (mshr->isCacheFill && !is_error) {
         DPRINTF(Cache, "Block for addr %x being updated in Cache\n",
                 pkt->getAddr());
 
@@ -744,13 +756,18 @@ Cache<TagStore>::handleResponse(PacketPtr pkt)
             } else {
                 // not a cache fill, just forwarding response
                 completion_time = tags->getHitLatency() + pkt->finishTime;
-                if (pkt->isRead()) {
+                if (pkt->isRead() && !is_error) {
                     target->pkt->setData(pkt->getPtr<uint8_t>());
                 }
             }
             target->pkt->makeTimingResponse();
+            // if this packet is an error copy that to the new packet
+            if (is_error)
+                target->pkt->copyError(pkt);
             cpuSidePort->respond(target->pkt, completion_time);
         } else {
+            // I don't believe that a snoop can be in an error state
+            assert(!is_error);
             // response to snoop request
             DPRINTF(Cache, "processing deferred snoop...\n");
             handleSnoop(target->pkt, blk, true, true);

src/mem/packet.cc

@@ -117,11 +117,11 @@ MemCmd::commandInfo[] =
     { SET5(IsRead, IsWrite, NeedsExclusive, IsResponse, HasData),
             InvalidCmd, "SwapResp" },
     /* NetworkNackError  -- nacked at network layer (not by protocol) */
-    { SET2(IsRequest, IsError), InvalidCmd, "NetworkNackError" },
+    { SET2(IsResponse, IsError), InvalidCmd, "NetworkNackError" },
     /* InvalidDestError  -- packet dest field invalid */
-    { SET2(IsRequest, IsError), InvalidCmd, "InvalidDestError" },
+    { SET2(IsResponse, IsError), InvalidCmd, "InvalidDestError" },
     /* BadAddressError   -- memory address invalid */
-    { SET2(IsRequest, IsError), InvalidCmd, "BadAddressError" }
+    { SET2(IsResponse, IsError), InvalidCmd, "BadAddressError" }
 };
 
 

src/mem/packet.hh

@@ -332,10 +332,11 @@ class Packet : public FastAlloc
     // Network error conditions... encapsulate them as methods since
     // their encoding keeps changing (from result field to command
     // field, etc.)
-    void setNacked()     { origCmd = cmd; cmd = MemCmd::NetworkNackError; }
+    void setNacked()     { assert(isResponse()); cmd = MemCmd::NetworkNackError; }
-    void setBadAddress() { origCmd = cmd; cmd = MemCmd::BadAddressError; }
+    void setBadAddress() { assert(isResponse()); cmd = MemCmd::BadAddressError; }
     bool wasNacked()     { return cmd == MemCmd::NetworkNackError; }
     bool hadBadAddress() { return cmd == MemCmd::BadAddressError; }
+    void copyError(Packet *pkt) { assert(pkt->isError()); cmd = pkt->cmd; }
 
     bool nic_pkt() { panic("Unimplemented"); M5_DUMMY_RETURN }
 
@@ -431,6 +432,7 @@ class Packet : public FastAlloc
     {
         assert(needsResponse());
         assert(isRequest());
+        origCmd = cmd;
         cmd = cmd.responseCommand();
         dest = src;
         destValid = srcValid;