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.

Whatever we do here, we should make sure that the ccw incarnation of
this device indeed forces virtio-1.

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

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,
  };

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?

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.

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

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.

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

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.
2. transports: could use a core API to detect whether
device can be a legacy one, to block device creation.


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

-- 
MST

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.

(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.)

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

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

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?



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


-- 
MST

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

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.

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.

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.

Well I notice that this device is not migrateable.
So I think that we can just switch it over and be done with it.


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


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

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

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

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

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

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

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.

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


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

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

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

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

On Tue, 30 Jun 2020 09:02:31 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

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

IIRC, yes.

But I think there are ARM machines that use virtio-pci as well, right?

On Wed, Jul 01, 2020 at 03:58:19PM +0200, Cornelia Huck wrote:
> On Tue, 30 Jun 2020 09:02:31 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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:  
> 
> > > > > 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?
> 
> IIRC, yes.
> 
> But I think there are ARM machines that use virtio-pci as well, right?


Right :(

I guess we do need a compat property for that.


-- 
MST

* 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.
> 
> Well I notice that this device is not migrateable.
> So I think that we can just switch it over and be done with it.

Yes, I think I'm OK with that - although you could add the compat
flag in the machine type I guess.

Dave

> 
> > 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.
> 
> 
> > ---
> >  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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK