| Message ID | 20201113182602.471776-4-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Disabled PMU handling | expand |
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. 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; > } >
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.
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@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
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; + } break; }
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(-)