| Message ID | 20190104032226.21428-1-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | compat: Use explicit type names on HW_COMPAT_2_6 | expand |
On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > assumption: that all virtio-pci subclasses support the > disable-legacy and disable-modern options. > > That assumption was broken by commit f6e501a28ef9 ("virtio: > Provide version-specific variants of virtio PCI devices"). This > caused QEMU to crash if using the new -non-transitional or > -transitional device types: > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > -device virtio-net-pci-non-transitional > Unexpected error in object_property_find() at qom/object.c:1092: > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > Aborted (core dumped) > > Replace the virtio-pci.disable-legacy=off and > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > explicit entries for each generic virtio device type. > > The full list of generic virtio device types was extracted by > just grepping for ".generic_name". Note that we don't need to > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > the future, because new devices won't require QEMU 2.6 > compatibility. I fully expect that e.g. packed ring support will need to affect all virtio devices too. And it's likely that we'll have some new virtio-pci transport features too. > This makes the compat entries annoyingly verbose, but is simpler > than the alternative of making the virtio-pci type inheritance > rules even more complex. God forbid we forgot something, the only way to notice is to run a cross version migration with an old qemu. I think we need to come up with something less verbose and fragile. > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > --- > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 138 insertions(+), 2 deletions(-) > > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 3ca85b037c..fbb6c1138a 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -163,11 +163,147 @@ > .property = "format_transport_address",\ > .value = "off",\ > },{\ > - .driver = "virtio-pci",\ > + .driver = "vhost-scsi-pci",\ > .property = "disable-modern",\ > .value = "on",\ > },{\ > - .driver = "virtio-pci",\ > + .driver = "vhost-scsi-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "vhost-user-blk-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "vhost-user-blk-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "vhost-user-scsi-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "vhost-user-scsi-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "vhost-vsock-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "vhost-vsock-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-9p-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-9p-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-crypto-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-crypto-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > + },{\ > + .driver = "virtio-vga",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-vga",\ > .property = "disable-legacy",\ > .value = "off",\ > }, > -- > 2.18.0.rc1.1.g3f1ff2140
On Thu, 3 Jan 2019 23:23:45 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > assumption: that all virtio-pci subclasses support the > > disable-legacy and disable-modern options. > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > Provide version-specific variants of virtio PCI devices"). This > > caused QEMU to crash if using the new -non-transitional or > > -transitional device types: > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > -device virtio-net-pci-non-transitional > > Unexpected error in object_property_find() at qom/object.c:1092: > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > Aborted (core dumped) > > > > Replace the virtio-pci.disable-legacy=off and > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > explicit entries for each generic virtio device type. > > > > The full list of generic virtio device types was extracted by > > just grepping for ".generic_name". Note that we don't need to > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > the future, because new devices won't require QEMU 2.6 > > compatibility. > > I fully expect that e.g. packed ring support will need > to affect all virtio devices too. And it's likely > that we'll have some new virtio-pci transport features too. Yes, but as this post-dates the introduction of the version-specific devices, it will all go into the base class and therefore not be problematic. > > > This makes the compat entries annoyingly verbose, but is simpler > > than the alternative of making the virtio-pci type inheritance > > rules even more complex. > > God forbid we forgot something, the only way to notice is to > run a cross version migration with an old qemu. > I think we need to come up with something less verbose and fragile. I'd rather prefer to do this change now, so that we don't need to drag around some complicated infrastructure forever. As Eduardo said, we don't need to care about new devices, so this grep-based approach sounds like it would catch everything. > > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > --- > > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 138 insertions(+), 2 deletions(-)
On Fri, 4 Jan 2019 01:22:26 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > assumption: that all virtio-pci subclasses support the > disable-legacy and disable-modern options. > > That assumption was broken by commit f6e501a28ef9 ("virtio: > Provide version-specific variants of virtio PCI devices"). This > caused QEMU to crash if using the new -non-transitional or > -transitional device types: > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > -device virtio-net-pci-non-transitional > Unexpected error in object_property_find() at qom/object.c:1092: > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > Aborted (core dumped) > > Replace the virtio-pci.disable-legacy=off and > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > explicit entries for each generic virtio device type. > > The full list of generic virtio device types was extracted by > just grepping for ".generic_name". Note that we don't need to > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > the future, because new devices won't require QEMU 2.6 > compatibility. Some of these will not be relevant for 2.6 compat, but better safe than sorry :) > > This makes the compat entries annoyingly verbose, but is simpler > than the alternative of making the virtio-pci type inheritance > rules even more complex. Maybe also write out explicitly: Fixes: f6e501a28ef9 ("virtio: Provide version-specific variants of virtio PCI devices") > > Reported-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > --- > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 138 insertions(+), 2 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
* Michael S. Tsirkin (mst@redhat.com) wrote: > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > assumption: that all virtio-pci subclasses support the > > disable-legacy and disable-modern options. > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > Provide version-specific variants of virtio PCI devices"). This > > caused QEMU to crash if using the new -non-transitional or > > -transitional device types: > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > -device virtio-net-pci-non-transitional > > Unexpected error in object_property_find() at qom/object.c:1092: > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > Aborted (core dumped) > > > > Replace the virtio-pci.disable-legacy=off and > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > explicit entries for each generic virtio device type. > > > > The full list of generic virtio device types was extracted by > > just grepping for ".generic_name". Note that we don't need to > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > the future, because new devices won't require QEMU 2.6 > > compatibility. > > I fully expect that e.g. packed ring support will need > to affect all virtio devices too. And it's likely > that we'll have some new virtio-pci transport features too. > > > This makes the compat entries annoyingly verbose, but is simpler > > than the alternative of making the virtio-pci type inheritance > > rules even more complex. > > God forbid we forgot something, the only way to notice is to > run a cross version migration with an old qemu. > I think we need to come up with something less verbose and fragile. I guess we could use a script like tests/acceptance/virtio_version.py to do a check? As for something less verbose, I guess something is doable with a few macros (more complex but less verbose); or I guess you could add these properties to the other devices but just refuse to operate when they were set the wrong way. Dave > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > --- > > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 138 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 3ca85b037c..fbb6c1138a 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -163,11 +163,147 @@ > > .property = "format_transport_address",\ > > .value = "off",\ > > },{\ > > - .driver = "virtio-pci",\ > > + .driver = "vhost-scsi-pci",\ > > .property = "disable-modern",\ > > .value = "on",\ > > },{\ > > - .driver = "virtio-pci",\ > > + .driver = "vhost-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-user-blk-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-user-blk-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-user-scsi-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-user-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-vsock-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-vsock-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-9p-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-9p-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-balloon-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-balloon-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-blk-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-blk-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-crypto-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-crypto-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-gpu-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-gpu-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-input-host-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-input-host-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-keyboard-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-keyboard-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-mouse-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-mouse-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-net-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-net-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-rng-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-rng-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-scsi-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-serial-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-serial-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-tablet-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-tablet-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-vga",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-vga",\ > > .property = "disable-legacy",\ > > .value = "off",\ > > }, > > -- > > 2.18.0.rc1.1.g3f1ff2140 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jan 03, 2019 at 11:23:45PM -0500, Michael S. Tsirkin wrote: > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > assumption: that all virtio-pci subclasses support the > > disable-legacy and disable-modern options. > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > Provide version-specific variants of virtio PCI devices"). This > > caused QEMU to crash if using the new -non-transitional or > > -transitional device types: > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > -device virtio-net-pci-non-transitional > > Unexpected error in object_property_find() at qom/object.c:1092: > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > Aborted (core dumped) > > > > Replace the virtio-pci.disable-legacy=off and > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > explicit entries for each generic virtio device type. > > > > The full list of generic virtio device types was extracted by > > just grepping for ".generic_name". Note that we don't need to > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > the future, because new devices won't require QEMU 2.6 > > compatibility. > > I fully expect that e.g. packed ring support will need > to affect all virtio devices too. And it's likely > that we'll have some new virtio-pci transport features too. If you need to affect all virtio devices, you can still use .driver="virtio-pci" on compat_props. This is just about the disable-legacy and disable-modern properties. > > > This makes the compat entries annoyingly verbose, but is simpler > > than the alternative of making the virtio-pci type inheritance > > rules even more complex. > > God forbid we forgot something, the only way to notice is to > run a cross version migration with an old qemu. > I think we need to come up with something less verbose and fragile. Both approaches can break if we forget something or do something wrong. I don't think this one is more fragile than the others. The difference is that this one lets us list the device types once on compat_props and never think about it again. The others would require us to worry forever about subtle interactions between the disable-modern/disable-legacy properties and QOM inheritance. > > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > --- > > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 138 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 3ca85b037c..fbb6c1138a 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -163,11 +163,147 @@ > > .property = "format_transport_address",\ > > .value = "off",\ > > },{\ > > - .driver = "virtio-pci",\ > > + .driver = "vhost-scsi-pci",\ > > .property = "disable-modern",\ > > .value = "on",\ > > },{\ > > - .driver = "virtio-pci",\ > > + .driver = "vhost-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-user-blk-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-user-blk-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-user-scsi-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-user-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "vhost-vsock-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "vhost-vsock-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-9p-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-9p-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-balloon-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-balloon-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-blk-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-blk-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-crypto-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-crypto-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-gpu-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-gpu-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-input-host-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-input-host-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-keyboard-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-keyboard-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-mouse-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-mouse-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-net-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-net-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-rng-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-rng-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-scsi-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-scsi-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-serial-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-serial-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-tablet-pci",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-tablet-pci",\ > > + .property = "disable-legacy",\ > > + .value = "off",\ > > + },{\ > > + .driver = "virtio-vga",\ > > + .property = "disable-modern",\ > > + .value = "on",\ > > + },{\ > > + .driver = "virtio-vga",\ > > .property = "disable-legacy",\ > > .value = "off",\ > > }, > > -- > > 2.18.0.rc1.1.g3f1ff2140
On Fri, Jan 04, 2019 at 10:49:19AM +0100, Cornelia Huck wrote: > On Fri, 4 Jan 2019 01:22:26 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > assumption: that all virtio-pci subclasses support the > > disable-legacy and disable-modern options. > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > Provide version-specific variants of virtio PCI devices"). This > > caused QEMU to crash if using the new -non-transitional or > > -transitional device types: > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > -device virtio-net-pci-non-transitional > > Unexpected error in object_property_find() at qom/object.c:1092: > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > Aborted (core dumped) > > > > Replace the virtio-pci.disable-legacy=off and > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > explicit entries for each generic virtio device type. > > > > The full list of generic virtio device types was extracted by > > just grepping for ".generic_name". Note that we don't need to > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > the future, because new devices won't require QEMU 2.6 > > compatibility. > > Some of these will not be relevant for 2.6 compat, but better safe than > sorry :) Yes. My first version was based on grepping the v2.6.0 tree, but then I decided to be safer and grep for ".generic_name" on qemu.git master. Just in case somebody is using a v2.7+ device type with 2.6 machine-types. > > > > > This makes the compat entries annoyingly verbose, but is simpler > > than the alternative of making the virtio-pci type inheritance > > rules even more complex. > > Maybe also write out explicitly: > > Fixes: f6e501a28ef9 ("virtio: Provide version-specific variants of virtio PCI devices") Yes, I forgot to do it. Thanks no noting! > > > > > Reported-by: Thomas Huth <thuth@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > --- > > include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 138 insertions(+), 2 deletions(-) > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > assumption: that all virtio-pci subclasses support the > > > disable-legacy and disable-modern options. > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > Provide version-specific variants of virtio PCI devices"). This > > > caused QEMU to crash if using the new -non-transitional or > > > -transitional device types: > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > -device virtio-net-pci-non-transitional > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > Aborted (core dumped) > > > > > > Replace the virtio-pci.disable-legacy=off and > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > explicit entries for each generic virtio device type. > > > > > > The full list of generic virtio device types was extracted by > > > just grepping for ".generic_name". Note that we don't need to > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > the future, because new devices won't require QEMU 2.6 > > > compatibility. > > > > I fully expect that e.g. packed ring support will need > > to affect all virtio devices too. And it's likely > > that we'll have some new virtio-pci transport features too. > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > than the alternative of making the virtio-pci type inheritance > > > rules even more complex. > > > > God forbid we forgot something, the only way to notice is to > > run a cross version migration with an old qemu. > > I think we need to come up with something less verbose and fragile. > > I guess we could use a script like tests/acceptance/virtio_version.py to > do a check? That's a good idea. On test code we can try additional tricks to detect the hybrid virtio devices without increasing the complexity of QEMU code. I'll give it a try. > > As for something less verbose, I guess something is doable with a few > macros (more complex but less verbose); or I guess you could add these > properties to the other devices but just refuse to operate when they > were set the wrong way. Adding the properties to all virtio-pci devices is the kind of complexity I would like to avoid. If we need to define rules for when disable-modern/disable-legacy is set to unexpected values, we'll have to maintain the code that implement those rules forever.
On Fri, Jan 04, 2019 at 03:54:39PM -0200, Eduardo Habkost wrote: > On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > > assumption: that all virtio-pci subclasses support the > > > > disable-legacy and disable-modern options. > > > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > > Provide version-specific variants of virtio PCI devices"). This > > > > caused QEMU to crash if using the new -non-transitional or > > > > -transitional device types: > > > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > > -device virtio-net-pci-non-transitional > > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > > Aborted (core dumped) > > > > > > > > Replace the virtio-pci.disable-legacy=off and > > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > > explicit entries for each generic virtio device type. > > > > > > > > The full list of generic virtio device types was extracted by > > > > just grepping for ".generic_name". Note that we don't need to > > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > > the future, because new devices won't require QEMU 2.6 > > > > compatibility. > > > > > > I fully expect that e.g. packed ring support will need > > > to affect all virtio devices too. And it's likely > > > that we'll have some new virtio-pci transport features too. > > > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > > than the alternative of making the virtio-pci type inheritance > > > > rules even more complex. > > > > > > God forbid we forgot something, the only way to notice is to > > > run a cross version migration with an old qemu. > > > I think we need to come up with something less verbose and fragile. > > > > I guess we could use a script like tests/acceptance/virtio_version.py to > > do a check? > > That's a good idea. On test code we can try additional tricks to > detect the hybrid virtio devices without increasing the > complexity of QEMU code. I'll give it a try. I did it but I'm not happy with the result: many of the virtio devices can't be tested without extra arguments. Some of them (like vhost-*) require extra privileges on the host that might be unavailable. Anyway, while writing this I noticed another issue: many of the virtio devices in QEMU 2.6 were already modern-only! Setting disable-modern=off on modern-only devices like virtio-vga or virtio-tablet-pci doesn't make sense. This means setting virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even before the -non-transitional and -transitional device types were introduced. --- diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py index ce990250d8..9157a1b173 100644 --- a/tests/acceptance/virtio_version.py +++ b/tests/acceptance/virtio_version.py @@ -55,6 +55,18 @@ def get_pci_interfaces(vm, devtype): interfaces = ('pci-express-device', 'conventional-pci-device') return [i for i in interfaces if devtype_implements(vm, devtype, i)] +def is_hybrid_dev(vm, devtype): + props = [p['name'] for p in vm.command('device-list-properties', + typename=devtype)] + return 'disable-legacy' in props + +def get_all_hybrid_devs(vm): + """Return list of all hybrid virtio device types (the ones that can be + configured using the disable-legacy & disable-modern properties)""" + alldevs = [d['name'] for d in vm.command('qom-list-types', + implements='virtio-pci')] + return [d for d in alldevs if is_hybrid_dev(vm, d)] + class VirtioVersionCheck(Test): """ Check if virtio-version-specific device types result in the @@ -174,3 +186,33 @@ class VirtioVersionCheck(Test): self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT) self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT) self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT) + + def get_all_hybrid_devs(self): + with QEMUMachine(self.qemu_bin) as vm: + vm.set_machine('none') + vm.add_args('-S') + vm.launch() + return get_all_hybrid_devs(vm) + + def check_legacy_compat(self, qemu_devtype): + # these device types can't be run with extra arguments, so we can't + # test them directly: + if qemu_devtype in ('virtio-input-host-pci', 'virtio-blk-pci', + 'virtio-crypto-pci', 'virtio-9p-pci', + 'vhost-vsock-pci', 'vhost-scsi-pci', + 'vhost-user-blk-pci', 'vhost-user-scsi-pci'): + return + + # Force legacy mode: + dev_legacy, _ = self.run_device(qemu_devtype, + 'disable-modern=on,disable-legacy=off', + machine='pc-i440fx-2.6') + + # No options: default to legacy on <= 2.6 machine-types: + no_opts_pc, _ = self.run_device(qemu_devtype, machine='pc-i440fx-2.6') + self.assertEqual(dev_legacy, no_opts_pc) + + def test_legacy_compat(self): + devs = self.get_all_hybrid_devs() + for d in devs: + self.check_legacy_compat(d)
On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > On Fri, Jan 04, 2019 at 03:54:39PM -0200, Eduardo Habkost wrote: > > On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > > > assumption: that all virtio-pci subclasses support the > > > > > disable-legacy and disable-modern options. > > > > > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > > > Provide version-specific variants of virtio PCI devices"). This > > > > > caused QEMU to crash if using the new -non-transitional or > > > > > -transitional device types: > > > > > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > > > -device virtio-net-pci-non-transitional > > > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > > > Aborted (core dumped) > > > > > > > > > > Replace the virtio-pci.disable-legacy=off and > > > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > > > explicit entries for each generic virtio device type. > > > > > > > > > > The full list of generic virtio device types was extracted by > > > > > just grepping for ".generic_name". Note that we don't need to > > > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > > > the future, because new devices won't require QEMU 2.6 > > > > > compatibility. > > > > > > > > I fully expect that e.g. packed ring support will need > > > > to affect all virtio devices too. And it's likely > > > > that we'll have some new virtio-pci transport features too. > > > > > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > > > than the alternative of making the virtio-pci type inheritance > > > > > rules even more complex. > > > > > > > > God forbid we forgot something, the only way to notice is to > > > > run a cross version migration with an old qemu. > > > > I think we need to come up with something less verbose and fragile. > > > > > > I guess we could use a script like tests/acceptance/virtio_version.py to > > > do a check? > > > > That's a good idea. On test code we can try additional tricks to > > detect the hybrid virtio devices without increasing the > > complexity of QEMU code. I'll give it a try. > > I did it but I'm not happy with the result: many of the virtio > devices can't be tested without extra arguments. Some of them > (like vhost-*) require extra privileges on the host that might be > unavailable. > > Anyway, while writing this I noticed another issue: many of the > virtio devices in QEMU 2.6 were already modern-only! > > Setting disable-modern=off on modern-only devices like virtio-vga > or virtio-tablet-pci doesn't make sense. This means setting > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > before the -non-transitional and -transitional device types were > introduced. It did create an opportunity to create non working devices. Whether that's incorrect as such I'm not sure. > --- > diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py > index ce990250d8..9157a1b173 100644 > --- a/tests/acceptance/virtio_version.py > +++ b/tests/acceptance/virtio_version.py > @@ -55,6 +55,18 @@ def get_pci_interfaces(vm, devtype): > interfaces = ('pci-express-device', 'conventional-pci-device') > return [i for i in interfaces if devtype_implements(vm, devtype, i)] > > +def is_hybrid_dev(vm, devtype): > + props = [p['name'] for p in vm.command('device-list-properties', > + typename=devtype)] > + return 'disable-legacy' in props > + > +def get_all_hybrid_devs(vm): > + """Return list of all hybrid virtio device types (the ones that can be > + configured using the disable-legacy & disable-modern properties)""" > + alldevs = [d['name'] for d in vm.command('qom-list-types', > + implements='virtio-pci')] > + return [d for d in alldevs if is_hybrid_dev(vm, d)] > + > class VirtioVersionCheck(Test): > """ > Check if virtio-version-specific device types result in the > @@ -174,3 +186,33 @@ class VirtioVersionCheck(Test): > self.check_modern_only('virtio-mouse-pci', VIRTIO_INPUT) > self.check_modern_only('virtio-tablet-pci', VIRTIO_INPUT) > self.check_modern_only('virtio-keyboard-pci', VIRTIO_INPUT) > + > + def get_all_hybrid_devs(self): > + with QEMUMachine(self.qemu_bin) as vm: > + vm.set_machine('none') > + vm.add_args('-S') > + vm.launch() > + return get_all_hybrid_devs(vm) > + > + def check_legacy_compat(self, qemu_devtype): > + # these device types can't be run with extra arguments, so we can't > + # test them directly: > + if qemu_devtype in ('virtio-input-host-pci', 'virtio-blk-pci', > + 'virtio-crypto-pci', 'virtio-9p-pci', > + 'vhost-vsock-pci', 'vhost-scsi-pci', > + 'vhost-user-blk-pci', 'vhost-user-scsi-pci'): > + return > + > + # Force legacy mode: > + dev_legacy, _ = self.run_device(qemu_devtype, > + 'disable-modern=on,disable-legacy=off', > + machine='pc-i440fx-2.6') > + > + # No options: default to legacy on <= 2.6 machine-types: > + no_opts_pc, _ = self.run_device(qemu_devtype, machine='pc-i440fx-2.6') > + self.assertEqual(dev_legacy, no_opts_pc) > + > + def test_legacy_compat(self): > + devs = self.get_all_hybrid_devs() > + for d in devs: > + self.check_legacy_compat(d) > > -- > Eduardo
On Fri, Jan 04, 2019 at 03:48:02PM -0500, Michael S. Tsirkin wrote: > On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > > On Fri, Jan 04, 2019 at 03:54:39PM -0200, Eduardo Habkost wrote: > > > On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > > > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > > > > assumption: that all virtio-pci subclasses support the > > > > > > disable-legacy and disable-modern options. > > > > > > > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > > > > Provide version-specific variants of virtio PCI devices"). This > > > > > > caused QEMU to crash if using the new -non-transitional or > > > > > > -transitional device types: > > > > > > > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > > > > -device virtio-net-pci-non-transitional > > > > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > > > > Aborted (core dumped) > > > > > > > > > > > > Replace the virtio-pci.disable-legacy=off and > > > > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > > > > explicit entries for each generic virtio device type. > > > > > > > > > > > > The full list of generic virtio device types was extracted by > > > > > > just grepping for ".generic_name". Note that we don't need to > > > > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > > > > the future, because new devices won't require QEMU 2.6 > > > > > > compatibility. > > > > > > > > > > I fully expect that e.g. packed ring support will need > > > > > to affect all virtio devices too. And it's likely > > > > > that we'll have some new virtio-pci transport features too. > > > > > > > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > > > > than the alternative of making the virtio-pci type inheritance > > > > > > rules even more complex. > > > > > > > > > > God forbid we forgot something, the only way to notice is to > > > > > run a cross version migration with an old qemu. > > > > > I think we need to come up with something less verbose and fragile. > > > > > > > > I guess we could use a script like tests/acceptance/virtio_version.py to > > > > do a check? > > > > > > That's a good idea. On test code we can try additional tricks to > > > detect the hybrid virtio devices without increasing the > > > complexity of QEMU code. I'll give it a try. > > > > I did it but I'm not happy with the result: many of the virtio > > devices can't be tested without extra arguments. Some of them > > (like vhost-*) require extra privileges on the host that might be > > unavailable. > > > > Anyway, while writing this I noticed another issue: many of the > > virtio devices in QEMU 2.6 were already modern-only! > > > > Setting disable-modern=off on modern-only devices like virtio-vga > > or virtio-tablet-pci doesn't make sense. This means setting > > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > > before the -non-transitional and -transitional device types were > > introduced. > > > It did create an opportunity to create non working devices. > > Whether that's incorrect as such I'm not sure. This is not just creating the opportunity for an user to disable-modern=on. HW_COMPAT_2_6 is actually setting disable-modern=on on virtio-vga and other modern-only devices. Sounds like a mistake to me. Luckily those modern-only devices silently ignore the disable-modern/disable-legacy properties, but this might change in the future.
On Fri, Jan 04, 2019 at 07:06:56PM -0200, Eduardo Habkost wrote: > On Fri, Jan 04, 2019 at 03:48:02PM -0500, Michael S. Tsirkin wrote: > > On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > > > On Fri, Jan 04, 2019 at 03:54:39PM -0200, Eduardo Habkost wrote: > > > > On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > > > > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > > > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > > > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > > > > > assumption: that all virtio-pci subclasses support the > > > > > > > disable-legacy and disable-modern options. > > > > > > > > > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > > > > > Provide version-specific variants of virtio PCI devices"). This > > > > > > > caused QEMU to crash if using the new -non-transitional or > > > > > > > -transitional device types: > > > > > > > > > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > > > > > -device virtio-net-pci-non-transitional > > > > > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > > > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > > > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > Replace the virtio-pci.disable-legacy=off and > > > > > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > > > > > explicit entries for each generic virtio device type. > > > > > > > > > > > > > > The full list of generic virtio device types was extracted by > > > > > > > just grepping for ".generic_name". Note that we don't need to > > > > > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > > > > > the future, because new devices won't require QEMU 2.6 > > > > > > > compatibility. > > > > > > > > > > > > I fully expect that e.g. packed ring support will need > > > > > > to affect all virtio devices too. And it's likely > > > > > > that we'll have some new virtio-pci transport features too. > > > > > > > > > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > > > > > than the alternative of making the virtio-pci type inheritance > > > > > > > rules even more complex. > > > > > > > > > > > > God forbid we forgot something, the only way to notice is to > > > > > > run a cross version migration with an old qemu. > > > > > > I think we need to come up with something less verbose and fragile. > > > > > > > > > > I guess we could use a script like tests/acceptance/virtio_version.py to > > > > > do a check? > > > > > > > > That's a good idea. On test code we can try additional tricks to > > > > detect the hybrid virtio devices without increasing the > > > > complexity of QEMU code. I'll give it a try. > > > > > > I did it but I'm not happy with the result: many of the virtio > > > devices can't be tested without extra arguments. Some of them > > > (like vhost-*) require extra privileges on the host that might be > > > unavailable. > > > > > > Anyway, while writing this I noticed another issue: many of the > > > virtio devices in QEMU 2.6 were already modern-only! > > > > > > Setting disable-modern=off on modern-only devices like virtio-vga > > > or virtio-tablet-pci doesn't make sense. This means setting > > > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > > > before the -non-transitional and -transitional device types were > > > introduced. > > > > > > It did create an opportunity to create non working devices. > > > > Whether that's incorrect as such I'm not sure. > > This is not just creating the opportunity for an user to > disable-modern=on. HW_COMPAT_2_6 is actually setting > disable-modern=on on virtio-vga and other modern-only devices. > Sounds like a mistake to me. > > Luckily those modern-only devices silently ignore the > disable-modern/disable-legacy properties, but this might change > in the future. Worry about it then?
On Fri, Jan 04, 2019 at 04:13:15PM -0500, Michael S. Tsirkin wrote: > On Fri, Jan 04, 2019 at 07:06:56PM -0200, Eduardo Habkost wrote: > > On Fri, Jan 04, 2019 at 03:48:02PM -0500, Michael S. Tsirkin wrote: > > > On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > > > > On Fri, Jan 04, 2019 at 03:54:39PM -0200, Eduardo Habkost wrote: > > > > > On Fri, Jan 04, 2019 at 10:12:00AM +0000, Dr. David Alan Gilbert wrote: > > > > > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > > > > > On Fri, Jan 04, 2019 at 01:22:26AM -0200, Eduardo Habkost wrote: > > > > > > > > The virtio-pci entries in HW_COMPAT_2_6 had an implicit > > > > > > > > assumption: that all virtio-pci subclasses support the > > > > > > > > disable-legacy and disable-modern options. > > > > > > > > > > > > > > > > That assumption was broken by commit f6e501a28ef9 ("virtio: > > > > > > > > Provide version-specific variants of virtio PCI devices"). This > > > > > > > > caused QEMU to crash if using the new -non-transitional or > > > > > > > > -transitional device types: > > > > > > > > > > > > > > > > $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ > > > > > > > > -device virtio-net-pci-non-transitional > > > > > > > > Unexpected error in object_property_find() at qom/object.c:1092: > > > > > > > > qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ > > > > > > > > global virtio-pci.disable-modern=on: Property '.disable-modern' not found > > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > > > Replace the virtio-pci.disable-legacy=off and > > > > > > > > virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with > > > > > > > > explicit entries for each generic virtio device type. > > > > > > > > > > > > > > > > The full list of generic virtio device types was extracted by > > > > > > > > just grepping for ".generic_name". Note that we don't need to > > > > > > > > worry about listing new virtio-pci devices in HW_COMPAT_2_6 in > > > > > > > > the future, because new devices won't require QEMU 2.6 > > > > > > > > compatibility. > > > > > > > > > > > > > > I fully expect that e.g. packed ring support will need > > > > > > > to affect all virtio devices too. And it's likely > > > > > > > that we'll have some new virtio-pci transport features too. > > > > > > > > > > > > > > > This makes the compat entries annoyingly verbose, but is simpler > > > > > > > > than the alternative of making the virtio-pci type inheritance > > > > > > > > rules even more complex. > > > > > > > > > > > > > > God forbid we forgot something, the only way to notice is to > > > > > > > run a cross version migration with an old qemu. > > > > > > > I think we need to come up with something less verbose and fragile. > > > > > > > > > > > > I guess we could use a script like tests/acceptance/virtio_version.py to > > > > > > do a check? > > > > > > > > > > That's a good idea. On test code we can try additional tricks to > > > > > detect the hybrid virtio devices without increasing the > > > > > complexity of QEMU code. I'll give it a try. > > > > > > > > I did it but I'm not happy with the result: many of the virtio > > > > devices can't be tested without extra arguments. Some of them > > > > (like vhost-*) require extra privileges on the host that might be > > > > unavailable. > > > > > > > > Anyway, while writing this I noticed another issue: many of the > > > > virtio devices in QEMU 2.6 were already modern-only! > > > > > > > > Setting disable-modern=off on modern-only devices like virtio-vga > > > > or virtio-tablet-pci doesn't make sense. This means setting > > > > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > > > > before the -non-transitional and -transitional device types were > > > > introduced. > > > > > > > > > It did create an opportunity to create non working devices. > > > > > > Whether that's incorrect as such I'm not sure. > > > > This is not just creating the opportunity for an user to > > disable-modern=on. HW_COMPAT_2_6 is actually setting > > disable-modern=on on virtio-vga and other modern-only devices. > > Sounds like a mistake to me. > > > > Luckily those modern-only devices silently ignore the > > disable-modern/disable-legacy properties, but this might change > > in the future. > > Worry about it then? Right, we don't need to worry about it today. But if a solution to the crash reported by Thomas will make the problem go away, that's even better.
On Fri, 4 Jan 2019 20:00:50 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Jan 04, 2019 at 04:13:15PM -0500, Michael S. Tsirkin wrote: > > On Fri, Jan 04, 2019 at 07:06:56PM -0200, Eduardo Habkost wrote: > > > On Fri, Jan 04, 2019 at 03:48:02PM -0500, Michael S. Tsirkin wrote: > > > > On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > > > > > Anyway, while writing this I noticed another issue: many of the > > > > > virtio devices in QEMU 2.6 were already modern-only! > > > > > > > > > > Setting disable-modern=off on modern-only devices like virtio-vga > > > > > or virtio-tablet-pci doesn't make sense. This means setting > > > > > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > > > > > before the -non-transitional and -transitional device types were > > > > > introduced. > > > > > > > > > > > > It did create an opportunity to create non working devices. > > > > > > > > Whether that's incorrect as such I'm not sure. > > > > > > This is not just creating the opportunity for an user to > > > disable-modern=on. HW_COMPAT_2_6 is actually setting > > > disable-modern=on on virtio-vga and other modern-only devices. > > > Sounds like a mistake to me. > > > > > > Luckily those modern-only devices silently ignore the > > > disable-modern/disable-legacy properties, but this might change > > > in the future. > > > > Worry about it then? > > Right, we don't need to worry about it today. But if a solution > to the crash reported by Thomas will make the problem go away, > that's even better. It seems your patch with the modern-only devices removed from the list would achieve that?
On Mon, Jan 07, 2019 at 09:12:06AM +0100, Cornelia Huck wrote: > On Fri, 4 Jan 2019 20:00:50 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Jan 04, 2019 at 04:13:15PM -0500, Michael S. Tsirkin wrote: > > > On Fri, Jan 04, 2019 at 07:06:56PM -0200, Eduardo Habkost wrote: > > > > On Fri, Jan 04, 2019 at 03:48:02PM -0500, Michael S. Tsirkin wrote: > > > > > On Fri, Jan 04, 2019 at 06:09:52PM -0200, Eduardo Habkost wrote: > > > > > > > Anyway, while writing this I noticed another issue: many of the > > > > > > virtio devices in QEMU 2.6 were already modern-only! > > > > > > > > > > > > Setting disable-modern=off on modern-only devices like virtio-vga > > > > > > or virtio-tablet-pci doesn't make sense. This means setting > > > > > > virtio-pci.disable-modern=off on HW_COMPAT_2_6 was incorrect even > > > > > > before the -non-transitional and -transitional device types were > > > > > > introduced. > > > > > > > > > > > > > > > It did create an opportunity to create non working devices. > > > > > > > > > > Whether that's incorrect as such I'm not sure. > > > > > > > > This is not just creating the opportunity for an user to > > > > disable-modern=on. HW_COMPAT_2_6 is actually setting > > > > disable-modern=on on virtio-vga and other modern-only devices. > > > > Sounds like a mistake to me. > > > > > > > > Luckily those modern-only devices silently ignore the > > > > disable-modern/disable-legacy properties, but this might change > > > > in the future. > > > > > > Worry about it then? > > > > Right, we don't need to worry about it today. But if a solution > > to the crash reported by Thomas will make the problem go away, > > that's even better. > > It seems your patch with the modern-only devices removed from the list > would achieve that? I think so. But I think I'll do the removal of modern-only devices from the list in a separate patch, just to be safe.
diff --git a/include/hw/compat.h b/include/hw/compat.h index 3ca85b037c..fbb6c1138a 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -163,11 +163,147 @@ .property = "format_transport_address",\ .value = "off",\ },{\ - .driver = "virtio-pci",\ + .driver = "vhost-scsi-pci",\ .property = "disable-modern",\ .value = "on",\ },{\ - .driver = "virtio-pci",\ + .driver = "vhost-scsi-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "vhost-user-blk-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "vhost-user-blk-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "vhost-user-scsi-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "vhost-user-scsi-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "vhost-vsock-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "vhost-vsock-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-9p-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-9p-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-balloon-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-balloon-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-blk-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-blk-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-crypto-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-crypto-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-gpu-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-gpu-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-input-host-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-input-host-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-keyboard-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-keyboard-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-mouse-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-mouse-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-net-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-net-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-rng-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-rng-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-scsi-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-scsi-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-serial-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-serial-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-tablet-pci",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-tablet-pci",\ + .property = "disable-legacy",\ + .value = "off",\ + },{\ + .driver = "virtio-vga",\ + .property = "disable-modern",\ + .value = "on",\ + },{\ + .driver = "virtio-vga",\ .property = "disable-legacy",\ .value = "off",\ },
The virtio-pci entries in HW_COMPAT_2_6 had an implicit assumption: that all virtio-pci subclasses support the disable-legacy and disable-modern options. That assumption was broken by commit f6e501a28ef9 ("virtio: Provide version-specific variants of virtio PCI devices"). This caused QEMU to crash if using the new -non-transitional or -transitional device types: $ qemu-system-x86_64 -machine pc-i440fx-2.6 \ -device virtio-net-pci-non-transitional Unexpected error in object_property_find() at qom/object.c:1092: qemu-system-x86_64: -device virtio-net-pci-non-transitional: can't apply \ global virtio-pci.disable-modern=on: Property '.disable-modern' not found Aborted (core dumped) Replace the virtio-pci.disable-legacy=off and virtio-pci.disable-modern=on entries on HW_COMPAT_2_6 with explicit entries for each generic virtio device type. The full list of generic virtio device types was extracted by just grepping for ".generic_name". Note that we don't need to worry about listing new virtio-pci devices in HW_COMPAT_2_6 in the future, because new devices won't require QEMU 2.6 compatibility. This makes the compat entries annoyingly verbose, but is simpler than the alternative of making the virtio-pci type inheritance rules even more complex. Reported-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- --- include/hw/compat.h | 140 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 138 insertions(+), 2 deletions(-)