Re: [Qemu-devel] [PATCH RFC 4/7] vhost: set vring endianness for legacy virtio

[Cornelia Huck]

On Tue, 12 May 2015 17:15:53 +0200
"Michael S. Tsirkin" <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > ---
> > >  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.