| Message ID | 1418304322-7546-7-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.huck@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 <stefanha@redhat.com>
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.huck@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.
On Thu, 22 Jan 2015 12:54:09 +1100 David Gibson <david@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.huck@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. >
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; } 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
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.huck@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(-)