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

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.

On Tue, May 12, 2015 at 06:25:32PM +0200, Cornelia Huck
wrote:
> 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?
No because the virtio 1 spec says it shouldn't.

> > 
> > 
> > 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?
Basically we expose through modern interface a superset
of bits exposed through legacy.
F_BAD for pci is probably the only exception.

> > 
> > 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.
Yes, I think we should focus on infrastructure cleanups in master first.
> > 
> > 
> > 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.
I'm kind of unhappy that it's up to guest though as that controls
whether we run in modern mode. But yea.

-- 
MST

On Tue, 12 May 2015 18:40:35 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Tue, May 12, 2015 at 06:25:32PM +0200, Cornelia Huck wrote:
> > On Tue, 12 May 2015 17:15:53 +0200
> > "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > > 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?
> 
> No because the virtio 1 spec says it shouldn't.
Can you point me to the part where it says that? Can't seem to find it.
> > > 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?
> 
> Basically we expose through modern interface a superset
> of bits exposed through legacy.
> F_BAD for pci is probably the only exception.
ccw has F_BAD as well.

NOTIFY_ON_EMPTY is in the MAY be offered category.

I'm still wondering about the old legacy bits. Transitional devices not
offering them is easiest to implement from the host side, but I'm
wondering if all legacy devices will be able to function with missing
bits? Is breaking some legacy drivers OK?