Re: [Qemu-devel] [PATCH RFC v6 06/20] virtio: endianness checks for virtio 1.0 devices
On Thu, 22 Jan 2015 12:54:09 +1100
David Gibson da...@gibson.dropbear.id.au wrote:
On Thu, Dec 11, 2014 at 02:25:08PM +0100, Cornelia Huck wrote:
Add code that checks for the VERSION_1 feature bit in order to make
decisions about the device's endianness. This allows us to support
transitional devices.
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
hw/virtio/virtio.c|6 +-
include/hw/virtio/virtio-access.h |4
include/hw/virtio/virtio.h|8 ++--
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7f74ae5..8f69ffa 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -881,7 +881,11 @@ static bool virtio_device_endian_needed(void *opaque)
VirtIODevice *vdev = opaque;
assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian != virtio_default_endian();
+if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+return vdev-device_endian != virtio_default_endian();
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
This doesn't seem quite right. Since virtio 1.0 is always LE, this
should just assert that device_endian == LE and return false,
right?
The device_endian field is ONLY used by devices when the software is
legacy.
It is set at device reset time (see virtio_reset()) since we can reasonably
assume that when the software changes endianness, it always reset the device
before using it again (aka. reboot or kexec).
In the case we would have a BE guest, device_endian would be BE, even if the
software is virtio 1.0. So, no, we shouldn't assert.
I had questioned Cornelia about why we care to migrate device_endian when in
virtio 1.0 mode. I got these answers:
https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03979.html
https://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg03888.html
My understanding is that a transitional device will necessarily be reset
if the software changes from legacy to 1.0 or vice-versa. So, yes, I still
think virtio_device_endian_needed() should return false.
}
static const VMStateDescription vmstate_virtio_device_endian = {
diff --git a/include/hw/virtio/virtio-access.h
b/include/hw/virtio/virtio-access.h
index 46456fd..ee28c21 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,6 +19,10 @@
static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
{
+if (virtio_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)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 08141c7..68c40db 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -297,7 +297,11 @@ static inline bool virtio_has_feature(VirtIODevice
*vdev, unsigned int fbit)
static inline bool virtio_is_big_endian(VirtIODevice *vdev)
{
-assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
}
#endif
AFAICT, the only real difference between virtio_is_big_endian() and
virtio_access_is_big_endian() is that the latter will become
compile-time constant on targets that don't do bi-endian.
With virtio 1.0 support, that's no longer true, so those two macros
should just be merged, I think.
pgp4bYsXGzcPz.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v6 06/20] virtio: endianness checks for virtio 1.0 devices
On Thu, Dec 11, 2014 at 02:25:08PM +0100, Cornelia Huck wrote:
Add code that checks for the VERSION_1 feature bit in order to make
decisions about the device's endianness. This allows us to support
transitional devices.
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
hw/virtio/virtio.c|6 +-
include/hw/virtio/virtio-access.h |4
include/hw/virtio/virtio.h|8 ++--
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7f74ae5..8f69ffa 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -881,7 +881,11 @@ static bool virtio_device_endian_needed(void *opaque)
VirtIODevice *vdev = opaque;
assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian != virtio_default_endian();
+if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+return vdev-device_endian != virtio_default_endian();
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
This doesn't seem quite right. Since virtio 1.0 is always LE, this
should just assert that device_endian == LE and return false,
right?
}
static const VMStateDescription vmstate_virtio_device_endian = {
diff --git a/include/hw/virtio/virtio-access.h
b/include/hw/virtio/virtio-access.h
index 46456fd..ee28c21 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,6 +19,10 @@
static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
{
+if (virtio_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)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 08141c7..68c40db 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -297,7 +297,11 @@ static inline bool virtio_has_feature(VirtIODevice
*vdev, unsigned int fbit)
static inline bool virtio_is_big_endian(VirtIODevice *vdev)
{
-assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
}
#endif
AFAICT, the only real difference between virtio_is_big_endian() and
virtio_access_is_big_endian() is that the latter will become
compile-time constant on targets that don't do bi-endian.
With virtio 1.0 support, that's no longer true, so those two macros
should just be merged, I think.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgph9u9iOC_0n.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH RFC v6 06/20] virtio: endianness checks for virtio 1.0 devices
On Thu, Dec 11, 2014 at 02:25:08PM +0100, Cornelia Huck wrote: Add code that checks for the VERSION_1 feature bit in order to make decisions about the device's endianness. This allows us to support transitional devices. Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com --- hw/virtio/virtio.c|6 +- include/hw/virtio/virtio-access.h |4 include/hw/virtio/virtio.h|8 ++-- 3 files changed, 15 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi stefa...@redhat.com pgpyP3qS6pI8D.pgp Description: PGP signature
