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