Re: [PATCH v5 02/12] KVM: arm64: PMU: Set the default PMU for the guest on vCPU reset

[Raghavendra Rao Ananta]

On Fri, Sep 15, 2023 at 12:33 PM Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
>
> On Thu, Aug 17, 2023 at 12:30:19AM +0000, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <reijiw@xxxxxxxxxx>
> >
> > The following patches will use the number of counters information
> > from the arm_pmu and use this to set the PMCR.N for the guest
> > during vCPU reset. However, since the guest is not associated
> > with any arm_pmu until userspace configures the vPMU device
> > attributes, and a reset can happen before this event, call
> > kvm_arm_support_pmu_v3() just before doing the reset.
> >
> > No functional change intended.
>
> But there absolutely is a functional change here, and user visible at
> that. KVM_ARM_VCPU_INIT ioctls can now fail with -ENODEV, which is not
> part of the documented errors for the interface.
>
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > ---
> >  arch/arm64/kvm/pmu-emul.c |  9 +--------
> >  arch/arm64/kvm/reset.c    | 18 +++++++++++++-----
> >  include/kvm/arm_pmu.h     |  6 ++++++
> >  3 files changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 0ffd1efa90c07..b87822024828a 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -865,7 +865,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
> >       return true;
> >  }
> >
> > -static int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > +int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> >  {
> >       lockdep_assert_held(&kvm->arch.config_lock);
> >
> > @@ -937,13 +937,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >       if (vcpu->arch.pmu.created)
> >               return -EBUSY;
> >
> > -     if (!kvm->arch.arm_pmu) {
> > -             int ret = kvm_arm_set_vm_pmu(kvm, NULL);
> > -
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> >       switch (attr->attr) {
> >       case KVM_ARM_VCPU_PMU_V3_IRQ: {
> >               int __user *uaddr = (int __user *)(long)attr->addr;
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > index bc8556b6f4590..4c20f1ccd0789 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -206,6 +206,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> >   */
> >  int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >  {
> > +     struct kvm *kvm = vcpu->kvm;
> >       struct vcpu_reset_state reset_state;
> >       int ret;
> >       bool loaded;
> > @@ -216,6 +217,18 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> >       vcpu->arch.reset_state.reset = false;
> >       spin_unlock(&vcpu->arch.mp_state_lock);
> >
> > +     /*
> > +      * When the vCPU has a PMU, but no PMU is set for the guest
> > +      * yet, set the default one.
> > +      */
> > +     if (kvm_vcpu_has_pmu(vcpu) && unlikely(!kvm->arch.arm_pmu)) {
> > +             ret = -EINVAL;
> > +             if (kvm_arm_support_pmu_v3())
> > +                     ret = kvm_arm_set_vm_pmu(kvm, NULL);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
>
> On top of my prior suggestion w.r.t. the default PMU helper, I'd rather
> see this block look like:
>
>         if (kvm_vcpu_has_pmu(vcpu)) {
>                 if (!kvm_arm_support_pmu_v3())
>                         return -EINVAL;
>                 /*
>                  * When the vCPU has a PMU but no PMU is set for the
>                  * guest yet, set the default one.
>                  */
>                 if (unlikely(!kvm->arch.arm_pmu) && kvm_set_default_pmu(kvm))
>                         return -EINVAL;
>         }
>
> This would eliminate the possibility of returning ENODEV to userspace
> where we shouldn't.
>
I understand that we'll be breaking the API contract and userspace may
have to adapt to this change, but is it not acceptable to document and
return ENODEV, since ENODEV may offer more clarity to userspace as to
why the ioctl failed? In general, do we never extend the APIs?

Thank you.
Raghavendra
> --
> Thanks,
> Oliver