Re: [PATCH 1/4] iommu: Add a broken_unmanaged_domain flag in iommu_ops
From: Jason Gunthorpe
Date: Fri Jan 27 2023 - 18:54:49 EST
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@xxxxxxxxxx/
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@xxxxxxxxxx>
---
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;
};
--
2.39.0