[PATCH 0/2] virtio: non-legacy device handling

[PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

As discussed in "virtio-fs: force virtio 1.x usage", it seems like
a good idea to make sure that any new virtio device (which does not
support legacy virtio) is indeed a non-transitional device, just to
catch accidental misconfigurations. We can easily compile a list
of virtio devices with legacy support and have transports verify
in their plugged callbacks that legacy support is off for any device
not in that list.

Most new virtio devices force non-transitional already, so nothing
changes for them. vhost-user-fs-pci even does not allow to configure
a non-transitional device, so it is fine as well.

One problematic device, however, is virtio-iommu-pci. It currently
offers both the transitional and the non-transitional variety of the
device, and does not force anything. I'm unsure whether we should
consider transitional virtio-iommu unsupported, or if we should add
some compat handling. (The support for legacy or not generally may
change based upon the bus, IIUC, so I'm unsure how to come up with
something generic.)

Cornelia Huck (2):
  virtio: list legacy-capable devices
  virtio: verify that legacy support is not accidentally on

 hw/s390x/virtio-ccw.c      |  6 ++++++
 hw/virtio/virtio-pci.c     |  4 ++++
 hw/virtio/virtio.c         | 25 +++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 4 files changed, 37 insertions(+)

-- 
2.25.4

[PATCH 1/2] virtio: list legacy-capable devices

[Cornelia Huck]

Several types of virtio devices had already been around before the
virtio standard was specified. These devices support virtio in legacy
(and transitional) mode.

Devices that have been added in the virtio standard are considered
non-transitional (i.e. with no support for legacy virtio).

Provide a helper function so virtio transports can figure that out
easily.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/virtio/virtio.c         | 25 +++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cc9c9dc1621e..c713698df7ab 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "sysemu/dma.h"
 #include "sysemu/runstate.h"
+#include "standard-headers/linux/virtio_ids.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -3279,6 +3280,30 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     vdev->use_guest_notifier_mask = true;
 }
 
+/*
+ * Only devices that have already been around prior to defining the virtio
+ * standard support legacy mode; this includes devices not specified in the
+ * standard. All newer devices conform to the virtio standard only.
+ */
+bool virtio_legacy_allowed(VirtIODevice *vdev)
+{
+    switch (vdev->device_id) {
+    case VIRTIO_ID_NET:
+    case VIRTIO_ID_BLOCK:
+    case VIRTIO_ID_CONSOLE:
+    case VIRTIO_ID_RNG:
+    case VIRTIO_ID_BALLOON:
+    case VIRTIO_ID_RPMSG:
+    case VIRTIO_ID_SCSI:
+    case VIRTIO_ID_9P:
+    case VIRTIO_ID_RPROC_SERIAL:
+    case VIRTIO_ID_CAIF:
+        return true;
+    default:
+        return false;
+    }
+}
+
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
 {
     return vdev->vq[n].vring.desc;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b69d51749635..198ffc762678 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -396,4 +396,6 @@ static inline bool virtio_device_disabled(VirtIODevice *vdev)
     return unlikely(vdev->disabled || vdev->broken);
 }
 
+bool virtio_legacy_allowed(VirtIODevice *vdev);
+
 #endif
-- 
2.25.4

[PATCH 2/2] virtio: verify that legacy support is not accidentally on

[Cornelia Huck]

If a virtio device does not have legacy support, make sure that
it is actually off, and bail out if not.

For virtio-pci, this means that any device without legacy support
that has been specified to modern-only (or that has been forced
to it) will work.

For virtio-ccw, this duplicates the check that is currently done
prior to realization for any device that explicitly specified no
support for legacy.

This catches devices that have not been fenced properly.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/virtio-ccw.c  | 6 ++++++
 hw/virtio/virtio-pci.c | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 3c988a000b5b..0e602702971b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1121,6 +1121,12 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
         dev->max_rev = 0;
     }
 
+    if (!virtio_ccw_rev_max(dev) && !virtio_legacy_allowed(vdev)) {
+        error_setg(errp, "Invalid value of property max_rev "
+                   "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
+        return;
+    }
+
     if (virtio_get_num_queues(vdev) > VIRTIO_QUEUE_MAX) {
         error_setg(errp, "The number of virtqueues %d "
                    "exceeds virtio limit %d", n,
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7bc8c1c056e9..d4c4128ea507 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1569,6 +1569,10 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     }
 
     if (legacy) {
+        if (!virtio_legacy_allowed(vdev)) {
+            error_setg(errp, "device is modern-only, use disable-legacy=on");
+            return;
+        }
         if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
             error_setg(errp, "VIRTIO_F_IOMMU_PLATFORM was supported by"
                        " neither legacy nor transitional device");
-- 
2.25.4

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Tue,  7 Jul 2020 12:54:44 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> a good idea to make sure that any new virtio device (which does not
> support legacy virtio) is indeed a non-transitional device, just to
> catch accidental misconfigurations. We can easily compile a list
> of virtio devices with legacy support and have transports verify
> in their plugged callbacks that legacy support is off for any device
> not in that list.
> 
> Most new virtio devices force non-transitional already, so nothing
> changes for them. vhost-user-fs-pci even does not allow to configure
> a non-transitional device, so it is fine as well.
> 
> One problematic device, however, is virtio-iommu-pci. It currently
> offers both the transitional and the non-transitional variety of the
> device, and does not force anything. I'm unsure whether we should
> consider transitional virtio-iommu unsupported, or if we should add
> some compat handling. (The support for legacy or not generally may
> change based upon the bus, IIUC, so I'm unsure how to come up with
> something generic.)
> 
> Cornelia Huck (2):
>   virtio: list legacy-capable devices
>   virtio: verify that legacy support is not accidentally on
> 
>  hw/s390x/virtio-ccw.c      |  6 ++++++
>  hw/virtio/virtio-pci.c     |  4 ++++
>  hw/virtio/virtio.c         | 25 +++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  4 files changed, 37 insertions(+)
> 

Friendly ping.

Re: [PATCH 0/2] virtio: non-legacy device handling

[David Hildenbrand]

On 07.07.20 12:54, Cornelia Huck wrote:
> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> a good idea to make sure that any new virtio device (which does not
> support legacy virtio) is indeed a non-transitional device, just to
> catch accidental misconfigurations. We can easily compile a list
> of virtio devices with legacy support and have transports verify
> in their plugged callbacks that legacy support is off for any device
> not in that list.
> 
> Most new virtio devices force non-transitional already, so nothing
> changes for them. vhost-user-fs-pci even does not allow to configure
> a non-transitional device, so it is fine as well.
> 
> One problematic device, however, is virtio-iommu-pci. It currently
> offers both the transitional and the non-transitional variety of the
> device, and does not force anything. I'm unsure whether we should
> consider transitional virtio-iommu unsupported, or if we should add
> some compat handling. (The support for legacy or not generally may
> change based upon the bus, IIUC, so I'm unsure how to come up with
> something generic.)
> 
> Cornelia Huck (2):
>   virtio: list legacy-capable devices
>   virtio: verify that legacy support is not accidentally on

I'd squash both patches. Looking at patch #1, I wonder why we don't
store that information along with the device implementation? What was
the motivation to define this information separately?


-- 
Thanks,

David / dhildenb

Re: [PATCH 0/2] virtio: non-legacy device handling

[Michael S. Tsirkin]

On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:
> On 07.07.20 12:54, Cornelia Huck wrote:
> > As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> > a good idea to make sure that any new virtio device (which does not
> > support legacy virtio) is indeed a non-transitional device, just to
> > catch accidental misconfigurations. We can easily compile a list
> > of virtio devices with legacy support and have transports verify
> > in their plugged callbacks that legacy support is off for any device
> > not in that list.
> > 
> > Most new virtio devices force non-transitional already, so nothing
> > changes for them. vhost-user-fs-pci even does not allow to configure
> > a non-transitional device, so it is fine as well.
> > 
> > One problematic device, however, is virtio-iommu-pci. It currently
> > offers both the transitional and the non-transitional variety of the
> > device, and does not force anything. I'm unsure whether we should
> > consider transitional virtio-iommu unsupported, or if we should add
> > some compat handling. (The support for legacy or not generally may
> > change based upon the bus, IIUC, so I'm unsure how to come up with
> > something generic.)
> > 
> > Cornelia Huck (2):
> >   virtio: list legacy-capable devices
> >   virtio: verify that legacy support is not accidentally on
> 
> I'd squash both patches. Looking at patch #1, I wonder why we don't
> store that information along with the device implementation? What was
> the motivation to define this information separately?

Because people seem to cut and paste code, so when one
enables it in an old device, it gets pasted into a new one.
With a list in a central place, it's easier to figure out
what's going on.

> 
> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH 0/2] virtio: non-legacy device handling

[David Hildenbrand]

On 20.07.20 11:03, Michael S. Tsirkin wrote:
> On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:
>> On 07.07.20 12:54, Cornelia Huck wrote:
>>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
>>> a good idea to make sure that any new virtio device (which does not
>>> support legacy virtio) is indeed a non-transitional device, just to
>>> catch accidental misconfigurations. We can easily compile a list
>>> of virtio devices with legacy support and have transports verify
>>> in their plugged callbacks that legacy support is off for any device
>>> not in that list.
>>>
>>> Most new virtio devices force non-transitional already, so nothing
>>> changes for them. vhost-user-fs-pci even does not allow to configure
>>> a non-transitional device, so it is fine as well.
>>>
>>> One problematic device, however, is virtio-iommu-pci. It currently
>>> offers both the transitional and the non-transitional variety of the
>>> device, and does not force anything. I'm unsure whether we should
>>> consider transitional virtio-iommu unsupported, or if we should add
>>> some compat handling. (The support for legacy or not generally may
>>> change based upon the bus, IIUC, so I'm unsure how to come up with
>>> something generic.)
>>>
>>> Cornelia Huck (2):
>>>   virtio: list legacy-capable devices
>>>   virtio: verify that legacy support is not accidentally on
>>
>> I'd squash both patches. Looking at patch #1, I wonder why we don't
>> store that information along with the device implementation? What was
>> the motivation to define this information separately?
> 
> Because people seem to cut and paste code, so when one
> enables it in an old device, it gets pasted into a new one.
> With a list in a central place, it's easier to figure out
> what's going on.

Makes sense, I suggest adding that to the patch description.

Both patches look sane to me (- squashing them).

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/2] virtio: non-legacy device handling

[Halil Pasic]

On Tue,  7 Jul 2020 12:54:44 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> a good idea to make sure that any new virtio device (which does not
> support legacy virtio) is indeed a non-transitional device, just to
> catch accidental misconfigurations. We can easily compile a list
> of virtio devices with legacy support and have transports verify
> in their plugged callbacks that legacy support is off for any device
> not in that list.
> 
> Most new virtio devices force non-transitional already, so nothing
> changes for them. vhost-user-fs-pci even does not allow to configure
> a non-transitional device, so it is fine as well.
> 
> One problematic device, however, is virtio-iommu-pci. It currently
> offers both the transitional and the non-transitional variety of the
> device, and does not force anything. I'm unsure whether we should
> consider transitional virtio-iommu unsupported, or if we should add
> some compat handling. (The support for legacy or not generally may
> change based upon the bus, IIUC, so I'm unsure how to come up with
> something generic.)
> 

Both patches look good to me (Acked-by: Halil Pasic
<pasic@linux.ibm.com>). I tend to agree with Davids comment on how
this information is coded: the more object oriented way would have
been to store this at the something like VirtioDeviceClass, but
Michael's argument stands.

Another OO option would be to expose this as a virtio property. Would
enable introspection, and would also give the host admin means to
force non-legacy for transitional devices. But the juice is probably not
worth the squeeze.

Regards,
Halil

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Mon, 20 Jul 2020 11:07:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 20.07.20 11:03, Michael S. Tsirkin wrote:
> > On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:  
> >> On 07.07.20 12:54, Cornelia Huck wrote:  
> >>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> >>> a good idea to make sure that any new virtio device (which does not
> >>> support legacy virtio) is indeed a non-transitional device, just to
> >>> catch accidental misconfigurations. We can easily compile a list
> >>> of virtio devices with legacy support and have transports verify
> >>> in their plugged callbacks that legacy support is off for any device
> >>> not in that list.
> >>>
> >>> Most new virtio devices force non-transitional already, so nothing
> >>> changes for them. vhost-user-fs-pci even does not allow to configure
> >>> a non-transitional device, so it is fine as well.
> >>>
> >>> One problematic device, however, is virtio-iommu-pci. It currently
> >>> offers both the transitional and the non-transitional variety of the
> >>> device, and does not force anything. I'm unsure whether we should
> >>> consider transitional virtio-iommu unsupported, or if we should add
> >>> some compat handling. (The support for legacy or not generally may
> >>> change based upon the bus, IIUC, so I'm unsure how to come up with
> >>> something generic.)
> >>>
> >>> Cornelia Huck (2):
> >>>   virtio: list legacy-capable devices
> >>>   virtio: verify that legacy support is not accidentally on  
> >>
> >> I'd squash both patches. Looking at patch #1, I wonder why we don't
> >> store that information along with the device implementation? What was
> >> the motivation to define this information separately?  
> > 
> > Because people seem to cut and paste code, so when one
> > enables it in an old device, it gets pasted into a new one.
> > With a list in a central place, it's easier to figure out
> > what's going on.  
> 
> Makes sense, I suggest adding that to the patch description.

"The list of devices supporting legacy is supposed to be static. We
keep it in a central place to make sure that new devices do not enable
legacy by accident."

?

> 
> Both patches look sane to me (- squashing them).
> 

Patch 1 does not change behaviour, while patch 2 does (for
virtio-iommu-pci). Still would like an opinion whether changing the
behaviour for virtio-iommu-pci with no compat handling is ok.

(I could be persuaded to squash them.)

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Mon, 20 Jul 2020 11:54:06 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue,  7 Jul 2020 12:54:44 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> > a good idea to make sure that any new virtio device (which does not
> > support legacy virtio) is indeed a non-transitional device, just to
> > catch accidental misconfigurations. We can easily compile a list
> > of virtio devices with legacy support and have transports verify
> > in their plugged callbacks that legacy support is off for any device
> > not in that list.
> > 
> > Most new virtio devices force non-transitional already, so nothing
> > changes for them. vhost-user-fs-pci even does not allow to configure
> > a non-transitional device, so it is fine as well.
> > 
> > One problematic device, however, is virtio-iommu-pci. It currently
> > offers both the transitional and the non-transitional variety of the
> > device, and does not force anything. I'm unsure whether we should
> > consider transitional virtio-iommu unsupported, or if we should add
> > some compat handling. (The support for legacy or not generally may
> > change based upon the bus, IIUC, so I'm unsure how to come up with
> > something generic.)
> >   
> 
> Both patches look good to me (Acked-by: Halil Pasic
> <pasic@linux.ibm.com>). I tend to agree with Davids comment on how
> this information is coded: the more object oriented way would have
> been to store this at the something like VirtioDeviceClass, but
> Michael's argument stands.
> 
> Another OO option would be to expose this as a virtio property. Would
> enable introspection, and would also give the host admin means to
> force non-legacy for transitional devices. But the juice is probably not
> worth the squeeze.

I agree, that would be a lot of hassle for exposing what is basically
static information.

Re: [PATCH 0/2] virtio: non-legacy device handling

[David Hildenbrand]

On 23.07.20 08:33, Cornelia Huck wrote:
> On Mon, 20 Jul 2020 11:07:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.07.20 11:03, Michael S. Tsirkin wrote:
>>> On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:  
>>>> On 07.07.20 12:54, Cornelia Huck wrote:  
>>>>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
>>>>> a good idea to make sure that any new virtio device (which does not
>>>>> support legacy virtio) is indeed a non-transitional device, just to
>>>>> catch accidental misconfigurations. We can easily compile a list
>>>>> of virtio devices with legacy support and have transports verify
>>>>> in their plugged callbacks that legacy support is off for any device
>>>>> not in that list.
>>>>>
>>>>> Most new virtio devices force non-transitional already, so nothing
>>>>> changes for them. vhost-user-fs-pci even does not allow to configure
>>>>> a non-transitional device, so it is fine as well.
>>>>>
>>>>> One problematic device, however, is virtio-iommu-pci. It currently
>>>>> offers both the transitional and the non-transitional variety of the
>>>>> device, and does not force anything. I'm unsure whether we should
>>>>> consider transitional virtio-iommu unsupported, or if we should add
>>>>> some compat handling. (The support for legacy or not generally may
>>>>> change based upon the bus, IIUC, so I'm unsure how to come up with
>>>>> something generic.)
>>>>>
>>>>> Cornelia Huck (2):
>>>>>   virtio: list legacy-capable devices
>>>>>   virtio: verify that legacy support is not accidentally on  
>>>>
>>>> I'd squash both patches. Looking at patch #1, I wonder why we don't
>>>> store that information along with the device implementation? What was
>>>> the motivation to define this information separately?  
>>>
>>> Because people seem to cut and paste code, so when one
>>> enables it in an old device, it gets pasted into a new one.
>>> With a list in a central place, it's easier to figure out
>>> what's going on.  
>>
>> Makes sense, I suggest adding that to the patch description.
> 
> "The list of devices supporting legacy is supposed to be static. We
> keep it in a central place to make sure that new devices do not enable
> legacy by accident."
> 
> ?

Ack!

> 
>>
>> Both patches look sane to me (- squashing them).
>>
> 
> Patch 1 does not change behaviour, while patch 2 does (for
> virtio-iommu-pci). Still would like an opinion whether changing the
> behaviour for virtio-iommu-pci with no compat handling is ok.
> 
> (I could be persuaded to squash them.)

I'm a friend of introducing helper functions along with code that
actually uses it. But I agree that the change in behavior might be
hairy. Maybe we can split that out somehow to give it more attention?

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/2] virtio: non-legacy device handling

[Cornelia Huck]

On Thu, 23 Jul 2020 13:57:08 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 23.07.20 08:33, Cornelia Huck wrote:
> > On Mon, 20 Jul 2020 11:07:51 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 20.07.20 11:03, Michael S. Tsirkin wrote:  
> >>> On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:    
> >>>> On 07.07.20 12:54, Cornelia Huck wrote:    
> >>>>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> >>>>> a good idea to make sure that any new virtio device (which does not
> >>>>> support legacy virtio) is indeed a non-transitional device, just to
> >>>>> catch accidental misconfigurations. We can easily compile a list
> >>>>> of virtio devices with legacy support and have transports verify
> >>>>> in their plugged callbacks that legacy support is off for any device
> >>>>> not in that list.
> >>>>>
> >>>>> Most new virtio devices force non-transitional already, so nothing
> >>>>> changes for them. vhost-user-fs-pci even does not allow to configure
> >>>>> a non-transitional device, so it is fine as well.
> >>>>>
> >>>>> One problematic device, however, is virtio-iommu-pci. It currently
> >>>>> offers both the transitional and the non-transitional variety of the
> >>>>> device, and does not force anything. I'm unsure whether we should
> >>>>> consider transitional virtio-iommu unsupported, or if we should add
> >>>>> some compat handling. (The support for legacy or not generally may
> >>>>> change based upon the bus, IIUC, so I'm unsure how to come up with
> >>>>> something generic.)
> >>>>>
> >>>>> Cornelia Huck (2):
> >>>>>   virtio: list legacy-capable devices
> >>>>>   virtio: verify that legacy support is not accidentally on    
> >>>>
> >>>> I'd squash both patches. Looking at patch #1, I wonder why we don't
> >>>> store that information along with the device implementation? What was
> >>>> the motivation to define this information separately?    
> >>>
> >>> Because people seem to cut and paste code, so when one
> >>> enables it in an old device, it gets pasted into a new one.
> >>> With a list in a central place, it's easier to figure out
> >>> what's going on.    
> >>
> >> Makes sense, I suggest adding that to the patch description.  
> > 
> > "The list of devices supporting legacy is supposed to be static. We
> > keep it in a central place to make sure that new devices do not enable
> > legacy by accident."
> > 
> > ?  
> 
> Ack!
> 
> >   
> >>
> >> Both patches look sane to me (- squashing them).
> >>  
> > 
> > Patch 1 does not change behaviour, while patch 2 does (for
> > virtio-iommu-pci). Still would like an opinion whether changing the
> > behaviour for virtio-iommu-pci with no compat handling is ok.
> > 
> > (I could be persuaded to squash them.)  
> 
> I'm a friend of introducing helper functions along with code that
> actually uses it. But I agree that the change in behavior might be
> hairy. Maybe we can split that out somehow to give it more attention?

It should not really be noticeable for anything but virtio-iommu.

However, I see these are already in a pull request...

Re: [PATCH 0/2] virtio: non-legacy device handling

[Michael S. Tsirkin]

On Thu, Jul 23, 2020 at 02:15:07PM +0200, Cornelia Huck wrote:
> On Thu, 23 Jul 2020 13:57:08 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
> > On 23.07.20 08:33, Cornelia Huck wrote:
> > > On Mon, 20 Jul 2020 11:07:51 +0200
> > > David Hildenbrand <david@redhat.com> wrote:
> > >   
> > >> On 20.07.20 11:03, Michael S. Tsirkin wrote:  
> > >>> On Mon, Jul 20, 2020 at 10:09:57AM +0200, David Hildenbrand wrote:    
> > >>>> On 07.07.20 12:54, Cornelia Huck wrote:    
> > >>>>> As discussed in "virtio-fs: force virtio 1.x usage", it seems like
> > >>>>> a good idea to make sure that any new virtio device (which does not
> > >>>>> support legacy virtio) is indeed a non-transitional device, just to
> > >>>>> catch accidental misconfigurations. We can easily compile a list
> > >>>>> of virtio devices with legacy support and have transports verify
> > >>>>> in their plugged callbacks that legacy support is off for any device
> > >>>>> not in that list.
> > >>>>>
> > >>>>> Most new virtio devices force non-transitional already, so nothing
> > >>>>> changes for them. vhost-user-fs-pci even does not allow to configure
> > >>>>> a non-transitional device, so it is fine as well.
> > >>>>>
> > >>>>> One problematic device, however, is virtio-iommu-pci. It currently
> > >>>>> offers both the transitional and the non-transitional variety of the
> > >>>>> device, and does not force anything. I'm unsure whether we should
> > >>>>> consider transitional virtio-iommu unsupported, or if we should add
> > >>>>> some compat handling. (The support for legacy or not generally may
> > >>>>> change based upon the bus, IIUC, so I'm unsure how to come up with
> > >>>>> something generic.)
> > >>>>>
> > >>>>> Cornelia Huck (2):
> > >>>>>   virtio: list legacy-capable devices
> > >>>>>   virtio: verify that legacy support is not accidentally on    
> > >>>>
> > >>>> I'd squash both patches. Looking at patch #1, I wonder why we don't
> > >>>> store that information along with the device implementation? What was
> > >>>> the motivation to define this information separately?    
> > >>>
> > >>> Because people seem to cut and paste code, so when one
> > >>> enables it in an old device, it gets pasted into a new one.
> > >>> With a list in a central place, it's easier to figure out
> > >>> what's going on.    
> > >>
> > >> Makes sense, I suggest adding that to the patch description.  
> > > 
> > > "The list of devices supporting legacy is supposed to be static. We
> > > keep it in a central place to make sure that new devices do not enable
> > > legacy by accident."
> > > 
> > > ?  
> > 
> > Ack!
> > 
> > >   
> > >>
> > >> Both patches look sane to me (- squashing them).
> > >>  
> > > 
> > > Patch 1 does not change behaviour, while patch 2 does (for
> > > virtio-iommu-pci). Still would like an opinion whether changing the
> > > behaviour for virtio-iommu-pci with no compat handling is ok.
> > > 
> > > (I could be persuaded to squash them.)  
> > 
> > I'm a friend of introducing helper functions along with code that
> > actually uses it. But I agree that the change in behavior might be
> > hairy. Maybe we can split that out somehow to give it more attention?
> 
> It should not really be noticeable for anything but virtio-iommu.
> 
> However, I see these are already in a pull request...

Yea, sorry about being hasty.

-- 
MST