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(+)
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
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.
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
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
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
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.)
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
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...
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
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
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.
© 2016 - 2024 Red Hat, Inc.