Re: [PATCH RFC] virtio-fs: force virtio 1.x usage

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 29 Jun 2020 11:45:35 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jun 29, 2020 at 05:39:33PM +0200, Cornelia Huck wrote:
> > On Mon, 29 Jun 2020 10:53:23 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Mon, Jun 29, 2020 at 12:27:58PM +0200, Cornelia Huck wrote:  
> > > > virtio-fs devices are only specified for virtio-1, so it is unclear
> > > > how a legacy or transitional device should behave.
> > > > 
> > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > > ---
> > > > 
> > > > Forcing off legacy now (after the virtio-fs device has already been
> > > > available) may have unintended consequences, therefore RFC.
> > > > 
> > > > By default, a virtio-pci device uses 'AUTO' for disable_legacy, which
> > > > will resolve to different values based upon which bus the device has
> > > > been plugged. Therefore, forcing disable_legacy may result in the same
> > > > device or a quite different one.
> > > > 
> > > > Even though pre-virtio-1 behaviour of virtio-fs devices is simply not
> > > > specified, toggling disable_legacy will have implications for the BAR
> > > > layout, IIRC, and therefore a guest might end up getting a different
> > > > device, even if it always used it with virtio-1 anyway.
> > > > 
> > > > Not sure what the best way to solve this problem is. Adding a compat
> > > > property for disable_legacy=AUTO may be the right thing to do, but I'm
> > > > not quite clear if there are any further implications here.   

Hnm, I'm a bit confused where to actually set this property...
 
> > > 
> > > Well I notice that this device is not migrateable.
> > > So I think that we can just switch it over and be done with it.  
> > 
> > Oh, that makes things easier. (I'm wondering if libvirt already
> > configures this correctly?)
> >   
> > > 
> > >   
> > > > Whatever we do here, we should make sure that the ccw incarnation of
> > > > this device indeed forces virtio-1.    
> > > 
> > > I agree. I notice that the API virtio_pci_force_virtio_1 turned out
> > > to be too fragile. I propose that instead we have a whitelist of
> > > devices which can be legacy or transitional. Force rest to modern.  
> > 
> > Also, there are further complications because the mechanism is per
> > transport, and therefore easy to miss.
> > 
> > bool virtio_legacy_allowed(VirtIODevice *vdev)
> > {
> >     switch (vdev->device_id) {
> >     case <...>:
> >     <list of legacy-capable devices>
> >         return true;
> >     default:
> >         return false;
> > }
> > 
> > Seems straightforward enough.  
> 
> 
> Agreed. virtio spec has the list.

Ok, I've been staring at this a bit, and it's a bit messy for other
reasons.

First, I noticed that virtio-iommu does not force virtio-1, either; I
think it should? Eric?

Then, there's the mechanism using different names for transitional and
non-transitional devices. Devices that support both usually define both
names (with disable_legacy and disable_modern set appropriately) and a
base name (where the properties can be set manually for the desired
effect). Most virtio-1 only devices set neither the non-transitional
nor the transitional name and rely on virtio_pci_force_virtio_1() to
disable legacy support. But there are outliers:

* this device: it has only a non-transitional name
  ("vhost-user-fs-pci"), which means we automatically get the correct
  configuration; in order to define a transitional/legacy device, you
  would need to use the base name "vhost-user-fs-pci-base" explicitly,
  and it's unlikely that someone has been doing that.
* virtio-iommu (which I *think* is a virtio-1 only device): it defines
  the full set of transitional, non-transitional, and base names.

How should we proceed?
* With this patch here, we can fence off the very unlikely possibility
  of somebody configuring a non-modern virtio-fs device for pci. We
  probably should do it, but I don't think we need compat handling.
* For virtio-iommu, we should get an agreement what the desired state
  is. If it really should be modern only, we need compat handling, as
  the device had been added in 5.0. (And we need to figure out how to
  apply that compat handling.)
* What is the preferred way to express 'this virtio-pci device is
  modern only'? Using virtio_pci_force_virtio_1()? Setting the
  non-transitional name to the obvious name? Both?
* We probably still want to have a 'central authority' for whether a
  device is virtio-1 only or not, so all transports can refer to it.

Thoughts?

> 
> > > 
> > >   
> > > > ---
> > > >  hw/virtio/vhost-user-fs-pci.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
> > > > index e11c889d82b3..244205edf765 100644
> > > > --- a/hw/virtio/vhost-user-fs-pci.c
> > > > +++ b/hw/virtio/vhost-user-fs-pci.c
> > > > @@ -44,6 +44,7 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > > >          vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
> > > >      }
> > > >  
> > > > +    virtio_pci_force_virtio_1(vpci_dev);
> > > >      qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > > >  }
> > > >  
> > > > -- 
> > > > 2.25.4    
> > >   
>