Re: [PATCH 02/20] iommu/terga-gart: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, May 03, 2023 at 04:43:53PM +0200, Thierry Reding wrote:

> > The only thing it does is cause dma-iommu.c in ARM64 to use the
> > dma-ranges from OF instead of the domain aperture. sprd has no
> > dma-ranges in arch/arm64/boot/dts/sprd.
> > 
> > Further, sprd hard fails any map attempt outside the aperture, so it
> > looks like a bug if the OF somehow chooses a wider aperture as
> > dma-iommu.c will start failing maps.
> 
> That all sounds odd. of_dma_configure_id() already sets up the DMA mask
> based on dma-ranges and the DMA API uses that to restrict what IOVA any
> buffers can get mapped to for a given device.

Yes, and after it sets up the mask it also passes that range down like this:

 of_dma_configure_id():
	end = dma_start + size - 1;
	mask = DMA_BIT_MASK(ilog2(end) + 1);
	dev->coherent_dma_mask &= mask;

	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

Which eventually goes to:

 iommu_setup_dma_ops()
 iommu_dma_init_domain()

Which then does:

	if (domain->geometry.force_aperture) {

And if not set uses the dma_start/size parameter as the actual
aperture. (!?)

Since sprd does this in the iommu driver:

	dom->domain.geometry.aperture_start = 0;
	dom->domain.geometry.aperture_end = SZ_256M - 1;

And it is serious about enforcing it during map:

	unsigned long start = domain->geometry.aperture_start;
	unsigned long end = domain->geometry.aperture_end;

	if (iova < start || (iova + size) > (end + 1)) {
			return -EINVAL;

We must see the dma_start/size parameter be a subset of the aperture
or eventually dma-iommu.c will see map failures.

I can't see how this is can happen, so it looks like omitting
force_aperture is a mistake not a deliberate choice. I'll make a patch
and see if the SPRD folks have any comment. If there is no reason I
can go ahead and purge force_aperture and all the dma_start/size
passing through arch_setup_dma_ops().

> > Thus, I propose we just remove the whole thing. All drivers must set
> > an aperture and the aperture is the pure HW capability to map an
> > IOPTE at that address. ie it reflects the design of the page table
> > itself and nothing else.
> 
> Yeah, that sounds reasonable. If the aperture represents what the IOMMU
> supports. Together with each device's DMA mask we should have everything
> we need.

Arguably we should respect the dma-ranges as well, but I think that
should come up from the iommu driver via the get_resv_regions() which
is the usual place we return FW originated information.

> For Tegra GART I think there's indeed no use-cases at the moment. Dmitry
> had at one point tried to make use of it because it can be helpful on
> some of the older devices that were very memory-constrained. That
> support never made it upstream because it required significant changes
> in various places, if I recall correctly. For anything with a decent
> enough amount of RAM, CMA is usually a better option.

So the actual use case of this HW is to linearize buffers? ie it is a
general scatter/gather engine?

> This has occasionally come up in the past and I seem to remember that it
> had once been proposed to simply remove tegra-gart and there had been no
> objections. Adding Dmitry, if he doesn't have objections to remaving it,
> neither do I.

Dmitry, please say yes and I will remove it instead of trying to carry
it. The driver is almost 10 years old at this point, I'm skeptical
anyone will need it on a 6.2 era kernel..

Thanks,
Jason