Introduce automatic DMA configuration for IOMMU masters

After some fun times wondering why on Earth of_iommu_init was trying to instantiate a GIC, I think we may need one of these as part of this patch, too ;)

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 4256f3ce1673..821eb0bd9f6c 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>
 

Hello,

Quoted 27 lines

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 4256f3ce1673..821eb0bd9f6c 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>
 

@@ -140,6 +141,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 146

Quoted 3 lines

include/linux/iommu.h +3 -0

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

If I understand correctly, this callback is intended to do per-master initialization
of the iommu structures required by the given iommu driver (I stored them in dev->archdata.iommu). However I really don't get what is the meaning of the return
value. Is it a boolean value? It is used only by of_iommu_configure to check if
the parse loop should be terminated...

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. A list of iommu_dma_mapping structures are created for each device, which represent the set of IOMMU instances through which the device can master. The list is protected by a kref count and freed when no users remain. It is expected that DMA-mapping code will take a reference if it wishes to make use of the IOMMU information.

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

---
 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/of_iommu.c    | 70 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h |  8 ++++++
 include/linux/of_iommu.h    | 10 +++++++
 4 files changed, 89 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 +41 -0

index 13d800c4ce25..8656b63f27ee 100644
@@ -18,9 +18,11 @@
  */
 
 #include <linux/export.h>
+#include <linux/iommu.h>
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/slab.h>
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.

@@ -90,6 +92,74 @@ 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 device_node *np;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	/*
+	 * 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 iommu_data *data;
+
+		np = iommu_spec.np;
+		data = of_iommu_get_data(np);
+
+		if (!data || !data->ops || !data->ops->of_xlate)
+			goto err_put_node;
+
+		if (!iommu) {
+			iommu = data;
+		} else if (iommu != data) {
+			/* We don't currently support multi-IOMMU masters */
+			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+				dev_name(dev));
+			goto err_put_node;
+		}
+
+		if (!data->ops->of_xlate(dev, &iommu_spec))
+			goto err_put_node;
+
+		of_node_put(np);
+		idx++;
+	}

drivers/iommu/of_iommu.c +7 -0

+	mapping->iommu = iommu;
+	return mapping;
+
+err_put_node:
+	of_node_put(np);
+	return NULL;
+}

drivers/iommu/of_iommu.c +0 -0

 void __init of_iommu_init(void)
 {
 	struct device_node *np;

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

index 1e944e77d38d..e60e52d82db9 100644

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

+
 #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
 
 #define DMA_MASK_NONE	0x0ULL

include/linux/of_iommu.h +9 -0

index 0a685e0ab33e..af6179557005 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,8 @@ 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);
+extern void of_iommu_deconfigure(struct kref *kref);
 
 #else
 

@@ -22,6 +27,11 @@ 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;
+}
+static inline void of_iommu_deconfigure(struct kref *kref) { }
 
 #endif	/* CONFIG_OF_IOMMU */

Hi Will,

Thank you for the patch.

Quoted 9 lines

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. A list of iommu_dma_mapping structures are created for each device, which represent the set of IOMMU instances through which the device can master. The list is protected by a kref count and freed when no users remain. It is expected that DMA-mapping code will take a reference if it wishes to make use of the IOMMU information.

Quoted 45 lines

drivers/iommu/of_iommu.c +45 -0

+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct device_node *np;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	/*
+	 * 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 iommu_data *data;
+
+		np = iommu_spec.np;
+		data = of_iommu_get_data(np);
+
+		if (!data || !data->ops || !data->ops->of_xlate)
+			goto err_put_node;
+
+		if (!iommu) {
+			iommu = data;
+		} else if (iommu != data) {
+			/* We don't currently support multi-IOMMU masters */
+			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+				dev_name(dev));
+			goto err_put_node;
+		}
+
+		if (!data->ops->of_xlate(dev, &iommu_spec))
+			goto err_put_node;
+
+		of_node_put(np);
+		idx++;
+	}
+
+	if (!iommu)
+		return NULL;
+
+	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)

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

Quoted 4 lines

drivers/iommu/of_iommu.c +4 -0

+		return NULL;
+
+	kref_init(&mapping->kref);
+	INIT_LIST_HEAD(&mapping->node);

I might be missing something, but that doesn't seem to match the commit message. This creates a single iommu_dma_mapping per device, where is the list supposed to be populated ?

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

Quoted 9 lines

drivers/iommu/of_iommu.c +9 -0

+void of_iommu_deconfigure(struct kref *kref)
+{
+	struct iommu_dma_mapping *mapping, *curr, *next;
+
+	mapping = container_of(kref, struct iommu_dma_mapping, kref);
+	list_for_each_entry_safe(curr, next, &mapping->node, node) {
+		list_del(&curr->node);
+		kfree(curr);
+	}

Don't you need to also kfree(mapping) here ?

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+

Do you expect other users of of_iommu_deconfigure() than kref_put() ? If not, how about exposing an of_iommu_put(struct iommu_dma_mapping *) that would call kref_put() internally, and hiding of_iommu_deconfigure() ?

Quoted 11 lines

void __init of_iommu_init(void)
{

drivers/iommu/of_iommu.c +0 -0

 	struct device_node *np;

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

index 1e944e77d38d..e60e52d82db9 100644
@@ -62,6 +62,14 @@ struct dma_map_ops {
 	int is_phys;
 };
 

include/linux/dma-mapping.h struct dma_map_ops { · line 71

Quoted 7 lines

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

+struct iommu_data;
+
+struct iommu_dma_mapping {
+	struct iommu_data *iommu;
+	struct list_head node;
+	struct kref kref;
+};

Could you please document the structure with kerneldoc ?

Quoted 1 line · Laurent Pinchart

Hi Will,

Hi Laurent,

Quoted 1 line

Thank you for the patch.

Sorry for the delay in replying, I was at Connect last week and the email has backed up.

Quoted 10 lines · On Friday 12 September 2014 17:34:52 Will Deacon wrote:

Quoted 9 lines

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. A list of iommu_dma_mapping structures are created for each device, which represent the set of IOMMU instances through which the device can master. The list is protected by a kref count and freed when no users remain. It is expected that DMA-mapping code will take a reference if it wishes to make use of the IOMMU information.

[...]

Quoted 21 lines

Quoted 21 lines

drivers/iommu/of_iommu.c +21 -0

+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct device_node *np;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	/*
+	 * 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 iommu_data *data;
+
+		np = iommu_spec.np;
+		data = of_iommu_get_data(np);
+

Quoted 8 lines

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

Quoted 8 lines

drivers/iommu/of_iommu.c +8 -0

+		if (!data || !data->ops || !data->ops->of_xlate)
+			goto err_put_node;
+
+		if (!iommu) {
+			iommu = data;
+		} else if (iommu != data) {
+			/* We don't currently support multi-IOMMU masters */
+			pr_warn("Rejecting device %s with multiple IOMMU 

Quoted 16 lines

Quoted 16 lines

drivers/iommu/of_iommu.c +16 -0

+				dev_name(dev));
+			goto err_put_node;
+		}
+
+		if (!data->ops->of_xlate(dev, &iommu_spec))
+			goto err_put_node;
+
+		of_node_put(np);
+		idx++;
+	}
+
+	if (!iommu)
+		return NULL;
+
+	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)

Quoted 8 lines

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

Quoted 4 lines

drivers/iommu/of_iommu.c +4 -0

+		return NULL;
+
+	kref_init(&mapping->kref);
+	INIT_LIST_HEAD(&mapping->node);

I might be missing something, but that doesn't seem to match the commit message. This creates a single iommu_dma_mapping per device, where is the list supposed to be populated ?

Right. I currently only populate the first node in the list, and the loop above barfs if we find multiple IOMMUs. I was hoping you'd add support for that later on, as you mentioned the need for this so I guess you can test it too?

Quoted 11 lines

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

Quoted 9 lines

drivers/iommu/of_iommu.c +9 -0

+void of_iommu_deconfigure(struct kref *kref)
+{
+	struct iommu_dma_mapping *mapping, *curr, *next;
+
+	mapping = container_of(kref, struct iommu_dma_mapping, kref);
+	list_for_each_entry_safe(curr, next, &mapping->node, node) {
+		list_del(&curr->node);
+		kfree(curr);
+	}

Don't you need to also kfree(mapping) here ?

Yup, thanks.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+

Do you expect other users of of_iommu_deconfigure() than kref_put() ? If not, how about exposing an of_iommu_put(struct iommu_dma_mapping *) that would call kref_put() internally, and hiding of_iommu_deconfigure() ?

That's a good idea, I'll do that.

Quoted 11 lines

Quoted 11 lines

void __init of_iommu_init(void)
{

drivers/iommu/of_iommu.c +0 -0

 	struct device_node *np;

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

index 1e944e77d38d..e60e52d82db9 100644
@@ -62,6 +62,14 @@ struct dma_map_ops {
 	int is_phys;
 };
 

Quoted 9 lines

include/linux/dma-mapping.h struct dma_map_ops { · line 71

Quoted 7 lines

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

+struct iommu_data;
+
+struct iommu_dma_mapping {
+	struct iommu_data *iommu;
+	struct list_head node;
+	struct kref kref;
+};

Could you please document the structure with kerneldoc ?

Ok, I'll try!

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

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

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 24 +++++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 27 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3 100644

arch/arm/include/asm/dma-mapping.h +3 -1

@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
 *dev) }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent)
 {
 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index 946dd7ae0394..95ebd38db545 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct platform_device
 *pdev) int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 
 	/*

@@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
 *pdev) dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

drivers/of/platform.c +7 -6

+}
+
+static void of_dma_deconfigure(struct platform_device *pdev)
+{
+	arch_teardown_dma_ops(&pdev->dev);
 }
 
 /**

@@ -224,16 +238,12 @@ static struct platform_device
 *of_platform_device_create_pdata( if (!dev)
 		goto err_clear_flag;
 
-	of_dma_configure(dev);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
-
-	/* We do not fill the DMA ops for platform devices by default.
-	 * This is currently the responsibility of the platform code
-	 * to do such, possibly using a device notifier
-	 */
+	of_dma_configure(dev);
 
 	if (of_device_add(dev) != 0) {
+		of_dma_deconfigure(dev);
 		platform_device_put(dev);
 		goto err_clear_flag;
 	}

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

index e60e52d82db9..47e1ac30e300 100644
@@ -138,7 +138,13 @@ static inline int dma_coerce_mask_and_coherent(struct

device *dev, u64 mask) extern u64 dma_get_required_mask(struct device *dev);

#ifndef arch_setup_dma_ops
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { } +static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,

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

+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent) { }
+#endif
+
+#ifndef arch_teardown_dma_ops
+static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)

Hi Will,

Thank you for the patch.

Quoted 49 lines · On Friday 12 September 2014 17:34:53 Will Deacon wrote:

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

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

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 24 +++++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 27 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3 100644

arch/arm/include/asm/dma-mapping.h +3 -1

@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
 *dev) }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent)
 {
 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index 946dd7ae0394..95ebd38db545 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct platform_device
 *pdev) int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 
 	/*

@@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
 *pdev) dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

drivers/of/platform.c static void of_dma_configure(struct platform_device · line 207

Quoted 10 lines

drivers/of/platform.c +8 -1

 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);

What's the expected life cycle of the iommu_dma_mapping structure ? It gets created by of_iommu_configure() and the initial reference gets dropped here. I suppose you expect arch code to need to keep a reference to the structure, but your implementation in patch 7/7 doesn't. As far as I can see, you don't even use the contents of the structure in the ARM arch_setup_dma_ops() implementation. Do you expect that to change later, or other architectures to need it ?

By the way, now that I think about it, I find struct iommu_dma_mapping and struct dma_iommu_mapping very confusing.

Quoted 69 lines · Laurent Pinchart

Quoted 49 lines · On Friday 12 September 2014 17:34:53 Will Deacon wrote:

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

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

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 24 +++++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 27 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3 100644

arch/arm/include/asm/dma-mapping.h +3 -1

@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
 *dev) }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent)
 {
 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index 946dd7ae0394..95ebd38db545 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct platform_device
 *pdev) int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 
 	/*

@@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
 *pdev) dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

drivers/of/platform.c static void of_dma_configure(struct platform_device · line 207

Quoted 10 lines

drivers/of/platform.c +8 -1

 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);

What's the expected life cycle of the iommu_dma_mapping structure ? It gets created by of_iommu_configure() and the initial reference gets dropped here. I suppose you expect arch code to need to keep a reference to the structure, but your implementation in patch 7/7 doesn't. As far as I can see, you don't even use the contents of the structure in the ARM arch_setup_dma_ops() implementation. Do you expect that to change later, or other architectures to need it ?

Quoted 2 lines

By the way, now that I think about it, I find struct iommu_dma_mapping and struct dma_iommu_mapping very confusing.

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

Quoted 76 lines · Thierry Reding

Quoted 75 lines · Laurent Pinchart

Quoted 55 lines · On Friday 12 September 2014 17:34:53 Will Deacon wrote:

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

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

---

 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 24 +++++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 27 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3 100644

arch/arm/include/asm/dma-mapping.h +3 -1

@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct
 device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool
 coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64
 dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent)

{

arch/arm/include/asm/dma-mapping.h +0 -0

 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index 946dd7ae0394..95ebd38db545 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct
 platform_device *pdev)
 	int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 

drivers/of/platform.c +0 -0

 	/*

@@ -195,7 +197,19 @@ static void of_dma_configure(struct
 platform_device *pdev)
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

drivers/of/platform.c static void of_dma_configure(struct · line 208

Quoted 10 lines

drivers/of/platform.c +8 -1

 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);

What's the expected life cycle of the iommu_dma_mapping structure ? It gets created by of_iommu_configure() and the initial reference gets dropped here. I suppose you expect arch code to need to keep a reference to the structure, but your implementation in patch 7/7 doesn't. As far as I can see, you don't even use the contents of the structure in the ARM arch_setup_dma_ops() implementation. Do you expect that to change later, or other architectures to need it ?

Quoted 7 lines

Quoted 2 lines

By the way, now that I think about it, I find struct iommu_dma_mapping and struct dma_iommu_mapping very confusing.

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

I'm struggling to think how that would look. What do you mean by `master interfaces' in terms of the code we have in Linux? At the end of the day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you and Laurent have use-cases involving devices mastering through multiple IOMMUs. If it doesn't work for you, it might be best for you to send me the patch ;)

Hi Will,

Quoted 6 lines · Thierry Reding

Quoted 4 lines

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

Quoted 6 lines

I'm struggling to think how that would look. What do you mean by `master interfaces' in terms of the code we have in Linux? At the end of the day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you and Laurent have use-cases involving devices mastering through multiple IOMMUs. If it doesn't work for you, it might be best for you to send me the patch ;)

Just for the record, I've brought up the topic of masters being served by multiple IOMMUs, but don't have a use case for it (yet at least). I do have masters served through multiple streams with separate stream IDs, but all by the same IOMMU.

Quoted 12 lines · Laurent Pinchart

Quoted 11 lines · On Monday 22 September 2014 18:50:27 Will Deacon wrote:

Quoted 4 lines · Thierry Reding

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

I'm struggling to think how that would look. What do you mean by `master interfaces' in terms of the code we have in Linux? At the end of the day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you and Laurent have use-cases involving devices mastering through multiple IOMMUs. If it doesn't work for you, it might be best for you to send me the patch ;)

Quoted 4 lines

Just for the record, I've brought up the topic of masters being served by multiple IOMMUs, but don't have a use case for it (yet at least). I do have masters served through multiple streams with separate stream IDs, but all by the same IOMMU.

Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the DMA-mapping API should *not* be exposed to the details of masters that master through multiple IOMMUs. Instead, that should be abstracted by the device API by exposing that device as a single struct device.

So, that's certainly an area that needs more work and I'll drop the limited support I'd cooked up from this patch set in the next version.

Quoted 14 lines · Laurent Pinchart

Quoted 6 lines · On Monday 22 September 2014 18:50:27 Will Deacon wrote:

Quoted 4 lines · Thierry Reding

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

Quoted 6 lines

I'm struggling to think how that would look. What do you mean by `master interfaces' in terms of the code we have in Linux? At the end of the day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you and Laurent have use-cases involving devices mastering through multiple IOMMUs. If it doesn't work for you, it might be best for you to send me the patch ;)

Quoted 4 lines

Just for the record, I've brought up the topic of masters being served by multiple IOMMUs, but don't have a use case for it (yet at least). I do have masters served through multiple streams with separate stream IDs, but all by the same IOMMU.

Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the DMA-mapping API should *not* be exposed to the details of masters that master through multiple IOMMUs. Instead, that should be abstracted by the device API by exposing that device as a single struct device.

So, that's certainly an area that needs more work and I'll drop the limited support I'd cooked up from this patch set in the next version.

Hi Will,

Quoted 20 lines · Laurent Pinchart

Quoted 18 lines · On Monday 22 September 2014 18:50:27 Will Deacon wrote:

Quoted 6 lines · Thierry Reding

Quoted 4 lines

Agreed. I wonder how useful it is to know the set of IOMMU instances that each device can master through. Wouldn't it be more useful to keep a list of master interfaces for each device? The set of IOMMU instances can trivially be derived from that.

Quoted 6 lines

I'm struggling to think how that would look. What do you mean by `master interfaces' in terms of the code we have in Linux? At the end of the day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you and Laurent have use-cases involving devices mastering through multiple IOMMUs. If it doesn't work for you, it might be best for you to send me the patch ;)

Just for the record, I've brought up the topic of masters being served by multiple IOMMUs, but don't have a use case for it (yet at least). I do have masters served through multiple streams with separate stream IDs, but all by the same IOMMU.

Quoted 4 lines

Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the DMA-mapping API should *not* be exposed to the details of masters that master through multiple IOMMUs. Instead, that should be abstracted by the device API by exposing that device as a single struct device.

I'm not sure to follow you here. Aren't we already exposing masters that master through multiple IOMMUs as single instances of struct device ?

Quoted 2 lines

So, that's certainly an area that needs more work and I'll drop the limited support I'd cooked up from this patch set in the next version.

How about masters connected to multiple stream IDs of the same IOMMU ?

Quoted 2 lines · Laurent Pinchart

I'm not sure to follow you here. Aren't we already exposing masters that master through multiple IOMMUs as single instances of struct device ?

Hmm, yes, now you've confused me too! The conclusion was certainly that dma-mapping should not be the one dealing with the I/O topology. Domain allocation would then be an iommu callback (something like ->get_default_domain), but the rest of the details weren't fleshed out.

Hi Will,

drivers/of/platform.c static void of_dma_configure(struct platform_device · line 207

Quoted 13 lines

drivers/of/platform.c +8 -1

@@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
 *pdev) dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);

What's the expected life cycle of the iommu_dma_mapping structure ? It gets created by of_iommu_configure() and the initial reference gets dropped here. I suppose you expect arch code to need to keep a reference to the structure, but your implementation in patch 7/7 doesn't. As far as I can see, you don't even use the contents of the structure in the ARM arch_setup_dma_ops() implementation. Do you expect that to change later, or other architectures to need it ?

By the way, now that I think about it, I find struct iommu_dma_mapping and struct dma_iommu_mapping very confusing.

We need to ensure that the IOMMUs in the system have a chance to perform some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Quoted 4 lines · Will Deacon

We need to ensure that the IOMMUs in the system have a chance to perform some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Why can't you call it from of_platform_populate() directly? That way it would be usable for all architectures rather than just ARM. Eventually we're going to need the same thing for 64-bit ARM (and possibly others as well).

Quoted 6 lines · On Monday 22 September 2014 11:36:15 Thierry Reding wrote:

Quoted 4 lines · Will Deacon

We need to ensure that the IOMMUs in the system have a chance to perform some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Quoted 4 lines

Why can't you call it from of_platform_populate() directly? That way it would be usable for all architectures rather than just ARM. Eventually we're going to need the same thing for 64-bit ARM (and possibly others as well).

IIRC, of_platform_populate can be called multiple times, even recursively be drivers that populate their own child devices.

Quoted 12 lines · Arnd Bergmann

Quoted 10 lines · On Monday 22 September 2014 11:36:15 Thierry Reding wrote:

Quoted 4 lines · Will Deacon

We need to ensure that the IOMMUs in the system have a chance to perform some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Why can't you call it from of_platform_populate() directly? That way it would be usable for all architectures rather than just ARM. Eventually we're going to need the same thing for 64-bit ARM (and possibly others as well).

Quoted 2 lines

IIRC, of_platform_populate can be called multiple times, even recursively be drivers that populate their own child devices.

Indeed. Perhaps it could be conditionally called when root == NULL. But perhaps that's not safe either. Anyway, I still think we shouldn't be making this some randomly placed early initcall anyway.

Quoted 16 lines · On Monday 22 September 2014 13:40:40 Thierry Reding wrote:

Quoted 14 lines · Arnd Bergmann

Quoted 10 lines · On Monday 22 September 2014 11:36:15 Thierry Reding wrote:

Quoted 4 lines · Will Deacon

We need to ensure that the IOMMUs in the system have a chance to perform some basic initialisation before we start adding masters to them.

This patch adds a call to of_iommu_init before of_platform_populate.

Why can't you call it from of_platform_populate() directly? That way it would be usable for all architectures rather than just ARM. Eventually we're going to need the same thing for 64-bit ARM (and possibly others as well).

IIRC, of_platform_populate can be called multiple times, even recursively be drivers that populate their own child devices.

Quoted 3 lines

Indeed. Perhaps it could be conditionally called when root == NULL. But perhaps that's not safe either. Anyway, I still think we shouldn't be making this some randomly placed early initcall anyway.

I disagree. IOMMUs are special in the same sense that irqchips, clocks and timers are. This is not just a random call, it is being explicit about a base functionality that is needed for all devices attached to it.

While we pretend that these are just device drivers in some sense, I think it's perfectly reasonable to have an explicit entry point for each subsystem here.

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

The reason is that a driver does not actively ask for an IOMMU as it would for other subsystems (gpio, led, dmaengine, ...).

- As long as the dma_ops are not set, we can't actually call probe() for any device other than iommus, and even that would require adding special magic in the platform_device_probe(), so we'd just defer every device until we get to the iommu. This likely causes a lot of overhead at probe time.

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 2 lines

The reason is that a driver does not actively ask for an IOMMU as it would for other subsystems (gpio, led, dmaengine, ...).

Actually it does. At least in some cases. If you want to use the IOMMU API directly you'd call something like iommu_present() on the bus type and then allocate an IOMMU domain and attach to it. Unfortunately the API seems to have been designed under the assumption that IOMMU will have been registered before any users, so the API doesn't propagate any meaningful errors.

Also, even if in other cases the drivers don't actively ask for an IOMMU that doesn't mean that they couldn't be made to. For drivers that use the DMA/IOMMU integration layer this is probably not practicable, but there is no reason the core couldn't do it.

Quoted 6 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

Quoted 2 lines

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

Quoted 2 lines

The reason is that a driver does not actively ask for an IOMMU as it would for other subsystems (gpio, led, dmaengine, ...).

Actually it does. At least in some cases. If you want to use the IOMMU API directly you'd call something like iommu_present() on the bus type and then allocate an IOMMU domain and attach to it. Unfortunately the API seems to have been designed under the assumption that IOMMU will have been registered before any users, so the API doesn't propagate any meaningful errors.

That's just a special case that does not even work as it should yet,

please don't confuse the matter more by talking about drivers that use the IOMMU API explicitly, this series has very little to do with that.

Quoted 4 lines

Also, even if in other cases the drivers don't actively ask for an IOMMU that doesn't mean that they couldn't be made to. For drivers that use the DMA/IOMMU integration layer this is probably not practicable, but there is no reason the core couldn't do it.

We intentionally have an abstraction that is meant to let you write drivers without knowing about iommu, swiotlb or coherency, these are all hidden behind the dma_map_ops implementation.

Quoted 8 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

Quoted 2 lines

The reason is that a driver does not actively ask for an IOMMU as it would for other subsystems (gpio, led, dmaengine, ...).

Actually it does. At least in some cases. If you want to use the IOMMU API directly you'd call something like iommu_present() on the bus type and then allocate an IOMMU domain and attach to it. Unfortunately the API seems to have been designed under the assumption that IOMMU will have been registered before any users, so the API doesn't propagate any meaningful errors.

That's just a special case that does not even work as it should yet, please don't confuse the matter more by talking about drivers that use the IOMMU API explicitly, this series has very little to do with that.

Quoted 4 lines

Also, even if in other cases the drivers don't actively ask for an IOMMU that doesn't mean that they couldn't be made to. For drivers that use the DMA/IOMMU integration layer this is probably not practicable, but there is no reason the core couldn't do it.

We intentionally have an abstraction that is meant to let you write drivers without knowing about iommu, swiotlb or coherency, these are all hidden behind the dma_map_ops implementation.

Quoted 10 lines · On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:

Quoted 8 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 7 lines

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

If we want IOMMU devices to be supported by common device drivers we need to defer probing of the master devices, there's no doubt about that. Earlier approaches that hooked up into the device core code were rejected, but it should be possible to use bus notifiers to achieve the same result (with the drawback of having to register one notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return -EPROBE_DEFER when a iommus property is available and points to an IOMMU not registered yet. I'm not saying we have to do this, but I believe that at least from a technical point of view it could be done.

Quoted 19 lines · On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:

Quoted 10 lines · On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:

Quoted 8 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 7 lines

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

If we want IOMMU devices to be supported by common device drivers we need to defer probing of the master devices, there's no doubt about that. Earlier approaches that hooked up into the device core code were rejected, but it should be possible to use bus notifiers to achieve the same result (with the drawback of having to register one notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return -EPROBE_DEFER when a iommus property is available and points to an IOMMU not registered yet. I'm not saying we have to do this, but I believe that at least from a technical point of view it could be done.

I think that fundamentally speaking, relying on notifiers for something like this is very problematic, both in terms of maintainability and reliability. We should really try to get the notifiers out of the iommu handling, not put more of them in.

Quoted 30 lines · Arnd Bergmann

Quoted 19 lines · On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:

Quoted 10 lines · On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:

Quoted 8 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 7 lines

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

If we want IOMMU devices to be supported by common device drivers we need to defer probing of the master devices, there's no doubt about that. Earlier approaches that hooked up into the device core code were rejected, but it should be possible to use bus notifiers to achieve the same result (with the drawback of having to register one notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return - EPROBE_DEFER when a iommus property is available and points to an IOMMU not registered yet. I'm not saying we have to do this, but I believe that at least from a technical point of view it could be done.

Quoted 4 lines

I think that fundamentally speaking, relying on notifiers for something like this is very problematic, both in terms of maintainability and reliability. We should really try to get the notifiers out of the iommu handling, not put more of them in.

Agreed. Also last time I checked the driver core simply ignored the return value from notifiers, therefore this wouldn't work without changing the core either.

Still, I agree with Laurent that we really should be relying on probe deferral for probe ordering.

And while it's true that earlier attempts to put this into the core were rejected, I think there's still value in proposing it again. The alternative proposed here is similarly close to the core and needs to duplicated for every architecture. That itself is to me a strong indication that this really does belong in the core.

I think initially this was proposed to become part of really_probe() and I still think that's where it belongs. There's precedent for it with the pinctrl_bind_pins() call, though it seems like Greg regrets allowing that into the core. Perhaps if really_probe() is "too core", then platform_drv_probe() would be a better candidate?

Hi Thierry,

Quoted 36 lines · On Tuesday 14 October 2014 15:37:59 Thierry Reding wrote:

Quoted 30 lines · Arnd Bergmann

Quoted 19 lines · On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:

Quoted 10 lines · On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:

Quoted 8 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 5 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 7 lines

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

If we want IOMMU devices to be supported by common device drivers we need to defer probing of the master devices, there's no doubt about that. Earlier approaches that hooked up into the device core code were rejected, but it should be possible to use bus notifiers to achieve the same result (with the drawback of having to register one notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return - EPROBE_DEFER when a iommus property is available and points to an IOMMU not registered yet. I'm not saying we have to do this, but I believe that at least from a technical point of view it could be done.

Quoted 4 lines

I think that fundamentally speaking, relying on notifiers for something like this is very problematic, both in terms of maintainability and reliability. We should really try to get the notifiers out of the iommu handling, not put more of them in.

Quoted 6 lines

Agreed. Also last time I checked the driver core simply ignored the return value from notifiers, therefore this wouldn't work without changing the core either.

Still, I agree with Laurent that we really should be relying on probe deferral for probe ordering.

*If* we decide to support IOMMUs with real devices ;-)

I don't have a strong opinion on the subject. I initially thought it would be a shame not to be able to use devres, until realizing that binding to a DT node instead of a device meant that no unbind can ever occur. Loosing dev_* support is also an annoyance though.

Quoted 47 lines · Laurent Pinchart

Quoted 37 lines · On Tuesday 14 October 2014 15:37:59 Thierry Reding wrote:

Quoted 31 lines · Arnd Bergmann

Quoted 20 lines · On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:

Quoted 11 lines · On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:

Quoted 9 lines · On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

Quoted 6 lines

I see two problems with using deferred probing here:

- we don't actually need to defer the probing but the binding to the driver when no dma ops are set, but it seems silly to even create the device before we can find out which ops it should use.

What does device creation have to do with anything? Surely a device won't need IOMMU services before the device is bound to a driver.

Quoted 7 lines

The problem is that the driver will start using the IOMMU as soon as it calls dma_map_*, but that happens at runtime, not necessarily during the probe function.

So we can get into the weird situation that probe() returns success, but then you can't use the device yet because you don't know whether it is supposed to use an IOMMU or not.

Quoted 9 lines

If we want IOMMU devices to be supported by common device drivers we need to defer probing of the master devices, there's no doubt about that. Earlier approaches that hooked up into the device core code were rejected, but it should be possible to use bus notifiers to achieve the same result (with the drawback of having to register one notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return -EPROBE_DEFER when a iommus property is available and points to an IOMMU not registered yet. I'm not saying we have to do this, but I believe that at least from a technical point of view it could be done.

Quoted 4 lines

I think that fundamentally speaking, relying on notifiers for something like this is very problematic, both in terms of maintainability and reliability. We should really try to get the notifiers out of the iommu handling, not put more of them in.

Quoted 6 lines

Agreed. Also last time I checked the driver core simply ignored the return value from notifiers, therefore this wouldn't work without changing the core either.

Still, I agree with Laurent that we really should be relying on probe deferral for probe ordering.

*If* we decide to support IOMMUs with real devices ;-)

Quoted 4 lines

I don't have a strong opinion on the subject. I initially thought it would be a shame not to be able to use devres, until realizing that binding to a DT node instead of a device meant that no unbind can ever occur. Loosing dev_* support is also an annoyance though.

Binding to a DT node then also means that you can't build the driver as a module. And in particular for multiplatform kernels this is going to be a problem eventually.

On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
[...]

arch/arm/mm/dma-mapping.c void arm_iommu_detach_device(struct device *dev) · line 2093

Quoted 5 lines

+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size) +{

arch/arm/mm/dma-mapping.c +3 -0

+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);

If I understand correctly this will be called for each device that has an IOMMU master interface and will end up creating a new mapping for each of the devices. Each of these mappings will translate to a domain in the IOMMU API, which in turn is a separate address space.

How do you envision to support use-cases where a set of devices need to share a single domain? This is needed for example in DRM where SoCs often have a set of hardware blocks (each with its own master interface) that compose the display device. On Tegra for example there are two display controllers that need access to the same IOVA domain so that they can scan out framebuffers.

On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
[...]

arch/arm/mm/dma-mapping.c void arm_iommu_detach_device(struct device *dev) · line 2093

Quoted 5 lines

arch/arm/mm/dma-mapping.c +5 -0

+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);

If I understand correctly this will be called for each device that has an IOMMU master interface and will end up creating a new mapping for each of the devices. Each of these mappings will translate to a domain in the IOMMU API, which in turn is a separate address space.

How do you envision to support use-cases where a set of devices need to share a single domain? This is needed for example in DRM where SoCs often have a set of hardware blocks (each with its own master interface) that compose the display device. On Tegra for example there are two display controllers that need access to the same IOVA domain so that they can scan out framebuffers.

Hi Thierry,

Quoted 12 lines · Thierry Reding

On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
[...]

arch/arm/mm/dma-mapping.c void arm_iommu_detach_device(struct device *dev) · line 2093

Quoted 5 lines

arch/arm/mm/dma-mapping.c +5 -0

+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);

If I understand correctly this will be called for each device that has an IOMMU master interface and will end up creating a new mapping for each of the devices. Each of these mappings will translate to a domain in the IOMMU API, which in turn is a separate address space.

Correct, although that's largely because I've bolted on the existing ARM IOMMU code.

Quoted 6 lines

How do you envision to support use-cases where a set of devices need to share a single domain? This is needed for example in DRM where SoCs often have a set of hardware blocks (each with its own master interface) that compose the display device. On Tegra for example there are two display controllers that need access to the same IOVA domain so that they can scan out framebuffers.

Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops contains a domain and an allocator for each IOMMU instance in the system. It would then be up to the architecture how it makes use of those, but the most obvious thing to do would be to attach devices mastering through an IOMMU instance to that per-instance domain.

The other use-case is isolation (one domain per device), which I guess matches what the ARM code is doing at the moment.

Quoted 8 lines · Will Deacon

Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops contains a domain and an allocator for each IOMMU instance in the system. It would then be up to the architecture how it makes use of those, but the most obvious thing to do would be to attach devices mastering through an IOMMU instance to that per-instance domain.

The other use-case is isolation (one domain per device), which I guess matches what the ARM code is doing at the moment.

I think there are two cases here. You can have a composite device that wants to manage a single domain (using its own allocator) for a set of hardware devices. At the same time a set of devices (think 2D and 3D engines) could want to use a multiple domains for process separation. In that case I'd expect a logical DRM device to allocate one domain per process and then associate the 2D and 3D engines with that same domain on process switch.

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Will Deacon

Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops contains a domain and an allocator for each IOMMU instance in the system. It would then be up to the architecture how it makes use of those, but the most obvious thing to do would be to attach devices mastering through an IOMMU instance to that per-instance domain.

The other use-case is isolation (one domain per device), which I guess matches what the ARM code is doing at the moment.

Quoted 7 lines

I think there are two cases here. You can have a composite device that wants to manage a single domain (using its own allocator) for a set of hardware devices. At the same time a set of devices (think 2D and 3D engines) could want to use a multiple domains for process separation. In that case I'd expect a logical DRM device to allocate one domain per process and then associate the 2D and 3D engines with that same domain on process switch.

Sure, but that's well outside of what the dma-mapping API is going to setup as a default domain. These specialist setups are certainly possible, but I think they should be driven by, for example, the DRM code as opposed to being in the core dma-mapping code.

Quoted 9 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

Quoted 19 lines · Will Deacon

Quoted 17 lines · Thierry Reding

Quoted 8 lines · Will Deacon

Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops contains a domain and an allocator for each IOMMU instance in the system. It would then be up to the architecture how it makes use of those, but the most obvious thing to do would be to attach devices mastering through an IOMMU instance to that per-instance domain.

The other use-case is isolation (one domain per device), which I guess matches what the ARM code is doing at the moment.

I think there are two cases here. You can have a composite device that wants to manage a single domain (using its own allocator) for a set of hardware devices. At the same time a set of devices (think 2D and 3D engines) could want to use a multiple domains for process separation. In that case I'd expect a logical DRM device to allocate one domain per process and then associate the 2D and 3D engines with that same domain on process switch.

Quoted 4 lines

Sure, but that's well outside of what the dma-mapping API is going to setup as a default domain. These specialist setups are certainly possible, but I think they should be driven by, for example, the DRM code as opposed to being in the core dma-mapping code.

I completely agree that these special cases should be driven by the drivers that need them. However the problem here is that the current patch will already attach the device to an IOMMU domain by default.

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Quoted 10 lines

Quoted 9 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

Quoted 4 lines

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

It's not an extension, really. It's more that both need to be able to coexist. For some devices you may want to create an IOMMU domain and hook it up with the DMA mapping functions, for others you don't and handle mapping to IOVA space explicitly.

There is another issue with the approach you propose. I'm not sure if Tegra is special in this case (I'd expect not), but what we do is make an IOMMU domain correspond to an address space. Address spaces are a pretty limited resource (earlier generations have 4, newer have 128) and each address space can be up to 4 GiB. So I've always envisioned that we should be using a single IOMMU domain for devices that don't expose direct buffer access to userspace (SATA, PCIe, audio, SD/MMC, USB, ...). All of those would typically need only a small number of small buffers, so using a separate address space for each seems like a big waste.

Doing so would leave a large number of address spaces available for things like a GPU driver to keep per-process address spaces for isolation.

I don't see how we'd be able to do that with the approach that you propose in this series since it assumes that each device will be associated with a separate domain.

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:

Quoted 3 lines

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Which drivers and how would they know what to do? I think you might be jumping the gun a bit here, given where mainline is with using the IOMMU for anything at all.

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

Quoted 4 lines

It's not an extension, really. It's more that both need to be able to coexist. For some devices you may want to create an IOMMU domain and hook it up with the DMA mapping functions, for others you don't and handle mapping to IOVA space explicitly.

I think it's an extension in the sense that mainline doesn't currently do what you want, regardless of this patch series.

Quoted 7 lines

Doing so would leave a large number of address spaces available for things like a GPU driver to keep per-process address spaces for isolation.

I don't see how we'd be able to do that with the approach that you propose in this series since it assumes that each device will be associated with a separate domain.

No, that's an artifact of the existing code on ARM. My series adds a list of domains to each device, but those domains are per-IOMMU instance and can appear in multiple lists.

Quoted 1 line · Will Deacon

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:

[...]

Quoted 7 lines

Quoted 3 lines

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Which drivers and how would they know what to do? I think you might be jumping the gun a bit here, given where mainline is with using the IOMMU for anything at all.

I don't think I am. I've been working on patches to enable IOMMU on Tegra, with the specific use-case that we want to use it to allow physically non-contiguous framebuffers to be used for scan out.

In order to do so the DRM driver allocates an IOMMU domain and adds both display controllers to it. When a framebuffer is created or imported from DMA-BUF, it gets mapped into this domain and both display controllers can use the IOVA address as the framebuffer base address.

Given that a device can only be attached to a single domain at a time this will cause breakage when the ARM glue code starts automatically attaching the display controllers to a default domain.

Quoted 20 lines

Quoted 19 lines

Quoted 10 lines

Quoted 9 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

Quoted 4 lines

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

It's not an extension, really. It's more that both need to be able to coexist. For some devices you may want to create an IOMMU domain and hook it up with the DMA mapping functions, for others you don't and handle mapping to IOVA space explicitly.

Quoted 2 lines

I think it's an extension in the sense that mainline doesn't currently do what you want, regardless of this patch series.

It's interesting since you're now the second person to say this. Can you please elaborate why you think that's the case?

I do have local patches that allow precisely this use-case to work without changes to the IOMMU core or requiring any extra ARM-specific glue.

There's a fair bit of jumping through hoops, because for example you don't know what IOMMU instance a domain belongs to at .domain_init() time, so I have to defer most of the actual domain initalization until a device is actually attached to it, but I digress.

Quoted 8 lines

Quoted 7 lines

Doing so would leave a large number of address spaces available for things like a GPU driver to keep per-process address spaces for isolation.

I don't see how we'd be able to do that with the approach that you propose in this series since it assumes that each device will be associated with a separate domain.

Quoted 3 lines

No, that's an artifact of the existing code on ARM. My series adds a list of domains to each device, but those domains are per-IOMMU instance and can appear in multiple lists.

So you're saying the end result will be that there's a single domain per IOMMU device that will be associated with all devices that have a master interface to it?

Hi Thierry,

Quoted 15 lines · Thierry Reding

Quoted 1 line · Will Deacon

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:

[...]

Quoted 7 lines

Quoted 3 lines

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Which drivers and how would they know what to do? I think you might be jumping the gun a bit here, given where mainline is with using the IOMMU for anything at all.

I don't think I am. I've been working on patches to enable IOMMU on Tegra, with the specific use-case that we want to use it to allow physically non-contiguous framebuffers to be used for scan out.

Quoted 4 lines

In order to do so the DRM driver allocates an IOMMU domain and adds both display controllers to it. When a framebuffer is created or imported from DMA-BUF, it gets mapped into this domain and both display controllers can use the IOVA address as the framebuffer base address.

Does that mean you manually swizzle the dma_map_ops for the device in the DRM driver?

Quoted 3 lines

Given that a device can only be attached to a single domain at a time this will cause breakage when the ARM glue code starts automatically attaching the display controllers to a default domain.

Why couldn't you just re-use the domain already allocated by the DMA mapping API?

Quoted 20 lines

Quoted 20 lines

Quoted 19 lines

Quoted 10 lines

Quoted 9 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

Quoted 4 lines

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

It's not an extension, really. It's more that both need to be able to coexist. For some devices you may want to create an IOMMU domain and hook it up with the DMA mapping functions, for others you don't and handle mapping to IOVA space explicitly.

Quoted 5 lines

Quoted 2 lines

I think it's an extension in the sense that mainline doesn't currently do what you want, regardless of this patch series.

It's interesting since you're now the second person to say this. Can you please elaborate why you think that's the case?

Because the only way to set up DMA through an IOMMU on ARM is via the arm_iommu_* functions,

which are currently called from a subset of the IOMMU drivers themselves:

drivers/gpu/drm/exynos/exynos_drm_iommu.c
drivers/iommu/ipmmu-vmsa.c
drivers/iommu/shmobile-iommu.c
drivers/media/platform/omap3isp/isp.c

Of these, ipmmu-vmsa.c and shmobile.c both allocate a domain per device. The omap3 code seems to do something similar. That just leaves the exynos driver, which Marek has been reworking anyway.

Quoted 21 lines

I do have local patches that allow precisely this use-case to work without changes to the IOMMU core or requiring any extra ARM-specific glue.

There's a fair bit of jumping through hoops, because for example you don't know what IOMMU instance a domain belongs to at .domain_init() time, so I have to defer most of the actual domain initalization until a device is actually attached to it, but I digress.

Quoted 11 lines

Quoted 7 lines

Doing so would leave a large number of address spaces available for things like a GPU driver to keep per-process address spaces for isolation.

I don't see how we'd be able to do that with the approach that you propose in this series since it assumes that each device will be associated with a separate domain.

No, that's an artifact of the existing code on ARM. My series adds a list of domains to each device, but those domains are per-IOMMU instance and can appear in multiple lists.

Quoted 3 lines

So you're saying the end result will be that there's a single domain per IOMMU device that will be associated with all devices that have a master interface to it?

Yes, that's the plan. Having thought about it some more (after your comments), subsystems can still call of_dma_deconfigure if they want to do their own IOMMU domain management. That may well be useful for things like VFIO, for example.

Quoted 23 lines · Will Deacon

Quoted 21 lines · Thierry Reding

Quoted 1 line · Will Deacon

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:

[...]

Quoted 8 lines

Quoted 4 lines

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Which drivers and how would they know what to do? I think you might be jumping the gun a bit here, given where mainline is with using the IOMMU for anything at all.

I don't think I am. I've been working on patches to enable IOMMU on Tegra, with the specific use-case that we want to use it to allow physically non-contiguous framebuffers to be used for scan out.

In order to do so the DRM driver allocates an IOMMU domain and adds both display controllers to it. When a framebuffer is created or imported from DMA-BUF, it gets mapped into this domain and both display controllers can use the IOVA address as the framebuffer base address.

Quoted 2 lines

Does that mean you manually swizzle the dma_map_ops for the device in the DRM driver?

No. It means we use the IOMMU API directly instead of the DMA mapping API.

Quoted 4 lines

Quoted 3 lines

Given that a device can only be attached to a single domain at a time this will cause breakage when the ARM glue code starts automatically attaching the display controllers to a default domain.

Quoted 2 lines

Why couldn't you just re-use the domain already allocated by the DMA mapping API?

Because I don't see how you'd get access to it. And provided that we could do that it would also mean that there'd be at least two domains (one for each display controller) and we'd need to decide on using a single one of them. Which one do we choose? And what about the unused one? If there's no way to detach it we loose a precious resource.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

I'd live to have as little of this as possible in the IOMMU drivers, as we should leave those to deal with the IOMMU hardware and not domain management. Having subsystems manage their own dma ops is an extension to the dma-mapping API.

It's not an extension, really. It's more that both need to be able to coexist. For some devices you may want to create an IOMMU domain and hook it up with the DMA mapping functions, for others you don't and handle mapping to IOVA space explicitly.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I think it's an extension in the sense that mainline doesn't currently do what you want, regardless of this patch series.

It's interesting since you're now the second person to say this. Can you please elaborate why you think that's the case?

Because the only way to set up DMA through an IOMMU on ARM is via the arm_iommu_* functions,

No, you can use the IOMMU API directly just fine.

Quoted 9 lines

which are currently called from a subset of the IOMMU drivers themselves:
drivers/gpu/drm/exynos/exynos_drm_iommu.c drivers/iommu/ipmmu-vmsa.c
drivers/iommu/shmobile-iommu.c drivers/media/platform/omap3isp/isp.c

Of these, ipmmu-vmsa.c and shmobile.c both allocate a domain per device. The omap3 code seems to do something similar. That just leaves the exynos driver, which Marek has been reworking anyway.

Right, and as I remember one of the things that Marek did was introduce a flag to mark drivers as doing their own IOMMU domain management so that they wouldn't be automatically associated with a "mapping".

Quoted 21 lines

Quoted 21 lines

I do have local patches that allow precisely this use-case to work without changes to the IOMMU core or requiring any extra ARM-specific glue.

There's a fair bit of jumping through hoops, because for example you don't know what IOMMU instance a domain belongs to at .domain_init() time, so I have to defer most of the actual domain initalization until a device is actually attached to it, but I digress.

Quoted 11 lines

Quoted 7 lines

Doing so would leave a large number of address spaces available for things like a GPU driver to keep per-process address spaces for isolation.

I don't see how we'd be able to do that with the approach that you propose in this series since it assumes that each device will be associated with a separate domain.

No, that's an artifact of the existing code on ARM. My series adds a list of domains to each device, but those domains are per-IOMMU instance and can appear in multiple lists.

Quoted 8 lines

Quoted 3 lines

So you're saying the end result will be that there's a single domain per IOMMU device that will be associated with all devices that have a master interface to it?

Yes, that's the plan. Having thought about it some more (after your comments), subsystems can still call of_dma_deconfigure if they want to do their own IOMMU domain management. That may well be useful for things like VFIO, for example.

I think it's really weird to set up some complicated data structures at early boot without knowing that they'll ever be used and then require drivers to undo that if they decide not to use it.

As mentioned in an earlier reply I don't see why we need to set this all up that early in the boot in the first place. It only becomes important right before a driver's .probe() is called because the device can't perform any DMA-related operations before that point in time.

Now if we postpone initialization of the IOMMU masters and swizzling of the DMA operations until driver probe time we get rid of a lot of problems. For example we could use deferred probing if the IOMMU driver hasn't loaded yet. That in turn would allow IOMMU drivers to be built as modules rather than built-in. And especially with multi-platform kernels I think we really want to build as much as possible as modules.

Hi Thierry and Will,

Quoted 27 lines · On Monday 06 October 2014 11:52:50 Thierry Reding wrote:

Quoted 25 lines · Will Deacon

Quoted 13 lines · Thierry Reding

Quoted 1 line · Will Deacon

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:

[...]

Quoted 8 lines

Quoted 4 lines

So I think what we're going to need is a way to prevent the default attachment to DMA/IOMMU. Or alternatively not associate devices with IOMMU domains by default but let drivers explicitly make the decision.

Which drivers and how would they know what to do? I think you might be jumping the gun a bit here, given where mainline is with using the IOMMU for anything at all.

Quoted 8 lines

I don't think I am. I've been working on patches to enable IOMMU on Tegra, with the specific use-case that we want to use it to allow physically non-contiguous framebuffers to be used for scan out.

In order to do so the DRM driver allocates an IOMMU domain and adds both display controllers to it. When a framebuffer is created or imported from DMA-BUF, it gets mapped into this domain and both display controllers can use the IOVA address as the framebuffer base address.

Does that mean you manually swizzle the dma_map_ops for the device in the DRM driver?

Quoted 2 lines

No. It means we use the IOMMU API directly instead of the DMA mapping API.

Is there a reason why you can't use the DMA mapping API for this, assuming of course that it would provide a way to attach both display controllers to the same domain ? Do you need to have explicit control over the VA at which the buffers are mapped ?

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Given that a device can only be attached to a single domain at a time this will cause breakage when the ARM glue code starts automatically attaching the display controllers to a default domain.

Quoted 8 lines

Quoted 2 lines

Why couldn't you just re-use the domain already allocated by the DMA mapping API?

Because I don't see how you'd get access to it. And provided that we could do that it would also mean that there'd be at least two domains (one for each display controller) and we'd need to decide on using a single one of them. Which one do we choose? And what about the unused one? If there's no way to detach it we loose a precious resource.

This would also be an issue for my Renesas IOMMU (ipmmu-vmsa) use cases. The IOMMU supports up to four domains (each of them having its own hardware TLB) and shares them between all the bus masters connected to the IOMMU. The connections between bus master and TLBs are configurable. I thus can't live with one domain being created per device.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

What I proposed a while back was to leave it up to the IOMMU driver to choose an allocator for the device. Or rather, choose whether to use a custom allocator or the DMA/IOMMU integration allocator. The way this worked was to keep a list of devices in the IOMMU driver. Devices in this list would be added to domain reserved for DMA/IOMMU integration. Those would typically be devices such as SD/MMC, audio, ... devices that are in-kernel and need no per-process separation. By default devices wouldn't be added to a domain, so devices forming a composite DRM device would be able to manage their own domain.

The problem with your solution is that it requires knowledge of all bus master devices in the IOMMU driver. That's not where that knowledge belongs, as it's a property of a particular SoC integration, not of the IOMMU itself.

Quoted 14 lines

Quoted 4 lines

Yes, that's the plan. Having thought about it some more (after your comments), subsystems can still call of_dma_deconfigure if they want to do their own IOMMU domain management. That may well be useful for things like VFIO, for example.

I think it's really weird to set up some complicated data structures at early boot without knowing that they'll ever be used and then require drivers to undo that if they decide not to use it.

As mentioned in an earlier reply I don't see why we need to set this all up that early in the boot in the first place. It only becomes important right before a driver's .probe() is called because the device can't perform any DMA-related operations before that point in time.

Quoted 6 lines

Now if we postpone initialization of the IOMMU masters and swizzling of the DMA operations until driver probe time we get rid of a lot of problems. For example we could use deferred probing if the IOMMU driver hasn't loaded yet. That in turn would allow IOMMU drivers to be built as modules rather than built-in. And especially with multi-platform kernels I think we really want to build as much as possible as modules.

For what it's worth, given that I have no code to show, I was about to try implementing that when Will sent his patch set. My idea was to use a bus notifier in the IOMMU core to defer probing of devices for which the IOMMU isn't available yet based on the DT iommus property.

Hi all,

Here is version three of the RFC I've previously posted here:

   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html

Changes since RFCv2 include:

- Put the iommu_ops in iommu_data so of_iommu_configure can avoid using the bus_type
- Initialise the offset and DMA masks on the dev in of_dma_configure instead of in the arch callback (as this would cause a regression on some architectures)
- Added deconfigure/teardown code based on ref counting the iommu_dma_mapping
- A bunch of small fixes (_OF_DECLARE, some code shuffling, fix multiple IOMMU parsing)

All feedback welcome. Hopefully this is now at a point where people can start looking to port dma-mapping and/or IOMMU drivers to it.

Cheers,

Will

--->8

Will Deacon (7):
iommu: provide early initialisation hook for IOMMU drivers
dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
iommu: add new iommu_ops callback for adding an OF device
iommu: provide helper function to configure an IOMMU for an of master
dma-mapping: detect and configure IOMMU in of_dma_configure
arm: call iommu_init before of_platform_populate
arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

  arch/arm/include/asm/dma-mapping.h |  9 ++--
  arch/arm/kernel/setup.c            |  2 +
  arch/arm/mm/dma-mapping.c          | 68 ++++++++++++++++++++++++++----
  drivers/iommu/Kconfig              |  2 +-
  drivers/iommu/of_iommu.c           | 84 ++++++++++++++++++++++++++++++++++++++
  drivers/of/platform.c              | 53 +++++++++++++-----------
  include/asm-generic/vmlinux.lds.h  |  2 +
  include/linux/dma-mapping.h        | 21 +++++++---
  include/linux/iommu.h              | 15 +++++++
  include/linux/of_iommu.h           | 35 ++++++++++++++++
  10 files changed, 247 insertions(+), 44 deletions(-)

Quoted 16 lines · Will Deacon

Here is version three of the RFC I've previously posted here:

   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html

Changes since RFCv2 include:

- Put the iommu_ops in iommu_data so of_iommu_configure can avoid using the bus_type
- Initialise the offset and DMA masks on the dev in of_dma_configure instead of in the arch callback (as this would cause a regression on some architectures)
- Added deconfigure/teardown code based on ref counting the iommu_dma_mapping
- A bunch of small fixes (_OF_DECLARE, some code shuffling, fix multiple IOMMU parsing)

Quoted 3 lines

All feedback welcome. Hopefully this is now at a point where people can start looking to port dma-mapping and/or IOMMU drivers to it.

What about AMBA devices? Playing with this on Juno and wondering why my PL330 doesn't get any of_xlate callbacks, I see that of_amba_device_create doesn't call of_dma_configure or anything from that path. It's easy to work around by removing the arm,primecell compatible, but that feels pretty dirty.