iommu/ipmmu-vmsa: IPMMU slave device whitelist V2

iommu/ipmmu-vmsa: IPMMU slave device whitelist V2

[PATCH/RFC v2 1/4] iommu/of: Skip IOMMU devices disabled in DT [PATCH/RFC v2 2/4] iommu/ipmmu-vmsa: Get rid of disabled device check [PATCH/RFC v2 3/4] iommu/ipmmu-vmsa: Check devices in xlate() [PATCH/RFC v2 3/4] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version

Here's an updated prototype that shows how DT integration of IPMMU details may be integrated and merged upstream based on SoC data sheet ahead of time followed by enablement in the IPMMU driver code once the appropriate SoC ES version has been released and the hardware has been tested.

Changes since V1:
- Broke out patch 1 from the IPMMU driver
- Moved slave device check from ->add_device() to ->xlate() (Thanks Robin!)
- Updated white list patch to hook into ->xlate()

Patch 1 may be suitable for upstream merge, however other patches should in the future if agreed on be rolled into the IPMMU driver series.

---

Developed on top of renesas-drivers-2017-01-24-v4.10-rc5

 drivers/iommu/ipmmu-vmsa.c |   59 +++++++++++++++++++++++---------------------
 drivers/iommu/of_iommu.c   |    2 -
 2 files changed, 33 insertions(+), 28 deletions(-)

From: Magnus Damm <[email protected]>

Extend the shared IOMMU code to skip over ->xlate() in case the IOMMU device pointed to by a slave device has been disabled in DT.

Difficult to trigger in case a single IOMMU device is used, however when multiple IOMMUs are used and some of them are disabled in DT then this patch makes sure that ->xlate() only gets invoked for the enabled ones.

---

I used to keep this as a local check in the xlate() callback for the not-yet-merged-upstream R-Car Gen3 IPMMU driver stack.

Since honoring DT disabled devices probably makes sense for most users it seems like a good plan to try to push it into the common subsystem level.

Thanks to Geert for suggesting this ages ago.

Developed on top of renesas-drivers-2017-01-24-v4.10-rc5 which includes a recent version of iommu/next.

 drivers/iommu/of_iommu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -159,7 +159,7 @@ const struct iommu_ops *of_iommu_configu
 		np = iommu_spec.np;
 		ops = of_iommu_get_ops(np);
 
-		if (!ops || !ops->of_xlate ||
+		if (!ops || !ops->of_xlate || !of_device_is_available(np) ||
 		    iommu_fwspec_init(dev, &np->fwnode, ops) ||
 		    ops->of_xlate(dev, &iommu_spec))
 			goto err_put_node;

From: Magnus Damm <[email protected]>

Since of_iommu_configure() now skips over disabled devices we can simply drop this check in the IPMMU driver.

---

 drivers/iommu/ipmmu-vmsa.c |    7 -------
 1 file changed, 7 deletions(-)

@@ -1051,13 +1051,6 @@ static struct iommu_group *ipmmu_device_
 static int ipmmu_of_xlate_dma(struct device *dev,
 			      struct of_phandle_args *spec)
 {
-	/* If the IPMMU device is disabled in DT then return error
-	 * to make sure the of_iommu code does not install ops
-	 * even though the iommu device is disabled
-	 */
-	if (!of_device_is_available(spec->np))
-		return -ENODEV;
-
 	return 0;
 }
 

From: Magnus Damm <[email protected]>

Rework the IPMMU code to validate devices in ->xlate() instead of accepting all devices in xlate() and instead validating devices in ->add_device(). This makes it possible for the IPMMU device driver to reject slave devices based on software policy.

Once a slave device is rejected by the ->xlate() callback the shared function of_iommu_configure() will fail as well which in turn disables per-device IOMMU handing in the arch-specific mapping code by not passing any IOMMU callbacks to arch_setup_dma_ops().

---

TODO: Make sure this does not break R-Car Gen2 support

 drivers/iommu/ipmmu-vmsa.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

@@ -1007,16 +1007,14 @@ static int ipmmu_add_device_dma(struct d
 	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
 	struct iommu_group *group;
 
-	/* only accept devices with iommus property */
-	if (of_count_phandle_with_args(dev->of_node, "iommus",
-				       "#iommu-cells") < 0)
+	/* The device needs to be verified in xlate() */
+	if (!archdata)
 		return -ENODEV;
 
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
-	archdata = dev->archdata.iommu;
 	spin_lock(&ipmmu_slave_devices_lock);
 	list_add(&archdata->list, &ipmmu_slave_devices);
 	spin_unlock(&ipmmu_slave_devices_lock);

@@ -1034,24 +1032,13 @@ static void ipmmu_remove_device_dma(stru
 	iommu_group_remove_device(dev);
 }
 
-static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
-{
-	struct iommu_group *group;
-	int ret;
-
-	ret = ipmmu_init_platform_device(dev);
-	if (!ret)
-		group = ipmmu_find_group(dev);
-	else
-		group = ERR_PTR(ret);
-
-	return group;
-}
-
 static int ipmmu_of_xlate_dma(struct device *dev,
 			      struct of_phandle_args *spec)
 {
-	return 0;
+	/* For now only tested on R-Car Gen3 with ARM64 arch init order
+	 * TODO: Test R-Car Gen2 with ARM32 arch init order
+	 */
+	return ipmmu_init_platform_device(dev);
 }
 
 static const struct iommu_ops ipmmu_ops = {

@@ -1065,7 +1052,7 @@ static const struct iommu_ops ipmmu_ops
 	.iova_to_phys = ipmmu_iova_to_phys,
 	.add_device = ipmmu_add_device_dma,
 	.remove_device = ipmmu_remove_device_dma,
-	.device_group = ipmmu_device_group_dma,
+	.device_group = ipmmu_find_group,
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 	.of_xlate = ipmmu_of_xlate_dma,
 };

From: Magnus Damm <[email protected]>

Match on r8a7795 ES2 and enable a certain DMA controller. In other cases the IPMMU driver remains disabled.

---

Changes since V1:
- Perform white list check in ->xlate() instead of ->add_device()

 drivers/iommu/ipmmu-vmsa.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/sys_soc.h>
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include <asm/dma-iommu.h>

@@ -1032,9 +1033,33 @@ static void ipmmu_remove_device_dma(stru
 	iommu_group_remove_device(dev);
 }
 
+static const struct soc_device_attribute r8a7795es2[] = {
+	{ .soc_id = "r8a7795", .revision = "ES2.*" },
+	{ /* sentinel */ }
+};
+
+static int ipmmu_slave_whitelist(struct device *dev)
+{
+	/* Opt-in slave devices based on SoC and ES version */
+	if (soc_device_match(r8a7795es2)) {
+		if (!strcmp(dev_name(dev), "e7310000.dma-controller"))
+			return 0;
+	}
+
+	/* By default, do not allow use of IPMMU */
+	return -ENODEV;
+}
+
 static int ipmmu_of_xlate_dma(struct device *dev,
 			      struct of_phandle_args *spec)
 {
+	int ret;
+
+	/* Opt-in devices based on SoC and ES version */
+	ret = ipmmu_slave_whitelist(dev);
+	if (ret)
+		return ret;
+
 	/* For now only tested on R-Car Gen3 with ARM64 arch init order
 	 * TODO: Test R-Car Gen2 with ARM32 arch init order
 	 */

Hi Geert, everyone,

Do you have any opinion about the code in this version of the series?

I recall that you agreed with the approach in "[PATCH/RFC 2/2] iommu/ipmmu-vmsa: Opt-in slave devices based on ES version", however it was suggested to me by Robin that my code in "[PATCH/RFC 1/2]
arm64: mm: Silently allow devices lacking IOMMU group" should be reworked to use ->xlate().

Now this series makes use of ->xlate() to implement the white list, so I hope that makes everyone happy.

Also, based on your suggestion I finally managed to break out the code that skips over disabled devices in "[PATCH/RFC v2 1/4] iommu/of: Skip IOMMU devices disabled in DT" but I'm not sure if this will cause problem for other platforms.

Anyway, my current plan is to wait for feedback for "[PATCH/RFC v2 1/4] iommu/of: Skip IOMMU devices disabled in DT" and handle that independently, and also roll in the other changes in this series into my other IPMMU code.

Thanks,

/ magnus

For the series: