Packet: Remove NACKs from packet and its use in endpoints
authorAndreas Hansson <andreas.hansson@arm.com>
Wed, 22 Aug 2012 15:39:59 +0000 (11:39 -0400)
committerAndreas Hansson <andreas.hansson@arm.com>
Wed, 22 Aug 2012 15:39:59 +0000 (11:39 -0400)
This patch removes the NACK frrom the packet as there is no longer any
module in the system that issues them (the bridge was the only one and
the previous patch removes that).

The handling of NACKs was mostly avoided throughout the code base, by
using e.g. panic or assert false, but in a few locations the NACKs
were actually dealt with (although NACKs never occured in any of the
regressions). Most notably, the DMA port will now never receive a NACK
and the backoff time is thus never changed. As a consequence, the
entire backoff mechanism (similar to a PCI bus) is now removed and the
DMA port entirely relies on the bus performing the arbitration and
issuing a retry when appropriate. This is more in line with e.g. PCIe.

Surprisingly, this patch has no impact on any of the regressions. As
mentioned in the patch that removes the NACK from the bridge, a
follow-up patch should change the request and response buffer size for
at least one regression to also verify that the system behaves as
expected when the bridge fills up.

14 files changed:
src/arch/arm/ArmTLB.py
src/arch/arm/table_walker.cc
src/arch/arm/table_walker.hh
src/arch/x86/pagetable_walker.cc
src/cpu/o3/fetch_impl.hh
src/cpu/o3/lsq_unit_impl.hh
src/cpu/simple/timing.cc
src/dev/Device.py
src/dev/copy_engine.cc
src/dev/dma_device.cc
src/dev/dma_device.hh
src/mem/cache/cache_impl.hh
src/mem/packet.cc
src/mem/packet.hh

index 8599fa75f57a8b81335a7c36a91832b6d8102ff5..9572d20915f108d4b12f12ad3f4685ba4bfc0557 100644 (file)
@@ -47,9 +47,6 @@ class ArmTableWalker(MemObject):
     cxx_class = 'ArmISA::TableWalker'
     port = MasterPort("Port for TableWalker to do walk the translation with")
     sys = Param.System(Parent.any, "system object parameter")
-    min_backoff = Param.Tick(0, "Minimum backoff delay after failed send")
-    max_backoff = Param.Tick(100000, "Minimum backoff delay after failed send")
-
 
 class ArmTLB(SimObject):
     type = 'ArmTLB'
index 7dbe92d9b4d397b20b7d178af1a2023b6fffe5f6..ea71e6f1c2d35157ce5046b099a188b2393142d2 100644 (file)
@@ -51,8 +51,7 @@
 using namespace ArmISA;
 
 TableWalker::TableWalker(const Params *p)
-    : MemObject(p), port(this, params()->sys, params()->min_backoff,
-                         params()->max_backoff), drainEvent(NULL),
+    : MemObject(p), port(this, params()->sys), drainEvent(NULL),
       tlb(NULL), currState(NULL), pending(false),
       masterId(p->sys->getMasterId(name())),
       doL1DescEvent(this), doL2DescEvent(this), doProcessEvent(this)
index 1b95182c8ef32fadeb83f316df77cc4eb7d9c35f..b6fee66ffca0381994b1eadad0b25fc610fd223e 100644 (file)
@@ -287,9 +287,8 @@ class TableWalker : public MemObject
          * A snooping DMA port merely calls the construtor of the DMA
          * port.
          */
-        SnoopingDmaPort(MemObject *dev, System *s, Tick min_backoff,
-                        Tick max_backoff) :
-            DmaPort(dev, s, min_backoff, max_backoff)
+        SnoopingDmaPort(MemObject *dev, System *s) :
+            DmaPort(dev, s)
         { }
     };
 
index b6e6c33f44f8ac3d8e815daa0feb3d0caf98ad54..46d608aceb3372a99f05eeb9654d158d411d83d9 100644 (file)
@@ -570,63 +570,49 @@ bool
 Walker::WalkerState::recvPacket(PacketPtr pkt)
 {
     assert(pkt->isResponse());
-    if (!pkt->wasNacked()) {
-        assert(inflight);
-        assert(state == Waiting);
-        assert(!read);
-        inflight--;
-        if (pkt->isRead()) {
-            state = nextState;
-            nextState = Ready;
-            PacketPtr write = NULL;
-            read = pkt;
-            timingFault = stepWalk(write);
-            state = Waiting;
-            assert(timingFault == NoFault || read == NULL);
-            if (write) {
-                writes.push_back(write);
-            }
-            sendPackets();
-        } else {
-            sendPackets();
-        }
-        if (inflight == 0 && read == NULL && writes.size() == 0) {
-            state = Ready;
-            nextState = Waiting;
-            if (timingFault == NoFault) {
-                /*
-                 * Finish the translation. Now that we now the right entry is
-                 * in the TLB, this should work with no memory accesses.
-                 * There could be new faults unrelated to the table walk like
-                 * permissions violations, so we'll need the return value as
-                 * well.
-                 */
-                bool delayedResponse;
-                Fault fault = walker->tlb->translate(req, tc, NULL, mode,
-                        delayedResponse, true);
-                assert(!delayedResponse);
-                // Let the CPU continue.
-                translation->finish(fault, req, tc, mode);
-            } else {
-                // There was a fault during the walk. Let the CPU know.
-                translation->finish(timingFault, req, tc, mode);
-            }
-            return true;
+    assert(inflight);
+    assert(state == Waiting);
+    assert(!read);
+    inflight--;
+    if (pkt->isRead()) {
+        state = nextState;
+        nextState = Ready;
+        PacketPtr write = NULL;
+        read = pkt;
+        timingFault = stepWalk(write);
+        state = Waiting;
+        assert(timingFault == NoFault || read == NULL);
+        if (write) {
+            writes.push_back(write);
         }
+        sendPackets();
     } else {
-        DPRINTF(PageTableWalker, "Request was nacked. Entering retry state\n");
-        pkt->reinitNacked();
-        if (!walker->sendTiming(this, pkt)) {
-            inflight--;
-            retrying = true;
-            if (pkt->isWrite()) {
-                writes.push_back(pkt);
-            } else {
-                assert(!read);
-                read = pkt;
-            }
+        sendPackets();
+    }
+    if (inflight == 0 && read == NULL && writes.size() == 0) {
+        state = Ready;
+        nextState = Waiting;
+        if (timingFault == NoFault) {
+            /*
+             * Finish the translation. Now that we now the right entry is
+             * in the TLB, this should work with no memory accesses.
+             * There could be new faults unrelated to the table walk like
+             * permissions violations, so we'll need the return value as
+             * well.
+             */
+            bool delayedResponse;
+            Fault fault = walker->tlb->translate(req, tc, NULL, mode,
+                                                 delayedResponse, true);
+            assert(!delayedResponse);
+            // Let the CPU continue.
+            translation->finish(fault, req, tc, mode);
+        } else {
+            // There was a fault during the walk. Let the CPU know.
+            translation->finish(timingFault, req, tc, mode);
         }
+        return true;
     }
+
     return false;
 }
 
index 81d70bd610ff8b8e6fea3e69a641ffa277cb224e..caafa3fe356c15faf5e7cc15f70920c7f8622994 100644 (file)
@@ -363,8 +363,6 @@ 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 ||
index a878b1540bd42eb063f8b17462069200ebc12b80..7c98b99fbab30ec7a05cf502c0d549b75cdb5420 100644 (file)
@@ -95,8 +95,6 @@ LSQUnit<Impl>::completeDataAccess(PacketPtr pkt)
 
     //iewStage->ldstQueue.removeMSHR(inst->threadNumber,inst->seqNum);
 
-    assert(!pkt->wasNacked());
-
     // If this is a split access, wait until all packets are received.
     if (TheISA::HasUnalignedMemAcc && !state->complete()) {
         delete pkt->req;
index 6a9fe7efce963d3593be3d7e2d902eb1719885be..9022845ce44c602e672ad5620615ad554a91dc19 100644 (file)
@@ -719,25 +719,14 @@ TimingSimpleCPU::IcachePort::ITickEvent::process()
 bool
 TimingSimpleCPU::IcachePort::recvTimingResp(PacketPtr pkt)
 {
-    if (!pkt->wasNacked()) {
-        DPRINTF(SimpleCPU, "Received timing response %#x\n", pkt->getAddr());
-        // delay processing of returned data until next CPU clock edge
-        Tick next_tick = cpu->nextCycle(curTick());
-
-        if (next_tick == curTick())
-            cpu->completeIfetch(pkt);
-        else
-            tickEvent.schedule(pkt, next_tick);
+    DPRINTF(SimpleCPU, "Received timing response %#x\n", pkt->getAddr());
+    // delay processing of returned data until next CPU clock edge
+    Tick next_tick = cpu->nextCycle();
 
-        return true;
-    } else {
-        assert(cpu->_status == IcacheWaitResponse);
-        pkt->reinitNacked();
-        if (!sendTimingReq(pkt)) {
-            cpu->_status = IcacheRetry;
-            cpu->ifetch_pkt = pkt;
-        }
-    }
+    if (next_tick == curTick())
+        cpu->completeIfetch(pkt);
+    else
+        tickEvent.schedule(pkt, next_tick);
 
     return true;
 }
@@ -839,32 +828,21 @@ TimingSimpleCPU::completeDrain()
 bool
 TimingSimpleCPU::DcachePort::recvTimingResp(PacketPtr pkt)
 {
-    if (!pkt->wasNacked()) {
-        // delay processing of returned data until next CPU clock edge
-        Tick next_tick = cpu->nextCycle(curTick());
+    // delay processing of returned data until next CPU clock edge
+    Tick next_tick = cpu->nextCycle();
 
-        if (next_tick == curTick()) {
-            cpu->completeDataAccess(pkt);
+    if (next_tick == curTick()) {
+        cpu->completeDataAccess(pkt);
+    } else {
+        if (!tickEvent.scheduled()) {
+            tickEvent.schedule(pkt, next_tick);
         } else {
-            if (!tickEvent.scheduled()) {
-                tickEvent.schedule(pkt, next_tick);
-            } else {
-                // In the case of a split transaction and a cache that is
-                // faster than a CPU we could get two responses before
-                // next_tick expires
-                if (!retryEvent.scheduled())
-                    cpu->schedule(retryEvent, next_tick);
-                return false;
-            }
-        }
-
-        return true;
-    } else  {
-        assert(cpu->_status == DcacheWaitResponse);
-        pkt->reinitNacked();
-        if (!sendTimingReq(pkt)) {
-            cpu->_status = DcacheRetry;
-            cpu->dcache_pkt = pkt;
+            // In the case of a split transaction and a cache that is
+            // faster than a CPU we could get two responses before
+            // next_tick expires
+            if (!retryEvent.scheduled())
+                cpu->schedule(retryEvent, next_tick);
+            return false;
         }
     }
 
index 60c21df914aa3b234c0fb2af80fd19670064837a..b1a4f69bcc9c17a01e8dbeec3a8e8b0f8c21443b 100644 (file)
@@ -46,11 +46,6 @@ class DmaDevice(PioDevice):
     type = 'DmaDevice'
     abstract = True
     dma = MasterPort("DMA port")
-    min_backoff_delay = Param.Latency('4ns',
-      "min time between a nack packet being received and the next request made by the device")
-    max_backoff_delay = Param.Latency('10us',
-      "max time between a nack packet being received and the next request made by the device")
-
 
 
 class IsaFake(BasicPioDevice):
index bb15abab601ac3913a7e893449d2c411bcf2e33c..77cc735a9549ca5ec454b7dbacf0c3fad128e09a 100644 (file)
@@ -78,8 +78,7 @@ CopyEngine::CopyEngine(const Params *p)
 
 
 CopyEngine::CopyEngineChannel::CopyEngineChannel(CopyEngine *_ce, int cid)
-    : cePort(_ce, _ce->sys, _ce->params()->min_backoff_delay,
-             _ce->params()->max_backoff_delay),
+    : cePort(_ce, _ce->sys),
       ce(_ce), channelId(cid), busy(false), underReset(false),
     refreshNext(false), latBeforeBegin(ce->params()->latBeforeBegin),
     latAfterCompletion(ce->params()->latAfterCompletion),
index 5a2f52df141e390c00910e30336700eb94e6d00c..80253129f368a212c7bd5e0d51a8ea64d6920676 100644 (file)
 #include "dev/dma_device.hh"
 #include "sim/system.hh"
 
-DmaPort::DmaPort(MemObject *dev, System *s, Tick min_backoff, Tick max_backoff)
+DmaPort::DmaPort(MemObject *dev, System *s)
     : MasterPort(dev->name() + ".dma", dev), device(dev), sys(s),
       masterId(s->getMasterId(dev->name())),
       pendingCount(0), drainEvent(NULL),
-      backoffTime(0), minBackoffDelay(min_backoff),
-      maxBackoffDelay(max_backoff), inRetry(false),
-      backoffEvent(this)
+      inRetry(false)
 { }
 
 bool
 DmaPort::recvTimingResp(PacketPtr pkt)
 {
-    if (pkt->wasNacked()) {
-        DPRINTF(DMA, "Received nacked %s addr %#x\n",
-                pkt->cmdString(), pkt->getAddr());
-
-        if (backoffTime < minBackoffDelay)
-            backoffTime = minBackoffDelay;
-        else if (backoffTime < maxBackoffDelay)
-            backoffTime <<= 1;
-
-        device->reschedule(backoffEvent, curTick() + backoffTime, true);
-
-        DPRINTF(DMA, "Backoff time set to %d ticks\n", backoffTime);
-
-        pkt->reinitNacked();
-        queueDma(pkt, true);
-    } else if (pkt->senderState) {
+    if (pkt->senderState) {
         DmaReqState *state;
-        backoffTime >>= 2;
 
         DPRINTF(DMA, "Received response %s addr %#x size %#x\n",
                 pkt->cmdString(), pkt->getAddr(), pkt->req->getSize());
@@ -116,8 +98,7 @@ DmaPort::recvTimingResp(PacketPtr pkt)
 }
 
 DmaDevice::DmaDevice(const Params *p)
-    : PioDevice(p), dmaPort(this, sys, params()->min_backoff_delay,
-                            params()->max_backoff_delay)
+    : PioDevice(p), dmaPort(this, sys)
 { }
 
 void
@@ -168,16 +149,10 @@ DmaPort::recvRetry()
             inRetry = true;
             DPRINTF(DMA, "-- Failed, queued\n");
         }
-    } while (!backoffTime &&  result && transmitList.size());
+    } while (result && transmitList.size());
 
-    if (transmitList.size() && backoffTime && !inRetry) {
-        DPRINTF(DMA, "Scheduling backoff for %d\n", curTick()+backoffTime);
-        if (!backoffEvent.scheduled())
-            device->schedule(backoffEvent, backoffTime + curTick());
-    }
-    DPRINTF(DMA, "TransmitList: %d, backoffTime: %d inRetry: %d es: %d\n",
-            transmitList.size(), backoffTime, inRetry,
-            backoffEvent.scheduled());
+    DPRINTF(DMA, "TransmitList: %d, inRetry: %d\n",
+            transmitList.size(), inRetry);
 }
 
 void
@@ -231,8 +206,8 @@ DmaPort::sendDma()
 
     Enums::MemoryMode state = sys->getMemoryMode();
     if (state == Enums::timing) {
-        if (backoffEvent.scheduled() || inRetry) {
-            DPRINTF(DMA, "Can't send immediately, waiting for retry or backoff timer\n");
+        if (inRetry) {
+            DPRINTF(DMA, "Can't send immediately, waiting for retry\n");
             return;
         }
 
@@ -249,14 +224,7 @@ DmaPort::sendDma()
                 inRetry = true;
                 DPRINTF(DMA, "-- Failed: queued\n");
             }
-        } while (result && !backoffTime && transmitList.size());
-
-        if (transmitList.size() && backoffTime && !inRetry &&
-                !backoffEvent.scheduled()) {
-            DPRINTF(DMA, "-- Scheduling backoff timer for %d\n",
-                    backoffTime+curTick());
-            device->schedule(backoffEvent, backoffTime + curTick());
-        }
+        } while (result && transmitList.size());
     } else if (state == Enums::atomic) {
         transmitList.pop_front();
 
index ccf388fa4f6f49f2d21a1c2ad053a680eec54e8e..691a2174987d56b802d80816fd03ac90a317927c 100644 (file)
@@ -87,16 +87,6 @@ class DmaPort : public MasterPort
      * here.*/
     Event *drainEvent;
 
-    /** time to wait between sending another packet, increases as NACKs are
-     * recived, decreases as responses are recived. */
-    Tick backoffTime;
-
-    /** Minimum time that device should back off for after failed sendTiming */
-    Tick minBackoffDelay;
-
-    /** Maximum time that device should back off for after failed sendTiming */
-    Tick maxBackoffDelay;
-
     /** If the port is currently waiting for a retry before it can send whatever
      * it is that it's sending. */
     bool inRetry;
@@ -108,11 +98,9 @@ class DmaPort : public MasterPort
     void queueDma(PacketPtr pkt, bool front = false);
     void sendDma();
 
-    /** event to give us a kick every time we backoff time is reached. */
-    EventWrapper<DmaPort, &DmaPort::sendDma> backoffEvent;
-
   public:
-    DmaPort(MemObject *dev, System *s, Tick min_backoff, Tick max_backoff);
+
+    DmaPort(MemObject *dev, System *s);
 
     void dmaAction(Packet::Command cmd, Addr addr, int size, Event *event,
                    uint8_t *data, Tick delay, Request::Flags flag = 0);
index 4d8adbd90484ce575018d9478df9d4cab07ec2a1..2fdbc5c1dbaf16de33bf2cecf676eaa63deb5275 100644 (file)
@@ -692,8 +692,6 @@ Cache<TagStore>::atomicAccess(PacketPtr pkt)
         DPRINTF(Cache, "Receive response: %s for addr %x in state %i\n",
                 bus_pkt->cmdString(), bus_pkt->getAddr(), old_state);
 
-        assert(!bus_pkt->wasNacked());
-
         // If packet was a forward, the response (if any) is already
         // in place in the bus_pkt == pkt structure, so we don't need
         // to do anything.  Otherwise, use the separate bus_pkt to
@@ -823,12 +821,6 @@ Cache<TagStore>::handleResponse(PacketPtr pkt)
 
     assert(mshr);
 
-    if (pkt->wasNacked()) {
-        //pkt->reinitFromRequest();
-        warn("NACKs from devices not connected to the same bus "
-             "not implemented\n");
-        return;
-    }
     if (is_error) {
         DPRINTF(Cache, "Cache received packet with error for address %x, "
                 "cmd: %s\n", pkt->getAddr(), pkt->cmdString());
@@ -1644,12 +1636,6 @@ template<class TagStore>
 bool
 Cache<TagStore>::MemSidePort::recvTimingResp(PacketPtr pkt)
 {
-    // this needs to be fixed so that the cache updates the mshr and sends the
-    // packet back out on the link, but it probably won't happen so until this
-    // gets fixed, just panic when it does
-    if (pkt->wasNacked())
-        panic("Need to implement cache resending nacked packets!\n");
-
     cache->handleResponse(pkt);
     return true;
 }
index 69cf36a5c63914dd6bb78db2c332a570d897dc2b..dc5ff4362d09e974e5eea00136a9852eee2b2468 100644 (file)
@@ -154,8 +154,6 @@ MemCmd::commandInfo[] =
         MessageResp, "MessageReq" },
     /* IntResp -- for interrupts */
     { SET2(IsWrite, IsResponse), InvalidCmd, "MessageResp" },
-    /* NetworkNackError  -- nacked at network layer (not by protocol) */
-    { SET2(IsResponse, IsError), InvalidCmd, "NetworkNackError" },
     /* InvalidDestError  -- packet dest field invalid */
     { SET2(IsResponse, IsError), InvalidCmd, "InvalidDestError" },
     /* BadAddressError   -- memory address invalid */
index cdcefcadbd4518f5e0921a4ab82ed9a80b0e8119..7396048d6241227bbf4c3fe064e71dc2057b281c 100644 (file)
@@ -120,7 +120,6 @@ class MemCmd
         // @TODO these should be classified as responses rather than
         // requests; coding them as requests initially for backwards
         // compatibility
-        NetworkNackError,  // nacked at network layer (not by protocol)
         InvalidDestError,  // packet dest field invalid
         BadAddressError,   // memory address invalid
         FunctionalReadError, // unable to fulfill functional read
@@ -465,13 +464,6 @@ class Packet : public Printable
     // Network error conditions... encapsulate them as methods since
     // their encoding keeps changing (from result field to command
     // field, etc.)
-    void
-    setNacked()
-    {
-        assert(isResponse());
-        cmd = MemCmd::NetworkNackError;
-    }
-
     void
     setBadAddress()
     {
@@ -479,7 +471,6 @@ class Packet : public Printable
         cmd = MemCmd::BadAddressError;
     }
 
-    bool wasNacked() const     { return cmd == MemCmd::NetworkNackError; }
     bool hadBadAddress() const { return cmd == MemCmd::BadAddressError; }
     void copyError(Packet *pkt) { assert(pkt->isError()); cmd = pkt->cmd; }
 
@@ -669,20 +660,6 @@ class Packet : public Printable
         }
     }
 
-    /**
-     * Take a request packet that has been returned as NACKED and
-     * modify it so that it can be sent out again. Only packets that
-     * need a response can be NACKED, so verify that that is true.
-     */
-    void
-    reinitNacked()
-    {
-        assert(wasNacked());
-        cmd = origCmd;
-        assert(needsResponse());
-        clearDest();
-    }
-
     void
     setSize(unsigned size)
     {