Re: [PATCH 00/20] iommu: Refactoring domain allocation interface

[Baolu Lu <baolu.lu@linux.intel.com>]

On 5/31/24 1:59 AM, Robin Murphy wrote:
> On 29/05/2024 6:32 am, Lu Baolu wrote:
>> The IOMMU subsystem has undergone some changes, including the removal
>> of iommu_ops from the bus structure. Consequently, the existing domain
>> allocation interface, which relies on a bus type argument, is no longer
>> relevant:
>>
>>      struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>>
>> This series is designed to refactor the use of this interface. It
>> proposes two new interfaces to replace iommu_domain_alloc():
>>
>> - iommu_user_domain_alloc(): This interface is intended for allocating
>>    iommu domains managed by userspace for device passthrough scenarios,
>>    such as those used by iommufd, vfio, and vdpa. It clearly indicates
>>    that the domain is for user-managed device DMA.
>>
>>    If an IOMMU driver does not implement iommu_ops->domain_alloc_user,
>>    this interface will rollback to the generic paging domain allocation.
>>
>> - iommu_paging_domain_alloc(): This interface is for allocating iommu
>>    domains managed by kernel drivers for kernel DMA purposes. It takes a
>>    device pointer as a parameter, which better reflects the current
>>    design of the IOMMU subsystem.
>>
>> The majority of device drivers currently using iommu_domain_alloc() do
>> so to allocate a domain for a specific device and then attach that
>> domain to the device. These cases can be straightforwardly migrated to
>> the new interfaces.
> 
> Ooh, nice! This was rising back up my to-do list as well, but I concur 
> it's rather more straightforward than my version that did devious things 
> to keep the iommu_domain_alloc() name...
> 
>> However, there are some drivers with more complex use cases that do
>> not fit neatly into this new scheme. For example:
>>
>> $ git grep "= iommu_domain_alloc"
>> arch/arm/mm/dma-mapping.c:      mapping->domain = 
>> iommu_domain_alloc(bus);
> 
> This one's simple enough, the refactor just needs to go one step deeper. 
> I've just rebased and pushed my old patch for that, if you'd like it [1].

Great! With this change, we can safely replace iommu_domain_alloc().

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 52f9c56cc3cb..88c2d68a69c9 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1585,9 +1585,11 @@ arm_iommu_create_mapping(struct device *dev, 
dma_addr_t base, u64 size)

         spin_lock_init(&mapping->lock);

-       mapping->domain = iommu_domain_alloc(dev->bus);
-       if (!mapping->domain)
+       mapping->domain = iommu_paging_domain_alloc(dev);
+       if (IS_ERR(mapping->domain)) {
+               err = PTR_ERR(mapping->domain);
                 goto err4;
+       }

         kref_init(&mapping->kref);
         return mapping;

> 
>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    private->domain = 
>> iommu_domain_alloc(private->iommu_dev->bus);
> 
> Both this one and usnic_uiom_alloc_pd() should be OK - back when I did 
> all the figuring out to clean up iommu_present(), I specifically 
> reworked them into "dev->bus" style as a reminder that it *is* supposed 
> to be the right device for doing this with, even if the attach is a bit 
> more distant.

Yeah! I will cleanup these two in the next version.

> 
>> drivers/gpu/drm/tegra/drm.c:            tegra->domain = 
>> iommu_domain_alloc(&platform_bus_type);
> 
> This is the tricky one, where the device to hand may *not* be the right 
> device for IOMMU API use [2]. FWIW my plan was to pull the "walk the 
> platform bus to find any IOMMU-mapped device" trick into this code and 
> use it both to remove the final iommu_present() and for a device-based 
> domain allocation.

I am not familiar with this driver, so the solution you mentioned above
is the best option I can think of for now. I will incorporate this into
the next version.

> 
>> drivers/infiniband/hw/usnic/usnic_uiom.c:       pd->domain = domain = 
>> iommu_domain_alloc(dev->bus);
>>
>> This series leave those cases unchanged and keep iommu_domain_alloc()
>> for their usage. But new drivers should not use it anymore.
> 
> I'd certainly be keen for it to be gone ASAP, since I'm seeing 
> increasing demand for supporting multiple IOMMU drivers, and this is the 
> last bus-based thing standing in the way of that.

Agreed. With all iommu_domain_alloc() removed, iommu_domain_alloc()
could be dropped.

> 
> Thanks,
> Robin.
> 
> [1] 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commit/f048cc6a323d8641898025ca96071df7cbe8bd52
> [2] 
> https://lore.kernel.org/linux-iommu/add31812-50d5-6cb0-3908-143c523abd37@collabora.com/

Best regards,
baolu