Re: [PATCH v2 11/14] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace

[Marc Zyngier <maz@kernel.org>]

On Thu, 03 Nov 2022 05:31:56 +0000,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Allow userspace to write ID_AA64DFR0_EL1, on the condition that only
> > the PMUver field can be altered and be at most the one that was
> > initially computed for the guest.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 37 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 7a4cd644b9c0..4fa14b4ae2a6 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1247,6 +1247,40 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> >         return 0;
> >  }
> >
> > +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > +                              const struct sys_reg_desc *rd,
> > +                              u64 val)
> > +{
> > +       u8 pmuver, host_pmuver;
> > +
> > +       host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> > +
> > +       /*
> > +        * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
> > +        * as it doesn't promise more than what the HW gives us. We
> > +        * allow an IMPDEF PMU though, only if no PMU is supported
> > +        * (KVM backward compatibility handling).
> > +        */
> 
> It appears the patch allows userspace to set IMPDEF even
> when host_pmuver == 0.  Shouldn't it be allowed only when
> host_pmuver == IMPDEF (as before)?
> Probably, it may not cause any real problems though.

Given that we don't treat the two cases any differently, I thought it
would be reasonable to relax this particular case, and I can't see any
reason why we shouldn't tolerate this sort of migration.

> 
> 
> > +       pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val);
> > +       if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver)
> > +               return -EINVAL;
> > +
> > +       /* We already have a PMU, don't try to disable it... */
> > +       if (kvm_vcpu_has_pmu(vcpu) &&
> > +           (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF))
> > +               return -EINVAL;
> 
> Nit: Perhaps it might be useful to return a different error code for the
> above two (new) error cases (I plan to use -E2BIG and -EPERM
> respectively for those cases with my ID register series).

My worry in doing so is that we don't have an established practice for
these cases. I'm fine with introducing new error codes, but I'm not
sure there is an existing practice in userspace to actually interpret
them.

Even -EPERM has a slightly different meaning, and although there is
some language there saying that it is all nonsense, we should be very
careful.

Thanks,

	M.

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