[PATCH 3/4] iommu/tegra-gart: Add iommu_group support

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.

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

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 37e708fdbb5a..29bafc6e82ae 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -334,12 +334,31 @@ static bool gart_iommu_capable(enum iommu_cap cap)
return false;
}

+static int gart_iommu_add_device(struct device *dev)
+{
+ struct iommu_group *group = iommu_group_get_for_dev(dev);
+
+ if (IS_ERR(group))
+ return PTR_ERR(group);
+
+ iommu_group_put(group);
+ return 0;
+}
+
+static void gart_iommu_remove_device(struct device *dev)
+{
+ iommu_group_remove_device(dev);
+}
+
static const struct iommu_ops gart_iommu_ops = {
.capable = gart_iommu_capable,
.domain_alloc = gart_iommu_domain_alloc,
.domain_free = gart_iommu_domain_free,
.attach_dev = gart_iommu_attach_dev,
.detach_dev = gart_iommu_detach_dev,
+ .add_device = gart_iommu_add_device,
+ .remove_device = gart_iommu_remove_device,
+ .device_group = generic_device_group,
.map = gart_iommu_map,
.map_sg = default_iommu_map_sg,
.unmap = gart_iommu_unmap,

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect
the isolation capabilities of the hardware (tegra_smmu_swgroup sounds
suspiciously like something that might warrant representing at the
iommu_group level), using generic_device_group() should at least
maintain existing behaviour with respect to the API.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/tegra-smmu.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index eeb19f560a05..faa9c1e70482 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -704,6 +704,7 @@ static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
static int tegra_smmu_add_device(struct device *dev)
{
struct device_node *np = dev->of_node;
+ struct iommu_group *group;
struct of_phandle_args args;
unsigned int index = 0;

@@ -725,12 +726,19 @@ static int tegra_smmu_add_device(struct device *dev)
index++;
}

+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
+
+ iommu_group_put(group);
+
return 0;
}

static void tegra_smmu_remove_device(struct device *dev)
{
dev->archdata.iommu = NULL;
+ iommu_group_remove_device(dev);
}

static const struct iommu_ops tegra_smmu_ops = {
@@ -741,6 +749,7 @@ static const struct iommu_ops tegra_smmu_ops = {
.detach_dev = tegra_smmu_detach_dev,
.add_device = tegra_smmu_add_device,
.remove_device = tegra_smmu_remove_device,
+ .device_group = generic_device_group,
.map = tegra_smmu_map,
.unmap = tegra_smmu_unmap,
.map_sg = default_iommu_map_sg,

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d0448353d501..04f4d51ffacb 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
static int msm_iommu_add_device(struct device *dev)
{
struct msm_iommu_dev *iommu;
+ struct iommu_group *group;
unsigned long flags;
int ret = 0;

@@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)

spin_unlock_irqrestore(&msm_iommu_lock, flags);

- return ret;
+ if (ret)
+ return ret;
+
+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
+
+ iommu_group_put(group);
+
+ return 0;
}

static void msm_iommu_remove_device(struct device *dev)
@@ -421,6 +431,8 @@ static void msm_iommu_remove_device(struct device *dev)
iommu_device_unlink(&iommu->iommu, dev);

spin_unlock_irqrestore(&msm_iommu_lock, flags);
+
+ iommu_group_remove_device(dev);
}

static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -700,6 +712,7 @@ static struct iommu_ops msm_iommu_ops = {
.iova_to_phys = msm_iommu_iova_to_phys,
.add_device = msm_iommu_add_device,
.remove_device = msm_iommu_remove_device,
+ .device_group = generic_device_group,
.pgsize_bitmap = MSM_IOMMU_PGSIZES,
.of_xlate = qcom_iommu_of_xlate,
};

[Sricharan R]

Hi Robin,

Post by Robin Murphy
As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.
---
drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d0448353d501..04f4d51ffacb 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
static int msm_iommu_add_device(struct device *dev)
{
struct msm_iommu_dev *iommu;
+ struct iommu_group *group;
unsigned long flags;
int ret = 0;
@@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
spin_unlock_irqrestore(&msm_iommu_lock, flags);
- return ret;
+ if (ret)
+ return ret;
+
+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
+
+ iommu_group_put(group);
+
+ return 0;
}

While this is correct for completing the group support, this adds the default domain and
that might break in the driver while attaching a private domain. The msm_iomm_attach_dev
needs to be fixed by calling msm_iommu_detach_dev while trying to attach a new domain when
already connected to a default one. But let me test and confirm this.

Regards,
Sricharan

[Robin Murphy]

Post by Sricharan R
Hi Robin,

Post by Robin Murphy
As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.
---
drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d0448353d501..04f4d51ffacb 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
static int msm_iommu_add_device(struct device *dev)
{
struct msm_iommu_dev *iommu;
+ struct iommu_group *group;
unsigned long flags;
int ret = 0;
@@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
spin_unlock_irqrestore(&msm_iommu_lock, flags);
- return ret;
+ if (ret)
+ return ret;
+
+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
+
+ iommu_group_put(group);
+
+ return 0;
}

While this is correct for completing the group support, this adds the default domain and
that might break in the driver while attaching a private domain. The msm_iomm_attach_dev
needs to be fixed by calling msm_iommu_detach_dev while trying to attach a new domain when
already connected to a default one. But let me test and confirm this.

The default domain shouldn't matter, since msm_iommu_domain_alloc() will
refuse to create DMA ops or identity domains in the first place. In the
absence of a default domain, __iommu_detach_group() will still end up
calling ops->detach_dev, so I don't think the ultimate behaviour is any
different with this change. Testing is of course welcome, though ;)

Robin.

[Robin Murphy]

Now that all the drivers properly implementing the IOMMU API support
groups (I'm ignoring the etnaviv GPU MMUs which seemingly only do just
enough to convince the ARM DMA mapping ops), we can remove the FIXME
workarounds from the core code. In the process, it also seems logical to
make the .device_group callback non-optional for drivers calling
iommu_group_get_for_dev() - the current callers all implement it anyway,
and it doesn't make sense for any future callers not to either.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/iommu.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3f6ea160afed..af69bf7e035a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1005,11 +1005,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
if (group)
return group;

- group = ERR_PTR(-EINVAL);
-
- if (ops && ops->device_group)
- group = ops->device_group(dev);
+ if (!ops)
+ return ERR_PTR(-EINVAL);

+ group = ops->device_group(dev);
if (WARN_ON_ONCE(group == NULL))
return ERR_PTR(-EINVAL);

@@ -1298,12 +1297,8 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
int ret;

group = iommu_group_get(dev);
- /* FIXME: Remove this when groups a mandatory for iommu drivers */
- if (group == NULL)
- return __iommu_attach_device(domain, dev);
-
/*
- * We have a group - lock it to make sure the device-count doesn't
+ * Lock the group to make sure the device-count doesn't
* change while we are attaching
*/
mutex_lock(&group->mutex);
@@ -1336,9 +1331,6 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
struct iommu_group *group;

group = iommu_group_get(dev);
- /* FIXME: Remove this when groups a mandatory for iommu drivers */
- if (group == NULL)
- return __iommu_detach_device(domain, dev);

mutex_lock(&group->mutex);
if (iommu_group_device_count(group) != 1) {
@@ -1360,9 +1352,6 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
struct iommu_group *group;

group = iommu_group_get(dev);
- /* FIXME: Remove this when groups a mandatory for iommu drivers */
- if (group == NULL)
- return NULL;

domain = group->domain;