| Message ID | 148104617887.7699.984921868108236415.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
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(-)
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",\ > }, >
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.
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",\ > },
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.
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
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?
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
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.
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
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
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",\ },
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(-)