[Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[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.

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

 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",\
     },

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Cornelia Huck]

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(-)

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Michael S. Tsirkin]

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",\
>      },

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

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Stefan Hajnoczi]

[-- 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 --]

Re: [Qemu-devel] [PATCH] virtio: fix HW_COMPAT_2_6 macro for virtio-*-pci drivers

[Greg Kurz]

[-- 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 --]

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

[Greg Kurz]

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);
> +        }
>      }
>  }
>  

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

[Eduardo Habkost]

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

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

[Greg Kurz]

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

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

[Eduardo Habkost]

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

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

[Cornelia Huck]

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.

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

[Greg Kurz]

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

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

[Cornelia Huck]

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);
> +        }
>      }
>  }
> 

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

[Halil Pasic]

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

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

[Eduardo Habkost]

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

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

[Eduardo Habkost]

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

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

[Halil Pasic]

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