[Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes

[Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes

[Greg Kurz]

This series tries to rework cross-endian helpers for better clarity.
It does not change behaviour, except perhaps patch 3/3 even if I could not
measure any performance gain.

---

Greg Kurz (3):
      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/virtio/vhost.c                 |   22 ++++++++++++---
 include/hw/virtio/virtio-access.h |   56 +++++++++++++++----------------------
 2 files changed, 41 insertions(+), 37 deletions(-)

[Qemu-devel] [PATCH 1/3] 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>
---
 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 8aec843c8ff3..ba1530619939 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -32,19 +32,6 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 #endif
 }
 
-static inline bool virtio_legacy_is_cross_endian(VirtIODevice *vdev)
-{
-#ifdef TARGET_IS_BIENDIAN
-#ifdef HOST_WORDS_BIGENDIAN
-    return !virtio_is_big_endian(vdev);
-#else
-    return virtio_is_big_endian(vdev);
-#endif
-#else
-    return false;
-#endif
-}
-
 static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 {
     if (virtio_access_is_big_endian(vdev)) {

[Qemu-devel] [PATCH 2/3] 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>
---
 hw/virtio/vhost.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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

[Qemu-devel] [PATCH 3/3] 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, fixed little-endian targets stop checking for virtio 1.0,
since the result is little-endian in all cases. The helper also gets renamed
so it is clear it is optimized for fast paths.

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

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index ba1530619939..ff013519b9dc 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -17,12 +17,15 @@
 #include "hw/virtio/virtio.h"
 #include "exec/address-spaces.h"
 
-static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
+static inline bool virtio_is_big_endian_fast(VirtIODevice *vdev)
 {
+#if defined(TARGET_IS_BIENDIAN) || defined(TARGET_WORDS_BIGENDIAN)
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         /* Devices conforming to VIRTIO 1.0 or later are always LE. */
         return false;
     }
+#endif
+
 #if defined(TARGET_IS_BIENDIAN)
     return virtio_is_big_endian(vdev);
 #elif defined(TARGET_WORDS_BIGENDIAN)
@@ -34,7 +37,7 @@ static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 
 static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return lduw_be_phys(&address_space_memory, pa);
     }
     return lduw_le_phys(&address_space_memory, pa);
@@ -42,7 +45,7 @@ static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 
 static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return ldl_be_phys(&address_space_memory, pa);
     }
     return ldl_le_phys(&address_space_memory, pa);
@@ -50,7 +53,7 @@ static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa)
 
 static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return ldq_be_phys(&address_space_memory, pa);
     }
     return ldq_le_phys(&address_space_memory, pa);
@@ -59,7 +62,7 @@ static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa)
 static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa,
                                    uint16_t value)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         stw_be_phys(&address_space_memory, pa, value);
     } else {
         stw_le_phys(&address_space_memory, pa, value);
@@ -69,7 +72,7 @@ static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa,
 static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
                                    uint32_t value)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         stl_be_phys(&address_space_memory, pa, value);
     } else {
         stl_le_phys(&address_space_memory, pa, value);
@@ -78,7 +81,7 @@ static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa,
 
 static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         stw_be_p(ptr, v);
     } else {
         stw_le_p(ptr, v);
@@ -87,7 +90,7 @@ static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v)
 
 static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         stl_be_p(ptr, v);
     } else {
         stl_le_p(ptr, v);
@@ -96,7 +99,7 @@ static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
 
 static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         stq_be_p(ptr, v);
     } else {
         stq_le_p(ptr, v);
@@ -105,7 +108,7 @@ static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v)
 
 static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return lduw_be_p(ptr);
     } else {
         return lduw_le_p(ptr);
@@ -114,7 +117,7 @@ static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr)
 
 static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return ldl_be_p(ptr);
     } else {
         return ldl_le_p(ptr);
@@ -123,7 +126,7 @@ static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr)
 
 static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr)
 {
-    if (virtio_access_is_big_endian(vdev)) {
+    if (virtio_is_big_endian_fast(vdev)) {
         return ldq_be_p(ptr);
     } else {
         return ldq_le_p(ptr);
@@ -133,18 +136,18 @@ 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;
+    return virtio_is_big_endian_fast(vdev) ? false : true;
 #else
-    return virtio_access_is_big_endian(vdev) ? true : false;
+    return virtio_is_big_endian_fast(vdev) ? true : false;
 #endif
 }
 
 static inline uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-    return virtio_access_is_big_endian(vdev) ? s : bswap16(s);
+    return virtio_is_big_endian_fast(vdev) ? s : bswap16(s);
 #else
-    return virtio_access_is_big_endian(vdev) ? bswap16(s) : s;
+    return virtio_is_big_endian_fast(vdev) ? bswap16(s) : s;
 #endif
 }
 
@@ -156,9 +159,9 @@ static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s)
 static inline uint32_t virtio_tswap32(VirtIODevice *vdev, uint32_t s)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-    return virtio_access_is_big_endian(vdev) ? s : bswap32(s);
+    return virtio_is_big_endian_fast(vdev) ? s : bswap32(s);
 #else
-    return virtio_access_is_big_endian(vdev) ? bswap32(s) : s;
+    return virtio_is_big_endian_fast(vdev) ? bswap32(s) : s;
 #endif
 }
 
@@ -170,9 +173,9 @@ static inline void virtio_tswap32s(VirtIODevice *vdev, uint32_t *s)
 static inline uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s)
 {
 #ifdef HOST_WORDS_BIGENDIAN
-    return virtio_access_is_big_endian(vdev) ? s : bswap64(s);
+    return virtio_is_big_endian_fast(vdev) ? s : bswap64(s);
 #else
-    return virtio_access_is_big_endian(vdev) ? bswap64(s) : s;
+    return virtio_is_big_endian_fast(vdev) ? bswap64(s) : s;
 #endif
 }
 

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

[Cornelia Huck]

On Mon, 09 Nov 2015 18:58:16 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> 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>
> ---
>  hw/virtio/vhost.c                 |   17 +++++++++++++++--
>  include/hw/virtio/virtio-access.h |   13 -------------
>  2 files changed, 15 insertions(+), 15 deletions(-)

That's on top of your vnet header cleanup, right? With that in mind:

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

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

[Cornelia Huck]

On Mon, 09 Nov 2015 18:58:25 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> 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>
> ---
>  hw/virtio/vhost.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

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

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

[Cornelia Huck]

On Mon, 09 Nov 2015 18:58:34 +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, fixed little-endian targets stop checking for virtio 1.0,
> since the result is little-endian in all cases. 

So always-LE gets optimized, while always-BE and bi-endian stay the same?

(Is there a measurable impact?)

> The helper also gets renamed
> so it is clear it is optimized for fast paths.

Even if it isn't actually 'fast' on anything other than fixed-LE?
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  include/hw/virtio/virtio-access.h |   43 ++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 20 deletions(-)

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

[Greg Kurz]

On Thu, 12 Nov 2015 19:00:54 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Mon, 09 Nov 2015 18:58:16 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> 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>
> > ---
> >  hw/virtio/vhost.c                 |   17 +++++++++++++++--
> >  include/hw/virtio/virtio-access.h |   13 -------------
> >  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> That's on top of your vnet header cleanup, right? With that in mind:
> 

Yes.

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

Thanks for your time !

--
Greg

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

[Greg Kurz]

On Thu, 12 Nov 2015 19:08:59 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 09 Nov 2015 18:58:34 +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, fixed little-endian targets stop checking for virtio 1.0,
> > since the result is little-endian in all cases. 
> 
> So always-LE gets optimized, while always-BE and bi-endian stay the same?
> 

Yes.

> (Is there a measurable impact?)
> 

I tried to measure using iperf between host and guest but I could not
find any significant change... do you think about another test I could
try ?

> > The helper also gets renamed
> > so it is clear it is optimized for fast paths.
> 
> Even if it isn't actually 'fast' on anything other than fixed-LE?

Yes this is definitely a fixed-LE only optimization... should I drop the name
change and add a comment instead ?

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

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

[Cornelia Huck]

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

> On Thu, 12 Nov 2015 19:08:59 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Mon, 09 Nov 2015 18:58:34 +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, fixed little-endian targets stop checking for virtio 1.0,
> > > since the result is little-endian in all cases. 
> > 
> > So always-LE gets optimized, while always-BE and bi-endian stay the same?
> > 
> 
> Yes.
> 
> > (Is there a measurable impact?)
> > 
> 
> I tried to measure using iperf between host and guest but I could not
> find any significant change... do you think about another test I could
> try ?

My hunch is that the impact is small anyway.

> 
> > > The helper also gets renamed
> > > so it is clear it is optimized for fast paths.
> > 
> > Even if it isn't actually 'fast' on anything other than fixed-LE?
> 
> Yes this is definitely a fixed-LE only optimization... should I drop the name
> change and add a comment instead ?

I think that would be better as it does not raise expectations :)

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

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

[Greg Kurz]

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

> On Fri, 13 Nov 2015 10:28:31 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > On Thu, 12 Nov 2015 19:08:59 +0100
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > On Mon, 09 Nov 2015 18:58:34 +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, fixed little-endian targets stop checking for virtio 1.0,
> > > > since the result is little-endian in all cases. 
> > > 
> > > So always-LE gets optimized, while always-BE and bi-endian stay the same?
> > > 
> > 
> > Yes.
> > 
> > > (Is there a measurable impact?)
> > > 
> > 
> > I tried to measure using iperf between host and guest but I could not
> > find any significant change... do you think about another test I could
> > try ?
> 
> My hunch is that the impact is small anyway.
> 

Yeah... even when running TCG I don't see any difference.

> > 
> > > > The helper also gets renamed
> > > > so it is clear it is optimized for fast paths.
> > > 
> > > Even if it isn't actually 'fast' on anything other than fixed-LE?
> > 
> > Yes this is definitely a fixed-LE only optimization... should I drop the name
> > change and add a comment instead ?
> 
> I think that would be better as it does not raise expectations :)
> 

Ok I'll revert to the original name then.

Thanks for the review.

--
Greg

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