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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 02, 2020 at 01:55:59PM +0200, Cornelia Huck wrote:
> On Thu, 2 Jul 2020 07:22:49 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jul 02, 2020 at 12:45:38PM +0200, Cornelia Huck wrote:
> > > On Thu, 2 Jul 2020 06:16:06 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 01, 2020 at 06:19:17PM +0200, Cornelia Huck wrote:  
> > > > > On Tue, 30 Jun 2020 09:04:38 -0400
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Tue, Jun 30, 2020 at 02:25:04PM +0200, Cornelia Huck wrote:    
> > > > >     
> > > > > > > What bothers me most is that you need to explicitly request a device to
> > > > > > > be modern-only, while that should be the default for any newly added
> > > > > > > device. Hence the approach with the centralized list of device types
> > > > > > > mentioned in a parallel thread. The main problem with that is that the
> > > > > > > proxy device starts getting realized before the virtio device with its
> > > > > > > id is present... I failed to find a solution so far. But I'd really
> > > > > > > like an approach that can work for all transports.      
> > > > > > 
> > > > > > So how about simply validating that the device is modern only,
> > > > > > unless it's one of the whitelist?    
> > > > > 
> > > > > Who would do the validation, the virtio core? How can it distinguish
> > > > > between transitional and non-transitional? But maybe I'm just not
> > > > > getting your idea.    
> > > > 
> > > > OK I've been thinking about two ideas, we can use them both:
> > > > 1. virtio core: that can detect VIRTIO_1 being clear
> > > > in virtio_validate_features.  
> > > 
> > > After feature negotiation is complete? That feels like a regression in
> > > behaviour: You would be able to add a device that may not be usable
> > > (and you'll only find out after the guest tried to use it), instead of
> > > making sure that only a non-transitional device can be added to start
> > > with.  
> > 
> > I mean, we can still have transports validate, that is point 2.
> > It seems prudent to check though, since guest could be buggy
> > ignoring bits that it got.
> > 
> > > (We do not validate if the guest did not negotiate VERSION_1, but we
> > > can certainly add a special case for the "guest did not accept offered
> > > VERSION_1" case.)  
> > 
> > exaclty.
> > 
> > >   
> > > > 2. transports: could use a core API to detect whether
> > > > device can be a legacy one, to block device creation.  
> > > 
> > > That would be the best, but how do we get around the "transport does
> > > not know the device type until it is too late" problem? Unless you want
> > > to redo the internal interfaces.  
> > 
> > Oh. I think I am missing something.
> > So I'm considering virtio_pci_device_plugged for example.
> > 
> > 
> > static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
> > {
> >     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> >     VirtioBusState *bus = &proxy->bus;
> >     bool legacy = virtio_pci_legacy(proxy);
> >     bool modern;
> >     bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
> >     uint8_t *config;
> >     uint32_t size;
> >     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > 
> >     /*
> > 
> > ..
> > 
> > }
> > 
> > can't we check device type here and make sure it matches the "legacy"
> > flag?
> 
> It would be a change in behaviour: Currently, I can specify e.g.
> 
> -device virtio-gpu-pci,disable-legacy=off,disable-modern=true

I don't think we care about this at all.
User is explicitly asking for a non-compliant configuration,
user gets to keep both pieces.




> and the code in the realize function would force it to a modern-only
> device. Checking in the plugged function would cause it to fail. This
> might be preferable, but could break existing command lines.

> Note that ccw is different: if I specify
> 
> -device virtio-gpu-ccw,max_revision=0
> 
> it actually fails with
> 
> qemu-system-s390x: -device virtio-gpu-ccw,max_revision=0: Invalid value of property max_rev (is 0 expected >= 1)
> 
> so moving to the plugged function would not cause a change in behaviour
> from the user's point of view.
> 
> > 
> > 
> > 
> > > > 
> > > >   
> > > > > Also, ccw does not currently have a way to explicitly configure a
> > > > > device non-transitional; the revisions can be used to fence off newer
> > > > > features, going down to legacy-only, but fencing off older features is
> > > > > not possible (that is only done by the device, if it has no legacy
> > > > > support).    
> > > > 
> > > > I guess for ccw only option 1 works.
> > > >   
> > > 
> > > Or keep it as-is, and disallow legacy for the individual device types,
> > > with the validate check as a safety net during development.  
> > 
> > Problem is people cut and paste from transitional devices.
> 
> That should not be a problem for ccw (as transitional and
> non-transitional are the same on the command line); moreover, people
> are unlikely to set max_revision themselves (this is usually only done
> by compat machines).
> 
> If changing the behaviour for pci is acceptable, we can sure move to
> the plugged approach.