On Wed, 23 Nov 2022 05:58:17 +0000,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> Hi Marc,
>
> On Sun, Nov 13, 2022 at 8:46 AM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
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.
