The "<apic/>" feature, although it's available only for x86 guests, can be declared in the domain XML of other archs without errors. But setting its 'eoi' attribute will break QEMU. For "<apic eoi='on'/>", in a ppc64 guest: qemu-kvm: Expected key=value format, found +kvm_pv_eoi A similar error happens with eoi='off'. One can argue that it's better to simply forbid launching non-x86 guests with "<apic/>" declared in the XML - it is a feature that the architecture doesn't support and this would make it clearer about it. This is sensible, but there are non-x86 guests that are running with "<apic/>" declared in the domain (and A LOT of guests running with "<acpi/>" for that matter, probably reminiscent of x86 templates that were reused for other archs) that will stop working if we go this route. A more subtle approach is to detect if the 'eoi' element is being set for non-x86 guests and warn the user about it with a better error message than the one QEMU provides. This is the new error message when any value is set for the 'eoi' element in a ppc64 XML: error: unsupported configuration: The 'eoi' attribute of the 'apic' feature is not supported for architecture 'ppc64' or machine type 'pseries'. Suggested-by: Andrea Bolognani <abologna@redhat.com> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1236440 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2c9fb47d17..73a65128c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5255,8 +5255,23 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, } break; - case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_APIC: + /* The kvm_pv_eoi feature is x86-only. */ + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The 'eoi' attribute of the '%s' feature " + "is not supported for architecture '%s' or " + "machine type '%s'"), + featureName, + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: -- 2.25.1

The 'pvspinlock' feature is x86 only. The "<pvspinlock/>" declaration will always have a value 'on' or 'off', and both will break QEMU when launching non-x86 guests. This is the error message for "<pvspinlock state='on'/>" when running a ppc64 guest: qemu-kvm: Expected key=value format, found +kvm_pv_unhalt A similar error message is thrown for "<pvspinlock state='off'/>". This patch prevents non-x86 guests from launching with any pvspinlock setting with a more informative error message: error: unsupported configuration: The 'pvspinlock' feature is not supported for architecture 'ppc64' or machine type 'pseries' Suggested-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 73a65128c6..c56b3a893b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5271,13 +5271,25 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, } break; + case VIR_DOMAIN_FEATURE_PVSPINLOCK: + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is not supported for " + "architecture '%s' or machine type '%s'"), + featureName, + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: case VIR_DOMAIN_FEATURE_PRIVNET: case VIR_DOMAIN_FEATURE_HYPERV: - case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_MSRS: -- 2.25.1

Hyperv features are supported by both x86 and aarch64. The <hyperv/> declaration in the XML by itself is benign to other architectures, but any of its 14 current features will break QEMU with an error like this (from ppc64): qemu-kvm: Expected key=value format, found hv_relaxed This is a more extreme case than the one for apic eoi because we would need an extra 'switch' statement, with all current Hyperv features in the body of qemuDomainDefValidateFeatures(), to check if the user attempted to activate any of them. It's easier to simply fail to launch with any 'hyperv' declaration in the XML for every arch which is not x86 and aarch64. A fair disclaimer about Windows and PowerPC: the last Windows version that ran in the architecture is the hall of famer Windows NT 4.0, launched in 1996 and with end of extended support for the Server version in 2004 [1]. I am acknowledging that there might be Windows NT 4.0 users running in PowerPC, but not enough people running it under KVM/QEMU to justify Libvirt allowing 'hyperv' to exist in the domain XML of ppc64 domains. [1] https://en.wikipedia.org/wiki/Windows_NT_4.0 Suggested-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c56b3a893b..b03e7bbb30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5284,12 +5284,23 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, } break; + case VIR_DOMAIN_FEATURE_HYPERV: + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + !ARCH_IS_X86(def->os.arch) && !qemuDomainIsARMVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Hyperv features are not supported for " + "architecture '%s' or machine type '%s'"), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: case VIR_DOMAIN_FEATURE_PRIVNET: - case VIR_DOMAIN_FEATURE_HYPERV: case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_MSRS: -- 2.25.1

The PMU feature is enabled by default in ppc64 guests and can't be disabled via Libvirt or QEMU [1]. The current PMU feature implementation does not allow PMU to enabled or disabled in the ppc64 guest. Declaring the PMU feature will make the 'pmu' property to be passed on to QEMU, but this property isn't available for ppc64: qemu-kvm: can't apply global host-powerpc64-cpu.pmu=on: Property '.pmu' not found A similar error is thrown when trying to disable the PMU. This patch standardizes the PMU handling for ppc64 guests by: - throwing an error if the user attempts to set the feature to 'off', given that this feature can't be turned off at all; - allowing the feature to be declared as 'on' in the domain XML. This is done by skipping ppc64 guests when creating the command line for this feature. [1] https://www.redhat.com/archives/libvir-list/2020-March/msg00874.html Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_command.c | 4 +++- src/qemu/qemu_domain.c | 11 ++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a772fa3f3..d1b689dfd3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6784,7 +6784,9 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, } } - if (def->features[VIR_DOMAIN_FEATURE_PMU]) { + /* ppc64 guests always have PMU enabled, but the 'pmu' option + * is not supported. */ + if (def->features[VIR_DOMAIN_FEATURE_PMU] && !ARCH_IS_PPC64(def->os.arch)) { virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU]; virBufferAsprintf(&buf, ",pmu=%s", virTristateSwitchTypeToString(pmu)); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b03e7bbb30..7d29f3f114 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5296,13 +5296,22 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, } break; + case VIR_DOMAIN_FEATURE_PMU: + if (def->features[i] == VIR_TRISTATE_SWITCH_OFF && + ARCH_IS_PPC64(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("PMU is always enabled for architecture '%s'"), + virArchToString(def->os.arch)); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: case VIR_DOMAIN_FEATURE_PRIVNET: case VIR_DOMAIN_FEATURE_CAPABILITIES: - case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_MSRS: case VIR_DOMAIN_FEATURE_LAST: break; -- 2.25.1