[1/4] iommu: Add a broken_unmanaged_domain flag in iommu_ops

  Both IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA require the support
of __IOMMU_DOMAIN_PAGING capability, i.e. iommu_map/unmap. However,
some older iommu drivers do not fully support that, and these drivers
also do not advertise support for dma-iommu.c via IOMMU_DOMAIN_DMA,
or use arm_iommu_create_mapping(), so largely their implementations
of IOMMU_DOMAIN_UNMANAGED are untested. This means that a user like
vfio/iommufd does not likely work with them.

Several of them have obvious problems:
  * fsl_pamu_domain.c
    Without map/unmap ops in the default_domain_ops, it isn't an
    unmanaged domain at all.
  * mtk_iommu_v1.c
    With a fixed 4M "pagetable", it can only map exactly 4G of
    memory, but doesn't set the aperture.
  * tegra-gart.c
    Its notion of attach/detach and groups has to be a complete lie to
    get around all the other API expectations.

Some others might work but have never been tested with vfio/iommufd:
  * msm_iommu.c
  * omap-iommu.c
  * tegra-smmu.c

Thus, mark all these drivers as having "broken" UNAMANGED domains and
add a new device_iommu_unmanaged_supported() API for vfio/iommufd and
dma-iommu to refuse to work with these drivers.

Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/fsl_pamu_domain.c |  1 +
 drivers/iommu/iommu.c           | 24 ++++++++++++++++++++++++
 drivers/iommu/msm_iommu.c       |  1 +
 drivers/iommu/mtk_iommu_v1.c    |  1 +
 drivers/iommu/omap-iommu.c      |  1 +
 drivers/iommu/tegra-gart.c      |  1 +
 drivers/iommu/tegra-smmu.c      |  1 +
 include/linux/iommu.h           | 11 +++++++++++
 8 files changed, 41 insertions(+)
  

On 2023-01-27 20:04, Nicolin Chen wrote:
> Both IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA require the support
> of __IOMMU_DOMAIN_PAGING capability, i.e. iommu_map/unmap. However,
> some older iommu drivers do not fully support that, and these drivers
> also do not advertise support for dma-iommu.c via IOMMU_DOMAIN_DMA,
> or use arm_iommu_create_mapping(), so largely their implementations
> of IOMMU_DOMAIN_UNMANAGED are untested. This means that a user like
> vfio/iommufd does not likely work with them.
> 
> Several of them have obvious problems:
>    * fsl_pamu_domain.c
>      Without map/unmap ops in the default_domain_ops, it isn't an
>      unmanaged domain at all.
>    * mtk_iommu_v1.c
>      With a fixed 4M "pagetable", it can only map exactly 4G of
>      memory, but doesn't set the aperture.

The aperture is easily fixed (one could argue that what's broken there 
are the ARM DMA ops for assuming every IOMMU has a 32-bit IOVA space and 
not checking).

>    * tegra-gart.c
>      Its notion of attach/detach and groups has to be a complete lie to
>      get around all the other API expectations.

That's true, and the domain is tiny and not isolated from the rest of 
the address space outside the aperture, but the one thing it does do is 
support iommu_map/unmap just fine, which is what this flag is documented 
as saying it doesn't.

> Some others might work but have never been tested with vfio/iommufd:
>    * msm_iommu.c
>    * omap-iommu.c
>    * tegra-smmu.c

And yet they all have other in-tree users (GPUs on MSM and Tegra, 
remoteproc on OMAP) that allocate unmanaged domains and use 
iommu_map/unmap just fine, so they're clearly not broken either.

On the flipside, you're also missing cases like apple-dart, which can 
have broken unmanaged domains by any definition, but only under certain 
conditions (at least it "fails safe" and they will refuse attempts to 
attach anything). I'd also question sprd-iommu, which hardly has a 
generally-useful domain size, and has only just recently gained the 
ability to unmap anything successfully. TBH none of the SoC IOMMUs are 
likely to ever be of interest to VFIO or IOMMUFD, since the only things 
they could assign to userspace are the individual devices - usually 
graphics and media engines - that they're coupled to, whose useful 
functionality tends to depend on clocks, phys, and random other 
low-level stuff that would be somewhere between impractical and 
downright unsafe to attempt to somehow expose as well.

> Thus, mark all these drivers as having "broken" UNAMANGED domains and
> add a new device_iommu_unmanaged_supported() API for vfio/iommufd and
> dma-iommu to refuse to work with these drivers.
> 
> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

[...]

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 46e1347bfa22..919a5dbad75b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -245,6 +245,10 @@ struct iommu_iotlb_gather {
>    *                    pasid, so that any DMA transactions with this pasid
>    *                    will be blocked by the hardware.
>    * @pgsize_bitmap: bitmap of all possible supported page sizes
> + * @broken_unmanaged_domain: IOMMU_DOMAIN_UNMANAGED is not fully functional; the
> + *                           driver does not really support iommu_map/unmap, but
> + *                           uses UNMANAGED domains for the IOMMU API, called by
> + *                           other SOC drivers.

"uses UNMANAGED domains for the IOMMU API" is literally the definition 
of unmanaged domains :/

Some "other SOC drivers" use more of the IOMMU API than VFIO does :/

Please just add IOMMU_CAP_IOMMUFD to represent whatever the nebulous 
requirements of IOMMUFD actually are (frankly it's no less informative 
than calling domains "broken"), handle that in the drivers you care 
about and have tested, and use device_iommu_capable(). What you're 
describing in this series is a capability, and we have a perfectly good 
API for drivers to express those already. Plus, as demonstrated above, a 
positive capability based on empirical testing will be infinitely more 
robust than a negative one based on guessing.

Thanks,
Robin.
  

Hi Robin.

On Fri, Jan 27, 2023 at 09:58:46PM +0000, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2023-01-27 20:04, Nicolin Chen wrote:
> > Both IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA require the support
> > of __IOMMU_DOMAIN_PAGING capability, i.e. iommu_map/unmap. However,
> > some older iommu drivers do not fully support that, and these drivers
> > also do not advertise support for dma-iommu.c via IOMMU_DOMAIN_DMA,
> > or use arm_iommu_create_mapping(), so largely their implementations
> > of IOMMU_DOMAIN_UNMANAGED are untested. This means that a user like
> > vfio/iommufd does not likely work with them.
> > 
> > Several of them have obvious problems:
> >    * fsl_pamu_domain.c
> >      Without map/unmap ops in the default_domain_ops, it isn't an
> >      unmanaged domain at all.
> >    * mtk_iommu_v1.c
> >      With a fixed 4M "pagetable", it can only map exactly 4G of
> >      memory, but doesn't set the aperture.
> 
> The aperture is easily fixed (one could argue that what's broken there
> are the ARM DMA ops for assuming every IOMMU has a 32-bit IOVA space and
> not checking).
>
> >    * tegra-gart.c
> >      Its notion of attach/detach and groups has to be a complete lie to
> >      get around all the other API expectations.
> 
> That's true, and the domain is tiny and not isolated from the rest of
> the address space outside the aperture, but the one thing it does do is
> support iommu_map/unmap just fine, which is what this flag is documented
> as saying it doesn't.
> 
> > Some others might work but have never been tested with vfio/iommufd:
> >    * msm_iommu.c
> >    * omap-iommu.c
> >    * tegra-smmu.c
> 
> And yet they all have other in-tree users (GPUs on MSM and Tegra,
> remoteproc on OMAP) that allocate unmanaged domains and use
> iommu_map/unmap just fine, so they're clearly not broken either.
> 
> On the flipside, you're also missing cases like apple-dart, which can
> have broken unmanaged domains by any definition, but only under certain
> conditions (at least it "fails safe" and they will refuse attempts to
> attach anything). I'd also question sprd-iommu, which hardly has a
> generally-useful domain size, and has only just recently gained the
> ability to unmap anything successfully. TBH none of the SoC IOMMUs are
> likely to ever be of interest to VFIO or IOMMUFD, since the only things
> they could assign to userspace are the individual devices - usually
> graphics and media engines - that they're coupled to, whose useful
> functionality tends to depend on clocks, phys, and random other
> low-level stuff that would be somewhere between impractical and
> downright unsafe to attempt to somehow expose as well.

Thanks for all the inputs.

> > Thus, mark all these drivers as having "broken" UNAMANGED domains and
> > add a new device_iommu_unmanaged_supported() API for vfio/iommufd and
> > dma-iommu to refuse to work with these drivers.
> > 
> > Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> [...]
> 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 46e1347bfa22..919a5dbad75b 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -245,6 +245,10 @@ struct iommu_iotlb_gather {
> >    *                    pasid, so that any DMA transactions with this pasid
> >    *                    will be blocked by the hardware.
> >    * @pgsize_bitmap: bitmap of all possible supported page sizes
> > + * @broken_unmanaged_domain: IOMMU_DOMAIN_UNMANAGED is not fully functional; the
> > + *                           driver does not really support iommu_map/unmap, but
> > + *                           uses UNMANAGED domains for the IOMMU API, called by
> > + *                           other SOC drivers.
> 
> "uses UNMANAGED domains for the IOMMU API" is literally the definition
> of unmanaged domains :/
> 
> Some "other SOC drivers" use more of the IOMMU API than VFIO does :/
> 
> Please just add IOMMU_CAP_IOMMUFD to represent whatever the nebulous
> requirements of IOMMUFD actually are (frankly it's no less informative
> than calling domains "broken"), handle that in the drivers you care
> about and have tested, and use device_iommu_capable(). What you're
> describing in this series is a capability, and we have a perfectly good
> API for drivers to express those already. Plus, as demonstrated above, a
> positive capability based on empirical testing will be infinitely more
> robust than a negative one based on guessing.

OK. I can change to IOMMU_CAP_IOMMUFD, and add to the drivers that
are tested. And an IOMMU driver that wants to use IOMMUFD can add
such a CAP later whenever it's ready.

Yet, "IOMMU_CAP_IOMMUFD" would make the VFIO change suspicious, so
perhaps the next version is just one CAP patch + one IOMMUFD patch.
@Jason, any concern?

Thank you
Nicolin
  

On Fri, Jan 27, 2023 at 09:58:46PM +0000, Robin Murphy wrote:

> Please just add IOMMU_CAP_IOMMUFD to represent whatever the nebulous
> requirements of IOMMUFD actually are (frankly it's no less informative than
> calling domains "broken"), handle that in the drivers you care about
> and

I don't want to tie this to iommufd, that isn't the point.

We clearly have drivers that don't implement the iommu kernel API
properly, because their only use is between the iommu driver and some
other same-SOC driver.

As a user of the iommu API iommufd and VFIO want to avoid these
drivers.

We have that acknowledgment already via the IOMMU_DOMAIN_DMA stuff
protecting the dma_iommu.c from those same drivers.

So, how about this below instead. Imagine it is followed by something along
the lines of my other sketch:

https://lore.kernel.org/linux-iommu/Y4%2FLsZKmR3iWFphU@nvidia.com/

And we completely delete IOMMU_DOMAIN_DMA/_FQ and get dma-iommu.c
mostly out of driver code.

iommufd/vfio would refuse to work with drivers that don't indicate
they support dma_iommu.c, that is good enough.

We'd eventually rename IOMMU_DOMAIN_UNMANAGED to IOMMU_DOMAIN_MAPPING.

Subject: [PATCH] iommu: Remove IOMMU_DOMAIN_DMA from drivers

The IOMMU_DOMAIN_DMA is exactly the same driver functionality as
IOMMU_DOMAIN_UNMANAGED, except it also implies that dma_iommu.c should
be able to use this driver.

As a first step to removing IOMMU_DOMAIN_DMA remove all references to
it from the drivers. Two simple ops flags are enough to indicate if
the driver is compatible with dma_iommu.c and if the driver will allow
the FQ mode to be selected. When dma-iommu.c needs an a domain it will
request an IOMMU_DOMAIN_UNMANAGED.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/apple-dart.c                  |  3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  4 +--
 drivers/iommu/arm/arm-smmu/arm-smmu.c       | 11 ++++----
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |  3 ++-
 drivers/iommu/exynos-iommu.c                |  3 ++-
 drivers/iommu/intel/iommu.c                 |  3 +--
 drivers/iommu/iommu.c                       | 29 ++++++++++++++++-----
 drivers/iommu/ipmmu-vmsa.c                  |  3 ++-
 drivers/iommu/mtk_iommu.c                   |  3 ++-
 drivers/iommu/rockchip-iommu.c              |  3 ++-
 drivers/iommu/sprd-iommu.c                  |  3 ++-
 drivers/iommu/sun50i-iommu.c                |  4 +--
 drivers/iommu/virtio-iommu.c                |  2 +-
 include/linux/iommu.h                       |  2 ++
 14 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 4f4a323be0d0ff..0eb3eb857d7e9e 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -580,7 +580,7 @@ static struct iommu_domain *apple_dart_domain_alloc(unsigned int type)
 {
 	struct apple_dart_domain *dart_domain;
 
-	if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED &&
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
 	    type != IOMMU_DOMAIN_IDENTITY && type != IOMMU_DOMAIN_BLOCKED)
 		return NULL;
 
@@ -769,6 +769,7 @@ static void apple_dart_get_resv_regions(struct device *dev,
 }
 
 static const struct iommu_ops apple_dart_iommu_ops = {
+	.use_dma_iommu = true,
 	.domain_alloc = apple_dart_domain_alloc,
 	.probe_device = apple_dart_probe_device,
 	.release_device = apple_dart_release_device,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index ab160198edd6b1..2253c5598092d0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2013,8 +2013,6 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 		return arm_smmu_sva_domain_alloc();
 
 	if (type != IOMMU_DOMAIN_UNMANAGED &&
-	    type != IOMMU_DOMAIN_DMA &&
-	    type != IOMMU_DOMAIN_DMA_FQ &&
 	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
@@ -2844,6 +2842,8 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.use_dma_iommu		= true,
+	.allow_dma_fq		= true,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 719fbca1fe52a0..7bb160bdc4b594 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -855,11 +855,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
-		if (using_legacy_binding ||
-		    (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_DMA_FQ))
-			return NULL;
-	}
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY)
+		return NULL;
+
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really do anything meaningful until we've added a
@@ -1555,6 +1553,8 @@ static int arm_smmu_def_domain_type(struct device *dev)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.use_dma_iommu		= true,
+	.allow_dma_fq		= true,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
@@ -1982,6 +1982,7 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,
 				  IS_ENABLED(CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS) ? "DMA API" : "SMMU");
 		}
 		using_legacy_binding = true;
+		arm_smmu_ops.use_dma_iommu = false;
 	} else if (!legacy_binding && !using_legacy_binding) {
 		using_generic_binding = true;
 	} else {
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 270c3d9128bab8..babd20108f6a17 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -323,7 +323,7 @@ static struct iommu_domain *qcom_iommu_domain_alloc(unsigned type)
 {
 	struct qcom_iommu_domain *qcom_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
@@ -575,6 +575,7 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 }
 
 static const struct iommu_ops qcom_iommu_ops = {
+	.use_dma_iommu  = true,
 	.capable	= qcom_iommu_capable,
 	.domain_alloc	= qcom_iommu_domain_alloc,
 	.probe_device	= qcom_iommu_probe_device,
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0cde22119875e..824350551e5933 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -825,7 +825,7 @@ 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)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
@@ -1396,6 +1396,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops exynos_iommu_ops = {
+	.use_dma_iommu = true,
 	.domain_alloc = exynos_iommu_domain_alloc,
 	.device_group = generic_device_group,
 	.probe_device = exynos_iommu_probe_device,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 59df7e42fd533c..bb34d3f641f17b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4165,8 +4165,6 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
 	switch (type) {
 	case IOMMU_DOMAIN_BLOCKED:
 		return &blocking_domain;
-	case IOMMU_DOMAIN_DMA:
-	case IOMMU_DOMAIN_DMA_FQ:
 	case IOMMU_DOMAIN_UNMANAGED:
 		dmar_domain = alloc_domain(type);
 		if (!dmar_domain) {
@@ -4761,6 +4759,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 }
 
 const struct iommu_ops intel_iommu_ops = {
+	.use_dma_iommu		= true,
 	.capable		= intel_iommu_capable,
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.probe_device		= intel_iommu_probe_device,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705bd3..877ef0a58b07f4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1618,17 +1618,32 @@ static int iommu_group_alloc_default_domain(struct bus_type *bus,
 {
 	struct iommu_domain *dom;
 
-	dom = __iommu_domain_alloc(bus, type);
-	if (!dom && type != IOMMU_DOMAIN_DMA) {
-		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
-		if (dom)
+	/*
+	 * Keep the DMA domain type out of the drivers, eventually it will go
+	 * away completely.
+	 */
+	if (type == IOMMU_DOMAIN_IDENTITY) {
+		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_IDENTITY);
+		if (!dom)
+			return -ENOMEM;
+	} else if (type == IOMMU_DOMAIN_DMA_FQ || type == IOMMU_DOMAIN_DMA) {
+		if (!bus->iommu_ops->use_dma_iommu)
+			return -EINVAL;
+
+		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+		if (!dom)
+			return -ENOMEM;
+		if (type == IOMMU_DOMAIN_DMA_FQ &&
+		    !bus->iommu_ops->allow_dma_fq) {
 			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
 				type, group->name);
+			type = IOMMU_DOMAIN_DMA;
+		}
+		dom->type = type;
+	} else {
+		return -EINVAL;
 	}
 
-	if (!dom)
-		return -ENOMEM;
-
 	group->default_domain = dom;
 	if (!group->domain)
 		group->domain = dom;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index a003bd5fc65c13..a22df69f7f4775 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -571,7 +571,7 @@ static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
 {
 	struct ipmmu_vmsa_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
@@ -866,6 +866,7 @@ static struct iommu_group *ipmmu_find_group(struct device *dev)
 }
 
 static const struct iommu_ops ipmmu_ops = {
+	.use_dma_iommu = true,
 	.domain_alloc = ipmmu_domain_alloc,
 	.probe_device = ipmmu_probe_device,
 	.release_device = ipmmu_release_device,
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 2badd6acfb23d6..e27cb428bd9583 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -636,7 +636,7 @@ static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
 {
 	struct mtk_iommu_domain *dom;
 
-	if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
@@ -936,6 +936,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev,
 }
 
 static const struct iommu_ops mtk_iommu_ops = {
+	.use_dma_iommu  = true,
 	.domain_alloc	= mtk_iommu_domain_alloc,
 	.probe_device	= mtk_iommu_probe_device,
 	.release_device	= mtk_iommu_release_device,
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index a68eadd64f38db..fa3ec38e5244db 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1061,7 +1061,7 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
 {
 	struct rk_iommu_domain *rk_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	if (!dma_dev)
@@ -1184,6 +1184,7 @@ static int rk_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops rk_iommu_ops = {
+	.use_dma_iommu = true,
 	.domain_alloc = rk_iommu_domain_alloc,
 	.probe_device = rk_iommu_probe_device,
 	.release_device = rk_iommu_release_device,
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 219bfa11f7f48f..fbff1831c16e78 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -136,7 +136,7 @@ static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
 {
 	struct sprd_iommu_domain *dom;
 
-	if (domain_type != IOMMU_DOMAIN_DMA && domain_type != IOMMU_DOMAIN_UNMANAGED)
+	if (domain_type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
@@ -406,6 +406,7 @@ static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 
 
 static const struct iommu_ops sprd_iommu_ops = {
+	.use_dma_iommu  = true,
 	.domain_alloc	= sprd_iommu_domain_alloc,
 	.probe_device	= sprd_iommu_probe_device,
 	.device_group	= sprd_iommu_device_group,
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 5b585eace3d46f..de2a033f65197a 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -671,8 +671,7 @@ static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)
 {
 	struct sun50i_iommu_domain *sun50i_domain;
 
-	if (type != IOMMU_DOMAIN_DMA &&
-	    type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
 	sun50i_domain = kzalloc(sizeof(*sun50i_domain), GFP_KERNEL);
@@ -827,6 +826,7 @@ static int sun50i_iommu_of_xlate(struct device *dev,
 }
 
 static const struct iommu_ops sun50i_iommu_ops = {
+	.use_dma_iommu  = true,
 	.pgsize_bitmap	= SZ_4K,
 	.device_group	= sun50i_iommu_device_group,
 	.domain_alloc	= sun50i_iommu_domain_alloc,
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 5b8fe9bfa9a5b9..8afa21a9c0b9d6 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -642,7 +642,6 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type)
 	struct viommu_domain *vdomain;
 
 	if (type != IOMMU_DOMAIN_UNMANAGED &&
-	    type != IOMMU_DOMAIN_DMA &&
 	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
@@ -1018,6 +1017,7 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
 }
 
 static struct iommu_ops viommu_ops = {
+	.use_dma_iommu		= true,
 	.capable		= viommu_capable,
 	.domain_alloc		= viommu_domain_alloc,
 	.probe_device		= viommu_probe_device,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa2286..954db8e77491c7 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -277,6 +277,8 @@ struct iommu_ops {
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
+	u8 use_dma_iommu : 1;
+	u8 allow_dma_fq : 1;
 	struct module *owner;
 };
  

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Saturday, January 28, 2023 4:04 AM
> 
> Both IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA require
> the support
> of __IOMMU_DOMAIN_PAGING capability, i.e. iommu_map/unmap.
> However,
> some older iommu drivers do not fully support that, and these drivers
> also do not advertise support for dma-iommu.c via IOMMU_DOMAIN_DMA,
> or use arm_iommu_create_mapping(), so largely their implementations
> of IOMMU_DOMAIN_UNMANAGED are untested. This means that a user like
> vfio/iommufd does not likely work with them.
> 
> Several of them have obvious problems:
>   * fsl_pamu_domain.c
>     Without map/unmap ops in the default_domain_ops, it isn't an
>     unmanaged domain at all.
>   * mtk_iommu_v1.c
>     With a fixed 4M "pagetable", it can only map exactly 4G of
>     memory, but doesn't set the aperture.
>   * tegra-gart.c
>     Its notion of attach/detach and groups has to be a complete lie to
>     get around all the other API expectations.
> 
> Some others might work but have never been tested with vfio/iommufd:
>   * msm_iommu.c
>   * omap-iommu.c
>   * tegra-smmu.c
> 

Do we have a link where all drivers tested with vfio/iommufd have been
listed?

In a quick glance at least exynos-iommu.c and apple-dart.c both support
UNMANAGED with map/unmap ops. They are not mentioned in above
list but I doubt they are tested for vfio/iommufd.
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, January 28, 2023 7:54 AM
> 
> On Fri, Jan 27, 2023 at 09:58:46PM +0000, Robin Murphy wrote:
> 
> > Please just add IOMMU_CAP_IOMMUFD to represent whatever the
> nebulous
> > requirements of IOMMUFD actually are (frankly it's no less informative
> than
> > calling domains "broken"), handle that in the drivers you care about
> > and
> 
> I don't want to tie this to iommufd, that isn't the point.
> 
> We clearly have drivers that don't implement the iommu kernel API
> properly, because their only use is between the iommu driver and some
> other same-SOC driver.
> 
> As a user of the iommu API iommufd and VFIO want to avoid these
> drivers.
> 
> We have that acknowledgment already via the IOMMU_DOMAIN_DMA stuff
> protecting the dma_iommu.c from those same drivers.
> 
> So, how about this below instead. Imagine it is followed by something along
> the lines of my other sketch:
> 
> https://lore.kernel.org/linux-iommu/Y4%2FLsZKmR3iWFphU@nvidia.com/
> 
> And we completely delete IOMMU_DOMAIN_DMA/_FQ and get dma-
> iommu.c
> mostly out of driver code.
> 
> iommufd/vfio would refuse to work with drivers that don't indicate
> they support dma_iommu.c, that is good enough.

this is a good idea. Just one doubt. Robin mentioned that sprd-iommu
might have a broken unmanaged domains:

" I'd also question sprd-iommu, which hardly has a generally-useful
domain size, and has only just recently gained the ability to unmap
anything successfully."

Want to understand why that restriction is not a problem for DMA API.

what Jason proposed here assumes that existing driver support for
DMA API can be safely applied to vfio/iommufd. But above looks
warning certain exception?

> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 59df7e42fd533c..bb34d3f641f17b 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4165,8 +4165,6 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type)
>  	switch (type) {
>  	case IOMMU_DOMAIN_BLOCKED:
>  		return &blocking_domain;
> -	case IOMMU_DOMAIN_DMA:
> -	case IOMMU_DOMAIN_DMA_FQ:
>  	case IOMMU_DOMAIN_UNMANAGED:
>  		dmar_domain = alloc_domain(type);
>  		if (!dmar_domain) {
> @@ -4761,6 +4759,7 @@ static void intel_iommu_remove_dev_pasid(struct
> device *dev, ioasid_t pasid)
>  }
> 
>  const struct iommu_ops intel_iommu_ops = {
> +	.use_dma_iommu		= true,

missed:
+	.allow_dma_fq			= true,
  

On Sun, Jan 29, 2023 at 08:11:48AM +0000, Tian, Kevin wrote:

> " I'd also question sprd-iommu, which hardly has a generally-useful
> domain size, and has only just recently gained the ability to unmap
> anything successfully."

So long as it has a correct kernel API and exposes the right (small)
aperture then it is OK.

The device will not be useful for qemu, but it would run some dpdk
configurations just fine.

All the VFIO world is not VMs.

> > @@ -4761,6 +4759,7 @@ static void intel_iommu_remove_dev_pasid(struct
> > device *dev, ioasid_t pasid)
> >  }
> > 
> >  const struct iommu_ops intel_iommu_ops = {
> > +	.use_dma_iommu		= true,
> 
> missed:
> +	.allow_dma_fq			= true,

Yep

Jason
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, January 30, 2023 9:33 PM
> 
> On Sun, Jan 29, 2023 at 08:11:48AM +0000, Tian, Kevin wrote:
> 
> > " I'd also question sprd-iommu, which hardly has a generally-useful
> > domain size, and has only just recently gained the ability to unmap
> > anything successfully."
> 
> So long as it has a correct kernel API and exposes the right (small)
> aperture then it is OK.
> 
> The device will not be useful for qemu, but it would run some dpdk
> configurations just fine.

I still didn't get the restriction here. Can you elaborate why it works
with dpdk but not qemu?

Can qemu verify this restriction via existing path or need new uAPI
flag to communicate?
  

On Wed, Feb 01, 2023 at 03:14:03AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Monday, January 30, 2023 9:33 PM
> > 
> > On Sun, Jan 29, 2023 at 08:11:48AM +0000, Tian, Kevin wrote:
> > 
> > > " I'd also question sprd-iommu, which hardly has a generally-useful
> > > domain size, and has only just recently gained the ability to unmap
> > > anything successfully."
> > 
> > So long as it has a correct kernel API and exposes the right (small)
> > aperture then it is OK.
> > 
> > The device will not be useful for qemu, but it would run some dpdk
> > configurations just fine.
> 
> I still didn't get the restriction here. Can you elaborate why it works
> with dpdk but not qemu?

dpdk needs like, say, 64M of aperture and doesn't care what the IOVAs
are

qemu needs the entire guest memory of aperture and must have IOVAs
that are 1:1 with the GPA.

So aperture size and location can exclude qemu

> Can qemu verify this restriction via existing path or need new uAPI
> flag to communicate?

It already happens, the aperture/etc is convayed to qemu through
IOMMUFD_CMD_IOAS_IOVA_RANGES and if qemu cannot get the IOVA's it
needs to create the guest it should fail.

Jason
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, February 1, 2023 10:56 PM
> 
> On Wed, Feb 01, 2023 at 03:14:03AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Monday, January 30, 2023 9:33 PM
> > >
> > > On Sun, Jan 29, 2023 at 08:11:48AM +0000, Tian, Kevin wrote:
> > >
> > > > " I'd also question sprd-iommu, which hardly has a generally-useful
> > > > domain size, and has only just recently gained the ability to unmap
> > > > anything successfully."
> > >
> > > So long as it has a correct kernel API and exposes the right (small)
> > > aperture then it is OK.
> > >
> > > The device will not be useful for qemu, but it would run some dpdk
> > > configurations just fine.
> >
> > I still didn't get the restriction here. Can you elaborate why it works
> > with dpdk but not qemu?
> 
> dpdk needs like, say, 64M of aperture and doesn't care what the IOVAs
> are
> 
> qemu needs the entire guest memory of aperture and must have IOVAs
> that are 1:1 with the GPA.
> 
> So aperture size and location can exclude qemu
> 
> > Can qemu verify this restriction via existing path or need new uAPI
> > flag to communicate?
> 
> It already happens, the aperture/etc is convayed to qemu through
> IOMMUFD_CMD_IOAS_IOVA_RANGES and if qemu cannot get the IOVA's it
> needs to create the guest it should fail.
> 

Make sense.