Re: [PATCH v8 02/13] KVM: arm64: PMU: Set the default PMU for the guest before vCPU reset

[Oliver Upton]

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