Linux Virtualization - May 2015 - [Qemu-devel] [PATCH RFC 4/7] vhost: set vring endianness for legacy virtio

On Wed, 06 May 2015 14:08:02 +0200
Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:
> Legacy virtio is native endian: if the guest and host endianness differ,
> we have to tell vhost so it can swap bytes where appropriate. This is
> done through a vhost ring ioctl.
> 
> Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
> ---
>  hw/virtio/vhost.c |   50
+++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 54851b7..1d7b939 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
(...)
> @@ -677,6 +700,16 @@ static int vhost_virtqueue_start(struct vhost_dev
*dev,
>          return -errno;
>      }
> 
> +    if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1) &&
I think this should either go in after the virtio-1 base support (more
feature bits etc.) or get a big fat comment and be touched up later.
I'd prefer the first solution so it does not get forgotten, but I'm not
sure when Michael plans to proceed with the virtio-1 patches (I think
they're mostly fine already).
> +        virtio_legacy_is_cross_endian(vdev)) {
> +        r = vhost_virtqueue_set_vring_endian_legacy(dev,
> +                                                   
virtio_is_big_endian(vdev),
> +                                                    vhost_vq_index);
> +        if (r) {
> +            return -errno;
> +        }
> +    }
> +
>      s = l = virtio_queue_get_desc_size(vdev, idx);
>      a = virtio_queue_get_desc_addr(vdev, idx);
>      vq->desc = cpu_physical_memory_map(a, &l, 0);

On Tue, May 12, 2015 at 03:25:30PM +0200, Cornelia Huck
wrote:
> On Wed, 06 May 2015 14:08:02 +0200
> Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:
> 
> > Legacy virtio is native endian: if the guest and host endianness
differ,
> > we have to tell vhost so it can swap bytes where appropriate. This is
> > done through a vhost ring ioctl.
> > 
> > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
> > ---
> >  hw/virtio/vhost.c |   50
+++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 54851b7..1d7b939 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> (...)
> > @@ -677,6 +700,16 @@ static int vhost_virtqueue_start(struct vhost_dev
*dev,
> >          return -errno;
> >      }
> > 
> > +    if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> 
> I think this should either go in after the virtio-1 base support (more
> feature bits etc.) or get a big fat comment and be touched up later.
> I'd prefer the first solution so it does not get forgotten, but I'm
not
> sure when Michael plans to proceed with the virtio-1 patches (I think
> they're mostly fine already).
There are three main issues with virtio 1 patches that I am aware of.

One issue with virtio 1 patches as they are is with how features are
handled ATM.  There are 3 types of features

	a. virtio 1 only features
	b. virtio 0 only features
	c. shared features

and 3 types of devices
	a. legacy device: has b+c features
	b. modern device: has a+c features
	c. transitional device: has a+c features but exposes
	   only c through the legacy interface


So I think a callback that gets features depending on guest
version isn't a good way to model it because fundamentally device
has one set of features.
A better way to model this is really just a single
host_features bitmask, and for transitional devices, a mask
hiding a features - which are so far all bits > 31, so maybe
for now we can just have a global mask.

We need to validate features at initialization time and make
sure they make sense, fail if not (sometimes we need to mask
features if they don't make sense - this is unfortunate
but might be needed for compatibility).

Moving host_features to virtio core would make all of the above
easier.


Second issue is migration, some of it is with migrating the new
features, so that's tied to the first one.


Third issue is fixing devices so they don't try to
access guest memory until DRIVER_OK is set.
This is surprisingly hard to do generally given need to support old
drivers which don't set DRIVER_OK or set it very late, and the fact that
we tied work-arounds for even older drivers which dont' set pci bus
master to the DRIVER_OK bit. I tried, and I'm close to giving up and
just checking guest ack for virtio 1, and ignoring DRIVER_OK requirement
if not there.


> > +        virtio_legacy_is_cross_endian(vdev)) {
> > +        r = vhost_virtqueue_set_vring_endian_legacy(dev,
> > +                                                   
virtio_is_big_endian(vdev),
> > +                                                    vhost_vq_index);
> > +        if (r) {
> > +            return -errno;
> > +        }
> > +    }
> > +
> >      s = l = virtio_queue_get_desc_size(vdev, idx);
> >      a = virtio_queue_get_desc_addr(vdev, idx);
> >      vq->desc = cpu_physical_memory_map(a, &l, 0);

On Tue, 12 May 2015 17:15:53 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Tue, May 12, 2015 at 03:25:30PM +0200, Cornelia Huck wrote:
> > On Wed, 06 May 2015 14:08:02 +0200
> > Greg Kurz <gkurz at linux.vnet.ibm.com> wrote:
> > 
> > > Legacy virtio is native endian: if the guest and host endianness
differ,
> > > we have to tell vhost so it can swap bytes where appropriate.
This is
> > > done through a vhost ring ioctl.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>
> > > ---
> > >  hw/virtio/vhost.c |   50
+++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 49 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 54851b7..1d7b939 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > (...)
> > > @@ -677,6 +700,16 @@ static int vhost_virtqueue_start(struct
vhost_dev *dev,
> > >          return -errno;
> > >      }
> > > 
> > > +    if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> > 
> > I think this should either go in after the virtio-1 base support (more
> > feature bits etc.) or get a big fat comment and be touched up later.
> > I'd prefer the first solution so it does not get forgotten, but
I'm not
> > sure when Michael plans to proceed with the virtio-1 patches (I think
> > they're mostly fine already).
> 
> There are three main issues with virtio 1 patches that I am aware of.
> 
> One issue with virtio 1 patches as they are is with how features are
> handled ATM.  There are 3 types of features
> 
> 	a. virtio 1 only features
> 	b. virtio 0 only features
> 	c. shared features
> 
> and 3 types of devices
> 	a. legacy device: has b+c features
> 	b. modern device: has a+c features
> 	c. transitional device: has a+c features but exposes
> 	   only c through the legacy interface
Wouldn't a transitional device be able to expose b as well?
> 
> 
> So I think a callback that gets features depending on guest
> version isn't a good way to model it because fundamentally device
> has one set of features.
> A better way to model this is really just a single
> host_features bitmask, and for transitional devices, a mask
> hiding a features - which are so far all bits > 31, so maybe
> for now we can just have a global mask.
How would this work for transitional presenting a modern device - would
you have a superset of bits and masks for legacy and modern?
> 
> We need to validate features at initialization time and make
> sure they make sense, fail if not (sometimes we need to mask
> features if they don't make sense - this is unfortunate
> but might be needed for compatibility).
> 
> Moving host_features to virtio core would make all of the above
> easier.
I have started hacking up code that moves host_features, but I'm quite
lost with all the different virtio versions floating around. Currently
trying against master, but that of course ignores the virtio-1 issues.
> 
> 
> Second issue is migration, some of it is with migrating the new
> features, so that's tied to the first one.
There's also the used and avail addresses, but that kind of follows
from virtio-1 support.
> 
> 
> Third issue is fixing devices so they don't try to
> access guest memory until DRIVER_OK is set.
> This is surprisingly hard to do generally given need to support old
> drivers which don't set DRIVER_OK or set it very late, and the fact
that
> we tied work-arounds for even older drivers which dont' set pci bus
> master to the DRIVER_OK bit. I tried, and I'm close to giving up and
> just checking guest ack for virtio 1, and ignoring DRIVER_OK requirement
> if not there.
If legacy survived like it is until now, it might be best to focus on
modern devices for this.