Skip to contentSkip to conversation

e1000: multi-buffer packet support

PATCH In review Michael S. Tsirkin 5 participants 8 messages Feb 15, 2011 – Feb 20, 2011

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

v2 cover e1000: multi-buffer packet support

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>

Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=602205

Changes from v1:
fix buffer overflow reported by Kevin
added a patch to fix EOP spec violation reported by Juan added a patch to fix spec violation noted by myself

Michael S. Tsirkin (3):
e1000: multi-buffer packet support
e1000: clear EOP for multi-buffer descriptors
e1000: verify we have buffers, upfront

 hw/e1000.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 48 insertions(+), 13 deletions(-)
Signature · 1 line
1.7.3.2.91.g446ac
author
#1 lore raw

PATCH v2 1/3 e1000: multi-buffer packet support

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>
Signed-off-by Michael S. Tsirkin <[email protected]>
Reviewed-by Stefan Hajnoczi <[email protected]>
---
 hw/e1000.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)
hw/e1000.c +28 -11
index af101bd..050ce02 100644
@@ -642,6 +642,9 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
     uint16_t vlan_special = 0;
     uint8_t vlan_status = 0, vlan_offset = 0;
     uint8_t min_buf[MIN_BUF_SIZE];
+    size_t desc_offset;
+    size_t desc_size;
+    size_t total_size;
 
     if (!(s->mac_reg[RCTL] & E1000_RCTL_EN))
         return -1;

@@ -654,12 +657,6 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
         size = sizeof(min_buf);
     }
 
-    if (size > s->rxbuf_size) {
-        DBGOUT(RX, "packet too large for buffers (%lu > %d)\n",
-               (unsigned long)size, s->rxbuf_size);
-        return -1;
-    }
-
     if (!receive_filter(s, buf, size))
         return size;
 

@@ -672,8 +669,16 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
     }
 
     rdh_start = s->mac_reg[RDH];
+    desc_offset = 0;
+    total_size = size + fcs_len(s);
     do {
+        desc_size = total_size - desc_offset;
+        if (desc_size > s->rxbuf_size) {
+            desc_size = s->rxbuf_size;
+        }
         if (s->mac_reg[RDH] == s->mac_reg[RDT] && s->check_rxov) {
+            /* Discard all data written so far */
+            s->mac_reg[RDH] = rdh_start;
             set_ics(s, 0, E1000_ICS_RXO);
             return -1;
         }

@@ -683,10 +688,22 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
         desc.special = vlan_special;
         desc.status |= (vlan_status | E1000_RXD_STAT_DD);
         if (desc.buffer_addr) {
-            cpu_physical_memory_write(le64_to_cpu(desc.buffer_addr),
-                                      (void *)(buf + vlan_offset), size);
-            desc.length = cpu_to_le16(size + fcs_len(s));
-            desc.status |= E1000_RXD_STAT_EOP|E1000_RXD_STAT_IXSM;
+            if (desc_offset < size) {
+                size_t copy_size = size - desc_offset;
+                if (copy_size > s->rxbuf_size) {
+                    copy_size = s->rxbuf_size;
+                }
+                cpu_physical_memory_write(le64_to_cpu(desc.buffer_addr),
+                                          (void *)(buf + desc_offset + vlan_offset),
+                                          copy_size);
+            }
+            desc_offset += desc_size;
+            if (desc_offset >= total_size) {
+                desc.length = cpu_to_le16(desc_size);
+                desc.status |= E1000_RXD_STAT_EOP | E1000_RXD_STAT_IXSM;
+            } else {
+                desc.length = cpu_to_le16(desc_size);
+            }
         } else { // as per intel docs; skip descriptors with null buf addr
             DBGOUT(RX, "Null RX descriptor!!\n");
         }

@@ -702,7 +719,7 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
             set_ics(s, 0, E1000_ICS_RXO);
             return -1;
         }
-    } while (desc.buffer_addr == 0);
+    } while (desc_offset < total_size);
 
     s->mac_reg[GPRC]++;
     s->mac_reg[TPR]++;
Signature · 1 line
1.7.3.2.91.g446ac
author
#2 lore raw

PATCH v2 2/3 e1000: clear EOP for multi-buffer descriptors

In reply to Michael S. Tsirkin

The e1000 spec says: if software statically allocates buffers, and uses memory read to check for completed descriptors, it simply has to zero the status byte in the descriptor to make it ready for reuse by hardware. This is not a hardware requirement (moving the hardware tail pointer is), but is necessary for performing an in–memory scan.

Thus the guest does not have to clear the status byte. In case it doesn't we need to clear EOP for all descriptors
except the last. While I don't know of any such guests, it's probably a good idea to stick to the spec.

Signed-off-by Michael S. Tsirkin <[email protected]>
Reported-by Juan Quintela <[email protected]>
---
 hw/e1000.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
hw/e1000.c +4 -2
index 050ce02..2943a1a 100644
@@ -698,11 +698,13 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
                                           copy_size);
             }
             desc_offset += desc_size;
+            desc.length = cpu_to_le16(desc_size);
             if (desc_offset >= total_size) {
-                desc.length = cpu_to_le16(desc_size);
                 desc.status |= E1000_RXD_STAT_EOP | E1000_RXD_STAT_IXSM;
             } else {
-                desc.length = cpu_to_le16(desc_size);
+                /* Guest zeroing out status is not a hardware requirement.
+                   Clear EOP in case guest didn't do it. */
+                desc.status &= ~E1000_RXD_STAT_EOP;
             }
         } else { // as per intel docs; skip descriptors with null buf addr
             DBGOUT(RX, "Null RX descriptor!!\n");
Signature · 1 line
1.7.3.2.91.g446ac
author
#3 lore raw

PATCH v2 3/3 e1000: verify we have buffers, upfront

In reply to Michael S. Tsirkin

The spec says: Any descriptor with a non-zero status byte has been processed by the hardware, and is ready to be handled by the software.

Thus, once we change a descriptor status to non-zero we should never move the head backwards and try to reuse this descriptor from hardware.

This actually happened with a multibuffer packet
that arrives when we don't have enough buffers.

Fix by checking that we have enough buffers upfront so we never need to discard the packet midway through.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/e1000.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)
hw/e1000.c +22 -6
index 2943a1a..0a4574c 100644
@@ -631,6 +631,24 @@ e1000_can_receive(VLANClientState *nc)
     return (s->mac_reg[RCTL] & E1000_RCTL_EN);
 }
 
+static bool e1000_has_rxbufs(E1000State *s, size_t total_size)
+{
+    int bufs;
+    /* Fast-path short packets */
+    if (total_size <= s->rxbuf_size) {
+        return s->mac_reg[RDH] != s->mac_reg[RDT] || !s->check_rxov;
+    }
+    if (s->mac_reg[RDH] < s->mac_reg[RDT]) {
+        bufs = s->mac_reg[RDT] - s->mac_reg[RDH];
+    } else if (s->mac_reg[RDH] > s->mac_reg[RDT] || !s->check_rxov) {
+        bufs = s->mac_reg[RDLEN] /  sizeof(struct e1000_rx_desc) +
+            s->mac_reg[RDT] - s->mac_reg[RDH];
+    } else {
+        return false;
+    }
+    return total_size <= bufs * s->rxbuf_size;
+}
+
 static ssize_t
 e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
 {

@@ -671,17 +689,15 @@ e1000_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
     rdh_start = s->mac_reg[RDH];
     desc_offset = 0;
     total_size = size + fcs_len(s);
+    if (!e1000_has_rxbufs(s, total_size)) {
+            set_ics(s, 0, E1000_ICS_RXO);
+            return -1;
+    }
     do {
         desc_size = total_size - desc_offset;
         if (desc_size > s->rxbuf_size) {
             desc_size = s->rxbuf_size;
         }
-        if (s->mac_reg[RDH] == s->mac_reg[RDT] && s->check_rxov) {
-            /* Discard all data written so far */
-            s->mac_reg[RDH] = rdh_start;
-            set_ics(s, 0, E1000_ICS_RXO);
-            return -1;
-        }
         base = ((uint64_t)s->mac_reg[RDBAH] << 32) + s->mac_reg[RDBAL] +
                sizeof(desc) * s->mac_reg[RDH];
         cpu_physical_memory_read(base, (void *)&desc, sizeof(desc));
Signature · 1 line
1.7.3.2.91.g446ac
#4 lore raw
In reply to Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>

Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=602205

Changes from v1:
fix buffer overflow reported by Kevin
added a patch to fix EOP spec violation reported by Juan added a patch to fix spec violation noted by myself

Quoted 7 lines

Michael S. Tsirkin (3):
e1000: multi-buffer packet support
e1000: clear EOP for multi-buffer descriptors
e1000: verify we have buffers, upfront

 hw/e1000.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 48 insertions(+), 13 deletions(-)

Took a quick look, looks good.

Stefan

Acked-by on the v2 series
#5 lore raw
In reply to Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>

Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=602205

Changes from v1:
fix buffer overflow reported by Kevin
added a patch to fix EOP spec violation reported by Juan added a patch to fix spec violation noted by myself

Quoted 8 lines

Michael S. Tsirkin (3):
e1000: multi-buffer packet support
e1000: clear EOP for multi-buffer descriptors
e1000: verify we have buffers, upfront

 hw/e1000.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 48 insertions(+), 13 deletions(-)
Acked-by Alex Williamson <[email protected]>
Acked-by on the v2 series
#6 lore raw
In reply to Michael S. Tsirkin

Am 15.02.2011 17:27, schrieb Michael S. Tsirkin:

Quoted 15 lines

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>

Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=602205

Changes from v1:
fix buffer overflow reported by Kevin
added a patch to fix EOP spec violation reported by Juan added a patch to fix spec violation noted by myself

Quoted 7 lines

Michael S. Tsirkin (3):
e1000: multi-buffer packet support
e1000: clear EOP for multi-buffer descriptors
e1000: verify we have buffers, upfront

 hw/e1000.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 48 insertions(+), 13 deletions(-)

I found patch 3 to be not that easy to understand (especially the s->check_rxov part), but after thinking a while about it, it seems to make sense.

Acked-by Kevin Wolf <[email protected]>
#7 lore raw
In reply to Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin

e1000 supports multi-buffer packets larger than rxbuf_size.

This fixes the following (on linux):
- in guest: ifconfig eth1 mtu 16110
- in host: ifconfig tap0 mtu 16110

           ping -s 16082 <guest-ip>

Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=602205

Changes from v1:
fix buffer overflow reported by Kevin
added a patch to fix EOP spec violation reported by Juan added a patch to fix spec violation noted by myself

Quoted 11 lines

Michael S. Tsirkin (3):
e1000: multi-buffer packet support
e1000: clear EOP for multi-buffer descriptors
e1000: verify we have buffers, upfront

 hw/e1000.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 48 insertions(+), 13 deletions(-)
Signature · 1 line
1.7.3.2.91.g446ac

Thanks, all applied.

Signature · 2 lines
Aurelien Jarno                          GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net