[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: David Hildenbrand
Subject: Re: [PATCH 0/2] virtio: non-legacy device handling
Date: Thu, 23 Jul 2020 13:57:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

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


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


David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]