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

[Qemu-devel] [PATCH v2 0/5] 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 v2 addresses Laurent Vivier's comments.

---

Greg Kurz (5):
      virtio-net: use the backend cross-endian capabilities
      Revert "vhost-net: tell tap backend about the vnet endianness"
      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               |   49 +++++++++++++++++++++++++++++++++++--
 hw/virtio/vhost.c                 |   22 ++++++++++++++---
 include/hw/virtio/virtio-access.h |   28 ++-------------------
 include/hw/virtio/virtio-net.h    |    1 +
 5 files changed, 70 insertions(+), 63 deletions(-)

[Qemu-devel] [PATCH v2 1/5] 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 loose 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.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v2:
- dropped useless check in the 'else if' branch in virtio_net_vnet_status()
- merged virtio_net_vhost_status() change from patch 2
- use semicolon in "backend does no support..." error message
- merged patch 3 (drop the virtio_needs_swap() helper)
- provided some more details in changelog and comments
---
 hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-access.h |    9 -------
 include/hw/virtio/virtio-net.h    |    1 +
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a877614e3e7a..497fb7119a08 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.
          */
@@ -152,6 +159,40 @@ 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;
+
+        /* 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.
+         */
+        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)) {
+        /* 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.
+         */
+        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 +200,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 +999,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 +1212,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 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 v2 1/5] virtio-net: use the backend cross-endian capabilities

[Cornelia Huck]

On Mon, 11 Jan 2016 17:12:21 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> 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 (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 loose network connectivity if rebooted into a

s/loose/lose/

> 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.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

(...)

> +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;
> +
> +        /* 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.
> +         */
> +        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)) {
> +        /* 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

here again

> +         * endianness.
> +         */
> +        if (virtio_is_big_endian(vdev)) {
> +            qemu_set_vnet_be(peer, false);
> +        } else {
> +            qemu_set_vnet_le(peer, false);
> +        }
> +    }
> +}
> +

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

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

[Laurent Vivier]

On 11/01/2016 17:12, 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 (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 loose 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.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> v2:
> - dropped useless check in the 'else if' branch in virtio_net_vnet_status()
> - merged virtio_net_vhost_status() change from patch 2
> - use semicolon in "backend does no support..." error message
> - merged patch 3 (drop the virtio_needs_swap() helper)
> - provided some more details in changelog and comments
> ---
>  hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-access.h |    9 -------
>  include/hw/virtio/virtio-net.h    |    1 +
>  3 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614e3e7a..497fb7119a08 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.
>           */
> @@ -152,6 +159,40 @@ 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;
> +
> +        /* 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.
> +         */
> +        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)) {
> +        /* 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.
> +         */
> +        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 +200,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 +999,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 +1212,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 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,
> 
> 

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

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

[Laurent Vivier]

On 11/01/2016 17:12, 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 (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 loose 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.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> v2:
> - dropped useless check in the 'else if' branch in virtio_net_vnet_status()
> - merged virtio_net_vhost_status() change from patch 2
> - use semicolon in "backend does no support..." error message
> - merged patch 3 (drop the virtio_needs_swap() helper)
> - provided some more details in changelog and comments
> ---
>  hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-access.h |    9 -------
>  include/hw/virtio/virtio-net.h    |    1 +
>  3 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614e3e7a..497fb7119a08 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.
>           */
> @@ -152,6 +159,40 @@ 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;
> +
> +        /* 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.
> +         */
> +        if (virtio_is_big_endian(vdev)) {
> +            r = qemu_set_vnet_be(peer, true);
> +        } else {
> +            r = qemu_set_vnet_le(peer, true);
> +        }

If endianess of the guest and the virtio device is the same, but r is <
0 (-ENOSYS or -EINVAL) you will badly swap header (and disable vhost).

I think you need something like this to fall back to the old method:

        if (r < 0) {
#ifdef HOST_WORDS_BIGENDIAN
            r = virtio_access_is_big_endian(vdev) ? false : true;
#else
            r = virtio_access_is_big_endian(vdev) ? true : false;
#endif
        }


But...

> +        n->needs_vnet_hdr_swap = !!r;
> +    } 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
> +         * loose network connectivity if it is rebooted into a different
> +         * endianness.
> +         */
> +        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 +200,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 +999,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);
> +        }

... this will change the behavior here, as before it was not
conditional. Why ?

>          iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
>      } else {
>          struct virtio_net_hdr hdr = {
> @@ -1167,7 +1212,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 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 v2 1/5] virtio-net: use the backend cross-endian capabilities

[Greg Kurz]

On Tue, 12 Jan 2016 18:24:28 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 11/01/2016 17:12, 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 (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 loose 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.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > v2:
> > - dropped useless check in the 'else if' branch in virtio_net_vnet_status()
> > - merged virtio_net_vhost_status() change from patch 2
> > - use semicolon in "backend does no support..." error message
> > - merged patch 3 (drop the virtio_needs_swap() helper)
> > - provided some more details in changelog and comments
> > ---
> >  hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
> >  include/hw/virtio/virtio-access.h |    9 -------
> >  include/hw/virtio/virtio-net.h    |    1 +
> >  3 files changed, 48 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a877614e3e7a..497fb7119a08 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.
> >           */
> > @@ -152,6 +159,40 @@ 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;
> > +
> > +        /* 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.
> > +         */
> > +        if (virtio_is_big_endian(vdev)) {
> > +            r = qemu_set_vnet_be(peer, true);
> > +        } else {
> > +            r = qemu_set_vnet_le(peer, true);
> > +        }  
> 
> If endianess of the guest and the virtio device is the same, but r is <
> 0 (-ENOSYS or -EINVAL) you will badly swap header (and disable vhost).
> 

This can only happen if the endianness of the host is not the same as the
endianness of the device. In this case (we usually call cross-endian) the
vnet headers must be byteswapped but the backend cannot handle it. This
has two consequences:
- vhost cannot be used since it requires the backend to support cross-endian
  vnet headers, so we fallback onto full emulation in QEMU
- the emulation code must byteswap vnet headers

> I think you need something like this to fall back to the old method:
> 
>         if (r < 0) {
> #ifdef HOST_WORDS_BIGENDIAN
>             r = virtio_access_is_big_endian(vdev) ? false : true;
> #else
>             r = virtio_access_is_big_endian(vdev) ? true : false;
> #endif
>         }
> 
> 
> But...
> 
> > +        n->needs_vnet_hdr_swap = !!r;
> > +    } 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
> > +         * loose network connectivity if it is rebooted into a different
> > +         * endianness.
> > +         */
> > +        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 +200,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 +999,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);
> > +        }  
> 
> ... this will change the behavior here, as before it was not
> conditional. Why ?
> 

This is what this patch is all about as described above.

> >          iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
> >      } else {
> >          struct virtio_net_hdr hdr = {
> > @@ -1167,7 +1212,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 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 v2 1/5] virtio-net: use the backend cross-endian capabilities

[Laurent Vivier]

On 13/01/2016 10:06, Greg Kurz wrote:
> On Tue, 12 Jan 2016 18:24:28 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 11/01/2016 17:12, 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 (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 loose 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.
>>>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>> v2:
>>> - dropped useless check in the 'else if' branch in virtio_net_vnet_status()
>>> - merged virtio_net_vhost_status() change from patch 2
>>> - use semicolon in "backend does no support..." error message
>>> - merged patch 3 (drop the virtio_needs_swap() helper)
>>> - provided some more details in changelog and comments
>>> ---
>>>  hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
>>>  include/hw/virtio/virtio-access.h |    9 -------
>>>  include/hw/virtio/virtio-net.h    |    1 +
>>>  3 files changed, 48 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index a877614e3e7a..497fb7119a08 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.
>>>           */
>>> @@ -152,6 +159,40 @@ 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;
>>> +
>>> +        /* 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.
>>> +         */
>>> +        if (virtio_is_big_endian(vdev)) {
>>> +            r = qemu_set_vnet_be(peer, true);
>>> +        } else {
>>> +            r = qemu_set_vnet_le(peer, true);
>>> +        }  
>>
>> If endianess of the guest and the virtio device is the same, but r is <
>> 0 (-ENOSYS or -EINVAL) you will badly swap header (and disable vhost).
>>
> 
> This can only happen if the endianness of the host is not the same as the

OK, you're right, I was studying sources without commit:

052bd52 net: don't set native endianness

Sorry for the noise...

> endianness of the device. In this case (we usually call cross-endian) the
> vnet headers must be byteswapped but the backend cannot handle it. This
> has two consequences:
> - vhost cannot be used since it requires the backend to support cross-endian
>   vnet headers, so we fallback onto full emulation in QEMU
> - the emulation code must byteswap vnet headers
> 
>> I think you need something like this to fall back to the old method:
>>
>>         if (r < 0) {
>> #ifdef HOST_WORDS_BIGENDIAN
>>             r = virtio_access_is_big_endian(vdev) ? false : true;
>> #else
>>             r = virtio_access_is_big_endian(vdev) ? true : false;
>> #endif
>>         }
>>
>>
>> But...
>>
>>> +        n->needs_vnet_hdr_swap = !!r;
>>> +    } 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
>>> +         * loose network connectivity if it is rebooted into a different
>>> +         * endianness.
>>> +         */
>>> +        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 +200,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 +999,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);
>>> +        }  
>>
>> ... this will change the behavior here, as before it was not
>> conditional. Why ?
>>
> 
> This is what this patch is all about as described above.
> 
>>>          iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
>>>      } else {
>>>          struct virtio_net_hdr hdr = {
>>> @@ -1167,7 +1212,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 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 v2 1/5] virtio-net: use the backend cross-endian capabilities

[Greg Kurz]

On Wed, 13 Jan 2016 10:53:11 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 13/01/2016 10:06, Greg Kurz wrote:
> > On Tue, 12 Jan 2016 18:24:28 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 11/01/2016 17:12, 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 (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 loose 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.
> >>>
> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>> ---
> >>> v2:
> >>> - dropped useless check in the 'else if' branch in virtio_net_vnet_status()
> >>> - merged virtio_net_vhost_status() change from patch 2
> >>> - use semicolon in "backend does no support..." error message
> >>> - merged patch 3 (drop the virtio_needs_swap() helper)
> >>> - provided some more details in changelog and comments
> >>> ---
> >>>  hw/net/virtio-net.c               |   49 +++++++++++++++++++++++++++++++++++--
> >>>  include/hw/virtio/virtio-access.h |    9 -------
> >>>  include/hw/virtio/virtio-net.h    |    1 +
> >>>  3 files changed, 48 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>> index a877614e3e7a..497fb7119a08 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.
> >>>           */
> >>> @@ -152,6 +159,40 @@ 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;
> >>> +
> >>> +        /* 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.
> >>> +         */
> >>> +        if (virtio_is_big_endian(vdev)) {
> >>> +            r = qemu_set_vnet_be(peer, true);
> >>> +        } else {
> >>> +            r = qemu_set_vnet_le(peer, true);
> >>> +        }    
> >>
> >> If endianess of the guest and the virtio device is the same, but r is <
> >> 0 (-ENOSYS or -EINVAL) you will badly swap header (and disable vhost).
> >>  
> > 
> > This can only happen if the endianness of the host is not the same as the  
> 
> OK, you're right, I was studying sources without commit:
> 
> 052bd52 net: don't set native endianness
> 
> Sorry for the noise...
> 

No problem. Thank you again for your time !


> > endianness of the device. In this case (we usually call cross-endian) the
> > vnet headers must be byteswapped but the backend cannot handle it. This
> > has two consequences:
> > - vhost cannot be used since it requires the backend to support cross-endian
> >   vnet headers, so we fallback onto full emulation in QEMU
> > - the emulation code must byteswap vnet headers
> >   
> >> I think you need something like this to fall back to the old method:
> >>
> >>         if (r < 0) {
> >> #ifdef HOST_WORDS_BIGENDIAN
> >>             r = virtio_access_is_big_endian(vdev) ? false : true;
> >> #else
> >>             r = virtio_access_is_big_endian(vdev) ? true : false;
> >> #endif
> >>         }
> >>
> >>
> >> But...
> >>  
> >>> +        n->needs_vnet_hdr_swap = !!r;
> >>> +    } 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
> >>> +         * loose network connectivity if it is rebooted into a different
> >>> +         * endianness.
> >>> +         */
> >>> +        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 +200,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 +999,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);
> >>> +        }    
> >>
> >> ... this will change the behavior here, as before it was not
> >> conditional. Why ?
> >>  
> > 
> > This is what this patch is all about as described above.
> >   
> >>>          iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
> >>>      } else {
> >>>          struct virtio_net_hdr hdr = {
> >>> @@ -1167,7 +1212,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 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 v2 2/5] Revert "vhost-net: tell tap backend about the vnet endianness"

[Greg Kurz]

This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.

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

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v2:
- moved changes not belonging to the revert to patch 1
- updated changelog accordingly
---
 hw/net/vhost_net.c |   33 +--------------------------------
 1 file changed, 1 insertion(+), 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)

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

[Laurent Vivier]

On 11/01/2016 17:12, Greg Kurz wrote:
> This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.
> 
> Cross-endian is now handled by the core virtio-net code.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> v2:
> - moved changes not belonging to the revert to patch 1
> - updated changelog accordingly
> ---
>  hw/net/vhost_net.c |   33 +--------------------------------
>  1 file changed, 1 insertion(+), 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)
> 
> 
Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[Qemu-devel] [PATCH v2 3/5] 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)) {

Re: [Qemu-devel] [PATCH v2 3/5] virtio: move cross-endian helper to vhost

[Laurent Vivier]

On 11/01/2016 17:13, Greg Kurz wrote:
> 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)) {
> 
> 
Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[Qemu-devel] [PATCH v2 4/5] 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);

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

[Laurent Vivier]

On 11/01/2016 17:13, 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);

I'm wondering if in term of performance you can remove the call of
virtio_vdev_has_feature() as it is done in virtio_is_big_endian().

> @@ -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);
> 
> 

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

[Greg Kurz]

On Tue, 12 Jan 2016 01:23:56 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 11/01/2016 17:13, 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);  
> 
> I'm wondering if in term of performance you can remove the call of
> virtio_vdev_has_feature() as it is done in virtio_is_big_endian().
> 

We cannot do this: in the case of a big endian host and a virtio 1.0 device,
virtio_is_big_endian() would return false and vhost_needs_vring_endian()
would return true. This is wrong: the byteswapping for virtio 1.0 is already
handled in the linux vhost driver since it has the feature bits. Only legacy
devices may need to call vhost_virtqueue_set_vring_endian_legacy() in a
cross-endian situation.

This isn't really a performance issue since this function is never called on
a hot path.

But indeed, your comment raises a concern about clarity. After the the call
to virtio_vdev_has_feature(), we are only caring for legacy devices: 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.

We simply need to open code the device_endian checking instead of calling
virtio_is_big_endian(). Like this:

/* Only legacy devices offered to an endian ambivalent guest may
 * face a cross-endian situation. All other cases return false.
 */
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 vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
#else
    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
#endif
#else
    return false;
#endif
}

I'll make it a separate patch.

Thanks !

--
Greg

> > @@ -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 v2 5/5] 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>
---
v2:
- simplified the logic as suggested by Laurent Vivier
---
 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 v2 5/5] virtio: optimize virtio_access_is_big_endian() for little-endian targets

[Cornelia Huck]

On Mon, 11 Jan 2016 17:18:06 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> 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>
> ---
> v2:
> - simplified the logic as suggested by Laurent Vivier
> ---
>  include/hw/virtio/virtio-access.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

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

[Laurent Vivier]

On 11/01/2016 17:18, 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>
> ---
> v2:
> - simplified the logic as suggested by Laurent Vivier
> ---
>  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;
> 
> 
Reviewed-by: Laurent Vivier <lvivier@redhat.com>

[Qemu-devel] [PATCH v2 0/5] 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 v2 addresses Laurent Vivier's comments.

---

Greg Kurz (5):
      virtio-net: use the backend cross-endian capabilities
      Revert "vhost-net: tell tap backend about the vnet endianness"
      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               |   49 +++++++++++++++++++++++++++++++++++--
 hw/virtio/vhost.c                 |   22 ++++++++++++++---
 include/hw/virtio/virtio-access.h |   28 ++-------------------
 include/hw/virtio/virtio-net.h    |    1 +
 5 files changed, 70 insertions(+), 63 deletions(-)

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

[Greg Kurz]

This reverts commit 5be7d9f1b1452613b95c6ba70b8d7ad3d0797991.

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

- moved changes not belonging to the revert to patch 1
- updated changelog accordingly

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/net/vhost_net.c |   33 +--------------------------------
 1 file changed, 1 insertion(+), 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)