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 <gr...@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
signature.asc
Description: PGP signature
