qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting

The "<apic/>" feature, although it's not available for pseries, can be declared in the domain XML of ppc64 guests without errors. But setting its 'eoi' attribute will break QEMU. For "<apic eoi='on'/>": qemu-kvm: Expected key=value format, found +kvm_pv_eoi A similar error happens with eoi='off'.

case VIR_DOMAIN_FEATURE_APIC: + /* the <apic/> declaration is harmless for ppc64, but + * its 'eoi' attribute isn't. To detect if 'eoi' was + * present in the XML we can check the tristate switch + * of def->apic_eoi */ + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT && + ARCH_IS_PPC64(def->os.arch)) {

On 3/23/20 2:01 PM, Andrea Bolognani wrote: > This is > > https://bugzilla.redhat.com/show_bug.cgi?id=1236440 > > Please include the Bugzilla URL for other patches in the series > as well, if applicable. Sure. I didn't include the link because it can't be opened unless you have a RH bugzilla account and I wasn't sure I could add it here.

And while we're at it, something that just occurred to me, I'll also gate the ppc64 only capabilities as well in a new patch.

On 3/23/20 2:45 PM, Andrea Bolognani wrote: > On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote: > > And while we're at it, something that just occurred to me, I'll also gate the ppc64 > > only capabilities as well in a new patch. > > Yeah, that makes sense too. We're probably never going to get to a > point where these checks are completely accurate, but any improvement > can only be a welcome one :) Just realized that we're already doing that here: case VIR_DOMAIN_FEATURE_HPT: case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: case VIR_DOMAIN_FEATURE_CCF_ASSIST: if (qemuDomainDefValidatePSeriesFeature(def, qemuCaps, i) < 0) return -1; break;

There is no need for an extra patch to handle it.

+ case VIR_DOMAIN_FEATURE_PVSPINLOCK: + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + ARCH_IS_PPC64(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;

The Perfomance Monitoring Unit (PMU) feature is not available for the Power architecture. The "<pmu/>" feature will always have a value 'on' or 'off' after saving the domain XML, and both will be rejected by QEMU when launching. This is the error message for "<pmu state='on'/>": qemu-kvm: can't apply global host-powerpc64-cpu.pmu=on: Property '.pmu' not found A similar error message is thrown for "<pmu state='off'/>". This patch prevents the pseries guest from launching with any pmu setting with a more informative error message: error: unsupported configuration: The 'pmu' feature is not supported for architecture 'ppc64' or machine type 'pseries'

On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote: > The Perfomance Monitoring Unit (PMU) feature is not available for > the Power architecture. The "<pmu/>" feature will always have a value > 'on' or 'off' after saving the domain XML, and both will be rejected > by QEMU when launching. This is the error message for > "<pmu state='on'/>": > > qemu-kvm: can't apply global host-powerpc64-cpu.pmu=on: Property '.pmu' not found > > A similar error message is thrown for "<pmu state='off'/>". > > This patch prevents the pseries guest from launching with any > pmu setting with a more informative error message: > > error: unsupported configuration: The 'pmu' feature is not > supported for architecture 'ppc64' or machine type 'pseries' I don't think this is right. While you are correct that PMU can't be configured for pSeries guests, I think that's because of the opposite reason: it's always on, and can't be turned off. For comparison's sake: in an x86 guest with <pmu state='on'/> I get $ perf list | grep -E 'Hardware.*event' branch-instructions OR branches [Hardware event] branch-misses [Hardware event] bus-cycles [Hardware event] cache-misses [Hardware event] cache-references [Hardware event] cpu-cycles OR cycles [Hardware event] instructions [Hardware event] ref-cycles [Hardware event] L1-dcache-load-misses [Hardware cache event] L1-dcache-loads [Hardware cache event] L1-dcache-stores [Hardware cache event] L1-icache-load-misses [Hardware cache event] branch-load-misses [Hardware cache event] branch-loads [Hardware cache event] dTLB-load-misses [Hardware cache event] dTLB-loads [Hardware cache event] dTLB-store-misses [Hardware cache event] dTLB-stores [Hardware cache event] iTLB-load-misses [Hardware cache event] iTLB-loads [Hardware cache event] whereas when I turn off PMU all of those are gone. In a pSeries guest running on POWER8, without any configuration, I get: $ perf list | grep -E 'Hardware.*event' branch-instructions OR branches [Hardware event] branch-misses [Hardware event] cache-misses [Hardware event] cache-references [Hardware event] cpu-cycles OR cycles [Hardware event] instructions [Hardware event] stalled-cycles-backend OR idle-cycles-backend [Hardware event] stalled-cycles-frontend OR idle-cycles-frontend [Hardware event] L1-dcache-load-misses [Hardware cache event] L1-dcache-loads [Hardware cache event] L1-dcache-prefetches [Hardware cache event] L1-dcache-store-misses [Hardware cache event] L1-icache-load-misses [Hardware cache event] L1-icache-loads [Hardware cache event] L1-icache-prefetches [Hardware cache event] LLC-load-misses [Hardware cache event] LLC-loads [Hardware cache event] LLC-prefetches [Hardware cache event] LLC-store-misses [Hardware cache event] LLC-stores [Hardware cache event] branch-load-misses [Hardware cache event] branch-loads [Hardware cache event] dTLB-load-misses [Hardware cache event] iTLB-load-misses [Hardware cache event] So it seems to me that, if anything, the PMU feature should be treated like the <panic/> device, that is, automatically added to pSeries guests if it's not present already. David, what's your opinion on the matter?

Andrea, I see that the behavior of the 'panic' device in the ppc64 XML is to always add a: <panic model='pseries'/> Even if I remove it Libvirt will automatically added it back again. How do you suggest we handle this PMU? We can copy what 'panic' is doing, adding a <pmu model='peries'/>

In the XML just like 'panic' does. Or we can add the 'pmu' feature: <feature> <pmu/> </feature> PMU can't be left without a state ATM, thus this would translate to: <feature> <pmu state='on'/> </feature>

This would require to add the whole <feature> element if none is present as well. Both requires some code tweaking here and there. I am not sure which one is better, although the first option would require us to handle the <pmu> XML feature anyway since we can't have both, so there's that. For the sake of completeness, I'll also mention that we can simply allow <pmu/> to be declared in the XML, handling the <pmu state='on'/> inside the QEMU driver to not add the bogus '.pmu' parameter for QEMU ppc64, forbid <pmu state='off'/> to be declared, and nothing else. No auto-generation of XML indicating that the guest will support a PMU.

On 3/24/20 11:00 AM, Andrea Bolognani wrote: > On Tue, 2020-03-24 at 09:27 -0300, Daniel Henrique Barboza wrote: > > For the sake of completeness, I'll also mention that we can simply allow <pmu/> to be > > declared in the XML, handling the <pmu state='on'/> inside the QEMU driver to not add the > > bogus '.pmu' parameter for QEMU ppc64, forbid <pmu state='off'/> to be declared, and > > nothing else. No auto-generation of XML indicating that the guest will support a PMU. > > Looking again at how other architectures, specifically x86 and ARM, > handle this, the PMU is generally enabled by default without this > fact being reflected in the domain XML; the user can then go ahead > and specifically ask for it to be turned on or off, at which point > libvirt will add the relevant bits to the QEMU command line. > > This is basically the second behavior you're describing above, and > I think it would be perfectly fine if that's the one we would adopt > for ppc64. I guess I'll roll with this one then. I will allow <pmu state='on'/> to be declared in the XML without breaking QEMU. For <pmu state='off'/> I'll throw an CONFIG_UNSUPPORTED error mentioning that PMU can't be turned off for ppc64.

+ case VIR_DOMAIN_FEATURE_HYPERV: + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && + ARCH_IS_PPC64(def->os.arch)) { + 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;