* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 17:50 [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Greg Kurz
@ 2016-12-06 18:02 ` Cornelia Huck
2016-12-06 18:46 ` Eduardo Habkost
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2016-12-06 18:02 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
On Tue, 06 Dec 2016 18:50:47 +0100
Greg Kurz <groug@kaod.org> 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.
s/prevail/prevails/
>
> 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
s/subtypes/subtype/
> virtio-pci type.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>
> This fix is for both QEMU 2.8 and 2.7.1.
>
> Generated with:
>
> for i in $(git grep 'define TYPE_VIRTIO_.*_PCI' hw/virtio/virtio-pci.h | \
> awk '{print $3 }'); do
> printf '{\\'"\n .driver = %s,"'\\'"\n .property = \"disable-modern\","'\\'"\n .value = \"on\","'\\'"\n },{"'\\'"\n .driver = %s,"'\\'"\n .property = \"disable-legacy\","'\\'"\n .value = \"off\","'\\'"\n }," $i $i
> done
Don't we want vhost-scsi-pci as well?
>
> include/hw/compat.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 114 insertions(+), 2 deletions(-)
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 17:50 [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Greg Kurz
2016-12-06 18:02 ` Cornelia Huck
@ 2016-12-06 18:46 ` Eduardo Habkost
2016-12-06 19:19 ` Eduardo Habkost
2016-12-06 19:31 ` [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Michael S. Tsirkin
3 siblings, 0 replies; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 18:46 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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>
> ---
>
> This fix is for both QEMU 2.8 and 2.7.1.
>
> Generated with:
>
> for i in $(git grep 'define TYPE_VIRTIO_.*_PCI' hw/virtio/virtio-pci.h | \
> awk '{print $3 }'); do
> printf '{\\'"\n .driver = %s,"'\\'"\n .property = \"disable-modern\","'\\'"\n .value = \"on\","'\\'"\n },{"'\\'"\n .driver = %s,"'\\'"\n .property = \"disable-legacy\","'\\'"\n .value = \"off\","'\\'"\n }," $i $i
> done
I have manually generated a list of typenames using
"qom-list-types implements=virtio-pci" on all QEMU binaries built
on my system. The list I got is:
* vhost-scsi-pci
* vhost-vsock-pci
* virtio-9p-pci
* virtio-balloon-pci
* virtio-blk-pci
* virtio-crypto-pci
* virtio-gpu-pci
* virtio-input-host-pci
* virtio-keyboard-pci
* virtio-mouse-pci
* virtio-net-pci
* virtio-rng-pci
* virtio-scsi-pci
* virtio-serial-pci
* virtio-tablet-pci
* virtio-vga
The following types are missing in the patch:
* vhost-scsi-pci
* vhost-vsock-pci
* virtio-vga
The following types are abstract classes and shouldn't be on the
compat_props list:
* virtio-input-hid-pci
* virtio-input-pci
>
> include/hw/compat.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e113bee2..793ba3ddb393 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -26,11 +26,123 @@
> .property = "format_transport_address",\
> .value = "off",\
> },{\
> - .driver = "virtio-pci",\
> + .driver = "virtio-scsi-pci",\
> .property = "disable-modern",\
> .value = "on",\
> },{\
> - .driver = "virtio-pci",\
> + .driver = "virtio-scsi-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-blk-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-blk-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-balloon-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-balloon-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-serial-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-serial-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-net-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-net-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-9p-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-9p-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-rng-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-rng-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-hid-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-hid-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-keyboard-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-keyboard-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-mouse-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-mouse-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-tablet-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-tablet-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-host-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-host-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-gpu-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-gpu-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-crypto-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-crypto-pci",\
> .property = "disable-legacy",\
> .value = "off",\
> },
>
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 17:50 [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Greg Kurz
2016-12-06 18:02 ` Cornelia Huck
2016-12-06 18:46 ` Eduardo Habkost
@ 2016-12-06 19:19 ` Eduardo Habkost
2016-12-06 19:31 ` [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names Eduardo Habkost
2016-12-06 19:31 ` [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Michael S. Tsirkin
3 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 19:19 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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.
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-06 19:19 ` Eduardo Habkost
@ 2016-12-06 19:31 ` Eduardo Habkost
2016-12-07 13:39 ` Greg Kurz
2016-12-12 12:25 ` Cornelia Huck
0 siblings, 2 replies; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 19:31 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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(-)
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);
+ }
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-06 19:31 ` [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names Eduardo Habkost
@ 2016-12-07 13:39 ` Greg Kurz
2016-12-07 13:59 ` Eduardo Habkost
2016-12-09 20:06 ` Eduardo Habkost
2016-12-12 12:25 ` Cornelia Huck
1 sibling, 2 replies; 24+ messages in thread
From: Greg Kurz @ 2016-12-07 13:39 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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);
> + }
> }
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-07 13:39 ` Greg Kurz
@ 2016-12-07 13:59 ` Eduardo Habkost
2016-12-07 14:58 ` Greg Kurz
2016-12-09 20:06 ` Eduardo Habkost
1 sibling, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-07 13:59 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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.
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-07 13:59 ` Eduardo Habkost
@ 2016-12-07 14:58 ` Greg Kurz
0 siblings, 0 replies; 24+ messages in thread
From: Greg Kurz @ 2016-12-07 14:58 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Halil Pasic, Michael S. Tsirkin, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-07 13:39 ` Greg Kurz
2016-12-07 13:59 ` Eduardo Habkost
@ 2016-12-09 20:06 ` Eduardo Habkost
2016-12-12 11:26 ` Cornelia Huck
2016-12-12 11:39 ` Greg Kurz
1 sibling, 2 replies; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-09 20:06 UTC (permalink / raw)
To: Greg Kurz
Cc: Halil Pasic, Michael S. Tsirkin, qemu-stable, qemu-devel,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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)
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-09 20:06 ` Eduardo Habkost
@ 2016-12-12 11:26 ` Cornelia Huck
2016-12-12 11:39 ` Greg Kurz
1 sibling, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2016-12-12 11:26 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Greg Kurz, Halil Pasic, Michael S. Tsirkin, qemu-stable,
qemu-devel, Stefan Hajnoczi, Paolo Bonzini
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-09 20:06 ` Eduardo Habkost
2016-12-12 11:26 ` Cornelia Huck
@ 2016-12-12 11:39 ` Greg Kurz
1 sibling, 0 replies; 24+ messages in thread
From: Greg Kurz @ 2016-12-12 11:39 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Halil Pasic, Michael S. Tsirkin, qemu-stable, qemu-devel,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-06 19:31 ` [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names Eduardo Habkost
2016-12-07 13:39 ` Greg Kurz
@ 2016-12-12 12:25 ` Cornelia Huck
2016-12-12 17:13 ` Halil Pasic
2016-12-12 17:36 ` Eduardo Habkost
1 sibling, 2 replies; 24+ messages in thread
From: Cornelia Huck @ 2016-12-12 12:25 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Greg Kurz, qemu-devel, Halil Pasic, Michael S. Tsirkin,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
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);
> + }
> }
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-12 12:25 ` Cornelia Huck
@ 2016-12-12 17:13 ` Halil Pasic
2016-12-12 17:47 ` Eduardo Habkost
2016-12-12 17:36 ` Eduardo Habkost
1 sibling, 1 reply; 24+ messages in thread
From: Halil Pasic @ 2016-12-12 17:13 UTC (permalink / raw)
To: Cornelia Huck, Eduardo Habkost
Cc: Michael S. Tsirkin, qemu-devel, Greg Kurz, qemu-stable,
Stefan Hajnoczi, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-12 17:13 ` Halil Pasic
@ 2016-12-12 17:47 ` Eduardo Habkost
2016-12-12 18:00 ` Halil Pasic
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-12 17:47 UTC (permalink / raw)
To: Halil Pasic
Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel, Greg Kurz,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
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!
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-12 17:47 ` Eduardo Habkost
@ 2016-12-12 18:00 ` Halil Pasic
0 siblings, 0 replies; 24+ messages in thread
From: Halil Pasic @ 2016-12-12 18:00 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel, Greg Kurz,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [RFC for-2.8] machine: Convert abstract typename on compat_props to subclass names
2016-12-12 12:25 ` Cornelia Huck
2016-12-12 17:13 ` Halil Pasic
@ 2016-12-12 17:36 ` Eduardo Habkost
1 sibling, 0 replies; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-12 17:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: Greg Kurz, qemu-devel, Halil Pasic, Michael S. Tsirkin,
qemu-stable, Stefan Hajnoczi, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 17:50 [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Greg Kurz
` (2 preceding siblings ...)
2016-12-06 19:19 ` Eduardo Habkost
@ 2016-12-06 19:31 ` Michael S. Tsirkin
2016-12-06 19:33 ` Eduardo Habkost
3 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2016-12-06 19:31 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Halil Pasic, Eduardo Habkost, qemu-stable,
Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
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>
What's the rush? Either the issue is or isn't 2.8 material. If it is,
let's fix it in qdev core now. If it isn't no need for quick fixes,
just wait a bit and merge qdev core fix after 2.8 is out.
> ---
>
> This fix is for both QEMU 2.8 and 2.7.1.
>
> Generated with:
>
> for i in $(git grep 'define TYPE_VIRTIO_.*_PCI' hw/virtio/virtio-pci.h | \
> awk '{print $3 }'); do
> printf '{\\'"\n .driver = %s,"'\\'"\n .property = \"disable-modern\","'\\'"\n .value = \"on\","'\\'"\n },{"'\\'"\n .driver = %s,"'\\'"\n .property = \"disable-legacy\","'\\'"\n .value = \"off\","'\\'"\n }," $i $i
> done
>
> include/hw/compat.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 0f06e113bee2..793ba3ddb393 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -26,11 +26,123 @@
> .property = "format_transport_address",\
> .value = "off",\
> },{\
> - .driver = "virtio-pci",\
> + .driver = "virtio-scsi-pci",\
> .property = "disable-modern",\
> .value = "on",\
> },{\
> - .driver = "virtio-pci",\
> + .driver = "virtio-scsi-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-blk-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-blk-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-balloon-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-balloon-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-serial-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-serial-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-net-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-net-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-9p-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-9p-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-rng-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-rng-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-hid-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-hid-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-keyboard-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-keyboard-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-mouse-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-mouse-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-tablet-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-tablet-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-input-host-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-input-host-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-gpu-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-gpu-pci",\
> + .property = "disable-legacy",\
> + .value = "off",\
> + },{\
> + .driver = "virtio-crypto-pci",\
> + .property = "disable-modern",\
> + .value = "on",\
> + },{\
> + .driver = "virtio-crypto-pci",\
> .property = "disable-legacy",\
> .value = "off",\
> },
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:31 ` [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers Michael S. Tsirkin
@ 2016-12-06 19:33 ` Eduardo Habkost
2016-12-06 19:36 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 19:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kurz, qemu-devel, Halil Pasic, qemu-stable, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini
On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
>
> What's the rush? Either the issue is or isn't 2.8 material. If it is,
> let's fix it in qdev core now. If it isn't no need for quick fixes,
> just wait a bit and merge qdev core fix after 2.8 is out.
Because it is a regression, and changing HW_COMPAT_* is less
risky than changing qdev core rules about -global ordering.
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:33 ` Eduardo Habkost
@ 2016-12-06 19:36 ` Michael S. Tsirkin
2016-12-06 19:40 ` Eduardo Habkost
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2016-12-06 19:36 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Greg Kurz, qemu-devel, Halil Pasic, qemu-stable, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini
On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> >
> > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > just wait a bit and merge qdev core fix after 2.8 is out.
>
> Because it is a regression,
Regression in 2.7 though, isn't it?
> and changing HW_COMPAT_* is less
> risky than changing qdev core rules about -global ordering.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:36 ` Michael S. Tsirkin
@ 2016-12-06 19:40 ` Eduardo Habkost
2016-12-06 19:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 19:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kurz, qemu-devel, Halil Pasic, qemu-stable, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini
On Tue, Dec 06, 2016 at 09:36:07PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> > On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> > >
> > > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > > just wait a bit and merge qdev core fix after 2.8 is out.
> >
> > Because it is a regression,
>
> Regression in 2.7 though, isn't it?
Yes. Does that mean we don't mind leaving a regression unfixed in
2.8 because it was introduced in 2.7?
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:40 ` Eduardo Habkost
@ 2016-12-06 19:44 ` Michael S. Tsirkin
2016-12-06 19:48 ` Eduardo Habkost
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2016-12-06 19:44 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Greg Kurz, qemu-devel, Halil Pasic, qemu-stable, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini
On Tue, Dec 06, 2016 at 05:40:29PM -0200, Eduardo Habkost wrote:
> On Tue, Dec 06, 2016 at 09:36:07PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> > > On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> > > >
> > > > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > > > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > > > just wait a bit and merge qdev core fix after 2.8 is out.
> > >
> > > Because it is a regression,
> >
> > Regression in 2.7 though, isn't it?
>
> Yes. Does that mean we don't mind leaving a regression unfixed in
> 2.8 because it was introduced in 2.7?
Not necessarily, it's a judgement call. But there will always be
another release. I would say don't rush it.
> --
> Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:44 ` Michael S. Tsirkin
@ 2016-12-06 19:48 ` Eduardo Habkost
2016-12-06 20:30 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2016-12-06 19:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kurz, qemu-devel, Halil Pasic, qemu-stable, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini
On Tue, Dec 06, 2016 at 09:44:11PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 06, 2016 at 05:40:29PM -0200, Eduardo Habkost wrote:
> > On Tue, Dec 06, 2016 at 09:36:07PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> > > > >
> > > > > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > > > > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > > > > just wait a bit and merge qdev core fix after 2.8 is out.
> > > >
> > > > Because it is a regression,
> > >
> > > Regression in 2.7 though, isn't it?
> >
> > Yes. Does that mean we don't mind leaving a regression unfixed in
> > 2.8 because it was introduced in 2.7?
>
> Not necessarily, it's a judgement call. But there will always be
> another release. I would say don't rush it.
My judgement is that it won't hurt to include the HW_COMPAT_* fix
for virtio-pci (this patch, after including the missing classes I
pointed out), because it is simple and obvious.
But I agree that this means we don't need the more complex hack I
just sent as RFC.
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 19:48 ` Eduardo Habkost
@ 2016-12-06 20:30 ` Stefan Hajnoczi
2016-12-07 8:36 ` Greg Kurz
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2016-12-06 20:30 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Michael S. Tsirkin, Halil Pasic, qemu-devel, qemu-stable,
Greg Kurz, Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]
On Tue, Dec 06, 2016 at 05:48:00PM -0200, Eduardo Habkost wrote:
> On Tue, Dec 06, 2016 at 09:44:11PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 06, 2016 at 05:40:29PM -0200, Eduardo Habkost wrote:
> > > On Tue, Dec 06, 2016 at 09:36:07PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> > > > > On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> > > > > >
> > > > > > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > > > > > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > > > > > just wait a bit and merge qdev core fix after 2.8 is out.
> > > > >
> > > > > Because it is a regression,
> > > >
> > > > Regression in 2.7 though, isn't it?
> > >
> > > Yes. Does that mean we don't mind leaving a regression unfixed in
> > > 2.8 because it was introduced in 2.7?
> >
> > Not necessarily, it's a judgement call. But there will always be
> > another release. I would say don't rush it.
>
> My judgement is that it won't hurt to include the HW_COMPAT_* fix
> for virtio-pci (this patch, after including the missing classes I
> pointed out), because it is simple and obvious.
>
> But I agree that this means we don't need the more complex hack I
> just sent as RFC.
This has missed QEMU 2.8.0-rc3 (the deadline was 19:00 UTC December
6th).
This doesn't seem like a release blocker anyway since it's not a 2.8
regression, it can be fixed in a -stable release.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers
2016-12-06 20:30 ` Stefan Hajnoczi
@ 2016-12-07 8:36 ` Greg Kurz
0 siblings, 0 replies; 24+ messages in thread
From: Greg Kurz @ 2016-12-07 8:36 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Eduardo Habkost, Michael S. Tsirkin, Halil Pasic, qemu-devel,
qemu-stable, Stefan Hajnoczi, Cornelia Huck, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]
On Tue, 6 Dec 2016 20:30:52 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Dec 06, 2016 at 05:48:00PM -0200, Eduardo Habkost wrote:
> > On Tue, Dec 06, 2016 at 09:44:11PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 06, 2016 at 05:40:29PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Dec 06, 2016 at 09:36:07PM +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Dec 06, 2016 at 05:33:49PM -0200, Eduardo Habkost wrote:
> > > > > > On Tue, Dec 06, 2016 at 09:31:43PM +0200, Michael S. Tsirkin 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>
> > > > > > >
> > > > > > > What's the rush? Either the issue is or isn't 2.8 material. If it is,
> > > > > > > let's fix it in qdev core now. If it isn't no need for quick fixes,
> > > > > > > just wait a bit and merge qdev core fix after 2.8 is out.
> > > > > >
> > > > > > Because it is a regression,
> > > > >
> > > > > Regression in 2.7 though, isn't it?
> > > >
> > > > Yes. Does that mean we don't mind leaving a regression unfixed in
> > > > 2.8 because it was introduced in 2.7?
> > >
> > > Not necessarily, it's a judgement call. But there will always be
> > > another release. I would say don't rush it.
> >
> > My judgement is that it won't hurt to include the HW_COMPAT_* fix
> > for virtio-pci (this patch, after including the missing classes I
> > pointed out), because it is simple and obvious.
> >
> > But I agree that this means we don't need the more complex hack I
> > just sent as RFC.
>
> This has missed QEMU 2.8.0-rc3 (the deadline was 19:00 UTC December
> 6th).
>
> This doesn't seem like a release blocker anyway since it's not a 2.8
> regression, it can be fixed in a -stable release.
>
> Stefan
Sure. There were suggestions to handle the issue in an appropriate
manner. Fixes can be applied downstream if needed.
Thanks everyone!
--
Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread