KVM: arm64: Fixes for PMU overflow state

From: Raghavendra Rao Ananta <[email protected]>

DDI0487K D13.1.1 describes the PMU overflow condition, which evaluates

to true if any counter's global enable (PMCR_EL0.E), overflow flag (PMOVSSET_EL0[n]), and interrupt enable (PMINTENSET_EL1[n]) are all 1. Of note, this does not require a counter to be enabled (i.e. PMCNTENSET_EL0[n] = 1) to generate an overflow.

Align kvm_pmu_overflow_status() with the reality of the architecture and stop using PMCNTENSET_EL0 as part of the overflow condition. The bug was discovered while running an SBSA PMU test [*], which only sets PMCR.E, PMOVSSET<0>, PMINTENSET<0>, and expects an overflow interrupt.

Cc [email protected]

Fixes 76d883c4e640 ("arm64: KVM: Add access handler for PMOVSSET and PMOVSCLR register")

Link https://github.com/ARM-software/sbsa-acs/blob/master/test_pool/pmu/operating_system/test_pmu001.c

Signed-off-by Raghavendra Rao Ananta <[email protected]>

[ oliver: massaged changelog ]

Signed-off-by Oliver Upton <[email protected]>

---
 arch/arm64/kvm/pmu-emul.c | 1 -
 1 file changed, 1 deletion(-)

arch/arm64/kvm/pmu-emul.c +0 -0

index 8ad62284fa23..3855cc9d0ca5 100644

The 'global enable control' (as it is termed in the architecture) for counters reserved by EL2 is MDCR_EL2.HPME. Use that instead of PMCR_EL0.E when evaluating the overflow state for hyp counters.

Change the return value to a bool while at it, which better reflects the fact that the overflow state is a shared signal and not a per-counter property.

Signed-off-by Oliver Upton <[email protected]>

The 'global enable control' (as it is termed in the architecture) for counters reserved by EL2 is MDCR_EL2.HPME. Use that instead of PMCR_EL0.E when evaluating the overflow state for hyp counters.

Change the return value to a bool while at it, which better reflects the fact that the overflow state is a shared signal and not a per-counter property.

Signed-off-by Oliver Upton <[email protected]>

---
 arch/arm64/kvm/pmu-emul.c | 58 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 15 deletions(-)

arch/arm64/kvm/pmu-emul.c +13 -4

index 3855cc9d0ca5..fb79fa689ae5 100644
@@ -274,12 +274,21 @@ void kvm_pmu_vcpu_destroy(struct kvm_vcpu *vcpu)
 	irq_work_sync(&vcpu->arch.pmu.overflow_work);
 }
 
-bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int idx)
+static u64 kvm_pmu_hyp_counter_mask(struct kvm_vcpu *vcpu)
 {
-	unsigned int hpmn;
+	if (!vcpu_has_nv(vcpu))
+		return 0;
 
-	if (!vcpu_has_nv(vcpu) || idx == ARMV8_PMU_CYCLE_IDX)
-		return false;
+	hpmn = SYS_FIELD_GET(MDCR_EL2, HPMN, __vcpu_sys_reg(vcpu, MDCR_EL2));
+	n = vcpu->kvm->arch.pmcr_n;
+
+	/*
+	 * Programming HPMN to a value greater than PMCR_EL0.N is
+	 * CONSTRAINED UNPREDICTABLE. Make the implementation choice that an
+	 * UNKNOWN number of counters (in our case, zero) are reserved for EL2.
+	 */
+	if (hpmn >= n)
+		return 0;
 
 	/*
 	 * Programming HPMN=0 is CONSTRAINED UNPREDICTABLE if FEAT_HPMN0 isn't

arch/arm64/kvm/pmu-emul.c +13 -6

+}
+
+bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int idx)
+{
+	return kvm_pmu_hyp_counter_mask(vcpu) & BIT(idx);
 }
 
 u64 kvm_pmu_accessible_counter_mask(struct kvm_vcpu *vcpu)

@@ -300,8 +313,7 @@ u64 kvm_pmu_accessible_counter_mask(struct kvm_vcpu *vcpu)
 	if (!vcpu_has_nv(vcpu) || vcpu_is_el2(vcpu))
 		return mask;
 
-	hpmn = SYS_FIELD_GET(MDCR_EL2, HPMN, __vcpu_sys_reg(vcpu, MDCR_EL2));
-	return mask & ~GENMASK(vcpu->kvm->arch.pmcr_n - 1, hpmn);
+	return mask & ~kvm_pmu_hyp_counter_mask(vcpu);
 }
 
 u64 kvm_pmu_implemented_counter_mask(struct kvm_vcpu *vcpu)

@@ -375,14 +387,30 @@ void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
 	}
 }
 
-static u64 kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
+/*
+ * Returns the PMU overflow state, which is true if there exists an event
+ * counter where the values of the global enable control, PMOVSSET_EL0[n], and
+ * PMINTENSET_EL1[n] are all 1.
+ */
+static bool kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
 {
-	u64 reg = 0;
+	u64 reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
 
-	if ((kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)) {
-		reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);

Quoted 12 lines · Oliver Upton

arch/arm64/kvm/pmu-emul.c +7 -3

+/*
+ * Returns the PMU overflow state, which is true if there exists an event
+ * counter where the values of the global enable control, PMOVSSET_EL0[n], and
+ * PMINTENSET_EL1[n] are all 1.
+ */
+static bool kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
 {
-	u64 reg = 0;
+	u64 reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
 
-	if ((kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)) {
-		reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);

arch/arm64/kvm/pmu-emul.c void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val) · line 406

Quoted 10 lines

arch/arm64/kvm/pmu-emul.c +8 -2

-		reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
-	}
+	reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
+
+	/*
+	 * PMCR_EL0.E is the global enable control for event counters available
+	 * to EL0 and EL1.
+	 */
+	if (!(kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E))
+		reg &= kvm_pmu_hyp_counter_mask(vcpu);

So if the PMU is disabled at EL1, we remove the cycle counter from the list of counters that are considered for an overflow.

I don't think that's what you really want.

Quoted 27 lines · Marc Zyngier

Quoted 12 lines · Oliver Upton

arch/arm64/kvm/pmu-emul.c +7 -3

+/*
+ * Returns the PMU overflow state, which is true if there exists an event
+ * counter where the values of the global enable control, PMOVSSET_EL0[n], and
+ * PMINTENSET_EL1[n] are all 1.
+ */
+static bool kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
 {
-	u64 reg = 0;
+	u64 reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
 
-	if ((kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)) {
-		reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);

arch/arm64/kvm/pmu-emul.c void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val) · line 406

Quoted 10 lines

arch/arm64/kvm/pmu-emul.c +8 -2

-		reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
-	}
+	reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
+
+	/*
+	 * PMCR_EL0.E is the global enable control for event counters available
+	 * to EL0 and EL1.
+	 */
+	if (!(kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E))
+		reg &= kvm_pmu_hyp_counter_mask(vcpu);

So if the PMU is disabled at EL1, we remove the cycle counter from the list of counters that are considered for an overflow.

Quoted 1 line

I don't think that's what you really want.

That's how it is written though, no? My understanding is that the fixed counters are never reserved by EL2:

"""
The PE signals a request for:

[...]

- The cycle counter, when the values of PMCR_EL0.E, PMOVSSET[31] and PMINTENSET[31] are all 1
"""

PMUOverflowCondition() and PMUCounterIsHyp() seem to match this too. Unless I can't read, which happens a lot.

Respin of Raghavendra's fix for evaluating PMU overflow [*], with another patch stuck on top for my own blatant disregard of the architecture in the vEL2 bits.

Since I fiddled with this on the M2 these patches are obviously untested due to the lack of PMUv3. I'll give it a spin on supporting hardware before actually applying patches, of course.

[...]