| Message ID | 20200908075830.1161921-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Filtering PMU events | expand |
Hi Marc, On Tue, Sep 08, 2020 at 08:58:26AM +0100, Marc Zyngier wrote: > The PMU emulation error handling is pretty messy when dealing with > attributes. Let's refactor it so that we have less duplication, > and that it is easy to extend later on. > > A functional change is that kvm_arm_pmu_v3_init() used to return > -ENXIO when the PMU feature wasn't set. The error is now reported > as -ENODEV, matching the documentation. Hmm, I didn't think we could make changes like that, since some userspace somewhere may now depend on the buggy interface. That said, I'm not really against the change, but maybe it should go as a separate patch. > -ENXIO is still returned > when the interrupt isn't properly configured. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index f0d0312c0a55..93d797df42c6 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -735,15 +735,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu) > > static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu) > { > - if (!kvm_arm_support_pmu_v3()) > - return -ENODEV; > - > - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) > - return -ENXIO; > - > - if (vcpu->arch.pmu.created) > - return -EBUSY; > - > if (irqchip_in_kernel(vcpu->kvm)) { > int ret; > > @@ -796,6 +787,15 @@ 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()) > + return -ENODEV; > + > + if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) > + return -ENODEV; nit: could combine these two if's w/ an || > + > + if (vcpu->arch.pmu.created) > + return -EBUSY; > + > switch (attr->attr) { > case KVM_ARM_VCPU_PMU_V3_IRQ: { > int __user *uaddr = (int __user *)(long)attr->addr; > @@ -804,9 +804,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) > if (!irqchip_in_kernel(vcpu->kvm)) > return -EINVAL; > > - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) > - return -ENODEV; > - > if (get_user(irq, uaddr)) > return -EFAULT; > > -- > 2.28.0 > Thanks, drew > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
Hi Andrew, On 2020-09-08 10:53, Andrew Jones wrote: > Hi Marc, > > On Tue, Sep 08, 2020 at 08:58:26AM +0100, Marc Zyngier wrote: >> The PMU emulation error handling is pretty messy when dealing with >> attributes. Let's refactor it so that we have less duplication, >> and that it is easy to extend later on. >> >> A functional change is that kvm_arm_pmu_v3_init() used to return >> -ENXIO when the PMU feature wasn't set. The error is now reported >> as -ENODEV, matching the documentation. > > Hmm, I didn't think we could make changes like that, since some > userspace > somewhere may now depend on the buggy interface. Well, this is the whole point of this patch: discussing whether this change is acceptable or whether existing VMMs are relying on such behaviour. We *could* leave it as is, but I thought I'd bring it up! > That said, I'm not really > against the change, but maybe it should go as a separate patch. Sure, why not. >> -ENXIO is still returned >> when the interrupt isn't properly configured. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------ >> 1 file changed, 9 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >> index f0d0312c0a55..93d797df42c6 100644 >> --- a/arch/arm64/kvm/pmu-emul.c >> +++ b/arch/arm64/kvm/pmu-emul.c >> @@ -735,15 +735,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu) >> >> static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu) >> { >> - if (!kvm_arm_support_pmu_v3()) >> - return -ENODEV; >> - >> - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) >> - return -ENXIO; >> - >> - if (vcpu->arch.pmu.created) >> - return -EBUSY; >> - >> if (irqchip_in_kernel(vcpu->kvm)) { >> int ret; >> >> @@ -796,6 +787,15 @@ 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()) >> + return -ENODEV; >> + >> + if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) >> + return -ENODEV; > > nit: could combine these two if's w/ an || This was made to make the userspace visible change obvious to the reviewer. Now that you have noticed it, I'm happy to merge these two! ;-) Thanks, M.
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index f0d0312c0a55..93d797df42c6 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -735,15 +735,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu) static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu) { - if (!kvm_arm_support_pmu_v3()) - return -ENODEV; - - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) - return -ENXIO; - - if (vcpu->arch.pmu.created) - return -EBUSY; - if (irqchip_in_kernel(vcpu->kvm)) { int ret; @@ -796,6 +787,15 @@ 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()) + return -ENODEV; + + if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) + return -ENODEV; + + if (vcpu->arch.pmu.created) + return -EBUSY; + switch (attr->attr) { case KVM_ARM_VCPU_PMU_V3_IRQ: { int __user *uaddr = (int __user *)(long)attr->addr; @@ -804,9 +804,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) if (!irqchip_in_kernel(vcpu->kvm)) return -EINVAL; - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) - return -ENODEV; - if (get_user(irq, uaddr)) return -EFAULT;
The PMU emulation error handling is pretty messy when dealing with attributes. Let's refactor it so that we have less duplication, and that it is easy to extend later on. A functional change is that kvm_arm_pmu_v3_init() used to return -ENXIO when the PMU feature wasn't set. The error is now reported as -ENODEV, matching the documentation. -ENXIO is still returned when the interrupt isn't properly configured. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)