Re: [PATCH] virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged

[Eric Auger]

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 = {