Introduce automatic DMA configuration for IOMMU masters

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to set some per-instance data on their of_node in the form of a new iommu_data structure. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Signed-off-by Will Deacon <[email protected]>

---
  drivers/iommu/of_iommu.c          | 14 ++++++++++++++
  include/asm-generic/vmlinux.lds.h |  2 ++
  include/linux/iommu.h             |  6 ++++++
  include/linux/of_iommu.h          | 23 +++++++++++++++++++++++
  4 files changed, 45 insertions(+)

drivers/iommu/of_iommu.c +14 -0

index e550ccb7634e..f9209666157c 100644
@@ -89,3 +89,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
  	return 0;
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match, *matches = &__iommu_of_table;
+
+	for_each_matching_node_and_match(np, matches, &match) {
+		const int (*init_fn)(struct device_node *) = match->data;
+
+		if (init_fn(np))
+			pr_err("Failed to initialise IOMMU %s\n",
+				of_node_full_name(np));
+	}
+}

include/asm-generic/vmlinux.lds.h +2 -0

index 5ba0360663a7..b75ede8f99ae 100644
@@ -162,6 +162,7 @@
  #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
  #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
  #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
  #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
  #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
  #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -495,6 +496,7 @@
  	CLK_OF_TABLES()							\
  	RESERVEDMEM_OF_TABLES()						\
  	CLKSRC_OF_TABLES()						\
+	IOMMU_OF_TABLES()						\
  	CPU_METHOD_OF_TABLES()						\
  	KERNEL_DTB()							\
  	IRQCHIP_OF_MATCH_TABLE()					\

include/linux/iommu.h +6 -0

index 20f9a527922a..fdddb14cd8f5 100644
@@ -57,6 +57,12 @@ struct iommu_domain {
  	struct iommu_domain_geometry geometry;
  };
 
+struct iommu_data {
+	struct iommu_domain *domain;
+	struct iova_domain *iovad;
+	void *priv;
+};
+
  #define IOMMU_CAP_CACHE_COHERENCY	0x1
  #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
 

include/linux/of_iommu.h +12 -0

index 51a560f34bca..a39e2d78f735 100644
@@ -1,12 +1,17 @@
  #ifndef __OF_IOMMU_H
  #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
  #ifdef CONFIG_OF_IOMMU
 
  extern int of_get_dma_window(struct device_node *dn, const char *prefix,
  			     int index, unsigned long *busno, dma_addr_t *addr,
  			     size_t *size);
 
+extern void of_iommu_init(void);
+
  #else
 
  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -16,6 +21,24 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
  	return -EINVAL;
  }
 
+static inline void of_iommu_init(void) { }
+
  #endif	/* CONFIG_OF_IOMMU */
 
+static inline void of_iommu_set_data(struct device_node *np,
+				     struct iommu_data *data)
+{
+	np->data = data;
+}

set_arch_dma_coherent_ops is called from of_dma_configure in order to swizzle the architectural dma-mapping functions over to a cache-coherent implementation. This is currently implemented only for ARM.

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping ops too, this patch replaces the function with a broader arch_setup_dma_ops callback which is also responsible for setting the DMA mask and offset as well as selecting the correct mapping functions.

A further advantage of this split is that it nicely isolates the of-specific code from the dma-mapping code, allowing potential reuse by other buses (e.g. PCI) in the future.

Signed-off-by Will Deacon <[email protected]>

---
  arch/arm/include/asm/dma-mapping.h | 13 ++++++++----
  drivers/of/platform.c              | 42 ++++++++++----------------------------
  include/linux/dma-mapping.h        |  8 +++-----
  3 files changed, 23 insertions(+), 40 deletions(-)

drivers/of/platform.c +1 -0

+	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
  }
 
  /**

include/linux/dma-mapping.h +0 -1

index 931b70986272..0f7f7b68b0db 100644
@@ -129,11 +129,9 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 
  extern u64 dma_get_required_mask(struct device *dev);
 
-#ifndef set_arch_dma_coherent_ops

Hi Will,

Quoted 4 lines

set_arch_dma_coherent_ops is called from of_dma_configure in order to swizzle the architectural dma-mapping functions over to a cache-coherent implementation. This is currently implemented only for ARM.

Quoted 8 lines

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping ops too, this patch replaces the function with a broader arch_setup_dma_ops callback which is also responsible for setting the DMA mask and offset as well as selecting the correct mapping functions.

A further advantage of this split is that it nicely isolates the of-specific code from the dma-mapping code, allowing potential reuse by other buses (e.g. PCI) in the future.

I think this patch can introduce a regression if it will be used as is :(

When this code was initially created there ware a lot of discussion about and finally it was decided to configure all common (for all arches) DMA specific parameters for devices in common code, while strictly arch specific things using arch specific APIs/callbacks.

The following parameters are common now:
dev->coherent_dma_mask = mask;
dev->dma_mask = &dev->coherent_dma_mask;
dev->dma_pfn_offset = offset;

and they need to be set always, otherwise it will affect on other DT-based arches.

Quoted 26 lines · Grygorii Strashko

Quoted 4 lines · Will Deacon

set_arch_dma_coherent_ops is called from of_dma_configure in order to swizzle the architectural dma-mapping functions over to a cache-coherent implementation. This is currently implemented only for ARM.

Quoted 8 lines

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping ops too, this patch replaces the function with a broader arch_setup_dma_ops callback which is also responsible for setting the DMA mask and offset as well as selecting the correct mapping functions.

A further advantage of this split is that it nicely isolates the of-specific code from the dma-mapping code, allowing potential reuse by other buses (e.g. PCI) in the future.

I think this patch can introduce a regression if it will be used as is :(

When this code was initially created there ware a lot of discussion about and finally it was decided to configure all common (for all arches) DMA specific parameters for devices in common code, while strictly arch specific things using arch specific APIs/callbacks.

The following parameters are common now:
dev->coherent_dma_mask = mask;
dev->dma_mask = &dev->coherent_dma_mask;
dev->dma_pfn_offset = offset;

Quoted 2 lines

and they need to be set always, otherwise it will affect on other DT-based arches.

Ok, in which case we can either configure the struct device in of_dma_configure before calling arch_setup_dma_ops, or we can do the work in a generic version of arch_setup_dma_ops (that I haven't added yet).

This patch adds a new function to the iommu_ops structure to allow an OF device to be added to a specific IOMMU instance using the recently merged generic devicetree binding for IOMMUs. The callback (of_xlate) takes a struct device representing the master and an of_phandle_args representing the IOMMU and the correspondong IDs for the new master.

Signed-off-by Will Deacon <[email protected]>

---
  include/linux/iommu.h | 5 +++++
  1 file changed, 5 insertions(+)

include/linux/iommu.h +1 -0

index fdddb14cd8f5..3e766b85daa3 100644
@@ -21,6 +21,7 @@
 
  #include <linux/errno.h>
  #include <linux/err.h>
+#include <linux/of.h>
  #include <linux/types.h>
  #include <trace/events/iommu.h>
 

Quoted 27 lines · Will Deacon

This patch adds a new function to the iommu_ops structure to allow an OF device to be added to a specific IOMMU instance using the recently merged generic devicetree binding for IOMMUs. The callback (of_xlate) takes a struct device representing the master and an of_phandle_args representing the IOMMU and the correspondong IDs for the new master.

Signed-off-by Will Deacon <[email protected]>

---
  include/linux/iommu.h | 5 +++++
  1 file changed, 5 insertions(+)

include/linux/iommu.h +1 -0

index fdddb14cd8f5..3e766b85daa3 100644
@@ -21,6 +21,7 @@
 
  #include <linux/errno.h>
  #include <linux/err.h>
+#include <linux/of.h>
  #include <linux/types.h>
  #include <trace/events/iommu.h>
 

@@ -136,6 +137,10 @@ struct iommu_ops {
  	/* Get the numer of window per domain */
  	u32 (*domain_get_windows)(struct iommu_domain *domain);
 

include/linux/iommu.h struct iommu_ops { · line 141

Quoted 2 lines

include/linux/iommu.h +2 -0

+#ifdef CONFIG_OF_IOMMU
+	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);

If I understand correctly how it is designed to work, then it should be:

struct iommu_data *(*of_xlate)(struct device *dev, struct of_phandle_args *args);

Quoted 6 lines · Marek Szyprowski

include/linux/iommu.h struct iommu_ops { · line 141

Quoted 2 lines · Will Deacon

include/linux/iommu.h +2 -0

+#ifdef CONFIG_OF_IOMMU
+	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);

If I understand correctly how it is designed to work, then it should be:

Quoted 2 lines

struct iommu_data *(*of_xlate)(struct device *dev, struct of_phandle_args *args);

Yeah, that might be cleaner than the of_iommu_get_data call in of_dma_configure. I'm currently cooking a v3, so I'll include this change.

drivers/iommu/of_iommu.c +11 -0

+       struct iommu_data *iommu = NULL;
+       int idx = 0;
+
+       if (!iommu_present(bus) || !ops->of_xlate)
+               return NULL;
+
+       /*
+        * We don't currently walk up the tree looking for a parent IOMMU.
+        * See the `Notes:' section of
+        * Documentation/devicetree/bindings/iommu/iommu.txt
+        */

drivers/iommu/of_iommu.c +10 -0

+               } else if (iommu != data) {
+                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+                               dev_name(dev));
+                       iommu = NULL;
+               }
+
+               of_node_put(np);
+
+               if (!iommu)
+                       break;

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 99

Quoted 6 lines · On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:

drivers/iommu/of_iommu.c +6 -0

+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+       struct of_phandle_args iommu_spec;
+       struct iommu_dma_mapping *mapping;
+       struct bus_type *bus = dev->bus;
+       const struct iommu_ops *ops = bus->iommu_ops;

I think it would be best to not even introduce the tight coupling between bus_type and iommu_ops here, one of the main reasons we are doing this is to break that connection.

Better put the iommu_ops into the iommu_data pointer that gets looked up per iommu device.

Quoted 16 lines

drivers/iommu/of_iommu.c +16 -0

+       struct iommu_data *iommu = NULL;
+       int idx = 0;
+
+       if (!iommu_present(bus) || !ops->of_xlate)
+               return NULL;
+
+       /*
+        * We don't currently walk up the tree looking for a parent IOMMU.
+        * See the `Notes:' section of
+        * Documentation/devicetree/bindings/iommu/iommu.txt
+        */
+        */
+       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+                                          "#iommu-cells", idx,
+                                          &iommu_spec)) {
+               struct device_node *np = iommu_spec.np;

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 119

Quoted 5 lines

drivers/iommu/of_iommu.c +5 -0

+               struct iommu_data *data = of_iommu_get_data(np);
+
+               if (!iommu) {
+                       if (!ops->of_xlate(dev, &iommu_spec))
+                               iommu = data;

I would make the first argument of the of_xlate function the iommu_data, so the code can find the right instance.

Maybe also add an extra argument at the end that can be used by the PCI code and potentially other drivers with multiple master IDs behind one "iommus" property to pass some value identifying which of the masters is meant.

The format of that is unfortunately bus specific and our platform_bus really covers a number of different hardware buses (AXI, AHB, OPB, ...) but the caller should be able to provide the data in the right form that the iommu understands. I would try using a single u64 argument as a start, hoping that this covers all the buses we need. At least it's enough for PCI (bus/device/function) and for AXI (requester-id?).

Quoted 18 lines

drivers/iommu/of_iommu.c +17 -0

+               } else if (iommu != data) {
+                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+                               dev_name(dev));
+                       iommu = NULL;
+               }
+
+               of_node_put(np);
+
+               if (!iommu)
+                       break;
+
+               idx++;
+       }
+
+       if (!iommu)
+               return NULL;
+

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 139

Quoted 4 lines

drivers/iommu/of_iommu.c +3 -0

+       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
+       if (!mapping)
+               return NULL;

I don't think it's safe to use devm_* functions here: this is during device discovery, and this data will be freed if probe() fails or the device gets removed from a driver.

Quoted 18 lines

Quoted 18 lines

drivers/iommu/of_iommu.c +17 -0

+               } else if (iommu != data) {
+                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+                               dev_name(dev));
+                       iommu = NULL;
+               }
+
+               of_node_put(np);
+
+               if (!iommu)
+                       break;
+
+               idx++;
+       }
+
+       if (!iommu)
+               return NULL;
+

Quoted 8 lines

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 139

Quoted 4 lines

drivers/iommu/of_iommu.c +3 -0

+       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
+       if (!mapping)
+               return NULL;

I don't think it's safe to use devm_* functions here: this is during device discovery, and this data will be freed if probe() fails or the device gets removed from a driver.

So I can make this a standard kzalloc, but I have no idea where the corresponding kfree should live. Is there something equivalent to of_dma_unconfigure, or is this data that is expected to persist?

Quoted 27 lines · On Thursday 04 September 2014 12:26:25 Will Deacon wrote:

Quoted 26 lines

Quoted 12 lines

drivers/iommu/of_iommu.c +10 -0

+               } else if (iommu != data) {
+                       pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+                               dev_name(dev));
+                       iommu = NULL;
+               }
+
+               of_node_put(np);
+
+               if (!iommu)
+                       break;

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 139

Quoted 10 lines

drivers/iommu/of_iommu.c +10 -0

+
+               idx++;
+       }
+
+       if (!iommu)
+               return NULL;
+
+       mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
+       if (!mapping)
+               return NULL;

I don't think it's safe to use devm_* functions here: this is during device discovery, and this data will be freed if probe() fails or the device gets removed from a driver.

Quoted 4 lines

So I can make this a standard kzalloc, but I have no idea where the corresponding kfree should live. Is there something equivalent to of_dma_unconfigure, or is this data that is expected to persist?

Can this be a simple kfree in of_platform_device_create_pdata?

The generic IOMMU device-tree bindings can be used to add arbitrary OF masters to an IOMMU with a compliant binding.

This patch introduces of_iommu_configure, which does exactly that.

Signed-off-by Will Deacon <[email protected]>

---
 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/of_iommu.c    | 52 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h |  7 ++++++
 include/linux/of_iommu.h    |  8 +++++++
 4 files changed, 68 insertions(+), 1 deletion(-)

drivers/iommu/Kconfig +1 -1

index dd5112265cc9..6d13f962f156 100644
@@ -15,7 +15,7 @@ if IOMMU_SUPPORT
 
 config OF_IOMMU
        def_bool y
-       depends on OF
+       depends on OF && IOMMU_API
 
 config FSL_PAMU
 	bool "Freescale IOMMU support"

drivers/iommu/of_iommu.c +17 -0

index f9209666157c..1188c929ffa7 100644
@@ -18,6 +18,7 @@
  */
 
 #include <linux/export.h>
+#include <linux/iommu.h>
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>

@@ -90,6 +91,57 @@ int of_get_dma_window(struct device_node *dn, const char
 *prefix, int index, }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct bus_type *bus = dev->bus;
+	const struct iommu_ops *ops = bus->iommu_ops;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	if (!iommu_present(bus) || !ops->of_xlate)
+		return NULL;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt

drivers/iommu/of_iommu.c +25 -0

+		} else if (iommu != data) {
+			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+				dev_name(dev));
+			iommu = NULL;
+		}
+
+		of_node_put(np);
+
+		if (!iommu)
+			break;
+
+		idx++;
+	}
+
+	if (!iommu)
+		return NULL;
+
+	mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)
+		return NULL;
+
+	mapping->iommu = iommu;
+	return mapping;
+}
+
 void __init of_iommu_init(void)
 {
 	struct device_node *np;

include/linux/dma-mapping.h +7 -0

index 0f7f7b68b0db..16bf92f71c3e 100644
@@ -62,6 +62,13 @@ struct dma_map_ops {
 	int is_phys;
 };
 
+struct iommu_data;
+
+struct iommu_dma_mapping {
+	struct iommu_data *iommu;
+	struct list_head node;
+};
+
 #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
 
 #define DMA_MASK_NONE	0x0ULL

include/linux/of_iommu.h +7 -0

index a39e2d78f735..f2d3b1e780d7 100644
@@ -1,9 +1,12 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/device.h>
 #include <linux/iommu.h>
 #include <linux/of.h>
 
+struct iommu_dma_mapping;
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -11,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const
 char *prefix, size_t *size);
 
 extern void of_iommu_init(void);
+extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev);
 
 #else
 

@@ -22,6 +26,10 @@ static inline int of_get_dma_window(struct device_node
 *dn, const char *prefix, }
 
 static inline void of_iommu_init(void) { }
+static inline struct iommu_dma_mapping *of_iommu_configure(struct device
 *dev) +{
+	return NULL;
+}
 
 #endif	/* CONFIG_OF_IOMMU */

Quoted 23 lines · On Tuesday 02 September 2014 18:56:24 Will Deacon wrote:

drivers/iommu/of_iommu.c +23 -0

+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct bus_type *bus = dev->bus;
+	const struct iommu_ops *ops = bus->iommu_ops;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	if (!iommu_present(bus) || !ops->of_xlate)
+		return NULL;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt
+	 */
+	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+					   "#iommu-cells", idx,
+					   &iommu_spec)) {
+		struct device_node *np = iommu_spec.np;
+		struct iommu_data *data = of_iommu_get_data(np);
+

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char *prefix, int index, · line 119

Quoted 3 lines

drivers/iommu/of_iommu.c +3 -0

+		if (!iommu) {
+			if (!ops->of_xlate(dev, &iommu_spec))
+				iommu = data;

If I understand the code correctly, this will call of_xlate for the first IOMMU reference only and silently ignore all subsequent references if they point to the same IOMMU device but with different stream/requester IDs. Is that the desired behaviour ?

This patch plumbs the existing ARM IOMMU DMA infrastructure (which isn't actually called outside of a few drivers) into arch_setup_dma_ops, so that we can use IOMMUs for DMA transfers in a more generic fashion.

Since this significantly complicates the arch_setup_dma_ops function, it is moved out of line into dma-mapping.c. If CONFIG_ARM_DMA_USE_IOMMU is not set, the iommu paramater is ignored and the normal ops are used instead.

Signed-off-by Will Deacon <[email protected]>

The main difference since v1 is that I've introduced some generic structures to represent IOMMUs and their mappings (struct iommu_data and struct iommu_dma_mapping). This allows us to propagate the per-instance data all the way down to the DMA-mapping code, which can then manage a per-instance domain. Note that the ARM dma-mapping implementation does not currently make use of this information.