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

[Michael S. Tsirkin]

On Tue, Jun 30, 2020 at 02:25:04PM +0200, Cornelia Huck wrote:
> On Tue, 30 Jun 2020 13:10:37 +0100
> Stefan Hajnoczi <stefanha@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>
> > > ---  
> > 
> > I thought that the following already forced VIRTIO 1.0 because it
> > doesn't advertize Legacy or Transitional devices:
> > 
> >   static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = {
> >       .base_name             = TYPE_VHOST_USER_FS_PCI,
> >       .non_transitional_name = "vhost-user-fs-pci",
> >       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >       .instance_size = sizeof(VHostUserFSPCI),
> >       .instance_init = vhost_user_fs_pci_instance_init,
> >       .class_init    = vhost_user_fs_pci_class_init,
> >   };
> 
> This indeed makes vhost-user-fs-pci modern-only, I had not spotted that
> when I wrote the patch. Other modern-only devices do not go down this
> route and use the virtio_pci_force_virtio_1() approach.
> 
> > 
> > Do you have a guest that sees this VIRTIO 1.0 device and still fails to
> > negotiate the VERSION_1 feature bit?
> > 
> > > 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);  
> > 
> > Can this be moved to virtio_pci_types_register() so that it
> > automatically happens for .non_transitional_name devices?
> 
> There are several existing modern-only devices that don't use that kind
> of naming scheme...
> 
> 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?

-- 
MST