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.
