Skip to contentSkip to conversation

vfio: call irq_bypass_unregister_producer() before freeing irq

PATCH In review Jiang Yi 4 participants 4 messages Nov 27, 2019 – Dec 2, 2019

Subsystems (2)
KVM VFIO driver
lore
Share & tools
lore

Jiang Yi

author
#0 lore raw

PATCH 1/1 vfio: call irq_bypass_unregister_producer() before freeing irq

Since irq_bypass_register_producer() is called after request_irq(), we should do tear-down in reverse order: irq_bypass_unregister_producer() then free_irq().

Signed-off-by Jiang Yi <[email protected]>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci_intrs.c +1 -1
index 3fa3f728fb39..2056f3f85f59 100644
@@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 	int irq, ret;
 
 	if (vector < 0 || vector >= vdev->num_ctx)
 		return -EINVAL;
 
 	irq = pci_irq_vector(pdev, vector);
 
 	if (vdev->ctx[vector].trigger) {
-		free_irq(irq, vdev->ctx[vector].trigger);
 		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
+		free_irq(irq, vdev->ctx[vector].trigger);
 		kfree(vdev->ctx[vector].name);
 		eventfd_ctx_put(vdev->ctx[vector].trigger);
 		vdev->ctx[vector].trigger = NULL;
 	}
 
 	if (fd < 0)
 		return 0;
Signature · 1 line
2.17.1
Reviewed-by on v1 1/1
#1 lore raw

On Wed, 27 Nov 2019 16:49:10 +0000,
Jiang Yi <[email protected]> wrote:

Hi Jiang,

Thanks for spotting this!

Quoted 3 lines

Since irq_bypass_register_producer() is called after request_irq(), we should do tear-down in reverse order: irq_bypass_unregister_producer() then free_irq().

More importantly, free_irq() is going to releases resources that can still be required by the del_producer callback. Notably, for arm64 and GICv4:

free_irq(irq)
__free_irq(irq)
irq_domain_deactivate_irq(irq)
its_irq_domain_deactivate()
[unmap the VLPI from the ITS]

kvm_arch_irq_bypass_del_producer(cons, prod)
kvm_vgic_v4_unset_forwarding(kvm, irq, ...)
its_unmap_vlpi(irq)
[Unmap the VLPI from the ITS (again), remap the original LPI]

which isn't great, and has the potential to wedge the HW. Reversing the two makes more sense: Unmap the VLPI, remap the LPI, and finally unmap the LPI. I haven't checked what it does with VT-D.

Quoted 33 lines
Signed-off-by Jiang Yi <[email protected]>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci_intrs.c +1 -1
index 3fa3f728fb39..2056f3f85f59 100644
@@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 	int irq, ret;
 
 	if (vector < 0 || vector >= vdev->num_ctx)
 		return -EINVAL;
 
 	irq = pci_irq_vector(pdev, vector);
 
 	if (vdev->ctx[vector].trigger) {
-		free_irq(irq, vdev->ctx[vector].trigger);
 		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
+		free_irq(irq, vdev->ctx[vector].trigger);
 		kfree(vdev->ctx[vector].name);
 		eventfd_ctx_put(vdev->ctx[vector].trigger);
 		vdev->ctx[vector].trigger = NULL;
 	}
 
 	if (fd < 0)
 		return 0;
Signature · 1 line
2.17.1

FWIW:

Cc [email protected] # v4.4+
Fixes 6d7425f109d26 ("vfio: Register/unregister irq_bypass_producer")
Reviewed-by Marc Zyngier <[email protected]>

Thanks again,

	M.
Signature · 1 line
Jazz is not dead, it just smells funny.
#2 lore raw
Quoted 21 lines · On Wed, 27 Nov 2019 18:20:14 +0000 Marc Zyngier <[email protected]> wrote:

On Wed, 27 Nov 2019 16:49:10 +0000,
Jiang Yi <[email protected]> wrote:

Hi Jiang,

Thanks for spotting this!

Quoted 3 lines

Since irq_bypass_register_producer() is called after request_irq(), we should do tear-down in reverse order: irq_bypass_unregister_producer() then free_irq().

More importantly, free_irq() is going to releases resources that can still be required by the del_producer callback. Notably, for arm64 and GICv4:

free_irq(irq)
__free_irq(irq)
irq_domain_deactivate_irq(irq)
its_irq_domain_deactivate()
[unmap the VLPI from the ITS]

Quoted 8 lines

kvm_arch_irq_bypass_del_producer(cons, prod)
kvm_vgic_v4_unset_forwarding(kvm, irq, ...)
its_unmap_vlpi(irq)
[Unmap the VLPI from the ITS (again), remap the original LPI]

which isn't great, and has the potential to wedge the HW. Reversing the two makes more sense: Unmap the VLPI, remap the LPI, and finally unmap the LPI. I haven't checked what it does with VT-D.

Yep, it seems a lot safer to reverse this but we need to incorporate some of Marc's rationale above into the commit log to justify the stable and fixes tags. Here's an attempt:

--
free_irq() may release resources required by the irqbypass del_producer() callback. Notably on arm64 with GICv4:

free_irq(irq)
__free_irq(irq)
irq_domain_deactivate_irq(irq)
its_irq_domain_deactivate()
[unmap the VLPI from the ITS]

kvm_arch_irq_bypass_del_producer(cons, prod)
kvm_vgic_v4_unset_forwarding(kvm, irq, ...)
its_unmap_vlpi(irq)
[Unmap the VLPI from the ITS (again), remap the original LPI]

This has the potential to wedge hardware. Re-order to free the IRQ after unregistering the irqbypass producer, which also provides the proper mirror of setup ordering.
--

Cc'ing some usual suspects from AMD, Intel, and Power where the kvm_arch_irq_bypass_del_producer() callback is also implemented. Thanks,

Alex

Quoted 44 lines
Quoted 33 lines
Signed-off-by Jiang Yi <[email protected]>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci_intrs.c +1 -1
index 3fa3f728fb39..2056f3f85f59 100644
@@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 	int irq, ret;
 
 	if (vector < 0 || vector >= vdev->num_ctx)
 		return -EINVAL;
 
 	irq = pci_irq_vector(pdev, vector);
 
 	if (vdev->ctx[vector].trigger) {
-		free_irq(irq, vdev->ctx[vector].trigger);
 		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
+		free_irq(irq, vdev->ctx[vector].trigger);
 		kfree(vdev->ctx[vector].name);
 		eventfd_ctx_put(vdev->ctx[vector].trigger);
 		vdev->ctx[vector].trigger = NULL;
 	}
 
 	if (fd < 0)
 		return 0;
Signature · 1 line
2.17.1

FWIW:

Cc [email protected] # v4.4+
Fixes 6d7425f109d26 ("vfio: Register/unregister irq_bypass_producer")
Reviewed-by Marc Zyngier <[email protected]>

Thanks again,

	M.
Reviewed-by on v1 1/1
#3 lore raw
In reply to Jiang Yi

Hi,

Quoted 15 lines · Jiang Yi

Since irq_bypass_register_producer() is called after request_irq(), we should do tear-down in reverse order: irq_bypass_unregister_producer() then free_irq().

Signed-off-by Jiang Yi <[email protected]>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci_intrs.c +0 -0
index 3fa3f728fb39..2056f3f85f59 100644
@@ -289,18 +289,18 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 	int irq, ret;

drivers/vfio/pci/vfio_pci_intrs.c static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, · line 298

Quoted 10 lines
drivers/vfio/pci/vfio_pci_intrs.c +1 -1
 
 	if (vector < 0 || vector >= vdev->num_ctx)
 		return -EINVAL;
 
 	irq = pci_irq_vector(pdev, vector);
 
 	if (vdev->ctx[vector].trigger) {
-		free_irq(irq, vdev->ctx[vector].trigger);
 		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
+		free_irq(irq, vdev->ctx[vector].trigger);

Looks the right way too

Reviewed-by Eric Auger <[email protected]>

May be worth checking it does not alter the x86 posted interrupt setup though. update_pi_irte() gets called. I was concerned about the fact the interrupts may be enabled when doing the unregistration (TBC). The irq_bypass framework offers producer start/stop callbacks that would allow to handle this but nobody use them atm.

Thanks

Eric

Quoted 9 lines
drivers/vfio/pci/vfio_pci_intrs.c +0 -0
 		kfree(vdev->ctx[vector].name);
 		eventfd_ctx_put(vdev->ctx[vector].trigger);
 		vdev->ctx[vector].trigger = NULL;
 	}
 
 	if (fd < 0)
 		return 0;