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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5723 bytes --]

On Wed, May 03, 2023 at 10:45:11AM -0300, Jason Gunthorpe wrote:
> On Wed, May 03, 2023 at 01:01:34PM +0100, Robin Murphy wrote:
> > On 2023-05-03 12:01, Jason Gunthorpe wrote:
> > > On Wed, May 03, 2023 at 10:17:29AM +0100, Robin Murphy wrote:
> > > > On 2023-05-01 19:02, Jason Gunthorpe wrote:
> > > > > tegra-gart seems to be kind of wonky since from the start its 'detach_dev'
> > > > > op doesn't actually touch hardware. It is supposed to empty the GART of
> > > > > all translations loaded into it.
> > > > 
> > > > No, detach should never tear down translations - what if other devices are
> > > > still using the domain?
> > > 
> > > ?? All other drivers do this.
> > 
> > The only driver I'm aware of which effectively tore down mappings by freeing
> > its pagetable on detach was sprd-iommu, and that was recently fixed on
> > account of it being clearly wrong.
> 
> By "Teardown" I mean deconfigure the HW.
> 
> This driver is odd because it doesn't store a page table in the
> iommu_domain, it keeps it in the GART registers so it can't actually
> detach/attach fully correctly. :(
> 
> > Yes, I'm not disputing that we expect detach to remove that device's
> > *access* to the IOVA (which is what GART can't do...), but it should
> > absolutely not destroy the IOVA mapping itself. Follow that sequence with
> > iommu_attach_device(dom, dev) again and the caller can expect to be able to
> > continue using the same translation.
> 
> Yes
>  
> > > If the HW is multi-device then it is supposed to have groups.
> > 
> > Groups are in fact the most practical example: set up a VFIO domain, attach
> > two groups to it, map some IOVAs, detach one of the groups, keep using the
> > other. If the detach carried an implicit iommu_unmap() there would be
> > fireworks.
> 
> Yes, I'm not saying an unmap, I used the word teardown to mean remove
> the HW parts. This gart function doesn't touch the HW at all, that
> cannot be correct.
> 
> It should have an xarray in the iommu_domain and on detach it should
> purge the GART registers and on attach it should load the xarray into
> the GART registers. We are also technically expecting drivers to
> support map prior to attach, eg for the direct map reserved region
> setup.
> 
> > > Oh yuk, that is not an UNMANAGED domain either as we now assume empty
> > > UNMANAGED domains are blocking in the core...
> > 
> > They are, in the sense that accesses within the aperture won't go
> > anywhere.
> 
> That is not the definition of BLOCKING we came up with.. It is every
> IOVA is blocked and the device is safe to hand to VFIO. It can't be just
> blocking a subset of the IOVA.
> 
> > It might help if domain->geometry.force_aperture was meaningful, because
> > it's never been clear whether it was supposed to reflect a hardware
> > capability (in which case it should be false for GART) or be an instruction
> > to the user of the domain (wherein it's a bit pointless that everyone always
> > sets it).
> 
> force_aperture looks pointless now. Only two drivers don't set it -
> mtk_v1 and sprd.
> 
> The only real reader is dma-iommu.c and mtk_v1 doesn't use that.
> 
> So the only possible user is sprd.
> 
> 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.

Drivers can obviously still narrow down the DMA mask further if they
have any specific needs. On Tegra, for example, we use this to enforce
bus-level DMA masks. The Ethernet controller for instance might support
40 bit addresses, but the memory bus has a quirk where bit 39 is used
for extra "swizzling", so we have to restrict DMA masks to 39 bits for
all devices, regardless of what the drivers claim.

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

> 
> Probably OF dma-ranges should be reflected in the pre-device reserved
> ranges?
> 
> This is great, I was starting to look at this part wishing the OF path
> wasn't different, and this is a clear way forward :)
> 
> For GART, I'm tempted to give GART a blocking domain and just have its
> attach always fail - this is enough to block VFIO. Keep the weirdness
> in one place.. Or ignore it since I doubt anyone is actually using
> this now.

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.

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]