[libvirt] [PATCH] qemu: fix QEMU_CAPS_NO_ACPI detection
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless. This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390. It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 167cdb2..e0d0c2a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2175,13 +2175,11 @@ qemuCapsInitHelp(qemuCapsPtr caps, uid_t runUid, gid_t runGid) if (caps->arch == VIR_ARCH_X86_64 || caps->arch == VIR_ARCH_I686) { qemuCapsSet(caps, QEMU_CAPS_PCI_MULTIBUS); - } - - /* S390 and probably other archs do not support no-acpi - - maybe the qemu option parsing should be re-thought. */ - if (caps->arch == VIR_ARCH_S390 || - caps->arch == VIR_ARCH_S390X) + } else { + /* -no-acpi is not supported on other archs + * even if qemu reports it in -help */ qemuCapsClear(caps, QEMU_CAPS_NO_ACPI); + } /* qemuCapsExtractDeviceStr will only set additional caps if qemu * understands the 0.13.0+ notion of "-device driver,". */ @@ -2260,13 +2258,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) qemuCapsSet(caps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC); qemuCapsSet(caps, QEMU_CAPS_NO_SHUTDOWN); qemuCapsSet(caps, QEMU_CAPS_DRIVE_CACHE_UNSAFE); - - /* ACPI is only supported on x86, PPC or - * other platforms don't support it*/ - if (caps->arch == VIR_ARCH_I686 || - caps->arch == VIR_ARCH_X86_64) - qemuCapsSet(caps, QEMU_CAPS_NO_ACPI); - qemuCapsSet(caps, QEMU_CAPS_FSDEV_READONLY); qemuCapsSet(caps, QEMU_CAPS_VIRTIO_BLK_SG_IO); qemuCapsSet(caps, QEMU_CAPS_DRIVE_COPY_ON_READ); @@ -2402,16 +2393,12 @@ qemuCapsInitQMP(qemuCapsPtr caps, } VIR_FREE(archstr); - /* Currently only x86_64 and i686 support PCI-multibus. */ + /* Currently only x86_64 and i686 support PCI-multibus and -no-acpi. */ if (caps->arch == VIR_ARCH_X86_64 || - caps->arch == VIR_ARCH_I686) + caps->arch == VIR_ARCH_I686) { qemuCapsSet(caps, QEMU_CAPS_PCI_MULTIBUS); - - /* S390 and probably other archs do not support no-acpi - - maybe the qemu option parsing should be re-thought. */ - if (caps->arch == VIR_ARCH_S390 || - caps->arch == VIR_ARCH_S390X) - qemuCapsClear(caps, QEMU_CAPS_NO_ACPI); + qemuCapsSet(caps, QEMU_CAPS_NO_ACPI); + } if (qemuCapsProbeQMPCommands(caps, mon) < 0) goto cleanup; -- 1.7.8.6
On 12/21/2012 02:38 PM, Ján Tomko wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
Definitely good to see any S390 special handling become obsolete. +1 from my side. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Dec 21, 2012 at 9:38 PM, Ján Tomko <jtomko@redhat.com> wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
Will it get QEMU_CAPS_NO_ACPI capability by QMP if S390 doesn't support ACPI? I test it on PPC, it won't get this capability by QMP. Thanks. It also clears the flag for all non-x86 archs instead of just S390 in
qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 167cdb2..e0d0c2a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2175,13 +2175,11 @@ qemuCapsInitHelp(qemuCapsPtr caps, uid_t runUid, gid_t runGid) if (caps->arch == VIR_ARCH_X86_64 || caps->arch == VIR_ARCH_I686) { qemuCapsSet(caps, QEMU_CAPS_PCI_MULTIBUS); - } - - /* S390 and probably other archs do not support no-acpi - - maybe the qemu option parsing should be re-thought. */ - if (caps->arch == VIR_ARCH_S390 || - caps->arch == VIR_ARCH_S390X) + } else { + /* -no-acpi is not supported on other archs + * even if qemu reports it in -help */ qemuCapsClear(caps, QEMU_CAPS_NO_ACPI); + }
/* qemuCapsExtractDeviceStr will only set additional caps if qemu * understands the 0.13.0+ notion of "-device driver,". */ @@ -2260,13 +2258,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) qemuCapsSet(caps, QEMU_CAPS_DRIVE_CACHE_DIRECTSYNC); qemuCapsSet(caps, QEMU_CAPS_NO_SHUTDOWN); qemuCapsSet(caps, QEMU_CAPS_DRIVE_CACHE_UNSAFE); - - /* ACPI is only supported on x86, PPC or - * other platforms don't support it*/ - if (caps->arch == VIR_ARCH_I686 || - caps->arch == VIR_ARCH_X86_64) - qemuCapsSet(caps, QEMU_CAPS_NO_ACPI); - qemuCapsSet(caps, QEMU_CAPS_FSDEV_READONLY); qemuCapsSet(caps, QEMU_CAPS_VIRTIO_BLK_SG_IO); qemuCapsSet(caps, QEMU_CAPS_DRIVE_COPY_ON_READ); @@ -2402,16 +2393,12 @@ qemuCapsInitQMP(qemuCapsPtr caps, } VIR_FREE(archstr);
- /* Currently only x86_64 and i686 support PCI-multibus. */ + /* Currently only x86_64 and i686 support PCI-multibus and -no-acpi. */ if (caps->arch == VIR_ARCH_X86_64 || - caps->arch == VIR_ARCH_I686) + caps->arch == VIR_ARCH_I686) { qemuCapsSet(caps, QEMU_CAPS_PCI_MULTIBUS); - - /* S390 and probably other archs do not support no-acpi - - maybe the qemu option parsing should be re-thought. */ - if (caps->arch == VIR_ARCH_S390 || - caps->arch == VIR_ARCH_S390X) - qemuCapsClear(caps, QEMU_CAPS_NO_ACPI); + qemuCapsSet(caps, QEMU_CAPS_NO_ACPI); + }
if (qemuCapsProbeQMPCommands(caps, mon) < 0) goto cleanup; -- 1.7.8.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best Regards -Li
On 12/21/12 15:07, Li Zhang wrote:
On Fri, Dec 21, 2012 at 9:38 PM, Ján Tomko <jtomko@redhat.com <mailto:jtomko@redhat.com>> wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
Will it get QEMU_CAPS_NO_ACPI capability by QMP if S390 doesn't support ACPI? I test it on PPC, it won't get this capability by QMP.
Thanks.
No, QEMU_CAPS_NO_ACPI capability will only be set on i686 and x86_64. Jan
On Fri, Dec 21, 2012 at 10:20 PM, Ján Tomko <jtomko@redhat.com> wrote:
On 12/21/12 15:07, Li Zhang wrote:
On Fri, Dec 21, 2012 at 9:38 PM, Ján Tomko <jtomko@redhat.com <mailto:jtomko@redhat.com>> wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering
the
whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
Will it get QEMU_CAPS_NO_ACPI capability by QMP if S390 doesn't support ACPI? I test it on PPC, it won't get this capability by QMP.
Thanks.
No, QEMU_CAPS_NO_ACPI capability will only be set on i686 and x86_64.
So I think there shouldn't be the conditional setting in qemuCapsInitQMP. It doesn't need to clear this capability in this place. Jan
-- Best Regards -Li
On 12/21/2012 03:07 PM, Li Zhang wrote:
Will it get QEMU_CAPS_NO_ACPI capability by QMP if S390 doesn't support ACPI? I test it on PPC, it won't get this capability by QMP.
In the QMP case, basic capabilities are not being probed, but assumed to be present, however -no-acpi is only valid for x86, hence the conditional setting. The old help parsing code on older QEMUs could report -no-acpi for S390 (respectively non-x86), which has to be removed from the capability set to avoid errors starting the guest. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Dec 21, 2012 at 10:37 PM, Viktor Mihajlovski < mihajlov@linux.vnet.ibm.com> wrote:
On 12/21/2012 03:07 PM, Li Zhang wrote:
Will it get QEMU_CAPS_NO_ACPI capability by QMP if S390 doesn't support ACPI? I test it on PPC, it won't get this capability by QMP.
In the QMP case, basic capabilities are not being probed, but assumed
to be present, however -no-acpi is only valid for x86, hence the conditional setting.
You mean it still is present. I didn't get it on POWER.
The old help parsing code on older QEMUs could report -no-acpi for S390 (respectively non-x86), which has to be removed from the capability set to avoid errors starting the guest.
On POWER, QEMU's help string doesn't include -no-acpi. It seems that QEMU handles this differently. :) --
Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski
IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
-- Best Regards -Li
On 12/21/2012 03:45 PM, Li Zhang wrote:
On POWER, QEMU's help string doesn't include -no-acpi. It seems that QEMU handles this differently. :)
it depends on the version: newer QEMUs for S390 don't include -no-acpi either ... but libvirt should remain compatible with older QEMUs -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/21/12 14:38, Ján Tomko wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
gentle ping
On 2013年01月15日 19:47, Ján Tomko wrote:
On 12/21/12 14:38, Ján Tomko wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
gentle ping
This looks good for PPC. :) Thanks.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/21/2012 06:38 AM, Ján Tomko wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/16/13 00:04, Eric Blake wrote:
On 12/21/2012 06:38 AM, Ján Tomko wrote:
In commit c4bbaaf8, caps->arch was checked uninitialized, rendering the whole check useless.
This patch moves the conditional setting of QEMU_CAPS_NO_ACPI to qemuCapsInitQMP, and removes the no longer needed exception for S390.
It also clears the flag for all non-x86 archs instead of just S390 in qemuCapsInitHelp. --- src/qemu/qemu_capabilities.c | 29 ++++++++--------------------- 1 files changed, 8 insertions(+), 21 deletions(-)
ACK.
Thanks, pushed.
participants (4)
-
Eric Blake -
Ján Tomko -
Li Zhang -
Viktor Mihajlovski