[PATCH 0/2] KVM: arm64: EL2 PMU reset handling fixes

[PATCH 0/2] KVM: arm64: EL2 PMU reset handling fixes

[Marc Zyngier]

Joey reports that some of his PMU tests do not behave quite as
expected:

- MDCR_EL2.HPMN is set to 0 out of reset

- PMCR_EL0.P should reset all the counters when written from EL2

These couple of patches attempt to remedy the situation. Note that
they have only been compile-tested.

Marc Zyngier (2):
  KVM: arm64: Fix MDCR_EL2.HPMN reset value
  KVM: arm64: Contextualise the handling of PMCR_EL0.P writes

 arch/arm64/kvm/pmu-emul.c | 21 ++++++++++++++++-----
 arch/arm64/kvm/sys_regs.c |  7 ++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.39.2

[PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Marc Zyngier]

The MDCR_EL2 documentation indicates that the HPMN field has
the following behaviour:

"On a Warm reset, this field resets to the expression NUM_PMU_COUNTERS."

However, it appears we reset it to zero, which is not very useful.

Add a reset helper for MDCR_EL2, and handle the case where userspace
changes the target PMU, which may force us to change HPMN again.

Reported-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++
 arch/arm64/kvm/sys_regs.c |  7 ++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 6c5950b9ceac8..5a71c3744c4d7 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -1007,6 +1007,19 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
 
 	kvm->arch.arm_pmu = arm_pmu;
 	kvm->arch.pmcr_n = kvm_arm_pmu_get_max_counters(kvm);
+
+	/* Reset MDCR_EL2.HPMN behind the vcpus' back... */
+	if (test_bit(KVM_ARM_VCPU_HAS_EL2, kvm->arch.vcpu_features)) {
+		struct kvm_vcpu *vcpu;
+		unsigned long i;
+
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			u64 val = __vcpu_sys_reg(vcpu, MDCR_EL2);
+			val &= ~MDCR_EL2_HPMN;
+			val |= FIELD_PREP(MDCR_EL2_HPMN, kvm->arch.pmcr_n);
+			__vcpu_sys_reg(vcpu, MDCR_EL2) = val;
+		}
+	}
 }
 
 /**
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 82430c1e1dd02..380f22f19cb42 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2493,6 +2493,11 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static u64 reset_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	__vcpu_sys_reg(vcpu, r->reg) = vcpu->kvm->arch.pmcr_n;
+	return vcpu->kvm->arch.pmcr_n;
+}
 
 /*
  * Architected system registers.
@@ -3034,7 +3039,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(SCTLR_EL2, access_rw, reset_val, SCTLR_EL2_RES1),
 	EL2_REG(ACTLR_EL2, access_rw, reset_val, 0),
 	EL2_REG_VNCR(HCR_EL2, reset_hcr, 0),
-	EL2_REG(MDCR_EL2, access_mdcr, reset_val, 0),
+	EL2_REG(MDCR_EL2, access_mdcr, reset_mdcr, 0),
 	EL2_REG(CPTR_EL2, access_rw, reset_val, CPTR_NVHE_EL2_RES1),
 	EL2_REG_VNCR(HSTR_EL2, reset_val, 0),
 	EL2_REG_VNCR(HFGRTR_EL2, reset_val, 0),
-- 
2.39.2

[PATCH 2/2] KVM: arm64: Contextualise the handling of PMCR_EL0.P writes

[Marc Zyngier]

Contrary to what the comment says in kvm_pmu_handle_pmcr(),
writing PMCR_EL0.P==1 has the following effects:

<quote>
The event counters affected by this field are:
  * All event counters in the first range.
  * If any of the following are true, all event counters in the second
    range:
    - EL2 is disabled or not implemented in the current Security state.
    - The PE is executing at EL2 or EL3.
</quote>

where the "first range" represent the counters in the [0..HPMN-1]
range, and the "second range" the counters in the [HPMN..MAX] range.

It so appears that writing P from EL2 should nuke all counters,
and not just the "guest" view. Just do that, and nuke the misleading
comment.

Reported-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 5a71c3744c4d7..636cae89310cd 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -617,14 +617,12 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
 
 	if (val & ARMV8_PMU_PMCR_P) {
-		/*
-		 * Unlike other PMU sysregs, the controls in PMCR_EL0 always apply
-		 * to the 'guest' range of counters and never the 'hyp' range.
-		 */
 		unsigned long mask = kvm_pmu_implemented_counter_mask(vcpu) &
-				     ~kvm_pmu_hyp_counter_mask(vcpu) &
 				     ~BIT(ARMV8_PMU_CYCLE_IDX);
 
+		if (!vcpu_is_el2(vcpu))
+			mask &= ~kvm_pmu_hyp_counter_mask(vcpu);
+
 		for_each_set_bit(i, &mask, 32)
 			kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, i), 0, true);
 	}
-- 
2.39.2

Re: [PATCH 2/2] KVM: arm64: Contextualise the handling of PMCR_EL0.P writes

[Oliver Upton]

On Mon, Feb 17, 2025 at 11:24:12AM +0000, Marc Zyngier wrote:
> Contrary to what the comment says in kvm_pmu_handle_pmcr(),
> writing PMCR_EL0.P==1 has the following effects:
> 
> <quote>
> The event counters affected by this field are:
>   * All event counters in the first range.
>   * If any of the following are true, all event counters in the second
>     range:
>     - EL2 is disabled or not implemented in the current Security state.
>     - The PE is executing at EL2 or EL3.
> </quote>
> 
> where the "first range" represent the counters in the [0..HPMN-1]
> range, and the "second range" the counters in the [HPMN..MAX] range.
> 
> It so appears that writing P from EL2 should nuke all counters,
> and not just the "guest" view. Just do that, and nuke the misleading
> comment.
> 
> Reported-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks,
Oliver

Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Oliver Upton]

Hey,

On Mon, Feb 17, 2025 at 11:24:11AM +0000, Marc Zyngier wrote:
> The MDCR_EL2 documentation indicates that the HPMN field has
> the following behaviour:
> 
> "On a Warm reset, this field resets to the expression NUM_PMU_COUNTERS."
> 
> However, it appears we reset it to zero, which is not very useful.
> 
> Add a reset helper for MDCR_EL2, and handle the case where userspace
> changes the target PMU, which may force us to change HPMN again.
> 
> Reported-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

The existing ABI expectations are that writes to PMCR_EL0.N constrain
the number of counters, so that should have a similar effect on
MDCR_EL2.HPMN.

At the same time, I get the feeling that we should throw out this whole
behavior of writing N to change the shape of the PMU, because it
complete breaks down for NV. PMCR_EL0.N is another one of those fields
that change behavior based on EL and isn't a global source of truth on
the shape of the PMU.

What do you think about adding a new vCPU attribute for selecting the
number of counters for a VM? We can allow non-nested VMs to use the
'old' method of writing PMCR_EL0.N and force nested VMs to use the
attribute.

We can then enforce ordering on the attribute and prevent it from being
used after vCPU reset.

Thanks,
Oliver

Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Marc Zyngier]

On Mon, 17 Feb 2025 18:53:50 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey,
> 
> On Mon, Feb 17, 2025 at 11:24:11AM +0000, Marc Zyngier wrote:
> > The MDCR_EL2 documentation indicates that the HPMN field has
> > the following behaviour:
> > 
> > "On a Warm reset, this field resets to the expression NUM_PMU_COUNTERS."
> > 
> > However, it appears we reset it to zero, which is not very useful.
> > 
> > Add a reset helper for MDCR_EL2, and handle the case where userspace
> > changes the target PMU, which may force us to change HPMN again.
> > 
> > Reported-by: Joey Gouly <joey.gouly@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> The existing ABI expectations are that writes to PMCR_EL0.N constrain
> the number of counters, so that should have a similar effect on
> MDCR_EL2.HPMN.
> 
> At the same time, I get the feeling that we should throw out this whole
> behavior of writing N to change the shape of the PMU, because it
> complete breaks down for NV. PMCR_EL0.N is another one of those fields
> that change behavior based on EL and isn't a global source of truth on
> the shape of the PMU.
> 
> What do you think about adding a new vCPU attribute for selecting the
> number of counters for a VM? We can allow non-nested VMs to use the
> 'old' method of writing PMCR_EL0.N and force nested VMs to use the
> attribute.

VCPU attribute? or PMU attribute? I'm really not keen on the former,
but the latter is probably workable, as it is VM-wide, similar to the
way we keep track of pmcr_n.

> We can then enforce ordering on the attribute and prevent it from being
> used after vCPU reset.

How would that work? Do you really want to mandate the PMU selection
(with its counter capping) to strictly occur between vcpu creation and
init?

This would, for example, break kvmtool which has these two operations
back-to-back, and sneaking new device-specific actions in the middle
is a bit unpalatable (there is a split between VM-wide and per-vcpu
actions).

Any idea?

	M.

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

Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Oliver Upton]

On Wed, Feb 19, 2025 at 02:03:49PM +0000, Marc Zyngier wrote:
> On Mon, 17 Feb 2025 18:53:50 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > What do you think about adding a new vCPU attribute for selecting the
> > number of counters for a VM? We can allow non-nested VMs to use the
> > 'old' method of writing PMCR_EL0.N and force nested VMs to use the
> > attribute.
> 
> VCPU attribute? or PMU attribute? I'm really not keen on the former,
> but the latter is probably workable, as it is VM-wide, similar to the
> way we keep track of pmcr_n.

Well the _existing_ PMU attributes are actually vCPU attributes. I do
agree that accessing them as a VM attribute makes more sense, but that's
the UAPI we already have...

> > We can then enforce ordering on the attribute and prevent it from being
> > used after vCPU reset.
> 
> How would that work? Do you really want to mandate the PMU selection
> (with its counter capping) to strictly occur between vcpu creation and
> init?
> 
> This would, for example, break kvmtool which has these two operations
> back-to-back, and sneaking new device-specific actions in the middle
> is a bit unpalatable (there is a split between VM-wide and per-vcpu
> actions).
> 
> Any idea?

If we want to do this the 'right' way, we should provide VM attributes
for selecting the PMU implementation / configuring the event filter
to complement an attribute for setting the number of event counters.

I don't want to have a mix-and-match approach where vPMU attributes are
scattered between the vCPU and the VM since it requires a similar amount
of gymnastics in userspace to set crap up.

Thanks,
Oliver

Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Marc Zyngier]

On Wed, 19 Feb 2025 19:04:12 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Feb 19, 2025 at 02:03:49PM +0000, Marc Zyngier wrote:
> > On Mon, 17 Feb 2025 18:53:50 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > What do you think about adding a new vCPU attribute for selecting the
> > > number of counters for a VM? We can allow non-nested VMs to use the
> > > 'old' method of writing PMCR_EL0.N and force nested VMs to use the
> > > attribute.
> > 
> > VCPU attribute? or PMU attribute? I'm really not keen on the former,
> > but the latter is probably workable, as it is VM-wide, similar to the
> > way we keep track of pmcr_n.
> 
> Well the _existing_ PMU attributes are actually vCPU attributes. I do
> agree that accessing them as a VM attribute makes more sense, but that's
> the UAPI we already have...

Gah, I remember now. Someone please take the API to the backyard...

> 
> > > We can then enforce ordering on the attribute and prevent it from being
> > > used after vCPU reset.
> > 
> > How would that work? Do you really want to mandate the PMU selection
> > (with its counter capping) to strictly occur between vcpu creation and
> > init?
> > 
> > This would, for example, break kvmtool which has these two operations
> > back-to-back, and sneaking new device-specific actions in the middle
> > is a bit unpalatable (there is a split between VM-wide and per-vcpu
> > actions).
> > 
> > Any idea?
> 
> If we want to do this the 'right' way, we should provide VM attributes
> for selecting the PMU implementation / configuring the event filter
> to complement an attribute for setting the number of event counters.
> 
> I don't want to have a mix-and-match approach where vPMU attributes are
> scattered between the vCPU and the VM since it requires a similar amount
> of gymnastics in userspace to set crap up.

I agree on not mixing vCPU and VM scoped attributes, even if that
amounts to the same thing in the back-end. But freezing the number of
counters on vcpu reset is something that should be considered very
carefully, and I fear it breaks existing models -- specially given
that this is yet another one-off.

I wonder if we should instead make use of the KVM_ARM_VCPU_FINALIZE
ioctl just like we do for SVE, passing KVM_ARM_VCPU_PMU_V3 instead.
This would make use of an existing mechanism and lock the PMU for good
(implementation, IRQ, number of counters...).

	M.

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

Re: [PATCH 0/2] KVM: arm64: EL2 PMU reset handling fixes

[Joey Gouly]

On Mon, Feb 17, 2025 at 11:24:10AM +0000, Marc Zyngier wrote:
> Joey reports that some of his PMU tests do not behave quite as
> expected:
> 
> - MDCR_EL2.HPMN is set to 0 out of reset
> 
> - PMCR_EL0.P should reset all the counters when written from EL2
> 
> These couple of patches attempt to remedy the situation. Note that
> they have only been compile-tested.
> 
> Marc Zyngier (2):
>   KVM: arm64: Fix MDCR_EL2.HPMN reset value
>   KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
> 
>  arch/arm64/kvm/pmu-emul.c | 21 ++++++++++++++++-----
>  arch/arm64/kvm/sys_regs.c |  7 ++++++-
>  2 files changed, 22 insertions(+), 6 deletions(-)

I see you're discussing the API around this, but this does fix the PMU tests, so:

Tested-by: Joey Gouly <joey.gouly@arm.com>