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:
signature.asc
Description: PGP signature
