Message ID | 1481575745-26120-1-git-send-email-ehabkost@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, 12 Dec 2016 18:49:05 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > Original problem description by Greg Kurz: > > > 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. > > The same bug also affects other abstract type names mentioned on > compat_props by machine-types: apic-common, i386-cpu, pci-device, > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > virtio-pci, x86_64-cpu. > > The right fix for this problem is to make sure compat_props and > -global options are always applied in the order they are > registered, instead of reordering them based on the type > hierarchy. But changing the ordering rules of -global is risky > and might break existing configurations, so we shouldn't do that > on a stable branch. > > This is a temporary hack that will work around the bug when > registering compat_props properties: if we find an abstract class > on compat_props, register properties for all its non-abstract > subtypes instead. This will make sure -global won't be overridden > by compat_props, while keeping the existing ordering rules on > -global options. > > Note that there's one case that won't be fixed by this hack: > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > able to override compat_props, because spapr-pci-host-bridge is > not an abstract class. Hm... static void spapr_phb_vfio_instance_init(Object *obj) { if (!qtest_enabled()) { error_report("spapr-pci-vfio-host-bridge is deprecated"); } } I guess we can live with that :) > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 (RFC) -> v2: > * New commit message > * Clarified comments > Suggested-by: Halil Pasic <pasic@linux.vnet.ibm.com> > * Moved variable declaration outside of loop > Suggested-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/core/machine.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On 12/12/2016 09:49 PM, Eduardo Habkost wrote: > Original problem description by Greg Kurz: > >> 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. > The same bug also affects other abstract type names mentioned on > compat_props by machine-types: apic-common, i386-cpu, pci-device, > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > virtio-pci, x86_64-cpu. > > The right fix for this problem is to make sure compat_props and > -global options are always applied in the order they are > registered, instead of reordering them based on the type > hierarchy. But changing the ordering rules of -global is risky > and might break existing configurations, so we shouldn't do that > on a stable branch. > > This is a temporary hack that will work around the bug when > registering compat_props properties: if we find an abstract class > on compat_props, register properties for all its non-abstract > subtypes instead. This will make sure -global won't be overridden > by compat_props, while keeping the existing ordering rules on > -global options. > > Note that there's one case that won't be fixed by this hack: > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > able to override compat_props, because spapr-pci-host-bridge is > not an abstract class. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
On Mon, 12 Dec 2016 18:49:05 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > Original problem description by Greg Kurz: > > > 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. > > The same bug also affects other abstract type names mentioned on > compat_props by machine-types: apic-common, i386-cpu, pci-device, > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > virtio-pci, x86_64-cpu. > > The right fix for this problem is to make sure compat_props and > -global options are always applied in the order they are > registered, instead of reordering them based on the type > hierarchy. But changing the ordering rules of -global is risky > and might break existing configurations, so we shouldn't do that > on a stable branch. > > This is a temporary hack that will work around the bug when > registering compat_props properties: if we find an abstract class > on compat_props, register properties for all its non-abstract > subtypes instead. This will make sure -global won't be overridden > by compat_props, while keeping the existing ordering rules on > -global options. > > Note that there's one case that won't be fixed by this hack: > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > able to override compat_props, because spapr-pci-host-bridge is > not an abstract class. > As mentioned by Connie, spapr-pci-vfio-host-bridge is deprecated: commit 72700d7e733948fa7fbb735ccdf2209931c88476 Author: David Gibson <david@gibson.dropbear.id.au> Date: Mon Feb 29 17:19:50 2016 +1100 spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> Tested-by: Greg Kurz <groug@kaod.org> # with virtio-blk-pci and pseries-2.6 > Changes v1 (RFC) -> v2: > * New commit message > * Clarified comments > Suggested-by: Halil Pasic <pasic@linux.vnet.ibm.com> > * Moved variable declaration outside of loop > Suggested-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/core/machine.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index b0fd91f..213e8e0 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -554,11 +554,31 @@ 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); > int i; > GlobalProperty *p; > + ObjectClass *oc; > > if (!mc->compat_props) { > return; > @@ -566,9 +586,22 @@ void machine_register_compat_props(MachineState *machine) > > for (i = 0; i < mc->compat_props->len; i++) { > 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 do not override > + * globals set explicitly on -global: if an abstract class > + * is on compat_props, register globals for all its > + * non-abstract subtypes instead. > + * > + * This doesn't solve the problem for cases where > + * a non-abstract typename mentioned on compat_props > + * has subclasses, like spapr-pci-host-bridge. > + */ > + object_class_foreach(machine_register_compat_for_subclass, > + p->driver, false, p); > + } else { > + register_compat_prop(p->driver, p->property, p->value); > + } > } > } >
On Mon, 12 Dec 2016 18:49:05 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > Original problem description by Greg Kurz: > > > 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. > > The same bug also affects other abstract type names mentioned on > compat_props by machine-types: apic-common, i386-cpu, pci-device, > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > virtio-pci, x86_64-cpu. > > The right fix for this problem is to make sure compat_props and > -global options are always applied in the order they are > registered, instead of reordering them based on the type > hierarchy. But changing the ordering rules of -global is risky > and might break existing configurations, so we shouldn't do that > on a stable branch. > > This is a temporary hack that will work around the bug when > registering compat_props properties: if we find an abstract class > on compat_props, register properties for all its non-abstract > subtypes instead. This will make sure -global won't be overridden > by compat_props, while keeping the existing ordering rules on > -global options. > > Note that there's one case that won't be fixed by this hack: > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > able to override compat_props, because spapr-pci-host-bridge is > not an abstract class. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- This patch had 3 r-b. Is there any good reason to hold it up to 2.8.1 ? > Changes v1 (RFC) -> v2: > * New commit message > * Clarified comments > Suggested-by: Halil Pasic <pasic@linux.vnet.ibm.com> > * Moved variable declaration outside of loop > Suggested-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/core/machine.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index b0fd91f..213e8e0 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -554,11 +554,31 @@ 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); > int i; > GlobalProperty *p; > + ObjectClass *oc; > > if (!mc->compat_props) { > return; > @@ -566,9 +586,22 @@ void machine_register_compat_props(MachineState *machine) > > for (i = 0; i < mc->compat_props->len; i++) { > 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 do not override > + * globals set explicitly on -global: if an abstract class > + * is on compat_props, register globals for all its > + * non-abstract subtypes instead. > + * > + * This doesn't solve the problem for cases where > + * a non-abstract typename mentioned on compat_props > + * has subclasses, like spapr-pci-host-bridge. > + */ > + object_class_foreach(machine_register_compat_for_subclass, > + p->driver, false, p); > + } else { > + register_compat_prop(p->driver, p->property, p->value); > + } > } > } >
On Wed, Dec 14, 2016 at 04:42:14PM +0100, Greg Kurz wrote: > On Mon, 12 Dec 2016 18:49:05 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > Original problem description by Greg Kurz: > > > > > 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. > > > > The same bug also affects other abstract type names mentioned on > > compat_props by machine-types: apic-common, i386-cpu, pci-device, > > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > > virtio-pci, x86_64-cpu. > > > > The right fix for this problem is to make sure compat_props and > > -global options are always applied in the order they are > > registered, instead of reordering them based on the type > > hierarchy. But changing the ordering rules of -global is risky > > and might break existing configurations, so we shouldn't do that > > on a stable branch. > > > > This is a temporary hack that will work around the bug when > > registering compat_props properties: if we find an abstract class > > on compat_props, register properties for all its non-abstract > > subtypes instead. This will make sure -global won't be overridden > > by compat_props, while keeping the existing ordering rules on > > -global options. > > > > Note that there's one case that won't be fixed by this hack: > > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > > able to override compat_props, because spapr-pci-host-bridge is > > not an abstract class. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > This patch had 3 r-b. Is there any good reason to hold > it up to 2.8.1 ? Applying it today would mean delaying the 2.8.0 release, and the bug is not serious enough to justify that.
On Wed, 14 Dec 2016 15:04:27 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 14, 2016 at 04:42:14PM +0100, Greg Kurz wrote: > > On Mon, 12 Dec 2016 18:49:05 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > Original problem description by Greg Kurz: > > > > > > > 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. > > > > > > The same bug also affects other abstract type names mentioned on > > > compat_props by machine-types: apic-common, i386-cpu, pci-device, > > > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > > > virtio-pci, x86_64-cpu. > > > > > > The right fix for this problem is to make sure compat_props and > > > -global options are always applied in the order they are > > > registered, instead of reordering them based on the type > > > hierarchy. But changing the ordering rules of -global is risky > > > and might break existing configurations, so we shouldn't do that > > > on a stable branch. > > > > > > This is a temporary hack that will work around the bug when > > > registering compat_props properties: if we find an abstract class > > > on compat_props, register properties for all its non-abstract > > > subtypes instead. This will make sure -global won't be overridden > > > by compat_props, while keeping the existing ordering rules on > > > -global options. > > > > > > Note that there's one case that won't be fixed by this hack: > > > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > > > able to override compat_props, because spapr-pci-host-bridge is > > > not an abstract class. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > > This patch had 3 r-b. Is there any good reason to hold > > it up to 2.8.1 ? > > Applying it today would mean delaying the 2.8.0 release, and the > bug is not serious enough to justify that. > IIUC there will be a v2.8.0-rc4 because we want this patch in 2.8: [PATCH v3] virtio-pci: Fix cross-version migration with older machines From IRC: <stefanha> mcoquelin: Once Michael Tsirkin has reviewed your patch I'll merge it and tag v2.8.0-rc4 (today or tomorrow?). The QEMU 2.8 release will be next Tuesday.
On Wed, Dec 14, 2016 at 06:22:33PM +0100, Greg Kurz wrote: > On Wed, 14 Dec 2016 15:04:27 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Dec 14, 2016 at 04:42:14PM +0100, Greg Kurz wrote: > > > On Mon, 12 Dec 2016 18:49:05 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > Original problem description by Greg Kurz: > > > > > > > > > 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. > > > > > > > > The same bug also affects other abstract type names mentioned on > > > > compat_props by machine-types: apic-common, i386-cpu, pci-device, > > > > powerpc64-cpu, s390-skeys, spapr-pci-host-bridge, usb-device, > > > > virtio-pci, x86_64-cpu. > > > > > > > > The right fix for this problem is to make sure compat_props and > > > > -global options are always applied in the order they are > > > > registered, instead of reordering them based on the type > > > > hierarchy. But changing the ordering rules of -global is risky > > > > and might break existing configurations, so we shouldn't do that > > > > on a stable branch. > > > > > > > > This is a temporary hack that will work around the bug when > > > > registering compat_props properties: if we find an abstract class > > > > on compat_props, register properties for all its non-abstract > > > > subtypes instead. This will make sure -global won't be overridden > > > > by compat_props, while keeping the existing ordering rules on > > > > -global options. > > > > > > > > Note that there's one case that won't be fixed by this hack: > > > > "-global spapr-pci-vfio-host-bridge.<option>=<value>" won't be > > > > able to override compat_props, because spapr-pci-host-bridge is > > > > not an abstract class. > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > > > This patch had 3 r-b. Is there any good reason to hold > > > it up to 2.8.1 ? > > > > Applying it today would mean delaying the 2.8.0 release, and the > > bug is not serious enough to justify that. > > > > IIUC there will be a v2.8.0-rc4 because we want this patch in 2.8: > > [PATCH v3] virtio-pci: Fix cross-version migration with older machines > > From IRC: > > <stefanha> mcoquelin: Once Michael Tsirkin has reviewed your patch I'll merge it and tag v2.8.0-rc4 (today or tomorrow?). The QEMU 2.8 release will be next Tuesday. Oh, I didn't know that. Thanks for the pointer. But I'd still like to have more time to test this patch, as it affects multiple machine-types on multiple targets. I think the hack is a bit risky to be included in the last minute before the final release.
diff --git a/hw/core/machine.c b/hw/core/machine.c index b0fd91f..213e8e0 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -554,11 +554,31 @@ 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); int i; GlobalProperty *p; + ObjectClass *oc; if (!mc->compat_props) { return; @@ -566,9 +586,22 @@ void machine_register_compat_props(MachineState *machine) for (i = 0; i < mc->compat_props->len; i++) { 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 do not override + * globals set explicitly on -global: if an abstract class + * is on compat_props, register globals for all its + * non-abstract subtypes instead. + * + * This doesn't solve the problem for cases where + * a non-abstract typename mentioned on compat_props + * has subclasses, like spapr-pci-host-bridge. + */ + object_class_foreach(machine_register_compat_for_subclass, + p->driver, false, p); + } else { + register_compat_prop(p->driver, p->property, p->value); + } } }