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

[Oliver Upton]

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.

-- 
Thanks,
Oliver