On Tue, Aug 19, 2025 at 09:52:49AM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 18, 2025 at 10:09:11PM -0700, Nicolin Chen wrote:
> > Yes, I've thought about that. The concern is that some other place
> > someday may want to use iommu_get_domain_for_dev() in similar cases
> > but would find that it doesn't work. So it would have to duplicate
> > the domain pointer in its "master" structure.
> >
> > Overall, having a _locked version feels cleaner to me.
>
> We probably need the locked version, but it just shouldn't be called very
> much..
OK. Let's have one patch upgrading the attach_dev to pass in the
old domain pointer (aligning with the SVA version of attach_dev),
and another patch adding the _locked version that then will have
very limited callers.
> > > With sensible internal locking
> >
> > Hmm, I feel this iommu_get_translation_mode() is somewhat the same
> > as the current iommu_get_domain_for_dev(). It would just return the
> > group->domain->type v.s. group->domain, right?
> >
> > This doesn't have any UAF concern though.
>
> Yes, no UAF concern is the point
I see.
> > > So that is another bunch. Not sure what will be left after.
> >
> > I recall that some of the drivers manages their own domains, e.g.
> > drivers/gpu/drm/tegra/drm.c
> >
> > So, they would want more out of the domain pointer than just type.
>
> This looks like it wants an 'is currently attached' operation
That's certainly one of the wide use cases. And I think we could
have an IOMMU_DOMAIN_NONE to fit that into the type helper.
Yet, I also see some other cases that cannot be helped with the
type function. Just listing a few:
1) domain matching (and type)
drivers/gpu/drm/tegra/drm.c:965: if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c:966: domain != tegra->domain)
drivers/gpu/drm/tegra/drm.c-967- return 0;
2) page size
drivers/gpu/drm/arm/malidp_planes.c:307: mmu_dom = iommu_get_domain_for_dev(mp->base.dev->dev);
drivers/gpu/drm/arm/malidp_planes.c-308- if (mmu_dom)
drivers/gpu/drm/arm/malidp_planes.c-309- return mmu_dom->pgsize_bitmap;
3) iommu_iova_to_phys
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2597: dom = iommu_get_domain_for_dev(adev->dev);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2598-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2599- while (size) {
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2600- phys_addr_t addr = *pos & PAGE_MASK;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2601- loff_t off = *pos & ~PAGE_MASK;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2602- size_t bytes = PAGE_SIZE - off;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2603- unsigned long pfn;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2604- struct page *p;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2605- void *ptr;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2606-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2607- bytes = min(bytes, size);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2608-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2609: addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
4) map/unmap
drivers/net/ipa/ipa_mem.c:465: domain = iommu_get_domain_for_dev(dev);
drivers/net/ipa/ipa_mem.c-466- if (!domain) {
drivers/net/ipa/ipa_mem.c-467- dev_err(dev, "no IOMMU domain found for IMEM\n");
drivers/net/ipa/ipa_mem.c-468- return -EINVAL;
drivers/net/ipa/ipa_mem.c-469- }
drivers/net/ipa/ipa_mem.c-470-
drivers/net/ipa/ipa_mem.c-471- /* Align the address down and the size up to page boundaries */
drivers/net/ipa/ipa_mem.c-472- phys = addr & PAGE_MASK;
drivers/net/ipa/ipa_mem.c-473- size = PAGE_ALIGN(size + addr - phys);
drivers/net/ipa/ipa_mem.c-474- iova = phys; /* We just want a direct mapping */
drivers/net/ipa/ipa_mem.c-475-
drivers/net/ipa/ipa_mem.c-476- ret = iommu_map(domain, iova, phys, size, IOMMU_READ | IOMMU_WRITE,
...
drivers/net/ipa/ipa_mem.c:495: domain = iommu_get_domain_for_dev(dev);
drivers/net/ipa/ipa_mem.c-496- if (domain) {
drivers/net/ipa/ipa_mem.c-497- size_t size;
drivers/net/ipa/ipa_mem.c-498-
drivers/net/ipa/ipa_mem.c-499- size = iommu_unmap(domain, ipa->imem_iova, ipa->imem_size);
We could probably invent something for case 1-3. But for case 4,
it feels that iommu_get_domain_for_dev() is the only helper..
Thanks
Nicolin
