compat: Use explicit type names on HW_COMPAT_2_6
diff mbox series

Message ID 20190104032226.21428-1-ehabkost@redhat.com
State New
Headers show
Series
  • compat: Use explicit type names on HW_COMPAT_2_6
Related show

Commit Message

Eduardo Habkost Jan. 4, 2019, 3:22 a.m. UTC
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(-)

Comments

Michael S. Tsirkin Jan. 4, 2019, 4:23 a.m. UTC | #1
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
Cornelia Huck Jan. 4, 2019, 9:37 a.m. UTC | #2
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(-)
Cornelia Huck Jan. 4, 2019, 9:49 a.m. UTC | #3
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>
Dr. David Alan Gilbert (git) Jan. 4, 2019, 10:12 a.m. UTC | #4
* 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
Eduardo Habkost Jan. 4, 2019, 4:08 p.m. UTC | #5
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
Eduardo Habkost Jan. 4, 2019, 4:11 p.m. UTC | #6
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>
Eduardo Habkost Jan. 4, 2019, 5:54 p.m. UTC | #7
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.
Eduardo Habkost Jan. 4, 2019, 8:09 p.m. UTC | #8
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)
Michael S. Tsirkin Jan. 4, 2019, 8:48 p.m. UTC | #9
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
Eduardo Habkost Jan. 4, 2019, 9:06 p.m. UTC | #10
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.
Michael S. Tsirkin Jan. 4, 2019, 9:13 p.m. UTC | #11
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?
Eduardo Habkost Jan. 4, 2019, 10 p.m. UTC | #12
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.
Cornelia Huck Jan. 7, 2019, 8:12 a.m. UTC | #13
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?
Eduardo Habkost Jan. 7, 2019, 7:26 p.m. UTC | #14
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.

Patch
diff mbox series

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",\
     },