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

[Halil Pasic]

On 07/13/2017 06:15 PM, Eduardo Habkost wrote:
> On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote:
>>
>>
>> On 07/12/2017 08:29 PM, Eduardo Habkost wrote:
>>> On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:


 On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> From: Greg Kurz 
[..]
 Code looks good to me. Given that high profile people are happy with the
 patch I won't object on the behavior aspect which I don't understand fully.
 Thus:

 Reviewed-by: Halil Pasic 


 Now a couple of questions for keeping  my clear conscience:
 * What did we test? Since this patch is fixing a problem it
 changes behavior. Did we test if there is something that breaks?

 * The previous version seems to establish a (somewhat strange)
 precedence for the case the same device property (storage object)
 is set via multiple super-classes (e.g. set both by parent and
 parents parent). This seems to have at least been possible,
 and technically it still is I guess. Now instead of most general
 (super class) wins we have last added property wins. I assume it
 isn't a problem, because we don't have something obscure like that.
 Or am I wrong? This obviously connects to the first question.
 (By the way, most specialized wins would not have been that
 surprising given how inheritance and OO usually works. My assumption
 that nobody used this obscure mechanism is largely based on it's
 strangeness).
>>>
>>> Note that we are not changing the behavior when the classes
>>> themselves set different defaults.  Subclasses are still free to
>>> override defaults set by superclasses inside QEMU code, and they
>>> will be unaffected by this series.  What we are changing here are
>>> the semantics of the -global command-line option when applied to
>>> superclasses.
>>
>> I was not referring to this.
> 
> Good.  :)
> 
>>
>>>
>>> The main sources of global properties we have are:
>>> * MachineClass::compat_props> * -global command-line option
>>
>> I was thinking about these two.
> 
> Good, this is what we're really trying to address, so let's
> review that:
> 
>>
>>> * -cpu command-line option
>>>
>>> The behavior on the compat_props case was addressed by the hack
>>> in commit 0bcba41f "machine: Convert abstract typename on
>>> compat_props to subclass names".  This means compat_props was
>>> already following the order in which properties were registered.
>>
>> I disagree. Commit 0bcba41f affects only *abstract* classes. So
>> your statement is true if a non-abstract class inheriting form
>> device can only have abstract ancestor classes. I'm not aware
>> such rule exists in QEMU, and according to your reply to my comment
>> on patch 2 there are even people using non-abstract superclasses
>> for devices.
> 
> Good catch!  You are correct, and your experiment below is
> correct.  But I thought no non-abstract superclasses where used
> on compat_props on any machine-type (then this patch wouldn't
> have any visible effect in the current tree).
> 
> However, commit 0bcba41f has this note, which I had forgot about:
> 
> Note that there's one case that won't be fixed by this hack:
> "-global spapr-pci-vfio-host-bridge.=" won't be
> able to override compat_props, because spapr-pci-host-bridge is
> not an abstract class.
> 
> We really have a
> spapr-pci-host-bridge.dynamic-reconfiguration=off entry in
> compat_props for pseries-2.3.
> 
> This means this series will also fix the ordering between
> compat_props and -global if "-global
> spapr-pci-vfio-host-bridge.dynamic-reconfiguration=..." is used
> with machine-type pseries <= 2.3.
> 
> 
>>
>> Since I don't tend to trust myself with constructing proofs
>> for such stuff in my head, I've tried out my hypothesis before
>> making my review.
>>
>> This is the patch I used to verify my hypothesis:
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 41ca666..d524cd0 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -445,6 +445,10 @@ static const TypeInfo ccw_machine_info = {
>>  .property = "max_revision",\
>>  .value= "0",\
>>  },{\
>> +.driver   = "virtio-ccw-device",\
>> +.property = "max_revision",\
>> +.value= "1",\
>> +},{\
>>  .driver   = "virtio-balloon-ccw",\
>>  .property = "max_revision",\
>>  .value= "0",\
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index e18fd26..6992697 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1666,7 +1666,7 @@ static const TypeInfo virtio_ccw_device_info = {
>>  .instance_size = sizeof(VirtioCcwDevice),
>>  .class_init = virtio_ccw_device_class_init,
>>  .class_size = sizeof(VirtIOCCWDeviceClass),
>> -.abstract = true,
>> +

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

[Eduardo Habkost]

On Thu, Jul 13, 2017 at 01:54:01PM +0200, Halil Pasic wrote:
> 
> 
> On 07/12/2017 08:29 PM, Eduardo Habkost wrote:
> > On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:
> >>
> >>
> >> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> >>> From: 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 
> >>> Message-Id: 
> >>> <148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
> >>> Signed-off-by: Eduardo Habkost 
> >>> ---
> >>>  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 f11d578..41cca9d 100644
> >>> --- a/hw/core/qdev-properties.c
> >>> +++ b/hw/core/qdev-properties.c
> >>> @@ -1148,8 +1148,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;
> >>>
> >>> @@ -1157,7 +1156,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;
> >>> @@ -1175,16 +1174,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,
> >>>
> >>
> >> Code looks good to me. Given that high profile people are happy with the
> >> patch I won't object on the behavior aspect which I don't understand fully.
> >> Thus:
> >>
> >> Reviewed-by: Halil Pasic 
> >>
> >>
> >> Now a couple of questions for keeping  my clear conscience:
> >> * What did we test? Since this patch is fixing a problem it
> >> changes behavior. Did we test if there is something that breaks?
> >>
> >> * The previous version seems to establish a (somewhat strange)
> >> precedence for the case the same device property (storage object)
> >> is set via multiple super-classes (e.g. set both by parent and
> >> parents parent). This seems to have at least been possible,
> >> and technically it still is I guess. Now instead of most general
> >> (super class) wins we have last added property wins. I assume it
> >> isn't a problem, because we don't have something obscure like that.
> >> Or am I wrong? This obviously connects to the first question.
> >> (By the way, most specialized wins would not have been that
> >> surprising given how inheritance and OO usually works. My assumption
> >> that nobody used this obscure mechanism is largely based on it's
> >> strangeness).
> > 
> > Note that we are not changing the behavior when the classes
> > themselves set different defaults.  Subclasses are still free to
> > override defaults set by superclasses inside QEMU code, and they
> > will be unaffected by this series.  What we are changing here are
> > the semantics of the -global command-line option when applied to
> > superclasses.
> 
> I was not referring to this.

Good.  :)

> 
> > 
> > The main sources of global properties we have are:
> > * MachineClass::compat_props> * -global command-line option
> 
> I was thinking about these two.

Good, this is what we're really trying to address, so let's
review that:

> 
> > * -cpu 

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

[Halil Pasic]

On 07/12/2017 08:29 PM, Eduardo Habkost wrote:
> On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:
>>
>>
>> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
>>> From: 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 
>>> Message-Id: 
>>> <148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
>>> Signed-off-by: Eduardo Habkost 
>>> ---
>>>  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 f11d578..41cca9d 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -1148,8 +1148,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;
>>>
>>> @@ -1157,7 +1156,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;
>>> @@ -1175,16 +1174,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,
>>>
>>
>> Code looks good to me. Given that high profile people are happy with the
>> patch I won't object on the behavior aspect which I don't understand fully.
>> Thus:
>>
>> Reviewed-by: Halil Pasic 
>>
>>
>> Now a couple of questions for keeping  my clear conscience:
>> * What did we test? Since this patch is fixing a problem it
>> changes behavior. Did we test if there is something that breaks?
>>
>> * The previous version seems to establish a (somewhat strange)
>> precedence for the case the same device property (storage object)
>> is set via multiple super-classes (e.g. set both by parent and
>> parents parent). This seems to have at least been possible,
>> and technically it still is I guess. Now instead of most general
>> (super class) wins we have last added property wins. I assume it
>> isn't a problem, because we don't have something obscure like that.
>> Or am I wrong? This obviously connects to the first question.
>> (By the way, most specialized wins would not have been that
>> surprising given how inheritance and OO usually works. My assumption
>> that nobody used this obscure mechanism is largely based on it's
>> strangeness).
> 
> Note that we are not changing the behavior when the classes
> themselves set different defaults.  Subclasses are still free to
> override defaults set by superclasses inside QEMU code, and they
> will be unaffected by this series.  What we are changing here are
> the semantics of the -global command-line option when applied to
> superclasses.

I was not referring to this.

> 
> The main sources of global properties we have are:
> * MachineClass::compat_props> * -global command-line option

I was thinking about these two.

> * -cpu command-line option
> 
> The behavior on the compat_props case was addressed by the hack
> in commit 0bcba41f "machine: Convert abstract typename on
> compat_props to subclass names".  This means compat_props was
> already following the order in which properties were registered.

I disagree. Commit 0bcba41f affects only *abstract* classes. So
your statement is true if a non-abstract class 

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

[Eduardo Habkost]

On Wed, Jul 12, 2017 at 07:33:15PM +0200, Halil Pasic wrote:
> 
> 
> On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> > From: 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 
> > Message-Id: 
> > <148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  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 f11d578..41cca9d 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -1148,8 +1148,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;
> > 
> > @@ -1157,7 +1156,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;
> > @@ -1175,16 +1174,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,
> > 
> 
> Code looks good to me. Given that high profile people are happy with the
> patch I won't object on the behavior aspect which I don't understand fully.
> Thus:
> 
> Reviewed-by: Halil Pasic 
> 
> 
> Now a couple of questions for keeping  my clear conscience:
> * What did we test? Since this patch is fixing a problem it
> changes behavior. Did we test if there is something that breaks?
> 
> * The previous version seems to establish a (somewhat strange)
> precedence for the case the same device property (storage object)
> is set via multiple super-classes (e.g. set both by parent and
> parents parent). This seems to have at least been possible,
> and technically it still is I guess. Now instead of most general
> (super class) wins we have last added property wins. I assume it
> isn't a problem, because we don't have something obscure like that.
> Or am I wrong? This obviously connects to the first question.
> (By the way, most specialized wins would not have been that
> surprising given how inheritance and OO usually works. My assumption
> that nobody used this obscure mechanism is largely based on it's
> strangeness).

Note that we are not changing the behavior when the classes
themselves set different defaults.  Subclasses are still free to
override defaults set by superclasses inside QEMU code, and they
will be unaffected by this series.  What we are changing here are
the semantics of the -global command-line option when applied to
superclasses.

The main sources of global properties we have are:
* MachineClass::compat_props
* -global command-line option
* -cpu command-line option

The behavior on the compat_props case was addressed by the hack
in commit 0bcba41f "machine: Convert abstract typename on
compat_props to subclass names".  This means compat_props was
already following the order in which properties were registered.
In this case there should be no behavior change, and we have
something to test: check if the original bug[1] (where -global
was unable to override compat_props) is still fixed.

However, the behavior of -global will change if the user
specifies command-line options that 

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

[Halil Pasic]

On 07/11/2017 02:43 AM, Eduardo Habkost wrote:
> From: 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 
> Message-Id: 
> <148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
> Signed-off-by: Eduardo Habkost 
> ---
>  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 f11d578..41cca9d 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1148,8 +1148,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;
> 
> @@ -1157,7 +1156,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;
> @@ -1175,16 +1174,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,
> 

Code looks good to me. Given that high profile people are happy with the
patch I won't object on the behavior aspect which I don't understand fully.
Thus:

Reviewed-by: Halil Pasic 


Now a couple of questions for keeping  my clear conscience:
* What did we test? Since this patch is fixing a problem it
changes behavior. Did we test if there is something that breaks?

* The previous version seems to establish a (somewhat strange)
precedence for the case the same device property (storage object)
is set via multiple super-classes (e.g. set both by parent and
parents parent). This seems to have at least been possible,
and technically it still is I guess. Now instead of most general
(super class) wins we have last added property wins. I assume it
isn't a problem, because we don't have something obscure like that.
Or am I wrong? This obviously connects to the first question.
(By the way, most specialized wins would not have been that
surprising given how inheritance and OO usually works. My assumption
that nobody used this obscure mechanism is largely based on it's
strangeness).

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

[Cornelia Huck]

On Mon, 10 Jul 2017 21:43:01 -0300
Eduardo Habkost  wrote:

Some description tweaks, as we had the workaround in the meanwhile:

> From: 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:

s/happens/happened/
s/since/after/

> 
> "9a4c0e220d8a hw/virtio-pci: fix virtio behaviour"
> 
> Passing -device virtio-blk-pci.disable-modern=off has no effect on 2.6

s/has/had/

> machine types because the internal virtio-pci.disable-modern=on compat
> property always prevail.

s/prevail/prevailed/

A workaround for this was included with commit 0bcba41f ("machine:
Convert abstract typename on compat_props to subclass names").

> 
> This patch fixes the issue by reversing the logic: we now go through the

s/fixes the issue/fixes the issue properly/

> 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

s/result/results/

> appear in the HW_COMPAT_* macros, followed by global properties, in they
> order appear on the command line.

s/in they order appear/in the order they appear/

> 
> Signed-off-by: Greg Kurz 
> Message-Id: 
> <148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
> Signed-off-by: Eduardo Habkost 
> ---
>  hw/core/qdev-properties.c | 15 ++-
>  1 file changed, 2 insertions(+), 13 deletions(-)

Reviewed-by: Cornelia Huck 

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

[Eduardo Habkost]

From: 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 
Message-Id: 
<148103887228.22326.47840687360929.st...@bahia.lab.toulouse-stg.fr.ibm.com>
Signed-off-by: Eduardo Habkost 
---
 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 f11d578..41cca9d 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1148,8 +1148,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;
 
@@ -1157,7 +1156,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;
@@ -1175,16 +1174,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,
-- 
2.9.4