Hi Cédric, On 7/1/24 12:14, Cédric Le Goater wrote: > When a VFIO device is hoplugged in a VM using virtio-iommu, IOMMUPciBus > and IOMMUDevice cache entries are created in the .get_address_space() > handler of the machine IOMMU device. However, these entries are never > destroyed, not even when the VFIO device is detached from the machine. > This can lead to an assert if the device is reattached again. > > When reattached, the .get_address_space() handler reuses an > IOMMUDevice entry allocated when the VFIO device was first attached. > virtio_iommu_set_host_iova_ranges() is called later on from the > .set_iommu_device() handler an fails with an assert on 'probe_done' > because the device appears to have been already probed when this is > not the case. > > The IOMMUDevice entry is allocated in pci_device_iommu_address_space() > called from under vfio_realize(), the VFIO PCI realize handler. Since > pci_device_unset_iommu_device() is called from vfio_exitfn(), a sub > function of the PCIDevice unrealize() handler, it seems that the > .unset_iommu_device() handler is the best place to release resources > allocated at realize time. Clear the IOMMUDevice cache entry there to > fix hotplug. > > Fixes: 817ef10da23c ("virtio-iommu: Implement set|unset]_iommu_device() > callbacks") > Signed-off-by: Cédric Le Goater <c...@redhat.com> > --- > hw/virtio/virtio-iommu.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index > 72011d2d11ebf1da343b5924f5514ccfe6b2580d..57f53f0fa79cb34bfb75f80bcb9301b523b2a6ab > 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -467,6 +467,26 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus > *bus, void *opaque, > return &sdev->as; > } > > +static void virtio_iommu_device_clear(VirtIOIOMMU *s, PCIBus *bus, int devfn) > +{ > + IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus); > + IOMMUDevice *sdev; > + > + if (!sbus) { > + return; > + } > + > + sdev = sbus->pbdev[devfn]; > + if (!sdev) { > + return; > + } > + > + g_list_free_full(sdev->resv_regions, g_free); > + sdev->resv_regions = NULL; Besides my previous general comments, I think this is a reasonable fix to get in while implementing something better Feel free to add my
Reviewed-by: Eric Auger <eric.au...@redhat.com> Tested-by: Eric Auger <eric.au...@redhat.com> Thanks Eric > + g_free(sdev); > + sbus->pbdev[devfn] = NULL; > +} > + > static gboolean hiod_equal(gconstpointer v1, gconstpointer v2) > { > const struct hiod_key *key1 = v1; > @@ -708,6 +728,7 @@ virtio_iommu_unset_iommu_device(PCIBus *bus, void > *opaque, int devfn) > } > > g_hash_table_remove(viommu->host_iommu_devices, &key); > + virtio_iommu_device_clear(viommu, bus, devfn); > } > > static const PCIIOMMUOps virtio_iommu_ops = {