* [Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes
@ 2015-11-09 17:58 Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost Greg Kurz
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Greg Kurz @ 2015-11-09 17:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost
2015-11-09 17:58 [Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes Greg Kurz
@ 2015-11-09 17:58 ` Greg Kurz
2015-11-12 18:00 ` Cornelia Huck
2015-11-09 17:58 ` [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets Greg Kurz
2 siblings, 1 reply; 11+ messages in thread
From: Greg Kurz @ 2015-11-09 17:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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)) {
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper
2015-11-09 17:58 [Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost Greg Kurz
@ 2015-11-09 17:58 ` Greg Kurz
2015-11-12 18:03 ` Cornelia Huck
2015-11-09 17:58 ` [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets Greg Kurz
2 siblings, 1 reply; 11+ messages in thread
From: Greg Kurz @ 2015-11-09 17:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
2015-11-09 17:58 [Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper Greg Kurz
@ 2015-11-09 17:58 ` Greg Kurz
2015-11-12 18:08 ` Cornelia Huck
2 siblings, 1 reply; 11+ messages in thread
From: Greg Kurz @ 2015-11-09 17:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost
2015-11-09 17:58 ` [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost Greg Kurz
@ 2015-11-12 18:00 ` Cornelia Huck
2015-11-13 8:28 ` Greg Kurz
0 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2015-11-12 18:00 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, Michael S. Tsirkin
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper
2015-11-09 17:58 ` [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper Greg Kurz
@ 2015-11-12 18:03 ` Cornelia Huck
0 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2015-11-12 18:03 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, Michael S. Tsirkin
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
2015-11-09 17:58 ` [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets Greg Kurz
@ 2015-11-12 18:08 ` Cornelia Huck
2015-11-13 9:28 ` Greg Kurz
0 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2015-11-12 18:08 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, Michael S. Tsirkin
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(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost
2015-11-12 18:00 ` Cornelia Huck
@ 2015-11-13 8:28 ` Greg Kurz
0 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2015-11-13 8:28 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-devel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
2015-11-12 18:08 ` Cornelia Huck
@ 2015-11-13 9:28 ` Greg Kurz
2015-11-13 14:42 ` Cornelia Huck
0 siblings, 1 reply; 11+ messages in thread
From: Greg Kurz @ 2015-11-13 9:28 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-devel, Michael S. Tsirkin
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(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
2015-11-13 9:28 ` Greg Kurz
@ 2015-11-13 14:42 ` Cornelia Huck
2015-11-13 15:25 ` Greg Kurz
0 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2015-11-13 14:42 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, Michael S. Tsirkin
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(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets
2015-11-13 14:42 ` Cornelia Huck
@ 2015-11-13 15:25 ` Greg Kurz
0 siblings, 0 replies; 11+ messages in thread
From: Greg Kurz @ 2015-11-13 15:25 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-devel, Michael S. Tsirkin
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(-)
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-11-13 15:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-09 17:58 [Qemu-devel] [PATCH 0/3] virtio: cross-endian helpers fixes Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 1/3] virtio: move cross-endian helper to vhost Greg Kurz
2015-11-12 18:00 ` Cornelia Huck
2015-11-13 8:28 ` Greg Kurz
2015-11-09 17:58 ` [Qemu-devel] [PATCH 2/3] vhost: move virtio 1.0 check to cross-endian helper Greg Kurz
2015-11-12 18:03 ` Cornelia Huck
2015-11-09 17:58 ` [Qemu-devel] [PATCH 3/3] virtio: optimize virtio_access_is_big_endian() for little-endian targets Greg Kurz
2015-11-12 18:08 ` Cornelia Huck
2015-11-13 9:28 ` Greg Kurz
2015-11-13 14:42 ` Cornelia Huck
2015-11-13 15:25 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.