[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] spapr_pci: Unregister listeners before destroying
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-ppc] [PATCH] spapr_pci: Unregister listeners before destroying the IOMMU address space |
Date: |
Mon, 24 Jun 2019 17:20:32 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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 <address@hidden>
> ---
> 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 <address@hidden>
> + *
> + * 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