Re: [libvirt] [PATCH 1/4] qemu: Advertise ACPI support for aarch64 guests

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?