[Qemu-devel] [PATCHv2 00/14] virtio-net: iovec handling cleanup

[Qemu-devel] [PATCHv2 00/14] virtio-net: iovec handling cleanup

[Michael S. Tsirkin]

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

-- 
MST

[Qemu-devel] [PATCHv2 01/14] virtio-net: track host/guest header length

[Michael S. Tsirkin]

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 6490743..0c5081e 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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);
-- 
MST

[Qemu-devel] [PATCHv2 02/14] iov: add const annotation

[Michael S. Tsirkin]

iov_from_buf does not change iov, make it const.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 iov.c | 2 +-
 iov.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/iov.c b/iov.c
index 60705c7..c6a66f0 100644
--- a/iov.c
+++ b/iov.c
@@ -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;
diff --git a/iov.h b/iov.h
index 381f37a..a73569f 100644
--- a/iov.h
+++ b/iov.h
@@ -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);
-- 
MST

[Qemu-devel] [PATCHv2 03/14] iov: add iov_cpy

[Michael S. Tsirkin]

Add API to copy part of iovec safely.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 iov.c | 23 +++++++++++++++++++++++
 iov.h |  9 +++++++++
 2 files changed, 32 insertions(+)

diff --git a/iov.c b/iov.c
index c6a66f0..b7378bf 100644
--- a/iov.c
+++ b/iov.c
@@ -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;
+}
diff --git a/iov.h b/iov.h
index a73569f..34c8ec9 100644
--- a/iov.h
+++ b/iov.h
@@ -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);
-- 
MST

[Qemu-devel] [PATCHv2 04/14] virtio-net: avoid sg copy

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 0c5081e..e2f354f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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;
-- 
MST

[Qemu-devel] [PATCHv2 05/14] virtio-net: use safe iov operations for rx

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index e2f354f..883d381 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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);
-- 
MST

[Qemu-devel] [PATCHv2 06/14] virtio-net: refactor receive_hdr

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 883d381..fde2322 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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 {
-- 
MST

[Qemu-devel] [PATCHv2 07/14] virtio-net: first s/g is always at start of buf

[Michael S. Tsirkin]

We know offset is 0, assert that.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index fde2322..b9a8be4 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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 {
-- 
MST

[Qemu-devel] [PATCHv2 08/14] virtio-net: switch tx to safe iov functions

[Michael S. Tsirkin]

Avoid mangling iovec manually: use safe iov_*
functions.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index b9a8be4..358093e 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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) {
-- 
MST

[Qemu-devel] [PATCHv2 09/14] virtio-net: simplify rx code

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 358093e..86cd4a9 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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);
-- 
MST

[Qemu-devel] [PATCHv2 10/14] virtio: don't mark unaccessed memory as dirty

[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 <mst@redhat.com>
---
 hw/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 209c763..b5764bb 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -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++)
-- 
MST

[Qemu-devel] [PATCHv2 11/14] virtio-net: fix used len for tx

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 86cd4a9..ef7f399 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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) {
-- 
MST

[Qemu-devel] [PATCHv2 12/14] virtio-net: minor code simplification

[Michael S. Tsirkin]

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ef7f399..8ecc07b 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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;
-- 
MST

[Qemu-devel] [PATCHv2 13/14] virtio-net: test peer header support at init time

[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 <mst@redhat.com>
---
 hw/virtio-net.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 8ecc07b..86207a1 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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);
 
-- 
MST

[Qemu-devel] [PATCHv2 14/14] virtio-net: enable mrg buf header in tap on linux

[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 <mst@redhat.com>
---
 hw/vhost_net.c  | 13 -------------
 hw/virtio-net.c | 26 ++++++++++++++++++--------
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/hw/vhost_net.c b/hw/vhost_net.c
index df2c4a3..8241601 100644
--- a/hw/vhost_net.c
+++ b/hw/vhost_net.c
@@ -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
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 86207a1..c2a6bbf 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -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);
-- 
MST