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

[Qemu-devel] [PATCH v2 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 |   16 +++-------------
 2 files changed, 21 insertions(+), 17 deletions(-)

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/virtio/vhost.c                 |   17 +++++++++++++++--
 include/hw/virtio/virtio-access.h |   13 -------------
 2 files changed, 15 insertions(+), 15 deletions(-)

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/virtio/vhost.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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

[Qemu-devel] [PATCH v2 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, 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: - don't rename helper to virtio_is_big_endian_fast()
---
 include/hw/virtio/virtio-access.h |    3 +++
 1 file changed, 3 insertions(+)

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

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

[Michael S. Tsirkin]

On Thu, Dec 17, 2015 at 09:52:46AM +0100, Greg Kurz wrote:
> 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.

Breaks build:

  CC    mips64-softmmu/hw/mips/mips_malta.o
/home/mst/scm/qemu/hw/net/vhost_net.c: In function
‘vhost_net_set_vnet_endian’:
/home/mst/scm/qemu/hw/net/vhost_net.c:208:10: error: implicit
declaration of function ‘virtio_legacy_is_cross_endian’
[-Werror=implicit-function-declaration]
         (virtio_legacy_is_cross_endian(dev) &&
!virtio_is_big_endian(dev))) {
          ^
/home/mst/scm/qemu/hw/net/vhost_net.c:208:9: error: nested extern
declaration of ‘virtio_legacy_is_cross_endian’ [-Werror=nested-externs]
         (virtio_legacy_is_cross_endian(dev) &&
!virtio_is_big_endian(dev))) {
         ^
cc1: all warnings being treated as errors
/home/mst/scm/qemu/rules.mak:57: recipe for target 'hw/net/vhost_net.o'
failed
make[1]: *** [hw/net/vhost_net.o] Error 1
Makefile:186: recipe for target 'subdir-i386-softmmu' failed
make: *** [subdir-i386-softmmu] Error 2


please always build all architectures.

> ---
> 
> 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 |   16 +++-------------
>  2 files changed, 21 insertions(+), 17 deletions(-)

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

[Greg Kurz]

On Wed, 23 Dec 2015 15:47:00 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Dec 17, 2015 at 09:52:46AM +0100, Greg Kurz wrote:
> > 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.
> 
> Breaks build:
> 
>   CC    mips64-softmmu/hw/mips/mips_malta.o
> /home/mst/scm/qemu/hw/net/vhost_net.c: In function
> ‘vhost_net_set_vnet_endian’:
> /home/mst/scm/qemu/hw/net/vhost_net.c:208:10: error: implicit
> declaration of function ‘virtio_legacy_is_cross_endian’
> [-Werror=implicit-function-declaration]
>          (virtio_legacy_is_cross_endian(dev) &&
> !virtio_is_big_endian(dev))) {
>           ^
> /home/mst/scm/qemu/hw/net/vhost_net.c:208:9: error: nested extern
> declaration of ‘virtio_legacy_is_cross_endian’ [-Werror=nested-externs]
>          (virtio_legacy_is_cross_endian(dev) &&
> !virtio_is_big_endian(dev))) {
>          ^
> cc1: all warnings being treated as errors
> /home/mst/scm/qemu/rules.mak:57: recipe for target 'hw/net/vhost_net.o'
> failed
> make[1]: *** [hw/net/vhost_net.o] Error 1
> Makefile:186: recipe for target 'subdir-i386-softmmu' failed
> make: *** [subdir-i386-softmmu] Error 2
> 
> 
> please always build all architectures.
> 

Ok. I'll do so from now on.

> > ---
> > 
> > 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 |   16 +++-------------
> >  2 files changed, 21 insertions(+), 17 deletions(-)
> 

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

[Greg Kurz]

On Wed, 23 Dec 2015 17:28:23 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Wed, 23 Dec 2015 15:47:00 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Dec 17, 2015 at 09:52:46AM +0100, Greg Kurz wrote:
> > > 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.
> > 
> > Breaks build:
> > 
> >   CC    mips64-softmmu/hw/mips/mips_malta.o
> > /home/mst/scm/qemu/hw/net/vhost_net.c: In function
> > ‘vhost_net_set_vnet_endian’:
> > /home/mst/scm/qemu/hw/net/vhost_net.c:208:10: error: implicit
> > declaration of function ‘virtio_legacy_is_cross_endian’
> > [-Werror=implicit-function-declaration]
> >          (virtio_legacy_is_cross_endian(dev) &&
> > !virtio_is_big_endian(dev))) {
> >           ^
> > /home/mst/scm/qemu/hw/net/vhost_net.c:208:9: error: nested extern
> > declaration of ‘virtio_legacy_is_cross_endian’ [-Werror=nested-externs]
> >          (virtio_legacy_is_cross_endian(dev) &&
> > !virtio_is_big_endian(dev))) {
> >          ^
> > cc1: all warnings being treated as errors
> > /home/mst/scm/qemu/rules.mak:57: recipe for target 'hw/net/vhost_net.o'
> > failed
> > make[1]: *** [hw/net/vhost_net.o] Error 1
> > Makefile:186: recipe for target 'subdir-i386-softmmu' failed
> > make: *** [subdir-i386-softmmu] Error 2
> > 
> > 
> > please always build all architectures.
> > 
> 
> Ok. I'll do so from now on.
> 

The break isn't architecture related actually. It is because this series
depends on the "virtio-net/vhost-net: share cross-endian enablement" series
I had posted before... my bad. Since most of these series is cleanup of the
cross-endian code, I'll repost a single series with all the patches.

> > > ---
> > > 
> > > 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 |   16 +++-------------
> > >  2 files changed, 21 insertions(+), 17 deletions(-)
> > 
> 
> 

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

[Michael S. Tsirkin]

On Tue, Jan 05, 2016 at 08:19:22PM +0100, Greg Kurz wrote:
> On Wed, 23 Dec 2015 17:28:23 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > On Wed, 23 Dec 2015 15:47:00 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Dec 17, 2015 at 09:52:46AM +0100, Greg Kurz wrote:
> > > > 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.
> > > 
> > > Breaks build:
> > > 
> > >   CC    mips64-softmmu/hw/mips/mips_malta.o
> > > /home/mst/scm/qemu/hw/net/vhost_net.c: In function
> > > ‘vhost_net_set_vnet_endian’:
> > > /home/mst/scm/qemu/hw/net/vhost_net.c:208:10: error: implicit
> > > declaration of function ‘virtio_legacy_is_cross_endian’
> > > [-Werror=implicit-function-declaration]
> > >          (virtio_legacy_is_cross_endian(dev) &&
> > > !virtio_is_big_endian(dev))) {
> > >           ^
> > > /home/mst/scm/qemu/hw/net/vhost_net.c:208:9: error: nested extern
> > > declaration of ‘virtio_legacy_is_cross_endian’ [-Werror=nested-externs]
> > >          (virtio_legacy_is_cross_endian(dev) &&
> > > !virtio_is_big_endian(dev))) {
> > >          ^
> > > cc1: all warnings being treated as errors
> > > /home/mst/scm/qemu/rules.mak:57: recipe for target 'hw/net/vhost_net.o'
> > > failed
> > > make[1]: *** [hw/net/vhost_net.o] Error 1
> > > Makefile:186: recipe for target 'subdir-i386-softmmu' failed
> > > make: *** [subdir-i386-softmmu] Error 2
> > > 
> > > 
> > > please always build all architectures.
> > > 
> > 
> > Ok. I'll do so from now on.
> > 
> 
> The break isn't architecture related actually. It is because this series
> depends on the "virtio-net/vhost-net: share cross-endian enablement" series
> I had posted before... my bad. Since most of these series is cleanup of the
> cross-endian code,



Oh. And I thought it's the reverse order somehow.

> I'll repost a single series with all the patches.

That's probably best.

> > > > ---
> > > > 
> > > > 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 |   16 +++-------------
> > > >  2 files changed, 21 insertions(+), 17 deletions(-)
> > > 
> > 
> >