Re: [PATCH v3 5/9] iommu/of: Handle iommu-map property for PCI

On 01/07/16 11:31, Will Deacon wrote:

On Tue, Jun 28, 2016 at 04:48:24PM +0100, Robin Murphy wrote:

quoted

Now that we have a way to pick up the RID translation and target IOMMU,
hook up of_iommu_configure() to bring PCI devices into the of_xlate
mechanism and allow them IOMMU-backed DMA ops without the need for
driver-specific handling.

Signed-off-by: Robin Murphy <redacted>
---

v3: Use explicit "iommu-map-mask", drop of_device_is_available() check.

 drivers/iommu/of_iommu.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 7e6369cffc95..25406a8b9d4e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c

@@ -22,6 +22,7 @@
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 

@@ -138,20 +139,48 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 	return ops;
 }
 
+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct of_phandle_args *iommu_spec = data;
+
+	iommu_spec->args[0] = alias;
+	return iommu_spec->np == pdev->bus->dev.of_node;
+}
+
 const struct iommu_ops *of_iommu_configure(struct device *dev,
 					   struct device_node *master_np)
 {
 	struct of_phandle_args iommu_spec;
-	struct device_node *np;
+	struct device_node *np = NULL;

This sets off some alarm bells...

quoted

 	const struct iommu_ops *ops = NULL;
 	int idx = 0;
 
-	/*
-	 * We can't do much for PCI devices without knowing how
-	 * device IDs are wired up from the PCI bus to the IOMMU.
-	 */
-	if (dev_is_pci(dev))
-		return NULL;
+	if (dev_is_pci(dev)) {
+		/*
+		 * Start by tracing the RID alias down the PCI topology as
+		 * far as the host bridge whose OF node we have...
+		 */
+		iommu_spec.np = master_np;
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+				       &iommu_spec);
+		/*
+		 * ...then find out what that becomes once it escapes the PCI
+		 * bus into the system beyond, and which IOMMU it ends up at.
+		 */
+		if (of_pci_map_rid(master_np, iommu_spec.args[0], "iommu-map",
+				    "iommu-map-mask", &np, iommu_spec.args))
+			return NULL;

... because you're assumedly initialising np to NULL in case of_pci_map_rid
returns 0, but doesn't set np...

Not quite; we're passing &np as the optional "target" argument to
of_pci_map_rid - since that argument is provided, it will either find a
matching translation (and fill in np in the process) or return -EFAULT.

quoted

+
+		/* We're not attempting to handle multi-alias devices yet */
+		iommu_spec.np = np;
+		iommu_spec.args_count = 1;
+		ops = of_iommu_get_ops(np);

... and then we call of_iommu_get_ops(NULL). Does that make any sense?

It would actually work out, since nobody should have registered any ops
for a NULL node, but as above won't happen in practice anyway.

Robin.

Will