[Qemu-devel] [PATCH 0/5] stellaris_enet: overhaul TX handling

[Qemu-devel] [PATCH 0/5] stellaris_enet: overhaul TX handling

[Peter Maydell]

This patchset fixes the stellaris_enet TX handling code.
The principal motivation is to fix the buffer overrun noted
in the first patch, and to remove tx_fifo_len from the state
(which completely sidesteps the question of how to validate it
in incoming migration state :-)). I also fixed a couple of
other bugs I noticed while I was there.

This isn't actually sufficient to get my test image to work:
that needs proper implementation of the MII registers in the PHY.
I tested this with a minor hack to make all MII registers return
0x24, which happens to satisfy the test image's setup code.
However implementing the PHY registers is more work than I want
to do on this device right now...

Peter Maydell (5):
  hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer
    overrun
  hw/net/stellaris_enet: Correct handling of packet padding
  hw/net/stellaris_enet: Rewrite tx fifo handling code
  hw/net/stellaris_enet: Correctly implement the TR and THR registers
  hw/net/stellaris_enet: Fix debug format strings

 hw/net/stellaris_enet.c | 128 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 88 insertions(+), 40 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 1/5] hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer overrun

[Peter Maydell]

The current tx_fifo code has a corner case where the guest can overrun
the fifo buffer: if automatic CRCs are disabled we allow the guest to write
the CRC word even if there isn't actually space for it in the FIFO.
The datasheet is unclear about exactly how the hardware deals with this
situation; the most plausible answer seems to be that the CRC word is
just lost.

Implement this fix by separating the "can we stuff another word in the
FIFO" logic from the "should we transmit the packet now" check. This
also moves us closer to the real hardware, which has a number of ways
it can be configured to trigger sending the packet, some of which we
don't implement.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d04e6a4..bd844cd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -253,10 +253,12 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                 s->tx_fifo[s->tx_fifo_len++] = value >> 24;
             }
         } else {
-            s->tx_fifo[s->tx_fifo_len++] = value;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+                s->tx_fifo[s->tx_fifo_len++] = value;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            }
             if (s->tx_fifo_len >= s->tx_frame_len) {
                 /* We don't implement explicit CRC, so just chop it off.  */
                 if ((s->tctl & SE_TCTL_CRC) == 0)
-- 
1.9.0

Re: [Qemu-devel] [PATCH 1/5] hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer overrun

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The current tx_fifo code has a corner case where the guest can overrun
> the fifo buffer: if automatic CRCs are disabled we allow the guest to write
> the CRC word even if there isn't actually space for it in the FIFO.
> The datasheet is unclear about exactly how the hardware deals with this
> situation; the most plausible answer seems to be that the CRC word is
> just lost.
> 
> Implement this fix by separating the "can we stuff another word in the
> FIFO" logic from the "should we transmit the packet now" check. This
> also moves us closer to the real hardware, which has a number of ways
> it can be configured to trigger sending the packet, some of which we
> don't implement.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/net/stellaris_enet.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d04e6a4..bd844cd 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -253,10 +253,12 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>                  s->tx_fifo[s->tx_fifo_len++] = value >> 24;
>              }
>          } else {
> -            s->tx_fifo[s->tx_fifo_len++] = value;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> -            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> +            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> +                s->tx_fifo[s->tx_fifo_len++] = value;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> +                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> +            }
>              if (s->tx_fifo_len >= s->tx_frame_len) {
>                  /* We don't implement explicit CRC, so just chop it off.  */
>                  if ((s->tctl & SE_TCTL_CRC) == 0)
> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/5] hw/net/stellaris_enet: Correct handling of packet padding

[Peter Maydell]

The PADEN bit in the transmit control register enables padding of short
data packets out to the required minimum length. However a typo here
meant we were adjusting tx_fifo_len rather than tx_frame_len, so the
padding didn't actually happen. Fix this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index bd844cd..d0da819 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -265,7 +265,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                     s->tx_frame_len -= 4;
                 if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
                     memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_fifo_len = 60;
+                    s->tx_frame_len = 60;
                 }
                 qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
                                  s->tx_frame_len);
-- 
1.9.0

Re: [Qemu-devel] [PATCH 2/5] hw/net/stellaris_enet: Correct handling of packet padding

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The PADEN bit in the transmit control register enables padding of short
> data packets out to the required minimum length. However a typo here
> meant we were adjusting tx_fifo_len rather than tx_frame_len, so the
> padding didn't actually happen. Fix this bug.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/net/stellaris_enet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index bd844cd..d0da819 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -265,7 +265,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>                      s->tx_frame_len -= 4;
>                  if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
>                      memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
> -                    s->tx_fifo_len = 60;
> +                    s->tx_frame_len = 60;
>                  }
>                  qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
>                                   s->tx_frame_len);
> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Peter Maydell]

The datasheet is clear that the frame length written to the DATA
register is actually stored in the TX FIFO; this means we don't
need to keep both tx_frame_len and tx_fifo_len state separately.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 42 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d0da819..b99f93e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -59,7 +59,6 @@ typedef struct {
     uint32_t mtxd;
     uint32_t mrxd;
     uint32_t np;
-    int tx_frame_len;
     int tx_fifo_len;
     uint8_t tx_fifo[2048];
     /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
@@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
     qemu_set_irq(s->irq, (s->ris & s->im) != 0);
 }
 
+/* Return the data length of the packet currently being assembled
+ * in the TX fifo.
+ */
+static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
+{
+    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
+}
+
+/* Return true if the packet currently in the TX FIFO is complete,
+* ie the FIFO holds enough bytes for the data length, ethernet header,
+* payload and optionally CRC.
+*/
+static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+    framelen += 16;
+    if (!(s->tctl & SE_TCTL_CRC)) {
+        framelen += 4;
+    }
+    /* Cover the corner case of a 2032 byte payload with auto-CRC disabled:
+     * this requires more bytes than will fit in the FIFO. It's not totally
+     * clear how the h/w handles this, but if using threshold-based TX
+     * it will definitely try to transmit something.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo));
+    return s->tx_fifo_len >= framelen;
+}
+
+/* Send the packet currently in the TX FIFO */
+static void stellaris_enet_send(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+
+    /* Ethernet header is in the FIFO but not in the datacount.
+     * We don't implement explicit CRC, so just ignore any
+     * CRC value in the FIFO.
+     */
+    framelen += 14;
+    if ((s->tctl & SE_TCTL_PADEN) && framelen < 60) {
+        memset(&s->tx_fifo[framelen + 2], 0, 60 - framelen);
+        framelen = 60;
+    }
+    /* This MIN will have no effect unless the FIFO data is corrupt
+     * (eg bad data from an incoming migration); otherwise the check
+     * on the datalen at the start of writing the data into the FIFO
+     * will have caught this. Silently write a corrupt half-packet,
+     * which is what the hardware does in FIFO underrun situations.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo) - 2);
+    qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo + 2, framelen);
+    s->tx_fifo_len = 0;
+    s->ris |= SE_INT_TXEMP;
+    stellaris_enet_update(s);
+    DPRINTF("Done TX\n");
+}
+
 /* TODO: Implement MAC address filtering.  */
 static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
@@ -215,8 +270,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
-        if (value & SE_INT_TXER)
-            s->tx_frame_len = -1;
+        if (value & SE_INT_TXER) {
+            s->tx_fifo_len = 0;
+        }
         break;
     case 0x04: /* IM */
         DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
@@ -235,46 +291,27 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         s->tctl = value;
         break;
     case 0x10: /* DATA */
-        if (s->tx_frame_len == -1) {
-            s->tx_frame_len = value & 0xffff;
-            if (s->tx_frame_len > 2032) {
-                DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
-                s->tx_frame_len = 0;
+        if (s->tx_fifo_len == 0) {
+            /* The first word is special, it contains the data length */
+            int framelen = value & 0xffff;
+            if (framelen > 2032) {
+                DPRINTF("TX frame too long (%d)\n", framelen);
                 s->ris |= SE_INT_TXER;
                 stellaris_enet_update(s);
-            } else {
-                DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
-                /* The value written does not include the ethernet header.  */
-                s->tx_frame_len += 14;
-                if ((s->tctl & SE_TCTL_CRC) == 0)
-                    s->tx_frame_len += 4;
-                s->tx_fifo_len = 0;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-        } else {
-            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
-                s->tx_fifo[s->tx_fifo_len++] = value;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-            if (s->tx_fifo_len >= s->tx_frame_len) {
-                /* We don't implement explicit CRC, so just chop it off.  */
-                if ((s->tctl & SE_TCTL_CRC) == 0)
-                    s->tx_frame_len -= 4;
-                if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
-                    memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_frame_len = 60;
-                }
-                qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
-                                 s->tx_frame_len);
-                s->tx_frame_len = -1;
-                s->ris |= SE_INT_TXEMP;
-                stellaris_enet_update(s);
-                DPRINTF("Done TX\n");
+                break;
             }
         }
+
+        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+            s->tx_fifo[s->tx_fifo_len++] = value;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+        }
+
+        if (stellaris_txpacket_complete(s)) {
+            stellaris_enet_send(s);
+        }
         break;
     case 0x14: /* IA0 */
         s->conf.macaddr.a[0] = value;
@@ -326,7 +363,7 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
     s->im = SE_INT_PHY | SE_INT_MD | SE_INT_RXER | SE_INT_FOV | SE_INT_TXEMP
             | SE_INT_TXER | SE_INT_RX;
     s->thr = 0x3f;
-    s->tx_frame_len = -1;
+    s->tx_fifo_len = 0;
 }
 
 static void stellaris_enet_save(QEMUFile *f, void *opaque)
@@ -344,7 +381,6 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->mtxd);
     qemu_put_be32(f, s->mrxd);
     qemu_put_be32(f, s->np);
-    qemu_put_be32(f, s->tx_frame_len);
     qemu_put_be32(f, s->tx_fifo_len);
     qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < 31; i++) {
@@ -375,7 +411,6 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->mtxd = qemu_get_be32(f);
     s->mrxd = qemu_get_be32(f);
     s->np = qemu_get_be32(f);
-    s->tx_frame_len = qemu_get_be32(f);
     s->tx_fifo_len = qemu_get_be32(f);
     qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < 31; i++) {
-- 
1.9.0

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The datasheet is clear that the frame length written to the DATA
> register is actually stored in the TX FIFO; this means we don't
> need to keep both tx_frame_len and tx_fifo_len state separately.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 77 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index d0da819..b99f93e 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -59,7 +59,6 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_frame_len;
>      int tx_fifo_len;
>      uint8_t tx_fifo[2048];
>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
> @@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
>  }
>  
> +/* Return the data length of the packet currently being assembled
> + * in the TX fifo.
> + */
> +static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
> +{
> +    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
> +}
> +
> +/* Return true if the packet currently in the TX FIFO is complete,
> +* ie the FIFO holds enough bytes for the data length, ethernet header,
> +* payload and optionally CRC.
> +*/
> +static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
> +{
> +    int framelen = stellaris_txpacket_datalen(s);
> +    framelen += 16;

What's the magical 16? (It doesn't jump out from the data sheet).

> +    if (!(s->tctl & SE_TCTL_CRC)) {
> +        framelen += 4;
> +    }
> +    /* Cover the corner case of a 2032 byte payload with auto-CRC disabled:
> +     * this requires more bytes than will fit in the FIFO. It's not totally
> +     * clear how the h/w handles this, but if using threshold-based TX
> +     * it will definitely try to transmit something.
> +     */
> +    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo));
> +    return s->tx_fifo_len >= framelen;
> +}
> +
> +/* Send the packet currently in the TX FIFO */
> +static void stellaris_enet_send(stellaris_enet_state *s)
> +{
> +    int framelen = stellaris_txpacket_datalen(s);
> +
> +    /* Ethernet header is in the FIFO but not in the datacount.
> +     * We don't implement explicit CRC, so just ignore any
> +     * CRC value in the FIFO.
> +     */
> +    framelen += 14;
> +    if ((s->tctl & SE_TCTL_PADEN) && framelen < 60) {
> +        memset(&s->tx_fifo[framelen + 2], 0, 60 - framelen);
> +        framelen = 60;
> +    }
> +    /* This MIN will have no effect unless the FIFO data is corrupt
> +     * (eg bad data from an incoming migration); otherwise the check
> +     * on the datalen at the start of writing the data into the FIFO
> +     * will have caught this. Silently write a corrupt half-packet,
> +     * which is what the hardware does in FIFO underrun situations.
> +     */
> +    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo) - 2);
> +    qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo + 2, framelen);
> +    s->tx_fifo_len = 0;
> +    s->ris |= SE_INT_TXEMP;
> +    stellaris_enet_update(s);
> +    DPRINTF("Done TX\n");
> +}
> +
>  /* TODO: Implement MAC address filtering.  */
>  static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>  {
> @@ -215,8 +270,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>          DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
>          stellaris_enet_update(s);
>          /* Clearing TXER also resets the TX fifo.  */
> -        if (value & SE_INT_TXER)
> -            s->tx_frame_len = -1;
> +        if (value & SE_INT_TXER) {
> +            s->tx_fifo_len = 0;
> +        }
>          break;
>      case 0x04: /* IM */
>          DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
> @@ -235,46 +291,27 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>          s->tctl = value;
>          break;
>      case 0x10: /* DATA */
> -        if (s->tx_frame_len == -1) {
> -            s->tx_frame_len = value & 0xffff;
> -            if (s->tx_frame_len > 2032) {
> -                DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
> -                s->tx_frame_len = 0;
> +        if (s->tx_fifo_len == 0) {
> +            /* The first word is special, it contains the data length */
> +            int framelen = value & 0xffff;
> +            if (framelen > 2032) {
> +                DPRINTF("TX frame too long (%d)\n", framelen);
>                  s->ris |= SE_INT_TXER;
>                  stellaris_enet_update(s);
> -            } else {
> -                DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
> -                /* The value written does not include the ethernet header.  */
> -                s->tx_frame_len += 14;
> -                if ((s->tctl & SE_TCTL_CRC) == 0)
> -                    s->tx_frame_len += 4;
> -                s->tx_fifo_len = 0;
> -                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> -                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> -            }
> -        } else {
> -            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> -                s->tx_fifo[s->tx_fifo_len++] = value;
> -                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> -                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> -                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> -            }
> -            if (s->tx_fifo_len >= s->tx_frame_len) {
> -                /* We don't implement explicit CRC, so just chop it off.  */
> -                if ((s->tctl & SE_TCTL_CRC) == 0)
> -                    s->tx_frame_len -= 4;
> -                if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
> -                    memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
> -                    s->tx_frame_len = 60;
> -                }
> -                qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
> -                                 s->tx_frame_len);
> -                s->tx_frame_len = -1;
> -                s->ris |= SE_INT_TXEMP;
> -                stellaris_enet_update(s);
> -                DPRINTF("Done TX\n");
> +                break;
>              }
>          }
> +
> +        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
> +            s->tx_fifo[s->tx_fifo_len++] = value;
> +            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
> +            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
> +            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
> +        }
> +
> +        if (stellaris_txpacket_complete(s)) {
> +            stellaris_enet_send(s);
> +        }
>          break;
>      case 0x14: /* IA0 */
>          s->conf.macaddr.a[0] = value;
> @@ -326,7 +363,7 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
>      s->im = SE_INT_PHY | SE_INT_MD | SE_INT_RXER | SE_INT_FOV | SE_INT_TXEMP
>              | SE_INT_TXER | SE_INT_RX;
>      s->thr = 0x3f;
> -    s->tx_frame_len = -1;
> +    s->tx_fifo_len = 0;
>  }
>  
>  static void stellaris_enet_save(QEMUFile *f, void *opaque)
> @@ -344,7 +381,6 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, s->mtxd);
>      qemu_put_be32(f, s->mrxd);
>      qemu_put_be32(f, s->np);
> -    qemu_put_be32(f, s->tx_frame_len);
>      qemu_put_be32(f, s->tx_fifo_len);
>      qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
>      for (i = 0; i < 31; i++) {
> @@ -375,7 +411,6 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>      s->mtxd = qemu_get_be32(f);
>      s->mrxd = qemu_get_be32(f);
>      s->np = qemu_get_be32(f);
> -    s->tx_frame_len = qemu_get_be32(f);
>      s->tx_fifo_len = qemu_get_be32(f);
>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
>      for (i = 0; i < 31; i++) {

You should probably increment the migration state version number to 2.

Dave
> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Peter Maydell]

On 1 April 2014 18:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> The datasheet is clear that the frame length written to the DATA
>> register is actually stored in the TX FIFO; this means we don't
>> need to keep both tx_frame_len and tx_fifo_len state separately.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 77 insertions(+), 42 deletions(-)
>>
>> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
>> index d0da819..b99f93e 100644
>> --- a/hw/net/stellaris_enet.c
>> +++ b/hw/net/stellaris_enet.c
>> @@ -59,7 +59,6 @@ typedef struct {
>>      uint32_t mtxd;
>>      uint32_t mrxd;
>>      uint32_t np;
>> -    int tx_frame_len;
>>      int tx_fifo_len;
>>      uint8_t tx_fifo[2048];
>>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
>> @@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
>>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
>>  }
>>
>> +/* Return the data length of the packet currently being assembled
>> + * in the TX fifo.
>> + */
>> +static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
>> +{
>> +    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
>> +}
>> +
>> +/* Return true if the packet currently in the TX FIFO is complete,
>> +* ie the FIFO holds enough bytes for the data length, ethernet header,
>> +* payload and optionally CRC.
>> +*/
>> +static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
>> +{
>> +    int framelen = stellaris_txpacket_datalen(s);
>> +    framelen += 16;
>
> What's the magical 16? (It doesn't jump out from the data sheet).

See table 15-3: 2 bytes of data length, 6 bytes dest addr,
6 bytes source addr, 2 bytes len/type.

> You should probably increment the migration state version number to 2.

Oops, yes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 1 April 2014 18:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> The datasheet is clear that the frame length written to the DATA
> >> register is actually stored in the TX FIFO; this means we don't
> >> need to keep both tx_frame_len and tx_fifo_len state separately.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >>  hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 77 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> >> index d0da819..b99f93e 100644
> >> --- a/hw/net/stellaris_enet.c
> >> +++ b/hw/net/stellaris_enet.c
> >> @@ -59,7 +59,6 @@ typedef struct {
> >>      uint32_t mtxd;
> >>      uint32_t mrxd;
> >>      uint32_t np;
> >> -    int tx_frame_len;
> >>      int tx_fifo_len;
> >>      uint8_t tx_fifo[2048];
> >>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
> >> @@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
> >>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> >>  }
> >>
> >> +/* Return the data length of the packet currently being assembled
> >> + * in the TX fifo.
> >> + */
> >> +static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
> >> +{
> >> +    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
> >> +}
> >> +
> >> +/* Return true if the packet currently in the TX FIFO is complete,
> >> +* ie the FIFO holds enough bytes for the data length, ethernet header,
> >> +* payload and optionally CRC.
> >> +*/
> >> +static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
> >> +{
> >> +    int framelen = stellaris_txpacket_datalen(s);
> >> +    framelen += 16;
> >
> > What's the magical 16? (It doesn't jump out from the data sheet).
> 
> See table 15-3: 2 bytes of data length, 6 bytes dest addr,
> 6 bytes source addr, 2 bytes len/type.

Ah yes or the text in 15.3.1.2 para 4 'referes to the Etehernet frame data
payload, as shown in the 5th to nth FIFO positions' (1,2,3,4 each 4 bytes).

> > You should probably increment the migration state version number to 2.
> 
> Oops, yes.

Fix if you need to reroll, but not too important since i doubt people are
migrating it.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Dr. David Alan Gilbert]

(resend reply - the mail gru got some of the 1st one)

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 1 April 2014 18:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> The datasheet is clear that the frame length written to the DATA
> >> register is actually stored in the TX FIFO; this means we don't
> >> need to keep both tx_frame_len and tx_fifo_len state separately.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >>  hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 77 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> >> index d0da819..b99f93e 100644
> >> --- a/hw/net/stellaris_enet.c
> >> +++ b/hw/net/stellaris_enet.c
> >> @@ -59,7 +59,6 @@ typedef struct {
> >>      uint32_t mtxd;
> >>      uint32_t mrxd;
> >>      uint32_t np;
> >> -    int tx_frame_len;
> >>      int tx_fifo_len;
> >>      uint8_t tx_fifo[2048];
> >>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
> >> @@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
> >>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> >>  }
> >>
> >> +/* Return the data length of the packet currently being assembled
> >> + * in the TX fifo.
> >> + */
> >> +static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
> >> +{
> >> +    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
> >> +}
> >> +
> >> +/* Return true if the packet currently in the TX FIFO is complete,
> >> +* ie the FIFO holds enough bytes for the data length, ethernet header,
> >> +* payload and optionally CRC.
> >> +*/
> >> +static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
> >> +{
> >> +    int framelen = stellaris_txpacket_datalen(s);
> >> +    framelen += 16;
> >
> > What's the magical 16? (It doesn't jump out from the data sheet).
> 
> See table 15-3: 2 bytes of data length, 6 bytes dest addr,
> 6 bytes source addr, 2 bytes len/type.

Ah yes or the text in 15.3.1.2 para 4 'referes to the Etehernet frame data
payload, as shown in the 5th to nth FIFO positions' (1,2,3,4 each 4 bytes).

> > You should probably increment the migration state version number to 2.
>
> Oops, yes.

Fix if you need to reroll, but not too important since i doubt people are
migrating it.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Peter Maydell]

On 1 April 2014 18:45, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> (resend reply - the mail gru got some of the 1st one)
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> > You should probably increment the migration state version number to 2.
>>
>> Oops, yes.
>
> Fix if you need to reroll, but not too important since i doubt people are
> migrating it.

I'm tempted to convert it to vmstate, actually.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 1 April 2014 18:45, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > (resend reply - the mail gru got some of the 1st one)
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> > You should probably increment the migration state version number to 2.
> >>
> >> Oops, yes.
> >
> > Fix if you need to reroll, but not too important since i doubt people are
> > migrating it.
> 
> I'm tempted to convert it to vmstate, actually.

That would be a wonderful thing to do.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 4/5] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Peter Maydell]

Packet transmission for the stellaris ethernet controller can be triggered
in one of two ways:
 * by setting a threshold value in the THR register; when the FIFO
   fill level reaches the threshold, the h/w starts transmitting.
   Software has to finish filling the FIFO before the transmit
   process completes to avoid a (silent) underrun
 * by software writing to the TR register to explicitly trigger
   transmission

Since QEMU transmits packets instantaneously (from the guest's
point of view), implement "transmit based on threshold" with
our existing mechanism of "transmit as soon as we have the whole
packet", with the additional wrinkle that we don't transmit if
the packet size is below the specified threshold, and implement
"transmit by specific request" properly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index b99f93e..0210108 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -109,6 +109,15 @@ static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
     return s->tx_fifo_len >= framelen;
 }
 
+/* Return true if the TX FIFO threshold is enabled and the FIFO
+ * has filled enough to reach it.
+ */
+static inline bool stellaris_tx_thr_reached(stellaris_enet_state *s)
+{
+    return (s->thr < 0x3f &&
+            (s->tx_fifo_len >= 4 * (s->thr * 8 + 1)));
+}
+
 /* Send the packet currently in the TX FIFO */
 static void stellaris_enet_send(stellaris_enet_state *s)
 {
@@ -309,7 +318,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
             s->tx_fifo[s->tx_fifo_len++] = value >> 24;
         }
 
-        if (stellaris_txpacket_complete(s)) {
+        if (stellaris_tx_thr_reached(s) && stellaris_txpacket_complete(s)) {
             stellaris_enet_send(s);
         }
         break;
@@ -338,9 +347,11 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     case 0x2c: /* MTXD */
         s->mtxd = value & 0xff;
         break;
+    case 0x38: /* TR */
+        stellaris_enet_send(s);
+        break;
     case 0x30: /* MRXD */
     case 0x34: /* NP */
-    case 0x38: /* TR */
         /* Ignored.  */
     case 0x3c: /* Undocuented: Timestamp? */
         /* Ignored.  */
-- 
1.9.0

Re: [Qemu-devel] [PATCH 4/5] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:

<snip>

> @@ -338,9 +347,11 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>      case 0x2c: /* MTXD */
>          s->mtxd = value & 0xff;
>          break;
> +    case 0x38: /* TR */
> +        stellaris_enet_send(s);
> +        break;

Shouldn't that be something like:

   if (value & 1) {
       stellaris_enet_send(s);
   }

Dave
>      case 0x30: /* MRXD */
>      case 0x34: /* NP */
> -    case 0x38: /* TR */
>          /* Ignored.  */
>      case 0x3c: /* Undocuented: Timestamp? */
>          /* Ignored.  */
> -- 
> 1.9.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/5] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Peter Maydell]

On 1 April 2014 18:44, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>
> <snip>
>
>> @@ -338,9 +347,11 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>>      case 0x2c: /* MTXD */
>>          s->mtxd = value & 0xff;
>>          break;
>> +    case 0x38: /* TR */
>> +        stellaris_enet_send(s);
>> +        break;
>
> Shouldn't that be something like:
>
>    if (value & 1) {
>        stellaris_enet_send(s);
>    }

Yep.

-- PMM

[Qemu-devel] [PATCH 5/5] hw/net/stellaris_enet: Fix debug format strings

[Peter Maydell]

Fix various debug format strings which were incorrect for the
data type, so that building with debug enabled is possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 0210108..db20b05 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -161,7 +161,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
         return -1;
     }
 
-    DPRINTF("Received packet len=%d\n", size);
+    DPRINTF("Received packet len=%zu\n", size);
     n = s->next_packet + s->np;
     if (n >= 31)
         n -= 31;
@@ -276,7 +276,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     switch (offset) {
     case 0x00: /* IACK */
         s->ris &= ~value;
-        DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ ack %02" PRIx64 "/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
         if (value & SE_INT_TXER) {
@@ -284,7 +284,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         }
         break;
     case 0x04: /* IM */
-        DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ mask %02" PRIx64 "/%02x\n", value, s->ris);
         s->im = value;
         stellaris_enet_update(s);
         break;
-- 
1.9.0

Re: [Qemu-devel] [PATCH 5/5] hw/net/stellaris_enet: Fix debug format strings

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Fix various debug format strings which were incorrect for the
> data type, so that building with debug enabled is possible.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/net/stellaris_enet.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 0210108..db20b05 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -161,7 +161,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
>          return -1;
>      }
>  
> -    DPRINTF("Received packet len=%d\n", size);
> +    DPRINTF("Received packet len=%zu\n", size);
>      n = s->next_packet + s->np;
>      if (n >= 31)
>          n -= 31;
> @@ -276,7 +276,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>      switch (offset) {
>      case 0x00: /* IACK */
>          s->ris &= ~value;
> -        DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
> +        DPRINTF("IRQ ack %02" PRIx64 "/%02x\n", value, s->ris);
>          stellaris_enet_update(s);
>          /* Clearing TXER also resets the TX fifo.  */
>          if (value & SE_INT_TXER) {
> @@ -284,7 +284,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>          }
>          break;
>      case 0x04: /* IM */
> -        DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
> +        DPRINTF("IRQ mask %02" PRIx64 "/%02x\n", value, s->ris);
>          s->im = value;
>          stellaris_enet_update(s);
>          break;
> -- 
> 1.9.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/5] stellaris_enet: overhaul TX handling

[Peter Maydell]

On 1 April 2014 17:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes the stellaris_enet TX handling code.
> The principal motivation is to fix the buffer overrun noted
> in the first patch, and to remove tx_fifo_len from the state
> (which completely sidesteps the question of how to validate it
> in incoming migration state :-)). I also fixed a couple of
> other bugs I noticed while I was there.
>
> This isn't actually sufficient to get my test image to work:
> that needs proper implementation of the MII registers in the PHY.
> I tested this with a minor hack to make all MII registers return
> 0x24, which happens to satisfy the test image's setup code.
> However implementing the PHY registers is more work than I want
> to do on this device right now...
>
> Peter Maydell (5):
>   hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer
>     overrun
>   hw/net/stellaris_enet: Correct handling of packet padding
>   hw/net/stellaris_enet: Rewrite tx fifo handling code
>   hw/net/stellaris_enet: Correctly implement the TR and THR registers
>   hw/net/stellaris_enet: Fix debug format strings

Should probably have CC:d stable on patches 1 and 2 at least,
though I'm sceptical that anybody's actually using this
device, given the missing PHY registers.

thanks
-- PMM