Re: [PATCH for-5.2] s390x/pci: Unregister listeners before destroying IOMMU address space

[Thomas Huth <thuth@redhat.com>]

On 16/11/2020 22.42, Matthew Rosato wrote:
> Hot-unplugging a vfio-pci device on s390x causes a QEMU crash:
> 
> qemu-system-s390x: ../softmmu/memory.c:2772:
>  do_address_space_destroy: Assertion `QTAILQ_EMPTY(&as->listeners)' failed.
> 
> In s390, the IOMMU address space is freed during device unplug but the
> associated vfio-pci device may not yet be finalized and therefore may
> still have a listener registered to the IOMMU address space.
> 
> Commit a2166410ad74 ("spapr_pci: Unregister listeners before destroying
> the IOMMU address space") previously resolved this issue for spapr_pci.
> We are now seeing this in s390x; it would seem the possibility for this
> issue was already present but based on a bisect commit 2d24a6466154
> ("device-core: use RCU for list of children of a bus") has now changed
> the timing such that it is now readily reproducible.
> 
> Add logic to ensure listeners are removed before destroying the address
> space.
> 
> Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 48a3be8..e0dc20c 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -732,6 +732,13 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
>  
>      table->iommu[PCI_SLOT(devfn)] = NULL;
>      g_hash_table_destroy(iommu->iotlb);
> +    /*
> +     * An attached PCI device may have memory listeners, eg. VFIO PCI.
> +     * The associated subregion will already have been unmapped in
> +     * s390_pci_iommu_disable in response to the guest deconfigure request.
> +     * Remove the listeners now before destroying the address space.
> +     */
> +    address_space_remove_listeners(&iommu->as);
>      address_space_destroy(&iommu->as);
>      object_unparent(OBJECT(&iommu->mr));
>      object_unparent(OBJECT(iommu));
> 

Looks reasonable to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>