Re: [PATCH v8 12/17] vfio-user: IOMMU support for remote device

[Stefan Hajnoczi]

On Tue, Apr 19, 2022 at 04:44:17PM -0400, Jagannathan Raman wrote:
> +static AddressSpace *remote_iommu_find_add_as(PCIBus *pci_bus,
> +                                              void *opaque, int devfn)
> +{
> +    RemoteIommu *iommu = opaque;
> +    RemoteIommuElem *elem = NULL;
> +
> +    qemu_mutex_lock(&iommu->lock);
> +
> +    elem = g_hash_table_lookup(iommu->elem_by_devfn, INT2VOIDP(devfn));
> +
> +    if (!elem) {
> +        elem = g_malloc0(sizeof(RemoteIommuElem));
> +        g_hash_table_insert(iommu->elem_by_devfn, INT2VOIDP(devfn), elem);
> +    }
> +
> +    if (!elem->mr) {
> +        elem->mr = MEMORY_REGION(object_new(TYPE_MEMORY_REGION));
> +        memory_region_set_size(elem->mr, UINT64_MAX);
> +        address_space_init(&elem->as, elem->mr, NULL);
> +    }
> +
> +    qemu_mutex_unlock(&iommu->lock);
> +
> +    return &elem->as;
> +}

A few comments that can be added to this file to explain the design:

- Each vfio-user server handles one PCIDevice on a PCIBus. There is one
  RemoteIommu per PCIBus, so the RemoteIommu tracks multiple PCIDevices
  by maintaining a ->elem_by_devfn mapping.

- memory_region_init_iommu() is not used because vfio-user MemoryRegions
  will be added to the elem->mr container instead. This is more natural
  than implementing the IOMMUMemoryRegionClass APIs since vfio-user
  provides something that is close to a full-fledged MemoryRegion and
  not like an IOMMU mapping.

- When a device is hot unplugged, the elem->mr reference is dropped so
  all vfio-user MemoryRegions associated with this vfio-user server are
  destroyed.

> +static void remote_iommu_finalize(Object *obj)
> +{
> +    RemoteIommu *iommu = REMOTE_IOMMU(obj);
> +
> +    qemu_mutex_destroy(&iommu->lock);
> +
> +    if (iommu->elem_by_devfn) {

->init() and ->finalize() are a pair, so I don't think ->finalize() will
ever be called with a NULL ->elem_by_devfn.

If ->elem_by_devfn can be NULL then there would probably need to be a
check around qemu_mutex_destroy(&iommu->lock) too.

> +        g_hash_table_destroy(iommu->elem_by_devfn);
> +        iommu->elem_by_devfn = NULL;
> +    }
> +}

signature.asc
Description: PGP signature