[Qemu-devel] [PATCH v2 0/2] virtio-net/vhost-net: share cross-endian enablement

[Qemu-devel] [PATCH v2 0/2] virtio-net/vhost-net: share cross-endian enablement

[Greg Kurz]

Since QEMU 2.4.0, vhost-net uses the cross-endian support of TAP devices to
fix vnet headers. In fact, virtio-net can do the same instead of hackily
patching headers in virtio_net_hdr_swap().

This series moves the enablement of cross-endian support from vhost-net to
virtio-net: both vhost and full emulation can now benefit from it. Of course
we keep the current hack to fall back on when the backend doesn't support
cross-endian.

---

Greg Kurz (2):
      virtio-net: use the backend cross-endian capabilities
      Revert "vhost-net: tell tap backend about the vnet endianness"


 hw/net/vhost_net.c             |   33 +-------------------------
 hw/net/virtio-net.c            |   50 ++++++++++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-net.h |    1 +
 3 files changed, 50 insertions(+), 34 deletions(-)

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

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>
---
v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one
      place
---
 hw/net/virtio-net.c            |   43 ++++++++++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-net.h |    1 +
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a877614e3e7a..a10f7a0b4d28 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
     }
 }
 
+static int peer_has_vnet_hdr(VirtIONet *n);
+
+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 +186,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++) {
@@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
     }
 }
 
+static bool virtio_net_needs_hdr_swap(VirtIONet *n)
+{
+    /* virtio_needs_swap() is constant for fixed endian targets: call it
+     * first to filter them out without penalty.
+     */
+    return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap;
+}
+
 static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
                            const void *buf, size_t size)
 {
@@ -957,7 +993,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 (virtio_net_needs_hdr_swap(n)) {
+            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 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
                 error_report("virtio-net header incorrect");
                 exit(1);
             }
-            if (virtio_needs_swap(vdev)) {
+            if (virtio_net_needs_hdr_swap(n)) {
                 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 v2 2/2] 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>
---
v2: - display unsupported endiannes in error report
---
 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 d91b7b155ea7..fd7ea47fd38d 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;
@@ -205,27 +204,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)
 {
@@ -320,11 +298,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);
     }
@@ -332,7 +305,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++) {
@@ -354,8 +327,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;
 }
@@ -378,8 +349,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 a10f7a0b4d28..e109337fcf8d 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.
          */

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

[Cornelia Huck]

On Mon, 09 Nov 2015 18:41:33 +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. 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.
> 
> 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>
> ---
> v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one
>       place
> ---
>  hw/net/virtio-net.c            |   43 ++++++++++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-net.h |    1 +
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614e3e7a..a10f7a0b4d28 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>      }
>  }
> 
> +static int peer_has_vnet_hdr(VirtIONet *n);

Hm, you do not seem to use this?

> +
> +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 +186,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++) {
> @@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
>      }
>  }
> 
> +static bool virtio_net_needs_hdr_swap(VirtIONet *n)
> +{
> +    /* virtio_needs_swap() is constant for fixed endian targets: call it
> +     * first to filter them out without penalty.

What do you mean with 'constant' here? It still needs to retrieve the
feature bit from the device, no?

> +     */
> +    return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap;
> +}
> +
>  static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
>                             const void *buf, size_t size)
>  {
> @@ -957,7 +993,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 (virtio_net_needs_hdr_swap(n)) {
> +            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 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>                  error_report("virtio-net header incorrect");
>                  exit(1);
>              }
> -            if (virtio_needs_swap(vdev)) {
> +            if (virtio_net_needs_hdr_swap(n)) {
>                  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 v2 1/2] virtio-net: use the backend cross-endian capabilities

[Greg Kurz]

On Thu, 12 Nov 2015 18:52:55 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 09 Nov 2015 18:41:33 +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. 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.
> > 
> > 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>
> > ---
> > v2: - introduced virtio_net_needs_hdr_swap() to consolidate the logic in one
> >       place
> > ---
> >  hw/net/virtio-net.c            |   43 ++++++++++++++++++++++++++++++++++++++--
> >  include/hw/virtio/virtio-net.h |    1 +
> >  2 files changed, 42 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a877614e3e7a..a10f7a0b4d28 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -152,6 +152,33 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >      }
> >  }
> > 
> > +static int peer_has_vnet_hdr(VirtIONet *n);
> 
> Hm, you do not seem to use this?
> 

Oops I needed that in a earlier version of this patch and forgot to remove it...

> > +
> > +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 +186,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++) {
> > @@ -949,6 +977,14 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> >      }
> >  }
> > 
> > +static bool virtio_net_needs_hdr_swap(VirtIONet *n)
> > +{
> > +    /* virtio_needs_swap() is constant for fixed endian targets: call it
> > +     * first to filter them out without penalty.
> 
> What do you mean with 'constant' here? It still needs to retrieve the
> feature bit from the device, no?
> 

Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes"
series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little
endian targets but we still need to check the feature bit when the target is big
endian.

If I drop the call to virtio_needs_swap(), all targets will have the same
penalty. If I keep it, fixed little endian targets have no penalty but fixed
big endian targets get an extra check and bi-endian targets get two extra checks...

Not sure what to decide...

> > +     */
> > +    return virtio_needs_swap(VIRTIO_DEVICE(n)) && n->needs_vnet_hdr_swap;
> > +}
> > +
> >  static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> >                             const void *buf, size_t size)
> >  {
> > @@ -957,7 +993,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 (virtio_net_needs_hdr_swap(n)) {
> > +            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 +1206,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >                  error_report("virtio-net header incorrect");
> >                  exit(1);
> >              }
> > -            if (virtio_needs_swap(vdev)) {
> > +            if (virtio_net_needs_hdr_swap(n)) {
> >                  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 v2 1/2] virtio-net: use the backend cross-endian capabilities

[Cornelia Huck]

On Fri, 13 Nov 2015 09:26:26 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Thu, 12 Nov 2015 18:52:55 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Mon, 09 Nov 2015 18:41:33 +0100
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n)
> > > +{
> > > +    /* virtio_needs_swap() is constant for fixed endian targets: call it
> > > +     * first to filter them out without penalty.
> > 
> > What do you mean with 'constant' here? It still needs to retrieve the
> > feature bit from the device, no?
> > 
> 
> Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes"
> series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little
> endian targets but we still need to check the feature bit when the target is big
> endian.
> 
> If I drop the call to virtio_needs_swap(), all targets will have the same
> penalty. If I keep it, fixed little endian targets have no penalty but fixed
> big endian targets get an extra check and bi-endian targets get two extra checks...
> 
> Not sure what to decide...

The impact probably isn't too large, but improving one configuration
via penalizing others feels wrong.

Just check the bool value to make the decision?

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

[Greg Kurz]

On Fri, 13 Nov 2015 15:46:06 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 13 Nov 2015 09:26:26 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > On Thu, 12 Nov 2015 18:52:55 +0100
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > On Mon, 09 Nov 2015 18:41:33 +0100
> > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > > > +static bool virtio_net_needs_hdr_swap(VirtIONet *n)
> > > > +{
> > > > +    /* virtio_needs_swap() is constant for fixed endian targets: call it
> > > > +     * first to filter them out without penalty.
> > > 
> > > What do you mean with 'constant' here? It still needs to retrieve the
> > > feature bit from the device, no?
> > > 
> > 
> > Yes the comment is inaccurate... With the "virtio: cross-endian helpers fixes"
> > series, virtio_needs_swap() indeed resolves to { return 0; } for fixed little
> > endian targets but we still need to check the feature bit when the target is big
> > endian.
> > 
> > If I drop the call to virtio_needs_swap(), all targets will have the same
> > penalty. If I keep it, fixed little endian targets have no penalty but fixed
> > big endian targets get an extra check and bi-endian targets get two extra checks...
> > 
> > Not sure what to decide...
> 
> The impact probably isn't too large, but improving one configuration
> via penalizing others feels wrong.
> 
> Just check the bool value to make the decision?

Sounds fair. I'll drop the call to virtio_needs_swap() then.

Thanks.

--
Greg