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 (m...@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
