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:

quoted

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.

quoted

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