Re: [PATCH v7 3/4] KVM: arm64: PMU: Introduce FIXED_COUNTERS_ONLY

[Marc Zyngier <maz@kernel.org>]

On Mon, 20 Apr 2026 09:36:16 +0100,
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
> 
> On 2026/04/20 2:19, Marc Zyngier wrote:
> > On Sat, 18 Apr 2026 09:14:25 +0100,
> > Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
> >> 
> >> On a heterogeneous arm64 system, KVM's PMU emulation is based on the
> >> features of a single host PMU instance. When a vCPU is migrated to a
> >> pCPU with an incompatible PMU, counters such as PMCCNTR_EL0 stop
> >> incrementing.
> >> 
> >> Although this behavior is permitted by the architecture, Windows does
> >> not handle it gracefully and may crash with a division-by-zero error.
> >> 
> >> The current workaround requires VMMs to pin vCPUs to a set of pCPUs
> >> that share a compatible PMU. This is difficult to implement correctly in
> >> QEMU/libvirt, where pinning occurs after vCPU initialization, and it
> >> also restricts the guest to a subset of available pCPUs.
> >> 
> >> Introduce the KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY attribute to
> >> create a "fixed-counters-only" PMU. When set, KVM exposes a PMU that is
> >> compatible with all pCPUs but that does not support programmable
> >> event counters which may have different feature sets on different PMUs.
> >> 
> >> This allows Windows guests to run reliably on heterogeneous systems
> >> without crashing, even without vCPU pinning, and enables VMMs to
> >> schedule vCPUs across all available pCPUs, making full use of the host
> >> hardware.
> >> 
> >> Much like KVM_ARM_VCPU_PMU_V3_IRQ and other read-write attributes, this
> >> attribute provides a getter that facilitates kernel and userspace
> >> debugging/testing.
> > 
> > OK, so that's the sales pitch. But how is it implemented? I would like
> > to be able to read a high-level description of the implementation
> > trade-offs.
> 
> Implementation-wise it is very trivial. Essentially the following
> addition in kvm_arm_pmu_v3_get_attr() is the entire implementation:
> +	case KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY:
> +		if (test_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY,
> &vcpu->kvm->arch.flags))
> +			return 0;
> 
> Both its functionality and code complexity is trivial. So we can argue that:
> - the functionality is too trivial to be useful or
> - the interface/implementation complexity is so trivial that it does not
>   incur maintenance burden
> 
> In this case the selftest uses the getter so I was more inclined to
> have it, but adding one just for the selftest sounds too ad-hoc, so
> here I looked into other attributes to ensure that it was not
> introducing inconsistency with existing interfaces.
> 
> As the result, I found there are other read-write attributes; in fact
> there are more read-write attributes than write-only ones.

You're completely missing the point. I'm referring to the whole of the
commit message, which is more of a marketing slide than a technical
description.

I really don't care about the getter at this stage, which while
pointless, does not make things more awful than they already are.

> 
> > 
> >> 
> >> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> >> ---
> >>   Documentation/virt/kvm/devices/vcpu.rst |  29 ++++++
> >>   arch/arm64/include/asm/kvm_host.h       |   2 +
> >>   arch/arm64/include/uapi/asm/kvm.h       |   1 +
> >>   arch/arm64/kvm/arm.c                    |   1 +
> >>   arch/arm64/kvm/pmu-emul.c               | 155 +++++++++++++++++++++++---------
> >>   include/kvm/arm_pmu.h                   |   2 +
> >>   6 files changed, 147 insertions(+), 43 deletions(-)
> >> 
> >> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> >> index 60bf205cb373..e0aeb1897d77 100644
> >> --- a/Documentation/virt/kvm/devices/vcpu.rst
> >> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> >> @@ -161,6 +161,35 @@ explicitly selected, or the number of counters is out of range for the
> >>   selected PMU. Selecting a new PMU cancels the effect of setting this
> >>   attribute.
> >>   +1.6 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY
> >> +------------------------------------------------------
> >> +
> >> +:Parameters: no additional parameter in kvm_device_attr.addr
> >> +
> >> +:Returns:
> >> +
> >> +	 =======  =====================================================
> >> +	 -EBUSY   Attempted to set after initializing PMUv3 or running
> >> +		  VCPU, or attempted to set for the first time after
> >> +		  setting an event filter
> >> +	 -ENXIO   Attempted to get before setting
> >> +	 -ENODEV  Attempted to set while PMUv3 not supported
> >> +	 =======  =====================================================
> >> +
> >> +If set, PMUv3 will be emulated without programmable event counters. The VCPU
> >> +will use any compatible hardware PMU. This attribute is particularly useful on
> > 
> > Not quite "any PMU". It will use *the* PMU of the physical CPU,
> > irrespective of the implementation.
> 
> I think:
> 
> - this comment
> - one on the KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED note
> - one on kvm_pmu_create_perf_event()
> - and one on kvm_arm_pmu_v3_set_pmu_fixed_counters_only()
> 
> All boil down into one question: will it support all possible CPUs, or
> will it support a subset? Let me answer here:
> 
> This patch is written to support a subset instead of all possible
> CPUs. If a pCPU does not have a compatible PMU, the pCPU will not be
> supported and cause KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED.

This is not a thing. Either *all* the CPUs have a PMU that can be used
for KVM, or PMU support is not offered to guests. That's a hard line
in the sand. And the code already upholds this by checking the
sanitised PMUVer field.

>
> This patch does not enforce all possible CPUs are covered by the
> compatible PMUs. Theoretically speaking,
> kvm_arm_pmu_get_pmuver_limit() enables the PMU emulation when real
> PMUv3 hardware covers all possible CPUs *or* the relevant registers
> can be trapped with IMPDEF, so some pCPU may not have a compatible PMU
> and only provide the IMPDEF trapping.

How is that possible? Please describe the case where that can happen,
and I will make sure that such a system stops booting. The intent is
definitely that that:

- for early CPUs, we take the minimal capability of all CPUs

- for late CPUs, either they match at least the capability recorded by
  early CPUs, or they don't boot.

> Practically, I don't think any sane configuration will ever have such
> a subset support, so we can explicitly enforce all possible CPUs are
> covered by the compatible PMUs if desired.

That's not just desired. This is a requirement. And it is already
enforced AFAICS.

> 
> > 
> >> +heterogeneous systems where different hardware PMUs cover different physical
> >> +CPUs. The compatibility of hardware PMUs can be checked with
> >> +KVM_ARM_VCPU_PMU_V3_SET_PMU. All VCPUs in a VM share this attribute. It isn't
> >> +possible to set it for the first time if a PMU event filter is already present.
> > 
> > "for the first time" gives the impression that it will work if you try
> > again. I'd rather we say that "This feature is incompatible with the
> > existence of a PMU event filter".
> 
> The following sequence will work:
> 1. Set KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY
> 2. Set KVM_ARM_VCPU_PMU_V3_FILTER
> 3. Set KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY
> 
> This is to make the behavior conistent with KVM_ARM_VCPU_PMU_V3_SET_PMU.

I don't think this is correct. Filtering is completely at odds with
this patch, and I don't want to have to reason about the combination.

[...]

> >> +	int i;
> >> +
> >> +	for_each_set_bit(i, &mask, 32) {
> >> +		pmc = kvm_vcpu_idx_to_pmc(vcpu, i);
> >> +		if (!pmc->perf_event)
> >> +			continue;
> >> +
> >> +		cpu_pmu = to_arm_pmu(pmc->perf_event->pmu);
> >> +		if (!cpumask_test_cpu(vcpu->cpu, &cpu_pmu->supported_cpus)) {
> >> +			kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> >> +			break;
> >> +		}
> >> +	}
> >> +}
> >> +
> > 
> > Why do we need to inflict this on VMs that do not have the fixed
> > counter restriction?
> 
> This function is to re-create the perf_event in case the current
> perf_event does not support the pCPU because e.g., the pCPU is a
> E-core while the perf_event only covers the P-cores.

That's not what I meant. This code is only here to support the
fixed-function feature. It makes no sense outside of it, because *we
don't support counter migration across implementations*.

So what's the purpose of this stuff for the normal KVM setup?

> 
> > 
> > And even then, all you have to reconfigure is the cycle counter. So
> > why the loop? All we want to find out is whether the cycle counter is
> > instantiated on the PMU that matches the current CPU.
> 
> I just wanted to avoid hardcoding assumptions on the fixed
> counter(s). FEAT_PMUv3_ICNTR will be naturaly handled with a loop, for
> example.

Well, not that loop, since ICNTR is counter 32. So please let's stop
the nonsense and only add what is required?

[...]

> >>   +
> >> clear_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY,
> >> &kvm->arch.flags);
> > 
> > Why does this need to be cleared? I'd rather we make sure it is never
> > set the first place.
> 
> KVM_ARM_VCPU_PMU_V3_SET_PMU and
> KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY can be set on the same
> VCPU. The last KVM_ARM_VCPU_PMU_V3_SET_PMU or
> KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY setting will be effective.
> 
> A VMM may try set these attributes to check if the setting is
> supported. For example, the RFC QEMU patch first uses
> KVM_ARM_VCPU_PMU_V3_SET_PMU to find a compatible PMU that covers all
> pCPUs, and then falls back to
> KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY. The order of such probing is
> up to the VMM.

KVM_ARM_VCPU_PMU_V3_SET_PMU is not a probing mechanism. You must probe
the PMUs by looking in /sys/bus/event_source/devices/, like kvmtool
does.

So there is no reason to support this stuff, and the two flags should
be made mutually exclusive.

[...]

> >>
> > 
> > In conclusion, I find this patch to be rather messy. For a start, it
> > needs to be split in at least 5 patches:
> > 
> > - at least two for the refactoring
> > - one for the PMU core changes
> > - one for the UAPI
> > - one for documentation
> 
> That clarifies the expected granurarity of patches. The next version
> will be in that layout, perhaps with more patches if an additional
> change. Thanks for the guidance.
> 
> > 
> > I'd also like some clarification on how this is intended to work if we
> > enable FEAT_PMUv3_ICNTR, because the definition seems to be designed
> > to encompass all fixed-function counters, and I expect this to grow
> > over time.
> 
> Indeed the UAPI was designed to encompass all fixed-function counters
> as suggested by Oliver.
> 
> To support the UAPI, the implementation avoids hardcoding the
> assumption on the fixed counter(s). FEAT_PMUv3_INCTR will be naturaly
> supported once the common code is properly updated (i.e., the size of
> the event counter bitmask is grown the corresponding registers are
> wired up with a proper check of the feature.)
> 
> I expect migration will be handled with the conventional register
> getters and setters, but please share if you have a concern.

At the very least I want to see some documentation explaining that.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.