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

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On 2020-11-26 14:59, Alexandru Elisei wrote:
> 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@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.

I'm not sure it actually matters. Here's my rational:

- PMU support not compiled in: no problem!
- PMU support compiled in, but no HW PMU: we just reset some state to 0, 
no harm done
- HW PMU, but no KVM PMU for this vcpu: same thing
- HW PMU, and KVM PMU: we do the right thing!

Am I missing anything?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel