[PATCH 00/20] iommu: Make default_domain's mandatory

[PATCH 00/20] iommu: Make default_domain's mandatory

[Jason Gunthorpe]

It has been a long time coming, this series completes the default_domain
transition and makes it so that the core IOMMU code will always have a
non-NULL default_domain for every driver on every
platform. set_platform_dma_ops() turned out to be a bad idea, and so
completely remove it.

This is achieved by changing each driver to either:

1 - Convert the existing (or deleted) ops->detach_dev() into an
    op->attach_dev() of an IDENTITY domain.

    This is based on the theory that the ARM32 HW is able to function when
    the iommu is turned off as so the turned off state is an IDENTITY
    translation.

2 - Use a new PLATFORM domain type. This is a hack to accommodate drivers
    that we don't really know WTF they do. S390 is legitimately using this
    to switch to it's platform dma_ops implementation, which is where the
    name comes from.

3 - Do #1 and force the default domain to be IDENTITY, this corrects
    the tegra-smmu case where even an ARM64 system would have a NULL
    default_domain.

Using this we can apply the rules:

a) ARM_DMA_USE_IOMMU mode always uses either the driver's
   ops->default_domain, ops->def_domain_type(), or an IDENTITY domain.
   All ARM32 drivers provide one of these three options.

b) dma-iommu.c mode uses either the driver's ops->default_domain,
   ops->def_domain_type or the usual DMA API policy logic based on the
   command line/etc to pick IDENTITY/DMA domain types

c) All other arch's (PPC/S390) use ops->default_domain always.

See the patch "Require a default_domain for all iommu drivers" for a
per-driver breakdown.

The conversion broadly teaches a bunch of ARM32 drivers that they can do
IDENTITY domains. There is some educated guessing involved that these are
actual IDENTITY domains. If this turns out to be wrong the driver can be
trivially changed to use a BLOCKING domain type instead. Further, the
domain type only matters for drivers using ARM64's dma-iommu.c mode as it
will select IDENTITY based on the command line and expect IDENTITY to
work. For ARM32 and other arch cases it is purely documentation.

Finally, based on all the analysis in this series, we can purge
IOMMU_DOMAIN_UNMANAGED/DMA constants from most of the drivers. This
greatly simplifies understanding the driver contract to the core
code. IOMMU drivers should not be involved in policy for how the DMA API
works, that should be a core core decision.

The main gain from this work is to remove alot of ARM_DMA_USE_IOMMU
specific code and behaviors from drivers. All that remains in iommu
drivers after this series is the calls to arm_iommu_create_mapping().

This is a step toward removing ARM_DMA_USE_IOMMU.

The IDENTITY domains added to the ARM64 supporting drivers can be tested
by booting in ARM64 mode and enabling CONFIG_IOMMU_DEFAULT_PASSTHROUGH. If
the system still boots then most likely the implementation is an IDENTITY
domain. If not we can trivially change it to BLOCKING or at worst PLATFORM
if there is no detail what is going on in the HW.

I think this is pretty safe for the ARM32 drivers as they don't really
change, the code that was in detach_dev continues to be called in the same
places it was called before.

This follows the prior series:

https://lore.kernel.org/r/0-v4-79d0c229580a+650-iommu_err_unwind_jgg@nvidia.com

This is on github: https://github.com/jgunthorpe/linux/commits/iommu_all_defdom

Jason Gunthorpe (20):
  iommu: Add IOMMU_DOMAIN_PLATFORM
  iommu/terga-gart: Replace set_platform_dma_ops() with
    IOMMU_DOMAIN_PLATFORM
  iommu/s390: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM
  iommu/fsl_pamu: Replace set_platform_dma_ops() with
    IOMMU_DOMAIN_PLATFORM
  iommu: Allow an IDENTITY domain as the default_domain in ARM32
  iommu/exynos: Implement an IDENTITY domain
  iommu/tegra-smmu: Implement an IDENTITY domain
  iommu/tegra-smmu: Support DMA domains in tegra
  iommu/omap: Implement an IDENTITY domain
  iommu/msm: Implement an IDENTITY domain
  iommu/mtk_iommu_v1: Implement an IDENTITY domain
  iommu: Remove ops->set_platform_dma_ops()
  iommu/qcom_iommu: Add an IOMMU_IDENTITIY_DOMAIN
  iommu/ipmmu: Add an IOMMU_IDENTITIY_DOMAIN
  iommu/mtk_iommu: Add an IOMMU_IDENTITIY_DOMAIN
  iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN
  iommu: Require a default_domain for all iommu drivers
  iommu: Add ops->domain_alloc_paging()
  iommu: Convert simple drivers with DOMAIN_DMA to domain_alloc_paging()
  iommu: Convert remaining simple drivers to domain_alloc_paging()

 drivers/iommu/arm/arm-smmu/qcom_iommu.c |  45 +++++++++-
 drivers/iommu/exynos-iommu.c            |  71 ++++++++--------
 drivers/iommu/fsl_pamu_domain.c         |  36 ++++++--
 drivers/iommu/iommu.c                   | 108 +++++++++++++-----------
 drivers/iommu/ipmmu-vmsa.c              |  50 +++++++++--
 drivers/iommu/msm_iommu.c               |  30 +++++--
 drivers/iommu/mtk_iommu.c               |  30 +++++--
 drivers/iommu/mtk_iommu_v1.c            |  28 +++---
 drivers/iommu/omap-iommu.c              |  28 ++++--
 drivers/iommu/rockchip-iommu.c          |  26 +-----
 drivers/iommu/s390-iommu.c              |  28 ++++--
 drivers/iommu/sprd-iommu.c              |   7 +-
 drivers/iommu/sun50i-iommu.c            |  30 +++++--
 drivers/iommu/tegra-gart.c              |  37 ++++++--
 drivers/iommu/tegra-smmu.c              |  39 ++++++---
 include/linux/iommu.h                   |  15 +++-
 16 files changed, 407 insertions(+), 201 deletions(-)


base-commit: 91d1e2076e3a796fbf3ec5ddcf5266febc7acb39
-- 
2.40.0

[PATCH 01/20] iommu: Add IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe]

This is an opaque domain type that is an escape hatch for tegra-gart and
fsl_pamu that we can't figure out how to convert into actual BLOCKING or
IDENTITY domains. It is designed to preserve the original
ops->detach_dev() semantic that these drivers were built around.

The PLATFORM domain will be set as the default domain and attached as
normal during probe. The drivers will ignore the initial attach from a
NULL domain to the PLATFORM domain.

After this, the PLATFORM domain's attach_dev will be called whenever we
detach from an UNMANAGED domain (eg for VFIO). This is the same time the
original design would have called op->detach_dev().

Add an ops->default_domain member so drivers can trivially opt into this
mode.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 14 +++++++++++++-
 include/linux/iommu.h |  6 ++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 532383e4e90f05..ba7f38630665b5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1640,6 +1640,17 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 
 	lockdep_assert_held(&group->mutex);
 
+	/*
+	 * Allow legacy drivers to specify the domain that will be the default
+	 * domain. This should always be either an IDENTITY or PLATFORM domain.
+	 * Do not use in new drivers.
+	 */
+	if (bus->iommu_ops->default_domain) {
+		if (req_type)
+			return ERR_PTR(-EINVAL);
+		return bus->iommu_ops->default_domain;
+	}
+
 	if (req_type)
 		return __iommu_group_alloc_default_domain(bus, group, req_type);
 
@@ -1945,7 +1956,8 @@ void iommu_domain_free(struct iommu_domain *domain)
 	if (domain->type == IOMMU_DOMAIN_SVA)
 		mmdrop(domain->mm);
 	iommu_put_dma_cookie(domain);
-	domain->ops->free(domain);
+	if (domain->ops->free)
+		domain->ops->free(domain);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_free);
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7dbdd13d7ce046..ddcad3597c177b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -64,6 +64,7 @@ struct iommu_domain_geometry {
 #define __IOMMU_DOMAIN_DMA_FQ	(1U << 3)  /* DMA-API uses flush queue    */
 
 #define __IOMMU_DOMAIN_SVA	(1U << 4)  /* Shared process address space */
+#define __IOMMU_DOMAIN_PLATFORM	(1U << 5)
 
 /*
  * This are the possible domain-types
@@ -80,6 +81,8 @@ struct iommu_domain_geometry {
  *				  invalidation.
  *	IOMMU_DOMAIN_SVA	- DMA addresses are shared process addresses
  *				  represented by mm_struct's.
+ *	IOMMU_DOMAIN_PLATFORM	- Legacy domain for drivers that do their own
+ *				  dma_api stuff. Do not use in new drivers.
  */
 #define IOMMU_DOMAIN_BLOCKED	(0U)
 #define IOMMU_DOMAIN_IDENTITY	(__IOMMU_DOMAIN_PT)
@@ -90,6 +93,7 @@ struct iommu_domain_geometry {
 				 __IOMMU_DOMAIN_DMA_API |	\
 				 __IOMMU_DOMAIN_DMA_FQ)
 #define IOMMU_DOMAIN_SVA	(__IOMMU_DOMAIN_SVA)
+#define IOMMU_DOMAIN_PLATFORM	(__IOMMU_DOMAIN_PLATFORM)
 
 struct iommu_domain {
 	unsigned type;
@@ -248,6 +252,7 @@ struct iommu_iotlb_gather {
  *                    will be blocked by the hardware.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
+ * @default_domain: If not NULL this will always be set as the default domain.
  */
 struct iommu_ops {
 	bool (*capable)(struct device *dev, enum iommu_cap);
@@ -281,6 +286,7 @@ struct iommu_ops {
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
 	struct module *owner;
+	struct iommu_domain *default_domain;
 };
 
 /**
-- 
2.40.0

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

[Jason Gunthorpe]

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.

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.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/tegra-gart.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a482ff838b5331..09865889ff2480 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -124,11 +124,27 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain,
 	return ret;
 }
 
-static void gart_iommu_set_platform_dma(struct device *dev)
+/*
+ * FIXME: This weird function that doesn't touch the HW, but it is supposed to
+ * zap any current translation from the HW.
+ *
+ * Preserve whatever this was doing in 2011 as basically the same in the
+ * new API. The IOMMU_DOMAIN_PLATFORM's attach_dev function is called at almost
+ * the same times as the old detach_dev.
+ *
+ * I suspect the reality is that the UNMANAGED domain is never actually detached
+ * in real systems, or it is only detached once eveything is already unmapped,
+ * so this could get by this way.
+ */
+static int gart_iommu_platform_attach(struct iommu_domain *identity_domain,
+				      struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct gart_device *gart = gart_handle;
 
+	if (domain == identity_domain || !domain)
+		return 0;
+
 	spin_lock(&gart->dom_lock);
 
 	if (dev_iommu_priv_get(dev) == domain) {
@@ -139,8 +155,18 @@ static void gart_iommu_set_platform_dma(struct device *dev)
 	}
 
 	spin_unlock(&gart->dom_lock);
+	return 0;
 }
 
+static struct iommu_domain_ops gart_iommu_platform_ops = {
+	.attach_dev = gart_iommu_platform_attach,
+};
+
+static struct iommu_domain gart_iommu_platform_domain = {
+	.type = IOMMU_DOMAIN_PLATFORM,
+	.ops = &gart_iommu_platform_ops,
+};
+
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
 	struct iommu_domain *domain;
@@ -267,10 +293,10 @@ static void gart_iommu_sync(struct iommu_domain *domain,
 }
 
 static const struct iommu_ops gart_iommu_ops = {
+	.default_domain = &gart_iommu_platform_domain,
 	.domain_alloc	= gart_iommu_domain_alloc,
 	.probe_device	= gart_iommu_probe_device,
 	.device_group	= generic_device_group,
-	.set_platform_dma_ops = gart_iommu_set_platform_dma,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 	.of_xlate	= gart_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.40.0

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

[Robin Murphy]

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?

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

FWIW I wrote up this patch a while ago, not sure if it needs rebasing 
again...

Thanks,
Robin.

----->8-----
Subject: [PATCH] iommu/tegra-gart: Add default identity domain support

The nature of a GART means that supporting identity domains is as easy
as doing nothing, so bring the Tegra driver into the modern world of
default domains with a trivial implementation. Identity domains are
allowed to exist alongside any explicit domain for the translation
aperture, since they both simply represent regions of the physical
address space with no isolation from each other. As such we'll continue
to do the "wrong" thing with groups to allow that to work, since the
notion of isolation that groups represent is counterproductive to the
GART's established usage model.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  drivers/iommu/tegra-gart.c | 39 +++++++++++++++++++-------------------
  1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index c4136eec1f97..07aa7ea6a306 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -111,7 +111,13 @@ static int gart_iommu_attach_dev(struct 
iommu_domain *domain,

  	spin_lock(&gart->dom_lock);

-	if (gart->active_domain && gart->active_domain != domain) {
+	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+		if (dev_iommu_priv_get(dev)) {
+			dev_iommu_priv_set(dev, NULL);
+			if (--gart->active_devices == 0)
+				gart->active_domain = NULL;
+		}
+	} else if (gart->active_domain && gart->active_domain != domain) {
  		ret = -EINVAL;
  	} else if (dev_iommu_priv_get(dev) != domain) {
  		dev_iommu_priv_set(dev, domain);
@@ -124,28 +130,15 @@ static int gart_iommu_attach_dev(struct 
iommu_domain *domain,
  	return ret;
  }

-static void gart_iommu_set_platform_dma(struct device *dev)
-{
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct gart_device *gart = gart_handle;
-
-	spin_lock(&gart->dom_lock);
-
-	if (dev_iommu_priv_get(dev) == domain) {
-		dev_iommu_priv_set(dev, NULL);
-
-		if (--gart->active_devices == 0)
-			gart->active_domain = NULL;
-	}
-
-	spin_unlock(&gart->dom_lock);
-}
-
  static struct iommu_domain *gart_iommu_domain_alloc(struct device *dev,
  						    unsigned type)
  {
+	static struct iommu_domain identity;
  	struct iommu_domain *domain;

+	if (type == IOMMU_DOMAIN_IDENTITY)
+		return &identity;
+
  	if (type != IOMMU_DOMAIN_UNMANAGED)
  		return NULL;

@@ -162,7 +155,8 @@ static struct iommu_domain 
*gart_iommu_domain_alloc(struct device *dev,
  static void gart_iommu_domain_free(struct iommu_domain *domain)
  {
  	WARN_ON(gart_handle->active_domain == domain);
-	kfree(domain);
+	if (domain->type != IOMMU_DOMAIN_IDENTITY)
+		kfree(domain);
  }

  static inline int __gart_iommu_map(struct gart_device *gart, unsigned 
long iova,
@@ -247,6 +241,11 @@ static struct iommu_device 
*gart_iommu_probe_device(struct device *dev)
  	return &gart_handle->iommu;
  }

+static int gart_iommu_def_domain_type(struct device *dev)
+{
+	return IOMMU_DOMAIN_IDENTITY;
+}
+
  static int gart_iommu_of_xlate(struct device *dev,
  			       struct of_phandle_args *args)
  {
@@ -271,9 +270,9 @@ static const struct iommu_ops gart_iommu_ops = {
  	.domain_alloc	= gart_iommu_domain_alloc,
  	.probe_device	= gart_iommu_probe_device,
  	.device_group	= generic_device_group,
-	.set_platform_dma_ops = gart_iommu_set_platform_dma,
  	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
  	.of_xlate	= gart_iommu_of_xlate,
+	.def_domain_type = gart_iommu_def_domain_type,
  	.default_domain_ops = &(const struct iommu_domain_ops) {
  		.attach_dev	= gart_iommu_attach_dev,
  		.map		= gart_iommu_map,
-- 
2.39.2.101.g768bb238c484.dirty

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

[Jason Gunthorpe]

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

If the HW is multi-device then it is supposed to have groups.

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

> FWIW I wrote up this patch a while ago, not sure if it needs rebasing
> again...

That looks like the same as this patch, just calling the detach dev
behavior IDENTITY. Can do..

Thanks,
Jason

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

[Robin Murphy]

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.

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

[Jason Gunthorpe]

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5723 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Wed, May 03, 2023 at 04:43:53PM +0200, Thierry Reding wrote:

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

Yes, and after it sets up the mask it also passes that range down like this:

 of_dma_configure_id():
	end = dma_start + size - 1;
	mask = DMA_BIT_MASK(ilog2(end) + 1);
	dev->coherent_dma_mask &= mask;

	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);

Which eventually goes to:

 iommu_setup_dma_ops()
 iommu_dma_init_domain()

Which then does:

	if (domain->geometry.force_aperture) {

And if not set uses the dma_start/size parameter as the actual
aperture. (!?)

Since sprd does this in the iommu driver:

	dom->domain.geometry.aperture_start = 0;
	dom->domain.geometry.aperture_end = SZ_256M - 1;

And it is serious about enforcing it during map:

	unsigned long start = domain->geometry.aperture_start;
	unsigned long end = domain->geometry.aperture_end;

	if (iova < start || (iova + size) > (end + 1)) {
			return -EINVAL;

We must see the dma_start/size parameter be a subset of the aperture
or eventually dma-iommu.c will see map failures.

I can't see how this is can happen, so it looks like omitting
force_aperture is a mistake not a deliberate choice. I'll make a patch
and see if the SPRD folks have any comment. If there is no reason I
can go ahead and purge force_aperture and all the dma_start/size
passing through arch_setup_dma_ops().

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

Arguably we should respect the dma-ranges as well, but I think that
should come up from the iommu driver via the get_resv_regions() which
is the usual place we return FW originated information.

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

So the actual use case of this HW is to linearize buffers? ie it is a
general scatter/gather engine?

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

Dmitry, please say yes and I will remove it instead of trying to carry
it. The driver is almost 10 years old at this point, I'm skeptical
anyone will need it on a 6.2 era kernel..

Thanks,
Jason

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

[Dmitry Osipenko]

03.05.2023 20:20, Jason Gunthorpe пишет:
> On Wed, May 03, 2023 at 04:43:53PM +0200, Thierry Reding wrote:
> 
>>> 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.
> 
> Yes, and after it sets up the mask it also passes that range down like this:
> 
>  of_dma_configure_id():
> 	end = dma_start + size - 1;
> 	mask = DMA_BIT_MASK(ilog2(end) + 1);
> 	dev->coherent_dma_mask &= mask;
> 
> 	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
> 
> Which eventually goes to:
> 
>  iommu_setup_dma_ops()
>  iommu_dma_init_domain()
> 
> Which then does:
> 
> 	if (domain->geometry.force_aperture) {
> 
> And if not set uses the dma_start/size parameter as the actual
> aperture. (!?)
> 
> Since sprd does this in the iommu driver:
> 
> 	dom->domain.geometry.aperture_start = 0;
> 	dom->domain.geometry.aperture_end = SZ_256M - 1;
> 
> And it is serious about enforcing it during map:
> 
> 	unsigned long start = domain->geometry.aperture_start;
> 	unsigned long end = domain->geometry.aperture_end;
> 
> 	if (iova < start || (iova + size) > (end + 1)) {
> 			return -EINVAL;
> 
> We must see the dma_start/size parameter be a subset of the aperture
> or eventually dma-iommu.c will see map failures.
> 
> I can't see how this is can happen, so it looks like omitting
> force_aperture is a mistake not a deliberate choice. I'll make a patch
> and see if the SPRD folks have any comment. If there is no reason I
> can go ahead and purge force_aperture and all the dma_start/size
> passing through arch_setup_dma_ops().
> 
>>> 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.
> 
> Arguably we should respect the dma-ranges as well, but I think that
> should come up from the iommu driver via the get_resv_regions() which
> is the usual place we return FW originated information.
> 
>> 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.
> 
> So the actual use case of this HW is to linearize buffers? ie it is a
> general scatter/gather engine?
> 
>> 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.
> 
> Dmitry, please say yes and I will remove it instead of trying to carry
> it. The driver is almost 10 years old at this point, I'm skeptical
> anyone will need it on a 6.2 era kernel..

You probably missed that support for many of 10 years old Tegra2/3
devices was added to kernel during last years.

This GART isn't used by upstream DRM driver, but it's used by downstream
kernel which uses alternative Tegra DRM driver that works better for
older hardware.

If it's too much burden to maintain this driver, then feel free to
remove it and I'll continue maintaining it in downstream myself.
Otherwise I can test your changes if needed.

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

[Jason Gunthorpe]

On Fri, May 12, 2023 at 05:55:23AM +0300, Dmitry Osipenko wrote:

> >> 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.
> > 
> > Dmitry, please say yes and I will remove it instead of trying to carry
> > it. The driver is almost 10 years old at this point, I'm skeptical
> > anyone will need it on a 6.2 era kernel..
> 
> You probably missed that support for many of 10 years old Tegra2/3
> devices was added to kernel during last years.
> 
> This GART isn't used by upstream DRM driver, but it's used by downstream
> kernel which uses alternative Tegra DRM driver that works better for
> older hardware.

It is kernel policy not to carry code to only support out of tree drivers in
mainline, so it should be removed, thanks

Jason

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

[Robin Murphy]

On 2023-05-12 17:49, Jason Gunthorpe wrote:
> On Fri, May 12, 2023 at 05:55:23AM +0300, Dmitry Osipenko wrote:
> 
>>>> 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.
>>>
>>> Dmitry, please say yes and I will remove it instead of trying to carry
>>> it. The driver is almost 10 years old at this point, I'm skeptical
>>> anyone will need it on a 6.2 era kernel..
>>
>> You probably missed that support for many of 10 years old Tegra2/3
>> devices was added to kernel during last years.
>>
>> This GART isn't used by upstream DRM driver, but it's used by downstream
>> kernel which uses alternative Tegra DRM driver that works better for
>> older hardware.
> 
> It is kernel policy not to carry code to only support out of tree drivers in
> mainline, so it should be removed, thanks

Aww, I was literally in the middle of writing a Friday-afternoon patch 
to fix it... still need to build-test, but it's really not looking too 
bad at all:

  drivers/iommu/tegra-gart.c | 53 +++++++++++++++++-----------------
  1 file changed, 27 insertions(+), 26 deletions(-)

After that I was going to clean up the force_aperture confusion. TBH 
I've grown to rather like having this driver around as an exception to 
prove our abstractions and help make sure they make sense - it doesn't 
take much effort to keep it functional, and it's not like there aren't 
plenty of in-tree drivers for hardware even more ancient, obscure and 
less-used than Tegra20. FWIW I have *20*-year-old hardware at home 
running an up-to-date mainline-based distro for a practical purpose, but 
I guess that's considered valid if it says Intel on it? :P

Thanks,
Robin.

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

[Jason Gunthorpe]

On Fri, May 12, 2023 at 07:12:21PM +0100, Robin Murphy wrote:
> On 2023-05-12 17:49, Jason Gunthorpe wrote:
> > On Fri, May 12, 2023 at 05:55:23AM +0300, Dmitry Osipenko wrote:
> > 
> > > > > 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.
> > > > 
> > > > Dmitry, please say yes and I will remove it instead of trying to carry
> > > > it. The driver is almost 10 years old at this point, I'm skeptical
> > > > anyone will need it on a 6.2 era kernel..
> > > 
> > > You probably missed that support for many of 10 years old Tegra2/3
> > > devices was added to kernel during last years.
> > > 
> > > This GART isn't used by upstream DRM driver, but it's used by downstream
> > > kernel which uses alternative Tegra DRM driver that works better for
> > > older hardware.
> > 
> > It is kernel policy not to carry code to only support out of tree drivers in
> > mainline, so it should be removed, thanks
> 
> Aww, I was literally in the middle of writing a Friday-afternoon patch to
> fix it... still need to build-test, but it's really not looking too bad at
> all:

But we still need some kind of core support to accommodate a domain
with a mixture of identity and translation. Seems a bit pointless for
a driver with no in tree user even?

> After that I was going to clean up the force_aperture confusion.

Oh I already made a patch to delete it :)

> TBH I've grown to rather like having this driver around as an
> exception to prove our abstractions and help make sure they make
> sense 

Except nobody else seems to know it is here or really understand how
weird/broken it is compared to the other drivers. We've managed to
ignore the issues, but I wouldn't say it is helping build
abstractions.

If we remove this driver then the iommu subsystem has no HW with a
mixed translation in the iommu_domain and looking forwards I think
that will be the kind of HW people want to build. The remaining
GART-like hardware is in arch code.

> not like there aren't plenty of in-tree drivers for hardware even
> more ancient, obscure and less-used than Tegra20. FWIW I have
> *20*-year-old hardware at home running an up-to-date mainline-based
> distro for a practical purpose, but I guess that's considered valid
> if it says Intel on it? :P

Well, We keep trying to remove stuff across the kernel.. This week I
heard about removing areas of HIGHMEM support, removing ia64 and even
some rumbles that it is time to say good bye to ia32 - apparently it
is now slightly broken. Who knows what will stick in the end but it is
not just ARM.

Stuff tends to get removed when it stands in the way..

On the other hand stuff with no in-tree user should just be summarily
deleted. We have enough problems keeping actual ancient used code
going, the community doesn't need even more work to support some out
of tree stuff.

Jason

[PATCH 03/20] iommu/s390: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe]

This is temporary until the S390 dma-iommu.c conversion is merged.

s390 is actually moving the dma_ops over to platform control.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/s390-iommu.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index fbf59a8db29b11..f0c867c57a5b9b 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -142,14 +142,31 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
-static void s390_iommu_set_platform_dma(struct device *dev)
+/*
+ * Switch control over the IOMMU to S390's internal dma_api ops
+ */
+static int s390_iommu_platform_attach(struct iommu_domain *platform_domain,
+				      struct device *dev)
 {
 	struct zpci_dev *zdev = to_zpci_dev(dev);
 
+	if (!zdev->s390_domain)
+		return 0;
+
 	__s390_iommu_detach_device(zdev);
 	zpci_dma_init_device(zdev);
+	return 0;
 }
 
+static struct iommu_domain_ops s390_iommu_platform_ops = {
+	.attach_dev = s390_iommu_platform_attach,
+};
+
+static struct iommu_domain s390_iommu_platform_domain = {
+	.type = IOMMU_DOMAIN_PLATFORM,
+	.ops = &s390_iommu_platform_ops,
+};
+
 static void s390_iommu_get_resv_regions(struct device *dev,
 					struct list_head *list)
 {
@@ -428,12 +445,12 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
 }
 
 static const struct iommu_ops s390_iommu_ops = {
+	.default_domain = &s390_iommu_platform_domain,
 	.capable = s390_iommu_capable,
 	.domain_alloc = s390_domain_alloc,
 	.probe_device = s390_iommu_probe_device,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
-	.set_platform_dma_ops = s390_iommu_set_platform_dma,
 	.pgsize_bitmap = SZ_4K,
 	.get_resv_regions = s390_iommu_get_resv_regions,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.40.0

Re: [PATCH 03/20] iommu/s390: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Niklas Schnelle]

On Mon, 2023-05-01 at 15:02 -0300, Jason Gunthorpe wrote:
> This is temporary until the S390 dma-iommu.c conversion is merged.

For anyone wanting to accelerate the conversion. Comments, testing and
reviews for the current version of my dma-iommu.c conversion patches is
welcome and the single flush queue thing might even give others
(virtio-iommu?) performance benefits:
https://lore.kernel.org/linux-iommu/20230310-dma_iommu-v8-0-2347dfbed7af@linux.ibm.com/

> 
> s390 is actually moving the dma_ops over to platform control.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/s390-iommu.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index fbf59a8db29b11..f0c867c57a5b9b 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -142,14 +142,31 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>  	return 0;
>  }
>  
> -static void s390_iommu_set_platform_dma(struct device *dev)
> +/*
> + * Switch control over the IOMMU to S390's internal dma_api ops
> + */
> +static int s390_iommu_platform_attach(struct iommu_domain *platform_domain,
> +				      struct device *dev)
>  {
>  	struct zpci_dev *zdev = to_zpci_dev(dev);
>  
> +	if (!zdev->s390_domain)
> +		return 0;
> +
>  	__s390_iommu_detach_device(zdev);
>  	zpci_dma_init_device(zdev);
> +	return 0;
>  }
>  
> +static struct iommu_domain_ops s390_iommu_platform_ops = {
> +	.attach_dev = s390_iommu_platform_attach,
> +};
> +
> +static struct iommu_domain s390_iommu_platform_domain = {
> +	.type = IOMMU_DOMAIN_PLATFORM,
> +	.ops = &s390_iommu_platform_ops,
> +};
> +
>  static void s390_iommu_get_resv_regions(struct device *dev,
>  					struct list_head *list)
>  {
> @@ -428,12 +445,12 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
>  }
>  
>  static const struct iommu_ops s390_iommu_ops = {
> +	.default_domain = &s390_iommu_platform_domain,
>  	.capable = s390_iommu_capable,
>  	.domain_alloc = s390_domain_alloc,
>  	.probe_device = s390_iommu_probe_device,
>  	.release_device = s390_iommu_release_device,
>  	.device_group = generic_device_group,
> -	.set_platform_dma_ops = s390_iommu_set_platform_dma,
>  	.pgsize_bitmap = SZ_4K,
>  	.get_resv_regions = s390_iommu_get_resv_regions,
>  	.default_domain_ops = &(const struct iommu_domain_ops) {

Looks good to me and I think semantically this is a good use of an
IOMMU domain type.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

[PATCH 04/20] iommu/fsl_pamu: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe]

It is not clear what this is actually doing, most likely this is IDENTITY
behavior, but I think there is a chance it is BLOCKING given how the PAMU
stuff is oddly used.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/fsl_pamu_domain.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index bce37229709965..4c65f1adfe7511 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -283,15 +283,28 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
-static void fsl_pamu_set_platform_dma(struct device *dev)
+/*
+ * FIXME: This seems to turn off the iommu HW but it is not obvious what state
+ * it leaves the HW in. This is probably really a BLOCKING or IDENTITY domain.
+ * For now this ensures that the old detach_dev behavior functions about the
+ * same as it always did, and we turn off the IOMMU whenever the UNMANAGED
+ * domain is detached.
+ */
+static int fsl_pamu_platform_attach(struct iommu_domain *platform_domain,
+				    struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain);
+	struct fsl_dma_domain *dma_domain;
 	const u32 *prop;
 	int len;
 	struct pci_dev *pdev = NULL;
 	struct pci_controller *pci_ctl;
 
+	if (domain == platform_domain || !domain)
+		return 0;
+
+	dma_domain = to_fsl_dma_domain(domain);
+
 	/*
 	 * Use LIODN of the PCI controller while detaching a
 	 * PCI device.
@@ -312,8 +325,18 @@ static void fsl_pamu_set_platform_dma(struct device *dev)
 		detach_device(dev, dma_domain);
 	else
 		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
+	return 0;
 }
 
+static struct iommu_domain_ops fsl_pamu_platform_ops = {
+	.attach_dev = fsl_pamu_platform_attach,
+};
+
+static struct iommu_domain fsl_pamu_platform_domain = {
+	.type = IOMMU_DOMAIN_PLATFORM,
+	.ops = &fsl_pamu_platform_ops,
+};
+
 /* Set the domain stash attribute */
 int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu)
 {
@@ -448,11 +471,11 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev)
 }
 
 static const struct iommu_ops fsl_pamu_ops = {
+	.default_domain = &fsl_pamu_platform_domain,
 	.capable	= fsl_pamu_capable,
 	.domain_alloc	= fsl_pamu_domain_alloc,
 	.probe_device	= fsl_pamu_probe_device,
 	.device_group   = fsl_pamu_device_group,
-	.set_platform_dma_ops = fsl_pamu_set_platform_dma,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= fsl_pamu_attach_device,
 		.iova_to_phys	= fsl_pamu_iova_to_phys,
-- 
2.40.0

Re: [PATCH 04/20] iommu/fsl_pamu: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Robin Murphy]

On 2023-05-01 19:02, Jason Gunthorpe wrote:
> It is not clear what this is actually doing, most likely this is IDENTITY
> behavior, but I think there is a chance it is BLOCKING given how the PAMU
> stuff is oddly used.

Logically it has to be identity, since there are no DMA ops interacting 
with this driver (it's not the TCE IOMMU of 
arch/powerpc/kernel/iommu.c), so any device using a kernel driver rather 
than VFIO must be using dma-direct and thus require an identity mapping.

At this point I finally got sufficiently fed up of this driver always 
being the mystery weirdo and tracked down an old QorIQ reference manual, 
and now I have about half an hours' worth of understanding of how the 
PAMU actually works.

Based on that, what setup_liodns() is doing is indeed setting up 
identity for everything initially. It also becomes apparent that it's 
never supported giving a PCI device back to its regular driver after 
using vfio-pci, since an attach/detach cycle will then leave the PPAACE 
invalid and thus DMA blocked. Oh well, that's been broken for 10 years; 
nobody cares. Call it an identity domain and move on.

Thanks,
Robin.

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/fsl_pamu_domain.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
> index bce37229709965..4c65f1adfe7511 100644
> --- a/drivers/iommu/fsl_pamu_domain.c
> +++ b/drivers/iommu/fsl_pamu_domain.c
> @@ -283,15 +283,28 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
>   	return ret;
>   }
>   
> -static void fsl_pamu_set_platform_dma(struct device *dev)
> +/*
> + * FIXME: This seems to turn off the iommu HW but it is not obvious what state
> + * it leaves the HW in. This is probably really a BLOCKING or IDENTITY domain.
> + * For now this ensures that the old detach_dev behavior functions about the
> + * same as it always did, and we turn off the IOMMU whenever the UNMANAGED
> + * domain is detached.
> + */
> +static int fsl_pamu_platform_attach(struct iommu_domain *platform_domain,
> +				    struct device *dev)
>   {
>   	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> -	struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain);
> +	struct fsl_dma_domain *dma_domain;
>   	const u32 *prop;
>   	int len;
>   	struct pci_dev *pdev = NULL;
>   	struct pci_controller *pci_ctl;
>   
> +	if (domain == platform_domain || !domain)
> +		return 0;
> +
> +	dma_domain = to_fsl_dma_domain(domain);
> +
>   	/*
>   	 * Use LIODN of the PCI controller while detaching a
>   	 * PCI device.
> @@ -312,8 +325,18 @@ static void fsl_pamu_set_platform_dma(struct device *dev)
>   		detach_device(dev, dma_domain);
>   	else
>   		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
> +	return 0;
>   }
>   
> +static struct iommu_domain_ops fsl_pamu_platform_ops = {
> +	.attach_dev = fsl_pamu_platform_attach,
> +};
> +
> +static struct iommu_domain fsl_pamu_platform_domain = {
> +	.type = IOMMU_DOMAIN_PLATFORM,
> +	.ops = &fsl_pamu_platform_ops,
> +};
> +
>   /* Set the domain stash attribute */
>   int fsl_pamu_configure_l1_stash(struct iommu_domain *domain, u32 cpu)
>   {
> @@ -448,11 +471,11 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev)
>   }
>   
>   static const struct iommu_ops fsl_pamu_ops = {
> +	.default_domain = &fsl_pamu_platform_domain,
>   	.capable	= fsl_pamu_capable,
>   	.domain_alloc	= fsl_pamu_domain_alloc,
>   	.probe_device	= fsl_pamu_probe_device,
>   	.device_group   = fsl_pamu_device_group,
> -	.set_platform_dma_ops = fsl_pamu_set_platform_dma,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
>   		.attach_dev	= fsl_pamu_attach_device,
>   		.iova_to_phys	= fsl_pamu_iova_to_phys,

Re: [PATCH 04/20] iommu/fsl_pamu: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Jason Gunthorpe]

On Wed, May 03, 2023 at 11:57:59AM +0100, Robin Murphy wrote:
> On 2023-05-01 19:02, Jason Gunthorpe wrote:
> > It is not clear what this is actually doing, most likely this is IDENTITY
> > behavior, but I think there is a chance it is BLOCKING given how the PAMU
> > stuff is oddly used.
> 
> Logically it has to be identity, since there are no DMA ops interacting with
> this driver (it's not the TCE IOMMU of arch/powerpc/kernel/iommu.c), so any
> device using a kernel driver rather than VFIO must be using dma-direct and
> thus require an identity mapping.

Yes, that is the usual argument.. FSL just seems so odd because it has
drivers manipulating the iommu so it wasn't clear to me if those
drivers were the only drivers hooked here or not.

> At this point I finally got sufficiently fed up of this driver always being
> the mystery weirdo and tracked down an old QorIQ reference manual, and now I
> have about half an hours' worth of understanding of how the PAMU actually
> works.

I'm going to post my other FSL patches for the group setup then before
you forget ;)

> Based on that, what setup_liodns() is doing is indeed setting up identity
> for everything initially. It also becomes apparent that it's never supported
> giving a PCI device back to its regular driver after using vfio-pci, since
> an attach/detach cycle will then leave the PPAACE invalid and thus DMA
> blocked. Oh well, that's been broken for 10 years; nobody cares. Call it an
> identity domain and move on.

Okay, easy to do thanks for checking the manual

Jason

[PATCH 05/20] iommu: Allow an IDENTITY domain as the default_domain in ARM32

[Jason Gunthorpe]

Even though dma-iommu.c and CONFIG_ARM_DMA_USE_IOMMU do approximately the
same stuff, the way they relate to the IOMMU core is quiet different.

dma-iommu.c expects the core code to setup an UNMANAGED domain (of type
IOMMU_DOMAIN_DMA) and then configures itself to use that domain. This
becomes the default_domain for the group.

ARM_DMA_USE_IOMMU does not use the default_domain, instead it directly
allocates an UNMANAGED domain and operates it just like an external
driver. In this case group->default_domain is NULL.

Allow iommu drivers to specify a global static identity_domain and, if
present, automatically use this domain as the default_domain when in
ARM_DMA_USE_IOMMU mode.

This allows drivers that implemented default_domain == NULL as an IDENTITY
translation to trivially get a properly labeled non-NULL default_domain on
ARM32 configs.

With this arrangment when ARM_DMA_USE_IOMMU wants to disconnect from the
device the normal detach_domain flow will restore the IDENTITY domain as
the default domain. Overall this makes attach_dev() of the IDENTITY domain
called in the same places as detach_dev().

This effectively migrates these drivers to default_domain mode. For
drivers that support ARM64 they will gain support for the IDENTITY
translation mode for the dma_api and behave in a uniform way.

Drivers use this by setting ops->identity_domain to a static singleton
iommu_domain that implements the identity attach. If the core detects
ARM_DMA_USE_IOMMU mode then it automatically attaches the IDENTITY domain
during probe.

If the driver does not want to support dma_api with translation then it
always sets default_domain to the identity domain and even if IOMMU_DMA is
turned on it will not allow it to be used.

This allows removing the set_platform_dma_ops() from every remaining
driver.

Add the core support and convert rockchip to use it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c          | 13 +++++++++++++
 drivers/iommu/rockchip-iommu.c | 19 +------------------
 include/linux/iommu.h          |  3 +++
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ba7f38630665b5..8b9af774de68f1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1654,6 +1654,16 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 	if (req_type)
 		return __iommu_group_alloc_default_domain(bus, group, req_type);
 
+	/*
+	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
+	 * identity_domain and it becomes their default domain. Later on
+	 * ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
+	    bus->iommu_ops->identity_domain)
+		return __iommu_group_alloc_default_domain(
+			bus, group, IOMMU_DOMAIN_IDENTITY);
+
 	/* The driver gave no guidance on what type to use, try the default */
 	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
 	if (dom)
@@ -1923,6 +1933,9 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 	if (bus == NULL || bus->iommu_ops == NULL)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
+		return bus->iommu_ops->identity_domain;
+
 	domain = bus->iommu_ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ea5a3088bb7e8a..9e1296a856ac4c 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1017,13 +1017,8 @@ static int rk_iommu_identity_attach(struct iommu_domain *identity_domain,
 	return 0;
 }
 
-static void rk_iommu_identity_free(struct iommu_domain *domain)
-{
-}
-
 static struct iommu_domain_ops rk_identity_ops = {
 	.attach_dev = rk_iommu_identity_attach,
-	.free = rk_iommu_identity_free,
 };
 
 static struct iommu_domain rk_identity_domain = {
@@ -1031,13 +1026,6 @@ static struct iommu_domain rk_identity_domain = {
 	.ops = &rk_identity_ops,
 };
 
-#ifdef CONFIG_ARM
-static void rk_iommu_set_platform_dma(struct device *dev)
-{
-	WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev));
-}
-#endif
-
 static int rk_iommu_attach_device(struct iommu_domain *domain,
 		struct device *dev)
 {
@@ -1087,9 +1075,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
 {
 	struct rk_iommu_domain *rk_domain;
 
-	if (type == IOMMU_DOMAIN_IDENTITY)
-		return &rk_identity_domain;
-
 	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
@@ -1214,13 +1199,11 @@ static int rk_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops rk_iommu_ops = {
+	.identity_domain = &rk_identity_domain,
 	.domain_alloc = rk_iommu_domain_alloc,
 	.probe_device = rk_iommu_probe_device,
 	.release_device = rk_iommu_release_device,
 	.device_group = rk_iommu_device_group,
-#ifdef CONFIG_ARM
-	.set_platform_dma_ops = rk_iommu_set_platform_dma,
-#endif
 	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 	.of_xlate = rk_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ddcad3597c177b..427490b5736d40 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -253,6 +253,8 @@ struct iommu_iotlb_gather {
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  * @default_domain: If not NULL this will always be set as the default domain.
+ * @identity_domain: An always available, always attachable identity
+ *                   translation.
  */
 struct iommu_ops {
 	bool (*capable)(struct device *dev, enum iommu_cap);
@@ -287,6 +289,7 @@ struct iommu_ops {
 	unsigned long pgsize_bitmap;
 	struct module *owner;
 	struct iommu_domain *default_domain;
+	struct iommu_domain *identity_domain;
 };
 
 /**
-- 
2.40.0

Re: [PATCH 05/20] iommu: Allow an IDENTITY domain as the default_domain in ARM32

[Robin Murphy]

On 2023-05-01 19:02, Jason Gunthorpe wrote:
> Even though dma-iommu.c and CONFIG_ARM_DMA_USE_IOMMU do approximately the
> same stuff, the way they relate to the IOMMU core is quiet different.
> 
> dma-iommu.c expects the core code to setup an UNMANAGED domain (of type
> IOMMU_DOMAIN_DMA) and then configures itself to use that domain. This
> becomes the default_domain for the group.
> 
> ARM_DMA_USE_IOMMU does not use the default_domain, instead it directly
> allocates an UNMANAGED domain and operates it just like an external
> driver. In this case group->default_domain is NULL.
> 
> Allow iommu drivers to specify a global static identity_domain and, if
> present, automatically use this domain as the default_domain when in
> ARM_DMA_USE_IOMMU mode.
> 
> This allows drivers that implemented default_domain == NULL as an IDENTITY
> translation to trivially get a properly labeled non-NULL default_domain on
> ARM32 configs.
> 
> With this arrangment when ARM_DMA_USE_IOMMU wants to disconnect from the
> device the normal detach_domain flow will restore the IDENTITY domain as
> the default domain. Overall this makes attach_dev() of the IDENTITY domain
> called in the same places as detach_dev().
> 
> This effectively migrates these drivers to default_domain mode. For
> drivers that support ARM64 they will gain support for the IDENTITY
> translation mode for the dma_api and behave in a uniform way.
> 
> Drivers use this by setting ops->identity_domain to a static singleton
> iommu_domain that implements the identity attach. If the core detects
> ARM_DMA_USE_IOMMU mode then it automatically attaches the IDENTITY domain
> during probe.
> 
> If the driver does not want to support dma_api with translation then it
> always sets default_domain to the identity domain and even if IOMMU_DMA is
> turned on it will not allow it to be used.

Could we not retain the use of .def_domain_type for this? I think if we 
can avoid the IOMMU_DOMAIN_PLATFORM thing altogether than that's the 
more appealing option.

> This allows removing the set_platform_dma_ops() from every remaining
> driver.
> 
> Add the core support and convert rockchip to use it.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c          | 13 +++++++++++++
>   drivers/iommu/rockchip-iommu.c | 19 +------------------
>   include/linux/iommu.h          |  3 +++
>   3 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ba7f38630665b5..8b9af774de68f1 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1654,6 +1654,16 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   	if (req_type)
>   		return __iommu_group_alloc_default_domain(bus, group, req_type);
>   
> +	/*
> +	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
> +	 * identity_domain and it becomes their default domain. Later on
> +	 * ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&

For the sake of reasoning, I think just use CONFIG_ARM for this. 
Otherwise we may still end up with potential corner cases of 
default_domain == NULL that we'd rather not have to accommodate.

> +	    bus->iommu_ops->identity_domain)
> +		return __iommu_group_alloc_default_domain(
> +			bus, group, IOMMU_DOMAIN_IDENTITY);

Why not simply return the identity_domain that we now know exists?

It would seem even more logical, however, to put this ARM workaround in 
iommu_get_default_domain_type() and keep the actual allocation path 
clean. Do we strictly need to check for identity_domain up front? (Note 
that as it stands this narrowly misses out on un-breaking arm-smmu for 
32-bit)

Thanks,
Robin.

> +
>   	/* The driver gave no guidance on what type to use, try the default */
>   	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
>   	if (dom)
> @@ -1923,6 +1933,9 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>   	if (bus == NULL || bus->iommu_ops == NULL)
>   		return NULL;
>   
> +	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
> +		return bus->iommu_ops->identity_domain;
> +
>   	domain = bus->iommu_ops->domain_alloc(type);
>   	if (!domain)
>   		return NULL;
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index ea5a3088bb7e8a..9e1296a856ac4c 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1017,13 +1017,8 @@ static int rk_iommu_identity_attach(struct iommu_domain *identity_domain,
>   	return 0;
>   }
>   
> -static void rk_iommu_identity_free(struct iommu_domain *domain)
> -{
> -}
> -
>   static struct iommu_domain_ops rk_identity_ops = {
>   	.attach_dev = rk_iommu_identity_attach,
> -	.free = rk_iommu_identity_free,
>   };
>   
>   static struct iommu_domain rk_identity_domain = {
> @@ -1031,13 +1026,6 @@ static struct iommu_domain rk_identity_domain = {
>   	.ops = &rk_identity_ops,
>   };
>   
> -#ifdef CONFIG_ARM
> -static void rk_iommu_set_platform_dma(struct device *dev)
> -{
> -	WARN_ON(rk_iommu_identity_attach(&rk_identity_domain, dev));
> -}
> -#endif
> -
>   static int rk_iommu_attach_device(struct iommu_domain *domain,
>   		struct device *dev)
>   {
> @@ -1087,9 +1075,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
>   {
>   	struct rk_iommu_domain *rk_domain;
>   
> -	if (type == IOMMU_DOMAIN_IDENTITY)
> -		return &rk_identity_domain;
> -
>   	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>   		return NULL;
>   
> @@ -1214,13 +1199,11 @@ static int rk_iommu_of_xlate(struct device *dev,
>   }
>   
>   static const struct iommu_ops rk_iommu_ops = {
> +	.identity_domain = &rk_identity_domain,
>   	.domain_alloc = rk_iommu_domain_alloc,
>   	.probe_device = rk_iommu_probe_device,
>   	.release_device = rk_iommu_release_device,
>   	.device_group = rk_iommu_device_group,
> -#ifdef CONFIG_ARM
> -	.set_platform_dma_ops = rk_iommu_set_platform_dma,
> -#endif
>   	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
>   	.of_xlate = rk_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ddcad3597c177b..427490b5736d40 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -253,6 +253,8 @@ struct iommu_iotlb_gather {
>    * @pgsize_bitmap: bitmap of all possible supported page sizes
>    * @owner: Driver module providing these ops
>    * @default_domain: If not NULL this will always be set as the default domain.
> + * @identity_domain: An always available, always attachable identity
> + *                   translation.
>    */
>   struct iommu_ops {
>   	bool (*capable)(struct device *dev, enum iommu_cap);
> @@ -287,6 +289,7 @@ struct iommu_ops {
>   	unsigned long pgsize_bitmap;
>   	struct module *owner;
>   	struct iommu_domain *default_domain;
> +	struct iommu_domain *identity_domain;
>   };
>   
>   /**

Re: [PATCH 05/20] iommu: Allow an IDENTITY domain as the default_domain in ARM32

[Jason Gunthorpe]

On Wed, May 03, 2023 at 02:50:02PM +0100, Robin Murphy wrote:

> > If the driver does not want to support dma_api with translation then it
> > always sets default_domain to the identity domain and even if IOMMU_DMA is
> > turned on it will not allow it to be used.
> 
> Could we not retain the use of .def_domain_type for this? I think if we can
> avoid the IOMMU_DOMAIN_PLATFORM thing altogether than that's the more
> appealing option.

We can, but given default_domain is already there this is the simpler
implementation.

If we can avoid platform then I would switch this too..

> > +	/*
> > +	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
> > +	 * identity_domain and it becomes their default domain. Later on
> > +	 * ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
> 
> For the sake of reasoning, I think just use CONFIG_ARM for this. Otherwise
> we may still end up with potential corner cases of default_domain == NULL
> that we'd rather not have to accommodate.

The last patches prevent that directly.

I picked this option because I want to eventually get to the point of
having the special ARM_IOMMU code consolidated and marked using this
ifdef so we can clearly find it all.

> > +	    bus->iommu_ops->identity_domain)
> > +		return __iommu_group_alloc_default_domain(
> > +			bus, group, IOMMU_DOMAIN_IDENTITY);
> 
> Why not simply return the identity_domain that we now know exists?

Yeah at this patch it is true, but the later patches it has to call
the function. I organized it like this to avoid some churn

> It would seem even more logical, however, to put this ARM workaround in
> iommu_get_default_domain_type() and keep the actual allocation path
> clean.

I thought about that too.. It felt it might end up more convoluted but
I can try to darft it and see.

It is still logical here because this is the "use the DMA API policy
default" code path, and the default for ARM32 right now is this.

> Do we strictly need to check for identity_domain up front? 

You mean when registering the driver? It does make sense

> (Note that as it
> stands this narrowly misses out on un-breaking arm-smmu for 32-bit)

Can you elaborate? I don't know about this..

Thanks,
Jason

[PATCH 06/20] iommu/exynos: Implement an IDENTITY domain

[Jason Gunthorpe]

What exynos calls exynos_iommu_detach_device is actually putting the iommu
into identity mode.

Move to the new core support for ARM_DMA_USE_IOMMU by defining
ops->identity_domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/exynos-iommu.c | 64 ++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index c275fe71c4db32..6ff7901103948a 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -24,6 +24,7 @@
 
 typedef u32 sysmmu_iova_t;
 typedef u32 sysmmu_pte_t;
+static struct iommu_domain exynos_identity_domain;
 
 /* We do not consider super section mapping (16MB) */
 #define SECT_ORDER 20
@@ -829,7 +830,7 @@ static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
 		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
 
 		mutex_lock(&owner->rpm_lock);
-		if (data->domain) {
+		if (&data->domain->domain != &exynos_identity_domain) {
 			dev_dbg(data->sysmmu, "saving state\n");
 			__sysmmu_disable(data);
 		}
@@ -847,7 +848,7 @@ static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
 		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
 
 		mutex_lock(&owner->rpm_lock);
-		if (data->domain) {
+		if (&data->domain->domain != &exynos_identity_domain) {
 			dev_dbg(data->sysmmu, "restoring state\n");
 			__sysmmu_enable(data);
 		}
@@ -980,17 +981,22 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 	kfree(domain);
 }
 
-static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
-				    struct device *dev)
+static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
+					struct device *dev)
 {
-	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
 	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
-	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
+	struct exynos_iommu_domain *domain;
+	phys_addr_t pagetable;
 	struct sysmmu_drvdata *data, *next;
 	unsigned long flags;
 
-	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
-		return;
+	if (!owner)
+		return -ENODEV;
+	if (owner->domain == identity_domain)
+		return 0;
+
+	domain = to_exynos_domain(owner->domain);
+	pagetable = virt_to_phys(domain->pgtable);
 
 	mutex_lock(&owner->rpm_lock);
 
@@ -1009,15 +1015,25 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 		list_del_init(&data->domain_node);
 		spin_unlock(&data->lock);
 	}
-	owner->domain = NULL;
+	owner->domain = identity_domain;
 	spin_unlock_irqrestore(&domain->lock, flags);
 
 	mutex_unlock(&owner->rpm_lock);
 
 	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
 		&pagetable);
+	return 0;
 }
 
+static struct iommu_domain_ops exynos_identity_ops = {
+	.attach_dev = exynos_iommu_identity_attach,
+};
+
+static struct iommu_domain exynos_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &exynos_identity_ops,
+};
+
 static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 				   struct device *dev)
 {
@@ -1026,12 +1042,11 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	struct sysmmu_drvdata *data;
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	unsigned long flags;
+	int err;
 
-	if (!has_sysmmu(dev))
-		return -ENODEV;
-
-	if (owner->domain)
-		exynos_iommu_detach_device(owner->domain, dev);
+	err = exynos_iommu_identity_attach(&exynos_identity_domain, dev);
+	if (err)
+		return err;
 
 	mutex_lock(&owner->rpm_lock);
 
@@ -1407,26 +1422,12 @@ static struct iommu_device *exynos_iommu_probe_device(struct device *dev)
 	return &data->iommu;
 }
 
-static void exynos_iommu_set_platform_dma(struct device *dev)
-{
-	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
-
-	if (owner->domain) {
-		struct iommu_group *group = iommu_group_get(dev);
-
-		if (group) {
-			exynos_iommu_detach_device(owner->domain, dev);
-			iommu_group_put(group);
-		}
-	}
-}
-
 static void exynos_iommu_release_device(struct device *dev)
 {
 	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
 	struct sysmmu_drvdata *data;
 
-	exynos_iommu_set_platform_dma(dev);
+	WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
 
 	list_for_each_entry(data, &owner->controllers, owner_node)
 		device_link_del(data->link);
@@ -1457,6 +1458,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 
 		INIT_LIST_HEAD(&owner->controllers);
 		mutex_init(&owner->rpm_lock);
+		owner->domain = &exynos_identity_domain;
 		dev_iommu_priv_set(dev, owner);
 	}
 
@@ -1471,11 +1473,9 @@ static int exynos_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops exynos_iommu_ops = {
+	.identity_domain = &exynos_identity_domain,
 	.domain_alloc = exynos_iommu_domain_alloc,
 	.device_group = generic_device_group,
-#ifdef CONFIG_ARM
-	.set_platform_dma_ops = exynos_iommu_set_platform_dma,
-#endif
 	.probe_device = exynos_iommu_probe_device,
 	.release_device = exynos_iommu_release_device,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
-- 
2.40.0

Re: [PATCH 06/20] iommu/exynos: Implement an IDENTITY domain

[Robin Murphy]

On 2023-05-01 19:02, Jason Gunthorpe wrote:
> What exynos calls exynos_iommu_detach_device is actually putting the iommu
> into identity mode.
> 
> Move to the new core support for ARM_DMA_USE_IOMMU by defining
> ops->identity_domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/exynos-iommu.c | 64 ++++++++++++++++++------------------
>   1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index c275fe71c4db32..6ff7901103948a 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -24,6 +24,7 @@
>   
>   typedef u32 sysmmu_iova_t;
>   typedef u32 sysmmu_pte_t;
> +static struct iommu_domain exynos_identity_domain;
>   
>   /* We do not consider super section mapping (16MB) */
>   #define SECT_ORDER 20
> @@ -829,7 +830,7 @@ static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
>   		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
>   
>   		mutex_lock(&owner->rpm_lock);
> -		if (data->domain) {
> +		if (&data->domain->domain != &exynos_identity_domain) {
>   			dev_dbg(data->sysmmu, "saving state\n");
>   			__sysmmu_disable(data);
>   		}
> @@ -847,7 +848,7 @@ static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
>   		struct exynos_iommu_owner *owner = dev_iommu_priv_get(master);
>   
>   		mutex_lock(&owner->rpm_lock);
> -		if (data->domain) {
> +		if (&data->domain->domain != &exynos_identity_domain) {
>   			dev_dbg(data->sysmmu, "restoring state\n");
>   			__sysmmu_enable(data);
>   		}
> @@ -980,17 +981,22 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
>   	kfree(domain);
>   }
>   
> -static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> -				    struct device *dev)
> +static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
> +					struct device *dev)
>   {
> -	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
>   	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> -	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> +	struct exynos_iommu_domain *domain;
> +	phys_addr_t pagetable;
>   	struct sysmmu_drvdata *data, *next;
>   	unsigned long flags;
>   
> -	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> -		return;
> +	if (!owner)
> +		return -ENODEV;

That can't be true - devices can't be attached without having already 
dereferenced their group, which means they've been through probe_device 
successfully.

> +	if (owner->domain == identity_domain)
> +		return 0;
> +
> +	domain = to_exynos_domain(owner->domain);
> +	pagetable = virt_to_phys(domain->pgtable);

Identity domains by definition shouldn't have a pagetable? I don't think 
virt_to_phys(NULL) can be assumed to be valid or safe in general.

>   
>   	mutex_lock(&owner->rpm_lock);
>   
> @@ -1009,15 +1015,25 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
>   		list_del_init(&data->domain_node);
>   		spin_unlock(&data->lock);
>   	}

This iterates the whole domain->clients list, which may include other 
devices from other groups belonging to other IOMMU instances. I think 
that's technically an issue already given that we support cross-instance 
domain attach here, which the DRM drivers rely on. I can't quite work 
out off-hand if this is liable to make it any worse or not :/

> -	owner->domain = NULL;
> +	owner->domain = identity_domain;
>   	spin_unlock_irqrestore(&domain->lock, flags);
>   
>   	mutex_unlock(&owner->rpm_lock);
>   
>   	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,

This no longer makes much sense.

>   		&pagetable);
> +	return 0;
>   }
>   
> +static struct iommu_domain_ops exynos_identity_ops = {
> +	.attach_dev = exynos_iommu_identity_attach,
> +};
> +
> +static struct iommu_domain exynos_identity_domain = {
> +	.type = IOMMU_DOMAIN_IDENTITY,
> +	.ops = &exynos_identity_ops,
> +};
> +
>   static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>   				   struct device *dev)
>   {
> @@ -1026,12 +1042,11 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>   	struct sysmmu_drvdata *data;
>   	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
>   	unsigned long flags;
> +	int err;
>   
> -	if (!has_sysmmu(dev))
> -		return -ENODEV;
> -
> -	if (owner->domain)
> -		exynos_iommu_detach_device(owner->domain, dev);
> +	err = exynos_iommu_identity_attach(&exynos_identity_domain, dev);
> +	if (err)
> +		return err;
>   
>   	mutex_lock(&owner->rpm_lock);
>   
> @@ -1407,26 +1422,12 @@ static struct iommu_device *exynos_iommu_probe_device(struct device *dev)
>   	return &data->iommu;
>   }
>   
> -static void exynos_iommu_set_platform_dma(struct device *dev)
> -{
> -	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> -
> -	if (owner->domain) {
> -		struct iommu_group *group = iommu_group_get(dev);
> -
> -		if (group) {
> -			exynos_iommu_detach_device(owner->domain, dev);
> -			iommu_group_put(group);
> -		}
> -	}
> -}
> -
>   static void exynos_iommu_release_device(struct device *dev)
>   {
>   	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
>   	struct sysmmu_drvdata *data;
>   
> -	exynos_iommu_set_platform_dma(dev);
> +	WARN_ON(exynos_iommu_identity_attach(&exynos_identity_domain, dev));
>   
>   	list_for_each_entry(data, &owner->controllers, owner_node)
>   		device_link_del(data->link);
> @@ -1457,6 +1458,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
>   
>   		INIT_LIST_HEAD(&owner->controllers);
>   		mutex_init(&owner->rpm_lock);
> +		owner->domain = &exynos_identity_domain;

I think strictly this would be more of a probe_device thing than an 
of_xlate thing, but it's not super-important.

Thanks,
Robin.

>   		dev_iommu_priv_set(dev, owner);
>   	}
>   
> @@ -1471,11 +1473,9 @@ static int exynos_iommu_of_xlate(struct device *dev,
>   }
>   
>   static const struct iommu_ops exynos_iommu_ops = {
> +	.identity_domain = &exynos_identity_domain,
>   	.domain_alloc = exynos_iommu_domain_alloc,
>   	.device_group = generic_device_group,
> -#ifdef CONFIG_ARM
> -	.set_platform_dma_ops = exynos_iommu_set_platform_dma,
> -#endif
>   	.probe_device = exynos_iommu_probe_device,
>   	.release_device = exynos_iommu_release_device,
>   	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,

Re: [PATCH 06/20] iommu/exynos: Implement an IDENTITY domain

[Jason Gunthorpe]

On Wed, May 03, 2023 at 04:31:39PM +0100, Robin Murphy wrote:
> > -				    struct device *dev)
> > +static int exynos_iommu_identity_attach(struct iommu_domain *identity_domain,
> > +					struct device *dev)
> >   {
> > -	struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> >   	struct exynos_iommu_owner *owner = dev_iommu_priv_get(dev);
> > -	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> > +	struct exynos_iommu_domain *domain;
> > +	phys_addr_t pagetable;
> >   	struct sysmmu_drvdata *data, *next;
> >   	unsigned long flags;
> > -	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> > -		return;
> > +	if (!owner)
> > +		return -ENODEV;
> 
> That can't be true - devices can't be attached without having already
> dereferenced their group, which means they've been through probe_device
> successfully.

Yes, the current code is wrong to check has_sysmmu(), I removed it

> > +	if (owner->domain == identity_domain)
> > +		return 0;
> > +
> > +	domain = to_exynos_domain(owner->domain);
> > +	pagetable = virt_to_phys(domain->pgtable);
> 
> Identity domains by definition shouldn't have a pagetable? I don't think
> virt_to_phys(NULL) can be assumed to be valid or safe in general.

Read again, the first if excludes that owner->domain is identity so
it must be paging. Thus pgtable mst be valid.

More broadly the struct exynos_iommu_domain is now always a paging
domain.

> >   	mutex_lock(&owner->rpm_lock);
> > @@ -1009,15 +1015,25 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> >   		list_del_init(&data->domain_node);
> >   		spin_unlock(&data->lock);
> >   	}
> 
> This iterates the whole domain->clients list, which may include other
> devices from other groups belonging to other IOMMU instances. I think that's
> technically an issue already given that we support cross-instance domain
> attach here, which the DRM drivers rely on. I can't quite work out off-hand
> if this is liable to make it any worse or not :/

Yeah, that looks wrong today - it should be a strict pairing with
exynos_iommu_attach_device() so it needs to check if each
client's sysmmu_drvdata is in the exynos_iommu_owner->controller list.
Marek?

At least this transformation shouldn't make it worse as we will still
call this code in all the same places as we always did. The identity
domain is also not threaded on this list.

> > -	owner->domain = NULL;
> > +	owner->domain = identity_domain;
> >   	spin_unlock_irqrestore(&domain->lock, flags);
> >   	mutex_unlock(&owner->rpm_lock);
> >   	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> 
> This no longer makes much sense.

	dev_dbg(dev, "%s: Restored IOMMU to IDENTITY from pgtable %pa\n",
		__func__, &pagetable);

Better?

> > @@ -1457,6 +1458,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
> >   		INIT_LIST_HEAD(&owner->controllers);
> >   		mutex_init(&owner->rpm_lock);
> > +		owner->domain = &exynos_identity_domain;
> 
> I think strictly this would be more of a probe_device thing than an of_xlate
> thing, but it's not super-important.

The full code block is this:

		owner = kzalloc(sizeof(*owner), GFP_KERNEL);
		if (!owner) {
			put_device(&sysmmu->dev);
			return -ENOMEM;
		}

		INIT_LIST_HEAD(&owner->controllers);
		mutex_init(&owner->rpm_lock);
		owner->domain = &exynos_identity_domain;
		dev_iommu_priv_set(dev, owner);

So we set the domain at the same time we allocate and initialize the
owner struct.

Why this is allocated in of_xlate is beyond me, but it is the right
place to put this initialization, we never want NULL to be stored
here.

Thanks,
Jason

[PATCH 07/20] iommu/tegra-smmu: Implement an IDENTITY domain

[Jason Gunthorpe]

What tegra-smmu does during tegra_smmu_set_platform_dma() is actually
putting the iommu into identity mode.

Move to the new core support for ARM_DMA_USE_IOMMU by defining
ops->identity_domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1cbf063ccf147a..153ea0b5de8db4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -511,23 +511,39 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 	return err;
 }
 
-static void tegra_smmu_set_platform_dma(struct device *dev)
+static int tegra_smmu_identity_attach(struct iommu_domain *identity_domain,
+				      struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct tegra_smmu *smmu = as->smmu;
+	struct tegra_smmu_as *as;
+	struct tegra_smmu *smmu;
 	unsigned int index;
 
 	if (!fwspec)
-		return;
+		return -ENODEV;
 
+	if (domain == identity_domain || !domain)
+		return 0;
+
+	as = to_smmu_as(domain);
+	smmu = as->smmu;
 	for (index = 0; index < fwspec->num_ids; index++) {
 		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
 		tegra_smmu_as_unprepare(smmu, as);
 	}
+	return 0;
 }
 
+static struct iommu_domain_ops tegra_smmu_identity_ops = {
+	.attach_dev = tegra_smmu_identity_attach,
+};
+
+static struct iommu_domain tegra_smmu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &tegra_smmu_identity_ops,
+};
+
 static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 			       u32 value)
 {
@@ -963,10 +979,10 @@ static int tegra_smmu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops tegra_smmu_ops = {
+	.identity_domain = &tegra_smmu_identity_domain,
 	.domain_alloc = tegra_smmu_domain_alloc,
 	.probe_device = tegra_smmu_probe_device,
 	.device_group = tegra_smmu_device_group,
-	.set_platform_dma_ops = tegra_smmu_set_platform_dma,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.40.0

[PATCH 08/20] iommu/tegra-smmu: Support DMA domains in tegra

[Jason Gunthorpe]

All ARM64 iommu drivers should support IOMMU_DOMAIN_DMA to enable
dma-iommu.c.

tegra is blocking dma-iommu usage, and also default_domain's, because it
wants an identity translation. This is needed for some device quirk. The
correct way to do this is to support IDENTITY domains and use
ops->def_domain_type() to return IOMMU_DOMAIN_IDENTITY for only the quirky
devices.

Add support for IOMMU_DOMAIN_DMA and force IOMMU_DOMAIN_IDENTITY mode for
everything so no behavior changes.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 153ea0b5de8db4..7c301a732db2c0 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -276,7 +276,7 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 {
 	struct tegra_smmu_as *as;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	as = kzalloc(sizeof(*as), GFP_KERNEL);
@@ -979,6 +979,12 @@ static int tegra_smmu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops tegra_smmu_ops = {
+	/*
+	 * FIXME: For now we want to run all translation in IDENTITY mode,
+	 * better would be to have a def_domain_type op do this for just the
+	 * quirky device.
+	 */
+	.default_domain = &tegra_smmu_identity_domain,
 	.identity_domain = &tegra_smmu_identity_domain,
 	.domain_alloc = tegra_smmu_domain_alloc,
 	.probe_device = tegra_smmu_probe_device,
-- 
2.40.0

[PATCH 09/20] iommu/omap: Implement an IDENTITY domain

[Jason Gunthorpe]

What omap does during omap_iommu_set_platform_dma() is actually putting
the iommu into identity mode.

Move to the new core support for ARM_DMA_USE_IOMMU by defining
ops->identity_domain.

This driver does not support IOMMU_DOMAIN_DMA, however it cannot be
compiled on ARM64 either. Most likely it is fine to support dma-iommu.c

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/omap-iommu.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 537e402f9bba97..34340ef15241bc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1555,16 +1555,31 @@ static void _omap_iommu_detach_dev(struct omap_iommu_domain *omap_domain,
 	omap_domain->dev = NULL;
 }
 
-static void omap_iommu_set_platform_dma(struct device *dev)
+static int omap_iommu_identity_attach(struct iommu_domain *identity_domain,
+				      struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct omap_iommu_domain *omap_domain = to_omap_domain(domain);
+	struct omap_iommu_domain *omap_domain;
 
+	if (domain == identity_domain || !domain)
+		return 0;
+
+	omap_domain = to_omap_domain(domain);
 	spin_lock(&omap_domain->lock);
 	_omap_iommu_detach_dev(omap_domain, dev);
 	spin_unlock(&omap_domain->lock);
+	return 0;
 }
 
+static struct iommu_domain_ops omap_iommu_identity_ops = {
+	.attach_dev = omap_iommu_identity_attach,
+};
+
+static struct iommu_domain omap_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &omap_iommu_identity_ops,
+};
+
 static struct iommu_domain *omap_iommu_domain_alloc(unsigned type)
 {
 	struct omap_iommu_domain *omap_domain;
@@ -1732,11 +1747,11 @@ static struct iommu_group *omap_iommu_device_group(struct device *dev)
 }
 
 static const struct iommu_ops omap_iommu_ops = {
+	.identity_domain = &omap_iommu_identity_domain,
 	.domain_alloc	= omap_iommu_domain_alloc,
 	.probe_device	= omap_iommu_probe_device,
 	.release_device	= omap_iommu_release_device,
 	.device_group	= omap_iommu_device_group,
-	.set_platform_dma_ops = omap_iommu_set_platform_dma,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= omap_iommu_attach_dev,
-- 
2.40.0

[PATCH 10/20] iommu/msm: Implement an IDENTITY domain

[Jason Gunthorpe]

What msm does during omap_iommu_set_platform_dma() is actually putting the
iommu into identity mode.

Move to the new core support for ARM_DMA_USE_IOMMU by defining
ops->identity_domain.

This driver does not support IOMMU_DOMAIN_DMA, however it cannot be
compiled on ARM64 either. Most likely it is fine to support dma-iommu.c

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/msm_iommu.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 79d89bad5132b7..26ed81cfeee897 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -443,15 +443,20 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
-static void msm_iommu_set_platform_dma(struct device *dev)
+static int msm_iommu_identity_attach(struct iommu_domain *identity_domain,
+				     struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct msm_priv *priv = to_msm_priv(domain);
+	struct msm_priv *priv;
 	unsigned long flags;
 	struct msm_iommu_dev *iommu;
 	struct msm_iommu_ctx_dev *master;
-	int ret;
+	int ret = 0;
+
+	if (domain == identity_domain || !domain)
+		return 0;
 
+	priv = to_msm_priv(domain);
 	free_io_pgtable_ops(priv->iop);
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
@@ -468,8 +473,18 @@ static void msm_iommu_set_platform_dma(struct device *dev)
 	}
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+	return ret;
 }
 
+static struct iommu_domain_ops msm_iommu_identity_ops = {
+	.attach_dev = msm_iommu_identity_attach,
+};
+
+static struct iommu_domain msm_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &msm_iommu_identity_ops,
+};
+
 static int msm_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t pa, size_t pgsize, size_t pgcount,
 			 int prot, gfp_t gfp, size_t *mapped)
@@ -675,10 +690,10 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 }
 
 static struct iommu_ops msm_iommu_ops = {
+	.identity_domain = &msm_iommu_identity_domain,
 	.domain_alloc = msm_iommu_domain_alloc,
 	.probe_device = msm_iommu_probe_device,
 	.device_group = generic_device_group,
-	.set_platform_dma_ops = msm_iommu_set_platform_dma,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 	.of_xlate = qcom_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.40.0

[PATCH 11/20] iommu/mtk_iommu_v1: Implement an IDENTITY domain

[Jason Gunthorpe]

What mtk does during omap_iommu_set_platform_dma() is actually putting the
iommu into identity mode.

Move to the new core support for ARM_DMA_USE_IOMMU by defining
ops->identity_domain.

Remove the mtk_iommu_v1_def_domain_type() hack,
commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type")
explains this was needed to allow probe_finalize() to be called, but
now the IDENTITY domain will do the same job.

This driver does not support IOMMU_DOMAIN_DMA, however it cannot be
compiled on ARM64 either. Most likely it is fine to support dma-iommu.c

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 8a0a5e5d049f4a..7c0c1d50df5f75 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -319,13 +319,24 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device
 	return 0;
 }
 
-static void mtk_iommu_v1_set_platform_dma(struct device *dev)
+static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain,
+					struct device *dev)
 {
 	struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev);
 
 	mtk_iommu_v1_config(data, dev, false);
+	return 0;
 }
 
+static struct iommu_domain_ops mtk_iommu_v1_identity_ops = {
+	.attach_dev = mtk_iommu_v1_identity_attach,
+};
+
+static struct iommu_domain mtk_iommu_v1_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &mtk_iommu_v1_identity_ops,
+};
+
 static int mtk_iommu_v1_map(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			    int prot, gfp_t gfp, size_t *mapped)
@@ -441,11 +452,6 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg
 	return 0;
 }
 
-static int mtk_iommu_v1_def_domain_type(struct device *dev)
-{
-	return IOMMU_DOMAIN_UNMANAGED;
-}
-
 static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -578,14 +584,13 @@ static int mtk_iommu_v1_hw_init(const struct mtk_iommu_v1_data *data)
 }
 
 static const struct iommu_ops mtk_iommu_v1_ops = {
+	.identity_domain = &mtk_iommu_v1_identity_domain,
 	.domain_alloc	= mtk_iommu_v1_domain_alloc,
 	.probe_device	= mtk_iommu_v1_probe_device,
 	.probe_finalize = mtk_iommu_v1_probe_finalize,
 	.release_device	= mtk_iommu_v1_release_device,
-	.def_domain_type = mtk_iommu_v1_def_domain_type,
 	.device_group	= generic_device_group,
 	.pgsize_bitmap	= MT2701_IOMMU_PAGE_SIZE,
-	.set_platform_dma_ops = mtk_iommu_v1_set_platform_dma,
 	.owner          = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= mtk_iommu_v1_attach_device,
-- 
2.40.0

[PATCH 12/20] iommu: Remove ops->set_platform_dma_ops()

[Jason Gunthorpe]

All drivers are now using IDENTITY or PLATFORM domains for what this did,
we can remove it now. It is no longer possible to attach to a NULL domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 30 +++++-------------------------
 include/linux/iommu.h |  4 ----
 2 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b9af774de68f1..fb646765a87f47 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2192,21 +2192,8 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
 	if (group->domain == new_domain)
 		return 0;
 
-	/*
-	 * New drivers should support default domains, so set_platform_dma()
-	 * op will never be called. Otherwise the NULL domain represents some
-	 * platform specific behavior.
-	 */
-	if (!new_domain) {
-		for_each_group_device(group, gdev) {
-			const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
-
-			if (!WARN_ON(!ops->set_platform_dma_ops))
-				ops->set_platform_dma_ops(gdev->dev);
-		}
-		group->domain = NULL;
-		return 0;
-	}
+	if (WARN_ON(!new_domain))
+		return -EINVAL;
 
 	/*
 	 * Changing the domain is done by calling attach_dev() on the new
@@ -2242,19 +2229,15 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
 	 */
 	last_gdev = gdev;
 	for_each_group_device(group, gdev) {
-		const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
-
 		/*
-		 * If set_platform_dma_ops is not present a NULL domain can
-		 * happen only for first probe, in which case we leave
-		 * group->domain as NULL and let release clean everything up.
+		 * A NULL domain can happen only for first probe, in which case
+		 * we leave group->domain as NULL and let release clean
+		 * everything up.
 		 */
 		if (group->domain)
 			WARN_ON(__iommu_device_set_domain(
 				group, gdev->dev, group->domain,
 				IOMMU_SET_DOMAIN_MUST_SUCCEED));
-		else if (ops->set_platform_dma_ops)
-			ops->set_platform_dma_ops(gdev->dev);
 		if (gdev == last_gdev)
 			break;
 	}
@@ -2868,9 +2851,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	/*
 	 * There are still some drivers which don't support default domains, so
 	 * we ignore the failure and leave group->default_domain NULL.
-	 *
-	 * We assume that the iommu driver starts up the device in
-	 * 'set_platform_dma_ops' mode if it does not support default domains.
 	 */
 	dom = iommu_group_alloc_default_domain(group, req_type);
 	if (!dom) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 427490b5736d40..f6a28ab78e607e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -231,9 +231,6 @@ struct iommu_iotlb_gather {
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
  *                  group and attached to the groups domain
- * @set_platform_dma_ops: Returning control back to the platform DMA ops. This op
- *                        is to support old IOMMU drivers, new drivers should use
- *                        default domains, and the common IOMMU DMA ops.
  * @device_group: find iommu group for a particular device
  * @get_resv_regions: Request list of reserved regions for a device
  * @of_xlate: add OF master IDs to iommu grouping
@@ -265,7 +262,6 @@ struct iommu_ops {
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
 	void (*probe_finalize)(struct device *dev);
-	void (*set_platform_dma_ops)(struct device *dev);
 	struct iommu_group *(*device_group)(struct device *dev);
 
 	/* Request/Free a list of reserved regions for a device */
-- 
2.40.0

[PATCH 13/20] iommu/qcom_iommu: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe]

This brings back the ops->detach_dev() code that commit
1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
into an IDENTITY domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 39 +++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index a503ed758ec302..9d7b9d8b4386d4 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -387,6 +387,44 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev
 	return 0;
 }
 
+static int qcom_iommu_identity_attach(struct iommu_domain *identity_domain,
+				      struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct qcom_iommu_domain *qcom_domain;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct qcom_iommu_dev *qcom_iommu = to_iommu(dev);
+	unsigned int i;
+
+	if (domain == identity_domain || !domain)
+		return 0;
+
+	qcom_domain = to_qcom_iommu_domain(domain);
+	if (WARN_ON(!qcom_domain->iommu))
+		return -EINVAL;
+
+	pm_runtime_get_sync(qcom_iommu->dev);
+	for (i = 0; i < fwspec->num_ids; i++) {
+		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
+
+		/* Disable the context bank: */
+		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
+
+		ctx->domain = NULL;
+	}
+	pm_runtime_put_sync(qcom_iommu->dev);
+	return 0;
+}
+
+static struct iommu_domain_ops qcom_iommu_identity_ops = {
+	.attach_dev = qcom_iommu_identity_attach,
+};
+
+static struct iommu_domain qcom_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &qcom_iommu_identity_ops,
+};
+
 static int qcom_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			  int prot, gfp_t gfp, size_t *mapped)
@@ -553,6 +591,7 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 }
 
 static const struct iommu_ops qcom_iommu_ops = {
+	.identity_domain = &qcom_iommu_identity_domain,
 	.capable	= qcom_iommu_capable,
 	.domain_alloc	= qcom_iommu_domain_alloc,
 	.probe_device	= qcom_iommu_probe_device,
-- 
2.40.0

[PATCH 14/20] iommu/ipmmu: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe]

This brings back the ops->detach_dev() code that commit
1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
into an IDENTITY domain.

Also reverts commit 584d334b1393 ("iommu/ipmmu-vmsa: Remove
ipmmu_utlb_disable()")

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/ipmmu-vmsa.c | 43 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9f64c5c9f5b90a..de958e411a92e0 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -298,6 +298,18 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
 	mmu->utlb_ctx[utlb] = domain->context_id;
 }
 
+/*
+ * Disable MMU translation for the microTLB.
+ */
+static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
+			       unsigned int utlb)
+{
+	struct ipmmu_vmsa_device *mmu = domain->mmu;
+
+	ipmmu_imuctr_write(mmu, utlb, 0);
+	mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
+}
+
 static void ipmmu_tlb_flush_all(void *cookie)
 {
 	struct ipmmu_vmsa_domain *domain = cookie;
@@ -630,6 +642,36 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 	return 0;
 }
 
+static int ipmmu_iommu_identity_attach(struct iommu_domain *identity_domain,
+				       struct device *dev)
+{
+	struct iommu_domain *io_domain = iommu_get_domain_for_dev(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct ipmmu_vmsa_domain *domain;
+	unsigned int i;
+
+	if (io_domain == identity_domain || !io_domain)
+		return 0;
+
+	domain = to_vmsa_domain(io_domain);
+	for (i = 0; i < fwspec->num_ids; ++i)
+		ipmmu_utlb_disable(domain, fwspec->ids[i]);
+
+	/*
+	 * TODO: Optimize by disabling the context when no device is attached.
+	 */
+	return 0;
+}
+
+static struct iommu_domain_ops ipmmu_iommu_identity_ops = {
+	.attach_dev = ipmmu_iommu_identity_attach,
+};
+
+static struct iommu_domain ipmmu_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &ipmmu_iommu_identity_ops,
+};
+
 static int ipmmu_map(struct iommu_domain *io_domain, unsigned long iova,
 		     phys_addr_t paddr, size_t pgsize, size_t pgcount,
 		     int prot, gfp_t gfp, size_t *mapped)
@@ -848,6 +890,7 @@ static struct iommu_group *ipmmu_find_group(struct device *dev)
 }
 
 static const struct iommu_ops ipmmu_ops = {
+	.identity_domain = &ipmmu_iommu_identity_domain,
 	.domain_alloc = ipmmu_domain_alloc,
 	.probe_device = ipmmu_probe_device,
 	.release_device = ipmmu_release_device,
-- 
2.40.0

[PATCH 15/20] iommu/mtk_iommu: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe]

This brings back the ops->detach_dev() code that commit
1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
into an IDENTITY domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/mtk_iommu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index aecc7d154f28ee..e4b9f728002403 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -753,6 +753,28 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
+static int mtk_iommu_identity_attach(struct iommu_domain *identity_domain,
+				     struct device *dev)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
+
+	if (domain == identity_domain || !domain)
+		return 0;
+
+	mtk_iommu_config(data, dev, false, 0);
+	return 0;
+}
+
+static struct iommu_domain_ops mtk_iommu_identity_ops = {
+	.attach_dev = mtk_iommu_identity_attach,
+};
+
+static struct iommu_domain mtk_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &mtk_iommu_identity_ops,
+};
+
 static int mtk_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			 int prot, gfp_t gfp, size_t *mapped)
@@ -971,6 +993,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev,
 }
 
 static const struct iommu_ops mtk_iommu_ops = {
+	.identity_domain = &mtk_iommu_identity_domain,
 	.domain_alloc	= mtk_iommu_domain_alloc,
 	.probe_device	= mtk_iommu_probe_device,
 	.release_device	= mtk_iommu_release_device,
-- 
2.40.0

[PATCH 16/20] iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe]

This brings back the ops->detach_dev() code that commit
1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
into an IDENTITY domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/sun50i-iommu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 74c5cb93e90027..15fd62d360778f 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -772,6 +772,26 @@ static void sun50i_iommu_detach_device(struct iommu_domain *domain,
 		sun50i_iommu_detach_domain(iommu, sun50i_domain);
 }
 
+static int sun50i_iommu_identity_attach(struct iommu_domain *identity_domain,
+					struct device *dev)
+{
+	struct sun50i_iommu *iommu = dev_iommu_priv_get(dev);
+
+	if (iommu->domain == identity_domain || !iommu->domain)
+		return 0;
+	sun50i_iommu_detach_device(iommu->domain, dev);
+	return 0;
+}
+
+static struct iommu_domain_ops sun50i_iommu_identity_ops = {
+	.attach_dev = sun50i_iommu_identity_attach,
+};
+
+static struct iommu_domain sun50i_iommu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &sun50i_iommu_identity_ops,
+};
+
 static int sun50i_iommu_attach_device(struct iommu_domain *domain,
 				      struct device *dev)
 {
@@ -827,6 +847,7 @@ static int sun50i_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops sun50i_iommu_ops = {
+	.identity_domain = &sun50i_iommu_identity_domain,
 	.pgsize_bitmap	= SZ_4K,
 	.device_group	= sun50i_iommu_device_group,
 	.domain_alloc	= sun50i_iommu_domain_alloc,
-- 
2.40.0

Re: [PATCH 16/20] iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN

[Robin Murphy]

On 2023-05-01 19:03, Jason Gunthorpe wrote:
> This brings back the ops->detach_dev() code that commit
> 1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
> into an IDENTITY domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/sun50i-iommu.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index 74c5cb93e90027..15fd62d360778f 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -772,6 +772,26 @@ static void sun50i_iommu_detach_device(struct iommu_domain *domain,
>   		sun50i_iommu_detach_domain(iommu, sun50i_domain);
>   }
>   
> +static int sun50i_iommu_identity_attach(struct iommu_domain *identity_domain,
> +					struct device *dev)
> +{
> +	struct sun50i_iommu *iommu = dev_iommu_priv_get(dev);
> +
> +	if (iommu->domain == identity_domain || !iommu->domain)

I don't think that first condition could ever be true.

Thanks,
Robin.

> +		return 0;
> +	sun50i_iommu_detach_device(iommu->domain, dev);
> +	return 0;
> +}
> +
> +static struct iommu_domain_ops sun50i_iommu_identity_ops = {
> +	.attach_dev = sun50i_iommu_identity_attach,
> +};
> +
> +static struct iommu_domain sun50i_iommu_identity_domain = {
> +	.type = IOMMU_DOMAIN_IDENTITY,
> +	.ops = &sun50i_iommu_identity_ops,
> +};
> +
>   static int sun50i_iommu_attach_device(struct iommu_domain *domain,
>   				      struct device *dev)
>   {
> @@ -827,6 +847,7 @@ static int sun50i_iommu_of_xlate(struct device *dev,
>   }
>   
>   static const struct iommu_ops sun50i_iommu_ops = {
> +	.identity_domain = &sun50i_iommu_identity_domain,
>   	.pgsize_bitmap	= SZ_4K,
>   	.device_group	= sun50i_iommu_device_group,
>   	.domain_alloc	= sun50i_iommu_domain_alloc,

Re: [PATCH 16/20] iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN

[Jason Gunthorpe]

On Wed, May 03, 2023 at 04:54:28PM +0100, Robin Murphy wrote:
> 
> On 2023-05-01 19:03, Jason Gunthorpe wrote:
> > This brings back the ops->detach_dev() code that commit
> > 1b932ceddd19 ("iommu: Remove detach_dev callbacks") deleted and turns it
> > into an IDENTITY domain.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/sun50i-iommu.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> > index 74c5cb93e90027..15fd62d360778f 100644
> > --- a/drivers/iommu/sun50i-iommu.c
> > +++ b/drivers/iommu/sun50i-iommu.c
> > @@ -772,6 +772,26 @@ static void sun50i_iommu_detach_device(struct iommu_domain *domain,
> >   		sun50i_iommu_detach_domain(iommu, sun50i_domain);
> >   }
> > +static int sun50i_iommu_identity_attach(struct iommu_domain *identity_domain,
> > +					struct device *dev)
> > +{
> > +	struct sun50i_iommu *iommu = dev_iommu_priv_get(dev);
> > +
> > +	if (iommu->domain == identity_domain || !iommu->domain)
> 
> I don't think that first condition could ever be true.

Oh yes, this needs more work.. I folded sun50i_iommu_detach_device()
into sun50i_iommu_identity_attach() and made iommu->domain be set to
identity immediately after allocation. Now it can't be NULL

Thanks
Jason

[PATCH 17/20] iommu: Require a default_domain for all iommu drivers

[Jason Gunthorpe]

At this point every iommu driver will cause a default_domain to be
selected, so we can finally remove this gap from the core code.

The following table explains what each driver supports and what the
resulting default_domain will be:

                                        ops->defaut_domain
                    IDENTITY   DMA  PLATFORM    v      ARM32          dma-iommu  ARCH
amd/iommu.c             Y       Y                       N/A             either
apple-dart.c            Y       Y                       N/A             either
arm-smmu.c              Y       Y                       IDENTITY        either
qcom_iommu.c            G       Y                       IDENTITY        either
arm-smmu-v3.c           Y       Y                       N/A             either
exynos-iommu.c          G       Y                       IDENTITY        either
fsl_pamu_domain.c                       Y       Y       N/A             N/A     PLATFORM
intel/iommu.c           Y       Y                       N/A             either
ipmmu-vmsa.c            G       Y                       IDENTITY        either
msm_iommu.c             G                               IDENTITY        N/A
mtk_iommu.c             G       Y                       IDENTITY        either
mtk_iommu_v1.c          G                               IDENTITY        N/A
omap-iommu.c            G                               IDENTITY        N/A
rockchip-iommu.c        G       Y                       IDENTITY        either
s390-iommu.c                            Y       Y       N/A             N/A     PLATFORM
sprd-iommu.c                    Y                       N/A             DMA
sun50i-iommu.c          G       Y                       IDENTITY        either
tegra-gart.c                            Y       Y       PLATFORM        PLATFORM
tegra-smmu.c            G       Y               Y       IDENTITY        IDENTITY
virtio-iommu.c          Y       Y                       N/A             either
 * G means ops->identity_domain is used
 * N/A means the driver will not compile in this configuration

ARM32 drivers, except for tegra-gart, select an IDENTITY default domain
through either the ops->identity_domain or directly requesting an IDENTIY
domain through alloc_domain(). tegra-gart will use its weird PLATFORM
domain.

In ARM64 mode tegra-smmu will still block the use of dma-iommu.c and
forces an IDENTITY domain.

S390 uses a PLATFORM domain to represent when the dma_ops are set to the
s390 iommu code.

fsl_pamu uses a PLATFORM domain.

The x86 drivers continue unchanged.

After this patch group->default_domain is only NULL for a short period
during bus iommu probing while all the groups are constituted. Otherwise
it is always !NULL.

This completes changing the iommu subsystem driver contract to a system
where the current iommu_domain always represents some form of translation
and the driver is continuously asserting a definable translation mode.

It resolves the confusion that the original ops->detach_dev() caused
around what translation, exactly, is the IOMMU performing after
detach. There were at least three different answers to that question in
the tree, they are all now clearly named with domain types.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fb646765a87f47..f20a031e2910b2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1655,14 +1655,18 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 		return __iommu_group_alloc_default_domain(bus, group, req_type);
 
 	/*
-	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
-	 * identity_domain and it becomes their default domain. Later on
-	 * ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
+	 * If ARM32 CONFIG_ARM_DMA_USE_IOMMU is enabled and the driver doesn't
+	 * use the ops->default_domain override, install an IDENTITY domain as
+	 * the default domain. Later on ARM_DMA_USE_IOMMU will install its
+	 * UNMANAGED domain.
 	 */
-	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
-	    bus->iommu_ops->identity_domain)
+	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
+		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
+				IS_ENABLED(CONFIG_IOMMU_DMA)));
+
 		return __iommu_group_alloc_default_domain(
 			bus, group, IOMMU_DOMAIN_IDENTITY);
+	}
 
 	/* The driver gave no guidance on what type to use, try the default */
 	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
@@ -2848,18 +2852,9 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	if (req_type < 0)
 		return -EINVAL;
 
-	/*
-	 * There are still some drivers which don't support default domains, so
-	 * we ignore the failure and leave group->default_domain NULL.
-	 */
 	dom = iommu_group_alloc_default_domain(group, req_type);
-	if (!dom) {
-		/* Once in default_domain mode we never leave */
-		if (group->default_domain)
-			return -ENODEV;
-		group->default_domain = NULL;
-		return 0;
-	}
+	if (!dom)
+		return -ENODEV;
 
 	if (group->default_domain == dom)
 		return 0;
-- 
2.40.0

[PATCH 18/20] iommu: Add ops->domain_alloc_paging()

[Jason Gunthorpe]

This callback requests the driver to create only a __IOMMU_DOMAIN_PAGING
domain, so it saves a few lines in a lot of drivers needlessly checking
the type.

More critically, this allows us to sweep out all the
IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA checks from a lot of the
drivers, simplifying what is going on in the code and ultimately removing
the now-unused special cases in drivers where they did not support
IOMMU_DOMAIN_DMA.

domain_alloc_paging() should return a struct iommu_domain that is
functionally compatible with ARM_DMA_USE_IOMMU, dma-iommu.c and iommufd.

Be forwards looking and pass in a 'struct device *' argument. We can
provide this when allocating the default_domain. No drivers will look at
this.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 40 ++++++++++++++++++++++++----------------
 include/linux/iommu.h |  2 ++
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f20a031e2910b2..fee417df8f195d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -93,6 +93,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
 static void iommu_release_device(struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 struct device *dev,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
@@ -1618,12 +1619,12 @@ static int iommu_get_def_domain_type(struct device *dev)
 }
 
 static struct iommu_domain *
-__iommu_group_alloc_default_domain(struct bus_type *bus,
+__iommu_group_alloc_default_domain(struct group_device *gdev,
 				   struct iommu_group *group, int req_type)
 {
 	if (group->default_domain && group->default_domain->type == req_type)
 		return group->default_domain;
-	return __iommu_domain_alloc(bus, req_type);
+	return __iommu_domain_alloc(gdev->dev->bus, gdev->dev, req_type);
 }
 
 /*
@@ -1633,9 +1634,9 @@ __iommu_group_alloc_default_domain(struct bus_type *bus,
 static struct iommu_domain *
 iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
-	struct bus_type *bus =
-		list_first_entry(&group->devices, struct group_device, list)
-			->dev->bus;
+	struct group_device *gdev =
+		list_first_entry(&group->devices, struct group_device, list);
+	const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
 	struct iommu_domain *dom;
 
 	lockdep_assert_held(&group->mutex);
@@ -1645,14 +1646,15 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 	 * domain. This should always be either an IDENTITY or PLATFORM domain.
 	 * Do not use in new drivers.
 	 */
-	if (bus->iommu_ops->default_domain) {
+	if (ops->default_domain) {
 		if (req_type)
 			return ERR_PTR(-EINVAL);
-		return bus->iommu_ops->default_domain;
+		return ops->default_domain;
 	}
 
 	if (req_type)
-		return __iommu_group_alloc_default_domain(bus, group, req_type);
+		return __iommu_group_alloc_default_domain(gdev, group,
+							  req_type);
 
 	/*
 	 * If ARM32 CONFIG_ARM_DMA_USE_IOMMU is enabled and the driver doesn't
@@ -1665,18 +1667,19 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 				IS_ENABLED(CONFIG_IOMMU_DMA)));
 
 		return __iommu_group_alloc_default_domain(
-			bus, group, IOMMU_DOMAIN_IDENTITY);
+			gdev, group, IOMMU_DOMAIN_IDENTITY);
 	}
 
 	/* The driver gave no guidance on what type to use, try the default */
-	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
+	dom = __iommu_group_alloc_default_domain(gdev, group,
+						 iommu_def_domain_type);
 	if (dom)
 		return dom;
 
 	/* Otherwise IDENTITY and DMA_FQ defaults will try DMA */
 	if (iommu_def_domain_type == IOMMU_DOMAIN_DMA)
 		return NULL;
-	dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA);
+	dom = __iommu_group_alloc_default_domain(gdev, group, IOMMU_DOMAIN_DMA);
 	if (!dom)
 		return NULL;
 
@@ -1930,6 +1933,7 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+						 struct device *dev,
 						 unsigned type)
 {
 	struct iommu_domain *domain;
@@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
 		return bus->iommu_ops->identity_domain;
 
-	domain = bus->iommu_ops->domain_alloc(type);
+	if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
+	    bus->iommu_ops->domain_alloc_paging)
+		domain = bus->iommu_ops->domain_alloc_paging(dev);
+	else
+		domain = bus->iommu_ops->domain_alloc(type);
 	if (!domain)
 		return NULL;
 
@@ -1964,7 +1972,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 
 struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
 {
-	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+	return __iommu_domain_alloc(bus, NULL, IOMMU_DOMAIN_UNMANAGED);
 }
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
@@ -3079,15 +3087,15 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 	if (group->blocking_domain)
 		return 0;
 
-	group->blocking_domain =
-		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
+	group->blocking_domain = __iommu_domain_alloc(dev->dev->bus, dev->dev,
+						      IOMMU_DOMAIN_BLOCKED);
 	if (!group->blocking_domain) {
 		/*
 		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
 		 * create an empty domain instead.
 		 */
 		group->blocking_domain = __iommu_domain_alloc(
-			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
+			dev->dev->bus, dev->dev, IOMMU_DOMAIN_UNMANAGED);
 		if (!group->blocking_domain)
 			return -EINVAL;
 	}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f6a28ab78e607e..cc9aff2d213eec 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -227,6 +227,7 @@ struct iommu_iotlb_gather {
  * struct iommu_ops - iommu ops and capabilities
  * @capable: check capability
  * @domain_alloc: allocate iommu domain
+ * @domain_alloc_paging: Allocate an IOMMU_DOMAIN_UNMANAGED
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
@@ -258,6 +259,7 @@ struct iommu_ops {
 
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
+	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
 
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
-- 
2.40.0

Re: [PATCH 18/20] iommu: Add ops->domain_alloc_paging()

[Robin Murphy]

On 2023-05-01 19:03, Jason Gunthorpe wrote:
> This callback requests the driver to create only a __IOMMU_DOMAIN_PAGING
> domain, so it saves a few lines in a lot of drivers needlessly checking
> the type.
> 
> More critically, this allows us to sweep out all the
> IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA checks from a lot of the
> drivers, simplifying what is going on in the code and ultimately removing
> the now-unused special cases in drivers where they did not support
> IOMMU_DOMAIN_DMA.
> 
> domain_alloc_paging() should return a struct iommu_domain that is
> functionally compatible with ARM_DMA_USE_IOMMU, dma-iommu.c and iommufd.
> 
> Be forwards looking and pass in a 'struct device *' argument. We can
> provide this when allocating the default_domain. No drivers will look at
> this.

As discussed previously, we're going to want a way for callers to pass 
through various options as well, initially to replace 
iommu_set_pgtable_quirks() at the very least. Maybe passing an 
easily-extensible structure around is the even better option? (Wherein 
we could even strictly enforce "no drivers will look at this" for the 
moment by leaving it empty)

I'm hoping I'll get another version of my bus ops removal out this 
cycle; there's obviously a lot of overlap here to figure out.

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 40 ++++++++++++++++++++++++----------------
>   include/linux/iommu.h |  2 ++
>   2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f20a031e2910b2..fee417df8f195d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>   			      unsigned long action, void *data);
>   static void iommu_release_device(struct device *dev);
>   static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> +						 struct device *dev,
>   						 unsigned type);
>   static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
> @@ -1618,12 +1619,12 @@ static int iommu_get_def_domain_type(struct device *dev)
>   }
>   
>   static struct iommu_domain *
> -__iommu_group_alloc_default_domain(struct bus_type *bus,
> +__iommu_group_alloc_default_domain(struct group_device *gdev,
>   				   struct iommu_group *group, int req_type)
>   {
>   	if (group->default_domain && group->default_domain->type == req_type)
>   		return group->default_domain;
> -	return __iommu_domain_alloc(bus, req_type);
> +	return __iommu_domain_alloc(gdev->dev->bus, gdev->dev, req_type);
>   }
>   
>   /*
> @@ -1633,9 +1634,9 @@ __iommu_group_alloc_default_domain(struct bus_type *bus,
>   static struct iommu_domain *
>   iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   {
> -	struct bus_type *bus =
> -		list_first_entry(&group->devices, struct group_device, list)
> -			->dev->bus;
> +	struct group_device *gdev =
> +		list_first_entry(&group->devices, struct group_device, list);

Eww, why pass around a group_device that nobody wants? Keeping the 
single dereference of ->dev here, rather than the three above and below, 
would be cleaner. And it makes my iommu_group_first_device() helper look 
even more appealing, if I dare say so myself :)

> +	const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
>   	struct iommu_domain *dom;
>   
>   	lockdep_assert_held(&group->mutex);
> @@ -1645,14 +1646,15 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   	 * domain. This should always be either an IDENTITY or PLATFORM domain.
>   	 * Do not use in new drivers.
>   	 */
> -	if (bus->iommu_ops->default_domain) {
> +	if (ops->default_domain) {
>   		if (req_type)
>   			return ERR_PTR(-EINVAL);
> -		return bus->iommu_ops->default_domain;
> +		return ops->default_domain;
>   	}
>   
>   	if (req_type)
> -		return __iommu_group_alloc_default_domain(bus, group, req_type);
> +		return __iommu_group_alloc_default_domain(gdev, group,
> +							  req_type);
>   
>   	/*
>   	 * If ARM32 CONFIG_ARM_DMA_USE_IOMMU is enabled and the driver doesn't
> @@ -1665,18 +1667,19 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   				IS_ENABLED(CONFIG_IOMMU_DMA)));
>   
>   		return __iommu_group_alloc_default_domain(
> -			bus, group, IOMMU_DOMAIN_IDENTITY);
> +			gdev, group, IOMMU_DOMAIN_IDENTITY);
>   	}
>   
>   	/* The driver gave no guidance on what type to use, try the default */
> -	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
> +	dom = __iommu_group_alloc_default_domain(gdev, group,
> +						 iommu_def_domain_type);
>   	if (dom)
>   		return dom;
>   
>   	/* Otherwise IDENTITY and DMA_FQ defaults will try DMA */
>   	if (iommu_def_domain_type == IOMMU_DOMAIN_DMA)
>   		return NULL;
> -	dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA);
> +	dom = __iommu_group_alloc_default_domain(gdev, group, IOMMU_DOMAIN_DMA);
>   	if (!dom)
>   		return NULL;
>   
> @@ -1930,6 +1933,7 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>   
>   static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> +						 struct device *dev,
>   						 unsigned type)
>   {
>   	struct iommu_domain *domain;
> @@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>   	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
>   		return bus->iommu_ops->identity_domain;
>   
> -	domain = bus->iommu_ops->domain_alloc(type);
> +	if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&

Logically, "type & __IOMMU_DOMAIN_PAGING", otherwise we're already 
missing IOMMU_DOMAIN_DMA_FQ. Except maybe that's deliberate? Not sure 
how much I like the idea of a introducing new interface design that we 
clearly can't even convert all the current drivers to as it stands :/

> +	    bus->iommu_ops->domain_alloc_paging)
> +		domain = bus->iommu_ops->domain_alloc_paging(dev);
> +	else
> +		domain = bus->iommu_ops->domain_alloc(type);
>   	if (!domain)
>   		return NULL;
>   
> @@ -1964,7 +1972,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>   
>   struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>   {
> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> +	return __iommu_domain_alloc(bus, NULL, IOMMU_DOMAIN_UNMANAGED);
>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
> @@ -3079,15 +3087,15 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>   	if (group->blocking_domain)
>   		return 0;
>   
> -	group->blocking_domain =
> -		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> +	group->blocking_domain = __iommu_domain_alloc(dev->dev->bus, dev->dev,
> +						      IOMMU_DOMAIN_BLOCKED);
>   	if (!group->blocking_domain) {
>   		/*
>   		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>   		 * create an empty domain instead.
>   		 */
>   		group->blocking_domain = __iommu_domain_alloc(
> -			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +			dev->dev->bus, dev->dev, IOMMU_DOMAIN_UNMANAGED);
>   		if (!group->blocking_domain)
>   			return -EINVAL;
>   	}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f6a28ab78e607e..cc9aff2d213eec 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -227,6 +227,7 @@ struct iommu_iotlb_gather {
>    * struct iommu_ops - iommu ops and capabilities
>    * @capable: check capability
>    * @domain_alloc: allocate iommu domain
> + * @domain_alloc_paging: Allocate an IOMMU_DOMAIN_UNMANAGED

...except for the other types of paging-capable domain which also exist 
and it also allocates :/

There remains good reason to keep track of the distinct subtypes of 
paging domain within the IOMMU core (i.e. between iommu.c and 
dma-iommu.c) even if drivers do finally get absolved of all the details. 
Sure we could come up with any number of different ways to encode those 
distinctions, but they wouldn't be objectively better than the domain 
flags and types we already have, they'd just be different.

Thanks,
Robin.

>    * @probe_device: Add device to iommu driver handling
>    * @release_device: Remove device from iommu driver handling
>    * @probe_finalize: Do final setup work after the device is added to an IOMMU
> @@ -258,6 +259,7 @@ struct iommu_ops {
>   
>   	/* Domain allocation and freeing by the iommu driver */
>   	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> +	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
>   
>   	struct iommu_device *(*probe_device)(struct device *dev);
>   	void (*release_device)(struct device *dev);

Re: [PATCH 18/20] iommu: Add ops->domain_alloc_paging()

[Jason Gunthorpe]

On Wed, May 03, 2023 at 06:17:58PM +0100, Robin Murphy wrote:
> On 2023-05-01 19:03, Jason Gunthorpe wrote:
> > This callback requests the driver to create only a __IOMMU_DOMAIN_PAGING
> > domain, so it saves a few lines in a lot of drivers needlessly checking
> > the type.
> > 
> > More critically, this allows us to sweep out all the
> > IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA checks from a lot of the
> > drivers, simplifying what is going on in the code and ultimately removing
> > the now-unused special cases in drivers where they did not support
> > IOMMU_DOMAIN_DMA.
> > 
> > domain_alloc_paging() should return a struct iommu_domain that is
> > functionally compatible with ARM_DMA_USE_IOMMU, dma-iommu.c and iommufd.
> > 
> > Be forwards looking and pass in a 'struct device *' argument. We can
> > provide this when allocating the default_domain. No drivers will look at
> > this.
> 
> As discussed previously, we're going to want a way for callers to pass
> through various options as well, initially to replace
> iommu_set_pgtable_quirks() at the very least.

Yeah, I like that direction

> Maybe passing an easily-extensible structure around is the even
> better option? (Wherein we could even strictly enforce "no drivers
> will look at this" for the moment by leaving it empty)

Can we leave this till when we get there? I think it makes sense to
add an args struct and the first one can contain the quirk field.

The trouble with this approach is we still have to get the alloc to
reject inputs it doesn't understand.

> I'm hoping I'll get another version of my bus ops removal out this cycle;
> there's obviously a lot of overlap here to figure out.

I think the places it needs to touch is slowly shrinking :)

But it would be great to get the bus stuff going, we can organize the
series in whatever order, but I think the "Consolidate the error
handling around device attachment" series is good now so we should get
it in Joerg's tree and start there.

> > @@ -1633,9 +1634,9 @@ __iommu_group_alloc_default_domain(struct bus_type *bus,
> >   static struct iommu_domain *
> >   iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
> >   {
> > -	struct bus_type *bus =
> > -		list_first_entry(&group->devices, struct group_device, list)
> > -			->dev->bus;
> > +	struct group_device *gdev =
> > +		list_first_entry(&group->devices, struct group_device, list);
> 
> Eww, why pass around a group_device that nobody wants? 

Oops, yeah, will fix

> cleaner. And it makes my iommu_group_first_device() helper look even more
> appealing, if I dare say so myself :)

It is a good idea, but I seem to recall the count of users has reduced
after my last two series

> > @@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> >   	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
> >   		return bus->iommu_ops->identity_domain;
> > -	domain = bus->iommu_ops->domain_alloc(type);
> > +	if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
> 
> Logically, "type & __IOMMU_DOMAIN_PAGING", otherwise we're already missing
> IOMMU_DOMAIN_DMA_FQ. Except maybe that's deliberate? 

It is deliberate for now, if it included FQ it would cause a bunch of
ARM64 drivers to switch to lazy mode. I'll add a comment.

I have drafted a followup series that removes all the
DMA/DMA_FQ/UNMANAGED checks from the remaining 6 drivers. I did this
by adding an op flag 'prefer to use FQ' and made the core code drive
the FQ decision from ops.

But that is another series for another day..

> Not sure how much I like the idea of a introducing new interface
> design that we clearly can't even convert all the current drivers to
> as it stands :/

I wouldn't say that. This converts 14 of the drivers and leaves 6. The
remaing 6 are more complicated and have to go in other series. eg I
have to reorganize DMA_FQ first.

Regardless, the point is not to be universal but to have a straight
forward sign off that these 14 drivers have been audited and they
don't have any policy logic in alloc_domain any more. Incremental
progress..

> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index f6a28ab78e607e..cc9aff2d213eec 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -227,6 +227,7 @@ struct iommu_iotlb_gather {
> >    * struct iommu_ops - iommu ops and capabilities
> >    * @capable: check capability
> >    * @domain_alloc: allocate iommu domain
> > + * @domain_alloc_paging: Allocate an IOMMU_DOMAIN_UNMANAGED
> 
> ...except for the other types of paging-capable domain which also exist and
> it also allocates :/

Yeah.. I can write something else here

> There remains good reason to keep track of the distinct subtypes of paging
> domain within the IOMMU core (i.e. between iommu.c and dma-iommu.c) even if
> drivers do finally get absolved of all the details. 

It is hard to keep the details out of the drivers if the argument to
ops->alloc_domain() encodes the details we want to keep opaque :)

So I have been taking the approach of just removing the
IOMMU_DOMAIN_xx text and related checks from the drivers as it becomes
possible. This series is the first wack and does every driver except
those that have dynamic identity domains.

DMA_FQ is the logical next step on this thread

> Sure we could come up with any number of different ways to encode
> those distinctions, but they wouldn't be objectively better than the
> domain flags and types we already have, they'd just be different.

My general feeling is that we can remove them completely. At least DMA
and DMA_FQ I think can be removed totally with some reorganizing.

PAGING can become maybe (bool)domain->ops->iova_to_phys with some small
work on the remaining 6 drivers.

So we might just get down to SVA, IDENTITY and everything else

Jason

Re: [PATCH 18/20] iommu: Add ops->domain_alloc_paging()

[Niklas Schnelle]

On Wed, 2023-05-03 at 16:35 -0300, Jason Gunthorpe wrote:
> On Wed, May 03, 2023 at 06:17:58PM +0100, Robin Murphy wrote:
> > On 2023-05-01 19:03, Jason Gunthorpe wrote:
> > > 
---8<---
> 
> > > @@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> > >   	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
> > >   		return bus->iommu_ops->identity_domain;
> > > -	domain = bus->iommu_ops->domain_alloc(type);
> > > +	if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
> > 
> > Logically, "type & __IOMMU_DOMAIN_PAGING", otherwise we're already missing
> > IOMMU_DOMAIN_DMA_FQ. Except maybe that's deliberate? 
> 
> It is deliberate for now, if it included FQ it would cause a bunch of
> ARM64 drivers to switch to lazy mode. I'll add a comment.
> 
> I have drafted a followup series that removes all the
> DMA/DMA_FQ/UNMANAGED checks from the remaining 6 drivers. I did this
> by adding an op flag 'prefer to use FQ' and made the core code drive
> the FQ decision from ops.

Ah that sounds like it could fit very well with s390's need for an even
lazier flush mode to handle the virtualized IOMMU with slow IOTLB flush
case aka _SQ / single flush queue mode. When you have anything ready
give me a ping and I can rework my DMA conversion on top of this.

Re: [PATCH 18/20] iommu: Add ops->domain_alloc_paging()

[Jason Gunthorpe]

On Thu, May 04, 2023 at 02:35:35PM +0200, Niklas Schnelle wrote:
> On Wed, 2023-05-03 at 16:35 -0300, Jason Gunthorpe wrote:
> > On Wed, May 03, 2023 at 06:17:58PM +0100, Robin Murphy wrote:
> > > On 2023-05-01 19:03, Jason Gunthorpe wrote:
> > > > 
> ---8<---
> > 
> > > > @@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> > > >   	if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
> > > >   		return bus->iommu_ops->identity_domain;
> > > > -	domain = bus->iommu_ops->domain_alloc(type);
> > > > +	if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
> > > 
> > > Logically, "type & __IOMMU_DOMAIN_PAGING", otherwise we're already missing
> > > IOMMU_DOMAIN_DMA_FQ. Except maybe that's deliberate? 
> > 
> > It is deliberate for now, if it included FQ it would cause a bunch of
> > ARM64 drivers to switch to lazy mode. I'll add a comment.
> > 
> > I have drafted a followup series that removes all the
> > DMA/DMA_FQ/UNMANAGED checks from the remaining 6 drivers. I did this
> > by adding an op flag 'prefer to use FQ' and made the core code drive
> > the FQ decision from ops.
> 
> Ah that sounds like it could fit very well with s390's need for an even
> lazier flush mode to handle the virtualized IOMMU with slow IOTLB flush
> case aka _SQ / single flush queue mode. When you have anything ready
> give me a ping and I can rework my DMA conversion on top of this.

I'd like to get your S390 dma-api conversion merged ASAP!

I have this general objective to get the modern architectures onto
dma-iommu.c because I want to add new things to the dma-api :\

I had imagined a new op flag because it is is pretty simple, but a
op->get_performance or something that reports some data that could
help dma-iommu.c decide if lazy mode is worthwhile and if there is
other tuning sounds interesting too..

Jason
 

[PATCH 19/20] iommu: Convert simple drivers with DOMAIN_DMA to domain_alloc_paging()

[Jason Gunthorpe]

These drivers are all trivially converted since the function is only
called if the domain type is going to be
IOMMU_DOMAIN_UNMANAGED/DMA.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 6 ++----
 drivers/iommu/exynos-iommu.c            | 7 ++-----
 drivers/iommu/ipmmu-vmsa.c              | 7 ++-----
 drivers/iommu/mtk_iommu.c               | 7 ++-----
 drivers/iommu/rockchip-iommu.c          | 7 ++-----
 drivers/iommu/sprd-iommu.c              | 7 ++-----
 drivers/iommu/sun50i-iommu.c            | 9 +++------
 drivers/iommu/tegra-smmu.c              | 7 ++-----
 8 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 9d7b9d8b4386d4..a2140fdc65ed58 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -319,12 +319,10 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	return ret;
 }
 
-static struct iommu_domain *qcom_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *qcom_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct qcom_iommu_domain *qcom_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
-		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really do anything meaningful until we've added a
@@ -593,7 +591,7 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 static const struct iommu_ops qcom_iommu_ops = {
 	.identity_domain = &qcom_iommu_identity_domain,
 	.capable	= qcom_iommu_capable,
-	.domain_alloc	= qcom_iommu_domain_alloc,
+	.domain_alloc_paging = qcom_iommu_domain_alloc_paging,
 	.probe_device	= qcom_iommu_probe_device,
 	.device_group	= generic_device_group,
 	.of_xlate	= qcom_iommu_of_xlate,
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 6ff7901103948a..2af0a735f3dc2c 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -887,7 +887,7 @@ static inline void exynos_iommu_set_pte(sysmmu_pte_t *ent, sysmmu_pte_t val)
 				   DMA_TO_DEVICE);
 }
 
-static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *exynos_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct exynos_iommu_domain *domain;
 	dma_addr_t handle;
@@ -896,9 +896,6 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
 	/* Check if correct PTE offsets are initialized */
 	BUG_ON(PG_ENT_SHIFT < 0 || !dma_dev);
 
-	if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
@@ -1474,7 +1471,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 
 static const struct iommu_ops exynos_iommu_ops = {
 	.identity_domain = &exynos_identity_domain,
-	.domain_alloc = exynos_iommu_domain_alloc,
+	.domain_alloc_paging = exynos_iommu_domain_alloc_paging,
 	.device_group = generic_device_group,
 	.probe_device = exynos_iommu_probe_device,
 	.release_device = exynos_iommu_release_device,
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index de958e411a92e0..27d36347e0fced 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -566,13 +566,10 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
  * IOMMU Operations
  */
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *ipmmu_domain_alloc_paging(struct device *dev)
 {
 	struct ipmmu_vmsa_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
-		return NULL;
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
@@ -891,7 +888,7 @@ static struct iommu_group *ipmmu_find_group(struct device *dev)
 
 static const struct iommu_ops ipmmu_ops = {
 	.identity_domain = &ipmmu_iommu_identity_domain,
-	.domain_alloc = ipmmu_domain_alloc,
+	.domain_alloc_paging = ipmmu_domain_alloc_paging,
 	.probe_device = ipmmu_probe_device,
 	.release_device = ipmmu_release_device,
 	.probe_finalize = ipmmu_probe_finalize,
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e4b9f728002403..e0bb10adc62967 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -667,13 +667,10 @@ static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom,
 	return 0;
 }
 
-static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *mtk_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct mtk_iommu_domain *dom;
 
-	if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
 	if (!dom)
 		return NULL;
@@ -994,7 +991,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev,
 
 static const struct iommu_ops mtk_iommu_ops = {
 	.identity_domain = &mtk_iommu_identity_domain,
-	.domain_alloc	= mtk_iommu_domain_alloc,
+	.domain_alloc_paging = mtk_iommu_domain_alloc_paging,
 	.probe_device	= mtk_iommu_probe_device,
 	.release_device	= mtk_iommu_release_device,
 	.device_group	= mtk_iommu_device_group,
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9e1296a856ac4c..24a2a09fea5503 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1071,13 +1071,10 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	return ret;
 }
 
-static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *rk_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct rk_iommu_domain *rk_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
-		return NULL;
-
 	if (!dma_dev)
 		return NULL;
 
@@ -1200,7 +1197,7 @@ static int rk_iommu_of_xlate(struct device *dev,
 
 static const struct iommu_ops rk_iommu_ops = {
 	.identity_domain = &rk_identity_domain,
-	.domain_alloc = rk_iommu_domain_alloc,
+	.domain_alloc_paging = rk_iommu_domain_alloc_paging,
 	.probe_device = rk_iommu_probe_device,
 	.release_device = rk_iommu_release_device,
 	.device_group = rk_iommu_device_group,
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 39e34fdeccda78..af68b3a2c123ab 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -133,13 +133,10 @@ sprd_iommu_pgt_size(struct iommu_domain *domain)
 		SPRD_IOMMU_PAGE_SHIFT) * sizeof(u32);
 }
 
-static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
+static struct iommu_domain *sprd_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct sprd_iommu_domain *dom;
 
-	if (domain_type != IOMMU_DOMAIN_DMA && domain_type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
 	if (!dom)
 		return NULL;
@@ -419,7 +416,7 @@ static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 
 
 static const struct iommu_ops sprd_iommu_ops = {
-	.domain_alloc	= sprd_iommu_domain_alloc,
+	.domain_alloc_paging = sprd_iommu_domain_alloc_paging,
 	.probe_device	= sprd_iommu_probe_device,
 	.device_group	= sprd_iommu_device_group,
 	.of_xlate	= sprd_iommu_of_xlate,
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 15fd62d360778f..2831398ea2ca71 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -667,14 +667,11 @@ static phys_addr_t sun50i_iommu_iova_to_phys(struct iommu_domain *domain,
 		sun50i_iova_get_page_offset(iova);
 }
 
-static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *
+sun50i_iommu_domain_alloc_paging(struct device *paging)
 {
 	struct sun50i_iommu_domain *sun50i_domain;
 
-	if (type != IOMMU_DOMAIN_DMA &&
-	    type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	sun50i_domain = kzalloc(sizeof(*sun50i_domain), GFP_KERNEL);
 	if (!sun50i_domain)
 		return NULL;
@@ -850,7 +847,7 @@ static const struct iommu_ops sun50i_iommu_ops = {
 	.identity_domain = &sun50i_iommu_identity_domain,
 	.pgsize_bitmap	= SZ_4K,
 	.device_group	= sun50i_iommu_device_group,
-	.domain_alloc	= sun50i_iommu_domain_alloc,
+	.domain_alloc_paging = sun50i_iommu_domain_alloc_paging,
 	.of_xlate	= sun50i_iommu_of_xlate,
 	.probe_device	= sun50i_iommu_probe_device,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 7c301a732db2c0..115e51442df5c1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -272,13 +272,10 @@ static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
 	clear_bit(id, smmu->asids);
 }
 
-static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
+static struct iommu_domain *tegra_smmu_domain_alloc_paging(struct device *dev)
 {
 	struct tegra_smmu_as *as;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
-		return NULL;
-
 	as = kzalloc(sizeof(*as), GFP_KERNEL);
 	if (!as)
 		return NULL;
@@ -986,7 +983,7 @@ static const struct iommu_ops tegra_smmu_ops = {
 	 */
 	.default_domain = &tegra_smmu_identity_domain,
 	.identity_domain = &tegra_smmu_identity_domain,
-	.domain_alloc = tegra_smmu_domain_alloc,
+	.domain_alloc_paging = tegra_smmu_domain_alloc_paging,
 	.probe_device = tegra_smmu_probe_device,
 	.device_group = tegra_smmu_device_group,
 	.of_xlate = tegra_smmu_of_xlate,
-- 
2.40.0

[PATCH 20/20] iommu: Convert remaining simple drivers to domain_alloc_paging()

[Jason Gunthorpe]

These drivers don't support IOMMU_DOMAIN_DMA, so this commit effectively
allows them to support that mode.

The prior work to require default_domains makes this safe because every
one of these drivers is either compilation incompatible with dma-iommu.c,
or already establishing a default_domain. In both cases alloc_domain()
will never be called with IOMMU_DOMAIN_DMA for these drivers so it is safe
to drop the test.

Removing these tests clarifies that the domain allocation path is only
about the functionality of a paging domain and has nothing to do with
policy of how the paging domain is used for UNMANAGED/DMA/DMA_FQ.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/fsl_pamu_domain.c | 7 ++-----
 drivers/iommu/msm_iommu.c       | 7 ++-----
 drivers/iommu/mtk_iommu_v1.c    | 7 ++-----
 drivers/iommu/omap-iommu.c      | 7 ++-----
 drivers/iommu/s390-iommu.c      | 7 ++-----
 drivers/iommu/tegra-gart.c      | 7 ++-----
 6 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 4c65f1adfe7511..641e89f7d7f444 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -192,13 +192,10 @@ static void fsl_pamu_domain_free(struct iommu_domain *domain)
 	kmem_cache_free(fsl_pamu_domain_cache, dma_domain);
 }
 
-static struct iommu_domain *fsl_pamu_domain_alloc(unsigned type)
+static struct iommu_domain *fsl_pamu_domain_alloc_paging(struct device *dev)
 {
 	struct fsl_dma_domain *dma_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	dma_domain = kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
 	if (!dma_domain)
 		return NULL;
@@ -473,7 +470,7 @@ static struct iommu_device *fsl_pamu_probe_device(struct device *dev)
 static const struct iommu_ops fsl_pamu_ops = {
 	.default_domain = &fsl_pamu_platform_domain,
 	.capable	= fsl_pamu_capable,
-	.domain_alloc	= fsl_pamu_domain_alloc,
+	.domain_alloc_paging = fsl_pamu_domain_alloc_paging,
 	.probe_device	= fsl_pamu_probe_device,
 	.device_group   = fsl_pamu_device_group,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 26ed81cfeee897..a163cee0b7242d 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -302,13 +302,10 @@ static void __program_context(void __iomem *base, int ctx,
 	SET_M(base, ctx, 1);
 }
 
-static struct iommu_domain *msm_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *msm_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct msm_priv *priv;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		goto fail_nomem;
@@ -691,7 +688,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 
 static struct iommu_ops msm_iommu_ops = {
 	.identity_domain = &msm_iommu_identity_domain,
-	.domain_alloc = msm_iommu_domain_alloc,
+	.domain_alloc_paging = msm_iommu_domain_alloc_paging,
 	.probe_device = msm_iommu_probe_device,
 	.device_group = generic_device_group,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 7c0c1d50df5f75..67e044c1a7d93b 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -270,13 +270,10 @@ static int mtk_iommu_v1_domain_finalise(struct mtk_iommu_v1_data *data)
 	return 0;
 }
 
-static struct iommu_domain *mtk_iommu_v1_domain_alloc(unsigned type)
+static struct iommu_domain *mtk_iommu_v1_domain_alloc_paging(struct device *dev)
 {
 	struct mtk_iommu_v1_domain *dom;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
 	if (!dom)
 		return NULL;
@@ -585,7 +582,7 @@ static int mtk_iommu_v1_hw_init(const struct mtk_iommu_v1_data *data)
 
 static const struct iommu_ops mtk_iommu_v1_ops = {
 	.identity_domain = &mtk_iommu_v1_identity_domain,
-	.domain_alloc	= mtk_iommu_v1_domain_alloc,
+	.domain_alloc_paging = mtk_iommu_v1_domain_alloc_paging,
 	.probe_device	= mtk_iommu_v1_probe_device,
 	.probe_finalize = mtk_iommu_v1_probe_finalize,
 	.release_device	= mtk_iommu_v1_release_device,
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 34340ef15241bc..fcf99bd195b32e 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1580,13 +1580,10 @@ static struct iommu_domain omap_iommu_identity_domain = {
 	.ops = &omap_iommu_identity_ops,
 };
 
-static struct iommu_domain *omap_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *omap_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct omap_iommu_domain *omap_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
 	if (!omap_domain)
 		return NULL;
@@ -1748,7 +1745,7 @@ static struct iommu_group *omap_iommu_device_group(struct device *dev)
 
 static const struct iommu_ops omap_iommu_ops = {
 	.identity_domain = &omap_iommu_identity_domain,
-	.domain_alloc	= omap_iommu_domain_alloc,
+	.domain_alloc_paging = omap_iommu_domain_alloc_paging,
 	.probe_device	= omap_iommu_probe_device,
 	.release_device	= omap_iommu_release_device,
 	.device_group	= omap_iommu_device_group,
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index f0c867c57a5b9b..5695ad71d60e24 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -39,13 +39,10 @@ static bool s390_iommu_capable(struct device *dev, enum iommu_cap cap)
 	}
 }
 
-static struct iommu_domain *s390_domain_alloc(unsigned domain_type)
+static struct iommu_domain *s390_domain_alloc_paging(struct device *dev)
 {
 	struct s390_domain *s390_domain;
 
-	if (domain_type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	s390_domain = kzalloc(sizeof(*s390_domain), GFP_KERNEL);
 	if (!s390_domain)
 		return NULL;
@@ -447,7 +444,7 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
 static const struct iommu_ops s390_iommu_ops = {
 	.default_domain = &s390_iommu_platform_domain,
 	.capable = s390_iommu_capable,
-	.domain_alloc = s390_domain_alloc,
+	.domain_alloc_paging = s390_domain_alloc_paging,
 	.probe_device = s390_iommu_probe_device,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 09865889ff2480..b90801dddef413 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -167,13 +167,10 @@ static struct iommu_domain gart_iommu_platform_domain = {
 	.ops = &gart_iommu_platform_ops,
 };
 
-static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
+static struct iommu_domain *gart_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct iommu_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (domain) {
 		domain->geometry.aperture_start = gart_handle->iovmm_base;
@@ -294,7 +291,7 @@ static void gart_iommu_sync(struct iommu_domain *domain,
 
 static const struct iommu_ops gart_iommu_ops = {
 	.default_domain = &gart_iommu_platform_domain,
-	.domain_alloc	= gart_iommu_domain_alloc,
+	.domain_alloc_paging = gart_iommu_domain_alloc_paging,
 	.probe_device	= gart_iommu_probe_device,
 	.device_group	= generic_device_group,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
-- 
2.40.0

Re: [PATCH 20/20] iommu: Convert remaining simple drivers to domain_alloc_paging()

[Niklas Schnelle]

On Mon, 2023-05-01 at 15:03 -0300, Jason Gunthorpe wrote:
> These drivers don't support IOMMU_DOMAIN_DMA, so this commit effectively
> allows them to support that mode.
> 
> The prior work to require default_domains makes this safe because every
> one of these drivers is either compilation incompatible with dma-iommu.c,
> or already establishing a default_domain. In both cases alloc_domain()
> will never be called with IOMMU_DOMAIN_DMA for these drivers so it is safe
> to drop the test.
> 
> Removing these tests clarifies that the domain allocation path is only
> about the functionality of a paging domain and has nothing to do with
> policy of how the paging domain is used for UNMANAGED/DMA/DMA_FQ.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/fsl_pamu_domain.c | 7 ++-----
>  drivers/iommu/msm_iommu.c       | 7 ++-----
>  drivers/iommu/mtk_iommu_v1.c    | 7 ++-----
>  drivers/iommu/omap-iommu.c      | 7 ++-----
>  drivers/iommu/s390-iommu.c      | 7 ++-----
>  drivers/iommu/tegra-gart.c      | 7 ++-----
>  6 files changed, 12 insertions(+), 30 deletions(-)
> 
> 
---8<---
> -static struct iommu_domain *s390_domain_alloc(unsigned domain_type)
> +static struct iommu_domain *s390_domain_alloc_paging(struct device *dev)
>  {
>  	struct s390_domain *s390_domain;
>  
> -	if (domain_type != IOMMU_DOMAIN_UNMANAGED)
> -		return NULL;
> -
>  	s390_domain = kzalloc(sizeof(*s390_domain), GFP_KERNEL);
>  	if (!s390_domain)
>  		return NULL;
> @@ -447,7 +444,7 @@ void zpci_destroy_iommu(struct zpci_dev *zdev)
>  static const struct iommu_ops s390_iommu_ops = {
>  	.default_domain = &s390_iommu_platform_domain,
>  	.capable = s390_iommu_capable,
> -	.domain_alloc = s390_domain_alloc,
> +	.domain_alloc_paging = s390_domain_alloc_paging,

Leaving .domain_alloc unset here leads to an OOPs with your GitHub
branch (iommu_mandatory_default) when I try to use vfio-pci for KVM
pass-through via the following call chain:

...
vfio_group_fops_unl_ioctl()
   vfio_group_ioctl_set_container()
      vfio_container_attach_group()
         iommu_group_claim_dma_owner()
            __iommu_take_dma_ownership()
               __iommu_group_alloc_blocking_domain()
               __iommu_domain_alloc(…, IOMMU_DOMAIN_BLOCKED)

The problem is that in __iommu_domain_alloc() a call to bus->iommu_ops-
>domain_alloc() is attempted for IOMMU_DOMAIN_BLCOKED even if the
function pointer is unset.

So I tried with the obvious fix:

@@ -1947,7 +1948,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
        if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
            bus->iommu_ops->domain_alloc_paging)
                domain = bus->iommu_ops->domain_alloc_paging(dev);
-       else
+       else if (bus->iommu_ops->domain_alloc)
                domain = bus->iommu_ops->domain_alloc(type);
        if (!domain)
                return NULL;

This then uses the fallback of an empty IOMMU_DOMAIN_UNMANAGED and I
get a working device in the guest. Also tried hot unplug where the
device is taken over by the host again. I think with my DMA API
conversion patches we can support blocking domains properly but for a
temporary solution the above may be acceptable.

>  	.probe_device = s390_iommu_probe_device,
>  	.release_device = s390_iommu_release_device,
>  	.device_group = generic_device_group,

Re: [PATCH 20/20] iommu: Convert remaining simple drivers to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, May 02, 2023 at 04:52:32PM +0200, Niklas Schnelle wrote:
> @@ -1947,7 +1948,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>         if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
>             bus->iommu_ops->domain_alloc_paging)
>                 domain = bus->iommu_ops->domain_alloc_paging(dev);
> -       else
> +       else if (bus->iommu_ops->domain_alloc)
>                 domain = bus->iommu_ops->domain_alloc(type);
>         if (!domain)
>                 return NULL;

Agh, yes, it should fail, this is right, I'll fold it in, thanks

> This then uses the fallback of an empty IOMMU_DOMAIN_UNMANAGED and I
> get a working device in the guest. Also tried hot unplug where the
> device is taken over by the host again.

Great, thanks, I'll add your tested-by for the s390 drivers.

> I think with my DMA API
> conversion patches we can support blocking domains properly but for a
> temporary solution the above may be acceptable.

Yes, this is a good idea, I encourage all drivers to implement at
least one of BLOCKING or IDENTITY as global static singletons that
can't fail - this will allow us to have cleaner error recovery flows.

Jason

Re: [PATCH 20/20] iommu: Convert remaining simple drivers to domain_alloc_paging()

[Niklas Schnelle]

On Tue, 2023-05-02 at 12:25 -0300, Jason Gunthorpe wrote:
> On Tue, May 02, 2023 at 04:52:32PM +0200, Niklas Schnelle wrote:
> > @@ -1947,7 +1948,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> >         if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
> >             bus->iommu_ops->domain_alloc_paging)
> >                 domain = bus->iommu_ops->domain_alloc_paging(dev);
> > -       else
> > +       else if (bus->iommu_ops->domain_alloc)
> >                 domain = bus->iommu_ops->domain_alloc(type);
> >         if (!domain)
> >                 return NULL;
> 
> Agh, yes, it should fail, this is right, I'll fold it in, thanks
> 
> > This then uses the fallback of an empty IOMMU_DOMAIN_UNMANAGED and I
> > get a working device in the guest. Also tried hot unplug where the
> > device is taken over by the host again.
> 
> Great, thanks, I'll add your tested-by for the s390 drivers.

Yes and with the above change feel free to add my for this patch and
see my other reply for the s390 specific change.

Acked-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390

(I've recently been added as additional S390 IOMMU (PCI) maintainer)

Re: [PATCH 00/20] iommu: Make default_domain's mandatory

[Heiko Stübner]

Hi,

Am Montag, 1. Mai 2023, 20:02:44 CEST schrieb Jason Gunthorpe:
> This follows the prior series:
> 
> https://lore.kernel.org/r/0-v4-79d0c229580a+650-iommu_err_unwind_jgg@nvidia.com

which probably will need an update for Greg's recent
b18d0a0f92a8 ("iommu: make the pointer to struct bus_type constant")

I've fixed that locally for me for testing

Heiko

Re: [PATCH 00/20] iommu: Make default_domain's mandatory

[Jason Gunthorpe]

On Mon, May 01, 2023 at 11:34:45PM +0200, Heiko Stübner wrote:
> Hi,
> 
> Am Montag, 1. Mai 2023, 20:02:44 CEST schrieb Jason Gunthorpe:
> > This follows the prior series:
> > 
> > https://lore.kernel.org/r/0-v4-79d0c229580a+650-iommu_err_unwind_jgg@nvidia.com
> 
> which probably will need an update for Greg's recent
> b18d0a0f92a8 ("iommu: make the pointer to struct bus_type constant")
> 
> I've fixed that locally for me for testing

Yes, I will rebase it to rc1 next week

Thanks,
Jason

Re: [PATCH 00/20] iommu: Make default_domain's mandatory

[Heiko Stübner]

Am Montag, 1. Mai 2023, 20:02:44 CEST schrieb Jason Gunthorpe:
> Jason Gunthorpe (20):
>   iommu: Add IOMMU_DOMAIN_PLATFORM
>   iommu/terga-gart: Replace set_platform_dma_ops() with
>     IOMMU_DOMAIN_PLATFORM
>   iommu/s390: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM
>   iommu/fsl_pamu: Replace set_platform_dma_ops() with
>     IOMMU_DOMAIN_PLATFORM
>   iommu: Allow an IDENTITY domain as the default_domain in ARM32
>   iommu/exynos: Implement an IDENTITY domain
>   iommu/tegra-smmu: Implement an IDENTITY domain
>   iommu/tegra-smmu: Support DMA domains in tegra
>   iommu/omap: Implement an IDENTITY domain
>   iommu/msm: Implement an IDENTITY domain
>   iommu/mtk_iommu_v1: Implement an IDENTITY domain
>   iommu: Remove ops->set_platform_dma_ops()
>   iommu/qcom_iommu: Add an IOMMU_IDENTITIY_DOMAIN
>   iommu/ipmmu: Add an IOMMU_IDENTITIY_DOMAIN
>   iommu/mtk_iommu: Add an IOMMU_IDENTITIY_DOMAIN
>   iommu/sun50i: Add an IOMMU_IDENTITIY_DOMAIN
>   iommu: Require a default_domain for all iommu drivers
>   iommu: Add ops->domain_alloc_paging()
>   iommu: Convert simple drivers with DOMAIN_DMA to domain_alloc_paging()
>   iommu: Convert remaining simple drivers to domain_alloc_paging()

Also needs an update for Greg's
b18d0a0f92a8 ("iommu: make the pointer to struct bus_type constant")

With that fixed, both my rk3288-pinky (arm32) and px30-minievb (arm64)
Rockchip boards keep their display working before and after applying this
series (on top of the previous), so


Tested-by: Heiko Stuebner <heiko@sntech.de>