[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

Hi all,

This patch series is an RFC to implement IOMMU master configuration into
of_dma_configure. I haven't yet ported any IOMMU drivers to it, so it
remains untested, but I wanted to get some early feedback to ensure that
this doesn't end up going in the wrong direction.

The idea comes out of my understanding following discussions with Arnd
and David at Kernel Summit / LinuxCon in Chicago. Essentially:

  - Allow IOMMUs to be probed early and set up per-instance data on their
    of_node

  - Add a new callback to the iommu_ops structure for adding a device
    with a set of opaque IDs (e.g. Stream IDs or Requester IDs)

  - Add an of_iommu_configure function, called from of_dma_configure,
    which registers the master IDs with the correspond IOMMU before
    probing the master itself

  - Where applicable, create an IOMMU domain per device and swizzle the
    DMA ops for that device to use the IOMMU variants.

I haven't bothered doing anything clever with the DMA masks yet, so I
wouldn't be surprised if we end up chewing through tonnes of memory
allocating IOMMU domains that are far too big at the moment. Again, this
is just an RFC and I'd welcome comments on the general direction of the
series.

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 a device with a set of
    IDs
  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 | 10 ++---
 arch/arm/kernel/setup.c            |  5 ++-
 arch/arm/mm/dma-mapping.c          | 77 ++++++++++++++++++++++++++++++++++----
 drivers/iommu/Kconfig              |  2 +-
 drivers/iommu/of_iommu.c           | 50 +++++++++++++++++++++++++
 drivers/of/platform.c              | 53 ++++++++++----------------
 include/asm-generic/vmlinux.lds.h  |  2 +
 include/linux/dma-mapping.h        | 10 ++---
 include/linux/iommu.h              |  2 +
 include/linux/of_iommu.h           | 27 +++++++++++++
 10 files changed, 184 insertions(+), 54 deletions(-)

-- 
2.1.0

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Will Deacon]

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. This data can then be
passed back to a new add_device function (added in a later patch) to
identify the IOMMU instance in question.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/of_iommu.c          | 14 ++++++++++++++
 include/asm-generic/vmlinux.lds.h |  2 ++
 include/linux/of_iommu.h          | 21 +++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..f9209666157c 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -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));
+	}
+}
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 5ba0360663a7..b75ede8f99ae 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -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()					\
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..29f2f3f88d6a 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,16 @@
 #ifndef __OF_IOMMU_H
 #define __OF_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 +20,23 @@ 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, void *data)
+{
+	np->data = data;
+}
+
+static inline void *of_iommu_get_data(struct device_node *np)
+{
+	return np->data;
+}
+
+extern struct of_device_id __iommu_of_table;
+
+#define IOMMU_OF_DECLARE(name, compat, fn) \
+	OF_DECLARE_1(iommu, name, compat, fn)
+
 #endif /* __OF_IOMMU_H */
-- 
2.1.0

[RFC PATCH 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[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.

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 <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h | 20 ++++++++++++++----
 drivers/of/platform.c              | 42 ++++++++++----------------------------
 include/linux/dma-mapping.h        |  8 +++-----
 3 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a4b4a5..936125ef3f3f 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,12 +121,24 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline int set_arch_dma_coherent_ops(struct device *dev)
+static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
+				      unsigned long offset, bool coherent)
 {
-	set_dma_ops(dev, &arm_coherent_dma_ops);
-	return 0;
+	dev->coherent_dma_mask = mask;
+
+	/*
+	 * Set dma_mask to coherent_dma_mask by default if the architecture
+	 * code has not set it.
+	 */
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+
+	dev->dma_pfn_offset = offset;
+
+	if (coherent)
+		set_dma_ops(dev, &arm_coherent_dma_ops);
 }
-#define set_arch_dma_coherent_ops(dev)	set_arch_dma_coherent_ops(dev)
+#define arch_setup_dma_ops arch_setup_dma_ops
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0197725e033a..484c558c63a6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -164,43 +164,23 @@ static void of_dma_configure(struct platform_device *pdev)
 {
 	u64 dma_addr, paddr, size;
 	int ret;
+	bool coherent;
+	unsigned long offset;
 	struct device *dev = &pdev->dev;
 
-	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
-	 */
-	dev->coherent_dma_mask = DMA_BIT_MASK(32);
-
-	/*
-	 * Set it to coherent_dma_mask by default if the architecture
-	 * code has not set it.
-	 */
-	if (!dev->dma_mask)
-		dev->dma_mask = &dev->coherent_dma_mask;
+	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+	offset = ret < 0 ? 0 : PFN_DOWN(paddr - dma_addr);
+	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
 
-	/*
-	 * if dma-coherent property exist, call arch hook to setup
-	 * dma coherent operations.
-	 */
-	if (of_dma_is_coherent(dev->of_node)) {
-		set_arch_dma_coherent_ops(dev);
-		dev_dbg(dev, "device is dma coherent\n");
-	}
+	coherent = of_dma_is_coherent(dev->of_node);
+	dev_dbg(dev, "device is%sdma coherent\n",
+		coherent ? " " : " not ");
 
 	/*
-	 * if dma-ranges property doesn't exist - just return else
-	 * setup the dma offset
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
 	 */
-	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
-	if (ret < 0) {
-		dev_dbg(dev, "no dma range information to setup\n");
-		return;
-	}
-
-	/* DMA ranges found. Calculate and set dma_pfn_offset */
-	dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
-	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
 }
 
 /**
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 931b70986272..0f7f7b68b0db 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -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
-static inline int set_arch_dma_coherent_ops(struct device *dev)
-{
-	return 0;
-}
+#ifndef arch_setup_dma_ops
+static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
+				      unsigned long offset, bool coherent) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.0

[RFC PATCH 3/7] iommu: add new iommu_ops callback for adding a device with a set of IDs

[Will Deacon]

This patch adds a new function to the iommu_ops structure to allow a
device to be added to a specific IOMMU instance along with a set of
opaque IDs that are used internally by the IOMMU for identifying and
configuring translations.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/linux/iommu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a527922a..3dd1b99c4542 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -114,6 +114,8 @@ struct iommu_ops {
 	int (*domain_has_cap)(struct iommu_domain *domain,
 			      unsigned long cap);
 	int (*add_device)(struct device *dev);
+	int (*add_device_master_ids)(struct device *dev, int count, u32 *ids,
+				     void *data);
 	void (*remove_device)(struct device *dev);
 	int (*device_group)(struct device *dev, unsigned int *groupid);
 	int (*domain_get_attr)(struct iommu_domain *domain,
-- 
2.1.0

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

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 <will.deacon@arm.com>
---
 drivers/iommu/Kconfig    |  2 +-
 drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |  6 ++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index dd5112265cc9..6d13f962f156 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -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"
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index f9209666157c..a7d3b3a13b83 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -19,6 +19,7 @@
 
 #include <linux/export.h>
 #include <linux/limits.h>
+#include <linux/iommu.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 
@@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+int of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct bus_type *bus = dev->bus;
+	const struct iommu_ops *ops = bus->iommu_ops;
+	int ret = -EINVAL, idx = 0;
+
+	if (!iommu_present(bus))
+		return -ENODEV;
+
+	/*
+	 * 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)) {
+		void *data = of_iommu_get_data(iommu_spec.np);
+
+		of_node_put(iommu_spec.np);
+		if (!ops->add_device_master_ids)
+			return -ENODEV;
+
+		ret = ops->add_device_master_ids(dev, iommu_spec.args_count,
+						 iommu_spec.args, data);
+		if (ret)
+			break;
+
+		idx++;
+	}
+
+	return ret;
+}
+
 void __init of_iommu_init(void)
 {
 	struct device_node *np;
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 29f2f3f88d6a..85c6d1152624 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,6 +1,7 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/device.h>
 #include <linux/of.h>
 
 #ifdef CONFIG_OF_IOMMU
@@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     size_t *size);
 
 extern void of_iommu_init(void);
+extern int of_iommu_configure(struct device *dev);
 
 #else
 
@@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 }
 
 static inline void of_iommu_init(void) { }
+static inline int of_iommu_configure(struct device *dev)
+{
+	return -ENODEV;
+}
 
 #endif	/* CONFIG_OF_IOMMU */
 
-- 
2.1.0

[RFC PATCH 5/7] dma-mapping: detect and configure IOMMU in of_dma_configure

[Will Deacon]

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 <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 37 ++++++++++++++++++++++---------------
 include/linux/dma-mapping.h        |  4 +++-
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 936125ef3f3f..48adace87d6d 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -122,7 +122,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, u64 mask,
-				      unsigned long offset, bool coherent)
+				      u64 dma_base, u64 size,
+				      unsigned long offset, bool coherent,
+				      bool iommu)
 {
 	dev->coherent_dma_mask = mask;
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 484c558c63a6..826beffdc50d 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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>
@@ -162,25 +163,36 @@ EXPORT_SYMBOL(of_device_alloc);
  */
 static void of_dma_configure(struct platform_device *pdev)
 {
-	u64 dma_addr, paddr, size;
+	u64 dma_addr, paddr, size, mask;
 	int ret;
-	bool coherent;
+	bool coherent, iommu;
 	unsigned long offset;
 	struct device *dev = &pdev->dev;
 
+	/*
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
+	 */
+	mask = DMA_BIT_MASK(32);
+
 	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
-	offset = ret < 0 ? 0 : PFN_DOWN(paddr - dma_addr);
-	dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	if (ret < 0) {
+		dma_addr = offset = 0;
+		size = mask;
+	} else {
+		offset = PFN_DOWN(paddr - dma_addr);
+		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	}
 
 	coherent = of_dma_is_coherent(dev->of_node);
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
-	 */
-	arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
+	iommu = !of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, mask, dma_addr, size, offset, coherent, iommu);
 }
 
 /**
@@ -209,14 +221,9 @@ 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) {
 		platform_device_put(dev);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0f7f7b68b0db..348ab66c7e0c 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -131,7 +131,9 @@ extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
 static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
-				      unsigned long offset, bool coherent) { }
+				      u64 dma_base, u64 size,
+				      unsigned long offset, bool coherent,
+				      bool iommu) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.0

[RFC PATCH 6/7] arm: call iommu_init before of_platform_populate

[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.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/setup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 84db893dedc2..38a0203523af 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -18,6 +18,7 @@
 #include <linux/bootmem.h>
 #include <linux/seq_file.h>
 #include <linux/screen_info.h>
+#include <linux/of_iommu.h>
 #include <linux/of_platform.h>
 #include <linux/init.h>
 #include <linux/kexec.h>
@@ -803,9 +804,11 @@ static int __init customize_machine(void)
 	if (machine_desc->init_machine)
 		machine_desc->init_machine();
 #ifdef CONFIG_OF
-	else
+	else {
+		of_iommu_init();
 		of_platform_populate(NULL, of_default_bus_match_table,
 					NULL, NULL);
+	}
 #endif
 	return 0;
 }
-- 
2.1.0

[RFC PATCH 7/7] arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

[Will Deacon]

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 <will.deacon@arm.com>
---
 arch/arm/include/asm/dma-mapping.h | 22 ++---------
 arch/arm/mm/dma-mapping.c          | 77 ++++++++++++++++++++++++++++++++++----
 2 files changed, 72 insertions(+), 27 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 48adace87d6d..621168f0eb20 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,26 +121,10 @@ 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, u64 mask,
-				      u64 dma_base, u64 size,
-				      unsigned long offset, bool coherent,
-				      bool iommu)
-{
-	dev->coherent_dma_mask = mask;
-
-	/*
-	 * Set dma_mask to coherent_dma_mask by default if the architecture
-	 * code has not set it.
-	 */
-	if (!dev->dma_mask)
-		dev->dma_mask = &dev->coherent_dma_mask;
-
-	dev->dma_pfn_offset = offset;
-
-	if (coherent)
-		set_dma_ops(dev, &arm_coherent_dma_ops);
-}
 #define arch_setup_dma_ops arch_setup_dma_ops
+extern void arch_setup_dma_ops(struct device *dev, u64 mask, u64 dma_base,
+			       u64 size, unsigned long offset, bool coherent,
+			       bool iommu);
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7a996aaa061e..b5ee6f2dcfb7 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2041,10 +2041,9 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  * @mapping: io address space mapping structure (returned from
  *	arm_iommu_create_mapping)
  *
- * Attaches specified io address space mapping to the provided device,
- * this replaces the dma operations (dma_map_ops pointer) with the
- * IOMMU aware version. More than one client might be attached to
- * the same io address space mapping.
+ * Attaches specified io address space mapping to the provided device.
+ * More than one client might be attached to the same io address space
+ * mapping.
  */
 int arm_iommu_attach_device(struct device *dev,
 			    struct dma_iommu_mapping *mapping)
@@ -2057,7 +2056,6 @@ int arm_iommu_attach_device(struct device *dev,
 
 	kref_get(&mapping->kref);
 	dev->archdata.mapping = mapping;
-	set_dma_ops(dev, &iommu_ops);
 
 	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
 	return 0;
@@ -2069,7 +2067,6 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
  * @dev: valid struct device pointer
  *
  * Detaches the provided device from a previously attached map.
- * This voids the dma operations (dma_map_ops pointer)
  */
 void arm_iommu_detach_device(struct device *dev)
 {
@@ -2084,10 +2081,74 @@ void arm_iommu_detach_device(struct device *dev)
 	iommu_detach_device(mapping->domain, dev);
 	kref_put(&mapping->kref, release_iommu_mapping);
 	dev->archdata.mapping = NULL;
-	set_dma_ops(dev, NULL);
 
 	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
 }
 EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
 
-#endif
+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 (IS_ERR(mapping)) {
+		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
+				size, dev_name(dev));
+		return false;
+	}
+
+	if (arm_iommu_attach_device(dev, mapping)) {
+		pr_warn("Failed to attach device %s to IOMMU mapping\n",
+				dev_name(dev));
+		arm_iommu_release_mapping(mapping);
+		return false;
+	}
+
+	return true;
+}
+
+static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
+{
+	return coherent ? &iommu_coherent_ops : &iommu_ops;
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
+{
+	return false;
+}
+
+#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
+
+#endif /* CONFIG_ARM_DMA_USE_IOMMU */
+
+static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
+{
+	return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
+}
+
+void arch_setup_dma_ops(struct device *dev, u64 mask, u64 dma_base, u64 size,
+			unsigned long offset, bool coherent, bool iommu)
+{
+	struct dma_map_ops *dma_ops;
+
+	dev->coherent_dma_mask = mask;
+
+	/*
+	 * Set dma_mask to coherent_dma_mask by default if the architecture
+	 * code has not set it.
+	 */
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+
+	if (iommu && arm_setup_iommu_dma_ops(dev, dma_base, size)) {
+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	} else {
+		dev->dma_pfn_offset = offset;
+		dma_ops = arm_get_dma_map_ops(coherent);
+	}
+
+	set_dma_ops(dev, dma_ops);
+}
+EXPORT_SYMBOL_GPL(arch_setup_dma_ops);
-- 
2.1.0

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Thierry Reding]

On Fri, Aug 29, 2014 at 04:54:24PM +0100, Will Deacon wrote:
> 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. This data can then be
> passed back to a new add_device function (added in a later patch) to
> identify the IOMMU instance in question.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/of_iommu.c          | 14 ++++++++++++++
>  include/asm-generic/vmlinux.lds.h |  2 ++
>  include/linux/of_iommu.h          | 21 +++++++++++++++++++++
>  3 files changed, 37 insertions(+)

I don't think this is the right direction. We've been preaching that
using initcall ordering is a bad thing and people should be using
deferred probe instead. Now we have a bunch of these OF tables that do
the exact opposite. Note that these are nothing more than a variant of
initcalls that get called at platform-specific points in time.

There are also ongoing discussions about how to change the device probe
order to use dependencies (such as phandles from device tree) to prevent
excessive deferred probing. With that in place this would be solved in a
much more generic way.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140901/ce639ee4/attachment-0001.sig>

[RFC PATCH 3/7] iommu: add new iommu_ops callback for adding a device with a set of IDs

[Thierry Reding]

On Fri, Aug 29, 2014 at 04:54:26PM +0100, Will Deacon wrote:
> This patch adds a new function to the iommu_ops structure to allow a
> device to be added to a specific IOMMU instance along with a set of
> opaque IDs that are used internally by the IOMMU for identifying and
> configuring translations.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  include/linux/iommu.h | 2 ++
>  1 file changed, 2 insertions(+)

I don't really see the point of this new callback. As I understand it,
the strength of the current .add_device() callback is that it uses only
the struct device and figures out which exact IOMMU to associate it with
in case there are multiple IOMMUs.

Although that doesn't seem to be the general case either. That's really
been one of the difficulties in dealing with IOMMU. Every driver seems
to do things very differently and the IOMMU subsystem itself is fairly
different from other subsystems too.

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a527922a..3dd1b99c4542 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -114,6 +114,8 @@ struct iommu_ops {
>  	int (*domain_has_cap)(struct iommu_domain *domain,
>  			      unsigned long cap);
>  	int (*add_device)(struct device *dev);
> +	int (*add_device_master_ids)(struct device *dev, int count, u32 *ids,
> +				     void *data);

If we want to pass around IOMMU instances I think we should make them
proper objects rather than some loosely specified void *.

Also the generic IOMMU binding doesn't require the IOMMU specifier to
contain master IDs. So this seems to be a callback that would only be
used by a restricted set of IOMMUs.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140901/93dc1a1e/attachment.sig>

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Thierry Reding]

On Fri, Aug 29, 2014 at 04:54:27PM +0100, Will Deacon wrote:
> 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 <will.deacon@arm.com>
> ---
>  drivers/iommu/Kconfig    |  2 +-
>  drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h |  6 ++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index dd5112265cc9..6d13f962f156 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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"
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index f9209666157c..a7d3b3a13b83 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -19,6 +19,7 @@
>  
>  #include <linux/export.h>
>  #include <linux/limits.h>
> +#include <linux/iommu.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
>  
> @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>  }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
>  
> +int of_iommu_configure(struct device *dev)
> +{
> +	struct of_phandle_args iommu_spec;
> +	struct bus_type *bus = dev->bus;
> +	const struct iommu_ops *ops = bus->iommu_ops;
> +	int ret = -EINVAL, idx = 0;
> +
> +	if (!iommu_present(bus))
> +		return -ENODEV;
> +
> +	/*
> +	 * 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)) {
> +		void *data = of_iommu_get_data(iommu_spec.np);
> +
> +		of_node_put(iommu_spec.np);
> +		if (!ops->add_device_master_ids)
> +			return -ENODEV;
> +
> +		ret = ops->add_device_master_ids(dev, iommu_spec.args_count,
> +						 iommu_spec.args, data);
> +		if (ret)
> +			break;

I think this could use a bit more formalization. As I said in another
reply earlier, there's very little standardization in the IOMMU API.
That certainly gives us a lot of flexibility but it also has the
downside that it's difficult to handle these abstractions in the core,
which is really what the core is all about, isn't it?

One method that worked really well for this in the past for other
subsystems is to allow drivers to specify an .of_xlate() function that
takes the controller device and a struct of_phandle_args. It is that
function's responsibility to take the information in an of_phandle_args
structure and use that to create some subsystem specific handle that
represents this information in a way that it can readily be used.

So I think it would really be helpful if IOMMU gained support for
something similar. We could create a struct iommu to represent an
instance of an IOMMU. IOMMU drivers can embed this structure and add
device-specific fields that they need. That way we can easily pass
around instances and upcast in the driver in a type-safe way.

At the same time, a struct iommu_master could provide the basis to
represent a single master interface on an IOMMU. Drivers can again embed
that in driver-specific structures with additional fields required for
the particular IOMMU implementation. .of_xlate() could return such an
IOMMU master for the core to use.

With such structures in place we should be able to eliminate many of the
loops in IOMMU drivers that serve no other purpose than to find the
master context from a struct device * and some parameters. It will also
allow us to keep a central registry of IOMMUs and masters rather than
duplicating that in every driver.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140901/5f29f373/attachment.sig>

[RFC PATCH 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Arnd Bergmann]

On Friday 29 August 2014 16:54:25 Will Deacon wrote:
> 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 <will.deacon@arm.com>

Looks good, just one tiny comment

> ---
>  arch/arm/include/asm/dma-mapping.h | 20 ++++++++++++++----
>  drivers/of/platform.c              | 42 ++++++++++----------------------------
>  include/linux/dma-mapping.h        |  8 +++-----
>  3 files changed, 30 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> index c45b61a4b4a5..936125ef3f3f 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -121,12 +121,24 @@ static inline unsigned long dma_max_pfn(struct device *dev)
>  }
>  #define dma_max_pfn(dev) dma_max_pfn(dev)
>  
> -static inline int set_arch_dma_coherent_ops(struct device *dev)
> +static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
> +                                     unsigned long offset, bool coherent)
>  {
> -       set_dma_ops(dev, &arm_coherent_dma_ops);
> -       return 0;
> +       dev->coherent_dma_mask = mask;
> +
> +       /*
> +        * Set dma_mask to coherent_dma_mask by default if the architecture
> +        * code has not set it.
> +        */
> +       if (!dev->dma_mask)
> +               dev->dma_mask = &dev->coherent_dma_mask;
> +

We are in architecture specific code here, and we know that this
architecture has not set up the dev->dma_mask pointer, so we can
remove the 'if (!dev->dma_mask)'

	Arnd

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Arnd Bergmann]

On Monday 01 September 2014 09:52:19 Thierry Reding wrote:
> I don't think this is the right direction. We've been preaching that
> using initcall ordering is a bad thing and people should be using
> deferred probe instead. Now we have a bunch of these OF tables that do
> the exact opposite. Note that these are nothing more than a variant of
> initcalls that get called at platform-specific points in time.
> 
> There are also ongoing discussions about how to change the device probe
> order to use dependencies (such as phandles from device tree) to prevent
> excessive deferred probing. With that in place this would be solved in a
> much more generic way.

We are not creating an ABI here, so it can always be changed later.
For now, I think iommus are clearly in the same category as irqchips,
timers, clock controllers and smp operations: we really want them
to be available before we set up any other devices.

I don't mind finding a more generic solution in the long run, but for
now, this gets the problem out of the way, and I can't think of any
realistic corner cases that would prevent it from working. Do you
think that an IOMMU driver may have a dependency on another device
driver that is probed later? Do you have an example of such hardware?

	Arnd

[RFC PATCH 3/7] iommu: add new iommu_ops callback for adding a device with a set of IDs

[Arnd Bergmann]

On Monday 01 September 2014 10:13:22 Thierry Reding wrote:
> On Fri, Aug 29, 2014 at 04:54:26PM +0100, Will Deacon wrote:
> > This patch adds a new function to the iommu_ops structure to allow a
> > device to be added to a specific IOMMU instance along with a set of
> > opaque IDs that are used internally by the IOMMU for identifying and
> > configuring translations.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  include/linux/iommu.h | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> I don't really see the point of this new callback. As I understand it,
> the strength of the current .add_device() callback is that it uses only
> the struct device and figures out which exact IOMMU to associate it with
> in case there are multiple IOMMUs.
> 
> Although that doesn't seem to be the general case either. That's really
> been one of the difficulties in dealing with IOMMU. Every driver seems
> to do things very differently and the IOMMU subsystem itself is fairly
> different from other subsystems too.

The problem with add_device is that it assumes that each bus_type only
has one IOMMU instance, and that we know which one that is. Current
implementations do more or less nasty hacks to work around this where
necessary, and we should try to remove those hacks rather than adding
more of them into future drivers.

A smaller issue with the add_device callback is the point at which it
gets called: the caller is in the bus_notifier when the device gets
added, but at a time when we don't have access to the bus-specific
information.

> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 20f9a527922a..3dd1b99c4542 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -114,6 +114,8 @@ struct iommu_ops {
> >       int (*domain_has_cap)(struct iommu_domain *domain,
> >                             unsigned long cap);
> >       int (*add_device)(struct device *dev);
> > +     int (*add_device_master_ids)(struct device *dev, int count, u32 *ids,
> > +                                  void *data);
> 
> If we want to pass around IOMMU instances I think we should make them
> proper objects rather than some loosely specified void *.

Agreed.

> Also the generic IOMMU binding doesn't require the IOMMU specifier to
> contain master IDs. So this seems to be a callback that would only be
> used by a restricted set of IOMMUs.

No, it should be used by all of them, but we might be passing empty
arguments.

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> 
> I think this could use a bit more formalization. As I said in another
> reply earlier, there's very little standardization in the IOMMU API.
> That certainly gives us a lot of flexibility but it also has the
> downside that it's difficult to handle these abstractions in the core,
> which is really what the core is all about, isn't it?
> 
> One method that worked really well for this in the past for other
> subsystems is to allow drivers to specify an .of_xlate() function that
> takes the controller device and a struct of_phandle_args. It is that
> function's responsibility to take the information in an of_phandle_args
> structure and use that to create some subsystem specific handle that
> represents this information in a way that it can readily be used.

Yes, good idea.

> So I think it would really be helpful if IOMMU gained support for
> something similar. We could create a struct iommu to represent an
> instance of an IOMMU. IOMMU drivers can embed this structure and add
> device-specific fields that they need. That way we can easily pass
> around instances and upcast in the driver in a type-safe way.

Right.

> At the same time, a struct iommu_master could provide the basis to
> represent a single master interface on an IOMMU. Drivers can again embed
> that in driver-specific structures with additional fields required for
> the particular IOMMU implementation. .of_xlate() could return such an
> IOMMU master for the core to use.

I'm not convinced it's necessary. Could this just be a 'struct device'
instead of 'struct iommu_master'?

> With such structures in place we should be able to eliminate many of the
> loops in IOMMU drivers that serve no other purpose than to find the
> master context from a struct device * and some parameters. It will also
> allow us to keep a central registry of IOMMUs and masters rather than
> duplicating that in every driver.

Yes, we should be able to identify an iommu context in a generic way,
but why do you want to break it down to individual masters within
one context?

	Arnd

[RFC PATCH 5/7] dma-mapping: detect and configure IOMMU in of_dma_configure

[Arnd Bergmann]

On Friday 29 August 2014 16:54:28 Will Deacon wrote:
>  static void of_dma_configure(struct platform_device *pdev)
>  {
> -       u64 dma_addr, paddr, size;
> +       u64 dma_addr, paddr, size, mask;
>         int ret;
> -       bool coherent;
> +       bool coherent, iommu;
>         unsigned long offset;
>         struct device *dev = &pdev->dev;
>  
> +       /*
> +        * Set default dma-mask to 32 bit. Drivers are expected to setup
> +        * the correct supported dma_mask.
> +        */
> +       mask = DMA_BIT_MASK(32);
> +
>         ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
> -       offset = ret < 0 ? 0 : PFN_DOWN(paddr - dma_addr);
> -       dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
> +       if (ret < 0) {
> +               dma_addr = offset = 0;
> +               size = mask;
> +       } else {
> +               offset = PFN_DOWN(paddr - dma_addr);
> +               dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
> +       }
>  
>         coherent = of_dma_is_coherent(dev->of_node);
>         dev_dbg(dev, "device is%sdma coherent\n",
>                 coherent ? " " : " not ");
>  
> -       /*
> -        * Set default dma-mask to 32 bit. Drivers are expected to setup
> -        * the correct supported dma_mask.
> -        */
> -       arch_setup_dma_ops(dev, DMA_BIT_MASK(32), offset, coherent);
> +       iommu = !of_iommu_configure(dev);
> +       dev_dbg(dev, "device is%sbehind an iommu\n",
> +               iommu ? " " : " not ");
> +
> +       arch_setup_dma_ops(dev, mask, dma_addr, size, offset, coherent, iommu);
>  }

Hmm, I was actually expecting of_iommu_configure() to be called from inside
arch_setup_dma_ops(), leaving the choice for how it's set up to the
architecture for now. It's probably not a big difference either way.

	Arnd

[RFC PATCH 2/7] dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops

[Will Deacon]

On Mon, Sep 01, 2014 at 03:27:30PM +0100, Arnd Bergmann wrote:
> On Friday 29 August 2014 16:54:25 Will Deacon wrote:
> > 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 <will.deacon@arm.com>
> 
> Looks good, just one tiny comment
> 
> > ---
> >  arch/arm/include/asm/dma-mapping.h | 20 ++++++++++++++----
> >  drivers/of/platform.c              | 42 ++++++++++----------------------------
> >  include/linux/dma-mapping.h        |  8 +++-----
> >  3 files changed, 30 insertions(+), 40 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> > index c45b61a4b4a5..936125ef3f3f 100644
> > --- a/arch/arm/include/asm/dma-mapping.h
> > +++ b/arch/arm/include/asm/dma-mapping.h
> > @@ -121,12 +121,24 @@ static inline unsigned long dma_max_pfn(struct device *dev)
> >  }
> >  #define dma_max_pfn(dev) dma_max_pfn(dev)
> >  
> > -static inline int set_arch_dma_coherent_ops(struct device *dev)
> > +static inline void arch_setup_dma_ops(struct device *dev, u64 mask,
> > +                                     unsigned long offset, bool coherent)
> >  {
> > -       set_dma_ops(dev, &arm_coherent_dma_ops);
> > -       return 0;
> > +       dev->coherent_dma_mask = mask;
> > +
> > +       /*
> > +        * Set dma_mask to coherent_dma_mask by default if the architecture
> > +        * code has not set it.
> > +        */
> > +       if (!dev->dma_mask)
> > +               dev->dma_mask = &dev->coherent_dma_mask;
> > +
> 
> We are in architecture specific code here, and we know that this
> architecture has not set up the dev->dma_mask pointer, so we can
> remove the 'if (!dev->dma_mask)'

Good point; I'll fix it.

Thanks,

Will

[RFC PATCH 3/7] iommu: add new iommu_ops callback for adding a device with a set of IDs

[Will Deacon]

On Mon, Sep 01, 2014 at 03:39:16PM +0100, Arnd Bergmann wrote:
> On Monday 01 September 2014 10:13:22 Thierry Reding wrote:
> > On Fri, Aug 29, 2014 at 04:54:26PM +0100, Will Deacon wrote:
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 20f9a527922a..3dd1b99c4542 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -114,6 +114,8 @@ struct iommu_ops {
> > >       int (*domain_has_cap)(struct iommu_domain *domain,
> > >                             unsigned long cap);
> > >       int (*add_device)(struct device *dev);
> > > +     int (*add_device_master_ids)(struct device *dev, int count, u32 *ids,
> > > +                                  void *data);
> > 
> > If we want to pass around IOMMU instances I think we should make them
> > proper objects rather than some loosely specified void *.
> 
> Agreed.

For OF, this data argument is the data field of the device_node for the
IOMMU. That's private to the corresponding IOMMU driver and I don't see
what we gain by making that a generic structure. It's likely going to
represent some internal driver data structures anyway, so that the IDs can
be recorded in the relevant place and for the relevant group etc.

In other words, I have no idea what a generic data structure would look
like for this.

> > Also the generic IOMMU binding doesn't require the IOMMU specifier to
> > contain master IDs. So this seems to be a callback that would only be
> > used by a restricted set of IOMMUs.
> 
> No, it should be used by all of them, but we might be passing empty
> arguments.

Yup; I'd hope to remove/replace the add_device callback in the future,
but that's a lot of work right now.

Will

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Will Deacon]

On Mon, Sep 01, 2014 at 03:31:28PM +0100, Arnd Bergmann wrote:
> On Monday 01 September 2014 09:52:19 Thierry Reding wrote:
> > I don't think this is the right direction. We've been preaching that
> > using initcall ordering is a bad thing and people should be using
> > deferred probe instead. Now we have a bunch of these OF tables that do
> > the exact opposite. Note that these are nothing more than a variant of
> > initcalls that get called at platform-specific points in time.
> > 
> > There are also ongoing discussions about how to change the device probe
> > order to use dependencies (such as phandles from device tree) to prevent
> > excessive deferred probing. With that in place this would be solved in a
> > much more generic way.
> 
> We are not creating an ABI here, so it can always be changed later.
> For now, I think iommus are clearly in the same category as irqchips,
> timers, clock controllers and smp operations: we really want them
> to be available before we set up any other devices.

I'm also trying to move the SMMU driver to the generic bindings before the
existing bindings start getting used. The sooner we have parsing of the
generic binding in the core code, the sooner that can happen.

Will

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote:
> On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > 
> > I think this could use a bit more formalization. As I said in another
> > reply earlier, there's very little standardization in the IOMMU API.
> > That certainly gives us a lot of flexibility but it also has the
> > downside that it's difficult to handle these abstractions in the core,
> > which is really what the core is all about, isn't it?
> > 
> > One method that worked really well for this in the past for other
> > subsystems is to allow drivers to specify an .of_xlate() function that
> > takes the controller device and a struct of_phandle_args. It is that
> > function's responsibility to take the information in an of_phandle_args
> > structure and use that to create some subsystem specific handle that
> > represents this information in a way that it can readily be used.
> 
> Yes, good idea.

Hmm, how does this work for PCI devices? The current RFC takes care to
ensure that the core changes work just as well for OF devices as PCI
devices, and the of-specific functions and data structures are not part of
it.

Will

[RFC PATCH 3/7] iommu: add new iommu_ops callback for adding a device with a set of IDs

[Arnd Bergmann]

On Monday 01 September 2014 17:34:00 Will Deacon wrote:
> On Mon, Sep 01, 2014 at 03:39:16PM +0100, Arnd Bergmann wrote:
> > On Monday 01 September 2014 10:13:22 Thierry Reding wrote:
> > > On Fri, Aug 29, 2014 at 04:54:26PM +0100, Will Deacon wrote:
> > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > > index 20f9a527922a..3dd1b99c4542 100644
> > > > --- a/include/linux/iommu.h
> > > > +++ b/include/linux/iommu.h
> > > > @@ -114,6 +114,8 @@ struct iommu_ops {
> > > >       int (*domain_has_cap)(struct iommu_domain *domain,
> > > >                             unsigned long cap);
> > > >       int (*add_device)(struct device *dev);
> > > > +     int (*add_device_master_ids)(struct device *dev, int count, u32 *ids,
> > > > +                                  void *data);
> > > 
> > > If we want to pass around IOMMU instances I think we should make them
> > > proper objects rather than some loosely specified void *.
> > 
> > Agreed.
> 
> For OF, this data argument is the data field of the device_node for the
> IOMMU. That's private to the corresponding IOMMU driver and I don't see
> what we gain by making that a generic structure. It's likely going to
> represent some internal driver data structures anyway, so that the IDs can
> be recorded in the relevant place and for the relevant group etc.
> 
> In other words, I have no idea what a generic data structure would look
> like for this.

Something like

	struct iommu {
		struct device *dev;
		const struct iommu_ops *ops;
		struct list_head domains;
		void *private;
	};

There are probably a few more fields we will need in the long run.

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Monday 01 September 2014 17:40:00 Will Deacon wrote:
> On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote:
> > On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > > 
> > > I think this could use a bit more formalization. As I said in another
> > > reply earlier, there's very little standardization in the IOMMU API.
> > > That certainly gives us a lot of flexibility but it also has the
> > > downside that it's difficult to handle these abstractions in the core,
> > > which is really what the core is all about, isn't it?
> > > 
> > > One method that worked really well for this in the past for other
> > > subsystems is to allow drivers to specify an .of_xlate() function that
> > > takes the controller device and a struct of_phandle_args. It is that
> > > function's responsibility to take the information in an of_phandle_args
> > > structure and use that to create some subsystem specific handle that
> > > represents this information in a way that it can readily be used.
> > 
> > Yes, good idea.
> 
> Hmm, how does this work for PCI devices? The current RFC takes care to
> ensure that the core changes work just as well for OF devices as PCI
> devices, and the of-specific functions and data structures are not part of
> it.

I don't mind handling PCI devices separately. They are different in a number
of ways already, in particular the way that they don't normally have an
of_node attached to them but actually have a PCI bus/dev/function number.

	Arnd

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hi Will,

On 2014-08-29 17:54, Will Deacon wrote:
> This patch series is an RFC to implement IOMMU master configuration into
> of_dma_configure. I haven't yet ported any IOMMU drivers to it, so it
> remains untested, but I wanted to get some early feedback to ensure that
> this doesn't end up going in the wrong direction.
>
> The idea comes out of my understanding following discussions with Arnd
> and David at Kernel Summit / LinuxCon in Chicago. Essentially:
>
>    - Allow IOMMUs to be probed early and set up per-instance data on their
>      of_node
>
>    - Add a new callback to the iommu_ops structure for adding a device
>      with a set of opaque IDs (e.g. Stream IDs or Requester IDs)
>
>    - Add an of_iommu_configure function, called from of_dma_configure,
>      which registers the master IDs with the correspond IOMMU before
>      probing the master itself
>
>    - Where applicable, create an IOMMU domain per device and swizzle the
>      DMA ops for that device to use the IOMMU variants.
>
> I haven't bothered doing anything clever with the DMA masks yet, so I
> wouldn't be surprised if we end up chewing through tonnes of memory
> allocating IOMMU domains that are far too big at the moment. Again, this
> is just an RFC and I'd welcome comments on the general direction of the
> series.

Thanks for your patches, I wasn't aware the fact that you are working on
this. When do you plan to send a second version? I would like to rebase
my Exynos IOMMU patches (https://lkml.org/lkml/2014/8/5/183) on top of
your work, but I wonder if I should select this version as a base or wait
a bit for an update.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Laurent Pinchart]

On Monday 01 September 2014 09:52:19 Thierry Reding wrote:
> On Fri, Aug 29, 2014 at 04:54:24PM +0100, Will Deacon wrote:
> > 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. This data can then be
> > passed back to a new add_device function (added in a later patch) to
> > identify the IOMMU instance in question.
> > 
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > 
> >  drivers/iommu/of_iommu.c          | 14 ++++++++++++++
> >  include/asm-generic/vmlinux.lds.h |  2 ++
> >  include/linux/of_iommu.h          | 21 +++++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> 
> I don't think this is the right direction. We've been preaching that
> using initcall ordering is a bad thing and people should be using
> deferred probe instead. Now we have a bunch of these OF tables that do
> the exact opposite. Note that these are nothing more than a variant of
> initcalls that get called at platform-specific points in time.

My idea to solve this problem was to defer probing of the bus master device 
from the add_device IOMMU operation. This obviously won't work with add_device 
called from the BUS_NOTIFY_ADD_DEVICE notifier, which led me to naively wonder 
whether we couldn't call the add_device operation from the 
BUS_NOTIFY_BIND_DRIVER notifier instead.

> There are also ongoing discussions about how to change the device probe
> order to use dependencies (such as phandles from device tree) to prevent
> excessive deferred probing. With that in place this would be solved in a
> much more generic way.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140902/74614f63/attachment.sig>

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Tue, Sep 02, 2014 at 07:26:01AM +0100, Marek Szyprowski wrote:
> Hi Will,

Hi Marek,

> On 2014-08-29 17:54, Will Deacon wrote:
> > This patch series is an RFC to implement IOMMU master configuration into
> > of_dma_configure. I haven't yet ported any IOMMU drivers to it, so it
> > remains untested, but I wanted to get some early feedback to ensure that
> > this doesn't end up going in the wrong direction.
> >
> > The idea comes out of my understanding following discussions with Arnd
> > and David at Kernel Summit / LinuxCon in Chicago. Essentially:
> >
> >    - Allow IOMMUs to be probed early and set up per-instance data on their
> >      of_node
> >
> >    - Add a new callback to the iommu_ops structure for adding a device
> >      with a set of opaque IDs (e.g. Stream IDs or Requester IDs)
> >
> >    - Add an of_iommu_configure function, called from of_dma_configure,
> >      which registers the master IDs with the correspond IOMMU before
> >      probing the master itself
> >
> >    - Where applicable, create an IOMMU domain per device and swizzle the
> >      DMA ops for that device to use the IOMMU variants.
> >
> > I haven't bothered doing anything clever with the DMA masks yet, so I
> > wouldn't be surprised if we end up chewing through tonnes of memory
> > allocating IOMMU domains that are far too big at the moment. Again, this
> > is just an RFC and I'd welcome comments on the general direction of the
> > series.
> 
> Thanks for your patches, I wasn't aware the fact that you are working on
> this. When do you plan to send a second version? I would like to rebase
> my Exynos IOMMU patches (https://lkml.org/lkml/2014/8/5/183) on top of
> your work, but I wonder if I should select this version as a base or wait
> a bit for an update.

I'll try and get something out today/tomorrow depending on how easily the
review comments fall out. It would be really great if you get an IOMMU
working with this (I was going to look at the ARM SMMU once this stops
moving) -- I have concerns that allocating one domain per master might be
too much, but it's hard to tell without an IOMMU driver ported over.

I'll CC you on v2.

Cheers,

Will

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hi Will,

On 2014-09-02 10:31, Will Deacon wrote:
> On Tue, Sep 02, 2014 at 07:26:01AM +0100, Marek Szyprowski wrote:
>> On 2014-08-29 17:54, Will Deacon wrote:
>>> This patch series is an RFC to implement IOMMU master configuration into
>>> of_dma_configure. I haven't yet ported any IOMMU drivers to it, so it
>>> remains untested, but I wanted to get some early feedback to ensure that
>>> this doesn't end up going in the wrong direction.
>>>
>>> The idea comes out of my understanding following discussions with Arnd
>>> and David at Kernel Summit / LinuxCon in Chicago. Essentially:
>>>
>>>     - Allow IOMMUs to be probed early and set up per-instance data on their
>>>       of_node
>>>
>>>     - Add a new callback to the iommu_ops structure for adding a device
>>>       with a set of opaque IDs (e.g. Stream IDs or Requester IDs)
>>>
>>>     - Add an of_iommu_configure function, called from of_dma_configure,
>>>       which registers the master IDs with the correspond IOMMU before
>>>       probing the master itself
>>>
>>>     - Where applicable, create an IOMMU domain per device and swizzle the
>>>       DMA ops for that device to use the IOMMU variants.
>>>
>>> I haven't bothered doing anything clever with the DMA masks yet, so I
>>> wouldn't be surprised if we end up chewing through tonnes of memory
>>> allocating IOMMU domains that are far too big at the moment. Again, this
>>> is just an RFC and I'd welcome comments on the general direction of the
>>> series.
>> Thanks for your patches, I wasn't aware the fact that you are working on
>> this. When do you plan to send a second version? I would like to rebase
>> my Exynos IOMMU patches (https://lkml.org/lkml/2014/8/5/183) on top of
>> your work, but I wonder if I should select this version as a base or wait
>> a bit for an update.
> I'll try and get something out today/tomorrow depending on how easily the
> review comments fall out. It would be really great if you get an IOMMU
> working with this (I was going to look at the ARM SMMU once this stops
> moving)

Great, I will wait then for v2.

>   -- I have concerns that allocating one domain per master might be
> too much, but it's hard to tell without an IOMMU driver ported over.

One domain per master is IMHO a sane default configuration. The only default
alternative I see is to have only one domain (related with dma-mapping
subsystem) and bind all devices to it. However I really don't see any
disadvantage of having separate domain per each master and such 
configuration
gives devices better separation.

However we also need to figure out how to let drivers to make their own
configuration, like it is required by Exynos DRM subsystem, which consist
of several devices, each having its own IOMMU controller, but for
convenience those drivers assume that they all have been bound to the same,
single domain.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
> 
> >   -- I have concerns that allocating one domain per master might be
> > too much, but it's hard to tell without an IOMMU driver ported over.
> 
> One domain per master is IMHO a sane default configuration. The only default
> alternative I see is to have only one domain (related with dma-mapping
> subsystem) and bind all devices to it. However I really don't see any
> disadvantage of having separate domain per each master and such 
> configuration
> gives devices better separation.

I was expecting that the dma-mapping implementation would by default use
one domain for all devices, since that is what the simpler IOMMUs without
domain support have to do anyway.

For isolation purposes, it can only help to have more domains, but
I would guess that there is some space overhead in maintaining lots
of page tables.

> However we also need to figure out how to let drivers to make their own
> configuration, like it is required by Exynos DRM subsystem, which consist
> of several devices, each having its own IOMMU controller, but for
> convenience those drivers assume that they all have been bound to the same,
> single domain.

IIRC with the way we ended up putting the mask into the iommu descriptor of
the ARM SMMU, you can put multiple devices into the same iommu group, and
have them automatically share a domain.

I don't know if the same would work for the Samsung implementation.

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote:
> On Monday 01 September 2014 17:40:00 Will Deacon wrote:
> > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote:
> > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > > > 
> > > > I think this could use a bit more formalization. As I said in another
> > > > reply earlier, there's very little standardization in the IOMMU API.
> > > > That certainly gives us a lot of flexibility but it also has the
> > > > downside that it's difficult to handle these abstractions in the core,
> > > > which is really what the core is all about, isn't it?
> > > > 
> > > > One method that worked really well for this in the past for other
> > > > subsystems is to allow drivers to specify an .of_xlate() function that
> > > > takes the controller device and a struct of_phandle_args. It is that
> > > > function's responsibility to take the information in an of_phandle_args
> > > > structure and use that to create some subsystem specific handle that
> > > > represents this information in a way that it can readily be used.
> > > 
> > > Yes, good idea.
> > 
> > Hmm, how does this work for PCI devices? The current RFC takes care to
> > ensure that the core changes work just as well for OF devices as PCI
> > devices, and the of-specific functions and data structures are not part of
> > it.
> 
> I don't mind handling PCI devices separately. They are different in a number
> of ways already, in particular the way that they don't normally have an
> of_node attached to them but actually have a PCI bus/dev/function number.

Sure, but at the point when we call back into the iommu_ops structure we
really don't want bus specific functions. That's why I avoided any OF
data structures being passed to add_device_master_ids.

Anyway, I'll try to hack something together shortly. I think the proposal
is:

  - Make add_device_master_ids take a generic structure (struct iommu)
  - Add an of_xlate callback into iommu_ops which returns a populated
    struct iommu based on the of_node

Sound about right?

Will

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Laurent Pinchart]

On Monday 01 September 2014 16:46:18 Arnd Bergmann wrote:
> On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > I think this could use a bit more formalization. As I said in another
> > reply earlier, there's very little standardization in the IOMMU API.
> > That certainly gives us a lot of flexibility but it also has the
> > downside that it's difficult to handle these abstractions in the core,
> > which is really what the core is all about, isn't it?
> > 
> > One method that worked really well for this in the past for other
> > subsystems is to allow drivers to specify an .of_xlate() function that
> > takes the controller device and a struct of_phandle_args. It is that
> > function's responsibility to take the information in an of_phandle_args
> > structure and use that to create some subsystem specific handle that
> > represents this information in a way that it can readily be used.
> 
> Yes, good idea.
> 
> > So I think it would really be helpful if IOMMU gained support for
> > something similar. We could create a struct iommu to represent an
> > instance of an IOMMU. IOMMU drivers can embed this structure and add
> > device-specific fields that they need. That way we can easily pass
> > around instances and upcast in the driver in a type-safe way.
> 
> Right.
> 
> > At the same time, a struct iommu_master could provide the basis to
> > represent a single master interface on an IOMMU. Drivers can again embed
> > that in driver-specific structures with additional fields required for
> > the particular IOMMU implementation. .of_xlate() could return such an
> > IOMMU master for the core to use.
> 
> I'm not convinced it's necessary. Could this just be a 'struct device'
> instead of 'struct iommu_master'?

As master drivers should in general be unaware of the IOMMU that serves them, 
embedding a struct iommu_master inside driver-specific structures is a no-go.

Let's also not forget that a device can be connected to multiple IOMMU ports. 
The only cases I've seen had multiple connections between a bus master device 
and a single IOMMU, all of them potentially sharing a TLB, so a simple 
implementation was possible.

I'm not sure how we could handle connections from one device to different 
IOMMUs, or to multiple ports of the same IOMMU with different TLBs. Splitting 
the device in multiple children struct device might be required.

> > With such structures in place we should be able to eliminate many of the
> > loops in IOMMU drivers that serve no other purpose than to find the
> > master context from a struct device * and some parameters. It will also
> > allow us to keep a central registry of IOMMUs and masters rather than
> > duplicating that in every driver.
> 
> Yes, we should be able to identify an iommu context in a generic way,
> but why do you want to break it down to individual masters within
> one context?

-- 
Regards,

Laurent Pinchart

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hi Arnd,

On 2014-09-02 10:56, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
>>>    -- I have concerns that allocating one domain per master might be
>>> too much, but it's hard to tell without an IOMMU driver ported over.
>> One domain per master is IMHO a sane default configuration. The only default
>> alternative I see is to have only one domain (related with dma-mapping
>> subsystem) and bind all devices to it. However I really don't see any
>> disadvantage of having separate domain per each master and such
>> configuration
>> gives devices better separation.
> I was expecting that the dma-mapping implementation would by default use
> one domain for all devices, since that is what the simpler IOMMUs without
> domain support have to do anyway.
>
> For isolation purposes, it can only help to have more domains, but
> I would guess that there is some space overhead in maintaining lots
> of page tables.

I'm okay with both approaches (separate domain for each device vs. single
common domain for all devices). Maybe this can be some kind of Kconfig
option added to DMA debugging? Separation might be really helpful when
debugging strange device behavior.

>> However we also need to figure out how to let drivers to make their own
>> configuration, like it is required by Exynos DRM subsystem, which consist
>> of several devices, each having its own IOMMU controller, but for
>> convenience those drivers assume that they all have been bound to the same,
>> single domain.
> IIRC with the way we ended up putting the mask into the iommu descriptor of
> the ARM SMMU, you can put multiple devices into the same iommu group, and
> have them automatically share a domain.
>
> I don't know if the same would work for the Samsung implementation.

The question is how to transfer such information from the device 
drivers, that
need/benefit from such configuration to iommu driver, which does all the 
setup?
This is something completely internal to particular drivers and should 
not be
exported to device tree or userspace. Thierry suggested to hardcode this
information in the iommu driver, but I'm looking for other approaches.
Maybe simply releasing device from the default dma-mapping domain before
attaching to custom one will be the easiest solution.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Laurent Pinchart]

Hi Will,

Thank you for the patch.

On Friday 29 August 2014 16:54:27 Will Deacon wrote:
> 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 <will.deacon@arm.com>
> ---
>  drivers/iommu/Kconfig    |  2 +-
>  drivers/iommu/of_iommu.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h |  6 ++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index dd5112265cc9..6d13f962f156 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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"
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index f9209666157c..a7d3b3a13b83 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -19,6 +19,7 @@
> 
>  #include <linux/export.h>
>  #include <linux/limits.h>
> +#include <linux/iommu.h>

Pet peeve of mine, how about keeping the headers alphabetically sorted ?

>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> 
> @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char
> *prefix, int index, }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
> 
> +int of_iommu_configure(struct device *dev)
> +{
> +	struct of_phandle_args iommu_spec;
> +	struct bus_type *bus = dev->bus;
> +	const struct iommu_ops *ops = bus->iommu_ops;
> +	int ret = -EINVAL, idx = 0;
> +
> +	if (!iommu_present(bus))
> +		return -ENODEV;
> +
> +	/*
> +	 * 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)) {
> +		void *data = of_iommu_get_data(iommu_spec.np);
> +
> +		of_node_put(iommu_spec.np);
> +		if (!ops->add_device_master_ids)
> +			return -ENODEV;

Can't you move this outside of the loop ?

> +
> +		ret = ops->add_device_master_ids(dev, iommu_spec.args_count,
> +						 iommu_spec.args, data);

I'm curious, how do you envision support for devices connected to multiple 
IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in the 
add_device operation and store it in the dev->archdata.iommu field. How would 
that work with multiple invocations of add_device or add_device_master_ids ?

> +		if (ret)
> +			break;
> +
> +		idx++;
> +	}
> +
> +	return ret;
> +}
> +
>  void __init of_iommu_init(void)
>  {
>  	struct device_node *np;
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 29f2f3f88d6a..85c6d1152624 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,6 +1,7 @@
>  #ifndef __OF_IOMMU_H
>  #define __OF_IOMMU_H
> 
> +#include <linux/device.h>
>  #include <linux/of.h>
> 
>  #ifdef CONFIG_OF_IOMMU
> @@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn, const
> char *prefix, size_t *size);
> 
>  extern void of_iommu_init(void);
> +extern int of_iommu_configure(struct device *dev);
> 
>  #else
> 
> @@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node
> *dn, const char *prefix, }
> 
>  static inline void of_iommu_init(void) { }
> +static inline int of_iommu_configure(struct device *dev)
> +{
> +	return -ENODEV;
> +}
> 
>  #endif	/* CONFIG_OF_IOMMU */

-- 
Regards,

Laurent Pinchart

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Tue, Sep 02, 2014 at 11:42:13AM +0100, Marek Szyprowski wrote:
> On 2014-09-02 10:56, Arnd Bergmann wrote:
> > On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
> >>>    -- I have concerns that allocating one domain per master might be
> >>> too much, but it's hard to tell without an IOMMU driver ported over.
> >> One domain per master is IMHO a sane default configuration. The only default
> >> alternative I see is to have only one domain (related with dma-mapping
> >> subsystem) and bind all devices to it. However I really don't see any
> >> disadvantage of having separate domain per each master and such
> >> configuration
> >> gives devices better separation.
> > I was expecting that the dma-mapping implementation would by default use
> > one domain for all devices, since that is what the simpler IOMMUs without
> > domain support have to do anyway.
> >
> > For isolation purposes, it can only help to have more domains, but
> > I would guess that there is some space overhead in maintaining lots
> > of page tables.
> 
> I'm okay with both approaches (separate domain for each device vs. single
> common domain for all devices). Maybe this can be some kind of Kconfig
> option added to DMA debugging? Separation might be really helpful when
> debugging strange device behavior.

One potential problem with a single domain is when you have multiple
instances of a given IOMMU, each with different hardware restrictions.
Then you can end up with multiple sets of page tables for the domain
which, although not impossible to work with, is a bit of a mess.

I think having one domain per IOMMU instance would make the most sense,
but then you have to teach more of the stack about the IOMMU topology. I
think we'll get there in the end, but that's a little way off right now.

Will

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Tue, Sep 02, 2014 at 11:51:54AM +0100, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> On Friday 29 August 2014 16:54:27 Will Deacon wrote:
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index dd5112265cc9..6d13f962f156 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -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"
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index f9209666157c..a7d3b3a13b83 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -19,6 +19,7 @@
> > 
> >  #include <linux/export.h>
> >  #include <linux/limits.h>
> > +#include <linux/iommu.h>
> 
> Pet peeve of mine, how about keeping the headers alphabetically sorted ?

D'oh, I'm an idiot. Will fix.

> >  #include <linux/of.h>
> >  #include <linux/of_iommu.h>
> > 
> > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const char
> > *prefix, int index, }
> >  EXPORT_SYMBOL_GPL(of_get_dma_window);
> > 
> > +int of_iommu_configure(struct device *dev)
> > +{
> > +	struct of_phandle_args iommu_spec;
> > +	struct bus_type *bus = dev->bus;
> > +	const struct iommu_ops *ops = bus->iommu_ops;
> > +	int ret = -EINVAL, idx = 0;
> > +
> > +	if (!iommu_present(bus))
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * 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)) {
> > +		void *data = of_iommu_get_data(iommu_spec.np);
> > +
> > +		of_node_put(iommu_spec.np);
> > +		if (!ops->add_device_master_ids)
> > +			return -ENODEV;
> 
> Can't you move this outside of the loop ?

Yup, done.

> > +
> > +		ret = ops->add_device_master_ids(dev, iommu_spec.args_count,
> > +						 iommu_spec.args, data);
> 
> I'm curious, how do you envision support for devices connected to multiple 
> IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in the 
> add_device operation and store it in the dev->archdata.iommu field. How would 
> that work with multiple invocations of add_device or add_device_master_ids ?

Even devices with a single IOMMU could have multiple callbacks, since the
IOMMU gets called back once per ID in reality. That means the IOMMU drivers
will need to be tolerant of adding a master they already know about (by
looking up the device and adding the additional IDs).

Once I've reworked the data argument to be a struct iommu, we can add fields
there if they're generally useful.

Will

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Tuesday 02 September 2014 11:03:42 Will Deacon wrote:
> On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote:
> > On Monday 01 September 2014 17:40:00 Will Deacon wrote:
> > > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote:
> > > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > > > > 
> > > > > I think this could use a bit more formalization. As I said in another
> > > > > reply earlier, there's very little standardization in the IOMMU API.
> > > > > That certainly gives us a lot of flexibility but it also has the
> > > > > downside that it's difficult to handle these abstractions in the core,
> > > > > which is really what the core is all about, isn't it?
> > > > > 
> > > > > One method that worked really well for this in the past for other
> > > > > subsystems is to allow drivers to specify an .of_xlate() function that
> > > > > takes the controller device and a struct of_phandle_args. It is that
> > > > > function's responsibility to take the information in an of_phandle_args
> > > > > structure and use that to create some subsystem specific handle that
> > > > > represents this information in a way that it can readily be used.
> > > > 
> > > > Yes, good idea.
> > > 
> > > Hmm, how does this work for PCI devices? The current RFC takes care to
> > > ensure that the core changes work just as well for OF devices as PCI
> > > devices, and the of-specific functions and data structures are not part of
> > > it.
> > 
> > I don't mind handling PCI devices separately. They are different in a number
> > of ways already, in particular the way that they don't normally have an
> > of_node attached to them but actually have a PCI bus/dev/function number.
> 
> Sure, but at the point when we call back into the iommu_ops structure we
> really don't want bus specific functions. That's why I avoided any OF
> data structures being passed to add_device_master_ids.

Well, we clearly need some format that the caller and the callee agree
on. It can't be a completely opaque pointer because it's something
that has to be filled out by someone who knows the format.

Using the DT format has the advantage that the caller does not have
to know anything about the underlying driver except for #size-cells,
and it just passes the data it gets from DT into the driver. This is
how we do the association in a lot of other subsystems.

> Anyway, I'll try to hack something together shortly. I think the proposal
> is:
> 
>   - Make add_device_master_ids take a generic structure (struct iommu)
>   - Add an of_xlate callback into iommu_ops which returns a populated
>     struct iommu based on the of_node

We may have been talking past one another. What I meant with 'struct iommu'
is something that identifies the iommu instance, not the connection to
a particular master. What you describe here would work, but then I think
the structure should have a different name. However, it seems easier to
not have the add_device_master_ids at and just do the association in the
xlate callback instead.

We still need to figure out how to do it for PCI of course. One
possibility would be to add another argument to the xlate function and
have that called by the PCI device probing method with the iommus
property of the PCI host controller along with the a u64 number that
is generated by the host bridge driver based on the bus/device/function
number of the device.

This means that the new callback function for the iommu API remains
DT specific, but is not really bus specific. It does however not
solve the embedded x86 use case, which may need some other callback.

We might be lucky there if we are able to just use the PCI b/d/f
number as a unique identifier and have a NULL argument for the
respective iommus property.

	Arnd

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Tuesday 02 September 2014 12:42:13 Marek Szyprowski wrote:
> Hi Arnd,
> 
> On 2014-09-02 10:56, Arnd Bergmann wrote:
> > On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
> >>>    -- I have concerns that allocating one domain per master might be
> >>> too much, but it's hard to tell without an IOMMU driver ported over.
> >> One domain per master is IMHO a sane default configuration. The only default
> >> alternative I see is to have only one domain (related with dma-mapping
> >> subsystem) and bind all devices to it. However I really don't see any
> >> disadvantage of having separate domain per each master and such
> >> configuration
> >> gives devices better separation.
> > I was expecting that the dma-mapping implementation would by default use
> > one domain for all devices, since that is what the simpler IOMMUs without
> > domain support have to do anyway.
> >
> > For isolation purposes, it can only help to have more domains, but
> > I would guess that there is some space overhead in maintaining lots
> > of page tables.
> 
> I'm okay with both approaches (separate domain for each device vs. single
> common domain for all devices). Maybe this can be some kind of Kconfig
> option added to DMA debugging? Separation might be really helpful when
> debugging strange device behavior.

We should probably support the iommu=strict command line option that some
other architectures have. This is mainly meant to ensure that IOTLBs
are shot down as soon as the driver unmaps some memory, which you often
want to avoid for performance reasons.

The iommu driver itself can then decide to also use separate domains
for iommu=strict but a shared domain otherwise.

For hardware on which the shared domain is hard to do, the driver might
always use separate domains.

> >> However we also need to figure out how to let drivers to make their own
> >> configuration, like it is required by Exynos DRM subsystem, which consist
> >> of several devices, each having its own IOMMU controller, but for
> >> convenience those drivers assume that they all have been bound to the same,
> >> single domain.
> > IIRC with the way we ended up putting the mask into the iommu descriptor of
> > the ARM SMMU, you can put multiple devices into the same iommu group, and
> > have them automatically share a domain.
> >
> > I don't know if the same would work for the Samsung implementation.
> 
> The question is how to transfer such information from the device 
> drivers, that
> need/benefit from such configuration to iommu driver, which does all the 
> setup?
> This is something completely internal to particular drivers and should 
> not be
> exported to device tree or userspace. Thierry suggested to hardcode this
> information in the iommu driver, but I'm looking for other approaches.
> Maybe simply releasing device from the default dma-mapping domain before
> attaching to custom one will be the easiest solution.

For the ARM SMMU, the problem is that there is not necessarily a good way
to partition the masters into IOMMU groups automatically, therefore we
want to provide some hints in DT. On a machine that can have more domains
than it has masters, this is not a problem and we can always use an
all-ones mask, but for a machine on which this is not the case, the
problem is simplified a lot of we hardcode the masks in a way that can
always work, putting multiple devices into an iommu group if necessary.

This is similar to how we do things for pinctrl, where you might have
a theoretically endless space of options to set stuff up, but we
can simplify it by defining the useful configurations.

	Arnd

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hi Will,

On 2014-09-02 12:57, Will Deacon wrote:
> On Tue, Sep 02, 2014 at 11:42:13AM +0100, Marek Szyprowski wrote:
>> On 2014-09-02 10:56, Arnd Bergmann wrote:
>>> On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
>>>>>     -- I have concerns that allocating one domain per master might be
>>>>> too much, but it's hard to tell without an IOMMU driver ported over.
>>>> One domain per master is IMHO a sane default configuration. The only default
>>>> alternative I see is to have only one domain (related with dma-mapping
>>>> subsystem) and bind all devices to it. However I really don't see any
>>>> disadvantage of having separate domain per each master and such
>>>> configuration
>>>> gives devices better separation.
>>> I was expecting that the dma-mapping implementation would by default use
>>> one domain for all devices, since that is what the simpler IOMMUs without
>>> domain support have to do anyway.
>>>
>>> For isolation purposes, it can only help to have more domains, but
>>> I would guess that there is some space overhead in maintaining lots
>>> of page tables.
>> I'm okay with both approaches (separate domain for each device vs. single
>> common domain for all devices). Maybe this can be some kind of Kconfig
>> option added to DMA debugging? Separation might be really helpful when
>> debugging strange device behavior.
> One potential problem with a single domain is when you have multiple
> instances of a given IOMMU, each with different hardware restrictions.
> Then you can end up with multiple sets of page tables for the domain
> which, although not impossible to work with, is a bit of a mess.

Maybe the default dma-mapping domain should be one per a given IOMMU 
instance?
This will simplify a lot of things in such case.

> I think having one domain per IOMMU instance would make the most sense,
> but then you have to teach more of the stack about the IOMMU topology. I
> think we'll get there in the end, but that's a little way off right now.

Right, those seems to be a details.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hi Arnd,

On 2014-09-02 14:22, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 12:42:13 Marek Szyprowski wrote:
>> On 2014-09-02 10:56, Arnd Bergmann wrote:
>>> On Tuesday 02 September 2014 10:48:02 Marek Szyprowski wrote:
>>>>>     -- I have concerns that allocating one domain per master might be
>>>>> too much, but it's hard to tell without an IOMMU driver ported over.
>>>> One domain per master is IMHO a sane default configuration. The only default
>>>> alternative I see is to have only one domain (related with dma-mapping
>>>> subsystem) and bind all devices to it. However I really don't see any
>>>> disadvantage of having separate domain per each master and such
>>>> configuration
>>>> gives devices better separation.
>>> I was expecting that the dma-mapping implementation would by default use
>>> one domain for all devices, since that is what the simpler IOMMUs without
>>> domain support have to do anyway.
>>>
>>> For isolation purposes, it can only help to have more domains, but
>>> I would guess that there is some space overhead in maintaining lots
>>> of page tables.
>> I'm okay with both approaches (separate domain for each device vs. single
>> common domain for all devices). Maybe this can be some kind of Kconfig
>> option added to DMA debugging? Separation might be really helpful when
>> debugging strange device behavior.
> We should probably support the iommu=strict command line option that some
> other architectures have. This is mainly meant to ensure that IOTLBs
> are shot down as soon as the driver unmaps some memory, which you often
> want to avoid for performance reasons.
>
> The iommu driver itself can then decide to also use separate domains
> for iommu=strict but a shared domain otherwise.
>
> For hardware on which the shared domain is hard to do, the driver might
> always use separate domains.

Just to let you know, lazy unmapping is not yet implemented in ARM 
dma-mapping
implementation based on IOMMU.

>>>> However we also need to figure out how to let drivers to make their own
>>>> configuration, like it is required by Exynos DRM subsystem, which consist
>>>> of several devices, each having its own IOMMU controller, but for
>>>> convenience those drivers assume that they all have been bound to the same,
>>>> single domain.
>>> IIRC with the way we ended up putting the mask into the iommu descriptor of
>>> the ARM SMMU, you can put multiple devices into the same iommu group, and
>>> have them automatically share a domain.
>>>
>>> I don't know if the same would work for the Samsung implementation.
>> The question is how to transfer such information from the device
>> drivers, that
>> need/benefit from such configuration to iommu driver, which does all the
>> setup?
>> This is something completely internal to particular drivers and should
>> not be
>> exported to device tree or userspace. Thierry suggested to hardcode this
>> information in the iommu driver, but I'm looking for other approaches.
>> Maybe simply releasing device from the default dma-mapping domain before
>> attaching to custom one will be the easiest solution.
> For the ARM SMMU, the problem is that there is not necessarily a good way
> to partition the masters into IOMMU groups automatically, therefore we
> want to provide some hints in DT. On a machine that can have more domains
> than it has masters, this is not a problem and we can always use an
> all-ones mask, but for a machine on which this is not the case, the
> problem is simplified a lot of we hardcode the masks in a way that can
> always work, putting multiple devices into an iommu group if necessary.

Well, I was talking about the Exynos IOMMU case, where there are no hw
restrictions and grouping is done just to make things easier for the Exynos
DRM drivers (a buffer gets the same DMA address for all devices, which
are a part of virtual Exynos DRM device).

> This is similar to how we do things for pinctrl, where you might have
> a theoretically endless space of options to set stuff up, but we
> can simplify it by defining the useful configurations.

Right, if hardware is limited, a sane working configuration is something 
that
should be encoded in device tree.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Tuesday 02 September 2014 14:24:18 Marek Szyprowski wrote:
> >>> For isolation purposes, it can only help to have more domains, but
> >>> I would guess that there is some space overhead in maintaining lots
> >>> of page tables.
> >> I'm okay with both approaches (separate domain for each device vs. single
> >> common domain for all devices). Maybe this can be some kind of Kconfig
> >> option added to DMA debugging? Separation might be really helpful when
> >> debugging strange device behavior.
> > One potential problem with a single domain is when you have multiple
> > instances of a given IOMMU, each with different hardware restrictions.
> > Then you can end up with multiple sets of page tables for the domain
> > which, although not impossible to work with, is a bit of a mess.
> 
> Maybe the default dma-mapping domain should be one per a given IOMMU 
> instance?
> This will simplify a lot of things in such case.

Yes, that sounds like a good idea.

	Arnd

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Tuesday 02 September 2014 14:30:36 Marek Szyprowski wrote:
> >>>> However we also need to figure out how to let drivers to make their own
> >>>> configuration, like it is required by Exynos DRM subsystem, which consist
> >>>> of several devices, each having its own IOMMU controller, but for
> >>>> convenience those drivers assume that they all have been bound to the same,
> >>>> single domain.
> >>> IIRC with the way we ended up putting the mask into the iommu descriptor of
> >>> the ARM SMMU, you can put multiple devices into the same iommu group, and
> >>> have them automatically share a domain.
> >>>
> >>> I don't know if the same would work for the Samsung implementation.
> >> The question is how to transfer such information from the device
> >> drivers, that
> >> need/benefit from such configuration to iommu driver, which does all the
> >> setup?
> >> This is something completely internal to particular drivers and should
> >> not be
> >> exported to device tree or userspace. Thierry suggested to hardcode this
> >> information in the iommu driver, but I'm looking for other approaches.
> >> Maybe simply releasing device from the default dma-mapping domain before
> >> attaching to custom one will be the easiest solution.
> > For the ARM SMMU, the problem is that there is not necessarily a good way
> > to partition the masters into IOMMU groups automatically, therefore we
> > want to provide some hints in DT. On a machine that can have more domains
> > than it has masters, this is not a problem and we can always use an
> > all-ones mask, but for a machine on which this is not the case, the
> > problem is simplified a lot of we hardcode the masks in a way that can
> > always work, putting multiple devices into an iommu group if necessary.
> 
> Well, I was talking about the Exynos IOMMU case, where there are no hw
> restrictions and grouping is done just to make things easier for the Exynos
> DRM drivers (a buffer gets the same DMA address for all devices, which
> are a part of virtual Exynos DRM device).

Does that mean you don't actually need to use multiple contexts here and
could actually just use the normal dma-mapping interface if there is
a way to ensure the mappings are always shared across these masters?

Or do you need this in addition to being able to use multiple masters
so you can have multiple rendering contexts in user space?

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Tue, Sep 02, 2014 at 01:15:06PM +0100, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 11:03:42 Will Deacon wrote:
> > On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote:
> > > I don't mind handling PCI devices separately. They are different in a number
> > > of ways already, in particular the way that they don't normally have an
> > > of_node attached to them but actually have a PCI bus/dev/function number.
> > 
> > Sure, but at the point when we call back into the iommu_ops structure we
> > really don't want bus specific functions. That's why I avoided any OF
> > data structures being passed to add_device_master_ids.
> 
> Well, we clearly need some format that the caller and the callee agree
> on. It can't be a completely opaque pointer because it's something
> that has to be filled out by someone who knows the format.
> 
> Using the DT format has the advantage that the caller does not have
> to know anything about the underlying driver except for #size-cells,
> and it just passes the data it gets from DT into the driver. This is
> how we do the association in a lot of other subsystems.

Ok. More below.

> > Anyway, I'll try to hack something together shortly. I think the proposal
> > is:
> > 
> >   - Make add_device_master_ids take a generic structure (struct iommu)
> >   - Add an of_xlate callback into iommu_ops which returns a populated
> >     struct iommu based on the of_node
> 
> We may have been talking past one another. What I meant with 'struct iommu'
> is something that identifies the iommu instance, not the connection to
> a particular master. What you describe here would work, but then I think
> the structure should have a different name. However, it seems easier to
> not have the add_device_master_ids at and just do the association in the
> xlate callback instead.

Yes, I think we were talking about two different things. If we move all
of the master handling into the xlate callback, then we can just use
of_phandle_args as the generic master representation (using the PCI host
controller node for PCI devices, as you mentioned). However, xlate is a
bit of a misnomer then, as it's not actually translating anything; the
handle used for DMA masters is still struct device, and we have that
already in of_dma_configure.

I'm still unsure about what to put inside struct iommu other than a private
data pointer. You previously suggested:

	struct iommu {
		struct device *dev;
		const struct iommu_ops *ops;
		struct list_head domains;
		void *private;
	};

but that has the following problems:

  - We don't have a struct device * for the IOMMU until it's been probed
    via the standard driver probing mechanisms, which may well be after
    we've started registering masters

  - The ops are still registered on a per-bus basis, and each domain has
    a pointer to them anyway

  - The IOMMU driver really doesn't care about the domains, as the domain
    in question is always passed back to the functions that need it (e.g.
    attach, map, ...).

The only useful field I can think of is something like a tree of masters,
but then we have to define a generic wrapper around struct device, which
is at odds with the rest of the IOMMU API.

One alternative is having the xlate call populate device->archdata.iommu,
but that's arch-specific and is essentially another opaque pointer.

Will

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

On 2014-09-02 14:46, Arnd Bergmann wrote:
> On Tuesday 02 September 2014 14:30:36 Marek Szyprowski wrote:
>>>>>> However we also need to figure out how to let drivers to make their own
>>>>>> configuration, like it is required by Exynos DRM subsystem, which consist
>>>>>> of several devices, each having its own IOMMU controller, but for
>>>>>> convenience those drivers assume that they all have been bound to the same,
>>>>>> single domain.
>>>>> IIRC with the way we ended up putting the mask into the iommu descriptor of
>>>>> the ARM SMMU, you can put multiple devices into the same iommu group, and
>>>>> have them automatically share a domain.
>>>>>
>>>>> I don't know if the same would work for the Samsung implementation.
>>>> The question is how to transfer such information from the device
>>>> drivers, that
>>>> need/benefit from such configuration to iommu driver, which does all the
>>>> setup?
>>>> This is something completely internal to particular drivers and should
>>>> not be
>>>> exported to device tree or userspace. Thierry suggested to hardcode this
>>>> information in the iommu driver, but I'm looking for other approaches.
>>>> Maybe simply releasing device from the default dma-mapping domain before
>>>> attaching to custom one will be the easiest solution.
>>> For the ARM SMMU, the problem is that there is not necessarily a good way
>>> to partition the masters into IOMMU groups automatically, therefore we
>>> want to provide some hints in DT. On a machine that can have more domains
>>> than it has masters, this is not a problem and we can always use an
>>> all-ones mask, but for a machine on which this is not the case, the
>>> problem is simplified a lot of we hardcode the masks in a way that can
>>> always work, putting multiple devices into an iommu group if necessary.
>> Well, I was talking about the Exynos IOMMU case, where there are no hw
>> restrictions and grouping is done just to make things easier for the Exynos
>> DRM drivers (a buffer gets the same DMA address for all devices, which
>> are a part of virtual Exynos DRM device).
> Does that mean you don't actually need to use multiple contexts here and
> could actually just use the normal dma-mapping interface if there is
> a way to ensure the mappings are always shared across these masters?

Well, a default, shared single domain for dma-mapping interface will work
with Exynos DRM and its multiple masters, although I never thought about
such configuration.

> Or do you need this in addition to being able to use multiple masters
> so you can have multiple rendering contexts in user space?

Such advanced IO space management is not yet implemented.

However there are also devices (like multimedia codec - exynos mfc and 
camera
capture subsystem exynos isp), which have limited DMA/IO window (256MiB in
case of video codec), so they will still need to use their own separate 
domain.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Tuesday 02 September 2014 14:05:08 Will Deacon wrote:
> On Tue, Sep 02, 2014 at 01:15:06PM +0100, Arnd Bergmann wrote:

> > > Anyway, I'll try to hack something together shortly. I think the proposal
> > > is:
> > > 
> > >   - Make add_device_master_ids take a generic structure (struct iommu)
> > >   - Add an of_xlate callback into iommu_ops which returns a populated
> > >     struct iommu based on the of_node
> > 
> > We may have been talking past one another. What I meant with 'struct iommu'
> > is something that identifies the iommu instance, not the connection to
> > a particular master. What you describe here would work, but then I think
> > the structure should have a different name. However, it seems easier to
> > not have the add_device_master_ids at and just do the association in the
> > xlate callback instead.
> 
> Yes, I think we were talking about two different things. If we move all
> of the master handling into the xlate callback, then we can just use
> of_phandle_args as the generic master representation (using the PCI host
> controller node for PCI devices, as you mentioned). However, xlate is a
> bit of a misnomer then, as it's not actually translating anything; the
> handle used for DMA masters is still struct device, and we have that
> already in of_dma_configure.
> 
> I'm still unsure about what to put inside struct iommu other than a private
> data pointer. You previously suggested:
> 
> 	struct iommu {
> 		struct device *dev;
> 		const struct iommu_ops *ops;
> 		struct list_head domains;
> 		void *private;
> 	};
> 
> but that has the following problems:
> 
>   - We don't have a struct device * for the IOMMU until it's been probed
>     via the standard driver probing mechanisms, which may well be after
>     we've started registering masters

Right, this may have to be the device_node pointer. I also realized that if
we have this structure, we can stop using the device_node->data field
and instead walk a list of iommu structures.

>   - The ops are still registered on a per-bus basis, and each domain has
>     a pointer to them anyway

I think that has to change in the long run: we may well require distinct
IOMMU operations if we have multiple IOMMUs used on the same bus_type.
 
>   - The IOMMU driver really doesn't care about the domains, as the domain
>     in question is always passed back to the functions that need it (e.g.
>     attach, map, ...).

This is an artifact of the API being single-instance at the moment.
We might not in fact need it, I was just trying to think of things
that naturally fit in there and that are probably already linked
together in the individual iommu drivers.

> The only useful field I can think of is something like a tree of masters,
> but then we have to define a generic wrapper around struct device, which
> is at odds with the rest of the IOMMU API.

Maybe a list of the groups instead? I don't think you should need
a list of every single master here.

> One alternative is having the xlate call populate device->archdata.iommu,
> but that's arch-specific and is essentially another opaque pointer.

I think every architecture that supports IOMMUs needs to have this
archdata pointer though, so that is still a good place to put private
stuff.

	Arnd

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Varun Sethi]

Hi Will,

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Friday, August 29, 2014 9:24 PM
> To: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org
> Cc: arnd at arndb.de; dwmw2 at infradead.org; jroedel at suse.de;
> hdoyu at nvidia.com; Sethi Varun-B16395; thierry.reding at gmail.com;
> laurent.pinchart at ideasonboard.com; Will Deacon
> Subject: [RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU
> drivers
> 
> 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. This data can then be
> passed back to a new add_device function (added in a later patch) to
> identify the IOMMU instance in question.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  drivers/iommu/of_iommu.c          | 14 ++++++++++++++
>  include/asm-generic/vmlinux.lds.h |  2 ++
>  include/linux/of_iommu.h          | 21 +++++++++++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e550ccb7634e..f9209666157c 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -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;

Is the init function also responsible for setting iommu_ops (per bus)? We need to take in to consideration that iommu API (iommu.c) initialization happens during  "arch_init". When we setup bus iommu ops add_device would be called for devices which have already been probed. Also, as I can see from the code we have of_platform_populate which gets called right after of_iommu_init, which would in turn also lead to add_device invocation (after add_device_master_ids). I believe this would happen before iommu API initialization which would cause issues.

-Varun

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Varun Sethi]

Hi Will,
I am not clear on the functionality we want to achieve with this new API. Is this a way to link devices to a particular IOMMU? Would this be used to filter out add_device invocations i.e. iommu group creations just  for the devices attached to a particular IOMMU?

What is the purpose of the data pointer, that is being passed to this API? Also, how would this be relevant to PCIe devices (also other hot plug devices).

Regards
Varun

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Friday, August 29, 2014 9:24 PM
> To: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org
> Cc: arnd at arndb.de; dwmw2 at infradead.org; jroedel at suse.de;
> hdoyu at nvidia.com; Sethi Varun-B16395; thierry.reding at gmail.com;
> laurent.pinchart at ideasonboard.com; Will Deacon
> Subject: [RFC PATCH 4/7] iommu: provide helper function to configure an
> IOMMU for an of master
> 
> 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 <will.deacon@arm.com>
> ---
>  drivers/iommu/Kconfig    |  2 +-
>  drivers/iommu/of_iommu.c | 36
> ++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h |  6 ++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index
> dd5112265cc9..6d13f962f156 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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"
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index
> f9209666157c..a7d3b3a13b83 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -19,6 +19,7 @@
> 
>  #include <linux/export.h>
>  #include <linux/limits.h>
> +#include <linux/iommu.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> 
> @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const
> char *prefix, int index,  }  EXPORT_SYMBOL_GPL(of_get_dma_window);
> 
> +int of_iommu_configure(struct device *dev) {
> +	struct of_phandle_args iommu_spec;
> +	struct bus_type *bus = dev->bus;
> +	const struct iommu_ops *ops = bus->iommu_ops;
> +	int ret = -EINVAL, idx = 0;
> +
> +	if (!iommu_present(bus))
> +		return -ENODEV;
> +
> +	/*
> +	 * 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)) {
> +		void *data = of_iommu_get_data(iommu_spec.np);
> +
> +		of_node_put(iommu_spec.np);
> +		if (!ops->add_device_master_ids)
> +			return -ENODEV;
> +
> +		ret = ops->add_device_master_ids(dev,
> iommu_spec.args_count,
> +						 iommu_spec.args, data);
> +		if (ret)
> +			break;
> +
> +		idx++;
> +	}
> +
> +	return ret;
> +}
> +
>  void __init of_iommu_init(void)
>  {
>  	struct device_node *np;
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index
> 29f2f3f88d6a..85c6d1152624 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,6 +1,7 @@
>  #ifndef __OF_IOMMU_H
>  #define __OF_IOMMU_H
> 
> +#include <linux/device.h>
>  #include <linux/of.h>
> 
>  #ifdef CONFIG_OF_IOMMU
> @@ -10,6 +11,7 @@ extern int of_get_dma_window(struct device_node *dn,
> const char *prefix,
>  			     size_t *size);
> 
>  extern void of_iommu_init(void);
> +extern int of_iommu_configure(struct device *dev);
> 
>  #else
> 
> @@ -21,6 +23,10 @@ static inline int of_get_dma_window(struct device_node
> *dn, const char *prefix,  }
> 
>  static inline void of_iommu_init(void) { }
> +static inline int of_iommu_configure(struct device *dev) {
> +	return -ENODEV;
> +}
> 
>  #endif	/* CONFIG_OF_IOMMU */
> 
> --
> 2.1.0

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Varun Sethi]

Hi Thierry/Will/Arnd,
Where would the of_xlate callback reside and what would be its function? 

Regards
Varun

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: Tuesday, September 02, 2014 5:45 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Will Deacon; jroedel at suse.de; iommu at lists.linux-foundation.org; Thierry
> Reding; laurent.pinchart at ideasonboard.com; Sethi Varun-B16395;
> dwmw2 at infradead.org; hdoyu at nvidia.com
> Subject: Re: [RFC PATCH 4/7] iommu: provide helper function to configure an
> IOMMU for an of master
> 
> On Tuesday 02 September 2014 11:03:42 Will Deacon wrote:
> > On Mon, Sep 01, 2014 at 09:18:26PM +0100, Arnd Bergmann wrote:
> > > On Monday 01 September 2014 17:40:00 Will Deacon wrote:
> > > > On Mon, Sep 01, 2014 at 03:46:18PM +0100, Arnd Bergmann wrote:
> > > > > On Monday 01 September 2014 10:29:40 Thierry Reding wrote:
> > > > > >
> > > > > > I think this could use a bit more formalization. As I said in
> > > > > > another reply earlier, there's very little standardization in the IOMMU
> API.
> > > > > > That certainly gives us a lot of flexibility but it also has
> > > > > > the downside that it's difficult to handle these abstractions
> > > > > > in the core, which is really what the core is all about, isn't it?
> > > > > >
> > > > > > One method that worked really well for this in the past for
> > > > > > other subsystems is to allow drivers to specify an .of_xlate()
> > > > > > function that takes the controller device and a struct
> > > > > > of_phandle_args. It is that function's responsibility to take
> > > > > > the information in an of_phandle_args structure and use that
> > > > > > to create some subsystem specific handle that represents this
> information in a way that it can readily be used.
> > > > >
> > > > > Yes, good idea.
> > > >
> > > > Hmm, how does this work for PCI devices? The current RFC takes
> > > > care to ensure that the core changes work just as well for OF
> > > > devices as PCI devices, and the of-specific functions and data
> > > > structures are not part of it.
> > >
> > > I don't mind handling PCI devices separately. They are different in
> > > a number of ways already, in particular the way that they don't
> > > normally have an of_node attached to them but actually have a PCI
> bus/dev/function number.
> >
> > Sure, but at the point when we call back into the iommu_ops structure
> > we really don't want bus specific functions. That's why I avoided any
> > OF data structures being passed to add_device_master_ids.
> 
> Well, we clearly need some format that the caller and the callee agree on. It
> can't be a completely opaque pointer because it's something that has to be
> filled out by someone who knows the format.
> 
> Using the DT format has the advantage that the caller does not have to know
> anything about the underlying driver except for #size-cells, and it just passes
> the data it gets from DT into the driver. This is how we do the association in a
> lot of other subsystems.
> 
> > Anyway, I'll try to hack something together shortly. I think the
> > proposal
> > is:
> >
> >   - Make add_device_master_ids take a generic structure (struct iommu)
> >   - Add an of_xlate callback into iommu_ops which returns a populated
> >     struct iommu based on the of_node
> 
> We may have been talking past one another. What I meant with 'struct iommu'
> is something that identifies the iommu instance, not the connection to a
> particular master. What you describe here would work, but then I think the
> structure should have a different name. However, it seems easier to not have
> the add_device_master_ids at and just do the association in the xlate callback
> instead.
> 
> We still need to figure out how to do it for PCI of course. One possibility would
> be to add another argument to the xlate function and have that called by the
> PCI device probing method with the iommus property of the PCI host controller
> along with the a u64 number that is generated by the host bridge driver based
> on the bus/device/function number of the device.
> 
> This means that the new callback function for the iommu API remains DT
> specific, but is not really bus specific. It does however not solve the embedded
> x86 use case, which may need some other callback.
> 
> We might be lucky there if we are able to just use the PCI b/d/f number as a
> unique identifier and have a NULL argument for the respective iommus
> property.

[RFC PATCH 1/7] iommu: provide early initialisation hook for IOMMU drivers

[Arnd Bergmann]

On Tuesday 02 September 2014 14:47:54 Varun Sethi wrote:
> > +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;
> 
> Is the init function also responsible for setting iommu_ops (per bus)?
> We need to take in to consideration that iommu API (iommu.c)
> initialization happens during  "arch_init".

I would hope that as part as one of the next steps, we can skip
the step of setting the iommu_ops per bus_type. It's really
not that meaningful when only some of the devices on one bus
are able to use an iommu, and some may need other ops than others.

> When we setup bus iommu
> ops add_device would be called for devices which have already been
> probed.

As soon as you move to the generic way of probing the IOMMU for
DT, you should not need an add_device callback any more.

Each iommu driver should do one one or the other, but not both.

> Also, as I can see from the code we have of_platform_populate
> which gets called right after of_iommu_init, which would in turn also
> lead to add_device invocation (after add_device_master_ids). 
> I believe this would happen before iommu API initialization which
> would cause issues. 

of_iommu_init should be the point where the iommu API gets
initialized.

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Arnd Bergmann]

On Tuesday 02 September 2014 15:03:14 Varun Sethi wrote:
> Hi Thierry/Will/Arnd,
> Where would the of_xlate callback reside and what would be its function? 
> 

It is an additional function pointer in iommu_ops and replaces the
add_device callback for IOMMUs that are DT-enabled.

The idea is that we can have a single function that sets up DMA
for the device properly, including iommus when they are configured,
and it lets us pass the necessary parameters describing the setup
of the master device into the iommu driver.

	Arnd

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Laurent Pinchart]

Hi Will,

On Tuesday 02 September 2014 12:03:40 Will Deacon wrote:
> On Tue, Sep 02, 2014 at 11:51:54AM +0100, Laurent Pinchart wrote:
> > On Friday 29 August 2014 16:54:27 Will Deacon wrote:

[snip]

> > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > > index f9209666157c..a7d3b3a13b83 100644
> > > --- a/drivers/iommu/of_iommu.c
> > > +++ b/drivers/iommu/of_iommu.c

[snip]

> > > @@ -90,6 +91,41 @@ int of_get_dma_window(struct device_node *dn, const
> > > char
> > > *prefix, int index, }
> > > 
> > >  EXPORT_SYMBOL_GPL(of_get_dma_window);
> > > 
> > > +int of_iommu_configure(struct device *dev)
> > > +{
> > > +	struct of_phandle_args iommu_spec;
> > > +	struct bus_type *bus = dev->bus;
> > > +	const struct iommu_ops *ops = bus->iommu_ops;
> > > +	int ret = -EINVAL, idx = 0;
> > > +
> > > +	if (!iommu_present(bus))
> > > +		return -ENODEV;
> > > +
> > > +	/*
> > > +	 * 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)) {
> > > +		void *data = of_iommu_get_data(iommu_spec.np);
> > > +
> > > +		of_node_put(iommu_spec.np);
> > > +		if (!ops->add_device_master_ids)
> > > +			return -ENODEV;
> > 
> > Can't you move this outside of the loop ?
> 
> Yup, done.
> 
> > > +
> > > +		ret = ops->add_device_master_ids(dev, iommu_spec.args_count,
> > > +						 iommu_spec.args, data);
> > 
> > I'm curious, how do you envision support for devices connected to multiple
> > IOMMUs ? IOMMU drivers usually allocate a per-device archdata structure in
> > the add_device operation and store it in the dev->archdata.iommu field.
> > How would that work with multiple invocations of add_device or
> > add_device_master_ids ?
>
> Even devices with a single IOMMU could have multiple callbacks, since the
> IOMMU gets called back once per ID in reality. That means the IOMMU drivers
> will need to be tolerant of adding a master they already know about (by
> looking up the device and adding the additional IDs).

That wouldn't be too difficult to implement in IOMMU drivers, but I have a 
feeling we would duplicate the same logic in several drivers. Adding fields to 
struct iommu (possibly with a couple of helper functions) as mentioned below 
might help. Another option would be to call the add_device_master_ids a single 
time with a list of all ids, but the core code might become pretty ugly.

> Once I've reworked the data argument to be a struct iommu, we can add fields
> there if they're generally useful.

-- 
Regards,

Laurent Pinchart

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[jroedel at suse.de]

On Tue, Sep 02, 2014 at 04:01:32PM +0200, Arnd Bergmann wrote:
> This is an artifact of the API being single-instance at the moment.
> We might not in fact need it, I was just trying to think of things
> that naturally fit in there and that are probably already linked
> together in the individual iommu drivers.

I am not sure what you mean by single-instance. Is it that currently the
API only supports one type of iommu_ops per bus? That should be fine as
long as there is only one type of IOMMU on the bus.

Besides that, it is a feature of the IOMMU-API to hide the details about
all the hardware IOMMUs in the system from its users.


	Joerg

[RFC PATCH 0/7] Introduce automatic DMA configuration for IOMMU masters

[Laurent Pinchart]

On Tuesday 02 September 2014 14:43:18 Arnd Bergmann wrote:
> On Tuesday 02 September 2014 14:24:18 Marek Szyprowski wrote:
> > >>> For isolation purposes, it can only help to have more domains, but
> > >>> I would guess that there is some space overhead in maintaining lots
> > >>> of page tables.
> > >> 
> > >> I'm okay with both approaches (separate domain for each device vs.
> > >> single common domain for all devices). Maybe this can be some kind of
> > >> Kconfig option added to DMA debugging? Separation might be really
> > >> helpful when debugging strange device behavior.
> > > 
> > > One potential problem with a single domain is when you have multiple
> > > instances of a given IOMMU, each with different hardware restrictions.
> > > Then you can end up with multiple sets of page tables for the domain
> > > which, although not impossible to work with, is a bit of a mess.
> > 
> > Maybe the default dma-mapping domain should be one per a given IOMMU
> > instance? This will simplify a lot of things in such case.
> 
> Yes, that sounds like a good idea.

That would work as a default configuration for the Renesas IPMMU IOMMU, which 
supports four TLBs to be shared between more than four bus masters. Ideally 
I'd like to make the bus master to TLB association somehow configurable, but 
associating all bus masters with a single TLB as a first step is fine.

-- 
Regards,

Laurent Pinchart

[RFC PATCH 4/7] iommu: provide helper function to configure an IOMMU for an of master

[Will Deacon]

On Tue, Sep 02, 2014 at 09:59:41PM +0100, jroedel at suse.de wrote:
> On Tue, Sep 02, 2014 at 04:01:32PM +0200, Arnd Bergmann wrote:
> > This is an artifact of the API being single-instance at the moment.
> > We might not in fact need it, I was just trying to think of things
> > that naturally fit in there and that are probably already linked
> > together in the individual iommu drivers.
> 
> I am not sure what you mean by single-instance. Is it that currently the
> API only supports one type of iommu_ops per bus? That should be fine as
> long as there is only one type of IOMMU on the bus.

The problem is really with the platform_bus, which is used as a catch-all
for all the non-probable memory-mapped buses on an SoC. We really want to
have different IOMMU types there.

> Besides that, it is a feature of the IOMMU-API to hide the details about
> all the hardware IOMMUs in the system from its users.

Sure, but even if we have multiple instances of the same IOMMU type, the
complexity in dealing with that is currently pushed down into the drivers,
with each one trying to construct its own notion of topology and master
linkages. Moving some of this into generic code would ease the burden on
IOMMU drivers.

Will