[Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Greg Kurz]

This series brings some improvements to the cross-endian support in the
virtio and vhost code:
- use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
  so that backend cross-endian capabilities benefit to both emulated and
  vhost accelerated devices
- optimize virtio_access_is_big_endian() for little-endian targets
- various cleanups

This v4 is a respin of the v3, taking into account the following fix:

commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
Author: Laurent Vivier <lvivier@redhat.com>
Date:   Wed Jan 13 20:26:25 2016 +0100

    net: set endianness on all backend devices

It impacts patches 1 and 2, that had already been reviewed.

Laurent, Cornelia,

Could you please ack/nack the changes ?

The series is based on Michael Tsirkin's tree:

git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci

Thanks.

--
Greg

---

Greg Kurz (6):
      virtio-net: use the backend cross-endian capabilities
      vhost-net: revert support of cross-endian vnet headers
      virtio: move cross-endian helper to vhost
      vhost: move virtio 1.0 check to cross-endian helper
      vhost: simplify vhost_needs_vring_endian()
      virtio: optimize virtio_access_is_big_endian() for little-endian targets


 hw/net/vhost_net.c                |   41 +++-------------------
 hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
 hw/virtio/vhost.c                 |   27 +++++++++++++--
 include/hw/virtio/virtio-access.h |   28 ++-------------
 include/hw/virtio/virtio-net.h    |    1 +
 5 files changed, 98 insertions(+), 67 deletions(-)

[Qemu-devel] [PATCH v4 1/6] virtio-net: use the backend cross-endian capabilities

[Greg Kurz]

When running a fully emulated device in cross-endian conditions, including
a virtio 1.0 device offered to a big endian guest, we need to fix the vnet
headers. This is currently handled by the virtio_net_hdr_swap() function
in the core virtio-net code but it should actually be handled by the net
backend.

With this patch, virtio-net now tries to configure the backend to do the
endian fixing when the device starts (i.e. drivers sets the CONFIG_OK bit).
If the backend cannot support the requested endiannes, we have to fallback
onto virtio_net_hdr_swap(): this is recorded in the needs_vnet_hdr_swap flag,
to be used in the TX and RX paths.

Note that we reset the backend to the default behaviour (guest native
endianness) when the device stops (i.e. device status had CONFIG_OK bit and
driver unsets it). This is needed, with the linux tap backend at least,
otherwise the guest may lose network connectivity if rebooted into a
different endianness.

The current vhost-net code also tries to configure net backends. This will
be no more needed and will be reverted in a subsequent patch.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v4:
  - fix bug with multiqueue
  - rename helper to virtio_net_vnet_endian_status, for better clarity on
    what it is used for
---
 hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio-access.h |    9 -----
 include/hw/virtio/virtio-net.h    |    1 +
 3 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de696e8dd0d6..5798f87d8ea2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -129,6 +129,13 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
     if (!n->vhost_started) {
         int r, i;
 
+        if (n->needs_vnet_hdr_swap) {
+            error_report("backend does not support %s vnet headers; "
+                         "falling back on userspace virtio",
+                         virtio_is_big_endian(vdev) ? "BE" : "LE");
+            return;
+        }
+
         /* Any packets outstanding? Purge them to avoid touching rings
          * when vhost is running.
          */
@@ -153,6 +160,59 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
     }
 }
 
+static int virtio_net_set_vnet_endian_one(VirtIODevice *vdev,
+                                          NetClientState *peer,
+                                          bool enable)
+{
+    if (virtio_is_big_endian(vdev)) {
+        return qemu_set_vnet_be(peer, enable);
+    } else {
+        return qemu_set_vnet_le(peer, enable);
+    }
+}
+
+static bool virtio_net_set_vnet_endian(VirtIODevice *vdev, NetClientState *ncs,
+                                       int queues, bool enable)
+{
+    int i;
+
+    for (i = 0; i < queues; i++) {
+        if (virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, enable) < 0 &&
+            enable) {
+            while (--i >= 0) {
+                virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, false);
+            }
+
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t status)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    int queues = n->multiqueue ? n->max_queues : 1;
+
+    if (virtio_net_started(n, status)) {
+        /* Before using the device, we tell the network backend about the
+         * endianness to use when parsing vnet headers. If the backend
+         * can't do it, we fallback onto fixing the headers in the core
+         * virtio-net code.
+         */
+        n->needs_vnet_hdr_swap = virtio_net_set_vnet_endian(vdev, n->nic->ncs,
+                                                            queues, true);
+    } else if (virtio_net_started(n, vdev->status)) {
+        /* After using the device, we need to reset the network backend to
+         * the default (guest native endianness), otherwise the guest may
+         * lose network connectivity if it is rebooted into a different
+         * endianness.
+         */
+        virtio_net_set_vnet_endian(vdev, n->nic->ncs, queues, false);
+    }
+}
+
 static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -160,6 +220,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     int i;
     uint8_t queue_status;
 
+    virtio_net_vnet_endian_status(n, status);
     virtio_net_vhost_status(n, status);
 
     for (i = 0; i < n->max_queues; i++) {
@@ -963,7 +1024,10 @@ static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
         void *wbuf = (void *)buf;
         work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
                                     size - n->host_hdr_len);
-        virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
+
+        if (n->needs_vnet_hdr_swap) {
+            virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
+        }
         iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
     } else {
         struct virtio_net_hdr hdr = {
@@ -1184,7 +1248,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
                 error_report("virtio-net header incorrect");
                 exit(1);
             }
-            if (virtio_needs_swap(vdev)) {
+            if (n->needs_vnet_hdr_swap) {
                 virtio_net_hdr_swap(vdev, (void *) &mhdr);
                 sg2[0].iov_base = &mhdr;
                 sg2[0].iov_len = n->guest_hdr_len;
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 8aec843c8ff3..a01fff2e51d7 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -143,15 +143,6 @@ static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr)
     }
 }
 
-static inline bool virtio_needs_swap(VirtIODevice *vdev)
-{
-#ifdef HOST_WORDS_BIGENDIAN
-    return virtio_access_is_big_endian(vdev) ? false : true;
-#else
-    return virtio_access_is_big_endian(vdev) ? true : false;
-#endif
-}
-
 static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 {
 #ifdef HOST_WORDS_BIGENDIAN
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 2ce3b03bd448..0cabdb682241 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -94,6 +94,7 @@ typedef struct VirtIONet {
     uint64_t curr_guest_offloads;
     QEMUTimer *announce_timer;
     int announce_counter;
+    bool needs_vnet_hdr_swap;
 } VirtIONet;
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,

[Qemu-devel] [PATCH v4 2/6] vhost-net: revert support of cross-endian vnet headers

[Greg Kurz]

Cross-endian is now handled by the core virtio-net code.

This patch reverts:

commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991
	vhost-net: tell tap backend about the vnet endianness

and

commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
	net: set endianness on all backend devices

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v4: - also revert Laurent's fix
---
 hw/net/vhost_net.c |   41 +++++------------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 3940a04b659a..b2428324b3a2 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -36,7 +36,6 @@
 #include "standard-headers/linux/virtio_ring.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/virtio-bus.h"
-#include "hw/virtio/virtio-access.h"
 
 struct vhost_net {
     struct vhost_dev dev;
@@ -197,27 +196,6 @@ static void vhost_net_set_vq_index(struct vhost_net *net, int vq_index)
     net->dev.vq_index = vq_index;
 }
 
-static int vhost_net_set_vnet_endian(VirtIODevice *dev, NetClientState *peer,
-                                     bool set)
-{
-    int r = 0;
-
-    if (virtio_vdev_has_feature(dev, VIRTIO_F_VERSION_1) ||
-        (virtio_legacy_is_cross_endian(dev) && !virtio_is_big_endian(dev))) {
-        r = qemu_set_vnet_le(peer, set);
-        if (r) {
-            error_report("backend does not support LE vnet headers");
-        }
-    } else if (virtio_legacy_is_cross_endian(dev)) {
-        r = qemu_set_vnet_be(peer, set);
-        if (r) {
-            error_report("backend does not support BE vnet headers");
-        }
-    }
-
-    return r;
-}
-
 static int vhost_net_start_one(struct vhost_net *net,
                                VirtIODevice *dev)
 {
@@ -298,25 +276,21 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
     VirtioBusState *vbus = VIRTIO_BUS(qbus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
-    int r, e, i, j;
+    int r, e, i;
 
     if (!k->set_guest_notifiers) {
         error_report("binding does not support guest notifiers");
         return -ENOSYS;
     }
 
-    for (j = 0; j < total_queues; j++) {
-        r = vhost_net_set_vnet_endian(dev, ncs[j].peer, true);
-        if (r < 0) {
-            goto err_endian;
-        }
-        vhost_net_set_vq_index(get_vhost_net(ncs[j].peer), j * 2);
+    for (i = 0; i < total_queues; i++) {
+        vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
     }
 
     r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
     if (r < 0) {
         error_report("Error binding guest notifier: %d", -r);
-        goto err_endian;
+        goto err;
     }
 
     for (i = 0; i < total_queues; i++) {
@@ -338,10 +312,7 @@ err_start:
         fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
         fflush(stderr);
     }
-err_endian:
-    while (--j >= 0) {
-        vhost_net_set_vnet_endian(dev, ncs[j].peer, false);
-    }
+err:
     return r;
 }
 
@@ -363,8 +334,6 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
         fflush(stderr);
     }
     assert(r >= 0);
-
-    assert(vhost_net_set_vnet_endian(dev, ncs[0].peer, false) >= 0);
 }
 
 void vhost_net_cleanup(struct vhost_net *net)

[Qemu-devel] [PATCH v4 3/6] virtio: move cross-endian helper to vhost

[Greg Kurz]

If target is bi-endian (ppc64, arm), the virtio_legacy_is_cross_endian()
indeed returns the runtime state of the virtio device. However, it returns
false unconditionally in the general case. This sounds a bit strange
given the name of the function.

This helper is only useful for vhost actually, where indeed non bi-endian
targets don't have to deal with cross-endian issues.

This patch moves the helper to vhost.c and gives it a more appropriate name.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/vhost.c                 |   17 +++++++++++++++--
 include/hw/virtio/virtio-access.h |   13 -------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7dff75547dc3..6fea07c8ae15 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -749,6 +749,19 @@ static void vhost_log_stop(MemoryListener *listener,
     /* FIXME: implement */
 }
 
+static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
+{
+#ifdef TARGET_IS_BIENDIAN
+#ifdef HOST_WORDS_BIGENDIAN
+    return !virtio_is_big_endian(vdev);
+#else
+    return virtio_is_big_endian(vdev);
+#endif
+#else
+    return false;
+#endif
+}
+
 static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
                                                    bool is_big_endian,
                                                    int vhost_vq_index)
@@ -800,7 +813,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     }
 
     if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-        virtio_legacy_is_cross_endian(vdev)) {
+        vhost_needs_vring_endian(vdev)) {
         r = vhost_virtqueue_set_vring_endian_legacy(dev,
                                                     virtio_is_big_endian(vdev),
                                                     vhost_vq_index);
@@ -897,7 +910,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
      * native as legacy devices expect so by default.
      */
     if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-        virtio_legacy_is_cross_endian(vdev)) {
+        vhost_needs_vring_endian(vdev)) {
         r = vhost_virtqueue_set_vring_endian_legacy(dev,
                                                     !virtio_is_big_endian(vdev),
                                                     vhost_vq_index);
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index a01fff2e51d7..f1f12afe9089 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -32,19 +32,6 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 #endif
 }
 
-static inline bool virtio_legacy_is_cross_endian(VirtIODevice *vdev)
-{
-#ifdef TARGET_IS_BIENDIAN
-#ifdef HOST_WORDS_BIGENDIAN
-    return !virtio_is_big_endian(vdev);
-#else
-    return virtio_is_big_endian(vdev);
-#endif
-#else
-    return false;
-#endif
-}
-
 static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 {
     if (virtio_access_is_big_endian(vdev)) {

[Qemu-devel] [PATCH v4 4/6] vhost: move virtio 1.0 check to cross-endian helper

[Greg Kurz]

Indeed vhost doesn't need to ask for vring endian fixing if the device is
virtio 1.0, since it is already handled by the in-kernel vhost driver. This
patch simply consolidates the logic into the existing helper.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/vhost.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6fea07c8ae15..bb17177f5e7e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -751,6 +751,9 @@ static void vhost_log_stop(MemoryListener *listener,
 
 static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
 {
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        return false;
+    }
 #ifdef TARGET_IS_BIENDIAN
 #ifdef HOST_WORDS_BIGENDIAN
     return !virtio_is_big_endian(vdev);
@@ -812,8 +815,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
         return -errno;
     }
 
-    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-        vhost_needs_vring_endian(vdev)) {
+    if (vhost_needs_vring_endian(vdev)) {
         r = vhost_virtqueue_set_vring_endian_legacy(dev,
                                                     virtio_is_big_endian(vdev),
                                                     vhost_vq_index);
@@ -909,8 +911,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
     /* In the cross-endian case, we need to reset the vring endianness to
      * native as legacy devices expect so by default.
      */
-    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
-        vhost_needs_vring_endian(vdev)) {
+    if (vhost_needs_vring_endian(vdev)) {
         r = vhost_virtqueue_set_vring_endian_legacy(dev,
                                                     !virtio_is_big_endian(vdev),
                                                     vhost_vq_index);

[Qemu-devel] [PATCH v4 5/6] vhost: simplify vhost_needs_vring_endian()

[Greg Kurz]

After the call to virtio_vdev_has_feature(), we only care for legacy
devices, so we don't need the extra check in virtio_is_big_endian().

Also the device_endian field is always set (VIRTIO_DEVICE_ENDIAN_UNKNOWN
may only happen on a virtio_load() path that cannot lead here), so we
don't need the assert() either.

This open codes the device_endian checking in vhost_needs_vring_endian().
It also adds a comment to explain the logic, as recent reviews showed the
cross-endian tweaks aren't that obvious.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/vhost.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index bb17177f5e7e..9f8ac38ccf2d 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -749,6 +749,11 @@ static void vhost_log_stop(MemoryListener *listener,
     /* FIXME: implement */
 }
 
+/* The vhost driver natively knows how to handle the vrings of non
+ * cross-endian legacy devices and modern devices. Only legacy devices
+ * exposed to a bi-endian guest may require the vhost driver to use a
+ * specific endianness.
+ */
 static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
 {
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
@@ -756,9 +761,9 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
     }
 #ifdef TARGET_IS_BIENDIAN
 #ifdef HOST_WORDS_BIGENDIAN
-    return !virtio_is_big_endian(vdev);
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
 #else
-    return virtio_is_big_endian(vdev);
+    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
 #endif
 #else
     return false;

[Qemu-devel] [PATCH v4 6/6] virtio: optimize virtio_access_is_big_endian() for little-endian targets

[Greg Kurz]

When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro
and the virtio_access_is_big_endian() helper to have a branchless fast path
in the virtio memory accessors for targets that don't switch endian.

This was considered as a strong requirement at the time.

Now we have added a runtime check for virtio 1.0, which ruins the benefit
of the virtio_access_is_big_endian() helper for always little-endian targets.

With this patch, always little-endian targets stop checking for virtio 1.0,
since the result is little-endian in all cases.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/hw/virtio/virtio-access.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index f1f12afe9089..8dc84f520316 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,13 +19,13 @@
 
 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
+#if defined(TARGET_IS_BIENDIAN)
+    return virtio_is_big_endian(vdev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         /* Devices conforming to VIRTIO 1.0 or later are always LE. */
         return false;
     }
-#if defined(TARGET_IS_BIENDIAN)
-    return virtio_is_big_endian(vdev);
-#elif defined(TARGET_WORDS_BIGENDIAN)
     return true;
 #else
     return false;

Re: [Qemu-devel] [PATCH v4 1/6] virtio-net: use the backend cross-endian capabilities

[Cornelia Huck]

On Fri, 05 Feb 2016 11:43:11 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> +static bool virtio_net_set_vnet_endian(VirtIODevice *vdev, NetClientState *ncs,
> +                                       int queues, bool enable)

You might consider adding a comment like "returns whether endianness
handling needs to fall back to the device" or so, as it's not
immediately obvious what true/false is supposed to mean. But the patch
is fine.

> +{
> +    int i;
> +
> +    for (i = 0; i < queues; i++) {
> +        if (virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, enable) < 0 &&
> +            enable) {
> +            while (--i >= 0) {
> +                virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, false);
> +            }
> +
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}

Re: [Qemu-devel] [PATCH v4 5/6] vhost: simplify vhost_needs_vring_endian()

[Cornelia Huck]

On Fri, 05 Feb 2016 11:46:04 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> After the call to virtio_vdev_has_feature(), we only care for legacy
> devices, so we don't need the extra check in virtio_is_big_endian().
> 
> Also the device_endian field is always set (VIRTIO_DEVICE_ENDIAN_UNKNOWN
> may only happen on a virtio_load() path that cannot lead here), so we
> don't need the assert() either.
> 
> This open codes the device_endian checking in vhost_needs_vring_endian().
> It also adds a comment to explain the logic, as recent reviews showed the
> cross-endian tweaks aren't that obvious.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/virtio/vhost.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v4 5/6] vhost: simplify vhost_needs_vring_endian()

[Laurent Vivier]

On 05/02/2016 11:46, Greg Kurz wrote:
> After the call to virtio_vdev_has_feature(), we only care for legacy
> devices, so we don't need the extra check in virtio_is_big_endian().
> 
> Also the device_endian field is always set (VIRTIO_DEVICE_ENDIAN_UNKNOWN
> may only happen on a virtio_load() path that cannot lead here), so we
> don't need the assert() either.
> 
> This open codes the device_endian checking in vhost_needs_vring_endian().
> It also adds a comment to explain the logic, as recent reviews showed the
> cross-endian tweaks aren't that obvious.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/virtio/vhost.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index bb17177f5e7e..9f8ac38ccf2d 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -749,6 +749,11 @@ static void vhost_log_stop(MemoryListener *listener,
>      /* FIXME: implement */
>  }
>  
> +/* The vhost driver natively knows how to handle the vrings of non
> + * cross-endian legacy devices and modern devices. Only legacy devices
> + * exposed to a bi-endian guest may require the vhost driver to use a
> + * specific endianness.
> + */
>  static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
>  {
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> @@ -756,9 +761,9 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
>      }
>  #ifdef TARGET_IS_BIENDIAN
>  #ifdef HOST_WORDS_BIGENDIAN
> -    return !virtio_is_big_endian(vdev);
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>  #else
> -    return virtio_is_big_endian(vdev);
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>  #endif
>  #else
>      return false;
> 
> 

Re: [Qemu-devel] [PATCH v4 4/6] vhost: move virtio 1.0 check to cross-endian helper

[Laurent Vivier]

On 05/02/2016 11:45, Greg Kurz wrote:
> Indeed vhost doesn't need to ask for vring endian fixing if the device is
> virtio 1.0, since it is already handled by the in-kernel vhost driver. This
> patch simply consolidates the logic into the existing helper.
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/virtio/vhost.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 6fea07c8ae15..bb17177f5e7e 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -751,6 +751,9 @@ static void vhost_log_stop(MemoryListener *listener,
>  
>  static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
>  {
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        return false;
> +    }
>  #ifdef TARGET_IS_BIENDIAN
>  #ifdef HOST_WORDS_BIGENDIAN
>      return !virtio_is_big_endian(vdev);
> @@ -812,8 +815,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
>          return -errno;
>      }
>  
> -    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> -        vhost_needs_vring_endian(vdev)) {
> +    if (vhost_needs_vring_endian(vdev)) {
>          r = vhost_virtqueue_set_vring_endian_legacy(dev,
>                                                      virtio_is_big_endian(vdev),
>                                                      vhost_vq_index);
> @@ -909,8 +911,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
>      /* In the cross-endian case, we need to reset the vring endianness to
>       * native as legacy devices expect so by default.
>       */
> -    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> -        vhost_needs_vring_endian(vdev)) {
> +    if (vhost_needs_vring_endian(vdev)) {
>          r = vhost_virtqueue_set_vring_endian_legacy(dev,
>                                                      !virtio_is_big_endian(vdev),
>                                                      vhost_vq_index);
> 
> 

Re: [Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Laurent Vivier]

On 05/02/2016 11:39, Greg Kurz wrote:
> This series brings some improvements to the cross-endian support in the
> virtio and vhost code:
> - use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
>   so that backend cross-endian capabilities benefit to both emulated and
>   vhost accelerated devices
> - optimize virtio_access_is_big_endian() for little-endian targets
> - various cleanups
> 
> This v4 is a respin of the v3, taking into account the following fix:
> 
> commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
> Author: Laurent Vivier <lvivier@redhat.com>
> Date:   Wed Jan 13 20:26:25 2016 +0100
> 
>     net: set endianness on all backend devices
> 
> It impacts patches 1 and 2, that had already been reviewed.
> 
> Laurent, Cornelia,
> 
> Could you please ack/nack the changes ?

Looks good to me.

Laurent
> 
> The series is based on Michael Tsirkin's tree:
> 
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> 
> Thanks.
> 
> --
> Greg
> 
> ---
> 
> Greg Kurz (6):
>       virtio-net: use the backend cross-endian capabilities
>       vhost-net: revert support of cross-endian vnet headers
>       virtio: move cross-endian helper to vhost
>       vhost: move virtio 1.0 check to cross-endian helper
>       vhost: simplify vhost_needs_vring_endian()
>       virtio: optimize virtio_access_is_big_endian() for little-endian targets
> 
> 
>  hw/net/vhost_net.c                |   41 +++-------------------
>  hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
>  hw/virtio/vhost.c                 |   27 +++++++++++++--
>  include/hw/virtio/virtio-access.h |   28 ++-------------
>  include/hw/virtio/virtio-net.h    |    1 +
>  5 files changed, 98 insertions(+), 67 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH v4 1/6] virtio-net: use the backend cross-endian capabilities

[Greg Kurz]

On Fri, 5 Feb 2016 12:33:44 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 05 Feb 2016 11:43:11 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > +static bool virtio_net_set_vnet_endian(VirtIODevice *vdev, NetClientState *ncs,
> > +                                       int queues, bool enable)  
> 
> You might consider adding a comment like "returns whether endianness
> handling needs to fall back to the device" or so, as it's not
> immediately obvious what true/false is supposed to mean. But the patch
> is fine.
> 

I agree it is not that obvious. If Michael agrees, I'll send a followup
patch when these series has been applied.

Thanks.

--
Greg

> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < queues; i++) {
> > +        if (virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, enable) < 0 &&
> > +            enable) {
> > +            while (--i >= 0) {
> > +                virtio_net_set_vnet_endian_one(vdev, ncs[i].peer, false);
> > +            }
> > +
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}  

Re: [Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Laurent Vivier]

On 05/02/2016 11:39, Greg Kurz wrote:
> This series brings some improvements to the cross-endian support in the
> virtio and vhost code:
> - use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
>   so that backend cross-endian capabilities benefit to both emulated and
>   vhost accelerated devices
> - optimize virtio_access_is_big_endian() for little-endian targets
> - various cleanups
> 
> This v4 is a respin of the v3, taking into account the following fix:
> 
> commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
> Author: Laurent Vivier <lvivier@redhat.com>
> Date:   Wed Jan 13 20:26:25 2016 +0100
> 
>     net: set endianness on all backend devices
> 
> It impacts patches 1 and 2, that had already been reviewed.
> 
> Laurent, Cornelia,
> 
> Could you please ack/nack the changes ?
> 
> The series is based on Michael Tsirkin's tree:
> 
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> 
> Thanks.
> 
> --
> Greg
> 
> ---
> 
> Greg Kurz (6):
>       virtio-net: use the backend cross-endian capabilities
>       vhost-net: revert support of cross-endian vnet headers
>       virtio: move cross-endian helper to vhost
>       vhost: move virtio 1.0 check to cross-endian helper
>       vhost: simplify vhost_needs_vring_endian()
>       virtio: optimize virtio_access_is_big_endian() for little-endian targets
> 
> 
>  hw/net/vhost_net.c                |   41 +++-------------------
>  hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
>  hw/virtio/vhost.c                 |   27 +++++++++++++--
>  include/hw/virtio/virtio-access.h |   28 ++-------------
>  include/hw/virtio/virtio-net.h    |    1 +
>  5 files changed, 98 insertions(+), 67 deletions(-)
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Greg Kurz]

On Wed, 10 Feb 2016 11:22:20 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> On 05/02/2016 11:39, Greg Kurz wrote:
> > This series brings some improvements to the cross-endian support in the
> > virtio and vhost code:
> > - use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
> >   so that backend cross-endian capabilities benefit to both emulated and
> >   vhost accelerated devices
> > - optimize virtio_access_is_big_endian() for little-endian targets
> > - various cleanups
> > 
> > This v4 is a respin of the v3, taking into account the following fix:
> > 
> > commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
> > Author: Laurent Vivier <lvivier@redhat.com>
> > Date:   Wed Jan 13 20:26:25 2016 +0100
> > 
> >     net: set endianness on all backend devices
> > 
> > It impacts patches 1 and 2, that had already been reviewed.
> > 
> > Laurent, Cornelia,
> > 
> > Could you please ack/nack the changes ?
> > 
> > The series is based on Michael Tsirkin's tree:
> > 
> > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> > 
> > Thanks.
> > 
> > --
> > Greg
> > 
> > ---
> > 
> > Greg Kurz (6):
> >       virtio-net: use the backend cross-endian capabilities
> >       vhost-net: revert support of cross-endian vnet headers
> >       virtio: move cross-endian helper to vhost
> >       vhost: move virtio 1.0 check to cross-endian helper
> >       vhost: simplify vhost_needs_vring_endian()
> >       virtio: optimize virtio_access_is_big_endian() for little-endian targets
> > 
> > 
> >  hw/net/vhost_net.c                |   41 +++-------------------
> >  hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
> >  hw/virtio/vhost.c                 |   27 +++++++++++++--
> >  include/hw/virtio/virtio-access.h |   28 ++-------------
> >  include/hw/virtio/virtio-net.h    |    1 +
> >  5 files changed, 98 insertions(+), 67 deletions(-)
> >   
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 

Michael,

Laurent has posted this R-b according to your request on irc. Most patches in this
series have also been reviewed by Cornelia.

Is there something else that prevents inclusion ?

Thanks.

--
Greg

Re: [Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Michael S. Tsirkin]

On Tue, Feb 16, 2016 at 10:57:19AM +0100, Greg Kurz wrote:
> On Wed, 10 Feb 2016 11:22:20 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> > On 05/02/2016 11:39, Greg Kurz wrote:
> > > This series brings some improvements to the cross-endian support in the
> > > virtio and vhost code:
> > > - use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
> > >   so that backend cross-endian capabilities benefit to both emulated and
> > >   vhost accelerated devices
> > > - optimize virtio_access_is_big_endian() for little-endian targets
> > > - various cleanups
> > > 
> > > This v4 is a respin of the v3, taking into account the following fix:
> > > 
> > > commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
> > > Author: Laurent Vivier <lvivier@redhat.com>
> > > Date:   Wed Jan 13 20:26:25 2016 +0100
> > > 
> > >     net: set endianness on all backend devices
> > > 
> > > It impacts patches 1 and 2, that had already been reviewed.
> > > 
> > > Laurent, Cornelia,
> > > 
> > > Could you please ack/nack the changes ?
> > > 
> > > The series is based on Michael Tsirkin's tree:
> > > 
> > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> > > 
> > > Thanks.
> > > 
> > > --
> > > Greg
> > > 
> > > ---
> > > 
> > > Greg Kurz (6):
> > >       virtio-net: use the backend cross-endian capabilities
> > >       vhost-net: revert support of cross-endian vnet headers
> > >       virtio: move cross-endian helper to vhost
> > >       vhost: move virtio 1.0 check to cross-endian helper
> > >       vhost: simplify vhost_needs_vring_endian()
> > >       virtio: optimize virtio_access_is_big_endian() for little-endian targets
> > > 
> > > 
> > >  hw/net/vhost_net.c                |   41 +++-------------------
> > >  hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
> > >  hw/virtio/vhost.c                 |   27 +++++++++++++--
> > >  include/hw/virtio/virtio-access.h |   28 ++-------------
> > >  include/hw/virtio/virtio-net.h    |    1 +
> > >  5 files changed, 98 insertions(+), 67 deletions(-)
> > >   
> > 
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > 
> 
> Michael,
> 
> Laurent has posted this R-b according to your request on irc. Most patches in this
> series have also been reviewed by Cornelia.
> 
> Is there something else that prevents inclusion ?
> 
> Thanks.

Nothing - my pull request got delayed as I fixed
a bug I have inadvertently intriduced.
They will be in the next pull request.

> --
> Greg

Re: [Qemu-devel] [PATCH v4 0/6] virtio/vhost cross-endian cleanup

[Greg Kurz]

On Tue, 16 Feb 2016 12:04:21 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 16, 2016 at 10:57:19AM +0100, Greg Kurz wrote:
> > On Wed, 10 Feb 2016 11:22:20 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:  
> > > On 05/02/2016 11:39, Greg Kurz wrote:  
> > > > This series brings some improvements to the cross-endian support in the
> > > > virtio and vhost code:
> > > > - use qemu_set_vnet_be() and qemu_set_vnet_le() directly from virtio-net,
> > > >   so that backend cross-endian capabilities benefit to both emulated and
> > > >   vhost accelerated devices
> > > > - optimize virtio_access_is_big_endian() for little-endian targets
> > > > - various cleanups
> > > > 
> > > > This v4 is a respin of the v3, taking into account the following fix:
> > > > 
> > > > commit cf0a628f6e81bfc9b7a944fa0b80c3594836df56
> > > > Author: Laurent Vivier <lvivier@redhat.com>
> > > > Date:   Wed Jan 13 20:26:25 2016 +0100
> > > > 
> > > >     net: set endianness on all backend devices
> > > > 
> > > > It impacts patches 1 and 2, that had already been reviewed.
> > > > 
> > > > Laurent, Cornelia,
> > > > 
> > > > Could you please ack/nack the changes ?
> > > > 
> > > > The series is based on Michael Tsirkin's tree:
> > > > 
> > > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> > > > 
> > > > Thanks.
> > > > 
> > > > --
> > > > Greg
> > > > 
> > > > ---
> > > > 
> > > > Greg Kurz (6):
> > > >       virtio-net: use the backend cross-endian capabilities
> > > >       vhost-net: revert support of cross-endian vnet headers
> > > >       virtio: move cross-endian helper to vhost
> > > >       vhost: move virtio 1.0 check to cross-endian helper
> > > >       vhost: simplify vhost_needs_vring_endian()
> > > >       virtio: optimize virtio_access_is_big_endian() for little-endian targets
> > > > 
> > > > 
> > > >  hw/net/vhost_net.c                |   41 +++-------------------
> > > >  hw/net/virtio-net.c               |   68 ++++++++++++++++++++++++++++++++++++-
> > > >  hw/virtio/vhost.c                 |   27 +++++++++++++--
> > > >  include/hw/virtio/virtio-access.h |   28 ++-------------
> > > >  include/hw/virtio/virtio-net.h    |    1 +
> > > >  5 files changed, 98 insertions(+), 67 deletions(-)
> > > >     
> > > 
> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > >   
> > 
> > Michael,
> > 
> > Laurent has posted this R-b according to your request on irc. Most patches in this
> > series have also been reviewed by Cornelia.
> > 
> > Is there something else that prevents inclusion ?
> > 
> > Thanks.  
> 
> Nothing - my pull request got delayed as I fixed
> a bug I have inadvertently intriduced.
> They will be in the next pull request.
> 

Great ! Thanks !

> > --
> > Greg  
>