| Message ID | 1412692807-12398-6-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 7 Oct 2014 16:40:01 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > Introduce a helper function to indicate whether a virtio device is > operating in legacy or virtio standard mode. > > It may be used to make decisions about the endianess of virtio accesses > and other virtio-1 specific changes, enabling us to support transitional > devices. > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > 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 | 13 +++++++++++-- > 3 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 7aaa953..e6ae3a0 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -883,7 +883,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_device_is_legacy(vdev)) { > + 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; > } > Shouldn't we have some code doing the following somewhere ? if (!virtio_device_is_legacy(vdev)) { vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; } also, since virtio-1 is LE only, do we expect device_endian to be different from VIRTIO_DEVICE_ENDIAN_LITTLE ? Cheers. -- Greg > 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..c123ee0 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_device_is_legacy(vdev)) { > + /* 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 b408166..40e567c 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -275,9 +275,18 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, > void virtio_queue_notify_vq(VirtQueue *vq); > void virtio_irq(VirtQueue *vq); > > +static inline bool virtio_device_is_legacy(VirtIODevice *vdev) > +{ > + return !(vdev->guest_features[1] & (1 << (VIRTIO_F_VERSION_1 - 32))); > +} > + > 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_device_is_legacy(vdev)) { > + 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
On Tue, 28 Oct 2014 16:40:18 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Tue, 7 Oct 2014 16:40:01 +0200 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > Introduce a helper function to indicate whether a virtio device is > > operating in legacy or virtio standard mode. > > > > It may be used to make decisions about the endianess of virtio accesses > > and other virtio-1 specific changes, enabling us to support transitional > > devices. > > > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > > 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 | 13 +++++++++++-- > > 3 files changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 7aaa953..e6ae3a0 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -883,7 +883,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_device_is_legacy(vdev)) { > > + 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; > > } > > > > Shouldn't we have some code doing the following somewhere ? > > if (!virtio_device_is_legacy(vdev)) { > vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; > } > > also, since virtio-1 is LE only, do we expect device_endian to > be different from VIRTIO_DEVICE_ENDIAN_LITTLE ? device_endian should not depend on whether the device is legacy or not. virtio_is_big_endian always returns false for virtio-1 devices, though.
On Thu, 30 Oct 2014 19:02:01 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 28 Oct 2014 16:40:18 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > On Tue, 7 Oct 2014 16:40:01 +0200 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > Introduce a helper function to indicate whether a virtio device is > > > operating in legacy or virtio standard mode. > > > > > > It may be used to make decisions about the endianess of virtio accesses > > > and other virtio-1 specific changes, enabling us to support transitional > > > devices. > > > > > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > > > 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 | 13 +++++++++++-- > > > 3 files changed, 20 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > index 7aaa953..e6ae3a0 100644 > > > --- a/hw/virtio/virtio.c > > > +++ b/hw/virtio/virtio.c > > > @@ -883,7 +883,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_device_is_legacy(vdev)) { > > > + 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; > > > } > > > > > > > Shouldn't we have some code doing the following somewhere ? > > > > if (!virtio_device_is_legacy(vdev)) { > > vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; > > } > > > > also, since virtio-1 is LE only, do we expect device_endian to > > be different from VIRTIO_DEVICE_ENDIAN_LITTLE ? > > device_endian should not depend on whether the device is legacy or not. > virtio_is_big_endian always returns false for virtio-1 devices, though. Sorry, I had missed the virtio_is_big_endian() change: it that makes device_endian a legacy virtio only matter. So why would we care to migrate the endian subsection when we have a virtio-1 device ? Shouldn't virtio_device_endian_needed() return false for virtio-1 ? -- Greg
On Thu, 30 Oct 2014 23:29:50 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > On Thu, 30 Oct 2014 19:02:01 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Tue, 28 Oct 2014 16:40:18 +0100 > > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > > > On Tue, 7 Oct 2014 16:40:01 +0200 > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > > > Introduce a helper function to indicate whether a virtio device is > > > > operating in legacy or virtio standard mode. > > > > > > > > It may be used to make decisions about the endianess of virtio accesses > > > > and other virtio-1 specific changes, enabling us to support transitional > > > > devices. > > > > > > > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > > > > 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 | 13 +++++++++++-- > > > > 3 files changed, 20 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > > index 7aaa953..e6ae3a0 100644 > > > > --- a/hw/virtio/virtio.c > > > > +++ b/hw/virtio/virtio.c > > > > @@ -883,7 +883,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_device_is_legacy(vdev)) { > > > > + 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; > > > > } > > > > > > > > > > Shouldn't we have some code doing the following somewhere ? > > > > > > if (!virtio_device_is_legacy(vdev)) { > > > vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; > > > } > > > > > > also, since virtio-1 is LE only, do we expect device_endian to > > > be different from VIRTIO_DEVICE_ENDIAN_LITTLE ? > > > > device_endian should not depend on whether the device is legacy or not. > > virtio_is_big_endian always returns false for virtio-1 devices, though. > > Sorry, I had missed the virtio_is_big_endian() change: it that makes > device_endian a legacy virtio only matter. > So why would we care to migrate the endian subsection when we have a > virtio-1 device ? Shouldn't virtio_device_endian_needed() return false > for virtio-1 ? Indeeed, we can just leave device_endian at the default value for virtio-1 devices.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 7aaa953..e6ae3a0 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -883,7 +883,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_device_is_legacy(vdev)) { + 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..c123ee0 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_device_is_legacy(vdev)) { + /* 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 b408166..40e567c 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -275,9 +275,18 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, void virtio_queue_notify_vq(VirtQueue *vq); void virtio_irq(VirtQueue *vq); +static inline bool virtio_device_is_legacy(VirtIODevice *vdev) +{ + return !(vdev->guest_features[1] & (1 << (VIRTIO_F_VERSION_1 - 32))); +} + 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_device_is_legacy(vdev)) { + 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