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

[Michael S. Tsirkin]

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" <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?

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