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

[Cornelia Huck <cohuck@redhat.com>]

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