[6/6] virtio: optimize virtio_access_is_big_endian() for little-endian targets

When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro
and the virtio_access_is_big_endian() helper to have a branchless fast path
in the virtio memory accessors for targets that don't switch endian.

This was considered as a strong requirement at the time.

Now we have added a runtime check for virtio 1.0, which ruins the benefit
of the virtio_access_is_big_endian() helper for always little-endian targets.

With this patch, always little-endian targets stop checking for virtio 1.0,
since the result is little-endian in all cases.

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

On 07/01/2016 12:32, Greg Kurz wrote:
> When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro
> and the virtio_access_is_big_endian() helper to have a branchless fast path
> in the virtio memory accessors for targets that don't switch endian.
> 
> This was considered as a strong requirement at the time.
> 
> Now we have added a runtime check for virtio 1.0, which ruins the benefit
> of the virtio_access_is_big_endian() helper for always little-endian targets.
> 
> With this patch, always little-endian targets stop checking for virtio 1.0,
> since the result is little-endian in all cases.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  include/hw/virtio/virtio-access.h |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index f1f12afe9089..fba4b4a4e1b2 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -19,10 +19,13 @@
>  
>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> +#if defined(TARGET_IS_BIENDIAN) || defined(TARGET_WORDS_BIGENDIAN)
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>          return false;
>      }
> +#endif


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

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

Laurent

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

> 
> 
> On 07/01/2016 12:32, Greg Kurz wrote:
> > When adding cross-endian support, we introduced the TARGET_IS_BIENDIAN macro
> > and the virtio_access_is_big_endian() helper to have a branchless fast path
> > in the virtio memory accessors for targets that don't switch endian.
> > 
> > This was considered as a strong requirement at the time.
> > 
> > Now we have added a runtime check for virtio 1.0, which ruins the benefit
> > of the virtio_access_is_big_endian() helper for always little-endian targets.
> > 
> > With this patch, always little-endian targets stop checking for virtio 1.0,
> > since the result is little-endian in all cases.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  include/hw/virtio/virtio-access.h |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> > index f1f12afe9089..fba4b4a4e1b2 100644
> > --- a/include/hw/virtio/virtio-access.h
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -19,10 +19,13 @@
> >  
> >  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
> >  {
> > +#if defined(TARGET_IS_BIENDIAN) || defined(TARGET_WORDS_BIGENDIAN)
> >      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> >          /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> >          return false;
> >      }
> > +#endif
> 
> 
> virtio_is_big_endian() checks VIRTIO_F_VERSION_1, so you don't need to
> check it here if TARGET_IS_BIENDIAN is defined.
> 
> > +
> >  #if defined(TARGET_IS_BIENDIAN)
> >      return virtio_is_big_endian(vdev);
> >  #elif defined(TARGET_WORDS_BIGENDIAN)
> > 
> > 
> So you can move virtio_vdev_has_feature() here.
> 
> Laurent
> 
> 

Indeed this is one step further and results in:

 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
+#if defined(TARGET_IS_BIENDIAN)
+    return virtio_is_big_endian(vdev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         /* Devices conforming to VIRTIO 1.0 or later are always LE. */
         return false;
     }
-#if defined(TARGET_IS_BIENDIAN)
-    return virtio_is_big_endian(vdev);
-#elif defined(TARGET_WORDS_BIGENDIAN)
     return true;
 #else
     return false;

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

Thanks for the suggestion !

--
Greg