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