All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state
@ 2024-11-20  0:52 Oliver Upton
  2024-11-20  0:52 ` [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status Oliver Upton
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Oliver Upton @ 2024-11-20  0:52 UTC (permalink / raw)
  To: kvmarm
  Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
	Mingwei Zhang, Colton Lewis, Raghavendra Rao Ananta, Oliver Upton

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.

[*]: https://lore.kernel.org/kvmarm/20241119205841.268247-1-rananta@google.com/

Oliver Upton (1):
  KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters

Raghavendra Rao Ananta (1):
  KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status

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


base-commit: 60ad25e14ab5a4e56c8bf7f7d6846eacb9cd53df
-- 
2.39.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status
  2024-11-20  0:52 [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
@ 2024-11-20  0:52 ` Oliver Upton
  2024-11-20  8:18   ` Marc Zyngier
  2024-11-20  0:52 ` [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters Oliver Upton
  2024-11-21  8:18 ` [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
  2 siblings, 1 reply; 9+ messages in thread
From: Oliver Upton @ 2024-11-20  0:52 UTC (permalink / raw)
  To: kvmarm
  Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
	Mingwei Zhang, Colton Lewis, Raghavendra Rao Ananta, stable,
	Oliver Upton

From: Raghavendra Rao Ananta <rananta@google.com>

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: stable@vger.kernel.org
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 <rananta@google.com>
[ oliver: massaged changelog ]
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 8ad62284fa23..3855cc9d0ca5 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -381,7 +381,6 @@ static u64 kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
 
 	if ((kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)) {
 		reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
-		reg &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
 		reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
 	}
 
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
  2024-11-20  0:52 [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
  2024-11-20  0:52 ` [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status Oliver Upton
@ 2024-11-20  0:52 ` Oliver Upton
  2024-11-20  1:00   ` Oliver Upton
  2024-11-20  8:38   ` Marc Zyngier
  2024-11-21  8:18 ` [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
  2 siblings, 2 replies; 9+ messages in thread
From: Oliver Upton @ 2024-11-20  0:52 UTC (permalink / raw)
  To: kvmarm
  Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
	Mingwei Zhang, Colton Lewis, Raghavendra Rao Ananta, Oliver Upton

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 <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 58 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 3855cc9d0ca5..fb79fa689ae5 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -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
@@ -288,8 +297,12 @@ bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int idx)
 	 * implementation choice that all counters are included in the second
 	 * range reserved for EL2/EL3.
 	 */
-	hpmn = SYS_FIELD_GET(MDCR_EL2, HPMN, __vcpu_sys_reg(vcpu, MDCR_EL2));
-	return idx >= hpmn;
+	return GENMASK(n - 1, hpmn);
+}
+
+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);
-		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);
+
+	/*
+	 * Otherwise, MDCR_EL2.HPME is the global enable control for event
+	 * counters reserved for EL2.
+	 */
+	if (!(vcpu_read_sys_reg(vcpu, MDCR_EL2) & MDCR_EL2_HPME))
+		reg &= ~kvm_pmu_hyp_counter_mask(vcpu);
 
 	return reg;
 }
@@ -395,7 +423,7 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu)
 	if (!kvm_vcpu_has_pmu(vcpu))
 		return;
 
-	overflow = !!kvm_pmu_overflow_status(vcpu);
+	overflow = kvm_pmu_overflow_status(vcpu);
 	if (pmu->irq_level == overflow)
 		return;
 
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
  2024-11-20  0:52 ` [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters Oliver Upton
@ 2024-11-20  1:00   ` Oliver Upton
  2024-11-20  8:38   ` Marc Zyngier
  1 sibling, 0 replies; 9+ messages in thread
From: Oliver Upton @ 2024-11-20  1:00 UTC (permalink / raw)
  To: kvmarm
  Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
	Mingwei Zhang, Colton Lewis, Raghavendra Rao Ananta

On Tue, Nov 19, 2024 at 04:52:30PM -0800, Oliver Upton wrote:
> 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.

... and the untested garbage award goes to:

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

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fb79fa689ae5..456102bc0b55 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -276,6 +276,8 @@ void kvm_pmu_vcpu_destroy(struct kvm_vcpu *vcpu)
 
 static u64 kvm_pmu_hyp_counter_mask(struct kvm_vcpu *vcpu)
 {
+	unsigned int hpmn, n;
+
 	if (!vcpu_has_nv(vcpu))
 		return 0;
 
@@ -308,7 +310,6 @@ bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int idx)
 u64 kvm_pmu_accessible_counter_mask(struct kvm_vcpu *vcpu)
 {
 	u64 mask = kvm_pmu_implemented_counter_mask(vcpu);
-	u64 hpmn;
 
 	if (!vcpu_has_nv(vcpu) || vcpu_is_el2(vcpu))
 		return mask;

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status
  2024-11-20  0:52 ` [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status Oliver Upton
@ 2024-11-20  8:18   ` Marc Zyngier
  0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2024-11-20  8:18 UTC (permalink / raw)
  To: Oliver Upton
  Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, Mingwei Zhang,
	Colton Lewis, Raghavendra Rao Ananta, stable

On Wed, 20 Nov 2024 00:52:29 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> From: Raghavendra Rao Ananta <rananta@google.com>
> 
> DDI0487K D13.1.1 describes the PMU overflow condition, which evaluates

nit: DDI0487K.a, and D13.3.1.

> 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: stable@vger.kernel.org
> 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 <rananta@google.com>
> [ oliver: massaged changelog ]
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/pmu-emul.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 8ad62284fa23..3855cc9d0ca5 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -381,7 +381,6 @@ static u64 kvm_pmu_overflow_status(struct kvm_vcpu *vcpu)
>  
>  	if ((kvm_vcpu_read_pmcr(vcpu) & ARMV8_PMU_PMCR_E)) {
>  		reg = __vcpu_sys_reg(vcpu, PMOVSSET_EL0);
> -		reg &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
>  		reg &= __vcpu_sys_reg(vcpu, PMINTENSET_EL1);
>  	}
>  

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
  2024-11-20  0:52 ` [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters Oliver Upton
  2024-11-20  1:00   ` Oliver Upton
@ 2024-11-20  8:38   ` Marc Zyngier
  2024-11-20  8:54     ` Oliver Upton
  1 sibling, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2024-11-20  8:38 UTC (permalink / raw)
  To: Oliver Upton
  Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, Mingwei Zhang,
	Colton Lewis, Raghavendra Rao Ananta

On Wed, 20 Nov 2024 00:52:30 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> 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 <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/pmu-emul.c | 58 +++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 3855cc9d0ca5..fb79fa689ae5 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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
> @@ -288,8 +297,12 @@ bool kvm_pmu_counter_is_hyp(struct kvm_vcpu *vcpu, unsigned int idx)
>  	 * implementation choice that all counters are included in the second
>  	 * range reserved for EL2/EL3.
>  	 */
> -	hpmn = SYS_FIELD_GET(MDCR_EL2, HPMN, __vcpu_sys_reg(vcpu, MDCR_EL2));
> -	return idx >= hpmn;
> +	return GENMASK(n - 1, hpmn);

This explicitly excludes the cycle counter (bit 31), and lead to some
unexpected results below.

> +}
> +
> +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);
> -		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.

Thanks,

	M.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
  2024-11-20  8:38   ` Marc Zyngier
@ 2024-11-20  8:54     ` Oliver Upton
  2024-11-20  9:55       ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Oliver Upton @ 2024-11-20  8:54 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, Mingwei Zhang,
	Colton Lewis, Raghavendra Rao Ananta

On Wed, Nov 20, 2024 at 08:38:22AM +0000, Marc Zyngier wrote:
> On Wed, 20 Nov 2024 00:52:30 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > +/*
> > + * 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);
> > -		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.

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.

-- 
Thanks,
Oliver

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
  2024-11-20  8:54     ` Oliver Upton
@ 2024-11-20  9:55       ` Marc Zyngier
  0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2024-11-20  9:55 UTC (permalink / raw)
  To: Oliver Upton
  Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, Mingwei Zhang,
	Colton Lewis, Raghavendra Rao Ananta

On Wed, 20 Nov 2024 08:54:45 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Nov 20, 2024 at 08:38:22AM +0000, Marc Zyngier wrote:
> > On Wed, 20 Nov 2024 00:52:30 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > +/*
> > > + * 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);
> > > -		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.
> 
> 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
> """

Ah, fair enough. I had the misguided impression that there would be
another control for that, but this is actually called out in D13.11.2
("The PMU does not provide any control that a hypervisor can use to
reserve the cycle counter for its own use.").

I stand corrected.

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

Nah, that's definitely me.

FWIW, and with the follow-up fixes:

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state
  2024-11-20  0:52 [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
  2024-11-20  0:52 ` [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status Oliver Upton
  2024-11-20  0:52 ` [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters Oliver Upton
@ 2024-11-21  8:18 ` Oliver Upton
  2 siblings, 0 replies; 9+ messages in thread
From: Oliver Upton @ 2024-11-21  8:18 UTC (permalink / raw)
  To: kvmarm, Oliver Upton
  Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu,
	Mingwei Zhang, Colton Lewis, Raghavendra Rao Ananta

On Tue, 19 Nov 2024 16:52:28 -0800, Oliver Upton wrote:
> 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.
> 
> [...]

Applied to fixes, thanks!

[1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status
      https://git.kernel.org/kvmarm/kvmarm/c/54bbee190d42
[2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters
      https://git.kernel.org/kvmarm/kvmarm/c/13905f4547b0

--
Best,
Oliver

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-11-21  8:18 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-20  0:52 [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton
2024-11-20  0:52 ` [PATCH v2 1/2] KVM: arm64: Ignore PMCNTENSET_EL0 while checking for overflow status Oliver Upton
2024-11-20  8:18   ` Marc Zyngier
2024-11-20  0:52 ` [PATCH v2 2/2] KVM: arm64: Use MDCR_EL2.HPME to evaluate overflow of hyp counters Oliver Upton
2024-11-20  1:00   ` Oliver Upton
2024-11-20  8:38   ` Marc Zyngier
2024-11-20  8:54     ` Oliver Upton
2024-11-20  9:55       ` Marc Zyngier
2024-11-21  8:18 ` [PATCH v2 0/2] KVM: arm64: Fixes for PMU overflow state Oliver Upton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.