On 23.07.20 08:33, Cornelia Huck wrote: > On Mon, 20 Jul 2020 11:07:51 +0200 > David Hildenbrand <da...@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
