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

[Michael S. Tsirkin]

On Tue, Jun 30, 2020 at 01:30:43PM +0200, Cornelia Huck wrote:
> 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).

Let's figure out if we actually need 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.

OK so MMIO does not have transitional at all right?


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