dev: Avoid arbitrarily deep stack depth in the i8254xGBe model.

When it comes time to send a packet with the i8254xGBe model hooked up to
EtherTap and while running in KVM mode, the packet will first go to the
EtherTap over the network style port between them. EtherTap, because it's
not actually a model of anything in the simulation, will immediately pass
the packet off to the real network and report that the transmission was
successful to the i8254xGBe. The i8254xGBe will notice that it still has
stuff it can send (the KVM mode CPU has no trouble keeping it full) and
will, without returning and collapsing the stack, immediately call back
into EtherTap with the next packet. This loop repeats, continually
deepening the stack, until gem5 crashes with a segfault.

To break this loop, a few small changes have been made. First, txFifoTick
has been repurposed slightly so that it continuously keeps track of
whether there's still work to do to flush out the fifo during the current
tick. The code in txWire has been adjusted slightly so that it clears that
variable at the start (also removing some redundancy), so that other code
can set it again if more work needs to be done. Specifically, the
ethTxDone function will set that flag.

If there's more work to be done flushing the Fifo while in tick(), it
will loop until txFifoTick stays cleared, meaning either the Fifo is
empty, or the object on the other end hasn't said it's done yet.

Finally, a new bool member called inTick has been added which keeps track
of whether the tick() function is still active somewhere in the callstack.
If it is, then the tick event shouldn't be rescheduled in ethTxDone, since
tick will take care of that before it returns. It won't check to see if it
needs to, and so without this check there's a panic from scheduling the
same event twice.

It's not completely clear that the Fifo should send packets over and over
as fast as the other side accepts them within the same tick, although it's
not clear that it shouldn't either. If not, then probably all that would
need to change would be to remove the "while" loop so that the tick event
would be rescheduled, and the Fifo would be further emptied the next time
around.

Change-Id: I653379b43389d0539ecfadb3fc6c40e38a8864c2
Reviewed-on: https://gem5-review.googlesource.com/3642
Reviewed-by: Andreas Hansson <andreas.hansson@arm.com>
Maintainer: Gabe Black <gabeblack@google.com>

2 files changed, 19 insertions, 12 deletions

diff --git a/src/dev/net/i8254xGBe.cc b/src/dev/net/i8254xGBe.cc
index 70909549a..74379592a 100644
--- a/src/dev/net/i8254xGBe.cc
+++ b/src/dev/net/i8254xGBe.cc
@@ -59,9 +59,9 @@ using namespace Net;
 
 IGbE::IGbE(const Params *p)
     : EtherDevice(p), etherInt(NULL), cpa(NULL),
-      rxFifo(p->rx_fifo_size), txFifo(p->tx_fifo_size), rxTick(false),
-      txTick(false), txFifoTick(false), rxDmaPacket(false), pktOffset(0),
-      fetchDelay(p->fetch_delay), wbDelay(p->wb_delay),
+      rxFifo(p->rx_fifo_size), txFifo(p->tx_fifo_size), inTick(false),
+      rxTick(false), txTick(false), txFifoTick(false), rxDmaPacket(false),
+      pktOffset(0), fetchDelay(p->fetch_delay), wbDelay(p->wb_delay),
       fetchCompDelay(p->fetch_comp_delay), wbCompDelay(p->wb_comp_delay),
       rxWriteDelay(p->rx_write_delay), txReadDelay(p->tx_read_delay),
       rdtrEvent(this), radvEvent(this),
@@ -2385,9 +2385,10 @@ IGbE::rxStateMachine()
 void
 IGbE::txWire()
 {
+    txFifoTick = false;
+
     if (txFifo.empty()) {
         anWe("TXQ", "TX FIFO Q");
-        txFifoTick = false;
         return;
     }
 
@@ -2411,12 +2412,8 @@ IGbE::txWire()
 
         txBytes += txFifo.front()->length;
         txPackets++;
-        txFifoTick = false;
 
         txFifo.pop();
-    } else {
-        // We'll get woken up when the packet ethTxDone() gets called
-        txFifoTick = false;
     }
 }
 
@@ -2425,18 +2422,27 @@ IGbE::tick()
 {
     DPRINTF(EthernetSM, "IGbE: -------------- Cycle --------------\n");
 
+    inTick = true;
+
     if (rxTick)
         rxStateMachine();
 
     if (txTick)
         txStateMachine();
 
-    if (txFifoTick)
+    // If txWire returns and txFifoTick is still set, that means the data we
+    // sent to the other end was already accepted and we can send another
+    // frame right away. This is consistent with the previous behavior which
+    // would send another frame if one was ready in ethTxDone. This version
+    // avoids growing the stack with each frame sent which can cause stack
+    // overflow.
+    while (txFifoTick)
         txWire();
 
-
     if (rxTick || txTick || txFifoTick)
         schedule(tickEvent, curTick() + clockPeriod());
+
+    inTick = false;
 }
 
 void
@@ -2450,8 +2456,8 @@ IGbE::ethTxDone()
     if (txDescCache.descLeft() != 0 && drainState() != DrainState::Draining)
         txTick = true;
 
-    restartClock();
-    txWire();
+    if (!inTick)
+        restartClock();
     DPRINTF(EthernetSM, "TxFIFO: Transmission complete\n");
 }
 
diff --git a/src/dev/net/i8254xGBe.hh b/src/dev/net/i8254xGBe.hh
index ed0bf4713..80e2104a0 100644
--- a/src/dev/net/i8254xGBe.hh
+++ b/src/dev/net/i8254xGBe.hh
@@ -75,6 +75,7 @@ class IGbE : public EtherDevice
     EthPacketPtr txPacket;
 
     // Should to Rx/Tx State machine tick?
+    bool inTick;
     bool rxTick;
     bool txTick;
     bool txFifoTick;