Message ID | 20160107113232.10897.40641.stgit@bahia.huguette.org |
---|---|
State | New |
Headers | show |
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
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)
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(+)