[PATCH 3/8] KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time

[Alexandru Elisei]

Hi Marc,

On 11/13/20 6:25 PM, Marc Zyngier wrote:
> We accept to configure a PMU when a vcpu is created, even if the
> HW (or the host) doesn't support it. This results in failures
> when attributes get set, which is a bit odd as we should have
> failed the vcpu creation the first place.
>
> Move the check to the point where we check the vcpu feature set,
> and fail early if we cannot support a PMU. This further simplifies
> the attribute handling.
>
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 4 ++--
>  arch/arm64/kvm/reset.c    | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index e7e3b4629864..200f2a0d8d17 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -913,7 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  
>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> -	if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
> +	if (!kvm_vcpu_has_pmu(vcpu))
>  		return -ENODEV;
>  
>  	if (vcpu->arch.pmu.created)
> @@ -1034,7 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>  	case KVM_ARM_VCPU_PMU_V3_FILTER:
> -		if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
> +		if (kvm_vcpu_has_pmu(vcpu))
>  			return 0;
>  	}
>  
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 74ce92a4988c..3e772ea4e066 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -285,6 +285,10 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  			pstate = VCPU_RESET_PSTATE_EL1;
>  		}
>  
> +		if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

This looks correct, but right at the beginning of the function, before this
non-preemptible section, we do kvm_pmu_vcpu_reset(), which is wrong for several
reasons:

- we don't check if the feature flag is set
- we don't check if the hardware supports a PMU
- kvm_pmu_vcpu_reset() relies on __vcpu_sys_reg(vcpu, PMCR_EL0), which is set in
kvm_reset_sys_regs() below when the VCPU is initialized.

This looks to me like a separate issue, I have a patch locally to fix it by moving
kvm_pmu_vcpu_reset() after the non-preemptible section, but it's fine by me if you
want to fold a fix into this patch.

Thanks,

Alex

>  		break;
>  	}
>