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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

On Wed, Nov 23, 2022 at 3:11 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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

I thought about that too.  What makes it a bit tricky is that
given that kvm->arch.dfr0_pmuver.imp is shared among all vCPUs
for the guest, updating the PMCR should be done for all the vCPUs.

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

Or if userspace is expected to set ID_AA64DFR0_EL1 (PMUVER) for
each vCPU, update_dfr0_pmuver() should update PMCR even when
'kvm->arch.dfr0_pmuver.imp' is the same as the given 'pmuver'.
(as PMCR for the vCPU might have not been updated yet)

> 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).

Yes, it makes some sense.
But, with that, for live migration, PMCR should be restored only
after ID_AA64DFR0_EL1/ID_DFR0_EL1 is restored (to avoid PMCR
being reset after PMCR is restored), which I would think might
affect live migration.
(Or am I misunderstanding something ?)

Thank you,
Reiji


>
> >
> >
> > >  }
> > >
> > >  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.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux