[Qemu-devel] [PATCH] spapr_pci: Unregister listeners before destroying the IOMMU address space

[Qemu-devel] [PATCH] spapr_pci: Unregister listeners before destroying the IOMMU address space

[Greg Kurz]

Hot-unplugging a PHB with a VFIO device connected to it crashes QEMU:

-device spapr-pci-host-bridge,index=1,id=phb1 \
-device vfio-pci,host=0034:01:00.3,id=vfio0

(qemu) device_del phb1
[  357.207183] iommu: Removing device 0001:00:00.0 from group 1
[  360.375523] rpadlpar_io: slot PHB 1 removed
qemu-system-ppc64: memory.c:2742:
 do_address_space_destroy: Assertion `QTAILQ_EMPTY(&as->listeners)' failed.

'as' is the IOMMU address space, which indeed has a listener registered
to by vfio_connect_container() when the VFIO device is realized. This
listener is supposed to be unregistered by vfio_disconnect_container()
when the VFIO device is finalized. Unfortunately, the VFIO device hasn't
reached finalize yet at the time the PHB unrealize function is called,
and address_space_destroy() gets called with the VFIO listener still
being registered.

All regions have just been unmapped from the address space. Listeners
aren't needed anymore at this point. Remove them before destroying the
address space.

The VFIO code will try to remove them _again_ at device finalize,
but it is okay since memory_listener_unregister() is idempotent.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c    |    6 ++++++
 include/exec/memory.h |   10 ++++++++++
 memory.c              |    7 +++++++
 3 files changed, 23 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2dca1e57f36c..eee92b102d5c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1788,6 +1788,12 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
 
     memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
 
+    /*
+     * An attached PCI device may have memory listeners, eg. VFIO PCI. We have
+     * unmapped all sections. Remove the listeners now, before destroying the
+     * address space.
+     */
+    address_space_remove_listeners(&sphb->iommu_as);
     address_space_destroy(&sphb->iommu_as);
 
     qbus_set_hotplug_handler(BUS(phb->bus), NULL, &error_abort);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e6140e8a0489..1ba2e89aa8ce 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1757,6 +1757,16 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
  */
 void address_space_destroy(AddressSpace *as);
 
+/**
+ * address_space_remove_listeners: unregister all listerners of an address space
+ *
+ * Removes all callbacks previously registered with memory_listener_register()
+ * for @as.
+ *
+ * @as: an initialized #AddressSpace
+ */
+void address_space_remove_listeners(AddressSpace *as);
+
 /**
  * address_space_rw: read from or write to an address space.
  *
diff --git a/memory.c b/memory.c
index 0a089a73ae1a..480f3d989b4f 100644
--- a/memory.c
+++ b/memory.c
@@ -2723,6 +2723,13 @@ void memory_listener_unregister(MemoryListener *listener)
     listener->address_space = NULL;
 }
 
+void address_space_remove_listeners(AddressSpace *as)
+{
+    while (!QTAILQ_EMPTY(&as->listeners)) {
+        memory_listener_unregister(QTAILQ_FIRST(&as->listeners));
+    }
+}
+
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_ref(root);

Re: [Qemu-devel] [PATCH] spapr_pci: Unregister listeners before destroying the IOMMU address space

[Alexey Kardashevskiy]

On 21/06/2019 19:27, Greg Kurz wrote:
> Hot-unplugging a PHB with a VFIO device connected to it crashes QEMU:
> 
> -device spapr-pci-host-bridge,index=1,id=phb1 \
> -device vfio-pci,host=0034:01:00.3,id=vfio0
> 
> (qemu) device_del phb1
> [  357.207183] iommu: Removing device 0001:00:00.0 from group 1
> [  360.375523] rpadlpar_io: slot PHB 1 removed
> qemu-system-ppc64: memory.c:2742:
>  do_address_space_destroy: Assertion `QTAILQ_EMPTY(&as->listeners)' failed.
> 
> 'as' is the IOMMU address space, which indeed has a listener registered
> to by vfio_connect_container() when the VFIO device is realized. This
> listener is supposed to be unregistered by vfio_disconnect_container()
> when the VFIO device is finalized. Unfortunately, the VFIO device hasn't
> reached finalize yet at the time the PHB unrealize function is called,
> and address_space_destroy() gets called with the VFIO listener still
> being registered.
> 
> All regions have just been unmapped from the address space. Listeners
> aren't needed anymore at this point. Remove them before destroying the
> address space.
> 
> The VFIO code will try to remove them _again_ at device finalize,
> but it is okay since memory_listener_unregister() is idempotent.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_pci.c    |    6 ++++++
>  include/exec/memory.h |   10 ++++++++++
>  memory.c              |    7 +++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2dca1e57f36c..eee92b102d5c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1788,6 +1788,12 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
>  
>      memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
>  
> +    /*
> +     * An attached PCI device may have memory listeners, eg. VFIO PCI. We have
> +     * unmapped all sections. Remove the listeners now, before destroying the
> +     * address space.
> +     */

The code around the comment (which is slightly incorrect as containers
have listeners, not devices) looks self-explanatory imho.
	

> +    address_space_remove_listeners(&sphb->iommu_as);
>      address_space_destroy(&sphb->iommu_as);
>  
>      qbus_set_hotplug_handler(BUS(phb->bus), NULL, &error_abort);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e6140e8a0489..1ba2e89aa8ce 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1757,6 +1757,16 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>   */
>  void address_space_destroy(AddressSpace *as);
>  
> +/**
> + * address_space_remove_listeners: unregister all listerners of an address space


s/listerners/listeners/g

Otherwise, fixes the bug and

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>




> + *
> + * Removes all callbacks previously registered with memory_listener_register()
> + * for @as.
> + *
> + * @as: an initialized #AddressSpace
> + */
> +void address_space_remove_listeners(AddressSpace *as);
> +
>  /**
>   * address_space_rw: read from or write to an address space.
>   *
> diff --git a/memory.c b/memory.c
> index 0a089a73ae1a..480f3d989b4f 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2723,6 +2723,13 @@ void memory_listener_unregister(MemoryListener *listener)
>      listener->address_space = NULL;
>  }
>  
> +void address_space_remove_listeners(AddressSpace *as)
> +{
> +    while (!QTAILQ_EMPTY(&as->listeners)) {
> +        memory_listener_unregister(QTAILQ_FIRST(&as->listeners));
> +    }
> +}
> +
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
>      memory_region_ref(root);
> 

-- 
Alexey

Re: [Qemu-devel] [PATCH] spapr_pci: Unregister listeners before destroying the IOMMU address space

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

On Fri, Jun 21, 2019 at 11:27:33AM +0200, Greg Kurz wrote:
> Hot-unplugging a PHB with a VFIO device connected to it crashes QEMU:
> 
> -device spapr-pci-host-bridge,index=1,id=phb1 \
> -device vfio-pci,host=0034:01:00.3,id=vfio0
> 
> (qemu) device_del phb1
> [  357.207183] iommu: Removing device 0001:00:00.0 from group 1
> [  360.375523] rpadlpar_io: slot PHB 1 removed
> qemu-system-ppc64: memory.c:2742:
>  do_address_space_destroy: Assertion `QTAILQ_EMPTY(&as->listeners)' failed.
> 
> 'as' is the IOMMU address space, which indeed has a listener registered
> to by vfio_connect_container() when the VFIO device is realized. This
> listener is supposed to be unregistered by vfio_disconnect_container()
> when the VFIO device is finalized. Unfortunately, the VFIO device hasn't
> reached finalize yet at the time the PHB unrealize function is called,
> and address_space_destroy() gets called with the VFIO listener still
> being registered.
> 
> All regions have just been unmapped from the address space. Listeners
> aren't needed anymore at this point. Remove them before destroying the
> address space.
> 
> The VFIO code will try to remove them _again_ at device finalize,
> but it is okay since memory_listener_unregister() is idempotent.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

LGTM, applied.

> ---
>  hw/ppc/spapr_pci.c    |    6 ++++++
>  include/exec/memory.h |   10 ++++++++++
>  memory.c              |    7 +++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2dca1e57f36c..eee92b102d5c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1788,6 +1788,12 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
>  
>      memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
>  
> +    /*
> +     * An attached PCI device may have memory listeners, eg. VFIO PCI. We have
> +     * unmapped all sections. Remove the listeners now, before destroying the
> +     * address space.
> +     */
> +    address_space_remove_listeners(&sphb->iommu_as);
>      address_space_destroy(&sphb->iommu_as);
>  
>      qbus_set_hotplug_handler(BUS(phb->bus), NULL, &error_abort);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e6140e8a0489..1ba2e89aa8ce 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1757,6 +1757,16 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>   */
>  void address_space_destroy(AddressSpace *as);
>  
> +/**
> + * address_space_remove_listeners: unregister all listerners of an address space
> + *
> + * Removes all callbacks previously registered with memory_listener_register()
> + * for @as.
> + *
> + * @as: an initialized #AddressSpace
> + */
> +void address_space_remove_listeners(AddressSpace *as);
> +
>  /**
>   * address_space_rw: read from or write to an address space.
>   *
> diff --git a/memory.c b/memory.c
> index 0a089a73ae1a..480f3d989b4f 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2723,6 +2723,13 @@ void memory_listener_unregister(MemoryListener *listener)
>      listener->address_space = NULL;
>  }
>  
> +void address_space_remove_listeners(AddressSpace *as)
> +{
> +    while (!QTAILQ_EMPTY(&as->listeners)) {
> +        memory_listener_unregister(QTAILQ_FIRST(&as->listeners));
> +    }
> +}
> +
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
>      memory_region_ref(root);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]