virt: kvm: arm: vgic: Process the failure case when kvm_register_device_ops() fails

PATCH Superseded Chen Gang 2 participants 8 messages Nov 13, 2014 – Nov 14, 2014 KVM

Share & tools

Chen Gang

author

2014-11-13 15:04 UTC #0 lore raw

PATCH 1/1 virt: kvm: arm: vgic: Process the failure case when kvm_register_device_ops() fails

When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().

Signed-off-by Chen Gang <[email protected]>

---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +8 -2

index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());

Signature · 1 line

1.9.3

Marc Zyngier

2014-11-13 15:30 UTC #1 lore raw

Quoted 20 lines · Chen Gang

When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().

Signed-off-by Chen Gang <[email protected]>

---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +1 -2

index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,

virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483

Quoted 10 lines

virt/kvm/arm/vgic.c +7 -0

+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());

Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Chen Gang

author

2014-11-14 14:05 UTC #2 lore raw

Quoted 28 lines · Marc Zyngier

Quoted 27 lines · Chen Gang

When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().

Signed-off-by Chen Gang <[email protected]>

---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +8 -2

index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;

Quoted 7 lines

virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 3 lines
virt/kvm/arm/vgic.c +0 -0
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?

OK, thanks. What you said sound reasonable to me. Need call on_each_cpu for disable_percpu_irq(). Also need call __unregister_cpu_notifier(), and need a new function vgic_arch_unsetup() for arm64.

I shall send patch v2 for it.

Thanks.

Signature · 3 lines

Chen Gang

Open share and attitude like air water and life which God blessed

Chen Gang

author

2014-11-14 14:05 UTC #3 lore raw

In reply to Marc Zyngier

Quoted 28 lines · Marc Zyngier

Quoted 27 lines · Chen Gang

When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().

Signed-off-by Chen Gang <[email protected]>

---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +8 -2

index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;

Quoted 7 lines

virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 3 lines
virt/kvm/arm/vgic.c +0 -0
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?

I shall send patch v2 for it.

Thanks.

Signature · 3 lines

Chen Gang

Open share and attitude like air water and life which God blessed

Marc Zyngier

2014-11-14 14:09 UTC #4 lore raw

Quoted 37 lines · Chen Gang

Quoted 28 lines · Marc Zyngier
Quoted 27 lines · Chen Gang
When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().
Signed-off-by Chen Gang <[email protected]>
---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
virt/kvm/arm/vgic.c +8 -2
index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
Quoted 7 lines
virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 3 lines
virt/kvm/arm/vgic.c +0 -0
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?

Quoted 3 lines

OK, thanks. What you said sound reasonable to me. Need call on_each_cpu for disable_percpu_irq(). Also need call __unregister_cpu_notifier(), and need a new function vgic_arch_unsetup() for arm64.

No. Just look at the code. Why don't you just move the kvm_register_device_ops call *before* enabling the interrupt?

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Chen Gang

author

2014-11-14 14:27 UTC #5 lore raw

Quoted 38 lines · Marc Zyngier

Quoted 37 lines · Chen Gang
Quoted 35 lines · Marc Zyngier
Quoted 20 lines · Chen Gang
When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().
Signed-off-by Chen Gang <[email protected]>
---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
virt/kvm/arm/vgic.c +1 -2
index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 10 lines
virt/kvm/arm/vgic.c +7 -0
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?

Quoted 7 lines

Quoted 3 lines
OK, thanks. What you said sound reasonable to me. Need call on_each_cpu for disable_percpu_irq(). Also need call __unregister_cpu_notifier(), and need a new function vgic_arch_unsetup() for arm64.
No. Just look at the code. Why don't you just move the kvm_register_device_ops call *before* enabling the interrupt?

Only based on the current code, what you said is reasonable to me.

But in the normal initializing sequence, firstly for architecture dependence features, then for common cpu features, at last for other devices (at least, other devices need be the last).

So for me, we need still remain current initializing sequence for extensible in the future.

Thanks.

Signature · 3 lines

Chen Gang

Open share and attitude like air water and life which God blessed

Marc Zyngier

2014-11-14 14:53 UTC #6 lore raw

Quoted 47 lines · Chen Gang

Quoted 38 lines · Marc Zyngier
Quoted 37 lines · Chen Gang
Quoted 35 lines · Marc Zyngier
Quoted 20 lines · Chen Gang
When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().
Signed-off-by Chen Gang <[email protected]>
---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
virt/kvm/arm/vgic.c +1 -2
index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 10 lines
virt/kvm/arm/vgic.c +7 -0
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?
Quoted 7 lines
Quoted 3 lines
OK, thanks. What you said sound reasonable to me. Need call on_each_cpu for disable_percpu_irq(). Also need call __unregister_cpu_notifier(), and need a new function vgic_arch_unsetup() for arm64.
No. Just look at the code. Why don't you just move the kvm_register_device_ops call *before* enabling the interrupt?

Quoted 8 lines

Only based on the current code, what you said is reasonable to me.
But in the normal initializing sequence, firstly for architecture dependence features, then for common cpu features, at last for other devices (at least, other devices need be the last).
So for me, we need still remain current initializing sequence for extensible in the future.

Well, the current code is what matters to me, not some hypothetical consideration about how things should (or should not) be.

If you plan to add some code that will require such a refactor, then post the code together with whatever you want to see changed, and we can talk about it.

Until then, I'm not willing to take something that looks over-designed in place of a 4 line fix.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Chen Gang

author

2014-11-14 15:18 UTC #7 lore raw

Quoted 54 lines · Marc Zyngier

Quoted 53 lines · Chen Gang
Quoted 38 lines · Marc Zyngier
Quoted 37 lines · Chen Gang
Quoted 35 lines · Marc Zyngier
Quoted 20 lines · Chen Gang
When kvm_register_device_ops() fails, also need call free_percpu_irq() just like others have down within kvm_vgic_hyp_init().
Signed-off-by Chen Gang <[email protected]>
---
 virt/kvm/arm/vgic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
virt/kvm/arm/vgic.c +1 -2
index 3aaca49..b799f17 100644
@@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
 
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
-	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
-				       KVM_DEV_TYPE_ARM_VGIC_V2);
+	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
virt/kvm/arm/vgic.c int kvm_vgic_hyp_init(void) · line 2483
Quoted 10 lines
virt/kvm/arm/vgic.c +7 -0
+				      KVM_DEV_TYPE_ARM_VGIC_V2);
+	if (ret) {
+		kvm_err("Cannot register device ops\n");
+		goto out_free_irq;
+	}
+
+	return 0;
 
 out_free_irq:
 	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
Awesome. You're now freeing a per-cpu interrupt after just after having enabled it on all CPUs. What could possibly go wrong?
Quoted 7 lines
Quoted 3 lines
OK, thanks. What you said sound reasonable to me. Need call on_each_cpu for disable_percpu_irq(). Also need call __unregister_cpu_notifier(), and need a new function vgic_arch_unsetup() for arm64.
No. Just look at the code. Why don't you just move the kvm_register_device_ops call *before* enabling the interrupt?
Only based on the current code, what you said is reasonable to me.
But in the normal initializing sequence, firstly for architecture dependence features, then for common cpu features, at last for other devices (at least, other devices need be the last).

Quoted 6 lines

Quoted 2 lines
So for me, we need still remain current initializing sequence for extensible in the future.
Well, the current code is what matters to me, not some hypothetical consideration about how things should (or should not) be.

Different members have different tastes. For me, I want to try to keep original author's taste no touch (try to keep orginal working flow and styles).

Quoted 4 lines

If you plan to add some code that will require such a refactor, then post the code together with whatever you want to see changed, and we can talk about it.

OK, thanks. I will send patch v2 for it, next.

Quoted 3 lines

Until then, I'm not willing to take something that looks over-designed in place of a 4 line fix.

At least, __unregister_cpu_notifier() or kvm_unregister_device_ops() is requited for us to process the related failure (which exceeds 4 lines).

Thanks.

Signature · 3 lines

Chen Gang

Open share and attitude like air water and life which God blessed