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@xxxxxxxxxx>
---
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...
