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

[Cornelia Huck]

On Tue, 30 Jun 2020 06:45:42 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 30, 2020 at 11:35:27AM +0200, Cornelia Huck wrote:
> > 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...  
> 
> 
> Not a property, just some flag that I'd set in the core,
> and then teach all transports to take that into account.

I was thinking about compat handling for the disable-legacy property
(struggling a bit with it).

> 
> > > > > 
> > > > > 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.)  
> 
> 
> Well I know it's not really used on x86 yet, so no problem there.
> 
> Which machines are actually affected?

I'd suspect ARM, but breaking even a subset is not nice.

> 
> 
> > * 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?  
> 
> 
> I still think that for the above the best approach is a whitelist
> of legacy virtio IDs.

I agree, a list of the device types that actually support legacy seems
like the way to go. Making it accessible at the right point in the
device instantiation process is the fiddly bit; but maybe I just need a
break from staring at this.

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