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

[Baolu Lu]

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@xxxxxxxxxxxxx/

Best regards,
baolu