virtio-net: iovec handling cleanup

PATCH New Michael S. Tsirkin 1 participant 15 messages Sep 25, 2012

Share & tools

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #0 lore raw

v2 cover virtio-net: iovec handling cleanup

virtio net is using lots of manual mangling of iovecs;
I don't see any obvious crashes but it does look scary since guest can manipulate iovecs. This patchset changes it to use iov_* functions which are safer.

On a recent linux kernel, new code will actually do less iovec processing because it is able to utilize support for mergeable buffer header size added in tun/tap in linux 2.6.35 - though I was unable to measure any speed difference.

This also removes the restriction on buffer layout, aligning qemu with recent versions of virtio spec which imply that device should be prepared to handle e.g. virtio net header inline with the packet data in the same buffer.

Tsted on x86 kvm only for now, would appreciate cross-endianness etc testing/reports. This is also why I split this to so many small patches to make bisect easier in case of problems.

Changes from v1: address Anthony's comments
patches 3,5,8: rename iov_cpy -> iov_copy
patches 3,4 move hunk that landed in a wrong patch by mistake patch 4: move evil cast around so it's localized better

Michael S. Tsirkin (14):
virtio-net: track host/guest header length
iov: add const annotation
iov: add iov_cpy
virtio-net: avoid sg copy
virtio-net: use safe iov operations for rx
virtio-net: refactor receive_hdr
virtio-net: first s/g is always at start of buf
virtio-net: switch tx to safe iov functions
virtio-net: simplify rx code
virtio: don't mark unaccessed memory as dirty
virtio-net: fix used len for tx
virtio-net: minor code simplification
virtio-net: test peer header support at init time
virtio-net: enable mrg buf header in tap on linux

 hw/vhost_net.c  |  13 ----
 hw/virtio-net.c | 180 +++++++++++++++++++++++++++++++-------------------------
 hw/virtio.c     |   2 +-
 iov.c           |  25 +++++++-
 iov.h           |  11 +++-
 5 files changed, 134 insertions(+), 97 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:10 UTC #1 lore raw

PATCH v2 1/14 virtio-net: track host/guest header length

Tracking these in device state instead of
re-calculating on each packet. No functional
changes.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

hw/virtio-net.c +17 ~~-12~~

index 6490743..0c5081e 100644
@@ -41,6 +41,8 @@ typedef struct VirtIONet
     int32_t tx_burst;
     int tx_waiting;
     uint32_t has_vnet_hdr;
+    size_t host_hdr_len;
+    size_t guest_hdr_len;
     uint8_t has_ufo;
     struct {
         VirtQueueElement elem;

@@ -231,6 +233,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 
     if (peer_has_vnet_hdr(n)) {
         tap_using_vnet_hdr(n->nic->nc.peer, 1);
+        n->host_hdr_len = sizeof(struct virtio_net_hdr);
     } else {
         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
         features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);

@@ -278,6 +281,8 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
     VirtIONet *n = to_virtio_net(vdev);
 
     n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
+    n->guest_hdr_len = n->mergeable_rx_bufs ?
+        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
 
     if (n->has_vnet_hdr) {
         tap_set_offload(n->nic->nc.peer,

@@ -593,18 +598,13 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t guest_hdr_len, offset, i, host_hdr_len;
+    size_t offset, i;
 
     if (!virtio_net_can_receive(&n->nic->nc))
         return -1;
 
     /* hdr_len refers to the header we supply to the guest */
-    guest_hdr_len = n->mergeable_rx_bufs ?
-        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
-
-
-    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
-    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
+    if (!virtio_net_has_buffers(n, size + n->guest_hdr_len - n->host_hdr_len))
         return 0;
 
     if (!receive_filter(n, buf, size))

@@ -626,7 +626,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
                     "i %zd mergeable %d offset %zd, size %zd, "
                     "guest hdr len %zd, host hdr len %zd guest features 0x%x",
                     i, n->mergeable_rx_bufs, offset, size,
-                    guest_hdr_len, host_hdr_len, n->vdev.guest_features);
+                    n->guest_hdr_len, n->host_hdr_len, n->vdev.guest_features);
             exit(1);
         }

@@ -635,7 +635,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
             exit(1);
         }
 
-        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
+        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != n->guest_hdr_len) {
             error_report("virtio-net header not in first element");
             exit(1);
         }

@@ -647,8 +647,9 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
                 mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
 
             offset += receive_header(n, sg, elem.in_num,
-                                     buf + offset, size - offset, guest_hdr_len);
-            total += guest_hdr_len;
+                                     buf + offset, size - offset,
+                                     n->guest_hdr_len);
+            total += n->guest_hdr_len;
         }
 
         /* copy in packet.  ugh */

@@ -665,7 +666,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
                          "i %zd mergeable %d offset %zd, size %zd, "
                          "guest hdr len %zd, host hdr len %zd",
                          i, n->mergeable_rx_bufs,
-                         offset, size, guest_hdr_len, host_hdr_len);
+                         offset, size, n->guest_hdr_len, n->host_hdr_len);
 #endif
             return size;
         }

@@ -900,6 +901,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_buffer(f, n->mac, ETH_ALEN);
     n->tx_waiting = qemu_get_be32(f);
     n->mergeable_rx_bufs = qemu_get_be32(f);
+    n->guest_hdr_len = n->mergeable_rx_bufs ?
+        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
 
     if (version_id >= 3)
         n->status = qemu_get_be16(f);

@@ -938,6 +941,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 
         if (n->has_vnet_hdr) {
             tap_using_vnet_hdr(n->nic->nc.peer, 1);
+            n->host_hdr_len = sizeof(struct virtio_net_hdr);
             tap_set_offload(n->nic->nc.peer,
                     (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
                     (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,

@@ -1037,6 +1041,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
     n->tx_waiting = 0;
     n->tx_burst = net->txburst;
     n->mergeable_rx_bufs = 0;
+    n->guest_hdr_len = sizeof(struct virtio_net_hdr);
     n->promisc = 1; /* for compatibility */
 
     n->mac_table.macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #2 lore raw

PATCH v2 2/14 iov: add const annotation

In reply to Michael S. Tsirkin

iov_from_buf does not change iov, make it const.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 iov.c | 2 +-
 iov.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

iov.c +1 -1

index 60705c7..c6a66f0 100644
@@ -26,7 +26,7 @@
 # include <sys/socket.h>
 #endif
 
-size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
+size_t iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
                     size_t offset, const void *buf, size_t bytes)
 {
     size_t done;

iov.h +1 -1

index 381f37a..a73569f 100644
@@ -36,7 +36,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);
  * such "large" value is -1 (sinice size_t is unsigned),
  * so specifying `-1' as `bytes' means 'up to the end of iovec'.
  */
-size_t iov_from_buf(struct iovec *iov, unsigned int iov_cnt,
+size_t iov_from_buf(const struct iovec *iov, unsigned int iov_cnt,
                     size_t offset, const void *buf, size_t bytes);
 size_t iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, void *buf, size_t bytes);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #3 lore raw

PATCH v2 3/14 iov: add iov_cpy

In reply to Michael S. Tsirkin

Add API to copy part of iovec safely.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 iov.c | 23 +++++++++++++++++++++++
 iov.h |  9 +++++++++
 2 files changed, 32 insertions(+)

iov.c +23 -0

index c6a66f0..b7378bf 100644
@@ -228,3 +228,26 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
         fprintf(fp, "\n");
     }
 }
+
+unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
+                 const struct iovec *iov, unsigned int iov_cnt,
+                 size_t offset, size_t bytes)
+{
+    size_t len;
+    unsigned int i, j;
+    for (i = 0, j = 0; i < iov_cnt && j < dst_iov_cnt && bytes; i++) {
+        if (offset >= iov[i].iov_len) {
+            offset -= iov[i].iov_len;
+            continue;
+        }
+        len = MIN(bytes, iov[i].iov_len - offset);
+
+        dst_iov[j].iov_base = iov[i].iov_base + offset;
+        dst_iov[j].iov_len = len;
+        j++;
+        bytes -= len;
+        offset = 0;
+    }
+    assert(offset == 0);
+    return j;
+}

iov.h +9 -0

index a73569f..34c8ec9 100644
@@ -86,3 +86,12 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt,
  */
 void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
                  FILE *fp, const char *prefix, size_t limit);
+
+/*
+ * Partial copy of vector from iov to dst_iov (data is not copied).
+ * dst_iov overlaps iov at a specified offset.
+ * size of dst_iov is at most bytes. dst vector count is returned.
+ */
+unsigned iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
+                 const struct iovec *iov, unsigned int iov_cnt,
+                 size_t offset, size_t bytes);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #4 lore raw

PATCH v2 4/14 virtio-net: avoid sg copy

In reply to Michael S. Tsirkin

Avoid tweaking iovec during receive. This removes
the need to copy the vector.
Note: we currently have an evil cast in work_around_broken_dhclient and unfortunately this patch does not fix it - just pushes the evil cast to another place.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

hw/virtio-net.c +23 ~~-25~~

index 0c5081e..e2f354f 100644
@@ -504,40 +504,37 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)
  * cache.
  */
 static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
-                                        const uint8_t *buf, size_t size)
+                                        uint8_t *buf, size_t size)
 {
     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
         (size > 27 && size < 1500) && /* normal sized MTU */
         (buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
         (buf[23] == 17) && /* ip.protocol == UDP */
         (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
-        /* FIXME this cast is evil */
-        net_checksum_calculate((uint8_t *)buf, size);
+        net_checksum_calculate(buf, size);
         hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
     }
 }
 
-static int receive_header(VirtIONet *n, struct iovec *iov, int iovcnt,
-                          const void *buf, size_t size, size_t hdr_len)
+static int receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
+                          const void *buf, size_t size)
 {
-    struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)iov[0].iov_base;
     int offset = 0;
 
-    hdr->flags = 0;
-    hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
-
     if (n->has_vnet_hdr) {
-        memcpy(hdr, buf, sizeof(*hdr));
-        offset = sizeof(*hdr);
-        work_around_broken_dhclient(hdr, buf + offset, size - offset);
+        /* FIXME this cast is evil */
+        void *wbuf = (void *)buf;
+        work_around_broken_dhclient(wbuf, wbuf + offset, size - offset);
+        offset = sizeof(struct virtio_net_hdr);
+        iov_from_buf(iov, iov_cnt, 0, buf, offset);
+    } else {
+        struct virtio_net_hdr hdr = {
+            .flags = 0,
+            .gso_type = VIRTIO_NET_HDR_GSO_NONE
+        };
+        iov_from_buf(iov, iov_cnt, 0, &hdr, sizeof hdr);
     }
 
-    /* We only ever receive a struct virtio_net_hdr from the tapfd,
-     * but we may be passing along a larger header to the guest.
-     */
-    iov[0].iov_base += hdr_len;
-    iov[0].iov_len  -= hdr_len;
-
     return offset;
 }

@@ -598,7 +595,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t offset, i;
+    const struct iovec *sg = elem.in_sg;
+    size_t offset, i, guest_offset;
 
     if (!virtio_net_can_receive(&n->nic->nc))
         return -1;

@@ -615,7 +613,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
     while (offset < size) {
         VirtQueueElement elem;
         int len, total;
-        struct iovec sg[VIRTQUEUE_MAX_SIZE];
+        const struct iovec *sg = elem.in_sg;
 
         total = 0;

@@ -640,20 +638,20 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
             exit(1);
         }
 
-        memcpy(&sg, &elem.in_sg[0], sizeof(sg[0]) * elem.in_num);
-
         if (i == 0) {
             if (n->mergeable_rx_bufs)
                 mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
 
             offset += receive_header(n, sg, elem.in_num,
-                                     buf + offset, size - offset,
-                                     n->guest_hdr_len);
+                                     buf + offset, size - offset);
             total += n->guest_hdr_len;
+            guest_offset = n->guest_hdr_len;
+        } else {
+            guest_offset = 0;
         }
 
         /* copy in packet.  ugh */
-        len = iov_from_buf(sg, elem.in_num, 0,
+        len = iov_from_buf(sg, elem.in_num, guest_offset,
                            buf + offset, size - offset);
         total += len;
         offset += len;

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #5 lore raw

PATCH v2 5/14 virtio-net: use safe iov operations for rx

In reply to Michael S. Tsirkin

Avoid magling iov manually: use safe iov operations for processing packets incoming to guest. This also removes the requirement for virtio header to fit the first s/g entry exactly.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

hw/virtio-net.c +14 ~~-11~~

index e2f354f..883d381 100644
@@ -594,8 +594,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
 static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
-    struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    const struct iovec *sg = elem.in_sg;
+    struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
+    struct virtio_net_hdr_mrg_rxbuf mhdr;
+    unsigned mhdr_cnt = 0;
     size_t offset, i, guest_offset;
 
     if (!virtio_net_can_receive(&n->nic->nc))

@@ -633,14 +634,13 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
             exit(1);
         }
 
-        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != n->guest_hdr_len) {
-            error_report("virtio-net header not in first element");
-            exit(1);
-        }
-
         if (i == 0) {
-            if (n->mergeable_rx_bufs)
-                mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
+            if (n->mergeable_rx_bufs) {
+                mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
+                                    sg, elem.in_num,
+                                    offsetof(typeof(mhdr), num_buffers),
+                                    sizeof(mhdr.num_buffers));
+            }
 
             offset += receive_header(n, sg, elem.in_num,
                                      buf + offset, size - offset);

@@ -673,8 +673,11 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
         virtqueue_fill(n->rx_vq, &elem, total, i++);
     }
 
-    if (mhdr) {
-        stw_p(&mhdr->num_buffers, i);
+    if (mhdr_cnt) {
+        stw_p(&mhdr.num_buffers, i);
+        iov_from_buf(mhdr_sg, mhdr_cnt,
+                     0,
+                     &mhdr.num_buffers, sizeof mhdr.num_buffers);
     }
 
     virtqueue_flush(n->rx_vq, i);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #6 lore raw

PATCH v2 6/14 virtio-net: refactor receive_hdr

In reply to Michael S. Tsirkin

Now that we know host hdr length, we don't need to duplicate the logic in receive_hdr: caller can figure out the offset itself.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

hw/virtio-net.c +7 ~~-11~~

index 883d381..fde2322 100644
@@ -516,17 +516,15 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
     }
 }
 
-static int receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
-                          const void *buf, size_t size)
+static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
+                           const void *buf, size_t size)
 {
-    int offset = 0;
-
     if (n->has_vnet_hdr) {
         /* FIXME this cast is evil */
         void *wbuf = (void *)buf;
-        work_around_broken_dhclient(wbuf, wbuf + offset, size - offset);
-        offset = sizeof(struct virtio_net_hdr);
-        iov_from_buf(iov, iov_cnt, 0, buf, offset);
+        work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
+                                    size - n->host_hdr_len);
+        iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
     } else {
         struct virtio_net_hdr hdr = {
             .flags = 0,

@@ -534,8 +532,6 @@ static int receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
         };
         iov_from_buf(iov, iov_cnt, 0, &hdr, sizeof hdr);
     }
-
-    return offset;
 }
 
 static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)

@@ -642,8 +638,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
                                     sizeof(mhdr.num_buffers));
             }
 
-            offset += receive_header(n, sg, elem.in_num,
-                                     buf + offset, size - offset);
+            receive_header(n, sg, elem.in_num, buf + offset, size - offset);
+            offset += n->host_hdr_len;
             total += n->guest_hdr_len;
             guest_offset = n->guest_hdr_len;
         } else {

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #7 lore raw

PATCH v2 7/14 virtio-net: first s/g is always at start of buf

In reply to Michael S. Tsirkin

We know offset is 0, assert that.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/virtio-net.c +3 -2

index fde2322..b9a8be4 100644
@@ -631,6 +631,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
         }
 
         if (i == 0) {
+            assert(offset == 0);
             if (n->mergeable_rx_bufs) {
                 mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
                                     sg, elem.in_num,

@@ -638,8 +639,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
                                     sizeof(mhdr.num_buffers));
             }
 
-            receive_header(n, sg, elem.in_num, buf + offset, size - offset);
-            offset += n->host_hdr_len;
+            receive_header(n, sg, elem.in_num, buf, size);
+            offset = n->host_hdr_len;
             total += n->guest_hdr_len;
             guest_offset = n->guest_hdr_len;
         } else {

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #8 lore raw

PATCH v2 8/14 virtio-net: switch tx to safe iov functions

In reply to Michael S. Tsirkin

Avoid mangling iovec manually: use safe iov_*
functions.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

hw/virtio-net.c +19 ~~-12~~

index b9a8be4..358093e 100644
@@ -715,10 +715,10 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
     }
 
     while (virtqueue_pop(vq, &elem)) {
-        ssize_t ret, len = 0;
+        ssize_t ret, len;
         unsigned int out_num = elem.out_num;
         struct iovec *out_sg = &elem.out_sg[0];
-        unsigned hdr_len;
+        struct iovec sg[VIRTQUEUE_MAX_SIZE];
 
         /* hdr_len refers to the header received from the guest */
         hdr_len = n->mergeable_rx_bufs ?

@@ -730,18 +730,25 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
             exit(1);
         }
 
-        /* ignore the header if GSO is not supported */
-        if (!n->has_vnet_hdr) {
-            out_num--;
-            out_sg++;
-            len += hdr_len;
-        } else if (n->mergeable_rx_bufs) {
-            /* tapfd expects a struct virtio_net_hdr */
-            hdr_len -= sizeof(struct virtio_net_hdr);
-            out_sg->iov_len -= hdr_len;
-            len += hdr_len;
+        /*
+         * If host wants to see the guest header as is, we can
+         * pass it on unchanged. Otherwise, copy just the parts
+         * that host is interested in.
+         */
+        assert(n->host_hdr_len <= n->guest_hdr_len);
+        if (n->host_hdr_len != n->guest_hdr_len) {
+            unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
+                                       out_sg, out_num,
+                                       0, n->host_hdr_len);
+            sg_num += iov_copy(sg + sg_num, ARRAY_SIZE(sg) - sg_num,
+                             out_sg, out_num,
+                             n->guest_hdr_len, -1);
+            out_num = sg_num;
+            out_sg = sg;
         }
 
+        len = hdr_len;
+
         ret = qemu_sendv_packet_async(&n->nic->nc, out_sg, out_num,
                                       virtio_net_tx_complete);
         if (ret == 0) {

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #9 lore raw

PATCH v2 9/14 virtio-net: simplify rx code

In reply to Michael S. Tsirkin

Remove code duplication using guest header length that we track. Drop specific layout requirement for rx buffers: things work using generic iovec functions in any case.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

hw/virtio-net.c +2 -7

index 358093e..86cd4a9 100644
@@ -720,12 +720,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
         struct iovec *out_sg = &elem.out_sg[0];
         struct iovec sg[VIRTQUEUE_MAX_SIZE];
 
-        /* hdr_len refers to the header received from the guest */
-        hdr_len = n->mergeable_rx_bufs ?
-            sizeof(struct virtio_net_hdr_mrg_rxbuf) :
-            sizeof(struct virtio_net_hdr);
-
-        if (out_num < 1 || out_sg->iov_len != hdr_len) {
+        if (out_num < 1) {
             error_report("virtio-net header not in first element");
             exit(1);
         }

@@ -747,7 +742,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
             out_sg = sg;
         }
 
-        len = hdr_len;
+        len = n->guest_hdr_len;
 
         ret = qemu_sendv_packet_async(&n->nic->nc, out_sg, out_num,
                                       virtio_net_tx_complete);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:54 UTC #10 lore raw

PATCH v2 10/14 virtio: don't mark unaccessed memory as dirty

In reply to Michael S. Tsirkin

offset of accessed buffer is calculated using iov_length, so it can exceed accessed len. If that happens
math in len - offset wraps around, and size becomes wrong. As real value is 0, so this is harmless but unnecessary.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/virtio.c +1 -1

index 209c763..b5764bb 100644
@@ -241,7 +241,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                                   elem->in_sg[i].iov_len,
                                   1, size);
 
-        offset += elem->in_sg[i].iov_len;
+        offset += size;
     }
 
     for (i = 0; i < elem->out_num; i++)

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:54 UTC #11 lore raw

PATCH v2 11/14 virtio-net: fix used len for tx

In reply to Michael S. Tsirkin

There is no out sg for TX, so used buf length for tx should always be 0.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/virtio-net.c +2 -2

index 86cd4a9..ef7f399 100644
@@ -689,7 +689,7 @@ static void virtio_net_tx_complete(NetClientState *nc, ssize_t len)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
 
-    virtqueue_push(n->tx_vq, &n->async_tx.elem, n->async_tx.len);
+    virtqueue_push(n->tx_vq, &n->async_tx.elem, 0);
     virtio_notify(&n->vdev, n->tx_vq);
 
     n->async_tx.elem.out_num = n->async_tx.len = 0;

@@ -755,7 +755,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 
         len += ret;
 
-        virtqueue_push(vq, &elem, len);
+        virtqueue_push(vq, &elem, 0);
         virtio_notify(&n->vdev, vq);
 
         if (++num_packets >= n->tx_burst) {

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #12 lore raw

PATCH v2 12/14 virtio-net: minor code simplification

In reply to Michael S. Tsirkin

During packet filtering, we can now use host hdr len to offset incoming buffer unconditionally.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

hw/virtio-net.c +1 -3

index ef7f399..8ecc07b 100644
@@ -544,9 +544,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
     if (n->promisc)
         return 1;
 
-    if (n->has_vnet_hdr) {
-        ptr += sizeof(struct virtio_net_hdr);
-    }
+    ptr += n->host_hdr_len;
 
     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #13 lore raw

PATCH v2 13/14 virtio-net: test peer header support at init time

In reply to Michael S. Tsirkin

There's no reason to query header support at random
times: at load or feature query.
Driver also might not query functions. Cleaner to do it at device init.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

hw/virtio-net.c +14 -9

index 8ecc07b..86207a1 100644
@@ -202,16 +202,19 @@ static void virtio_net_reset(VirtIODevice *vdev)
     memset(n->vlans, 0, MAX_VLAN >> 3);
 }
 
-static int peer_has_vnet_hdr(VirtIONet *n)
+static void peer_test_vnet_hdr(VirtIONet *n)
 {
     if (!n->nic->nc.peer)
-        return 0;
+        return;
 
     if (n->nic->nc.peer->info->type != NET_CLIENT_OPTIONS_KIND_TAP)
-        return 0;
+        return;
 
     n->has_vnet_hdr = tap_has_vnet_hdr(n->nic->nc.peer);
+}
 
+static int peer_has_vnet_hdr(VirtIONet *n)
+{
     return n->has_vnet_hdr;
 }

@@ -231,10 +234,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 
     features |= (1 << VIRTIO_NET_F_MAC);
 
-    if (peer_has_vnet_hdr(n)) {
-        tap_using_vnet_hdr(n->nic->nc.peer, 1);
-        n->host_hdr_len = sizeof(struct virtio_net_hdr);
-    } else {
+    if (!peer_has_vnet_hdr(n)) {
         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
         features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
         features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO6);

@@ -938,8 +938,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
 
         if (n->has_vnet_hdr) {
-            tap_using_vnet_hdr(n->nic->nc.peer, 1);
-            n->host_hdr_len = sizeof(struct virtio_net_hdr);
             tap_set_offload(n->nic->nc.peer,
                     (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
                     (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,

@@ -1033,6 +1031,13 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
     n->status = VIRTIO_NET_S_LINK_UP;
 
     n->nic = qemu_new_nic(&net_virtio_info, conf, object_get_typename(OBJECT(dev)), dev->id, n);
+    peer_test_vnet_hdr(n);
+    if (peer_has_vnet_hdr(n)) {
+        tap_using_vnet_hdr(n->nic->nc.peer, 1);
+        n->host_hdr_len = sizeof(struct virtio_net_hdr);
+    } else {
+        n->host_hdr_len = 0;
+    }
 
     qemu_format_nic_info_str(&n->nic->nc, conf->macaddr.a);

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-09-25 11:11 UTC #14 lore raw

PATCH v2 14/14 virtio-net: enable mrg buf header in tap on linux

In reply to Michael S. Tsirkin

Modern linux supports arbitrary header size, which makes it possible to pass mrg buf header to tap directly without iovec mangling. Use this capability when it is there.

This removes the need to deal with it in vhost-net as we do now.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/vhost_net.c  | 13 -------------
 hw/virtio-net.c | 26 ++++++++++++++++++--------
 2 files changed, 18 insertions(+), 21 deletions(-)

hw/vhost_net.c +0 ~~-13~~

index df2c4a3..8241601 100644
@@ -150,10 +150,6 @@ int vhost_net_start(struct vhost_net *net,
     if (r < 0) {
         goto fail_notifiers;
     }
-    if (net->dev.acked_features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-        tap_set_vnet_hdr_len(net->nc,
-                             sizeof(struct virtio_net_hdr_mrg_rxbuf));
-    }
 
     r = vhost_dev_start(&net->dev, dev);
     if (r < 0) {

@@ -179,9 +175,6 @@ fail:
     }
     net->nc->info->poll(net->nc, true);
     vhost_dev_stop(&net->dev, dev);
-    if (net->dev.acked_features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-        tap_set_vnet_hdr_len(net->nc, sizeof(struct virtio_net_hdr));
-    }
 fail_start:
     vhost_dev_disable_notifiers(&net->dev, dev);
 fail_notifiers:

@@ -199,18 +192,12 @@ void vhost_net_stop(struct vhost_net *net,
     }
     net->nc->info->poll(net->nc, true);
     vhost_dev_stop(&net->dev, dev);
-    if (net->dev.acked_features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-        tap_set_vnet_hdr_len(net->nc, sizeof(struct virtio_net_hdr));
-    }
     vhost_dev_disable_notifiers(&net->dev, dev);
 }
 
 void vhost_net_cleanup(struct vhost_net *net)
 {
     vhost_dev_cleanup(&net->dev);
-    if (net->dev.acked_features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-        tap_set_vnet_hdr_len(net->nc, sizeof(struct virtio_net_hdr));
-    }
     g_free(net);
 }
 #else

hw/virtio-net.c +18 -8

index 86207a1..c2a6bbf 100644
@@ -228,6 +228,20 @@ static int peer_has_ufo(VirtIONet *n)
     return n->has_ufo;
 }
 
+static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int mergeable_rx_bufs)
+{
+    n->mergeable_rx_bufs = mergeable_rx_bufs;
+
+    n->guest_hdr_len = n->mergeable_rx_bufs ?
+        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
+
+    if (peer_has_vnet_hdr(n) &&
+        tap_has_vnet_hdr_len(n->nic->nc.peer, n->guest_hdr_len)) {
+        tap_set_vnet_hdr_len(n->nic->nc.peer, n->guest_hdr_len);
+        n->host_hdr_len = n->guest_hdr_len;
+    }
+}
+
 static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIONet *n = to_virtio_net(vdev);

@@ -280,9 +294,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
-    n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
-    n->guest_hdr_len = n->mergeable_rx_bufs ?
-        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
+    virtio_net_set_mrg_rx_bufs(n, !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)));
 
     if (n->has_vnet_hdr) {
         tap_set_offload(n->nic->nc.peer,

@@ -898,9 +910,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 
     qemu_get_buffer(f, n->mac, ETH_ALEN);
     n->tx_waiting = qemu_get_be32(f);
-    n->mergeable_rx_bufs = qemu_get_be32(f);
-    n->guest_hdr_len = n->mergeable_rx_bufs ?
-        sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
+
+    virtio_net_set_mrg_rx_bufs(n, qemu_get_be32(f));
 
     if (version_id >= 3)
         n->status = qemu_get_be16(f);

@@ -1043,8 +1054,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
     n->tx_waiting = 0;
     n->tx_burst = net->txburst;
-    n->mergeable_rx_bufs = 0;
-    n->guest_hdr_len = sizeof(struct virtio_net_hdr);
+    virtio_net_set_mrg_rx_bufs(n, 0);
     n->promisc = 1; /* for compatibility */
 
     n->mac_table.macs = g_malloc0(MAC_TABLE_ENTRIES * ETH_ALEN);

Signature · 1 line

MST