So far, libvirt has assumed that only x86 supports ACPI,
but that's inaccurate since aarch64 supports it too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1429509
---
src/qemu/qemu_capabilities.c | 28 ++++++++++++++++------
.../caps_2.6.0-gicv2.aarch64.xml | 1 +
.../caps_2.6.0-gicv3.aarch64.xml | 1 +
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 5a3b4ac..4ec34f8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1038,13 +1038,17 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
machines = NULL;
nmachines = 0;
+ }
+ if ((ARCH_IS_X86(guestarch) || guestarch == VIR_ARCH_AARCH64) &&
+ virCapabilitiesAddGuestFeature(guest, "acpi", true, true) == NULL) {
+ goto cleanup;
}
if (ARCH_IS_X86(guestarch) &&
- (virCapabilitiesAddGuestFeature(guest, "acpi", true, true) == NULL ||
- virCapabilitiesAddGuestFeature(guest, "apic", true, false) == NULL))
+ virCapabilitiesAddGuestFeature(guest, "apic", true, false) == NULL) {
goto cleanup;
+ }
if ((guestarch == VIR_ARCH_I686) &&
(virCapabilitiesAddGuestFeature(guest, "pae", true, false) == NULL ||
@@ -4122,10 +4126,15 @@ virQEMUCapsInitHelp(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid, const c
qmperr) < 0)
goto cleanup;
- /* -no-acpi is not supported on non-x86
- * even if qemu reports it in -help */
- if (!ARCH_IS_X86(qemuCaps->arch))
+ /* Older QEMU versions reported -no-acpi in the output of -help even
+ * though it was not supported by the architecture. The issue has since
+ * been fixed, but to maintain compatibility with all release we still
+ * need to filter out the capability for architectures that we know
+ * don't support the feature, eg. anything but x86 and aarch64 */
+ if (!ARCH_IS_X86(qemuCaps->arch) &&
+ qemuCaps->arch != VIR_ARCH_AARCH64) {
virQEMUCapsClear(qemuCaps, QEMU_CAPS_NO_ACPI);
+ }
/* virQEMUCapsExtractDeviceStr will only set additional caps if qemu
* understands the 0.13.0+ notion of "-device driver,". */
@@ -4222,9 +4231,14 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
goto cleanup;
}
- /* ACPI/HPET/KVM PIT are x86 specific */
- if (ARCH_IS_X86(qemuCaps->arch)) {
+ /* ACPI only works on x86 and aarch64 */
+ if (ARCH_IS_X86(qemuCaps->arch) ||
+ qemuCaps->arch == VIR_ARCH_AARCH64) {
virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
+ }
+
+ /* HPET and KVM PIT are x86 specific */
+ if (ARCH_IS_X86(qemuCaps->arch)) {
virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
}
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
index 0aed651..b64a6f8 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
@@ -40,6 +40,7 @@
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
<flag name='ich9-ahci'/>
+ <flag name='no-acpi'/>
<flag name='fsdev-readonly'/>
<flag name='virtio-blk-pci.scsi'/>
<flag name='drive-copy-on-read'/>
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
index 1041a12..46e368f 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
@@ -40,6 +40,7 @@
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
<flag name='ich9-ahci'/>
+ <flag name='no-acpi'/>
<flag name='fsdev-readonly'/>
<flag name='virtio-blk-pci.scsi'/>
<flag name='drive-copy-on-read'/>
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/09/2017 12:37 PM, Andrea Bolognani wrote:
> So far, libvirt has assumed that only x86 supports ACPI,
> but that's inaccurate since aarch64 supports it too.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1429509
> ---
> src/qemu/qemu_capabilities.c | 28 ++++++++++++++++------
> .../caps_2.6.0-gicv2.aarch64.xml | 1 +
> .../caps_2.6.0-gicv3.aarch64.xml | 1 +
> 3 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 5a3b4ac..4ec34f8 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1038,13 +1038,17 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
>
> machines = NULL;
> nmachines = 0;
> + }
>
> + if ((ARCH_IS_X86(guestarch) || guestarch == VIR_ARCH_AARCH64) &&
> + virCapabilitiesAddGuestFeature(guest, "acpi", true, true) == NULL) {
> + goto cleanup;
> }
>
> if (ARCH_IS_X86(guestarch) &&
> - (virCapabilitiesAddGuestFeature(guest, "acpi", true, true) == NULL ||
> - virCapabilitiesAddGuestFeature(guest, "apic", true, false) == NULL))
> + virCapabilitiesAddGuestFeature(guest, "apic", true, false) == NULL) {
> goto cleanup;
> + }
>
> if ((guestarch == VIR_ARCH_I686) &&
> (virCapabilitiesAddGuestFeature(guest, "pae", true, false) == NULL ||
> @@ -4122,10 +4126,15 @@ virQEMUCapsInitHelp(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid, const c
> qmperr) < 0)
> goto cleanup;
>
> - /* -no-acpi is not supported on non-x86
> - * even if qemu reports it in -help */
> - if (!ARCH_IS_X86(qemuCaps->arch))
> + /* Older QEMU versions reported -no-acpi in the output of -help even
> + * though it was not supported by the architecture. The issue has since
> + * been fixed, but to maintain compatibility with all release we still
"releases"
> + * need to filter out the capability for architectures that we know
> + * don't support the feature, eg. anything but x86 and aarch64 */
> + if (!ARCH_IS_X86(qemuCaps->arch) &&
> + qemuCaps->arch != VIR_ARCH_AARCH64) {
> virQEMUCapsClear(qemuCaps, QEMU_CAPS_NO_ACPI);
> + }
Some day maybe we'll be able to stop parsing the help output. Still
makes me wonder is AARCH64 even support on those older versions and thus
is this necessary? IDC either way as I suppose this is preventative or
"more complete".
>
> /* virQEMUCapsExtractDeviceStr will only set additional caps if qemu
> * understands the 0.13.0+ notion of "-device driver,". */
> @@ -4222,9 +4231,14 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
> goto cleanup;
> }
>
> - /* ACPI/HPET/KVM PIT are x86 specific */
> - if (ARCH_IS_X86(qemuCaps->arch)) {
> + /* ACPI only works on x86 and aarch64 */
> + if (ARCH_IS_X86(qemuCaps->arch) ||
> + qemuCaps->arch == VIR_ARCH_AARCH64) {
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
> + }
> +
> + /* HPET and KVM PIT are x86 specific */
> + if (ARCH_IS_X86(qemuCaps->arch)) {
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET);
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
> }
Considering on what's coming in patch 2, this would be better as a
virQEMUCapsSetFirmwareCaps? "utility" function... That way the added
comments in both places referencing the other place could be dropped.
John
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> index 0aed651..b64a6f8 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml
> @@ -40,6 +40,7 @@
> <flag name='no-shutdown'/>
> <flag name='cache-unsafe'/>
> <flag name='ich9-ahci'/>
> + <flag name='no-acpi'/>
> <flag name='fsdev-readonly'/>
> <flag name='virtio-blk-pci.scsi'/>
> <flag name='drive-copy-on-read'/>
> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> index 1041a12..46e368f 100644
> --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml
> @@ -40,6 +40,7 @@
> <flag name='no-shutdown'/>
> <flag name='cache-unsafe'/>
> <flag name='ich9-ahci'/>
> + <flag name='no-acpi'/>
> <flag name='fsdev-readonly'/>
> <flag name='virtio-blk-pci.scsi'/>
> <flag name='drive-copy-on-read'/>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2017-03-27 at 08:03 -0400, John Ferlan wrote:
[...]
> > + /* Older QEMU versions reported -no-acpi in the output of -help even
> > + * though it was not supported by the architecture. The issue has since
> > + * been fixed, but to maintain compatibility with all release we still
>
> "releases"
Good catch!
> > + * need to filter out the capability for architectures that we know
> > + * don't support the feature, eg. anything but x86 and aarch64 */
> > + if (!ARCH_IS_X86(qemuCaps->arch) &&
> > + qemuCaps->arch != VIR_ARCH_AARCH64) {
> > virQEMUCapsClear(qemuCaps, QEMU_CAPS_NO_ACPI);
> > + }
>
> Some day maybe we'll be able to stop parsing the help output. Still
> makes me wonder is AARCH64 even support on those older versions and thus
> is this necessary? IDC either way as I suppose this is preventative or
> "more complete".
Probably not, but I don't think having different
arch-specific handling in the two code paths is a good
idea: we should stay consistent, if anything not to confuse
our future selves :)
[...]
> > @@ -4222,9 +4231,14 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
> > goto cleanup;
> > }
> >
> > - /* ACPI/HPET/KVM PIT are x86 specific */
> > - if (ARCH_IS_X86(qemuCaps->arch)) {
> > + /* ACPI only works on x86 and aarch64 */
> > + if (ARCH_IS_X86(qemuCaps->arch) ||
> > + qemuCaps->arch == VIR_ARCH_AARCH64) {
> > virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
> > + }
> > +
> > + /* HPET and KVM PIT are x86 specific */
> > + if (ARCH_IS_X86(qemuCaps->arch)) {
> > virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET);
> > virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
> > }
>
> Considering on what's coming in patch 2, this would be better as a
> virQEMUCapsSetFirmwareCaps? "utility" function... That way the added
> comments in both places referencing the other place could be dropped.
HPET and KVM PIT are not firmware-related, though.
How about I move setting the arch based on the monitor to
a separate virQEMUCapsInitQMPArch() and leave only setting
the actual arch-dependent capabilities in this function?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2017 09:57 AM, Andrea Bolognani wrote:
> On Mon, 2017-03-27 at 08:03 -0400, John Ferlan wrote:
> [...]
>>> + /* Older QEMU versions reported -no-acpi in the output of -help even
>>> + * though it was not supported by the architecture. The issue has since
>>> + * been fixed, but to maintain compatibility with all release we still
>>
>> "releases"
>
> Good catch!
>
>>> + * need to filter out the capability for architectures that we know
>>> + * don't support the feature, eg. anything but x86 and aarch64 */
>>> + if (!ARCH_IS_X86(qemuCaps->arch) &&
>>> + qemuCaps->arch != VIR_ARCH_AARCH64) {
>>> virQEMUCapsClear(qemuCaps, QEMU_CAPS_NO_ACPI);
>>> + }
>>
>> Some day maybe we'll be able to stop parsing the help output. Still
>> makes me wonder is AARCH64 even support on those older versions and thus
>> is this necessary? IDC either way as I suppose this is preventative or
>> "more complete".
>
> Probably not, but I don't think having different
> arch-specific handling in the two code paths is a good
> idea: we should stay consistent, if anything not to confuse
> our future selves :)
>
> [...]
That's fine...
>>> @@ -4222,9 +4231,14 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
>>> goto cleanup;
>>> }
>>>
>>> - /* ACPI/HPET/KVM PIT are x86 specific */
>>> - if (ARCH_IS_X86(qemuCaps->arch)) {
>>> + /* ACPI only works on x86 and aarch64 */
>>> + if (ARCH_IS_X86(qemuCaps->arch) ||
>>> + qemuCaps->arch == VIR_ARCH_AARCH64) {
>>> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_ACPI);
>>> + }
>>> +
>>> + /* HPET and KVM PIT are x86 specific */
>>> + if (ARCH_IS_X86(qemuCaps->arch)) {
>>> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_HPET);
>>> virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
>>> }
>>
>> Considering on what's coming in patch 2, this would be better as a
>> virQEMUCapsSetFirmwareCaps? "utility" function... That way the added
>> comments in both places referencing the other place could be dropped.
>
> HPET and KVM PIT are not firmware-related, though.
>
> How about I move setting the arch based on the monitor to
> a separate virQEMUCapsInitQMPArch() and leave only setting
> the actual arch-dependent capabilities in this function?
>
I think if "all" the lines were in a single API it would reduce the
chance that some future self would have to have to (or be told to) keep
this in sync with testUpdateQEMUCaps.
While PIT (Programmable? Interval Timer) and HPET (High Precision Event
Timer) aren't necessarily "firmware" type things they got lumped
together with the APCI and well are at least "related" to a degree...
One gsearch lands on http://wiki.osdev.org/HPET
Maybe "Firmware" is a poor selection of names on my part, but it was as
close as I could get.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2017-03-27 at 10:26 -0400, John Ferlan wrote: [...] > > > Considering on what's coming in patch 2, this would be better as a > > > virQEMUCapsSetFirmwareCaps? "utility" function... That way the added > > > comments in both places referencing the other place could be dropped. > > > > HPET and KVM PIT are not firmware-related, though. > > > > How about I move setting the arch based on the monitor to > > a separate virQEMUCapsInitQMPArch() and leave only setting > > the actual arch-dependent capabilities in this function? > > I think if "all" the lines were in a single API it would reduce the > chance that some future self would have to have to (or be told to) keep > this in sync with testUpdateQEMUCaps. Sorry, maybe I was not clear enough: I like your idea of moving those to a separate function and calling that function from the test suite instead of duplicating code! The only thing I'm questioning is the name. The attached patch should give you an idea of the direction I'm heading: virQEMUCapsInitQMPArch() would only be called from the library code, while virQEMUCapsInitArchQMPBasic() would be called both there and in the test suite. Does that look reasonable? -- Andrea Bolognani / Red Hat / Virtualization-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/27/2017 10:39 AM, Andrea Bolognani wrote: > On Mon, 2017-03-27 at 10:26 -0400, John Ferlan wrote: > [...] >>>> Considering on what's coming in patch 2, this would be better as a >>>> virQEMUCapsSetFirmwareCaps? "utility" function... That way the added >>>> comments in both places referencing the other place could be dropped. >>> >>> HPET and KVM PIT are not firmware-related, though. >>> >>> How about I move setting the arch based on the monitor to >>> a separate virQEMUCapsInitQMPArch() and leave only setting >>> the actual arch-dependent capabilities in this function? >> >> I think if "all" the lines were in a single API it would reduce the >> chance that some future self would have to have to (or be told to) keep >> this in sync with testUpdateQEMUCaps. > > Sorry, maybe I was not clear enough: I like your idea > of moving those to a separate function and calling that > function from the test suite instead of duplicating code! > The only thing I'm questioning is the name. Name I provided was just an "example"... > > The attached patch should give you an idea of the direction > I'm heading: virQEMUCapsInitQMPArch() would only be called > from the library code, while virQEMUCapsInitArchQMPBasic() > would be called both there and in the test suite. > > Does that look reasonable? > Sure... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.