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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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