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

[Qemu-devel] [PATCH 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.

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 | 177 +++++++++++++++++++++++++++++++-------------------------
 hw/virtio.c     |   2 +-
 iov.c           |  25 +++++++-
 iov.h           |  11 +++-
 5 files changed, 132 insertions(+), 96 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 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] [PATCH 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] [PATCH 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..0dfcb28 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_cpy(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..f0daefa 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. Actual size is returned.
+ */
+size_t iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
+               const struct iovec *iov, unsigned int iov_cnt,
+               size_t offset, size_t bytes);
-- 
MST

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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..0dfcb28 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_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> +                 const struct iovec *iov, unsigned int iov_cnt,
> +                 size_t offset, size_t bytes)

I think the readability f the cde wuld be imprved if yu added the
missing 'o' to iov_cpy.

Otherwise:

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> +{
> +    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..f0daefa 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. Actual size is returned.
> + */
> +size_t iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> +               const struct iovec *iov, unsigned int iov_cnt,
> +               size_t offset, size_t bytes);
> -- 
> MST

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

[Michael S. Tsirkin]

On Mon, Sep 24, 2012 at 07:34:00PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > 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..0dfcb28 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_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> > +                 const struct iovec *iov, unsigned int iov_cnt,
> > +                 size_t offset, size_t bytes)
> 
> I think the readability f the cde wuld be imprved if yu added the
> missing 'o' to iov_cpy.

I was thinking about memcpy but maybe that's not a good model to follow.
Will fix.

> Otherwise:
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Regards,
> 
> Anthony Liguori
> 
> > +{
> > +    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..f0daefa 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. Actual size is returned.
> > + */
> > +size_t iov_cpy(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] [PATCH 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 already have an evil cast in work_around_broken_dhclient
and unfortunately this adds another one.
const on buf is ignored by this function anyway so arguably
this is not making things much worse.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c | 45 +++++++++++++++++++++------------------------
 iov.h           |  8 ++++----
 2 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 0c5081e..6e53858 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)
  * we should provide a mechanism to disable it to avoid polluting the host
  * cache.
  */
-static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
+static void work_around_broken_dhclient(const struct virtio_net_hdr *hdr,
                                         const uint8_t *buf, size_t size)
 {
     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
@@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
         (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
         /* FIXME this cast is evil */
         net_checksum_calculate((uint8_t *)buf, size);
-        hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+        ((struct virtio_net_hdr *)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);
+        work_around_broken_dhclient(buf, buf + 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 +594,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 +612,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 +637,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;
diff --git a/iov.h b/iov.h
index f0daefa..6c9fdf1 100644
--- a/iov.h
+++ b/iov.h
@@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
 /*
  * 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. Actual size is returned.
+ * size of dst_iov is at most bytes. dst vector count is returned.
  */
-size_t iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
-               const struct iovec *iov, unsigned int iov_cnt,
-               size_t offset, size_t bytes);
+unsigned iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
+                 const struct iovec *iov, unsigned int iov_cnt,
+                 size_t offset, size_t bytes);
-- 
MST

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Avoid tweaking iovec during receive. This removes
> the need to copy the vector.
> Note: we already have an evil cast in work_around_broken_dhclient
> and unfortunately this adds another one.
> const on buf is ignored by this function anyway so arguably
> this is not making things much worse.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio-net.c | 45 +++++++++++++++++++++------------------------
>  iov.h           |  8 ++++----
>  2 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 0c5081e..6e53858 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)
>   * we should provide a mechanism to disable it to avoid polluting the host
>   * cache.
>   */
> -static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> +static void work_around_broken_dhclient(const struct virtio_net_hdr *hdr,
>                                          const uint8_t *buf, size_t size)
>  {
>      if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
> @@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
>          (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
>          /* FIXME this cast is evil */
>          net_checksum_calculate((uint8_t *)buf, size);
> -        hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +        ((struct virtio_net_hdr *)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);
> +        work_around_broken_dhclient(buf, buf + offset, size -
> offset);

I think it would be less ugly to cast here.

> +        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 +594,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 +612,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 +637,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;
> diff --git a/iov.h b/iov.h
> index f0daefa..6c9fdf1 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
>  /*
>   * 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. Actual size is returned.
> + * size of dst_iov is at most bytes. dst vector count is returned.
>   */
> -size_t iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> -               const struct iovec *iov, unsigned int iov_cnt,
> -               size_t offset, size_t bytes);
> +unsigned iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> +                 const struct iovec *iov, unsigned int iov_cnt,
> +                 size_t offset, size_t bytes);

I think this belongs in an earlier patch.

Regards,

Anthony Liguori

> -- 
> MST

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

[Michael S. Tsirkin]

On Mon, Sep 24, 2012 at 07:37:40PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Avoid tweaking iovec during receive. This removes
> > the need to copy the vector.
> > Note: we already have an evil cast in work_around_broken_dhclient
> > and unfortunately this adds another one.
> > const on buf is ignored by this function anyway so arguably
> > this is not making things much worse.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio-net.c | 45 +++++++++++++++++++++------------------------
> >  iov.h           |  8 ++++----
> >  2 files changed, 25 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 0c5081e..6e53858 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)
> >   * we should provide a mechanism to disable it to avoid polluting the host
> >   * cache.
> >   */
> > -static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> > +static void work_around_broken_dhclient(const struct virtio_net_hdr *hdr,
> >                                          const uint8_t *buf, size_t size)
> >  {
> >      if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
> > @@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> >          (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
> >          /* FIXME this cast is evil */
> >          net_checksum_calculate((uint8_t *)buf, size);
> > -        hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > +        ((struct virtio_net_hdr *)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);
> > +        work_around_broken_dhclient(buf, buf + offset, size -
> > offset);
> 
> I think it would be less ugly to cast here.

I was thinking about this but could not decide either way.
Will do.

> > +        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 +594,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 +612,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 +637,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;
> > diff --git a/iov.h b/iov.h
> > index f0daefa..6c9fdf1 100644
> > --- a/iov.h
> > +++ b/iov.h
> > @@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,
> >  /*
> >   * 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. Actual size is returned.
> > + * size of dst_iov is at most bytes. dst vector count is returned.
> >   */
> > -size_t iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> > -               const struct iovec *iov, unsigned int iov_cnt,
> > -               size_t offset, size_t bytes);
> > +unsigned iov_cpy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
> > +                 const struct iovec *iov, unsigned int iov_cnt,
> > +                 size_t offset, size_t bytes);
> 
> I think this belongs in an earlier patch.

Good catch thanks.

> Regards,
> 
> Anthony Liguori
> 
> > -- 
> > MST

[Qemu-devel] [PATCH 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 6e53858..9611d95 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -593,8 +593,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))
@@ -632,14 +633,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_cpy(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);
@@ -672,8 +672,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

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

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

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  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 6e53858..9611d95 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -593,8 +593,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))
> @@ -632,14 +633,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_cpy(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);
> @@ -672,8 +672,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] [PATCH 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 9611d95..3a0d1a7 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -517,15 +517,13 @@ static void work_around_broken_dhclient(const 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) {
-        work_around_broken_dhclient(buf, buf + offset, size - offset);
-        offset = sizeof(struct virtio_net_hdr);
-        iov_from_buf(iov, iov_cnt, 0, buf, offset);
+        work_around_broken_dhclient(buf, buf + 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,
@@ -533,8 +531,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)
@@ -641,8 +637,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

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

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

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  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 9611d95..3a0d1a7 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -517,15 +517,13 @@ static void work_around_broken_dhclient(const 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) {
> -        work_around_broken_dhclient(buf, buf + offset, size - offset);
> -        offset = sizeof(struct virtio_net_hdr);
> -        iov_from_buf(iov, iov_cnt, 0, buf, offset);
> +        work_around_broken_dhclient(buf, buf + 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,
> @@ -533,8 +531,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)
> @@ -641,8 +637,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] [PATCH 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 3a0d1a7..6e6f5f3 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -630,6 +630,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_cpy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
                                    sg, elem.in_num,
@@ -637,8 +638,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

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

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> We know offset is 0, assert that.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  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 3a0d1a7..6e6f5f3 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -630,6 +630,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_cpy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
>                                     sg, elem.in_num,
> @@ -637,8 +638,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] [PATCH 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 6e6f5f3..23feb21 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -714,10 +714,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 ?
@@ -729,18 +729,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_cpy(sg, ARRAY_SIZE(sg),
+                                      out_sg, out_num,
+                                      0, n->host_hdr_len);
+            sg_num += iov_cpy(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] [PATCH 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 23feb21..2381ee5 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -719,12 +719,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);
         }
@@ -746,7 +741,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] [PATCH 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] [PATCH 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 2381ee5..d9a9f8f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -688,7 +688,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;
@@ -754,7 +754,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

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

[Jason Wang]

On 09/25/2012 07:05 AM, Michael S. Tsirkin wrote:
> There is no out sg for TX, so used buf length for tx
> should always be 0.

According to the spec, the len is "Total length of the descriptor chain 
which was used (written to)". So I wonder if we need to pass the len 
here, it looks useful for guest how many bytes were sent by the driver 
(consider qemu may truncate the packet).

> 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 2381ee5..d9a9f8f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -688,7 +688,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;
> @@ -754,7 +754,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) {

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

[Michael S. Tsirkin]

On Tue, Sep 25, 2012 at 02:15:07PM +0800, Jason Wang wrote:
> On 09/25/2012 07:05 AM, Michael S. Tsirkin wrote:
> >There is no out sg for TX, so used buf length for tx
> >should always be 0.
> 
> According to the spec, the len is "Total length of the descriptor
> chain which was used (written to)".

Right. And with TX no bytes are written at all.

> So I wonder if we need to pass
> the len here, it looks useful for guest how many bytes were sent by
> the driver (consider qemu may truncate the packet).

I thik qemu may not truncate packet.

> >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 2381ee5..d9a9f8f 100644
> >--- a/hw/virtio-net.c
> >+++ b/hw/virtio-net.c
> >@@ -688,7 +688,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;
> >@@ -754,7 +754,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) {

[Qemu-devel] [PATCH 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 d9a9f8f..c65dead 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -543,9 +543,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] [PATCH 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 c65dead..a7c8bb3 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);
@@ -937,8 +937,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,
@@ -1032,6 +1030,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] [PATCH 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 a7c8bb3..7558eab 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,
@@ -897,9 +909,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);
@@ -1042,8 +1053,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