[Qemu-devel] [PATCH] qdev: fix the order compat and global properties are applied

[Qemu-devel] [PATCH] qdev: fix the order compat and global properties are applied

[Greg Kurz]

The current code recursively applies global properties from child up to
parent types. This can cause properties passed with the -global option to
be silently overridden by internal compat properties.

This is exactly what happens with virtio-*-pci drivers since commit:

"9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"

Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6
machine types because the internal virtio-pci.disable-modern=on compat
property always prevail.

This patch fixes the issue by reversing the logic: we now go through the
global property list and, for each property, we check if it is applicable
to the device.

This result in compat properties being applied first, in the order they
appear in the HW_COMPAT_* macros, followed by global properties, in they
order appear on the command line.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/core/qdev-properties.c |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 5972108b2699..93a4dfeb0836 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1089,8 +1089,7 @@ int qdev_prop_check_globals(void)
     return ret;
 }
 
-static void qdev_prop_set_globals_for_type(DeviceState *dev,
-                                           const char *typename)
+void qdev_prop_set_globals(DeviceState *dev)
 {
     GList *l;
 
@@ -1098,7 +1097,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
         GlobalProperty *prop = l->data;
         Error *err = NULL;
 
-        if (strcmp(typename, prop->driver) != 0) {
+        if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
             continue;
         }
         prop->used = true;
@@ -1120,16 +1119,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
     }
 }
 
-void qdev_prop_set_globals(DeviceState *dev)
-{
-    ObjectClass *class = object_get_class(OBJECT(dev));
-
-    do {
-        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
-        class = object_class_get_parent(class);
-    } while (class);
-}
-
 /* --- 64bit unsigned int 'size' type --- */
 
 static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,

Re: [Qemu-devel] [PATCH] qdev: fix the order compat and global properties are applied

[Eduardo Habkost]

On Tue, Dec 06, 2016 at 04:41:12PM +0100, Greg Kurz wrote:
> The current code recursively applies global properties from child up to
> parent types. This can cause properties passed with the -global option to
> be silently overridden by internal compat properties.
> 
> This is exactly what happens with virtio-*-pci drivers since commit:
> 
> "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"
> 
> Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6
> machine types because the internal virtio-pci.disable-modern=on compat
> property always prevail.
> 
> This patch fixes the issue by reversing the logic: we now go through the
> global property list and, for each property, we check if it is applicable
> to the device.
> 
> This result in compat properties being applied first, in the order they
> appear in the HW_COMPAT_* macros, followed by global properties, in they
> order appear on the command line.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

FWIW, this is the behavior I would like to see. But:

I think it's too late to change the rules on 2.8. I would like to
change behavior only on 2.9, so we have time to discuss and test
it. I believe the 2.8 fix should be just changing the HW_COMPAT_*
macros to touch only the device subclasses, so we fix the
regression introduced by commit 9a4c0e220d8a without changing any
rules on how -global is handled.

> ---
>  hw/core/qdev-properties.c |   15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 5972108b2699..93a4dfeb0836 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1089,8 +1089,7 @@ int qdev_prop_check_globals(void)
>      return ret;
>  }
>  
> -static void qdev_prop_set_globals_for_type(DeviceState *dev,
> -                                           const char *typename)
> +void qdev_prop_set_globals(DeviceState *dev)
>  {
>      GList *l;
>  
> @@ -1098,7 +1097,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>          GlobalProperty *prop = l->data;
>          Error *err = NULL;
>  
> -        if (strcmp(typename, prop->driver) != 0) {
> +        if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
>              continue;
>          }
>          prop->used = true;
> @@ -1120,16 +1119,6 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev,
>      }
>  }
>  
> -void qdev_prop_set_globals(DeviceState *dev)
> -{
> -    ObjectClass *class = object_get_class(OBJECT(dev));
> -
> -    do {
> -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
> -        class = object_class_get_parent(class);
> -    } while (class);
> -}
> -
>  /* --- 64bit unsigned int 'size' type --- */
>  
>  static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: fix the order compat and global properties are applied

[Cornelia Huck]

On Tue, 6 Dec 2016 14:25:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Dec 06, 2016 at 04:41:12PM +0100, Greg Kurz wrote:
> > The current code recursively applies global properties from child up to
> > parent types. This can cause properties passed with the -global option to
> > be silently overridden by internal compat properties.
> > 
> > This is exactly what happens with virtio-*-pci drivers since commit:
> > 
> > "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"
> > 
> > Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6
> > machine types because the internal virtio-pci.disable-modern=on compat
> > property always prevail.
> > 
> > This patch fixes the issue by reversing the logic: we now go through the
> > global property list and, for each property, we check if it is applicable
> > to the device.
> > 
> > This result in compat properties being applied first, in the order they
> > appear in the HW_COMPAT_* macros, followed by global properties, in they
> > order appear on the command line.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> FWIW, this is the behavior I would like to see. But:
> 
> I think it's too late to change the rules on 2.8. I would like to
> change behavior only on 2.9, so we have time to discuss and test
> it. I believe the 2.8 fix should be just changing the HW_COMPAT_*
> macros to touch only the device subclasses, so we fix the
> regression introduced by commit 9a4c0e220d8a without changing any
> rules on how -global is handled.

Agreed on both counts.