Re: [Qemu-devel] [PATCH] virtio-pci: fix missing device properties

[Eduardo Habkost]

On Wed, Jun 26, 2019 at 01:23:33AM +0200, Marc-André Lureau wrote:
> Since commit a4ee4c8baa37154 ("virtio: Helper for registering virtio
> device types"), virtio-gpu-pci, virtio-vga, and virtio-crypto-pci lost
> some properties: "ioeventfd" and "vectors". This may cause various
> issues, such as failing migration or invalid properties.
> 
> Since those VirtioPCI devices do not have a base name, their class are
> initialized with virtio_pci_generic_base_class_init(). However, if the
> VirtioPCIDeviceTypeInfo provided a class_init which sets dc->props,
> the properties were overwritten by virtio_pci_generic_class_init().
> 
> Instead, introduce an intermediary base-type to register the generic
> properties.
> 
> Fixes: a4ee4c8baa37154f42b4dc6a13fee79268d15238
> Cc: address@hidden
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/virtio/virtio-pci.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index e6d5467e54..62c4977332 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1913,13 +1913,6 @@ static void virtio_pci_generic_class_init(ObjectClass 
> *klass, void *data)
>      dc->props = virtio_pci_generic_properties;
>  }
>  
> -/* Used when the generic type and the base type is the same */
> -static void virtio_pci_generic_base_class_init(ObjectClass *klass, void 
> *data)
> -{
> -    virtio_pci_base_class_init(klass, data);
> -    virtio_pci_generic_class_init(klass, NULL);
> -}
> -
>  static void virtio_pci_transitional_instance_init(Object *obj)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> @@ -1938,14 +1931,13 @@ static void 
> virtio_pci_non_transitional_instance_init(Object *obj)
>  
>  void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
>  {
> +    char *base_name = NULL;
>      TypeInfo base_type_info = {
>          .name          = t->base_name,
>          .parent        = t->parent ? t->parent : TYPE_VIRTIO_PCI,
>          .instance_size = t->instance_size,
>          .instance_init = t->instance_init,
>          .class_size    = t->class_size,
> -        .class_init    = virtio_pci_base_class_init,
> -        .class_data    = (void *)t,
>          .abstract      = true,
>      };
>      TypeInfo generic_type_info = {
> @@ -1961,13 +1953,20 @@ void virtio_pci_types_register(const 
> VirtioPCIDeviceTypeInfo *t)
>  
>      if (!base_type_info.name) {
>          /* No base type -> register a single generic device type */
> -        base_type_info.name = t->generic_name;
> -        base_type_info.class_init = virtio_pci_generic_base_class_init;
> -        base_type_info.interfaces = generic_type_info.interfaces;
> -        base_type_info.abstract = false;
> -        generic_type_info.name = NULL;
> +        /* use intermediate %s-base-type to add generic device props */
> +        base_name = g_strdup_printf("%s-base-type", t->generic_name);
> +        base_type_info.name = base_name;
> +        base_type_info.class_init = virtio_pci_generic_class_init;
> +
> +        generic_type_info.parent = base_name;
> +        generic_type_info.class_init = virtio_pci_base_class_init;
> +        generic_type_info.class_data = (void *)t;

Why are you using virtio_pci_generic_class_init for the base
class, and virtio_pci_base_class_init for the subclass, but doing
exactly the opposite when t->base_name is set?

Isn't it simpler to just initialize base_type_info.name and leave
all the rest alone?  Patch below.

Signed-off-by: Eduardo Habkost <address@hidden>
---
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e6d5467e54..3ee50a0783 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1913,13 +1913,6 @@ static void virtio_pci_generic_class_init(ObjectClass 
*klass, void *data)
     dc->props = virtio_pci_generic_properties;
 }
 
-/* Used when the generic type and the base type is the same */
-static void virtio_pci_generic_base_class_init(ObjectClass *klass, void *data)
-{
-    virtio_pci_base_class_init(klass, data);
-    virtio_pci_generic_class_init(klass, NULL);
-}
-
 static void virtio_pci_transitional_instance_init(Object *obj)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
@@ -1938,8 +1931,11 @@ static void 
virtio_pci_non_transitional_instance_init(Object *obj)
 
 void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t)
 {
+    char *base_name = t->base_name ?
+                      NULL :
+                      g_strdup_printf("%s-base-type", t->generic_name);
     TypeInfo base_type_info = {
-        .name          = t->base_name,
+        .name          = t->base_name ?: base_name,
         .parent        = t->parent ? t->parent : TYPE_VIRTIO_PCI,
         .instance_size = t->instance_size,
         .instance_init = t->instance_init,
@@ -1959,21 +1955,8 @@ void virtio_pci_types_register(const 
VirtioPCIDeviceTypeInfo *t)
         },
     };
 
-    if (!base_type_info.name) {
-        /* No base type -> register a single generic device type */
-        base_type_info.name = t->generic_name;
-        base_type_info.class_init = virtio_pci_generic_base_class_init;
-        base_type_info.interfaces = generic_type_info.interfaces;
-        base_type_info.abstract = false;
-        generic_type_info.name = NULL;
-        assert(!t->non_transitional_name);
-        assert(!t->transitional_name);
-    }
-
     type_register(&base_type_info);
-    if (generic_type_info.name) {
-        type_register(&generic_type_info);
-    }
+    type_register(&generic_type_info);
 
     if (t->non_transitional_name) {
         const TypeInfo non_transitional_type_info = {
@@ -2005,6 +1988,7 @@ void virtio_pci_types_register(const 
VirtioPCIDeviceTypeInfo *t)
         };
         type_register(&transitional_type_info);
     }
+    g_free(base_name);
 }
 
 /* virtio-pci-bus */


-- 
Eduardo