[PATCH 1/2] KVM: arm64: PMU: Fix GET_ONE_REG for vPMC regs to return the current value

[Reiji Watanabe]

Hi Marc,

On Sun, Mar 12, 2023 at 7:57 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Thu, 02 Mar 2023 05:50:32 +0000,
> Reiji Watanabe <reijiw at google.com> wrote:
> >
> > Have KVM_GET_ONE_REG for vPMU counter (vPMC) registers (PMCCNTR_EL0
> > and PMEVCNTR<n>_EL0) return the sum of the register value in the sysreg
> > file and the current perf event counter value.
> >
> > Values of vPMC registers are saved in sysreg files on certain occasions.
> > These saved values don't represent the current values of the vPMC
> > registers if the perf events for the vPMCs count events after the save.
> > The current values of those registers are the sum of the sysreg file
> > value and the current perf event counter value.  But, when userspace
> > reads those registers (using KVM_GET_ONE_REG), KVM returns the sysreg
> > file value to userspace (not the sum value).
> >
> > Fix this to return the sum value for KVM_GET_ONE_REG.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index c6cbfe6b854b..c48c053d6146 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -765,6 +765,22 @@ static bool pmu_counter_idx_valid(struct kvm_vcpu *vcpu, u64 idx)
> >       return true;
> >  }
> >
> > +static int get_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > +                       u64 *val)
> > +{
> > +     u64 idx;
> > +
> > +     if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 0)
> > +             /* PMCCNTR_EL0 */
> > +             idx = ARMV8_PMU_CYCLE_IDX;
> > +     else
> > +             /* PMEVCNTRn_EL0 */
> > +             idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
> > +
> > +     *val = kvm_pmu_get_counter_value(vcpu, idx);
> > +     return 0;
>
> It is a bit odd not to return an error when no PMU present, but this
> is already filtered out by the top-level accessors.

Yes, exactly.

>
> > +}
> > +
> >  static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
> >                             struct sys_reg_params *p,
> >                             const struct sys_reg_desc *r)
> > @@ -981,7 +997,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >  /* Macro to expand the PMEVCNTRn_EL0 register */
> >  #define PMU_PMEVCNTR_EL0(n)                                          \
> >       { PMU_SYS_REG(SYS_PMEVCNTRn_EL0(n)),                            \
> > -       .reset = reset_pmevcntr,                                      \
> > +       .reset = reset_pmevcntr, .get_user = get_pmu_evcntr,          \
> >         .access = access_pmu_evcntr, .reg = (PMEVCNTR0_EL0 + n), }
> >
> >  /* Macro to expand the PMEVTYPERn_EL0 register */
> > @@ -1745,7 +1761,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >       { PMU_SYS_REG(SYS_PMCEID1_EL0),
> >         .access = access_pmceid, .reset = NULL },
> >       { PMU_SYS_REG(SYS_PMCCNTR_EL0),
> > -       .access = access_pmu_evcntr, .reset = reset_unknown, .reg = PMCCNTR_EL0 },
> > +       .access = access_pmu_evcntr, .reset = reset_unknown,
> > +       .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr},
> >       { PMU_SYS_REG(SYS_PMXEVTYPER_EL0),
> >         .access = access_pmu_evtyper, .reset = NULL },
> >       { PMU_SYS_REG(SYS_PMXEVCNTR_EL0),
>
> Reviewed-by: Marc Zyngier <maz at kernel.org>

Thank you!
Reiji

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