On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote:

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

s/qemu_domain.c/qemu/ Same for the other patches in the series.

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'.

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. [...]

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)) {

Since the underlying kvm_pv_eoi feature is x86-only, you should change this last part to !ARCH_IS_X86(def->os.arch) and benefit ARM, RISC-V and s390x users at the same time. When you do so, please reduce the comment to something like /* The kvm_pv_eoi feature is x86-only */ -- Andrea Bolognani / Red Hat / Virtualization

On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote:

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.

Oh, I did not realize that bug was private. That's very silly. I'll ask for it to be made public.

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 :) -- Andrea Bolognani / Red Hat / Virtualization

On Tue, 2020-03-24 at 10:22 -0300, Daniel Henrique Barboza wrote:

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;

Right! I had forgotten about writing that validation logic O:-)

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

Nope, we're good :) -- Andrea Bolognani / Red Hat / Virtualization

On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote: [...]

+ 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;

Same comment as the previous patch: the feature is x86-only, so there's no point in only preventing its use on ppc64. -- Andrea Bolognani / Red Hat / Virtualization

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 Bolognani / Red Hat / Virtualization

On Mon, Mar 23, 2020 at 06:28:34PM +0100, Andrea Bolognani wrote:

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?

You're correct. The difference originates at the hardware level. On x86 the host always "owns" the PMU, and it requires explicit work in KVM to make it available to the guest. On POWER, the guest owns the PMU. I'm not sure if it's possible to disable guest access to the PMU at all. Even if it is, it must be some obscure bit in the HFCR or somewhere which I don't believe we've wired up at all. So for now, certainly, pmu is effectively always on. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

On Tue, 2020-03-24 at 09:27 -0300, Daniel Henrique Barboza wrote:

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'/>

This is definitely not the right approach - we should use the existing interface rather than try to come up with a new one.

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 is what we should do.

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.

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. -- Andrea Bolognani / Red Hat / Virtualization

On Tue, 2020-03-24 at 11:05 -0300, Daniel Henrique Barboza wrote:

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.

Sounds good. -- Andrea Bolognani / Red Hat / Virtualization

On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote: [...]

+ 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;

Same as the previous patches, except Hyper-V is reasonable not only on x86 but also on aarch64, so the architecture check should be tweaked accordingly. -- Andrea Bolognani / Red Hat / Virtualization