[PATCH v1 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer

[Jing Zhang]

Hi Reiji,

On Thu, Feb 9, 2023 at 10:16 PM Reiji Watanabe <reijiw at google.com> wrote:
>
> Hi Jing,
>
> On Thu, Feb 9, 2023 at 1:25 PM Jing Zhang <jingzhangos at google.com> wrote:
> >
> > On Mon, Feb 6, 2023 at 9:30 PM Reiji Watanabe <reijiw at google.com> wrote:
> > >
> > > Hi Jing,
> > >
> > > On Tue, Jan 31, 2023 at 6:51 PM Jing Zhang <jingzhangos at google.com> wrote:
> > > >
> > > > With per guest ID registers, PMUver settings from userspace
> > > > can be stored in its corresponding ID register.
> > > >
> > > > No functional change intended.
> > > >
> > > > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h |  5 -----
> > > >  arch/arm64/kvm/arm.c              |  6 ------
> > > >  arch/arm64/kvm/id_regs.c          | 33 ++++++++++++++++++++-----------
> > > >  include/kvm/arm_pmu.h             |  6 ++++--
> > > >  4 files changed, 25 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index fabb30185a4a..1ab443b52c46 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -225,11 +225,6 @@ struct kvm_arch {
> > > >
> > > >         cpumask_var_t supported_cpus;
> > > >
> > > > -       struct {
> > > > -               u8 imp:4;
> > > > -               u8 unimp:4;
> > > > -       } dfr0_pmuver;
> > > > -
> > > >         /* Hypercall features firmware registers' descriptor */
> > > >         struct kvm_smccc_features smccc_feat;
> > > >
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index d8ba5106bf51..25bd95650223 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -138,12 +138,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > > >         kvm_arm_set_default_id_regs(kvm);
> > > >         kvm_arm_init_hypercalls(kvm);
> > > >
> > > > -       /*
> > > > -        * Initialise the default PMUver before there is a chance to
> > > > -        * create an actual PMU.
> > > > -        */
> > > > -       kvm->arch.dfr0_pmuver.imp = kvm_arm_pmu_get_pmuver_limit();
> > > > -
> > > >         return 0;
> > > >
> > > >  err_free_cpumask:
> > > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > > > index bc5d9bc84eb1..5eade7d380af 100644
> > > > --- a/arch/arm64/kvm/id_regs.c
> > > > +++ b/arch/arm64/kvm/id_regs.c
> > > > @@ -19,10 +19,12 @@
> > > >
> > > >  static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
> > > >  {
> > > > -       if (kvm_vcpu_has_pmu(vcpu))
> > > > -               return vcpu->kvm->arch.dfr0_pmuver.imp;
> > > > -
> > > > -       return vcpu->kvm->arch.dfr0_pmuver.unimp;
> > > > +       u8 pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> > > > +                       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1));
> > > > +       if (kvm_vcpu_has_pmu(vcpu) || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> > > > +               return pmuver;
> > > > +       else
> > > > +               return 0;
> > > >  }
> > > >
> > > >  static u8 perfmon_to_pmuver(u8 perfmon)
> > > > @@ -263,10 +265,9 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > > >         if (val)
> > > >                 return -EINVAL;
> > > >
> > > > -       if (valid_pmu)
> > > > -               vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
> > > > -       else
> > > > -               vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
> > > > +       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > > > +       IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |=
> > > > +               FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver);
> > >
> > > Did you consider there could be guests that have vCPUs with and
> > > without PMU ? It looks like the code doesn't work for such guests.
> > > (e.g. for such guests, if setting the register is done for vCPUs
> > >  without PMU, this code seems to make PMUVer zero or 0xf
> > >  even for vCPUs with PMU)
> > Yes, I did. The PMUVer field is a per-VCPU field, whose value was
> > determined on the fly in the get_user function. Check the function
> > kvm_arm_read_id_reg_with_encoding, vcpu_pmuver is called to set the
> > real value for the VCPU.
> > The perVM ID registers array we put in the kvm structure save
> > correctly only for those perVM field, for those pre-VCPU filed, their
> > real value is determined on the fly during the register reading
> > (get_user).
>
> Yes, I understand that part.
> But, if a guest has 2 VCPUs, one of which has a PMU and the other
> does not, the PMUVer for them will be different.  Which value do
> you try to save in the per VM field ?
>
> It appears that the code always saves the last value set by
> userspace in the per VM field, regardless of whether the vCPU
> has PMU or not.
> If the last value is set for the vCPU without PMU, the per VM
> field will be either 0 or 0xf.  Since the per VM field is always
> used by vcpu_pmuver() for vCPUs with PMU, I would think the
> PMUVer for the vCPU with PMU will end up being 0 or 0xf in this
> case (NOTE: the PMUVer for vCPUs with PMU should not be 0 or 0xf).
>
> Or am I missing something??
Thanks for the clarification. You are right about the issue. I'll fix
it in the next version by utilizing a bit in kvm->arch.flags to
indicate the IMPDEF PMU. The value stored in id_regs would be the one
for vCPU with PMU support.
>
> Thanks,
> Reiji