Generic DT bindings for PCI IOMMUs and ARM SMMUv3

So far, all the users of the generic of_xlate configuration mechanism are resorting to explicit platform device creation to ensure the IOMMU device is ready before anything tries to refer to it. As I'm about to convert two more drivers that will need exactly the same thing, let's nip that proliferation in the bud and move it to a single place.

A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is instantiated before giving out its associated ops, since it's a fairly safe assumption that the ops aren't going to be much use if the IOMMU can't or won't exist to back them up.

CC Marek Szyprowski <[email protected]>

CC Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

 drivers/iommu/exynos-iommu.c | 20 +++++++-------------
 drivers/iommu/mtk_iommu.c    |  8 +-------
 drivers/iommu/of_iommu.c     |  6 +++++-
 3 files changed, 13 insertions(+), 21 deletions(-)

drivers/iommu/exynos-iommu.c +7 -13

index 5ecc86cb74c8..97380ee56d71 100644
@@ -665,6 +665,13 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	/*
+	 * use the first registered sysmmu device for performing
+	 * dma mapping operations on iommu page tables (cpu cache flush)
+	 */
+	if (!dma_dev)
+		dma_dev = dev;
+
 	return 0;
 }
 

@@ -1341,22 +1348,9 @@ err_reg_driver:
 
 static int __init exynos_iommu_of_setup(struct device_node *np)
 {
-	struct platform_device *pdev;
-
 	if (!init_done)
 		exynos_iommu_init();
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
-
-	/*
-	 * use the first registered sysmmu device for performing
-	 * dma mapping operations on iommu page tables (cpu cache flush)
-	 */
-	if (!dma_dev)
-		dma_dev = &pdev->dev;
-
 	of_iommu_set_ops(np, &exynos_iommu_ops);
 	return 0;
 }

drivers/iommu/mtk_iommu.c +1 -7

index c3043d8754e3..f7ae87abea99 100644
@@ -724,14 +724,8 @@ static struct platform_driver mtk_iommu_driver = {
 
 static int mtk_iommu_init_fn(struct device_node *np)
 {
-	int ret;
-	struct platform_device *pdev;
+	int ret = platform_driver_register(&mtk_iommu_driver);
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (!pdev)
-		return -ENOMEM;
-
-	ret = platform_driver_register(&mtk_iommu_driver);
 	if (ret) {
 		pr_err("%s: Failed to register driver\n", __func__);
 		return ret;

drivers/iommu/of_iommu.c +5 -1

index af499aea0a1a..7e6369cffc95 100644
@@ -22,6 +22,7 @@
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/of_platform.h>
 #include <linux/slab.h>
 
 static const struct of_device_id __iommu_of_table_sentinel

@@ -127,7 +128,10 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 	spin_lock(&of_iommu_lock);
 	list_for_each_entry(node, &of_iommu_list, list)
 		if (node->np == np) {
-			ops = node->ops;
+			if (of_node_check_flag(np, OF_POPULATED) ||
+			    of_platform_device_create(np, NULL,
+					    platform_bus_type.dev_root))
+				ops = node->ops;
 			break;
 		}
 	spin_unlock(&of_iommu_lock);

Signature · 1 line

2.8.1.dirty

So far, all the users of the generic of_xlate configuration mechanism are resorting to explicit platform device creation to ensure the IOMMU device is ready before anything tries to refer to it. As I'm about to convert two more drivers that will need exactly the same thing, let's nip that proliferation in the bud and move it to a single place.

A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is instantiated before giving out its associated ops, since it's a fairly safe assumption that the ops aren't going to be much use if the IOMMU can't or won't exist to back them up.

CC Marek Szyprowski <[email protected]>

CC Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: New.

  drivers/iommu/exynos-iommu.c | 20 +++++++-------------
  drivers/iommu/mtk_iommu.c    |  8 +-------
  drivers/iommu/of_iommu.c     |  6 +++++-
  3 files changed, 13 insertions(+), 21 deletions(-)

drivers/iommu/exynos-iommu.c +7 -13

index 5ecc86cb74c8..97380ee56d71 100644
@@ -665,6 +665,13 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 
  	pm_runtime_enable(dev);
 
+	/*
+	 * use the first registered sysmmu device for performing
+	 * dma mapping operations on iommu page tables (cpu cache flush)
+	 */
+	if (!dma_dev)
+		dma_dev = dev;
+
  	return 0;
  }
 

@@ -1341,22 +1348,9 @@ err_reg_driver:
 
  static int __init exynos_iommu_of_setup(struct device_node *np)
  {
-	struct platform_device *pdev;
-
  	if (!init_done)
  		exynos_iommu_init();
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
-
-	/*
-	 * use the first registered sysmmu device for performing
-	 * dma mapping operations on iommu page tables (cpu cache flush)
-	 */
-	if (!dma_dev)
-		dma_dev = &pdev->dev;
-
  	of_iommu_set_ops(np, &exynos_iommu_ops);
  	return 0;
  }

drivers/iommu/mtk_iommu.c +1 -7

index c3043d8754e3..f7ae87abea99 100644
@@ -724,14 +724,8 @@ static struct platform_driver mtk_iommu_driver = {
 
  static int mtk_iommu_init_fn(struct device_node *np)
  {
-	int ret;
-	struct platform_device *pdev;
+	int ret = platform_driver_register(&mtk_iommu_driver);
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (!pdev)
-		return -ENOMEM;
-
-	ret = platform_driver_register(&mtk_iommu_driver);
  	if (ret) {
  		pr_err("%s: Failed to register driver\n", __func__);
  		return ret;

drivers/iommu/of_iommu.c +1 -0

index af499aea0a1a..7e6369cffc95 100644
@@ -22,6 +22,7 @@
  #include <linux/limits.h>
  #include <linux/of.h>
  #include <linux/of_iommu.h>
+#include <linux/of_platform.h>
  #include <linux/slab.h>
 
  static const struct of_device_id __iommu_of_table_sentinel

@@ -127,7 +128,10 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
  	spin_lock(&of_iommu_lock);

Hi Robin,

Quoted 6 lines · Robin Murphy

So far, all the users of the generic of_xlate configuration mechanism are resorting to explicit platform device creation to ensure the IOMMU device is ready before anything tries to refer to it. As I'm about to convert two more drivers that will need exactly the same thing, let's nip that proliferation in the bud and move it to a single place.

Quoted 8 lines

A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is instantiated before giving out its associated ops, since it's a fairly safe assumption that the ops aren't going to be much use if the IOMMU can't or won't exist to back them up.

CC Marek Szyprowski <[email protected]>

CC Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

Frankly, I would avoid moving this workaround to the iommu core. IMHO the best solution would be to let IOMMU controllers to be instantiated as normal
devices and implement proper support in the device core for waiting for the iommu controller. Then the workaround can be removed from exynos and mtk iommu drivers. What's the status of IOMMU deferred probe patches?

I've encountered a serious problems with current code (the one which instantiates iommu controller devices from iommu driver) and its integration
with power domains, clocks and runtime pm, which were not possible to resolve
without iommu deferred probe.

Hi Marek,

Quoted 18 lines · Marek Szyprowski

Hi Robin,

Quoted 6 lines · Robin Murphy

So far, all the users of the generic of_xlate configuration mechanism are resorting to explicit platform device creation to ensure the IOMMU device is ready before anything tries to refer to it. As I'm about to convert two more drivers that will need exactly the same thing, let's nip that proliferation in the bud and move it to a single place.

Quoted 8 lines

A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is instantiated before giving out its associated ops, since it's a fairly safe assumption that the ops aren't going to be much use if the IOMMU can't or won't exist to back them up.

CC Marek Szyprowski <[email protected]>

CC Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

Quoted 6 lines

Frankly, I would avoid moving this workaround to the iommu core. IMHO the best solution would be to let IOMMU controllers to be instantiated as normal
devices and implement proper support in the device core for waiting for the iommu controller. Then the workaround can be removed from exynos and mtk iommu drivers. What's the status of IOMMU deferred probe patches?

I think revisiting probe ordering is now second-from-top on my to-do list after this lot. This patch was kind of thinking ahead to get the "touch all the drivers" aspect out of the way before it grows any bigger, and all the development can then happen in the core code alone, but I admit it's not a particularly strong argument.

Quoted 6 lines

I've encountered a serious problems with current code (the one which instantiates iommu controller devices from iommu driver) and its integration
with power domains, clocks and runtime pm, which were not possible to resolve
without iommu deferred probe.

OK. Do you have any plans to try tweaking the current workaround, or is it really not worth it?

FWIW I do have an Exynos 5410 (Odroid-XU) on my
desk which I could theoretically test things on, but I suspect it would take a fair amount of work to get the SYSMMUs and relevant media bits up and running on top of Krzysztof's basic support.

Will: for the time being, the alternative to this patch would be to squash the following change into patch 7/9 (without either, patch 8/9 doesn't really work).

Robin.

-----8<-----

drivers/iommu/arm-smmu-v3.c +1 -0

index e4c3916efba9..c002ff06b625 100644
@@ -2678,6 +2678,14 @@ module_exit(arm_smmu_exit);
 
  static int __init arm_smmu_of_init(struct device_node *np)
  {
+	static bool registered;

Quoted 6 lines · Marek Szyprowski

Frankly, I would avoid moving this workaround to the iommu core. IMHO the best solution would be to let IOMMU controllers to be instantiated as normal
devices and implement proper support in the device core for waiting for the iommu controller. Then the workaround can be removed from exynos and mtk iommu drivers. What's the status of IOMMU deferred probe patches?

I think revisiting probe ordering is now second-from-top on my to-do list after this lot. This patch was kind of thinking ahead to get the "touch all the drivers" aspect out of the way before it grows any bigger, and all the development can then happen in the core code alone, but I admit it's not a particularly strong argument.

Quoted 6 lines

I've encountered a serious problems with current code (the one which instantiates iommu controller devices from iommu driver) and its integration
with power domains, clocks and runtime pm, which were not possible to resolve
without iommu deferred probe.

OK. Do you have any plans to try tweaking the current workaround, or is it really not worth it? FWIW I do have an Exynos 5410 (Odroid-XU) on my desk which I could theoretically test things on, but I suspect it would take a fair amount of work to get the SYSMMUs and relevant media bits up and running on top of Krzysztof's basic support.

Will: for the time being, the alternative to this patch would be to squash the following change into patch 7/9 (without either, patch 8/9 doesn't really work).

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

---

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(-)

drivers/iommu/of_iommu.c +5 -0

index 7e6369cffc95..25406a8b9d4e 100644
@@ -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;
+

drivers/iommu/of_iommu.c +5 -6

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

Quoted 33 lines · Robin Murphy

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

---

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(-)

drivers/iommu/of_iommu.c +5 -0

index 7e6369cffc95..25406a8b9d4e 100644
@@ -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;
+

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_get_ops(struct device_node *np) · line 154

Quoted 10 lines

drivers/iommu/of_iommu.c +5 -1

+	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 19 lines

drivers/iommu/of_iommu.c +10 -6

 	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

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_get_ops(struct device_node *np) · line 172

Quoted 5 lines

drivers/iommu/of_iommu.c +5 -0

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

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_get_ops(struct device_node *np) · line 177

Quoted 5 lines

drivers/iommu/of_iommu.c +5 -0

+
+		/* 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?

Will

Introduce a common structure to hold the per-device firmware data that non-architectural IOMMU drivers generally need to keep track of. Initially this is DT-specific to complement the existing of_iommu support code, but will generalise further once other firmware methods (e.g. ACPI IORT) come along.

Ultimately the aim is to promote the fwspec to a first-class member of struct device, and handle the init/free automatically in the firmware code. That way we can have API calls look for dev->fwspec->iommu_ops before falling back to dev->bus->iommu_ops, and thus gracefully handle those troublesome multi-IOMMU systems which we currently cannot. To start with, though, make use of the existing archdata field and delegate the init/free to drivers to allow an incremental conversion rather than the impractical pain of trying to attempt everything in one go.

Suggested-by Will Deacon <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: New.

 drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 18 ++++++++++++++++++
 2 files changed, 65 insertions(+)

drivers/iommu/of_iommu.c +0 -0

index 25406a8b9d4e..2b90c1f56ff2 100644

include/linux/of_iommu.h +9 -0

index bd02b44902d0..5a4f516cfcfe 100644
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;

Quoted 25 lines · Robin Murphy

Introduce a common structure to hold the per-device firmware data that non-architectural IOMMU drivers generally need to keep track of. Initially this is DT-specific to complement the existing of_iommu support code, but will generalise further once other firmware methods (e.g. ACPI IORT) come along.

Ultimately the aim is to promote the fwspec to a first-class member of struct device, and handle the init/free automatically in the firmware code. That way we can have API calls look for dev->fwspec->iommu_ops before falling back to dev->bus->iommu_ops, and thus gracefully handle those troublesome multi-IOMMU systems which we currently cannot. To start with, though, make use of the existing archdata field and delegate the init/free to drivers to allow an incremental conversion rather than the impractical pain of trying to attempt everything in one go.

Suggested-by Will Deacon <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: New.

 drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 18 ++++++++++++++++++
 2 files changed, 65 insertions(+)

drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 224

Quoted 10 lines

drivers/iommu/of_iommu.c +2 -0

index 25406a8b9d4e..2b90c1f56ff2 100644
@@ -220,3 +220,50 @@ void __init of_iommu_init(void)
 				of_node_full_name(np));
 	}
 }
+
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)

These functions might want an of_* prefix to match the rest of the file, or do you plan to re-use these later for ACPI?

Quoted 27 lines

include/linux/of_iommu.h +10 -0

index bd02b44902d0..5a4f516cfcfe 100644
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;
+
 #endif	/* CONFIG_OF_IOMMU */
 

include/linux/of_iommu.h static inline const struct iommu_ops *of_iommu_configure(struct device *dev, · line 52

Quoted 7 lines

include/linux/of_iommu.h +7 -0

+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}

I'm a bit nervous about putting this inline in a header file, since not all architectures unconditionally provide the iommu field in dev_archdata.

Will

Quoted 26 lines · Will Deacon

Quoted 25 lines · Robin Murphy

Introduce a common structure to hold the per-device firmware data that non-architectural IOMMU drivers generally need to keep track of. Initially this is DT-specific to complement the existing of_iommu support code, but will generalise further once other firmware methods (e.g. ACPI IORT) come along.

Ultimately the aim is to promote the fwspec to a first-class member of struct device, and handle the init/free automatically in the firmware code. That way we can have API calls look for dev->fwspec->iommu_ops before falling back to dev->bus->iommu_ops, and thus gracefully handle those troublesome multi-IOMMU systems which we currently cannot. To start with, though, make use of the existing archdata field and delegate the init/free to drivers to allow an incremental conversion rather than the impractical pain of trying to attempt everything in one go.

Suggested-by Will Deacon <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: New.

 drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 18 ++++++++++++++++++
 2 files changed, 65 insertions(+)

Quoted 13 lines

drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 224

Quoted 10 lines

drivers/iommu/of_iommu.c +2 -0

index 25406a8b9d4e..2b90c1f56ff2 100644
@@ -220,3 +220,50 @@ void __init of_iommu_init(void)
 				of_node_full_name(np));
 	}
 }
+
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)

These functions might want an of_* prefix to match the rest of the file, or do you plan to re-use these later for ACPI?

Indeed, the naming is a deliberate concession to the future - I was trying to pre-empt the idea that the interface might be intended to grow into separate of_* and iort_* versions. I'm just using raw device_nodes for the moment as I don't think we'd reached a consensus on whether fwnode_handle or some other abstraction was the appropriate way to go.

Quoted 27 lines

Quoted 27 lines

include/linux/of_iommu.h +10 -0

index bd02b44902d0..5a4f516cfcfe 100644
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;
+
 #endif	/* CONFIG_OF_IOMMU */
 

Quoted 10 lines

include/linux/of_iommu.h static inline const struct iommu_ops *of_iommu_configure(struct device *dev, · line 52

Quoted 7 lines

include/linux/of_iommu.h +7 -0

+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}

I'm a bit nervous about putting this inline in a header file, since not all architectures unconditionally provide the iommu field in dev_archdata.

Ah yes, you're right, this shouldn't be defined without OF_IOMMU (or the IORT equivalent) anyway. Clearly the premature optimisation demons had begun whispering "A measly one-line accessor in a separate compilation unit? Won't somebody think of the chil^Wbranch predictors!?"

Now that we can properly describe the mapping between PCI RIDs and stream IDs via "iommu-map", and have it fed it to the driver automatically via of_xlate(), rework the SMMUv3 driver to benefit from that, and get rid of the current misuse of the "iommus" binding.

Since having of_xlate wired up means that masters will now be given the appropriate DMA ops, we also need to make sure that default domains work properly. This necessitates dispensing with the "whole group at a time" notion for attaching to a domain, as devices which share a group get attached to the group's default domain one by one as they are initially probed.

Signed-off-by Robin Murphy <[email protected]>

---

static int arm_smmu_add_device(struct device *dev) {

drivers/iommu/arm-smmu-v3.c +25 -37

 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;
+	struct arm_smmu_master_data *master;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct iommu_group *group;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
 		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
-	smmu_group = iommu_group_get_iommudata(group);
-	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
-		if (!smmu) {
-			ret = -ENOENT;
-			goto out_remove_dev;
-		}
-
-		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
-		if (!smmu_group) {
-			ret = -ENOMEM;
-			goto out_remove_dev;
-		}
-
-		smmu_group->ste.valid	= true;
-		smmu_group->smmu	= smmu;
-		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+	/*
+	 * We _can_ actually withstand dodgy bus code re-calling add_device()
+	 * without an intervening remove_device()/of_xlate() sequence, but
+	 * we're not going to do so quietly...
+	 */
+	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
+		master = fwspec->iommu_priv;
+		smmu = master->smmu;
 	} else {
-		smmu = smmu_group->smmu;
+		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+		if (!smmu)
+			return -ENODEV;
+		master = kzalloc(sizeof(*master), GFP_KERNEL);
+		if (!master)
+			return -ENOMEM;
+
+		master->smmu = smmu;
+		fwspec->iommu_priv = master;
 	}
 
-	/* Assume SID == RID until firmware tells us otherwise */
-	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
-	for (i = 0; i < smmu_group->num_sids; ++i) {
-		/* If we already know about this SID, then we're done */
-		if (smmu_group->sids[i] == sid)
-			goto out_put_group;
+	/* Check the SIDs are in range of the SMMU and our stream table */
+	for (i = 0; i < fwspec->num_ids; i++) {
+		u32 sid = fwspec->ids[i];
+

Quoted 14 lines · Robin Murphy

Now that we can properly describe the mapping between PCI RIDs and stream IDs via "iommu-map", and have it fed it to the driver automatically via of_xlate(), rework the SMMUv3 driver to benefit from that, and get rid of the current misuse of the "iommus" binding.

Since having of_xlate wired up means that masters will now be given the appropriate DMA ops, we also need to make sure that default domains work properly. This necessitates dispensing with the "whole group at a time" notion for attaching to a domain, as devices which share a group get attached to the group's default domain one by one as they are initially probed.

Signed-off-by Robin Murphy <[email protected]>

---

[...]

Quoted 76 lines

static int arm_smmu_add_device(struct device *dev) {

drivers/iommu/arm-smmu-v3.c +30 -37

 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;
+	struct arm_smmu_master_data *master;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct iommu_group *group;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
 		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
-	smmu_group = iommu_group_get_iommudata(group);
-	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
-		if (!smmu) {
-			ret = -ENOENT;
-			goto out_remove_dev;
-		}
-
-		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
-		if (!smmu_group) {
-			ret = -ENOMEM;
-			goto out_remove_dev;
-		}
-
-		smmu_group->ste.valid	= true;
-		smmu_group->smmu	= smmu;
-		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+	/*
+	 * We _can_ actually withstand dodgy bus code re-calling add_device()
+	 * without an intervening remove_device()/of_xlate() sequence, but
+	 * we're not going to do so quietly...
+	 */
+	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
+		master = fwspec->iommu_priv;
+		smmu = master->smmu;
 	} else {
-		smmu = smmu_group->smmu;
+		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+		if (!smmu)
+			return -ENODEV;
+		master = kzalloc(sizeof(*master), GFP_KERNEL);
+		if (!master)
+			return -ENOMEM;
+
+		master->smmu = smmu;
+		fwspec->iommu_priv = master;
 	}
 
-	/* Assume SID == RID until firmware tells us otherwise */
-	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
-	for (i = 0; i < smmu_group->num_sids; ++i) {
-		/* If we already know about this SID, then we're done */
-		if (smmu_group->sids[i] == sid)
-			goto out_put_group;
+	/* Check the SIDs are in range of the SMMU and our stream table */
+	for (i = 0; i < fwspec->num_ids; i++) {
+		u32 sid = fwspec->ids[i];
+
+		if (!arm_smmu_sid_in_range(smmu, sid))
+			return -ERANGE;
+
+		/* Ensure l2 strtab is initialised */
+		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {

drivers/iommu/arm-smmu-v3.c static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid) · line 1800

Quoted 5 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+			ret = arm_smmu_init_l2_strtab(smmu, sid);
+			if (ret)
+				return ret;
+		}
 	}

Do you not need to do some cleanup here, rather than just returning (same for the -ERANGE above)? For example, freeing the master?

Will

With the device <-> stream ID relationship suitably abstracted and of_xlate() hooked up, the PCI dependency now looks, and is, entirely arbitrary. Any bus using the of_dma_configure() mechanism will work, so extend support to the platform and AMBA buses which do just that.

Signed-off-by Robin Murphy <[email protected]>

---

v3: Now actually tested; improve comment about duplicate IDs.

 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/arm-smmu-v3.c | 42 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 9 deletions(-)

drivers/iommu/Kconfig +1 -1

index ad0860383cb3..d1c66afefeed 100644
@@ -308,7 +308,7 @@ config ARM_SMMU
 
 config ARM_SMMU_V3
 	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
-	depends on ARM64 && PCI
+	depends on ARM64
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select GENERIC_MSI_IRQ_DOMAIN

drivers/iommu/arm-smmu-v3.c +2 -0

index 8ce2a4f9342b..735690e03818 100644
@@ -35,6 +35,8 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 
+#include <linux/amba/bus.h>
+
 #include "io-pgtable.h"
 
 /* MMIO registers */

@@ -1822,6 +1824,25 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_fwspec_free(dev);

Quoted 44 lines · Robin Murphy

With the device <-> stream ID relationship suitably abstracted and of_xlate() hooked up, the PCI dependency now looks, and is, entirely arbitrary. Any bus using the of_dma_configure() mechanism will work, so extend support to the platform and AMBA buses which do just that.

Signed-off-by Robin Murphy <[email protected]>

---

v3: Now actually tested; improve comment about duplicate IDs.

 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/arm-smmu-v3.c | 42 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 9 deletions(-)

drivers/iommu/Kconfig +1 -1

index ad0860383cb3..d1c66afefeed 100644
@@ -308,7 +308,7 @@ config ARM_SMMU
 
 config ARM_SMMU_V3
 	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
-	depends on ARM64 && PCI
+	depends on ARM64
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select GENERIC_MSI_IRQ_DOMAIN

drivers/iommu/arm-smmu-v3.c +2 -0

index 8ce2a4f9342b..735690e03818 100644
@@ -35,6 +35,8 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 
+#include <linux/amba/bus.h>
+
 #include "io-pgtable.h"
 
 /* MMIO registers */

@@ -1822,6 +1824,25 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_fwspec_free(dev);
 }
 

drivers/iommu/arm-smmu-v3.c static void arm_smmu_remove_device(struct device *dev) · line 1834

Quoted 8 lines

drivers/iommu/arm-smmu-v3.c +8 -0

+static struct iommu_group *arm_smmu_device_group(struct device *dev)
+{
+	struct iommu_group *group;
+
+	/*
+	 * The difficulty of efficient stream-ID-to-device lookup prevents us
+	 * from reasonably detecting aliasing outside of PCI buses, but for the
+	 * the same underlying reason (a sparse 32-bit ID space) there's also

extra 'the'

drivers/iommu/arm-smmu-v3.c static void arm_smmu_remove_device(struct device *dev) · line 1837

Quoted 3 lines

drivers/iommu/arm-smmu-v3.c +3 -0

+	 * little excuse for systems to be wired up with non-unique IDs in the
+	 * first place; consider them unsupported.
+	 */

This is a long-winded way of saying "we don't support aliasing SIDs outside of PCI".

Will

-----Original Message-----
From: Robin Murphy <[email protected]>
Date: Tue, Jun 28, 2016 at 11:18 AM
Subject: [RFC 1/2] iommu/dma: Restrict IOVAs to physical memory layout
To: iommu at lists.linux-foundation.org, linux-arm-kernel at lists.infradead.org

Certain peripherals may be bestowed with knowledge of the physical memory map of the system in which they live, and refuse to handle addresses that they do not think are memory, which causes issues when remapping to arbitrary IOVAs. Sidestep the issue by restricting IOVA domains to only allocate addresses within ranges which match the physical memory layout.

Signed-off-by Robin Murphy <[email protected]>

---

Posting this as an RFC because it's something I've been having to use on Juno for all the PCI IOMMU development - it's pretty horrible, but I can't easily think of a nicer solution...

Quoted 16 lines

-----Original Message-----
From: Robin Murphy <[email protected]>
Date: Tue, Jun 28, 2016 at 11:18 AM
Subject: [RFC 1/2] iommu/dma: Restrict IOVAs to physical memory layout
To: iommu at lists.linux-foundation.org, linux-arm-kernel at lists.infradead.org

Certain peripherals may be bestowed with knowledge of the physical memory map of the system in which they live, and refuse to handle addresses that they do not think are memory, which causes issues when remapping to arbitrary IOVAs. Sidestep the issue by restricting IOVA domains to only allocate addresses within ranges which match the physical memory layout.

Quoted 6 lines

Signed-off-by Robin Murphy <[email protected]>

---

Posting this as an RFC because it's something I've been having to use on Juno for all the PCI IOMMU development - it's pretty horrible, but I can't easily think of a nicer solution...

Maybe I'm not getting the implications of this looking at the patch in isolation, but how will this impact systems that have devices limited to 32-bit addressing?

In our memory map we have physical memory regions at:
0x00_8000_0000
0x80_8000_0000

Will devices with a 32-bit DMA mask still get 32-bit IOVAs?

Thanks,
Stuart

Quoted 27 lines · Stuart Yoder

Quoted 16 lines

-----Original Message-----
From: Robin Murphy <[email protected]>
Date: Tue, Jun 28, 2016 at 11:18 AM
Subject: [RFC 1/2] iommu/dma: Restrict IOVAs to physical memory layout
To: iommu at lists.linux-foundation.org, linux-arm-kernel at lists.infradead.org

Certain peripherals may be bestowed with knowledge of the physical memory map of the system in which they live, and refuse to handle addresses that they do not think are memory, which causes issues when remapping to arbitrary IOVAs. Sidestep the issue by restricting IOVA domains to only allocate addresses within ranges which match the physical memory layout.

Quoted 6 lines

Signed-off-by Robin Murphy <[email protected]>

---

Posting this as an RFC because it's something I've been having to use on Juno for all the PCI IOMMU development - it's pretty horrible, but I can't easily think of a nicer solution...

Maybe I'm not getting the implications of this looking at the patch in isolation, but how will this impact systems that have devices limited to 32-bit addressing?

Quoted 5 lines

In our memory map we have physical memory regions at:
0x00_8000_0000
0x80_8000_0000

Will devices with a 32-bit DMA mask still get 32-bit IOVAs?

Assuming there's some free IOVA space between 0x80000000 and 0xffffffff, yes, otherwise it gets nothing ;) This has no effect on the allocation behaviour in general, it just makes sure that within that behaviour, we avoid allocating any address that doesn't look "real". The primary issue is with 64-bit DMA masks - since it's a top-down allocator, you typically end up with the poor device issuing its first DMA transaction to 0xfffffffffffff000 which on Juno a) gets silently eaten by the root complex because it doesn't match any window in the PCI-AXI translation table, or b) goes wrong anyway because it's beyond the input address range of the SMMU (and there's something not quite right WRT truncation/sign-extension which I've not looked into closely and am semi-deliberately also sweeping under the rug thanks to the simpler hardware issue...)

As I say, it's hideous, but I can't see what else to do.

Quoted 4 lines

Thanks,
Stuart

_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Friday, July 01, 2016 12:40 PM
To: Stuart Yoder <[email protected]>
Cc: iommu at lists.linux-foundation.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC 1/2] iommu/dma: Restrict IOVAs to physical memory layout

Quoted 48 lines · Robin Murphy

Quoted 27 lines · Stuart Yoder

Quoted 16 lines

-----Original Message-----
From: Robin Murphy <[email protected]>
Date: Tue, Jun 28, 2016 at 11:18 AM
Subject: [RFC 1/2] iommu/dma: Restrict IOVAs to physical memory layout
To: iommu at lists.linux-foundation.org, linux-arm-kernel at lists.infradead.org

Certain peripherals may be bestowed with knowledge of the physical memory map of the system in which they live, and refuse to handle addresses that they do not think are memory, which causes issues when remapping to arbitrary IOVAs. Sidestep the issue by restricting IOVA domains to only allocate addresses within ranges which match the physical memory layout.

Quoted 6 lines

Signed-off-by Robin Murphy <[email protected]>

---

Posting this as an RFC because it's something I've been having to use on Juno for all the PCI IOMMU development - it's pretty horrible, but I can't easily think of a nicer solution...

Maybe I'm not getting the implications of this looking at the patch in isolation, but how will this impact systems that have devices limited to 32-bit addressing?

Quoted 5 lines

In our memory map we have physical memory regions at:
0x00_8000_0000
0x80_8000_0000

Will devices with a 32-bit DMA mask still get 32-bit IOVAs?

Assuming there's some free IOVA space between 0x80000000 and 0xffffffff, yes, otherwise it gets nothing ;) This has no effect on the allocation behaviour in general, it just makes sure that within that behaviour, we avoid allocating any address that doesn't look "real". The primary issue is with 64-bit DMA masks - since it's a top-down allocator, you typically end up with the poor device issuing its first DMA transaction to 0xfffffffffffff000 which on Juno a) gets silently eaten by the root complex because it doesn't match any window in the PCI-AXI translation table, or b) goes wrong anyway because it's beyond the input address range of the SMMU (and there's something not quite right WRT truncation/sign-extension which I've not looked into closely and am semi-deliberately also sweeping under the rug thanks to the simpler hardware issue...)

Quoted 1 line

As I say, it's hideous, but I can't see what else to do.

Urgh, thinking some more, this is OK on Juno and LS2085 only because there *is* some RAM below 4GB to begin with. On something like Seattle where it's all high up, 32-bit peripherals will be as screwed as if the IOMMU wasn't there :(