Linux KVM/arm64 development list
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox