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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Andy Gross <agross@kernel.org>,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Bjorn Andersson <andersson@kernel.org>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Baolin Wang <baolin.wang@linux.alibaba.com>,
	Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
	Heiko Stuebner <heiko@sntech.de>,
	iommu@lists.linux.dev, Jernej Skrabec <jernej.skrabec@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Joerg Roedel <joro@8bytes.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-s390@vger.kernel.org,
	linux-samsung-soc@vger.kernel.org, linux-sunxi@lists.linux.dev,
	linux-tegra@vger.kernel.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Orson Zhai <orsonzhai@gmail.com>, Rob Clark <robdclark@gmail.com>,
	Samuel Holland <samuel@sholland.org>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Krishna Reddy <vdumpa@nvidia.com>, Chen-Yu Tsai <wens@csie.org>,
	Will Deacon <will@kernel.org>, Yong Wu <yong.wu@mediatek.com>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Steven Price <steven.price@arm.com>
Subject: Re: [PATCH 02/20] iommu/terga-gart: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM
Date: Wed, 3 May 2023 10:45:11 -0300	[thread overview]
Message-ID: <ZFJlZ03lswl9uHD0@nvidia.com> (raw)
In-Reply-To: <1a995f30-31fe-354f-ddfe-e944fa36e7a0@arm.com>

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