IOMMU group cleanup

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 <[email protected]>

---
 drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +3 -1

index d0448353d501..04f4d51ffacb 100644
@@ -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;

Hi Robin,

Quoted 36 lines

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 <[email protected]>

---
 drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +10 -1

index d0448353d501..04f4d51ffacb 100644
@@ -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);
+

drivers/iommu/msm_iommu.c static int msm_iommu_add_device(struct device *dev) · line 421

Quoted 3 lines

drivers/iommu/msm_iommu.c +1 -0

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

Quoted 38 lines · Sricharan R

Hi Robin,

Quoted 36 lines

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 <[email protected]>

---
 drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +10 -1

index d0448353d501..04f4d51ffacb 100644
@@ -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);
+

Quoted 8 lines

drivers/iommu/msm_iommu.c static int msm_iommu_add_device(struct device *dev) · line 421

Quoted 3 lines

drivers/iommu/msm_iommu.c +1 -0

+	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 ;)

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 <[email protected]>

---
 drivers/iommu/tegra-smmu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/iommu/tegra-smmu.c +9 -0

index eeb19f560a05..faa9c1e70482 100644
@@ -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,

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 <[email protected]>

---
 drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Robin Murphy (4):
iommu/msm: Add iommu_group support iommu/tegra-smmu: Add iommu_group support iommu/tegra-gart: Add iommu_group support
iommu: Finish making iommu_group support mandatory

 drivers/iommu/iommu.c      | 19 ++++---------------
 drivers/iommu/msm_iommu.c  | 15 ++++++++++++++-
 drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
 drivers/iommu/tegra-smmu.c |  9 +++++++++
 4 files changed, 46 insertions(+), 16 deletions(-)

Hi all,

Prompted by some discussion around the questionable usefulness of iommu_present() for SoC topologies, I took a look at what was left to finish off the intention of making groups mandatory. Turns out it wasn't much, so here it is. As well as the general rationalisation aspect, between this and the probe-deferral changes in 4.12, we should now be able to rely on the presence of a group unambiguously indicating whether a given device is associated with an IOMMU or not.

Robin.

Robin Murphy (4):
iommu/msm: Add iommu_group support iommu/tegra-smmu: Add iommu_group support iommu/tegra-gart: Add iommu_group support
iommu: Finish making iommu_group support mandatory