Re: [PATCH v4 13/16] KVM: arm64: PMU: Implement PMUv3p5 long counter support

[Marc Zyngier <maz@kernel.org>]

On Wed, 23 Nov 2022 05:58:17 +0000,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Sun, Nov 13, 2022 at 8:46 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > PMUv3p5 (which is mandatory with ARMv8.5) comes with some extra
> > features:
> >
> > - All counters are 64bit
> >
> > - The overflow point is controlled by the PMCR_EL0.LP bit
> >
> > Add the required checks in the helpers that control counter
> > width and overflow, as well as the sysreg handling for the LP
> > bit. A new kvm_pmu_is_3p5() helper makes it easy to spot the
> > PMUv3p5 specific handling.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/pmu-emul.c | 8 +++++---
> >  arch/arm64/kvm/sys_regs.c | 4 ++++
> >  include/kvm/arm_pmu.h     | 7 +++++++
> >  3 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 4320c389fa7f..c37cc67ff1d7 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -52,13 +52,15 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
> >   */
> >  static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> >  {
> > -       return (select_idx == ARMV8_PMU_CYCLE_IDX);
> > +       return (select_idx == ARMV8_PMU_CYCLE_IDX || kvm_pmu_is_3p5(vcpu));
> >  }
> >
> >  static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
> >  {
> > -       return (select_idx == ARMV8_PMU_CYCLE_IDX &&
> > -               __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> > +       u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
> > +
> > +       return (select_idx < ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LP)) ||
> > +              (select_idx == ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LC));
> 
> Since the vCPU's PMCR_EL0 value is not always in sync with
> kvm->arch.dfr0_pmuver.imp, shouldn't kvm_pmu_idx_has_64bit_overflow()
> check kvm_pmu_is_3p5() ?
> (e.g. when the host supports PMUv3p5, PMCR.LP will be set by reset_pmcr()
> initially. Then, even if userspace sets ID_AA64DFR0_EL1.PMUVER to
> PMUVer_V3P1, PMCR.LP will stay the same (still set) unless PMCR is
> written.  So, kvm_pmu_idx_has_64bit_overflow() might return true
> even though the guest's PMU version is lower than PMUVer_V3P5.)

I can see two ways to address this: either we spray PMUv3p5 checks
every time we evaluate PMCR, or we sanitise PMCR after each userspace
write to ID_AA64DFR0_EL1.

I'd like to be able to take what is stored in the register file at
face value, so I'm angling towards the second possibility. It also
makes some sense from a 'HW' perspective: you change the HW
dynamically by selecting a new version, the HW comes up with its reset
configuration (i.e don't expect PMCR to stick after you write to
DFR0 with a different PMUVer).

>
> 
> >  }
> >
> >  static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index dc201a0557c0..615cb148e22a 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -654,6 +654,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >                | (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E);
> >         if (!kvm_supports_32bit_el0())
> >                 val |= ARMV8_PMU_PMCR_LC;
> > +       if (!kvm_pmu_is_3p5(vcpu))
> > +               val &= ~ARMV8_PMU_PMCR_LP;
> >         __vcpu_sys_reg(vcpu, r->reg) = val;
> >  }
> >
> > @@ -703,6 +705,8 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >                 val |= p->regval & ARMV8_PMU_PMCR_MASK;
> >                 if (!kvm_supports_32bit_el0())
> >                         val |= ARMV8_PMU_PMCR_LC;
> > +               if (!kvm_pmu_is_3p5(vcpu))
> > +                       val &= ~ARMV8_PMU_PMCR_LP;
> >                 __vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> >                 kvm_pmu_handle_pmcr(vcpu, val);
> >                 kvm_vcpu_pmu_restore_guest(vcpu);
> 
> For the read case of access_pmcr() (the code below),
> since PMCR.LP is RES0 when FEAT_PMUv3p5 is not implemented,
> shouldn't it clear PMCR.LP if kvm_pmu_is_3p5(vcpu) is false ?
> (Similar issue to kvm_pmu_idx_has_64bit_overflow())
> 
>         } else {
>                 /* PMCR.P & PMCR.C are RAZ */
>                 val = __vcpu_sys_reg(vcpu, PMCR_EL0)
>                       & ~(ARMV8_PMU_PMCR_P | ARMV8_PMU_PMCR_C);
>                 p->regval = val;
>         }

I think the above would address it. I've tentatively queued the
following patch, please let me know if this looks OK to you.

Thanks,

	M.

From d90ec0e8768ce5f7ae11403b29db76260dfaa3f2 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Wed, 23 Nov 2022 11:03:07 +0000
Subject: [PATCH] KVM: arm64: PMU: Reset PMCR_EL0 on PMU version change

Changing the version of the PMU emulation may result in stale
bits still being present in the PMCR_EL0 register, leading to
unexpected results.

Address it by forcing PMCR_EL0 to its reset value when the value
of ID_AA64DFR0.PMUVer (or ID_DFR0.Perfmon) changes.

Reported-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 67eac0f747be..12a873d94aaf 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -637,15 +637,10 @@ static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK;
 }
 
-static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static void __reset_pmcr(struct kvm_vcpu *vcpu, u64 pmcr)
 {
-	u64 pmcr, val;
-
-	/* No PMU available, PMCR_EL0 may UNDEF... */
-	if (!kvm_arm_support_pmu_v3())
-		return;
+	u64 val;
 
-	pmcr = read_sysreg(pmcr_el0);
 	/*
 	 * Writable bits of PMCR_EL0 (ARMV8_PMU_PMCR_MASK) are reset to UNKNOWN
 	 * except PMCR.E resetting to zero.
@@ -656,7 +651,16 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 		val |= ARMV8_PMU_PMCR_LC;
 	if (!kvm_pmu_is_3p5(vcpu))
 		val &= ~ARMV8_PMU_PMCR_LP;
-	__vcpu_sys_reg(vcpu, r->reg) = val;
+	__vcpu_sys_reg(vcpu, PMCR_EL0) = val;
+}
+
+static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	/* No PMU available, PMCR_EL0 may UNDEF... */
+	if (!kvm_arm_support_pmu_v3())
+		return;
+
+	__reset_pmcr(vcpu, read_sysreg(pmcr_el0));
 }
 
 static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
@@ -1259,6 +1263,14 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static void update_dfr0_pmuver(struct kvm_vcpu *vcpu, u8 pmuver)
+{
+	if (vcpu->kvm->arch.dfr0_pmuver.imp != pmuver) {
+		vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
+		__reset_pmcr(vcpu, __vcpu_sys_reg(vcpu, PMCR_EL0));
+	}
+}
+
 static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
@@ -1291,7 +1303,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	if (valid_pmu)
-		vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
+		update_dfr0_pmuver(vcpu, pmuver);
 	else
 		vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
 
@@ -1331,7 +1343,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	if (valid_pmu)
-		vcpu->kvm->arch.dfr0_pmuver.imp = perfmon_to_pmuver(perfmon);
+		update_dfr0_pmuver(vcpu, perfmon_to_pmuver(perfmon));
 	else
 		vcpu->kvm->arch.dfr0_pmuver.unimp = perfmon_to_pmuver(perfmon);
 
-- 
2.34.1


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel