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

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

These patches had already been posted last year as two separate series:

https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg04166.html
https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg02827.html

I could not observe performance changes between guest and host on x86 with
an emulated virtio-net device.

---

Greg Kurz (6):
      virtio-net: use the backend cross-endian capabilities
      Revert "vhost-net: tell tap backend about the vnet endianness"
      virtio: drop the virtio_needs_swap() helper
      virtio: move cross-endian helper to vhost
      vhost: move virtio 1.0 check to cross-endian helper
      virtio: optimize virtio_access_is_big_endian() for little-endian targets


 hw/net/vhost_net.c                |   33 +------------------------------
 hw/net/virtio-net.c               |   40 +++++++++++++++++++++++++++++++++++--
 hw/virtio/vhost.c                 |   22 +++++++++++++++++---
 include/hw/virtio/virtio-access.h |   25 +++--------------------
 include/hw/virtio/virtio-net.h    |    1 +
 5 files changed, 61 insertions(+), 60 deletions(-)

[Qemu-devel] [PATCH 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. If the backend cannot support the
requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
paths.

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

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-net.h |    1 +
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a877614e3e7a..d4cc94ea5e55 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
     }
 }
 
+static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    NetClientState *peer = qemu_get_queue(n->nic)->peer;
+
+    if (virtio_net_started(n, status)) {
+        int r;
+
+        if (virtio_is_big_endian(vdev)) {
+            r = qemu_set_vnet_be(peer, true);
+        } else {
+            r = qemu_set_vnet_le(peer, true);
+        }
+
+        n->needs_vnet_hdr_swap = !!r;
+    } else if (virtio_net_started(n, vdev->status) &&
+               !virtio_net_started(n, status)) {
+        if (virtio_is_big_endian(vdev)) {
+            qemu_set_vnet_be(peer, false);
+        } else {
+            qemu_set_vnet_le(peer, false);
+        }
+    }
+}
+
 static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     int i;
     uint8_t queue_status;
 
+    virtio_net_vnet_status(n, status);
     virtio_net_vhost_status(n, status);
 
     for (i = 0; i < n->max_queues; i++) {
@@ -957,7 +983,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 = {
@@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
index f3cc25feca2b..27bc868fbc7d 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 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Greg Kurz]

This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.

Cross-endian is now configured by the core virtio-net code. We simply
fall back on full emulation if the net backend cannot support the
requested endianness for vnet headers.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/vhost_net.c  |   33 +--------------------------------
 hw/net/virtio-net.c |    7 +++++++
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 318c3e6ad213..0c7362b7a772 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -38,7 +38,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;
@@ -199,27 +198,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)
 {
@@ -308,11 +286,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
         goto err;
     }
 
-    r = vhost_net_set_vnet_endian(dev, ncs[0].peer, true);
-    if (r < 0) {
-        goto err;
-    }
-
     for (i = 0; i < total_queues; i++) {
         vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
     }
@@ -320,7 +293,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
     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++) {
@@ -342,8 +315,6 @@ err_start:
         fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
         fflush(stderr);
     }
-err_endian:
-    vhost_net_set_vnet_endian(dev, ncs[0].peer, false);
 err:
     return r;
 }
@@ -366,8 +337,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)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index d4cc94ea5e55..5a0ab6ad5bb5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -128,6 +128,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.
          */

[Qemu-devel] [PATCH 3/6] virtio: drop the virtio_needs_swap() helper

[Greg Kurz]

It is not used anymore.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/hw/virtio/virtio-access.h |    9 ---------
 1 file changed, 9 deletions(-)

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

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

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.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 de29968a7945..2e1e792d599e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -748,6 +748,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)
@@ -799,7 +812,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);
@@ -896,7 +909,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 5/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.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.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 2e1e792d599e..aef750df22ad 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -750,6 +750,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);
@@ -811,8 +814,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);
@@ -908,8 +910,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 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.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 include/hw/virtio/virtio-access.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index f1f12afe9089..fba4b4a4e1b2 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,10 +19,13 @@
 
 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
+#if defined(TARGET_IS_BIENDIAN) || 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;
     }
+#endif
+
 #if defined(TARGET_IS_BIENDIAN)
     return virtio_is_big_endian(vdev);
 #elif defined(TARGET_WORDS_BIGENDIAN)

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

[Laurent Vivier]

On 07/01/2016 12:32, Greg Kurz wrote:
> 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. If the backend cannot support the
> requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
> is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
> paths.
> 
> The current vhost-net code also tries to configure net backends. This will
> be no more needed and will be addressed in a subsequent patch.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-net.h |    1 +
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614e3e7a..d4cc94ea5e55 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>      }
>  }
>  
> +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> +
> +    if (virtio_net_started(n, status)) {
> +        int r;
> +
> +        if (virtio_is_big_endian(vdev)) {
> +            r = qemu_set_vnet_be(peer, true);
> +        } else {
> +            r = qemu_set_vnet_le(peer, true);
> +        }
> +
> +        n->needs_vnet_hdr_swap = !!r;
> +    } else if (virtio_net_started(n, vdev->status) &&
> +               !virtio_net_started(n, status)) {

Except if I miss something,

   "!virtio_net_started(n, status)" is always true in the case of
"if (virtio_net_started(n, status)) { } else ...".

> +        if (virtio_is_big_endian(vdev)) {
> +            qemu_set_vnet_be(peer, false);
> +        } else {
> +            qemu_set_vnet_le(peer, false);
> +        }
> +    }
> +}
> +
>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>      int i;
>      uint8_t queue_status;
>  
> +    virtio_net_vnet_status(n, status);
>      virtio_net_vhost_status(n, status);
>  
>      for (i = 0; i < n->max_queues; i++) {
> @@ -957,7 +983,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 = {
> @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
> index f3cc25feca2b..27bc868fbc7d 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,
> 
> 

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

[Greg Kurz]

On Thu, 7 Jan 2016 17:22:34 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > 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. If the backend cannot support the
> > requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
> > is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
> > paths.
> > 
> > The current vhost-net code also tries to configure net backends. This will
> > be no more needed and will be addressed in a subsequent patch.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
> >  include/hw/virtio/virtio-net.h |    1 +
> >  2 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a877614e3e7a..d4cc94ea5e55 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >      }
> >  }
> >  
> > +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> > +
> > +    if (virtio_net_started(n, status)) {
> > +        int r;
> > +
> > +        if (virtio_is_big_endian(vdev)) {
> > +            r = qemu_set_vnet_be(peer, true);
> > +        } else {
> > +            r = qemu_set_vnet_le(peer, true);
> > +        }
> > +
> > +        n->needs_vnet_hdr_swap = !!r;
> > +    } else if (virtio_net_started(n, vdev->status) &&
> > +               !virtio_net_started(n, status)) {
> 
> Except if I miss something,
> 
>    "!virtio_net_started(n, status)" is always true in the case of
> "if (virtio_net_started(n, status)) { } else ...".
> 

Of course... I'll fix it.

> > +        if (virtio_is_big_endian(vdev)) {
> > +            qemu_set_vnet_be(peer, false);
> > +        } else {
> > +            qemu_set_vnet_le(peer, false);
> > +        }
> > +    }
> > +}
> > +
> >  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >  {
> >      VirtIONet *n = VIRTIO_NET(vdev);
> > @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >      int i;
> >      uint8_t queue_status;
> >  
> > +    virtio_net_vnet_status(n, status);
> >      virtio_net_vhost_status(n, status);
> >  
> >      for (i = 0; i < n->max_queues; i++) {
> > @@ -957,7 +983,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 = {
> > @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
> > index f3cc25feca2b..27bc868fbc7d 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,
> > 
> > 
> 

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

[Laurent Vivier]

On 07/01/2016 12:32, Greg Kurz wrote:
> 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. If the backend cannot support the
> requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
> is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
> paths.
> 
> The current vhost-net code also tries to configure net backends. This will
> be no more needed and will be addressed in a subsequent patch.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-net.h |    1 +
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614e3e7a..d4cc94ea5e55 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>      }
>  }
>  
> +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> +
> +    if (virtio_net_started(n, status)) {
> +        int r;
> +
> +        if (virtio_is_big_endian(vdev)) {
> +            r = qemu_set_vnet_be(peer, true);
> +        } else {
> +            r = qemu_set_vnet_le(peer, true);
> +        }
> +
> +        n->needs_vnet_hdr_swap = !!r;
> +    } else if (virtio_net_started(n, vdev->status) &&
> +               !virtio_net_started(n, status)) {
> +        if (virtio_is_big_endian(vdev)) {
> +            qemu_set_vnet_be(peer, false);
> +        } else {
> +            qemu_set_vnet_le(peer, false);
> +        }
> +    }
> +}

Could you explain why check 'virtio_net_started(n, status)' and then
'virtio_net_started(n, vdev->status)' ?

Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in
the second case ?

Why don't you store the result (r) in the second case ?

>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>      int i;
>      uint8_t queue_status;
>  
> +    virtio_net_vnet_status(n, status);
>      virtio_net_vhost_status(n, status);
>  
>      for (i = 0; i < n->max_queues; i++) {
> @@ -957,7 +983,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 = {
> @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
> index f3cc25feca2b..27bc868fbc7d 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,
> 
> 

Re: [Qemu-devel] [PATCH 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Laurent Vivier]

On 07/01/2016 12:32, Greg Kurz wrote:
> This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.
> 
> Cross-endian is now configured by the core virtio-net code. We simply
> fall back on full emulation if the net backend cannot support the
> requested endianness for vnet headers.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/net/vhost_net.c  |   33 +--------------------------------
>  hw/net/virtio-net.c |    7 +++++++
>  2 files changed, 8 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 318c3e6ad213..0c7362b7a772 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -38,7 +38,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;
> @@ -199,27 +198,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)
>  {
> @@ -308,11 +286,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>          goto err;
>      }
>  
> -    r = vhost_net_set_vnet_endian(dev, ncs[0].peer, true);
> -    if (r < 0) {
> -        goto err;
> -    }
> -
>      for (i = 0; i < total_queues; i++) {
>          vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
>      }
> @@ -320,7 +293,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>      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++) {
> @@ -342,8 +315,6 @@ err_start:
>          fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
>          fflush(stderr);
>      }
> -err_endian:
> -    vhost_net_set_vnet_endian(dev, ncs[0].peer, false);
>  err:
>      return r;
>  }
> @@ -366,8 +337,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)
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index d4cc94ea5e55..5a0ab6ad5bb5 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -128,6 +128,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;
> +        }
> +

This is not part of the revert, perhaps this can go in PATCH 1/6 ?
In virtio_net_vnet_status() ?

>          /* Any packets outstanding? Purge them to avoid touching rings
>           * when vhost is running.
>           */
> 
> 

Re: [Qemu-devel] [PATCH 3/6] virtio: drop the virtio_needs_swap() helper

[Laurent Vivier]

On 07/01/2016 12:32, Greg Kurz wrote:
> It is not used anymore.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  include/hw/virtio/virtio-access.h |    9 ---------
>  1 file changed, 9 deletions(-)
> 
> 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
> -}
> -

I think you can move this to PATCH 1/6 too.

>  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
>  {
>  #ifdef HOST_WORDS_BIGENDIAN
> 
> 

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

[Laurent Vivier]

On 07/01/2016 12:32, 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.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.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 2e1e792d599e..aef750df22ad 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -750,6 +750,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);
> @@ -811,8 +814,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);
> @@ -908,8 +910,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);
> 
> 

IMHO, I think 4/6 and 5/6 can be merged as there is no change in the
behavior and they are only consolidating code.

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

[Laurent Vivier]

On 07/01/2016 12:32, Greg Kurz wrote:
> 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.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  include/hw/virtio/virtio-access.h |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index f1f12afe9089..fba4b4a4e1b2 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -19,10 +19,13 @@
>  
>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> +#if defined(TARGET_IS_BIENDIAN) || 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;
>      }
> +#endif


virtio_is_big_endian() checks VIRTIO_F_VERSION_1, so you don't need to
check it here if TARGET_IS_BIENDIAN is defined.

> +
>  #if defined(TARGET_IS_BIENDIAN)
>      return virtio_is_big_endian(vdev);
>  #elif defined(TARGET_WORDS_BIGENDIAN)
> 
> 
So you can move virtio_vdev_has_feature() here.

Laurent

Re: [Qemu-devel] [PATCH 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Greg Kurz]

On Thu, 7 Jan 2016 20:52:04 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.
> > 
> > Cross-endian is now configured by the core virtio-net code. We simply
> > fall back on full emulation if the net backend cannot support the
> > requested endianness for vnet headers.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/net/vhost_net.c  |   33 +--------------------------------
> >  hw/net/virtio-net.c |    7 +++++++
> >  2 files changed, 8 insertions(+), 32 deletions(-)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 318c3e6ad213..0c7362b7a772 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -38,7 +38,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;
> > @@ -199,27 +198,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)
> >  {
> > @@ -308,11 +286,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >          goto err;
> >      }
> >  
> > -    r = vhost_net_set_vnet_endian(dev, ncs[0].peer, true);
> > -    if (r < 0) {
> > -        goto err;
> > -    }
> > -
> >      for (i = 0; i < total_queues; i++) {
> >          vhost_net_set_vq_index(get_vhost_net(ncs[i].peer), i * 2);
> >      }
> > @@ -320,7 +293,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >      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++) {
> > @@ -342,8 +315,6 @@ err_start:
> >          fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
> >          fflush(stderr);
> >      }
> > -err_endian:
> > -    vhost_net_set_vnet_endian(dev, ncs[0].peer, false);
> >  err:
> >      return r;
> >  }
> > @@ -366,8 +337,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)
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index d4cc94ea5e55..5a0ab6ad5bb5 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -128,6 +128,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;
> > +        }
> > +
> 
> This is not part of the revert, perhaps this can go in PATCH 1/6 ?
> In virtio_net_vnet_status() ?
> 

Makes sense. I'll fix that.

> >          /* Any packets outstanding? Purge them to avoid touching rings
> >           * when vhost is running.
> >           */
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 3/6] virtio: drop the virtio_needs_swap() helper

[Greg Kurz]

On Thu, 7 Jan 2016 20:55:50 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > It is not used anymore.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  include/hw/virtio/virtio-access.h |    9 ---------
> >  1 file changed, 9 deletions(-)
> > 
> > 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
> > -}
> > -
> 
> I think you can move this to PATCH 1/6 too.
> 

I usually prefer to separate cleanup from functional changes, but
indeed this one is trivial... I'll do that and we will see if it
helps when I re-post.

> >  static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
> >  {
> >  #ifdef HOST_WORDS_BIGENDIAN
> > 
> > 
> 

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

[Greg Kurz]

On Thu, 7 Jan 2016 21:07:26 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 07/01/2016 12:32, 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.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.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 2e1e792d599e..aef750df22ad 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -750,6 +750,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);
> > @@ -811,8 +814,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);
> > @@ -908,8 +910,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);
> > 
> > 
> 
> IMHO, I think 4/6 and 5/6 can be merged as there is no change in the
> behavior and they are only consolidating code.
> 

Maybe but I'm not sure it is really needed to help acceptance.
FWIW Cornelia already reviewed both patches.

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

[Greg Kurz]

On Thu, 7 Jan 2016 21:25:19 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > 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.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  include/hw/virtio/virtio-access.h |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> > index f1f12afe9089..fba4b4a4e1b2 100644
> > --- a/include/hw/virtio/virtio-access.h
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -19,10 +19,13 @@
> >  
> >  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
> >  {
> > +#if defined(TARGET_IS_BIENDIAN) || 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;
> >      }
> > +#endif
> 
> 
> virtio_is_big_endian() checks VIRTIO_F_VERSION_1, so you don't need to
> check it here if TARGET_IS_BIENDIAN is defined.
> 
> > +
> >  #if defined(TARGET_IS_BIENDIAN)
> >      return virtio_is_big_endian(vdev);
> >  #elif defined(TARGET_WORDS_BIGENDIAN)
> > 
> > 
> So you can move virtio_vdev_has_feature() here.
> 
> Laurent
> 
> 

Indeed this is one step further and results in:

 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;

which looks nicer: slowest path (2 branches) for bi-endian targets,
slow path (1 branch) for big endian targets and fast path for little
endian targets.

Thanks for the suggestion !

--
Greg

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

[Cornelia Huck]

On Fri, 8 Jan 2016 10:21:40 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Thu, 7 Jan 2016 21:07:26 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:

> > IMHO, I think 4/6 and 5/6 can be merged as there is no change in the
> > behavior and they are only consolidating code.
> > 
> 
> Maybe but I'm not sure it is really needed to help acceptance.
> FWIW Cornelia already reviewed both patches.

I'd keep the two patches separate, as each is easier to understand on
its own IMO.

Re: [Qemu-devel] [PATCH 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Cornelia Huck]

On Thu, 07 Jan 2016 12:32:08 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.
> 
> Cross-endian is now configured by the core virtio-net code. We simply
> fall back on full emulation if the net backend cannot support the
> requested endianness for vnet headers.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/net/vhost_net.c  |   33 +--------------------------------
>  hw/net/virtio-net.c |    7 +++++++
>  2 files changed, 8 insertions(+), 32 deletions(-)

> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index d4cc94ea5e55..5a0ab6ad5bb5 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -128,6 +128,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."

s/./;/

> +                         "falling back on userspace virtio",

s/on/to/

?

> +                         virtio_is_big_endian(vdev) ? "BE" : "LE");
> +            return;
> +        }
> +
>          /* Any packets outstanding? Purge them to avoid touching rings
>           * when vhost is running.
>           */
> 
> 

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

[Laurent Vivier]

On 08/01/2016 11:07, Cornelia Huck wrote:
> On Fri, 8 Jan 2016 10:21:40 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
>> On Thu, 7 Jan 2016 21:07:26 +0100
>> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>>> IMHO, I think 4/6 and 5/6 can be merged as there is no change in the
>>> behavior and they are only consolidating code.
>>>
>>
>> Maybe but I'm not sure it is really needed to help acceptance.
>> FWIW Cornelia already reviewed both patches.
> 
> I'd keep the two patches separate, as each is easier to understand on
> its own IMO.
> 

Ok, no problem.

Laurent

Re: [Qemu-devel] [PATCH 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Greg Kurz]

On Fri, 8 Jan 2016 11:11:20 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Thu, 07 Jan 2016 12:32:08 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.
> > 
> > Cross-endian is now configured by the core virtio-net code. We simply
> > fall back on full emulation if the net backend cannot support the
> > requested endianness for vnet headers.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/net/vhost_net.c  |   33 +--------------------------------
> >  hw/net/virtio-net.c |    7 +++++++
> >  2 files changed, 8 insertions(+), 32 deletions(-)
> 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index d4cc94ea5e55..5a0ab6ad5bb5 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -128,6 +128,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."
> 
> s/./;/
> 

I'll fix that.

> > +                         "falling back on userspace virtio",
> 
> s/on/to/
> 
> ?
> 

I thought the same but...

http://idioms.thefreedictionary.com/fall+back+on

> > +                         virtio_is_big_endian(vdev) ? "BE" : "LE");
> > +            return;
> > +        }
> > +
> >          /* Any packets outstanding? Purge them to avoid touching rings
> >           * when vhost is running.
> >           */
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 2/6] Revert "vhost-net: tell tap backend about the vnet endianness"

[Cornelia Huck]

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

> On Fri, 8 Jan 2016 11:11:20 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Thu, 07 Jan 2016 12:32:08 +0100
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> > > +                         "falling back on userspace virtio",
> > 
> > s/on/to/
> > 
> > ?
> > 
> 
> I thought the same but...
> 
> http://idioms.thefreedictionary.com/fall+back+on

Then I'd vote for "onto" :)

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

[Greg Kurz]

On Thu, 7 Jan 2016 19:32:37 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> 

Sorry for the late answer to this one, I got diverted :)

> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > 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. If the backend cannot support the
> > requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
> > is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
> > paths.
> > 
> > The current vhost-net code also tries to configure net backends. This will
> > be no more needed and will be addressed in a subsequent patch.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
> >  include/hw/virtio/virtio-net.h |    1 +
> >  2 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a877614e3e7a..d4cc94ea5e55 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >      }
> >  }
> >  
> > +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> > +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> > +
> > +    if (virtio_net_started(n, status)) {
> > +        int r;
> > +
> > +        if (virtio_is_big_endian(vdev)) {
> > +            r = qemu_set_vnet_be(peer, true);
> > +        } else {
> > +            r = qemu_set_vnet_le(peer, true);
> > +        }
> > +
> > +        n->needs_vnet_hdr_swap = !!r;
> > +    } else if (virtio_net_started(n, vdev->status) &&
> > +               !virtio_net_started(n, status)) {
> > +        if (virtio_is_big_endian(vdev)) {
> > +            qemu_set_vnet_be(peer, false);
> > +        } else {
> > +            qemu_set_vnet_le(peer, false);
> > +        }
> > +    }
> > +}
> 
> Could you explain why check 'virtio_net_started(n, status)' and then
> 'virtio_net_started(n, vdev->status)' ?
> 

Before using the device, we need to inform the network backend about
the endianness to use when parsing vnet headers. We do this when the
driver activates the device (DRIVER_OK). This is the first check.

After using the device, we need to reset the network backend to the
default (guest native endianness), otherwise the guest may loose network
connectivity if it is rebooted into a different endianness. We do this
when the driver deactivates the device (no DRIVER_OK). The second check
ensures the device was active before: if we don't check that, the 'else'
branch would be executed each time the driver updates the status with
something not containing DRIVER_OK... :-\

> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in
> the second case ?
> 

"true" tells the backend to enforce the corresponding endianness.
"false" tells the backed to reset to the default (guest native endianness).

> Why don't you store the result (r) in the second case ?
> 

Because @needs_vnet_hdr_swap is only being used when the device is active.

Thank you for the time you spent on reviewing this series !

Bonne Annee !

--
Greg

> >  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >  {
> >      VirtIONet *n = VIRTIO_NET(vdev);
> > @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >      int i;
> >      uint8_t queue_status;
> >  
> > +    virtio_net_vnet_status(n, status);
> >      virtio_net_vhost_status(n, status);
> >  
> >      for (i = 0; i < n->max_queues; i++) {
> > @@ -957,7 +983,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 = {
> > @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
> > index f3cc25feca2b..27bc868fbc7d 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,
> > 
> > 
> 

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

[Laurent Vivier]

On 08/01/2016 15:19, Greg Kurz wrote:
> On Thu, 7 Jan 2016 19:32:37 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
>>
> 
> Sorry for the late answer to this one, I got diverted :)
> 
>>
>> On 07/01/2016 12:32, Greg Kurz wrote:
>>> 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. If the backend cannot support the
>>> requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
>>> is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
>>> paths.
>>>
>>> The current vhost-net code also tries to configure net backends. This will
>>> be no more needed and will be addressed in a subsequent patch.
>>>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>>  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
>>>  include/hw/virtio/virtio-net.h |    1 +
>>>  2 files changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index a877614e3e7a..d4cc94ea5e55 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>      }
>>>  }
>>>  
>>> +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
>>> +{
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
>>> +
>>> +    if (virtio_net_started(n, status)) {
>>> +        int r;
>>> +
>>> +        if (virtio_is_big_endian(vdev)) {
>>> +            r = qemu_set_vnet_be(peer, true);
>>> +        } else {
>>> +            r = qemu_set_vnet_le(peer, true);
>>> +        }
>>> +
>>> +        n->needs_vnet_hdr_swap = !!r;
>>> +    } else if (virtio_net_started(n, vdev->status) &&
>>> +               !virtio_net_started(n, status)) {
>>> +        if (virtio_is_big_endian(vdev)) {
>>> +            qemu_set_vnet_be(peer, false);
>>> +        } else {
>>> +            qemu_set_vnet_le(peer, false);
>>> +        }
>>> +    }
>>> +}
>>
>> Could you explain why check 'virtio_net_started(n, status)' and then
>> 'virtio_net_started(n, vdev->status)' ?
>>
> 
> Before using the device, we need to inform the network backend about
> the endianness to use when parsing vnet headers. We do this when the
> driver activates the device (DRIVER_OK). This is the first check.
> 
> After using the device, we need to reset the network backend to the
> default (guest native endianness), otherwise the guest may loose network
> connectivity if it is rebooted into a different endianness. We do this
> when the driver deactivates the device (no DRIVER_OK). The second check
> ensures the device was active before: if we don't check that, the 'else'
> branch would be executed each time the driver updates the status with
> something not containing DRIVER_OK... :-\
> 
>> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in
>> the second case ?
>>
> 
> "true" tells the backend to enforce the corresponding endianness.
> "false" tells the backed to reset to the default (guest native endianness).
> 
>> Why don't you store the result (r) in the second case ?
>>
> 
> Because @needs_vnet_hdr_swap is only being used when the device is active.
> 
> Thank you for the time you spent on reviewing this series !

Thank you for the details, it's clear now.
Perhaps this can be added in the commit log or in some comments ?

> Bonne Annee !

Bonne Année ;)
Laurent

> --
> Greg
> 
>>>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>>>  {
>>>      VirtIONet *n = VIRTIO_NET(vdev);
>>> @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>>>      int i;
>>>      uint8_t queue_status;
>>>  
>>> +    virtio_net_vnet_status(n, status);
>>>      virtio_net_vhost_status(n, status);
>>>  
>>>      for (i = 0; i < n->max_queues; i++) {
>>> @@ -957,7 +983,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 = {
>>> @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
>>> index f3cc25feca2b..27bc868fbc7d 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,
>>>
>>>
>>
> 

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

[Greg Kurz]

On Fri, 8 Jan 2016 16:25:18 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> 
> 
> On 08/01/2016 15:19, Greg Kurz wrote:
> > On Thu, 7 Jan 2016 19:32:37 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> > 
> > Sorry for the late answer to this one, I got diverted :)
> > 
> >>
> >> On 07/01/2016 12:32, Greg Kurz wrote:
> >>> 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. If the backend cannot support the
> >>> requested endiannes, we have to fall back on virtio_net_hdr_swap(): this
> >>> is recorded in the needs_vnet_hdr_swap flag, to be used in the TX and RX
> >>> paths.
> >>>
> >>> The current vhost-net code also tries to configure net backends. This will
> >>> be no more needed and will be addressed in a subsequent patch.
> >>>
> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>> ---
> >>>  hw/net/virtio-net.c            |   33 +++++++++++++++++++++++++++++++--
> >>>  include/hw/virtio/virtio-net.h |    1 +
> >>>  2 files changed, 32 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>> index a877614e3e7a..d4cc94ea5e55 100644
> >>> --- a/hw/net/virtio-net.c
> >>> +++ b/hw/net/virtio-net.c
> >>> @@ -152,6 +152,31 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>>      }
> >>>  }
> >>>  
> >>> +static void virtio_net_vnet_status(VirtIONet *n, uint8_t status)
> >>> +{
> >>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>> +    NetClientState *peer = qemu_get_queue(n->nic)->peer;
> >>> +
> >>> +    if (virtio_net_started(n, status)) {
> >>> +        int r;
> >>> +
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>> +            r = qemu_set_vnet_be(peer, true);
> >>> +        } else {
> >>> +            r = qemu_set_vnet_le(peer, true);
> >>> +        }
> >>> +
> >>> +        n->needs_vnet_hdr_swap = !!r;
> >>> +    } else if (virtio_net_started(n, vdev->status) &&
> >>> +               !virtio_net_started(n, status)) {
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>> +            qemu_set_vnet_be(peer, false);
> >>> +        } else {
> >>> +            qemu_set_vnet_le(peer, false);
> >>> +        }
> >>> +    }
> >>> +}
> >>
> >> Could you explain why check 'virtio_net_started(n, status)' and then
> >> 'virtio_net_started(n, vdev->status)' ?
> >>
> > 
> > Before using the device, we need to inform the network backend about
> > the endianness to use when parsing vnet headers. We do this when the
> > driver activates the device (DRIVER_OK). This is the first check.
> > 
> > After using the device, we need to reset the network backend to the
> > default (guest native endianness), otherwise the guest may loose network
> > connectivity if it is rebooted into a different endianness. We do this
> > when the driver deactivates the device (no DRIVER_OK). The second check
> > ensures the device was active before: if we don't check that, the 'else'
> > branch would be executed each time the driver updates the status with
> > something not containing DRIVER_OK... :-\
> > 
> >> Why qemu_set_vnet_[bl]e() use "true" in the first case and "false" in
> >> the second case ?
> >>
> > 
> > "true" tells the backend to enforce the corresponding endianness.
> > "false" tells the backed to reset to the default (guest native endianness).
> > 
> >> Why don't you store the result (r) in the second case ?
> >>
> > 
> > Because @needs_vnet_hdr_swap is only being used when the device is active.
> > 
> > Thank you for the time you spent on reviewing this series !
> 
> Thank you for the details, it's clear now.
> Perhaps this can be added in the commit log or in some comments ?
> 

I realized when writing the mail that this is non-trivial indeed. I'm
currently adding comments and updating the changelog :)

> > Bonne Annee !
> 
> Bonne Année ;)
> Laurent
> 
> > --
> > Greg
> > 
> >>>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >>>  {
> >>>      VirtIONet *n = VIRTIO_NET(vdev);
> >>> @@ -159,6 +184,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> >>>      int i;
> >>>      uint8_t queue_status;
> >>>  
> >>> +    virtio_net_vnet_status(n, status);
> >>>      virtio_net_vhost_status(n, status);
> >>>  
> >>>      for (i = 0; i < n->max_queues; i++) {
> >>> @@ -957,7 +983,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 = {
> >>> @@ -1167,7 +1196,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-net.h b/include/hw/virtio/virtio-net.h
> >>> index f3cc25feca2b..27bc868fbc7d 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,
> >>>
> >>>
> >>
> > 
>