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

[Jag Raman]

> On Apr 25, 2022, at 5:31 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> 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.

OK, will add this comment.

> 
>> +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.

OK, got it.

Thanks!
--
Jag

> 
> 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;
>> +    }
>> +}