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

[Robin Murphy <robin.murphy@arm.com>]

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.

Remember that the GART registers here are literally just its pagetable, 
nothing more.

> The core contract is that this sequence:
> 
>     dom = iommu_domain_alloc()
>     iommu_attach_device(dom, dev)
>     iommu_map(dom,...)
>     iommu_detach_device(dom, dev)
> 
> Will not continue to have the IOVA mapped to the device. We rely on
> this for various error paths.

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.

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

>>> Call this weirdness PLATFORM which keeps the basic original
>>> ops->detach_dev() semantic alive without needing much special core code
>>> support. I'm guessing it really ends up in a BLOCKING configuration, but
>>> without any forced cleanup it is unsafe.
>>
>> The GART translation aperture is in physical address space, so the truth is
>> that all devices have access to it at the same time as having access to the
>> rest of physical address space. Attach/detach here really are only
>> bookkeeping for which domain currently owns the aperture.
> 
> 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. 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).

Thanks,
Robin.