On Mon, Oct 23, 2023 at 11:40:50AM +0100, Marc Zyngier wrote:
[...]
> > +static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm *kvm = vcpu->kvm;
> > +
> > + /*
> > + * 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) && !kvm->arch.arm_pmu &&
> > + kvm_arm_set_default_pmu(kvm))
> > + return -EINVAL;
>
> nit: I'm not keen on re-interpreting the error code. If
> kvm_arm_set_default_pmu() returns an error, we should return *that*
> particular error, and not any other. Something like:
The code took this shape because I had an issue with returning ENODEV on
the KVM_ARM_VCPU_INIT ioctl, which is not a documented error code.
Now that the vCPU flags are sanitised early in the ioctl, KVM has
decided at this point that vPMU is a supported feature.
Given that, I think ENODEV is fine now as the unexpected return value
would indicate a bug in KVM.
> Hmmm. Contrary to what the commit message says, the default PMU is not
> picked at reset time, but at the point where the target is set (the
> very first vcpu init). Which is pretty different from reset, which
> happens more than once.
>
> I also can't say I'm over the moon with yet another function that does
> a very tiny bit of initialisation outside of the rest of the code that
> performs the vcpu init. Following things is an absolute maze...
I'm fine with this being inlined into __kvm_vcpu_set_target() so long as
we maintain the clear distinction between one-time setup and vCPU reset.
--
Thanks,
Oliver
