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