[Qemu-devel] [PATCHv2 0/3] e1000: multi-buffer packet support

[Qemu-devel] [PATCHv2 0/3] e1000: multi-buffer packet support

[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


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(-)

-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 1/3] e1000: multi-buffer packet support

[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>

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/e1000.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index af101bd..050ce02 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -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]++;
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 2/3] e1000: clear EOP for multi-buffer descriptors

[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 <mst@redhat.com>
Reported-by: Juan Quintela <quintela@redhat.com>
---
 hw/e1000.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 050ce02..2943a1a 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -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");
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 3/3] e1000: verify we have buffers, upfront

[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 <mst@redhat.com>
---
 hw/e1000.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 2943a1a..0a4574c 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -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));
-- 
1.7.3.2.91.g446ac

[Qemu-devel] Re: [PATCHv2 0/3] e1000: multi-buffer packet support

[Stefan Hajnoczi]

On Tue, Feb 15, 2011 at 06:27:44PM +0200, Michael S. Tsirkin wrote:
> 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(-)

Took a quick look, looks good.

Stefan

Re: [Qemu-devel] [PATCHv2 0/3] e1000: multi-buffer packet support

[Alex Williamson]

On Tue, 2011-02-15 at 18:27 +0200, Michael S. Tsirkin wrote:
> 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(-)
> 

Acked-by: Alex Williamson <alex.williamson@redhat.com>

[Qemu-devel] Re: [PATCHv2 0/3] e1000: multi-buffer packet support

[Kevin Wolf]

Am 15.02.2011 17:27, schrieb 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
> 
> 
> 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 <kwolf@redhat.com>

Re: [Qemu-devel] [PATCHv2 0/3] e1000: multi-buffer packet support

[Aurelien Jarno]

On Tue, Feb 15, 2011 at 06:27:44PM +0200, Michael S. Tsirkin wrote:
> 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(-)
> 
> -- 
> 1.7.3.2.91.g446ac
> 

Thanks, all applied.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net