diff mbox

[RFC,for-2.8] machine: Convert abstract typename on compat_props to subclass names

Message ID 20161206193159.GE4027@thinpad.lan.raisama.net (mailing list archive)
State New, archived
Headers show

Commit Message

Eduardo Habkost Dec. 6, 2016, 7:31 p.m. UTC
On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:
> On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:
> > 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 should ideally be fixed in the qdev properties core code, but it is
> > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > the compat properties for every virtio-*-pci subtypes instead of the base
> > virtio-pci type.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> So, it looks like the bug is present in many other cases...
> 
> I have hacked QEMU to print a warning in case the driver name in
> compat_props refer to an abstract class or a class that have any
> subclasses. The results are below:
> 
>  apic-common.legacy-instance-id set for abstract class
>  apic-common.vapic set for abstract class
>  i386-cpu.arat set for abstract class
>  i386-cpu.check set for abstract class
>  i386-cpu.cpuid-0xb set for abstract class
>  i386-cpu.fill-mtrr-mask set for abstract class
>  i386-cpu.full-cpuid-auto-level set for abstract class
>  i386-cpu.l3-cache set for abstract class
>  i386-cpu.pmu set for abstract class
>  pci-device.command_serr_enable set for abstract class
>  pci-device.rombar set for abstract class
>  pci-device.x-pcie-lnksta-dllla set for abstract class
>  powerpc64-cpu.pre-2.8-migration set for abstract class
>  s390-skeys.migration-enabled set for abstract class
>  spapr-pci-host-bridge.ddw set for superclass
>  spapr-pci-host-bridge.dynamic-reconfiguration set for superclass
>  spapr-pci-host-bridge.mem64_win_size set for superclass
>  spapr-pci-host-bridge.mem_win_size set for superclass
>  spapr-pci-host-bridge.pre-2.8-migration set for superclass
>  usb-device.full-path set for abstract class
>  usb-device.msos-desc set for abstract class
>  virtio-pci.disable-legacy set for abstract class
>  virtio-pci.disable-modern set for abstract class
>  virtio-pci.migrate-extra set for abstract class
>  virtio-pci.page-per-vq set for abstract class
>  virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class
>  virtio-pci.x-disable-pcie set for abstract class
>  x86_64-cpu.arat set for abstract class
>  x86_64-cpu.check set for abstract class
>  x86_64-cpu.cpuid-0xb set for abstract class
>  x86_64-cpu.fill-mtrr-mask set for abstract class
>  x86_64-cpu.full-cpuid-auto-level set for abstract class
>  x86_64-cpu.l3-cache set for abstract class
>  x86_64-cpu.pmu set for abstract class
> 
> I believe the cases where we are likely to cause real-world bugs
> are virtio-pci and the *-cpu classes (because -cpu is translated
> to -global).
> 
> I'm not sure what should be the right fix in 2.8. I am
> considering a temporary hack to translate abstract class names in
> compat_props to global properties for all subclasses, in case
> they refer to an abstract class. This way we fix the bug where
> -global doesn't override compat_props properly, but keep the
> rules for -global untouched.

What about this?

(untested)

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Comments

Greg Kurz Dec. 7, 2016, 1:39 p.m. UTC | #1
On Tue, 6 Dec 2016 17:31:59 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:
> > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:  
> > > 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 should ideally be fixed in the qdev properties core code, but it is
> > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > > the compat properties for every virtio-*-pci subtypes instead of the base
> > > virtio-pci type.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > So, it looks like the bug is present in many other cases...
> > 
> > I have hacked QEMU to print a warning in case the driver name in
> > compat_props refer to an abstract class or a class that have any
> > subclasses. The results are below:
> > 
> >  apic-common.legacy-instance-id set for abstract class
> >  apic-common.vapic set for abstract class
> >  i386-cpu.arat set for abstract class
> >  i386-cpu.check set for abstract class
> >  i386-cpu.cpuid-0xb set for abstract class
> >  i386-cpu.fill-mtrr-mask set for abstract class
> >  i386-cpu.full-cpuid-auto-level set for abstract class
> >  i386-cpu.l3-cache set for abstract class
> >  i386-cpu.pmu set for abstract class
> >  pci-device.command_serr_enable set for abstract class
> >  pci-device.rombar set for abstract class
> >  pci-device.x-pcie-lnksta-dllla set for abstract class
> >  powerpc64-cpu.pre-2.8-migration set for abstract class
> >  s390-skeys.migration-enabled set for abstract class
> >  spapr-pci-host-bridge.ddw set for superclass
> >  spapr-pci-host-bridge.dynamic-reconfiguration set for superclass
> >  spapr-pci-host-bridge.mem64_win_size set for superclass
> >  spapr-pci-host-bridge.mem_win_size set for superclass
> >  spapr-pci-host-bridge.pre-2.8-migration set for superclass
> >  usb-device.full-path set for abstract class
> >  usb-device.msos-desc set for abstract class
> >  virtio-pci.disable-legacy set for abstract class
> >  virtio-pci.disable-modern set for abstract class
> >  virtio-pci.migrate-extra set for abstract class
> >  virtio-pci.page-per-vq set for abstract class
> >  virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class
> >  virtio-pci.x-disable-pcie set for abstract class
> >  x86_64-cpu.arat set for abstract class
> >  x86_64-cpu.check set for abstract class
> >  x86_64-cpu.cpuid-0xb set for abstract class
> >  x86_64-cpu.fill-mtrr-mask set for abstract class
> >  x86_64-cpu.full-cpuid-auto-level set for abstract class
> >  x86_64-cpu.l3-cache set for abstract class
> >  x86_64-cpu.pmu set for abstract class
> > 
> > I believe the cases where we are likely to cause real-world bugs
> > are virtio-pci and the *-cpu classes (because -cpu is translated
> > to -global).
> > 
> > I'm not sure what should be the right fix in 2.8. I am
> > considering a temporary hack to translate abstract class names in
> > compat_props to global properties for all subclasses, in case
> > they refer to an abstract class. This way we fix the bug where
> > -global doesn't override compat_props properly, but keep the
> > rules for -global untouched.  
> 
> What about this?
> 

Indeed that would have been a clever workaround but it is too late anyway. :)

BTW, that makes me think about Halil's comment in another mail, even if it
is not strictly related to the comapt issue (but rather because I mentioned
it in the changelog of my first patch):

"So I can't even tell if -global virtio-pci.disable-modern=off is even legit
on the command line."

And indeed, -device doesn't accept abstract classes... shouldn't -global
prohibit setting properties for them ?

> (untested)
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index b0fd91f..49f9007 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -554,6 +554,25 @@ static void machine_class_finalize(ObjectClass *klass, void *data)
>      g_free(mc->name);
>  }
>  
> +static void register_compat_prop(const char *driver,
> +                                 const char *property,
> +                                 const char *value)
> +{
> +    GlobalProperty *p = g_new0(GlobalProperty, 1);
> +    /* Machine compat_props must never cause errors: */
> +    p->errp = &error_abort;
> +    p->driver = driver;
> +    p->property = property;
> +    p->value = value;
> +    qdev_prop_register_global(p);
> +}
> +
> +static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
> +{
> +    GlobalProperty *p = opaque;
> +    register_compat_prop(object_class_get_name(oc), p->property, p->value);
> +}
> +
>  void machine_register_compat_props(MachineState *machine)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine)
>      }
>  
>      for (i = 0; i < mc->compat_props->len; i++) {
> +        ObjectClass *oc;
>          p = g_array_index(mc->compat_props, GlobalProperty *, i);
> -        /* Machine compat_props must never cause errors: */
> -        p->errp = &error_abort;
> -        qdev_prop_register_global(p);
> +        oc = object_class_by_name(p->driver);
> +        if (oc && object_class_is_abstract(oc)) {
> +            /* temporary hack to make sure we will never override
> +             * globals set explicitly on -global: if an abstract class
> +             * is on compat_props, register globals for each of their
> +             * subclasses instead.
> +             */
> +            object_class_foreach(machine_register_compat_for_subclass,
> +                                 p->driver, false, p);
> +        } else {
> +            register_compat_prop(p->driver, p->property, p->value);
> +        }
>      }
>  }
>
Eduardo Habkost Dec. 7, 2016, 1:59 p.m. UTC | #2
On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote:
> On Tue, 6 Dec 2016 17:31:59 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:
> > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:  
> > > > 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 should ideally be fixed in the qdev properties core code, but it is
> > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > > > the compat properties for every virtio-*-pci subtypes instead of the base
> > > > virtio-pci type.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > > 
> > > So, it looks like the bug is present in many other cases...
> > > 
> > > I have hacked QEMU to print a warning in case the driver name in
> > > compat_props refer to an abstract class or a class that have any
> > > subclasses. The results are below:
> > > 
> > >  apic-common.legacy-instance-id set for abstract class
> > >  apic-common.vapic set for abstract class
> > >  i386-cpu.arat set for abstract class
> > >  i386-cpu.check set for abstract class
> > >  i386-cpu.cpuid-0xb set for abstract class
> > >  i386-cpu.fill-mtrr-mask set for abstract class
> > >  i386-cpu.full-cpuid-auto-level set for abstract class
> > >  i386-cpu.l3-cache set for abstract class
> > >  i386-cpu.pmu set for abstract class
> > >  pci-device.command_serr_enable set for abstract class
> > >  pci-device.rombar set for abstract class
> > >  pci-device.x-pcie-lnksta-dllla set for abstract class
> > >  powerpc64-cpu.pre-2.8-migration set for abstract class
> > >  s390-skeys.migration-enabled set for abstract class
> > >  spapr-pci-host-bridge.ddw set for superclass
> > >  spapr-pci-host-bridge.dynamic-reconfiguration set for superclass
> > >  spapr-pci-host-bridge.mem64_win_size set for superclass
> > >  spapr-pci-host-bridge.mem_win_size set for superclass
> > >  spapr-pci-host-bridge.pre-2.8-migration set for superclass
> > >  usb-device.full-path set for abstract class
> > >  usb-device.msos-desc set for abstract class
> > >  virtio-pci.disable-legacy set for abstract class
> > >  virtio-pci.disable-modern set for abstract class
> > >  virtio-pci.migrate-extra set for abstract class
> > >  virtio-pci.page-per-vq set for abstract class
> > >  virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class
> > >  virtio-pci.x-disable-pcie set for abstract class
> > >  x86_64-cpu.arat set for abstract class
> > >  x86_64-cpu.check set for abstract class
> > >  x86_64-cpu.cpuid-0xb set for abstract class
> > >  x86_64-cpu.fill-mtrr-mask set for abstract class
> > >  x86_64-cpu.full-cpuid-auto-level set for abstract class
> > >  x86_64-cpu.l3-cache set for abstract class
> > >  x86_64-cpu.pmu set for abstract class
> > > 
> > > I believe the cases where we are likely to cause real-world bugs
> > > are virtio-pci and the *-cpu classes (because -cpu is translated
> > > to -global).
> > > 
> > > I'm not sure what should be the right fix in 2.8. I am
> > > considering a temporary hack to translate abstract class names in
> > > compat_props to global properties for all subclasses, in case
> > > they refer to an abstract class. This way we fix the bug where
> > > -global doesn't override compat_props properly, but keep the
> > > rules for -global untouched.  
> > 
> > What about this?
> > 
> 
> Indeed that would have been a clever workaround but it is too late anyway. :)
> 
> BTW, that makes me think about Halil's comment in another mail, even if it
> is not strictly related to the comapt issue (but rather because I mentioned
> it in the changelog of my first patch):
> 
> "So I can't even tell if -global virtio-pci.disable-modern=off is even legit
> on the command line."
> 
> And indeed, -device doesn't accept abstract classes... shouldn't -global
> prohibit setting properties for them ?

I'm not sure. Setting disable-modern for all virtio-pci devices
with a single -global option sounds very useful, for example.

Also, if we start rejecting it we risk breaking existing
configurations. We would need to deprecate it first (making it
print a warning) and reject it a few releases later.
Greg Kurz Dec. 7, 2016, 2:58 p.m. UTC | #3
On Wed, 7 Dec 2016 11:59:17 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote:
> > On Tue, 6 Dec 2016 17:31:59 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:  
> > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:    
> > > > > 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 should ideally be fixed in the qdev properties core code, but it is
> > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > > > > the compat properties for every virtio-*-pci subtypes instead of the base
> > > > > virtio-pci type.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > So, it looks like the bug is present in many other cases...
> > > > 
> > > > I have hacked QEMU to print a warning in case the driver name in
> > > > compat_props refer to an abstract class or a class that have any
> > > > subclasses. The results are below:
> > > > 
> > > >  apic-common.legacy-instance-id set for abstract class
> > > >  apic-common.vapic set for abstract class
> > > >  i386-cpu.arat set for abstract class
> > > >  i386-cpu.check set for abstract class
> > > >  i386-cpu.cpuid-0xb set for abstract class
> > > >  i386-cpu.fill-mtrr-mask set for abstract class
> > > >  i386-cpu.full-cpuid-auto-level set for abstract class
> > > >  i386-cpu.l3-cache set for abstract class
> > > >  i386-cpu.pmu set for abstract class
> > > >  pci-device.command_serr_enable set for abstract class
> > > >  pci-device.rombar set for abstract class
> > > >  pci-device.x-pcie-lnksta-dllla set for abstract class
> > > >  powerpc64-cpu.pre-2.8-migration set for abstract class
> > > >  s390-skeys.migration-enabled set for abstract class
> > > >  spapr-pci-host-bridge.ddw set for superclass
> > > >  spapr-pci-host-bridge.dynamic-reconfiguration set for superclass
> > > >  spapr-pci-host-bridge.mem64_win_size set for superclass
> > > >  spapr-pci-host-bridge.mem_win_size set for superclass
> > > >  spapr-pci-host-bridge.pre-2.8-migration set for superclass
> > > >  usb-device.full-path set for abstract class
> > > >  usb-device.msos-desc set for abstract class
> > > >  virtio-pci.disable-legacy set for abstract class
> > > >  virtio-pci.disable-modern set for abstract class
> > > >  virtio-pci.migrate-extra set for abstract class
> > > >  virtio-pci.page-per-vq set for abstract class
> > > >  virtio-pci.virtio-pci-bus-master-bug-migration set for abstract class
> > > >  virtio-pci.x-disable-pcie set for abstract class
> > > >  x86_64-cpu.arat set for abstract class
> > > >  x86_64-cpu.check set for abstract class
> > > >  x86_64-cpu.cpuid-0xb set for abstract class
> > > >  x86_64-cpu.fill-mtrr-mask set for abstract class
> > > >  x86_64-cpu.full-cpuid-auto-level set for abstract class
> > > >  x86_64-cpu.l3-cache set for abstract class
> > > >  x86_64-cpu.pmu set for abstract class
> > > > 
> > > > I believe the cases where we are likely to cause real-world bugs
> > > > are virtio-pci and the *-cpu classes (because -cpu is translated
> > > > to -global).
> > > > 
> > > > I'm not sure what should be the right fix in 2.8. I am
> > > > considering a temporary hack to translate abstract class names in
> > > > compat_props to global properties for all subclasses, in case
> > > > they refer to an abstract class. This way we fix the bug where
> > > > -global doesn't override compat_props properly, but keep the
> > > > rules for -global untouched.    
> > > 
> > > What about this?
> > >   
> > 
> > Indeed that would have been a clever workaround but it is too late anyway. :)
> > 
> > BTW, that makes me think about Halil's comment in another mail, even if it
> > is not strictly related to the comapt issue (but rather because I mentioned
> > it in the changelog of my first patch):
> > 
> > "So I can't even tell if -global virtio-pci.disable-modern=off is even legit
> > on the command line."
> > 
> > And indeed, -device doesn't accept abstract classes... shouldn't -global
> > prohibit setting properties for them ?  
> 
> I'm not sure. Setting disable-modern for all virtio-pci devices
> with a single -global option sounds very useful, for example.
> 

Indeed but, unless I've missed something *again*, you cannot know about
it without reading the code... maybe this calls for a '-global help' option
to list them all ?

> Also, if we start rejecting it we risk breaking existing
> configurations. We would need to deprecate it first (making it
> print a warning) and reject it a few releases later.
> 

This would be an undocumented configuration but I get your point, so
it's probably better to keep it... especially if it's useful :)

Cheers.

--
Greg
Eduardo Habkost Dec. 9, 2016, 8:06 p.m. UTC | #4
On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote:
> On Tue, 6 Dec 2016 17:31:59 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:
> > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:  
> > > > 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 should ideally be fixed in the qdev properties core code, but it is
> > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > > > the compat properties for every virtio-*-pci subtypes instead of the base
> > > > virtio-pci type.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > > 
> > > So, it looks like the bug is present in many other cases...
> > > 
> > > I have hacked QEMU to print a warning in case the driver name in
> > > compat_props refer to an abstract class or a class that have any
> > > subclasses. The results are below:
> > > 
[...]
> > > 
> > > I believe the cases where we are likely to cause real-world bugs
> > > are virtio-pci and the *-cpu classes (because -cpu is translated
> > > to -global).
> > > 
> > > I'm not sure what should be the right fix in 2.8. I am
> > > considering a temporary hack to translate abstract class names in
> > > compat_props to global properties for all subclasses, in case
> > > they refer to an abstract class. This way we fix the bug where
> > > -global doesn't override compat_props properly, but keep the
> > > rules for -global untouched.  
> > 
> > What about this?
> > 
> 
> Indeed that would have been a clever workaround but it is too late anyway. :)

Now my question is: what should we do in 2.8.1?

1) Change the rules and apply "[PATCH] qdev: fix the order compat
   and global properties are applied"
2) Fix only virtio-pci (apply a new version of "[PATCH] virtio:
   fix HW_COMPAT_2_6 macro for virtio-*-pci drivers")
3) Apply this hack so we fix all compat_props cases without
   changing ordering rules. (And then change the ordering rules
   in 2.9)
Cornelia Huck Dec. 12, 2016, 11:26 a.m. UTC | #5
On Fri, 9 Dec 2016 18:06:50 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Now my question is: what should we do in 2.8.1?
> 
> 1) Change the rules and apply "[PATCH] qdev: fix the order compat
>    and global properties are applied"

I don't think changing the rules is something that should be done for a
stable release.

> 2) Fix only virtio-pci (apply a new version of "[PATCH] virtio:
>    fix HW_COMPAT_2_6 macro for virtio-*-pci drivers")

This has the benefit of fixing exactly one problem in an easily
reviewable patch...

> 3) Apply this hack so we fix all compat_props cases without
>    changing ordering rules. (And then change the ordering rules
>    in 2.9)

...while this has the benefit of fixing the complete class of problems.

I'd say 2 or 3, but I'd favour 3. 2.8.1 will not be released next week,
so we have enough time to verify that we don't introduce any new
breakage.
Greg Kurz Dec. 12, 2016, 11:39 a.m. UTC | #6
On Fri, 9 Dec 2016 18:06:50 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Dec 07, 2016 at 02:39:25PM +0100, Greg Kurz wrote:
> > On Tue, 6 Dec 2016 17:31:59 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > On Tue, Dec 06, 2016 at 05:19:52PM -0200, Eduardo Habkost wrote:  
> > > > On Tue, Dec 06, 2016 at 06:50:47PM +0100, Greg Kurz wrote:    
> > > > > 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 should ideally be fixed in the qdev properties core code, but it is
> > > > > too late in the QEMU 2.8 schedule. So this patch fixes the issue by setting
> > > > > the compat properties for every virtio-*-pci subtypes instead of the base
> > > > > virtio-pci type.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > So, it looks like the bug is present in many other cases...
> > > > 
> > > > I have hacked QEMU to print a warning in case the driver name in
> > > > compat_props refer to an abstract class or a class that have any
> > > > subclasses. The results are below:
> > > >   
> [...]
> > > > 
> > > > I believe the cases where we are likely to cause real-world bugs
> > > > are virtio-pci and the *-cpu classes (because -cpu is translated
> > > > to -global).
> > > > 
> > > > I'm not sure what should be the right fix in 2.8. I am
> > > > considering a temporary hack to translate abstract class names in
> > > > compat_props to global properties for all subclasses, in case
> > > > they refer to an abstract class. This way we fix the bug where
> > > > -global doesn't override compat_props properly, but keep the
> > > > rules for -global untouched.    
> > > 
> > > What about this?
> > >   
> > 
> > Indeed that would have been a clever workaround but it is too late anyway. :)  
> 
> Now my question is: what should we do in 2.8.1?
> 
> 1) Change the rules and apply "[PATCH] qdev: fix the order compat
>    and global properties are applied"

Would it be more acceptable to change the rules in 2.8.1 than it is
for 2.8 ?

> 2) Fix only virtio-pci (apply a new version of "[PATCH] virtio:
>    fix HW_COMPAT_2_6 macro for virtio-*-pci drivers")

The patch is huge, but we're sure that it doesn't break anything else.

> 3) Apply this hack so we fix all compat_props cases without
>    changing ordering rules. (And then change the ordering rules
>    in 2.9)
> 

FWIW, I think this patch is ok, and not that hacky: it simply implements the
idea that 'compat properties of abstract classes are actually handled at the
non-abstract sub-classes level'. Of course, it would be great to document this
behavior somewhere. :)

Cheers.

--
Greg
Cornelia Huck Dec. 12, 2016, 12:25 p.m. UTC | #7
On Tue, 6 Dec 2016 17:31:59 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> What about this?
> 
> (untested)

Have not tested it either, but I think it looks good. Some nits below.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 

> @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine)
>      }
> 
>      for (i = 0; i < mc->compat_props->len; i++) {
> +        ObjectClass *oc;

Move the declaration out of the loop?

>          p = g_array_index(mc->compat_props, GlobalProperty *, i);
> -        /* Machine compat_props must never cause errors: */
> -        p->errp = &error_abort;
> -        qdev_prop_register_global(p);
> +        oc = object_class_by_name(p->driver);
> +        if (oc && object_class_is_abstract(oc)) {
> +            /* temporary hack to make sure we will never override
> +             * globals set explicitly on -global: if an abstract class
> +             * is on compat_props, register globals for each of their
> +             * subclasses instead.
> +             */

I think this should not just be a 'temporary hack'... rather document
this behaviour for abstract classes?

> +            object_class_foreach(machine_register_compat_for_subclass,
> +                                 p->driver, false, p);
> +        } else {
> +            register_compat_prop(p->driver, p->property, p->value);
> +        }
>      }
>  }
>
Halil Pasic Dec. 12, 2016, 5:13 p.m. UTC | #8
On 12/12/2016 01:25 PM, Cornelia Huck wrote:
>> +        if (oc && object_class_is_abstract(oc)) {
>> +            /* temporary hack to make sure we will never override
                                               ~~~~~~~~~~~~~~~~~~~~~~

I would say this is not enough to provide that guarantee. Inheriting
from non-abstract, or something like I described below, would still
get us in trouble. Or am I wrong? 

How about s/we will never/we do not/?

>> +             * globals set explicitly on -global: if an abstract class
>> +             * is on compat_props, register globals for each of their
>> +             * subclasses instead.

I'm not sure if this is clear enough regarding multilevel inheritance.
I would prefer "subtype" or "class derived from it".

>> +             */
> I think this should not just be a 'temporary hack'... rather document
> this behaviour for abstract classes?
> 

Connie, I have to disagree with you on this. I'm fine with this as a
temporary hack provided it remedies the acute problem, but I would this
becoming state of art.

The reason for this how abstract class is usually understood in OOP:
it's like a normal class except you can not instantiate it.  Adding an
extra property rule, and especially such a convoluted one, could result
in a conflict between intuition and reality.  And it's not like we have
no plan how to do this cleanly.

Why do I say convoluted:
* Inheriting form abstract and from non-abstract behaves differently.
* Theoretically we have something like non-abstract (C_3) inherits for
abstract (AC_2) inherits from non-abstract (C_1)  inherits from abstract
(AC_0), and we set the property P both via AC_0 and AC_2 for an instance
of C1 we would/could end up having the same problem as we have now (via
AC_0 taking precedence over AC_2) -- at least I think so. 

I did not test it either, but it looks sane to me. Regardless of the
comments on the comment (if you are going to consider them or not) you
can take my r-b.

Cheers,
Halil
Eduardo Habkost Dec. 12, 2016, 5:36 p.m. UTC | #9
On Mon, Dec 12, 2016 at 01:25:09PM +0100, Cornelia Huck wrote:
> On Tue, 6 Dec 2016 17:31:59 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > What about this?
> > 
> > (untested)
> 
> Have not tested it either, but I think it looks good. Some nits below.

Thanks!

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/core/machine.c | 35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> 
> > @@ -565,10 +584,20 @@ void machine_register_compat_props(MachineState *machine)
> >      }
> > 
> >      for (i = 0; i < mc->compat_props->len; i++) {
> > +        ObjectClass *oc;
> 
> Move the declaration out of the loop?

I will do it on v2.

> 
> >          p = g_array_index(mc->compat_props, GlobalProperty *, i);
> > -        /* Machine compat_props must never cause errors: */
> > -        p->errp = &error_abort;
> > -        qdev_prop_register_global(p);
> > +        oc = object_class_by_name(p->driver);
> > +        if (oc && object_class_is_abstract(oc)) {
> > +            /* temporary hack to make sure we will never override
> > +             * globals set explicitly on -global: if an abstract class
> > +             * is on compat_props, register globals for each of their
> > +             * subclasses instead.
> > +             */
> 
> I think this should not just be a 'temporary hack'... rather document
> this behaviour for abstract classes?

I'm not sure. I believe the translation from abstract superclass
to subclasses just emulates the behavior implemented by Greg Kurz
at "qdev: fix the order compat and global properties are applied"
in a more complicated way (and only for compat_props, not for
-global).

This hack specifically would not be enough because it affects
only compat_props, and still behaves in a weird way when
non-abstract classes have subclasses.

> 
> > +            object_class_foreach(machine_register_compat_for_subclass,
> > +                                 p->driver, false, p);
> > +        } else {
> > +            register_compat_prop(p->driver, p->property, p->value);
> > +        }
> >      }
> >  }
> > 
>
Eduardo Habkost Dec. 12, 2016, 5:47 p.m. UTC | #10
On Mon, Dec 12, 2016 at 06:13:50PM +0100, Halil Pasic wrote:
> 
> 
> On 12/12/2016 01:25 PM, Cornelia Huck wrote:
> >> +        if (oc && object_class_is_abstract(oc)) {
> >> +            /* temporary hack to make sure we will never override
>                                                ~~~~~~~~~~~~~~~~~~~~~~
> 
> I would say this is not enough to provide that guarantee. Inheriting
> from non-abstract, or something like I described below, would still
> get us in trouble. Or am I wrong? 

Yes, inheriting from non-abstract would still be broken, so this
hack is not enough for some cases. In the current tree, it won't
fix the problem for the spapr-pci-host-bridge properties.

> 
> How about s/we will never/we do not/?

I will change it.

> 
> >> +             * globals set explicitly on -global: if an abstract class
> >> +             * is on compat_props, register globals for each of their
> >> +             * subclasses instead.
> 
> I'm not sure if this is clear enough regarding multilevel inheritance.
> I would prefer "subtype" or "class derived from it".

I can rewrite it as "for all of their subtypes".

> 
> >> +             */
> > I think this should not just be a 'temporary hack'... rather document
> > this behaviour for abstract classes?
> > 
> 
> Connie, I have to disagree with you on this. I'm fine with this as a
> temporary hack provided it remedies the acute problem, but I would this
> becoming state of art.
> 
> The reason for this how abstract class is usually understood in OOP:
> it's like a normal class except you can not instantiate it.  Adding an
> extra property rule, and especially such a convoluted one, could result
> in a conflict between intuition and reality.  And it's not like we have
> no plan how to do this cleanly.
> 
> Why do I say convoluted:
> * Inheriting form abstract and from non-abstract behaves differently.
> * Theoretically we have something like non-abstract (C_3) inherits for
> abstract (AC_2) inherits from non-abstract (C_1)  inherits from abstract
> (AC_0), and we set the property P both via AC_0 and AC_2 for an instance
> of C1 we would/could end up having the same problem as we have now (via
> AC_0 taking precedence over AC_2) -- at least I think so. 

Well, we could change the code to:
1) Not check object_class_is_abstract(), and simply register the
   globals for all subtypes;
2) Call qdev_prop_set_globals_for_type() only for
   object_class_get_name(), instead of doing it for all
   parent classes in qdev_prop_set_globals().

But this sounds like a more complicated way of implementing
exactly the same behavior implemented by Greg Kurz in "qdev: fix
the order compat and global properties are applied".

> 
> I did not test it either, but it looks sane to me. Regardless of the
> comments on the comment (if you are going to consider them or not) you
> can take my r-b.

Thanks!
Halil Pasic Dec. 12, 2016, 6 p.m. UTC | #11
On 12/12/2016 06:47 PM, Eduardo Habkost wrote:
>>>> +             */
>>> I think this should not just be a 'temporary hack'... rather document
>>> this behaviour for abstract classes?
>>>
>> Connie, I have to disagree with you on this. I'm fine with this as a
>> temporary hack provided it remedies the acute problem, but I would this
>> becoming state of art.
>>
>> The reason for this how abstract class is usually understood in OOP:
>> it's like a normal class except you can not instantiate it.  Adding an
>> extra property rule, and especially such a convoluted one, could result
>> in a conflict between intuition and reality.  And it's not like we have
>> no plan how to do this cleanly.
>>
>> Why do I say convoluted:
>> * Inheriting form abstract and from non-abstract behaves differently.
>> * Theoretically we have something like non-abstract (C_3) inherits for
>> abstract (AC_2) inherits from non-abstract (C_1)  inherits from abstract
>> (AC_0), and we set the property P both via AC_0 and AC_2 for an instance
>> of C1 we would/could end up having the same problem as we have now (via
>> AC_0 taking precedence over AC_2) -- at least I think so. 
> Well, we could change the code to:
> 1) Not check object_class_is_abstract(), and simply register the
>    globals for all subtypes;
> 2) Call qdev_prop_set_globals_for_type() only for
>    object_class_get_name(), instead of doing it for all
>    parent classes in qdev_prop_set_globals().
> 
> But this sounds like a more complicated way of implementing
> exactly the same behavior implemented by Greg Kurz in "qdev: fix
> the order compat and global properties are applied".
> 

Yeah. This is what I meant with "And it's not like we have
no plan how to do this cleanly.", was trying to convince Connie
that this called a hack for a reason. I think we are on the same
page. I think this is a really good way to solve the acute problem
with a small LOC number.

Halil
diff mbox

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index b0fd91f..49f9007 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -554,6 +554,25 @@  static void machine_class_finalize(ObjectClass *klass, void *data)
     g_free(mc->name);
 }
 
+static void register_compat_prop(const char *driver,
+                                 const char *property,
+                                 const char *value)
+{
+    GlobalProperty *p = g_new0(GlobalProperty, 1);
+    /* Machine compat_props must never cause errors: */
+    p->errp = &error_abort;
+    p->driver = driver;
+    p->property = property;
+    p->value = value;
+    qdev_prop_register_global(p);
+}
+
+static void machine_register_compat_for_subclass(ObjectClass *oc, void *opaque)
+{
+    GlobalProperty *p = opaque;
+    register_compat_prop(object_class_get_name(oc), p->property, p->value);
+}
+
 void machine_register_compat_props(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -565,10 +584,20 @@  void machine_register_compat_props(MachineState *machine)
     }
 
     for (i = 0; i < mc->compat_props->len; i++) {
+        ObjectClass *oc;
         p = g_array_index(mc->compat_props, GlobalProperty *, i);
-        /* Machine compat_props must never cause errors: */
-        p->errp = &error_abort;
-        qdev_prop_register_global(p);
+        oc = object_class_by_name(p->driver);
+        if (oc && object_class_is_abstract(oc)) {
+            /* temporary hack to make sure we will never override
+             * globals set explicitly on -global: if an abstract class
+             * is on compat_props, register globals for each of their
+             * subclasses instead.
+             */
+            object_class_foreach(machine_register_compat_for_subclass,
+                                 p->driver, false, p);
+        } else {
+            register_compat_prop(p->driver, p->property, p->value);
+        }
     }
 }