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

[Alexandru Elisei]

Hi Marc,

On 11/26/20 3:25 PM, Marc Zyngier wrote:
> 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 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.
>
> 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?

I don't think so, it also looks harmless to me. When it's called on the VCPU init
path, there will be no perf_events, so that part will be skipped. On the reset
path, PMCR_EL0.N will have been initialized so we end up with the correct number
of counters. In both cases vcpu->arch.pmu.chained will be zero'ed

But I find it strange to reset the PMU before doing any checks and before setting
the VCPU register value it reads.

I am thinking that even though at the moment it's harmless, in the future the
function might change and I don't think whoever modifies it will expect the
function to be called like this. But I guess if we're vigilant enough we can
prevent that hypothetical situation from happening.

Thanks,
Alex