| Message ID | 20240701101453.203985-1-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged | expand |
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. This patch fixes the assert on hotplug/hotunplug/hotplug which is definitively needed. However the deallocation issue also exists for other devices (for instance a virtio-net device) where pbdev[devfn] is also leaked. I see Intel IOMMU uses a hash table indexeed by both the bus and devfn. This would allow to avoid leaking devfns on unrealize. I think we need that change both on virtio-iommu and smmu. Besides you could imagine to unplug a virtio-net device and plug a vfio device on same BDF. In such as case unplugging the virtio-net device won't deallocate the IOMMUDevice and the first device will be leaked. In mid term we really would need something symetrical to the get_address_space but it is unclear to me where we should call it. Michael, do you have any advice? Thanks Eric > > 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 <clg@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; I am not sure the above is requested > + 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 = {
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 <clg@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.auger@redhat.com> Tested-by: Eric Auger <eric.auger@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 = {
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; + 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 = {
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 <clg@redhat.com> --- hw/virtio/virtio-iommu.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)