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

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

[Will Deacon]

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

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

Changes since RFCv3 include:

  - Drastic simplification of the data structures, so that we no longer
    pass around lists of domains. Instead, dma-mapping is expected to
    allocate the domain (Joerg talked about adding a get_default_domain
    operation to iommu_ops).

  - iommu_ops is used to hold the per-instance IOMMU data

  - Configuration of DMA segments added to of_dma_configure

All feedback welcome.

Cheers,

Will

--->8

Will Deacon (8):
  iommu: provide early initialisation hook for IOMMU drivers
  dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
  iommu: add new iommu_ops callback for adding an OF device
  iommu: provide helper function to configure an IOMMU for an of master
  dma-mapping: detect and configure IOMMU in of_dma_configure
  dma-mapping: set dma segment properties 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 | 12 +++---
 arch/arm/kernel/setup.c            |  2 +
 arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
 drivers/iommu/Kconfig              |  2 +-
 drivers/iommu/of_iommu.c           | 50 ++++++++++++++++++++++++
 drivers/of/platform.c              | 54 +++++++++++++------------
 include/asm-generic/vmlinux.lds.h  |  2 +
 include/linux/amba/bus.h           |  1 +
 include/linux/dma-mapping.h        | 13 ++++---
 include/linux/iommu.h              |  8 ++++
 include/linux/of_iommu.h           | 31 +++++++++++++++
 include/linux/platform_device.h    |  2 +
 12 files changed, 214 insertions(+), 43 deletions(-)

-- 
2.1.1

[RFC PATCH v4 1/8] 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
store some per-instance data in their iommu_ops structure and store that
in their of_node. This can later be used when parsing OF masters to
identify the IOMMU instance in question.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  2 ++
 include/linux/iommu.h             |  2 ++
 include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..89b903406968 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,9 @@
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+	__used __section(__iommu_of_table_end);
+
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
  *
@@ -89,3 +92,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 of_iommu_init_fn init_fn = 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 aa70cbda327c..bee5d683074d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -164,6 +164,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)
@@ -497,6 +498,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/iommu.h b/include/linux/iommu.h
index e6a7c9ff72f2..7b83f9f8e11d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -103,6 +103,7 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {
 	bool (*capable)(enum iommu_cap);
@@ -133,6 +134,7 @@ struct iommu_ops {
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
 	unsigned long pgsize_bitmap;
+	void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..81447e0f68b5 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,12 +1,17 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     int index, unsigned long *busno, dma_addr_t *addr,
 			     size_t *size);
 
+extern void of_iommu_init(void);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -16,6 +21,26 @@ 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_ops(struct device_node *np,
+				    struct iommu_ops *ops)
+{
+	np->data = ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+	return np->data;
+}
+
+extern struct of_device_id __iommu_of_table;
+
+typedef int (*of_iommu_init_fn)(struct device_node *);
+
+#define IOMMU_OF_DECLARE(name, compat, fn) \
+	_OF_DECLARE(iommu, name, compat, fn, of_iommu_init_fn)
+
 #endif /* __OF_IOMMU_H */
-- 
2.1.1

Re: [RFC PATCH v4 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Marek Szyprowski]

Hello,

On 2014-11-14 19:56, 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
> store some per-instance data in their iommu_ops structure and store that
> in their of_node. This can later be used when parsing OF masters to
> identify the IOMMU instance in question.
>
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>   drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
>   include/asm-generic/vmlinux.lds.h |  2 ++
>   include/linux/iommu.h             |  2 ++
>   include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
>   4 files changed, 46 insertions(+)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e550ccb7634e..89b903406968 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -22,6 +22,9 @@
>   #include <linux/of.h>
>   #include <linux/of_iommu.h>
>   
> +static const struct of_device_id __iommu_of_table_sentinel
> +	__used __section(__iommu_of_table_end);
> +
>   /**
>    * of_get_dma_window - Parse *dma-window property and returns 0 if found.
>    *
> @@ -89,3 +92,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 of_iommu_init_fn init_fn = 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 aa70cbda327c..bee5d683074d 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -164,6 +164,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)
> @@ -497,6 +498,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/iommu.h b/include/linux/iommu.h
> index e6a7c9ff72f2..7b83f9f8e11d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -103,6 +103,7 @@ enum iommu_attr {
>    * @domain_get_attr: Query domain attributes
>    * @domain_set_attr: Change domain attributes
>    * @pgsize_bitmap: bitmap of supported page sizes
> + * @priv: per-instance data private to the iommu driver
>    */
>   struct iommu_ops {
>   	bool (*capable)(enum iommu_cap);
> @@ -133,6 +134,7 @@ struct iommu_ops {
>   	u32 (*domain_get_windows)(struct iommu_domain *domain);
>   
>   	unsigned long pgsize_bitmap;
> +	void *priv;
>   };
>   
>   #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 51a560f34bca..81447e0f68b5 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,12 +1,17 @@
>   #ifndef __OF_IOMMU_H
>   #define __OF_IOMMU_H
>   
> +#include <linux/iommu.h>
> +#include <linux/of.h>
> +
>   #ifdef CONFIG_OF_IOMMU
>   
>   extern int of_get_dma_window(struct device_node *dn, const char *prefix,
>   			     int index, unsigned long *busno, dma_addr_t *addr,
>   			     size_t *size);
>   
> +extern void of_iommu_init(void);
> +
>   #else
>   
>   static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
> @@ -16,6 +21,26 @@ 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_ops(struct device_node *np,
> +				    struct iommu_ops *ops)

I think it will be reasonable to change the second argument to "const 
struct iommu_ops *ops" to match other code that uses it.

> +{
> +	np->data = ops;

The above change require adding a cast:
     np->data = (struct iommu_ops *)ops;

> +}
> +
> +static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> +{
> +	return np->data;
> +}
> +
> +extern struct of_device_id __iommu_of_table;
> +
> +typedef int (*of_iommu_init_fn)(struct device_node *);
> +
> +#define IOMMU_OF_DECLARE(name, compat, fn) \
> +	_OF_DECLARE(iommu, name, compat, fn, of_iommu_init_fn)
> +
>   #endif /* __OF_IOMMU_H */

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

[RFC PATCH v4 2/8] 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 will be extended in future.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/include/asm/dma-mapping.h |  8 ++++----
 drivers/of/platform.c              | 31 +++++++++++++------------------
 include/linux/dma-mapping.h        |  7 ++-----
 3 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 85738b200023..dc3420e77758 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,12 +121,12 @@ 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, bool coherent)
 {
-	set_dma_ops(dev, &arm_coherent_dma_ops);
-	return 0;
+	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 3b64d0bf5bba..ff1f4e9afccb 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -164,6 +164,8 @@ static void of_dma_configure(struct device *dev)
 {
 	u64 dma_addr, paddr, size;
 	int ret;
+	bool coherent;
+	unsigned long offset;
 
 	/*
 	 * Set default dma-mask to 32 bit. Drivers are expected to setup
@@ -178,28 +180,21 @@ static void of_dma_configure(struct device *dev)
 	if (!dev->dma_mask)
 		dev->dma_mask = &dev->coherent_dma_mask;
 
-	/*
-	 * 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");
-	}
-
-	/*
-	 * if dma-ranges property doesn't exist - just return else
-	 * setup the dma offset
-	 */
 	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_addr = offset = 0;
+		size = dev->coherent_dma_mask;
+	} else {
+		offset = PFN_DOWN(paddr - dma_addr);
+		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
 	}
+	dev->dma_pfn_offset = offset;
+
+	coherent = of_dma_is_coherent(dev->of_node);
+	dev_dbg(dev, "device is%sdma coherent\n",
+		coherent ? " " : " not ");
 
-	/* 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, coherent);
 }
 
 /**
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index d5d388160f42..8a1560f95d4a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -129,11 +129,8 @@ 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, bool coherent) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.1

[RFC PATCH v4 3/8] iommu: add new iommu_ops callback for adding an OF device

[Will Deacon]

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

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7b83f9f8e11d..415c7613d02c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -21,6 +21,7 @@
 
 #include <linux/errno.h>
 #include <linux/err.h>
+#include <linux/of.h>
 #include <linux/types.h>
 #include <trace/events/iommu.h>
 
@@ -102,6 +103,7 @@ enum iommu_attr {
  * @remove_device: remove device from iommu grouping
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
+ * @of_xlate: add OF master IDs to iommu grouping
  * @pgsize_bitmap: bitmap of supported page sizes
  * @priv: per-instance data private to the iommu driver
  */
@@ -133,6 +135,10 @@ struct iommu_ops {
 	/* Get the numer of window per domain */
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
+#ifdef CONFIG_OF_IOMMU
+	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
+#endif
+
 	unsigned long pgsize_bitmap;
 	void *priv;
 };
-- 
2.1.1

[RFC PATCH v4 4/8] 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-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/Kconfig    |  2 +-
 drivers/iommu/of_iommu.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |  6 ++++++
 3 files changed, 40 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 89b903406968..73236d3cc955 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -18,6 +18,7 @@
  */
 
 #include <linux/export.h>
+#include <linux/iommu.h>
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
@@ -93,6 +94,38 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct iommu_ops *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct device_node *np;
+	struct iommu_ops *ops = NULL;
+	int idx = 0;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt
+	 */
+	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+					   "#iommu-cells", idx,
+					   &iommu_spec)) {
+		np = iommu_spec.np;
+		ops = of_iommu_get_ops(np);
+
+		if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
+			goto err_put_node;
+
+		of_node_put(np);
+		idx++;
+	}
+
+	return ops;
+
+err_put_node:
+	of_node_put(np);
+	return NULL;
+}
+
 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 81447e0f68b5..7ded21c0ccdf 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/iommu.h>
 #include <linux/of.h>
 
@@ -11,6 +12,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     size_t *size);
 
 extern void of_iommu_init(void);
+extern struct iommu_ops *of_iommu_configure(struct device *dev);
 
 #else
 
@@ -22,6 +24,10 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 }
 
 static inline void of_iommu_init(void) { }
+static inline struct iommu_ops *of_iommu_configure(struct device *dev)
+{
+	return NULL;
+}
 
 #endif	/* CONFIG_OF_IOMMU */
 
-- 
2.1.1

[RFC PATCH v4 5/8] 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-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 21 ++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index dc3420e77758..f3c0d953f6a2 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_ops *iommu,
+				      bool coherent)
 {
 	if (coherent)
 		set_dma_ops(dev, &arm_coherent_dma_ops);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ff1f4e9afccb..b89caf8c7586 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>
@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
 	int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_ops *iommu;
 
 	/*
 	 * Set default dma-mask to 32 bit. Drivers are expected to setup
@@ -194,7 +196,16 @@ static void of_dma_configure(struct device *dev)
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+}
+
+static void of_dma_deconfigure(struct device *dev)
+{
+	arch_teardown_dma_ops(dev);
 }
 
 /**
@@ -223,16 +234,12 @@ static struct platform_device *of_platform_device_create_pdata(
 	if (!dev)
 		goto err_clear_flag;
 
-	of_dma_configure(&dev->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->dev);
 
 	if (of_device_add(dev) != 0) {
+		of_dma_deconfigure(&dev->dev);
 		platform_device_put(dev);
 		goto err_clear_flag;
 	}
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 8a1560f95d4a..c3007cb4bfa6 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -130,7 +130,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { }
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_ops *iommu,
+				      bool coherent) { }
+#endif
+
+#ifndef arch_teardown_dma_ops
+static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.1

[RFC PATCH v4 6/8] dma-mapping: set dma segment properties in of_dma_configure

[Will Deacon]

of_dma_configure determines the size of the DMA range for a device by
either parsing the dma-ranges property or inspecting the coherent DMA
mask. This same information can be used to initialise the max segment
size and boundary_mask to a default value.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/of/platform.c           | 4 ++++
 include/linux/amba/bus.h        | 1 +
 include/linux/platform_device.h | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b89caf8c7586..0a2842d91db4 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -191,6 +191,8 @@ static void of_dma_configure(struct device *dev)
 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
 	}
 	dev->dma_pfn_offset = offset;
+	dma_set_max_seg_size(dev, size);
+	dma_set_seg_boundary(dev, size);
 
 	coherent = of_dma_is_coherent(dev->of_node);
 	dev_dbg(dev, "device is%sdma coherent\n",
@@ -236,6 +238,7 @@ static struct platform_device *of_platform_device_create_pdata(
 
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
+	dev->dev.dma_parms = &dev->dma_parms;
 	of_dma_configure(&dev->dev);
 
 	if (of_device_add(dev) != 0) {
@@ -295,6 +298,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 	dev->dev.of_node = of_node_get(node);
 	dev->dev.parent = parent;
 	dev->dev.platform_data = platform_data;
+	dev->dev.dma_parms = &dev->dma_parms;
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index c324f5700d1a..9b232eeb6c20 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	struct device_dma_parameters dma_parms;
 };
 
 struct amba_driver {
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 153d303af7eb..8dc48487b34b 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -27,6 +27,8 @@ struct platform_device {
 	u32		num_resources;
 	struct resource	*resource;
 
+	struct device_dma_parameters dma_parms;
+
 	const struct platform_device_id	*id_entry;
 	char *driver_override; /* Driver name to force a match */
 
-- 
2.1.1

Re: [RFC PATCH v4 6/8] dma-mapping: set dma segment properties in of_dma_configure

[Robin Murphy]

Hi Will,

On 14/11/14 18:56, Will Deacon wrote:
> of_dma_configure determines the size of the DMA range for a device by
> either parsing the dma-ranges property or inspecting the coherent DMA
> mask. This same information can be used to initialise the max segment
> size and boundary_mask to a default value.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Sadly, NAK on this one. After a thorough investigation and testing, 
generic code really shouldn't be touching dma_parms like this, and I 
have some corrupted files on my USB filesystem to prove it ;) It seems 
it's a bad idea to effectively change the default segment size 
universally unless you really want to go through the entire block layer 
(and who-knows-what-else) to find and fix everything that relies on it 
being 64k.

Simply dropping this patch and letting the existing defaults in 
dma_set_max_seg_size and dma_get_seg_boundary stand seems like the 
correct action.


Marek, I believe you may have some use case for this - if so, as I 
understand it your individual drivers should be setting up their own 
dma_parms directly, to tell the IOMMU they can handle it merging 
scatterlists into arbitrarily long segments, without affecting the other 
drivers that assume they'll never see >64k segments and go wrong if they 
do (including the generic USB stack, apparently).


Robin.

> ---
>   drivers/of/platform.c           | 4 ++++
>   include/linux/amba/bus.h        | 1 +
>   include/linux/platform_device.h | 2 ++
>   3 files changed, 7 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b89caf8c7586..0a2842d91db4 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -191,6 +191,8 @@ static void of_dma_configure(struct device *dev)
>   		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>   	}
>   	dev->dma_pfn_offset = offset;
> +	dma_set_max_seg_size(dev, size);
> +	dma_set_seg_boundary(dev, size);
>
>   	coherent = of_dma_is_coherent(dev->of_node);
>   	dev_dbg(dev, "device is%sdma coherent\n",
> @@ -236,6 +238,7 @@ static struct platform_device *of_platform_device_create_pdata(
>
>   	dev->dev.bus = &platform_bus_type;
>   	dev->dev.platform_data = platform_data;
> +	dev->dev.dma_parms = &dev->dma_parms;
>   	of_dma_configure(&dev->dev);
>
>   	if (of_device_add(dev) != 0) {
> @@ -295,6 +298,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>   	dev->dev.of_node = of_node_get(node);
>   	dev->dev.parent = parent;
>   	dev->dev.platform_data = platform_data;
> +	dev->dev.dma_parms = &dev->dma_parms;
>   	if (bus_id)
>   		dev_set_name(&dev->dev, "%s", bus_id);
>   	else
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index c324f5700d1a..9b232eeb6c20 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>   	struct clk		*pclk;
>   	unsigned int		periphid;
>   	unsigned int		irq[AMBA_NR_IRQS];
> +	struct device_dma_parameters dma_parms;
>   };
>
>   struct amba_driver {
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 153d303af7eb..8dc48487b34b 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -27,6 +27,8 @@ struct platform_device {
>   	u32		num_resources;
>   	struct resource	*resource;
>
> +	struct device_dma_parameters dma_parms;
> +
>   	const struct platform_device_id	*id_entry;
>   	char *driver_override; /* Driver name to force a match */
>
>

Re: [RFC PATCH v4 6/8] dma-mapping: set dma segment properties in of_dma_configure

[Will Deacon]

On Tue, Nov 25, 2014 at 01:05:36PM +0000, Robin Murphy wrote:
> On 14/11/14 18:56, Will Deacon wrote:
> > of_dma_configure determines the size of the DMA range for a device by
> > either parsing the dma-ranges property or inspecting the coherent DMA
> > mask. This same information can be used to initialise the max segment
> > size and boundary_mask to a default value.
> >
> > Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> 
> Sadly, NAK on this one. After a thorough investigation and testing, 
> generic code really shouldn't be touching dma_parms like this, and I 
> have some corrupted files on my USB filesystem to prove it ;) It seems 
> it's a bad idea to effectively change the default segment size 
> universally unless you really want to go through the entire block layer 
> (and who-knows-what-else) to find and fix everything that relies on it 
> being 64k.
> 
> Simply dropping this patch and letting the existing defaults in 
> dma_set_max_seg_size and dma_get_seg_boundary stand seems like the 
> correct action.
> 
> 
> Marek, I believe you may have some use case for this - if so, as I 
> understand it your individual drivers should be setting up their own 
> dma_parms directly, to tell the IOMMU they can handle it merging 
> scatterlists into arbitrarily long segments, without affecting the other 
> drivers that assume they'll never see >64k segments and go wrong if they 
> do (including the generic USB stack, apparently).

Ok, I'll drop this patch from the series for the time being. It's certainly
not a crucial part of the patchset and filesystem corruption is always bad.

Will

[RFC PATCH v4 7/8] 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-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/kernel/setup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index c03106378b49..1cfc94aa7fa2 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>
@@ -806,6 +807,7 @@ static int __init customize_machine(void)
 	 * machine from the device tree, if no callback is provided,
 	 * otherwise we would always need an init_machine callback.
 	 */
+	of_iommu_init();
 	if (machine_desc->init_machine)
 		machine_desc->init_machine();
 #ifdef CONFIG_OF
-- 
2.1.1

[RFC PATCH v4 8/8] 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 parameter is ignored and the normal ops are used
instead.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/include/asm/dma-mapping.h | 12 +++---
 arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
 2 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index f3c0d953f6a2..9410b7e548fc 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,14 +121,12 @@ 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 dma_base,
-				      u64 size, struct iommu_ops *iommu,
-				      bool coherent)
-{
-	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 dma_base, u64 size,
+			       struct iommu_ops *iommu, bool coherent);
+
+#define arch_teardown_dma_ops arch_teardown_dma_ops
+extern void arch_teardown_dma_ops(struct device *dev);
 
 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 e8907117861e..f15eae5e0513 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1947,9 +1947,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  *	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.
+ * 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)
@@ -1962,7 +1961,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;
@@ -1974,7 +1972,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)
 {
@@ -1989,10 +1986,79 @@ 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 struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
+{
+	return coherent ? &iommu_coherent_ops : &iommu_ops;
+}
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	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 attached device %s to IOMMU_mapping\n",
+				dev_name(dev));
+		arm_iommu_release_mapping(mapping);
+		return false;
+	}
+
+	return true;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev)
+{
+	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
+
+	arm_iommu_detach_device(dev);
+	arm_iommu_release_mapping(mapping);
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	return false;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev) { }
+
+#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 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent)
+{
+	struct dma_map_ops *dma_ops;
+
+	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	else
+		dma_ops = arm_get_dma_map_ops(coherent);
+
+	set_dma_ops(dev, dma_ops);
+}
+
+void arch_teardown_dma_ops(struct device *dev)
+{
+	arm_teardown_iommu_dma_ops(dev);
+}
-- 
2.1.1

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

[Robin Murphy]

Hi Will,

On 14/11/14 18:56, Will Deacon wrote:
> 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 parameter is ignored and the normal ops are used
> instead.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

[...]

> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +				    struct iommu_ops *iommu)
> +{
> +	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 attached device %s to IOMMU_mapping\n",
> +				dev_name(dev));
> +		arm_iommu_release_mapping(mapping);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void arm_teardown_iommu_dma_ops(struct device *dev)
> +{
> +	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
> +
> +	arm_iommu_detach_device(dev);
> +	arm_iommu_release_mapping(mapping);
> +}
> +
> +#else
> +
> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +				    struct iommu_ops *iommu)
> +{
> +	return false;
> +}
> +
> +static void arm_teardown_iommu_dma_ops(struct device *dev) { }
> +
> +#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 dma_base, u64 size,
> +			struct iommu_ops *iommu, bool coherent)
> +{
> +	struct dma_map_ops *dma_ops;
> +
> +	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))

Is the loss of a null check on iommu (compared to previous versions) 
intentional? It looks like you're always going to call 
arm_setup_iommu_dma_ops here for everything regardless, and given that 
that doesn't even look at the iommu parameter, relying on it to somehow 
fail correctly smells a bit off.


Robin.

> +		dma_ops = arm_get_iommu_dma_map_ops(coherent);
> +	else
> +		dma_ops = arm_get_dma_map_ops(coherent);
> +
> +	set_dma_ops(dev, dma_ops);
> +}
> +
> +void arch_teardown_dma_ops(struct device *dev)
> +{
> +	arm_teardown_iommu_dma_ops(dev);
> +}
>

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

[Will Deacon]

On Mon, Nov 17, 2014 at 11:29:23AM +0000, Robin Murphy wrote:
> On 14/11/14 18:56, Will Deacon wrote:
> > 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 parameter is ignored and the normal ops are used
> > instead.
> >
> > Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> 
> [...]
> 
> > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> > +				    struct iommu_ops *iommu)
> > +{
> > +	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 attached device %s to IOMMU_mapping\n",
> > +				dev_name(dev));
> > +		arm_iommu_release_mapping(mapping);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void arm_teardown_iommu_dma_ops(struct device *dev)
> > +{
> > +	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
> > +
> > +	arm_iommu_detach_device(dev);
> > +	arm_iommu_release_mapping(mapping);
> > +}
> > +
> > +#else
> > +
> > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> > +				    struct iommu_ops *iommu)
> > +{
> > +	return false;
> > +}
> > +
> > +static void arm_teardown_iommu_dma_ops(struct device *dev) { }
> > +
> > +#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 dma_base, u64 size,
> > +			struct iommu_ops *iommu, bool coherent)
> > +{
> > +	struct dma_map_ops *dma_ops;
> > +
> > +	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> 
> Is the loss of a null check on iommu (compared to previous versions) 
> intentional? It looks like you're always going to call 
> arm_setup_iommu_dma_ops here for everything regardless, and given that 
> that doesn't even look at the iommu parameter, relying on it to somehow 
> fail correctly smells a bit off.

Thanks, I'll fix that. I started writing a full implementation based off
a hypothetical ->get_default_domain callback (as suggested by Joerg), so
this is a hangover from that experiment.

Will

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Friday 14 November 2014 18:56:29 Will Deacon wrote:
> 
> Here is the fourth iteration of the RFC I've previously posted here:
> 
>   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
>   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
>   RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
> 
> Changes since RFCv3 include:
> 
>   - Drastic simplification of the data structures, so that we no longer
>     pass around lists of domains. Instead, dma-mapping is expected to
>     allocate the domain (Joerg talked about adding a get_default_domain
>     operation to iommu_ops).
> 
>   - iommu_ops is used to hold the per-instance IOMMU data
> 
>   - Configuration of DMA segments added to of_dma_configure
> 
> All feedback welcome.
> 
> 

Overall I think this is really nice, and I don't mind this going in,
I only have one issue with they way you use iommu_ops now:

At the moment, iommu_ops is a structure that can get used for any
number of iommus of the same type, but by putting per-device private
data into the same structure you have to duplicate it per instance.

I think rather than adding a .priv pointer to iommu_ops, we should do
the same thing that a lot of other subsystems have:

/* generic structure */
struct iommu {
	struct iommu_ops *ops;
	/* possibly other generic per-instance members */
};

/* driver specific structure */
struct arm_smmu {
	struct iommu iommu;

	/* smmu specific members */
};
static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
{
	return container_of(iommu, struct arm_smmu, iommu);
}

	Arnd

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

Hi Arnd,

Thanks for having a look.

On Fri, Nov 14, 2014 at 07:11:23PM +0000, Arnd Bergmann wrote:
> On Friday 14 November 2014 18:56:29 Will Deacon wrote:
> > 
> > Here is the fourth iteration of the RFC I've previously posted here:
> > 
> >   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
> >   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
> >   RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
> > 
> > Changes since RFCv3 include:
> > 
> >   - Drastic simplification of the data structures, so that we no longer
> >     pass around lists of domains. Instead, dma-mapping is expected to
> >     allocate the domain (Joerg talked about adding a get_default_domain
> >     operation to iommu_ops).
> > 
> >   - iommu_ops is used to hold the per-instance IOMMU data
> > 
> >   - Configuration of DMA segments added to of_dma_configure
> > 
> > All feedback welcome.
> > 
> > 
> 
> Overall I think this is really nice, and I don't mind this going in,
> I only have one issue with they way you use iommu_ops now:

Hehe, I thought you might have something to say about that. I also had second
thoughts, but decided it wasn't worse than what we already have (more below).

> At the moment, iommu_ops is a structure that can get used for any
> number of iommus of the same type, but by putting per-device private
> data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
different implementations of the same IOMMU. I think we already have this in
Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

> I think rather than adding a .priv pointer to iommu_ops, we should do
> the same thing that a lot of other subsystems have:
> 
> /* generic structure */
> struct iommu {
> 	struct iommu_ops *ops;
> 	/* possibly other generic per-instance members */
> };
> 
> /* driver specific structure */
> struct arm_smmu {
> 	struct iommu iommu;
> 
> 	/* smmu specific members */
> };
> static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
> {
> 	return container_of(iommu, struct arm_smmu, iommu);
> }

Regardless of the arguments above, I think this layout is cleaner. We could
also move the pgsize_bitmap into struct iommu in that case, however, that
would be a more invasive patch series than I what I currently have.

If I do another version of the patch, I can easily add a struct iommu and
stash that in the device_node data for the IOMMU instead of directly
putting the ops there. That's at least a step in the right direction.

Cheers,

Will

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Arnd Bergmann]

On Friday 14 November 2014 19:27:54 Will Deacon wrote:
> 
> > At the moment, iommu_ops is a structure that can get used for any
> > number of iommus of the same type, but by putting per-device private
> > data into the same structure you have to duplicate it per instance.
> 
> I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
> different implementations of the same IOMMU. I think we already have this in
> Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course,
not in iommu_ops.

> > I think rather than adding a .priv pointer to iommu_ops, we should do
> > the same thing that a lot of other subsystems have:
> > 
> > /* generic structure */
> > struct iommu {
> >       struct iommu_ops *ops;
> >       /* possibly other generic per-instance members */
> > };
> > 
> > /* driver specific structure */
> > struct arm_smmu {
> >       struct iommu iommu;
> > 
> >       /* smmu specific members */
> > };
> > static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
> > {
> >       return container_of(iommu, struct arm_smmu, iommu);
> > }
> 
> Regardless of the arguments above, I think this layout is cleaner. We could
> also move the pgsize_bitmap into struct iommu in that case, however, that
> would be a more invasive patch series than I what I currently have.

Right, it can be done as a follow-up. It's certainly not urgent.
 
> If I do another version of the patch, I can easily add a struct iommu and
> stash that in the device_node data for the IOMMU instead of directly
> putting the ops there. That's at least a step in the right direction.

Sounds good, yes. Alternatively, why not do the pointer in the opposite
direction and put a 'struct device_node *dn' and a list_head into
'struct iommu'. This means you will have to traverse the list of iommus
in of_iommu_get_ops, but I think it's safe to assume this is a short
list and it gets walked rarely. It would be somewhat cleaner not to have
to use device_node->data this way.

	Arnd

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

(resurrecting an old thread)

On Fri, Nov 14, 2014 at 08:01:56PM +0000, Arnd Bergmann wrote:
> On Friday 14 November 2014 19:27:54 Will Deacon wrote:
> > > At the moment, iommu_ops is a structure that can get used for any
> > > number of iommus of the same type, but by putting per-device private
> > > data into the same structure you have to duplicate it per instance.
> > 
> > I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
> > different implementations of the same IOMMU. I think we already have this in
> > Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).
> 
> Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course,
> not in iommu_ops.

Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
just move pgsize_bitmap into the iommu_domain, which I think makes a lot
more sense anyway.

diff below seems to do the trick. The next step would be postponing the
pgsize_bitmap initialisation until device attach for the ARM SMMU, since
it's at that point that we know the active page table format (and
therefore the supported page sizes).

Will

--->8

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98024856df07..ab9702d01e9a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain *dom)
 	dom->geometry.aperture_end   = ~0ULL;
 	dom->geometry.force_aperture = true;
 
+	dom->pgsize_bitmap = AMD_IOMMU_PGSIZES;
 	return 0;
 
 out_free:
@@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
 	.unmap = amd_iommu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = amd_iommu_iova_to_phys,
-	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 };
 
 /*****************************************************************************
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6cd47b75286f..898fab8d10a0 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain *domain)
 
 	spin_lock_init(&smmu_domain->lock);
 	domain->priv = smmu_domain;
+	domain->pgsize_bitmap = (SECTION_SIZE |
+				 ARM_SMMU_PTE_CONT_SIZE |
+				 PAGE_SIZE);
 	return 0;
 
 out_free_domain:
@@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
 	.remove_device		= arm_smmu_remove_device,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
-	.pgsize_bitmap		= (SECTION_SIZE |
-				   ARM_SMMU_PTE_CONT_SIZE |
-				   PAGE_SIZE),
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 7ce52737c7a1..404cad25db9b 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain *domain)
 	domain->geometry.aperture_end   = ~0UL;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
+
 	domain->priv = priv;
 	return 0;
 
@@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.iova_to_phys = exynos_iommu_iova_to_phys,
 	.add_device = exynos_iommu_add_device,
 	.remove_device = exynos_iommu_remove_device,
-	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 };
 
 static int __init exynos_iommu_init(void)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 40dfbc0444c0..e2b0f34baa9d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain *domain)
 	domain->geometry.aperture_end   = __DOMAIN_MAX_ADDR(dmar_domain->gaw);
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = INTEL_IOMMU_PGSIZES;
 	return 0;
 }
 
@@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
 	.iova_to_phys	= intel_iommu_iova_to_phys,
 	.add_device	= intel_iommu_add_device,
 	.remove_device	= intel_iommu_remove_device,
-	.pgsize_bitmap	= INTEL_IOMMU_PGSIZES,
 };
 
 static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f7718d73e984..b8b6b0bc6951 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain *domain,
 	pgsize = (1UL << (pgsize_idx + 1)) - 1;
 
 	/* throw away page sizes not supported by the hardware */
-	pgsize &= domain->ops->pgsize_bitmap;
+	pgsize &= domain->pgsize_bitmap;
 
 	/* make sure we're still sane */
 	BUG_ON(!pgsize);
@@ -1046,11 +1046,11 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 	int ret = 0;
 
 	if (unlikely(domain->ops->map == NULL ||
-		     domain->ops->pgsize_bitmap == 0UL))
+		     domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	/*
 	 * both the virtual address and the physical one, as well as
@@ -1096,11 +1096,11 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 	unsigned int min_pagesz;
 
 	if (unlikely(domain->ops->unmap == NULL ||
-		     domain->ops->pgsize_bitmap == 0UL))
+		     domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	/*
 	 * The virtual address, as well as the size of the mapping, must be
@@ -1146,10 +1146,10 @@ size_t default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
 	unsigned int i, min_pagesz;
 	int ret;
 
-	if (unlikely(domain->ops->pgsize_bitmap == 0UL))
+	if (unlikely(domain->pgsize_bitmap == 0UL))
 		return 0;
 
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	for_each_sg(sg, s, nents, i) {
 		phys_addr_t phys = page_to_phys(sg_page(s)) + s->offset;
@@ -1230,7 +1230,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 		break;
 	case DOMAIN_ATTR_PAGING:
 		paging  = data;
-		*paging = (domain->ops->pgsize_bitmap != 0UL);
+		*paging = (domain->pgsize_bitmap != 0UL);
 		break;
 	case DOMAIN_ATTR_WINDOWS:
 		count = data;
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 748693192c20..7e6c6a7afd81 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -872,6 +872,7 @@ static int ipmmu_domain_init(struct iommu_domain *io_domain)
 
 	io_domain->priv = domain;
 	domain->io_domain = io_domain;
+	domain->pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K;
 
 	return 0;
 }
@@ -1131,7 +1132,6 @@ static const struct iommu_ops ipmmu_ops = {
 	.iova_to_phys = ipmmu_iova_to_phys,
 	.add_device = ipmmu_add_device,
 	.remove_device = ipmmu_remove_device,
-	.pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index e1b05379ca0e..8b623dce8161 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -230,6 +230,8 @@ static int msm_iommu_domain_init(struct iommu_domain *domain)
 	domain->geometry.aperture_end   = (1ULL << 32) - 1;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = MSM_IOMMU_PGSIZES;
+
 	return 0;
 
 fail_nomem:
@@ -682,7 +684,6 @@ static const struct iommu_ops msm_iommu_ops = {
 	.unmap = msm_iommu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
-	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 };
 
 static int __init get_tex_class(int icp, int ocp, int mt, int nos)
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bbb7dcef02d3..bc4054967f18 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1250,6 +1250,8 @@ static int omap_iommu_domain_init(struct iommu_domain *domain)
 	domain->geometry.aperture_end   = (1ULL << 32) - 1;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = OMAP_IOMMU_PGSIZES;
+
 	return 0;
 
 fail_nomem:
@@ -1368,7 +1370,6 @@ static const struct iommu_ops omap_iommu_ops = {
 	.iova_to_phys	= omap_iommu_iova_to_phys,
 	.add_device	= omap_iommu_add_device,
 	.remove_device	= omap_iommu_remove_device,
-	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 };
 
 static int __init omap_iommu_init(void)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 6a8b1ec4a48a..5ab63e51e1c0 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -828,6 +828,7 @@ static int rk_iommu_domain_init(struct iommu_domain *domain)
 	INIT_LIST_HEAD(&rk_domain->iommus);
 
 	domain->priv = rk_domain;
+	domain->pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP;
 
 	return 0;
 err_dt:
@@ -961,7 +962,6 @@ static const struct iommu_ops rk_iommu_ops = {
 	.add_device = rk_iommu_add_device,
 	.remove_device = rk_iommu_remove_device,
 	.iova_to_phys = rk_iommu_iova_to_phys,
-	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 };
 
 static int rk_iommu_probe(struct platform_device *pdev)
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index f1b00774e4de..552bde09a69b 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -101,6 +101,7 @@ static int shmobile_iommu_domain_init(struct iommu_domain *domain)
 	spin_lock_init(&sh_domain->attached_list_lock);
 	INIT_LIST_HEAD(&sh_domain->attached_list);
 	domain->priv = sh_domain;
+	domain->pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K;
 	return 0;
 }
 
@@ -364,7 +365,6 @@ static const struct iommu_ops shmobile_iommu_ops = {
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = shmobile_iommu_iova_to_phys,
 	.add_device = shmobile_iommu_add_device,
-	.pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
 };
 
 int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu)
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index f722a0c466cf..6404aba9667a 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -218,6 +218,7 @@ out:
 
 static int gart_iommu_domain_init(struct iommu_domain *domain)
 {
+	domain->pgsize_bitmap = GART_IOMMU_PGSIZES;
 	return 0;
 }
 
@@ -317,7 +318,6 @@ static const struct iommu_ops gart_iommu_ops = {
 	.map		= gart_iommu_map,
 	.unmap		= gart_iommu_unmap,
 	.iova_to_phys	= gart_iommu_iova_to_phys,
-	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 };
 
 static int tegra_gart_suspend(struct device *dev)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6e134c7c227f..a423ed9a19da 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -265,6 +265,7 @@ static int tegra_smmu_domain_init(struct iommu_domain *domain)
 		pd[i] = 0;
 
 	domain->priv = as;
+	domain->pgsize_bitmap = SZ_4K;
 
 	return 0;
 }
@@ -643,8 +644,6 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.unmap = tegra_smmu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = tegra_smmu_iova_to_phys,
-
-	.pgsize_bitmap = SZ_4K,
 };
 
 static void tegra_smmu_ahb_enable(void)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4a9d666f1e91..3bb72499e4e1 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -386,7 +386,7 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(domain, &iommu->domain_list, next)
-		bitmap &= domain->domain->ops->pgsize_bitmap;
+		bitmap &= domain->domain->pgsize_bitmap;
 	mutex_unlock(&iommu->lock);
 
 	return bitmap;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 38daa453f2e5..ffc023e24b78 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -53,6 +53,7 @@ struct iommu_domain_geometry {
 
 struct iommu_domain {
 	const struct iommu_ops *ops;
+	unsigned long pgsize_bitmap;	/* Bitmap of supported page sizes */
 	void *priv;
 	iommu_fault_handler_t handler;
 	void *handler_token;
@@ -108,7 +109,6 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @of_xlate: add OF master IDs to iommu grouping
- * @pgsize_bitmap: bitmap of supported page sizes
  * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {
@@ -144,9 +144,6 @@ struct iommu_ops {
 #ifdef CONFIG_OF_IOMMU
 	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
 #endif
-
-	unsigned long pgsize_bitmap;
-	void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Laurent Pinchart]

Hi Will,

Thank you for the patch.

On Monday 19 January 2015 16:06:20 Will Deacon wrote:
> (resurrecting an old thread)
> 
> On Fri, Nov 14, 2014 at 08:01:56PM +0000, Arnd Bergmann wrote:
> > On Friday 14 November 2014 19:27:54 Will Deacon wrote:
> > > > At the moment, iommu_ops is a structure that can get used for any
> > > > number of iommus of the same type, but by putting per-device private
> > > > data into the same structure you have to duplicate it per instance.
> > > 
> > > I'm not sure I agree -- the pgsize_bitmap, for example, could vary
> > > between different implementations of the same IOMMU. I think we already
> > > have this in Juno (some SMMUs can only do 64k pages, whilst others can
> > > do 4k and 64k).
> >
> > Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
> > course, not in iommu_ops.
> 
> Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
> just move pgsize_bitmap into the iommu_domain, which I think makes a lot
> more sense anyway.

The code looks good to me, but what does it have to do with of_iommu_get_ops() 
having its own list of iommu_ops ?

> diff below seems to do the trick. The next step would be postponing the
> pgsize_bitmap initialisation until device attach for the ARM SMMU, since
> it's at that point that we know the active page table format (and
> therefore the supported page sizes).
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 98024856df07..ab9702d01e9a 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
> *dom) dom->geometry.aperture_end   = ~0ULL;
>  	dom->geometry.force_aperture = true;
> 
> +	dom->pgsize_bitmap = AMD_IOMMU_PGSIZES;
>  	return 0;
> 
>  out_free:
> @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
>  	.unmap = amd_iommu_unmap,
>  	.map_sg = default_iommu_map_sg,
>  	.iova_to_phys = amd_iommu_iova_to_phys,
> -	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
>  };
> 
>  /**************************************************************************
> *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 6cd47b75286f..898fab8d10a0 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
> *domain)
> 
>  	spin_lock_init(&smmu_domain->lock);
>  	domain->priv = smmu_domain;
> +	domain->pgsize_bitmap = (SECTION_SIZE |
> +				 ARM_SMMU_PTE_CONT_SIZE |
> +				 PAGE_SIZE);
>  	return 0;
> 
>  out_free_domain:
> @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
>  	.remove_device		= arm_smmu_remove_device,
>  	.domain_get_attr	= arm_smmu_domain_get_attr,
>  	.domain_set_attr	= arm_smmu_domain_set_attr,
> -	.pgsize_bitmap		= (SECTION_SIZE |
> -				   ARM_SMMU_PTE_CONT_SIZE |
> -				   PAGE_SIZE),
>  };
> 
>  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 7ce52737c7a1..404cad25db9b 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
> *domain) domain->geometry.aperture_end   = ~0UL;
>  	domain->geometry.force_aperture = true;
> 
> +	domain->pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
> +
>  	domain->priv = priv;
>  	return 0;
> 
> @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
>  	.iova_to_phys = exynos_iommu_iova_to_phys,
>  	.add_device = exynos_iommu_add_device,
>  	.remove_device = exynos_iommu_remove_device,
> -	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
>  };
> 
>  static int __init exynos_iommu_init(void)
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 40dfbc0444c0..e2b0f34baa9d 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
> *domain) domain->geometry.aperture_end   =
> __DOMAIN_MAX_ADDR(dmar_domain->gaw); domain->geometry.force_aperture =
> true;
> 
> +	domain->pgsize_bitmap = INTEL_IOMMU_PGSIZES;
>  	return 0;
>  }
> 
> @@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
>  	.iova_to_phys	= intel_iommu_iova_to_phys,
>  	.add_device	= intel_iommu_add_device,
>  	.remove_device	= intel_iommu_remove_device,
> -	.pgsize_bitmap	= INTEL_IOMMU_PGSIZES,
>  };
> 
>  static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f7718d73e984..b8b6b0bc6951 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain
> *domain, pgsize = (1UL << (pgsize_idx + 1)) - 1;
> 
>  	/* throw away page sizes not supported by the hardware */
> -	pgsize &= domain->ops->pgsize_bitmap;
> +	pgsize &= domain->pgsize_bitmap;
> 
>  	/* make sure we're still sane */
>  	BUG_ON(!pgsize);
> @@ -1046,11 +1046,11 @@ int iommu_map(struct iommu_domain *domain, unsigned
> long iova, int ret = 0;
> 
>  	if (unlikely(domain->ops->map == NULL ||
> -		     domain->ops->pgsize_bitmap == 0UL))
> +		     domain->pgsize_bitmap == 0UL))
>  		return -ENODEV;
> 
>  	/* find out the minimum page size supported */
> -	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> +	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> 
>  	/*
>  	 * both the virtual address and the physical one, as well as
> @@ -1096,11 +1096,11 @@ size_t iommu_unmap(struct iommu_domain *domain,
> unsigned long iova, size_t size) unsigned int min_pagesz;
> 
>  	if (unlikely(domain->ops->unmap == NULL ||
> -		     domain->ops->pgsize_bitmap == 0UL))
> +		     domain->pgsize_bitmap == 0UL))
>  		return -ENODEV;
> 
>  	/* find out the minimum page size supported */
> -	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> +	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> 
>  	/*
>  	 * The virtual address, as well as the size of the mapping, must be
> @@ -1146,10 +1146,10 @@ size_t default_iommu_map_sg(struct iommu_domain
> *domain, unsigned long iova, unsigned int i, min_pagesz;
>  	int ret;
> 
> -	if (unlikely(domain->ops->pgsize_bitmap == 0UL))
> +	if (unlikely(domain->pgsize_bitmap == 0UL))
>  		return 0;
> 
> -	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> +	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> 
>  	for_each_sg(sg, s, nents, i) {
>  		phys_addr_t phys = page_to_phys(sg_page(s)) + s->offset;
> @@ -1230,7 +1230,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
> break;
>  	case DOMAIN_ATTR_PAGING:
>  		paging  = data;
> -		*paging = (domain->ops->pgsize_bitmap != 0UL);
> +		*paging = (domain->pgsize_bitmap != 0UL);
>  		break;
>  	case DOMAIN_ATTR_WINDOWS:
>  		count = data;
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 748693192c20..7e6c6a7afd81 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -872,6 +872,7 @@ static int ipmmu_domain_init(struct iommu_domain
> *io_domain)
> 
>  	io_domain->priv = domain;
>  	domain->io_domain = io_domain;
> +	domain->pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K;
> 
>  	return 0;
>  }
> @@ -1131,7 +1132,6 @@ static const struct iommu_ops ipmmu_ops = {
>  	.iova_to_phys = ipmmu_iova_to_phys,
>  	.add_device = ipmmu_add_device,
>  	.remove_device = ipmmu_remove_device,
> -	.pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K,
>  };
> 
>  /*
> ---------------------------------------------------------------------------
> -- diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index
> e1b05379ca0e..8b623dce8161 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -230,6 +230,8 @@ static int msm_iommu_domain_init(struct iommu_domain
> *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
>  	domain->geometry.force_aperture = true;
> 
> +	domain->pgsize_bitmap = MSM_IOMMU_PGSIZES;
> +
>  	return 0;
> 
>  fail_nomem:
> @@ -682,7 +684,6 @@ static const struct iommu_ops msm_iommu_ops = {
>  	.unmap = msm_iommu_unmap,
>  	.map_sg = default_iommu_map_sg,
>  	.iova_to_phys = msm_iommu_iova_to_phys,
> -	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
>  };
> 
>  static int __init get_tex_class(int icp, int ocp, int mt, int nos)
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index bbb7dcef02d3..bc4054967f18 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1250,6 +1250,8 @@ static int omap_iommu_domain_init(struct iommu_domain
> *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
>  	domain->geometry.force_aperture = true;
> 
> +	domain->pgsize_bitmap = OMAP_IOMMU_PGSIZES;
> +
>  	return 0;
> 
>  fail_nomem:
> @@ -1368,7 +1370,6 @@ static const struct iommu_ops omap_iommu_ops = {
>  	.iova_to_phys	= omap_iommu_iova_to_phys,
>  	.add_device	= omap_iommu_add_device,
>  	.remove_device	= omap_iommu_remove_device,
> -	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
>  };
> 
>  static int __init omap_iommu_init(void)
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 6a8b1ec4a48a..5ab63e51e1c0 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -828,6 +828,7 @@ static int rk_iommu_domain_init(struct iommu_domain
> *domain) INIT_LIST_HEAD(&rk_domain->iommus);
> 
>  	domain->priv = rk_domain;
> +	domain->pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP;
> 
>  	return 0;
>  err_dt:
> @@ -961,7 +962,6 @@ static const struct iommu_ops rk_iommu_ops = {
>  	.add_device = rk_iommu_add_device,
>  	.remove_device = rk_iommu_remove_device,
>  	.iova_to_phys = rk_iommu_iova_to_phys,
> -	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
>  };
> 
>  static int rk_iommu_probe(struct platform_device *pdev)
> diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
> index f1b00774e4de..552bde09a69b 100644
> --- a/drivers/iommu/shmobile-iommu.c
> +++ b/drivers/iommu/shmobile-iommu.c
> @@ -101,6 +101,7 @@ static int shmobile_iommu_domain_init(struct
> iommu_domain *domain) spin_lock_init(&sh_domain->attached_list_lock);
>  	INIT_LIST_HEAD(&sh_domain->attached_list);
>  	domain->priv = sh_domain;
> +	domain->pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K;
>  	return 0;
>  }
> 
> @@ -364,7 +365,6 @@ static const struct iommu_ops shmobile_iommu_ops = {
>  	.map_sg = default_iommu_map_sg,
>  	.iova_to_phys = shmobile_iommu_iova_to_phys,
>  	.add_device = shmobile_iommu_add_device,
> -	.pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
>  };
> 
>  int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu)
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index f722a0c466cf..6404aba9667a 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -218,6 +218,7 @@ out:
> 
>  static int gart_iommu_domain_init(struct iommu_domain *domain)
>  {
> +	domain->pgsize_bitmap = GART_IOMMU_PGSIZES;
>  	return 0;
>  }
> 
> @@ -317,7 +318,6 @@ static const struct iommu_ops gart_iommu_ops = {
>  	.map		= gart_iommu_map,
>  	.unmap		= gart_iommu_unmap,
>  	.iova_to_phys	= gart_iommu_iova_to_phys,
> -	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
>  };
> 
>  static int tegra_gart_suspend(struct device *dev)
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 6e134c7c227f..a423ed9a19da 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -265,6 +265,7 @@ static int tegra_smmu_domain_init(struct iommu_domain
> *domain) pd[i] = 0;
> 
>  	domain->priv = as;
> +	domain->pgsize_bitmap = SZ_4K;
> 
>  	return 0;
>  }
> @@ -643,8 +644,6 @@ static const struct iommu_ops tegra_smmu_ops = {
>  	.unmap = tegra_smmu_unmap,
>  	.map_sg = default_iommu_map_sg,
>  	.iova_to_phys = tegra_smmu_iova_to_phys,
> -
> -	.pgsize_bitmap = SZ_4K,
>  };
> 
>  static void tegra_smmu_ahb_enable(void)
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 4a9d666f1e91..3bb72499e4e1 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -386,7 +386,7 @@ static unsigned long vfio_pgsize_bitmap(struct
> vfio_iommu *iommu)
> 
>  	mutex_lock(&iommu->lock);
>  	list_for_each_entry(domain, &iommu->domain_list, next)
> -		bitmap &= domain->domain->ops->pgsize_bitmap;
> +		bitmap &= domain->domain->pgsize_bitmap;
>  	mutex_unlock(&iommu->lock);
> 
>  	return bitmap;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 38daa453f2e5..ffc023e24b78 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -53,6 +53,7 @@ struct iommu_domain_geometry {
> 
>  struct iommu_domain {
>  	const struct iommu_ops *ops;
> +	unsigned long pgsize_bitmap;	/* Bitmap of supported page sizes */
>  	void *priv;
>  	iommu_fault_handler_t handler;
>  	void *handler_token;
> @@ -108,7 +109,6 @@ enum iommu_attr {
>   * @domain_get_attr: Query domain attributes
>   * @domain_set_attr: Change domain attributes
>   * @of_xlate: add OF master IDs to iommu grouping
> - * @pgsize_bitmap: bitmap of supported page sizes
>   * @priv: per-instance data private to the iommu driver
>   */
>  struct iommu_ops {
> @@ -144,9 +144,6 @@ struct iommu_ops {
>  #ifdef CONFIG_OF_IOMMU
>  	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
>  #endif
> -
> -	unsigned long pgsize_bitmap;
> -	void *priv;
>  };
> 
>  #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Tue, Jan 20, 2015 at 04:56:03PM +0000, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> Thank you for the patch.
> 
> On Monday 19 January 2015 16:06:20 Will Deacon wrote:
> > (resurrecting an old thread)
> >
> > On Fri, Nov 14, 2014 at 08:01:56PM +0000, Arnd Bergmann wrote:
> > > On Friday 14 November 2014 19:27:54 Will Deacon wrote:
> > > > > At the moment, iommu_ops is a structure that can get used for any
> > > > > number of iommus of the same type, but by putting per-device private
> > > > > data into the same structure you have to duplicate it per instance.
> > > >
> > > > I'm not sure I agree -- the pgsize_bitmap, for example, could vary
> > > > between different implementations of the same IOMMU. I think we already
> > > > have this in Juno (some SMMUs can only do 64k pages, whilst others can
> > > > do 4k and 64k).
> > >
> > > Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
> > > course, not in iommu_ops.
> >
> > Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
> > just move pgsize_bitmap into the iommu_domain, which I think makes a lot
> > more sense anyway.
> 
> The code looks good to me, but what does it have to do with of_iommu_get_ops()
> having its own list of iommu_ops ?

So, the initial patches for of_iommu_configure/of_xlate made use of a void
*priv field in struct iommu_ops (which actually made it to mainline but
isn't used). This was replaced by the list maintained by of_iommu_get_ops,
but the discussion highlighted that iommu_ops is not per-IOMMU instance and
therefore shouldn't contain the pgsize_bitmap, which can (and does) vary
between different IOMMU instances in a running system.

This patch is an effort to move the pgsize_bitmap field out of iommu_ops
(leaving that structure to contain only function pointers) and into
iommu_domain, where I think it makes more sense.

Will

> > diff below seems to do the trick. The next step would be postponing the
> > pgsize_bitmap initialisation until device attach for the ARM SMMU, since
> > it's at that point that we know the active page table format (and
> > therefore the supported page sizes).
> >
> > Will
> >
> > --->8
> >
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 98024856df07..ab9702d01e9a 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
> > *dom) dom->geometry.aperture_end   = ~0ULL;
> >       dom->geometry.force_aperture = true;
> >
> > +     dom->pgsize_bitmap = AMD_IOMMU_PGSIZES;
> >       return 0;
> >
> >  out_free:
> > @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
> >       .unmap = amd_iommu_unmap,
> >       .map_sg = default_iommu_map_sg,
> >       .iova_to_phys = amd_iommu_iova_to_phys,
> > -     .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
> >  };
> >
> >  /**************************************************************************
> > *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 6cd47b75286f..898fab8d10a0 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
> > *domain)
> >
> >       spin_lock_init(&smmu_domain->lock);
> >       domain->priv = smmu_domain;
> > +     domain->pgsize_bitmap = (SECTION_SIZE |
> > +                              ARM_SMMU_PTE_CONT_SIZE |
> > +                              PAGE_SIZE);
> >       return 0;
> >
> >  out_free_domain:
> > @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
> >       .remove_device          = arm_smmu_remove_device,
> >       .domain_get_attr        = arm_smmu_domain_get_attr,
> >       .domain_set_attr        = arm_smmu_domain_set_attr,
> > -     .pgsize_bitmap          = (SECTION_SIZE |
> > -                                ARM_SMMU_PTE_CONT_SIZE |
> > -                                PAGE_SIZE),
> >  };
> >
> >  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> > index 7ce52737c7a1..404cad25db9b 100644
> > --- a/drivers/iommu/exynos-iommu.c
> > +++ b/drivers/iommu/exynos-iommu.c
> > @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
> > *domain) domain->geometry.aperture_end   = ~0UL;
> >       domain->geometry.force_aperture = true;
> >
> > +     domain->pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
> > +
> >       domain->priv = priv;
> >       return 0;
> >
> > @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
> >       .iova_to_phys = exynos_iommu_iova_to_phys,
> >       .add_device = exynos_iommu_add_device,
> >       .remove_device = exynos_iommu_remove_device,
> > -     .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
> >  };
> >
> >  static int __init exynos_iommu_init(void)
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 40dfbc0444c0..e2b0f34baa9d 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
> > *domain) domain->geometry.aperture_end   =
> > __DOMAIN_MAX_ADDR(dmar_domain->gaw); domain->geometry.force_aperture =
> > true;
> >
> > +     domain->pgsize_bitmap = INTEL_IOMMU_PGSIZES;
> >       return 0;
> >  }
> >
> > @@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
> >       .iova_to_phys   = intel_iommu_iova_to_phys,
> >       .add_device     = intel_iommu_add_device,
> >       .remove_device  = intel_iommu_remove_device,
> > -     .pgsize_bitmap  = INTEL_IOMMU_PGSIZES,
> >  };
> >
> >  static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index f7718d73e984..b8b6b0bc6951 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain
> > *domain, pgsize = (1UL << (pgsize_idx + 1)) - 1;
> >
> >       /* throw away page sizes not supported by the hardware */
> > -     pgsize &= domain->ops->pgsize_bitmap;
> > +     pgsize &= domain->pgsize_bitmap;
> >
> >       /* make sure we're still sane */
> >       BUG_ON(!pgsize);
> > @@ -1046,11 +1046,11 @@ int iommu_map(struct iommu_domain *domain, unsigned
> > long iova, int ret = 0;
> >
> >       if (unlikely(domain->ops->map == NULL ||
> > -                  domain->ops->pgsize_bitmap == 0UL))
> > +                  domain->pgsize_bitmap == 0UL))
> >               return -ENODEV;
> >
> >       /* find out the minimum page size supported */
> > -     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> > +     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> >
> >       /*
> >        * both the virtual address and the physical one, as well as
> > @@ -1096,11 +1096,11 @@ size_t iommu_unmap(struct iommu_domain *domain,
> > unsigned long iova, size_t size) unsigned int min_pagesz;
> >
> >       if (unlikely(domain->ops->unmap == NULL ||
> > -                  domain->ops->pgsize_bitmap == 0UL))
> > +                  domain->pgsize_bitmap == 0UL))
> >               return -ENODEV;
> >
> >       /* find out the minimum page size supported */
> > -     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> > +     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> >
> >       /*
> >        * The virtual address, as well as the size of the mapping, must be
> > @@ -1146,10 +1146,10 @@ size_t default_iommu_map_sg(struct iommu_domain
> > *domain, unsigned long iova, unsigned int i, min_pagesz;
> >       int ret;
> >
> > -     if (unlikely(domain->ops->pgsize_bitmap == 0UL))
> > +     if (unlikely(domain->pgsize_bitmap == 0UL))
> >               return 0;
> >
> > -     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
> > +     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> >
> >       for_each_sg(sg, s, nents, i) {
> >               phys_addr_t phys = page_to_phys(sg_page(s)) + s->offset;
> > @@ -1230,7 +1230,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
> > break;
> >       case DOMAIN_ATTR_PAGING:
> >               paging  = data;
> > -             *paging = (domain->ops->pgsize_bitmap != 0UL);
> > +             *paging = (domain->pgsize_bitmap != 0UL);
> >               break;
> >       case DOMAIN_ATTR_WINDOWS:
> >               count = data;
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index 748693192c20..7e6c6a7afd81 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -872,6 +872,7 @@ static int ipmmu_domain_init(struct iommu_domain
> > *io_domain)
> >
> >       io_domain->priv = domain;
> >       domain->io_domain = io_domain;
> > +     domain->pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K;
> >
> >       return 0;
> >  }
> > @@ -1131,7 +1132,6 @@ static const struct iommu_ops ipmmu_ops = {
> >       .iova_to_phys = ipmmu_iova_to_phys,
> >       .add_device = ipmmu_add_device,
> >       .remove_device = ipmmu_remove_device,
> > -     .pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K,
> >  };
> >
> >  /*
> > ---------------------------------------------------------------------------
> > -- diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index
> > e1b05379ca0e..8b623dce8161 100644
> > --- a/drivers/iommu/msm_iommu.c
> > +++ b/drivers/iommu/msm_iommu.c
> > @@ -230,6 +230,8 @@ static int msm_iommu_domain_init(struct iommu_domain
> > *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
> >       domain->geometry.force_aperture = true;
> >
> > +     domain->pgsize_bitmap = MSM_IOMMU_PGSIZES;
> > +
> >       return 0;
> >
> >  fail_nomem:
> > @@ -682,7 +684,6 @@ static const struct iommu_ops msm_iommu_ops = {
> >       .unmap = msm_iommu_unmap,
> >       .map_sg = default_iommu_map_sg,
> >       .iova_to_phys = msm_iommu_iova_to_phys,
> > -     .pgsize_bitmap = MSM_IOMMU_PGSIZES,
> >  };
> >
> >  static int __init get_tex_class(int icp, int ocp, int mt, int nos)
> > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> > index bbb7dcef02d3..bc4054967f18 100644
> > --- a/drivers/iommu/omap-iommu.c
> > +++ b/drivers/iommu/omap-iommu.c
> > @@ -1250,6 +1250,8 @@ static int omap_iommu_domain_init(struct iommu_domain
> > *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
> >       domain->geometry.force_aperture = true;
> >
> > +     domain->pgsize_bitmap = OMAP_IOMMU_PGSIZES;
> > +
> >       return 0;
> >
> >  fail_nomem:
> > @@ -1368,7 +1370,6 @@ static const struct iommu_ops omap_iommu_ops = {
> >       .iova_to_phys   = omap_iommu_iova_to_phys,
> >       .add_device     = omap_iommu_add_device,
> >       .remove_device  = omap_iommu_remove_device,
> > -     .pgsize_bitmap  = OMAP_IOMMU_PGSIZES,
> >  };
> >
> >  static int __init omap_iommu_init(void)
> > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> > index 6a8b1ec4a48a..5ab63e51e1c0 100644
> > --- a/drivers/iommu/rockchip-iommu.c
> > +++ b/drivers/iommu/rockchip-iommu.c
> > @@ -828,6 +828,7 @@ static int rk_iommu_domain_init(struct iommu_domain
> > *domain) INIT_LIST_HEAD(&rk_domain->iommus);
> >
> >       domain->priv = rk_domain;
> > +     domain->pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP;
> >
> >       return 0;
> >  err_dt:
> > @@ -961,7 +962,6 @@ static const struct iommu_ops rk_iommu_ops = {
> >       .add_device = rk_iommu_add_device,
> >       .remove_device = rk_iommu_remove_device,
> >       .iova_to_phys = rk_iommu_iova_to_phys,
> > -     .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
> >  };
> >
> >  static int rk_iommu_probe(struct platform_device *pdev)
> > diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
> > index f1b00774e4de..552bde09a69b 100644
> > --- a/drivers/iommu/shmobile-iommu.c
> > +++ b/drivers/iommu/shmobile-iommu.c
> > @@ -101,6 +101,7 @@ static int shmobile_iommu_domain_init(struct
> > iommu_domain *domain) spin_lock_init(&sh_domain->attached_list_lock);
> >       INIT_LIST_HEAD(&sh_domain->attached_list);
> >       domain->priv = sh_domain;
> > +     domain->pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K;
> >       return 0;
> >  }
> >
> > @@ -364,7 +365,6 @@ static const struct iommu_ops shmobile_iommu_ops = {
> >       .map_sg = default_iommu_map_sg,
> >       .iova_to_phys = shmobile_iommu_iova_to_phys,
> >       .add_device = shmobile_iommu_add_device,
> > -     .pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
> >  };
> >
> >  int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu)
> > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > index f722a0c466cf..6404aba9667a 100644
> > --- a/drivers/iommu/tegra-gart.c
> > +++ b/drivers/iommu/tegra-gart.c
> > @@ -218,6 +218,7 @@ out:
> >
> >  static int gart_iommu_domain_init(struct iommu_domain *domain)
> >  {
> > +     domain->pgsize_bitmap = GART_IOMMU_PGSIZES;
> >       return 0;
> >  }
> >
> > @@ -317,7 +318,6 @@ static const struct iommu_ops gart_iommu_ops = {
> >       .map            = gart_iommu_map,
> >       .unmap          = gart_iommu_unmap,
> >       .iova_to_phys   = gart_iommu_iova_to_phys,
> > -     .pgsize_bitmap  = GART_IOMMU_PGSIZES,
> >  };
> >
> >  static int tegra_gart_suspend(struct device *dev)
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 6e134c7c227f..a423ed9a19da 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -265,6 +265,7 @@ static int tegra_smmu_domain_init(struct iommu_domain
> > *domain) pd[i] = 0;
> >
> >       domain->priv = as;
> > +     domain->pgsize_bitmap = SZ_4K;
> >
> >       return 0;
> >  }
> > @@ -643,8 +644,6 @@ static const struct iommu_ops tegra_smmu_ops = {
> >       .unmap = tegra_smmu_unmap,
> >       .map_sg = default_iommu_map_sg,
> >       .iova_to_phys = tegra_smmu_iova_to_phys,
> > -
> > -     .pgsize_bitmap = SZ_4K,
> >  };
> >
> >  static void tegra_smmu_ahb_enable(void)
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index 4a9d666f1e91..3bb72499e4e1 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -386,7 +386,7 @@ static unsigned long vfio_pgsize_bitmap(struct
> > vfio_iommu *iommu)
> >
> >       mutex_lock(&iommu->lock);
> >       list_for_each_entry(domain, &iommu->domain_list, next)
> > -             bitmap &= domain->domain->ops->pgsize_bitmap;
> > +             bitmap &= domain->domain->pgsize_bitmap;
> >       mutex_unlock(&iommu->lock);
> >
> >       return bitmap;
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 38daa453f2e5..ffc023e24b78 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -53,6 +53,7 @@ struct iommu_domain_geometry {
> >
> >  struct iommu_domain {
> >       const struct iommu_ops *ops;
> > +     unsigned long pgsize_bitmap;    /* Bitmap of supported page sizes */
> >       void *priv;
> >       iommu_fault_handler_t handler;
> >       void *handler_token;
> > @@ -108,7 +109,6 @@ enum iommu_attr {
> >   * @domain_get_attr: Query domain attributes
> >   * @domain_set_attr: Change domain attributes
> >   * @of_xlate: add OF master IDs to iommu grouping
> > - * @pgsize_bitmap: bitmap of supported page sizes
> >   * @priv: per-instance data private to the iommu driver
> >   */
> >  struct iommu_ops {
> > @@ -144,9 +144,6 @@ struct iommu_ops {
> >  #ifdef CONFIG_OF_IOMMU
> >       int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
> >  #endif
> > -
> > -     unsigned long pgsize_bitmap;
> > -     void *priv;
> >  };
> >
> >  #define IOMMU_GROUP_NOTIFY_ADD_DEVICE                1 /* Device added */
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Laurent Pinchart]

Hi Will,

On Wednesday 21 January 2015 14:48:35 Will Deacon wrote:
> On Tue, Jan 20, 2015 at 04:56:03PM +0000, Laurent Pinchart wrote:
> > On Monday 19 January 2015 16:06:20 Will Deacon wrote:
> > > On Fri, Nov 14, 2014 at 08:01:56PM +0000, Arnd Bergmann wrote:
> > > > On Friday 14 November 2014 19:27:54 Will Deacon wrote:
> > > > > > At the moment, iommu_ops is a structure that can get used for any
> > > > > > number of iommus of the same type, but by putting per-device
> > > > > > private data into the same structure you have to duplicate it per
> > > > > > instance.
> > > > > 
> > > > > I'm not sure I agree -- the pgsize_bitmap, for example, could vary
> > > > > between different implementations of the same IOMMU. I think we
> > > > > already have this in Juno (some SMMUs can only do 64k pages, whilst
> > > > > others can do 4k and 64k).
> > > > 
> > > > Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
> > > > course, not in iommu_ops.
> > > 
> > > Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
> > > just move pgsize_bitmap into the iommu_domain, which I think makes a lot
> > > more sense anyway.
> > 
> > The code looks good to me, but what does it have to do with
> > of_iommu_get_ops() having its own list of iommu_ops ?
> 
> So, the initial patches for of_iommu_configure/of_xlate made use of a void
> *priv field in struct iommu_ops (which actually made it to mainline but
> isn't used). This was replaced by the list maintained by of_iommu_get_ops,
> but the discussion highlighted that iommu_ops is not per-IOMMU instance and
> therefore shouldn't contain the pgsize_bitmap, which can (and does) vary
> between different IOMMU instances in a running system.
> 
> This patch is an effort to move the pgsize_bitmap field out of iommu_ops
> (leaving that structure to contain only function pointers) and into
> iommu_domain, where I think it makes more sense.

OK, that's what I had understood. Thanks for the clarification. I was just 
puzzled by what I thought you meant was a dependency on of_iommu_get_ops().

> > > diff below seems to do the trick. The next step would be postponing the
> > > pgsize_bitmap initialisation until device attach for the ARM SMMU, since
> > > it's at that point that we know the active page table format (and
> > > therefore the supported page sizes).

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hello,

On 2014-11-14 19:56, Will Deacon wrote:
> Hello everybody,
>
> Here is the fourth iteration of the RFC I've previously posted here:
>
>    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
>    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
>    RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
>
> Changes since RFCv3 include:
>
>    - Drastic simplification of the data structures, so that we no longer
>      pass around lists of domains. Instead, dma-mapping is expected to
>      allocate the domain (Joerg talked about adding a get_default_domain
>      operation to iommu_ops).
>
>    - iommu_ops is used to hold the per-instance IOMMU data
>
>    - Configuration of DMA segments added to of_dma_configure
>
> All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it 
works fine,
You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) 
integration with DT
and DMA-mapping subsystem" thread.

You can add to all your patches:
Acked-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

I'm also interested in adding get_default_domain() callback, but I 
assume that this
can be done once the basic patchset get merged. Do you plan to work on 
it, do you want
me to implement it?

> Cheers,
>
> Will
>
> --->8
>
> Will Deacon (8):
>    iommu: provide early initialisation hook for IOMMU drivers
>    dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
>    iommu: add new iommu_ops callback for adding an OF device
>    iommu: provide helper function to configure an IOMMU for an of master
>    dma-mapping: detect and configure IOMMU in of_dma_configure
>    dma-mapping: set dma segment properties 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 | 12 +++---
>   arch/arm/kernel/setup.c            |  2 +
>   arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
>   drivers/iommu/Kconfig              |  2 +-
>   drivers/iommu/of_iommu.c           | 50 ++++++++++++++++++++++++
>   drivers/of/platform.c              | 54 +++++++++++++------------
>   include/asm-generic/vmlinux.lds.h  |  2 +
>   include/linux/amba/bus.h           |  1 +
>   include/linux/dma-mapping.h        | 13 ++++---
>   include/linux/iommu.h              |  8 ++++
>   include/linux/of_iommu.h           | 31 +++++++++++++++
>   include/linux/platform_device.h    |  2 +
>   12 files changed, 214 insertions(+), 43 deletions(-)
>

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

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

Hi Marek,

On Wed, Nov 19, 2014 at 11:21:26AM +0000, Marek Szyprowski wrote:
> On 2014-11-14 19:56, Will Deacon wrote:
> > Hello everybody,
> >
> > Here is the fourth iteration of the RFC I've previously posted here:
> >
> >    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
> >    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
> >    RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
> >
> > Changes since RFCv3 include:
> >
> >    - Drastic simplification of the data structures, so that we no longer
> >      pass around lists of domains. Instead, dma-mapping is expected to
> >      allocate the domain (Joerg talked about adding a get_default_domain
> >      operation to iommu_ops).
> >
> >    - iommu_ops is used to hold the per-instance IOMMU data
> >
> >    - Configuration of DMA segments added to of_dma_configure
> >
> > All feedback welcome.
> 
> I've rebased my Exynos SYSMMU patches on top of this patchset and it 
> works fine,
> You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) 
> integration with DT
> and DMA-mapping subsystem" thread.

I just saw that and it looks great, thanks! FWIW, I'll take the first 3
patches you have into my series in some shape or another.

> You can add to all your patches:
> Acked-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Cheers.

> I'm also interested in adding get_default_domain() callback, but I 
> assume that this
> can be done once the basic patchset get merged. Do you plan to work on 
> it, do you want
> me to implement it?

If Joerg isn't working on it already (I don't think he is), then please
do have a go if you have time. You'll probably want to avoid adding devices
with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
to the default domain, otherwise you'll run into issues initialising the
iova allocator.

I had a go at getting ARM dma-mapping to use a hypothetical
get_default_domain function, so I've included the diff I ended up with below,
in case it's at all useful.

Will

--->8

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index f3c0d953f6a2..5071553bf6b8 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-				      u64 size, struct iommu_ops *iommu,
-				      bool coherent)
-{
-	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 dma_base, u64 size,
+			       struct iommu_ops *iommu, bool coherent);
 
 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 c245d903927f..da2c2667bbb1 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t base,
+			   size_t size)
 {
 	unsigned int bits = size >> PAGE_SHIFT;
 	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
@@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
 	mapping->extensions = extensions;
 	mapping->base = base;
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->domain = domain;
 
 	spin_lock_init(&mapping->lock);
 
-	mapping->domain = iommu_domain_alloc(bus);
-	if (!mapping->domain)
-		goto err4;
-
 	kref_init(&mapping->kref);
 	return mapping;
-err4:
-	kfree(mapping->bitmaps[0]);
 err3:
 	kfree(mapping->bitmaps);
 err2:
@@ -1901,6 +1897,23 @@ err2:
 err:
 	return ERR_PTR(err);
 }
+
+struct dma_iommu_mapping *
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+{
+	struct dma_iommu_mapping *mapping;
+	struct iommu_domain *domain;
+
+	domain = iommu_domain_alloc(bus);
+	if (!domain)
+		return ERR_PTR(-ENOMEM);
+
+	mapping = __arm_iommu_create_mapping(domain, base, size);
+	if (IS_ERR(mapping))
+		iommu_domain_free(domain);
+
+	return mapping;
+}
 EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
 static void release_iommu_mapping(struct kref *kref)
@@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  *	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.
+ * 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)
@@ -1963,7 +1975,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;
@@ -1975,7 +1986,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)
 {
@@ -1990,10 +2000,141 @@ 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 struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
+{
+	return coherent ? &iommu_coherent_ops : &iommu_ops;
+}
+
+struct dma_iommu_mapping_entry {
+	struct list_head		list;
+	struct dma_iommu_mapping	*mapping;
+	struct iommu_domain		*domain;
+	u64				dma_base;
+	u64				size;
+	struct kref			kref;
+};
+
+static DEFINE_SPINLOCK(dma_iommu_mapping_lock);
+static LIST_HEAD(dma_iommu_mapping_table);
+
+static void __remove_iommu_mapping_entry(struct kref *kref)
+{
+	struct dma_iommu_mapping_entry *entry;
+
+	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
+	list_del(&entry->list);
+}
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	struct iommu_domain *domain;
+	struct dma_iommu_mapping_entry *entry = NULL;
+
+	if (!iommu->get_default_domain)
+		return false;
+
+	domain = iommu->get_default_domain(dev);
+	if (!domain)
+		return false;
+
+	spin_lock(&dma_iommu_mapping_lock);
+
+	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {
+		if (entry->domain == domain)
+			break;
+	}
+
+	/* Load entry->mapping after entry  -- not strictly necessary for ARM */
+	smp_read_barrier_depends();
+
+	if (!entry) {
+		struct dma_iommu_mapping *mapping;
+
+		entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
+		if (!entry)
+			goto err_unlock;
+
+		entry->domain	= domain;
+		entry->dma_base	= dma_base;
+		entry->size	= size;
+		kref_init(&entry->kref);
+		list_add(&entry->list, &dma_iommu_mapping_table);
+		spin_unlock(&dma_iommu_mapping_lock);
+
+		mapping = __arm_iommu_create_mapping(domain, dma_base, size);
+		if (!IS_ERR(mapping))
+			return false;
+
+		smp_wmb();
+		entry->mapping = mapping;
+	} else if (entry->mapping) {
+		if (entry->dma_base > dma_base || entry->size > size)
+			goto err_unlock;
+
+		kref_get(&entry->kref);
+		spin_unlock(&dma_iommu_mapping_lock);
+	} else {
+		/* Racing on the same IOMMU */
+		goto err_unlock;
+	}
+
+	if (arm_iommu_attach_device(dev, entry->mapping)) {
+		int entry_dead;
+
+		pr_warn("Failed to attached device %s to IOMMU mapping\n",
+				dev_name(dev));
+		spin_lock(&dma_iommu_mapping_lock);
+		entry_dead = kref_put(&entry->kref,
+				      __remove_iommu_mapping_entry);
+		spin_unlock(&dma_iommu_mapping_lock);
+
+		if (entry_dead) {
+			entry->mapping->domain = NULL;
+			arm_iommu_release_mapping(entry->mapping);
+		}
+
+		return false;
+	}
+
+	return true;
+
+err_unlock:
+	spin_unlock(&dma_iommu_mapping_lock);
+	return false;
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	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 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent)
+{
+	struct dma_map_ops *dma_ops;
+
+	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	else
+		dma_ops = arm_get_dma_map_ops(coherent);
+
+	set_dma_ops(dev, dma_ops);
+}

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Marek Szyprowski]

Hello,

On 2014-11-19 12:41, Will Deacon wrote:
> Hi Marek,
>
> On Wed, Nov 19, 2014 at 11:21:26AM +0000, Marek Szyprowski wrote:
>> On 2014-11-14 19:56, Will Deacon wrote:
>>> Hello everybody,
>>>
>>> Here is the fourth iteration of the RFC I've previously posted here:
>>>
>>>     RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
>>>     RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
>>>     RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
>>>
>>> Changes since RFCv3 include:
>>>
>>>     - Drastic simplification of the data structures, so that we no longer
>>>       pass around lists of domains. Instead, dma-mapping is expected to
>>>       allocate the domain (Joerg talked about adding a get_default_domain
>>>       operation to iommu_ops).
>>>
>>>     - iommu_ops is used to hold the per-instance IOMMU data
>>>
>>>     - Configuration of DMA segments added to of_dma_configure
>>>
>>> All feedback welcome.
>> I've rebased my Exynos SYSMMU patches on top of this patchset and it
>> works fine,
>> You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU)
>> integration with DT
>> and DMA-mapping subsystem" thread.
> I just saw that and it looks great, thanks! FWIW, I'll take the first 3
> patches you have into my series in some shape or another.

It would be great if the iommu integration patches were merged to -next 
to give
them a try for a few days. Joerg: do you plan to take those patches to 
v3.19 or
do you want to wait more?

>> You can add to all your patches:
>> Acked-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cheers.
>
>> I'm also interested in adding get_default_domain() callback, but I
>> assume that this
>> can be done once the basic patchset get merged. Do you plan to work on
>> it, do you want
>> me to implement it?
> If Joerg isn't working on it already (I don't think he is), then please
> do have a go if you have time. You'll probably want to avoid adding devices
> with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
> to the default domain, otherwise you'll run into issues initialising the
> iova allocator.
>
> I had a go at getting ARM dma-mapping to use a hypothetical
> get_default_domain function, so I've included the diff I ended up with below,
> in case it's at all useful.

I will check that soon, but I hope this is not strictly needed to get 
basic iommu
and dma-mapping integration merged.

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

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

Hi all,

On Tue, Nov 25, 2014 at 07:35:21AM +0000, Marek Szyprowski wrote:
> On 2014-11-19 12:41, Will Deacon wrote:
> > On Wed, Nov 19, 2014 at 11:21:26AM +0000, Marek Szyprowski wrote:
> >> On 2014-11-14 19:56, Will Deacon wrote:
> >>> Hello everybody,
> >>>
> >>> Here is the fourth iteration of the RFC I've previously posted here:
> >>>
> >>>     RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
> >>>     RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
> >>>     RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
> >>>
> >>> Changes since RFCv3 include:
> >>>
> >>>     - Drastic simplification of the data structures, so that we no longer
> >>>       pass around lists of domains. Instead, dma-mapping is expected to
> >>>       allocate the domain (Joerg talked about adding a get_default_domain
> >>>       operation to iommu_ops).
> >>>
> >>>     - iommu_ops is used to hold the per-instance IOMMU data
> >>>
> >>>     - Configuration of DMA segments added to of_dma_configure
> >>>
> >>> All feedback welcome.
> >> I've rebased my Exynos SYSMMU patches on top of this patchset and it
> >> works fine,
> >> You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU)
> >> integration with DT
> >> and DMA-mapping subsystem" thread.
> > I just saw that and it looks great, thanks! FWIW, I'll take the first 3
> > patches you have into my series in some shape or another.
> 
> It would be great if the iommu integration patches were merged to -next 
> to give
> them a try for a few days. Joerg: do you plan to take those patches to 
> v3.19 or
> do you want to wait more?

I don't know what the plan is to get this series upstream.

Joerg, would you expect this to go via your tree or via something broader
like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?

I certainly don't see why patches 1-5 couldn't be queued, then we could add
the ARM bits later. I just need to know where to send them!

Will

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[jroedel-l3A5Bk7waGM]

On Wed, Nov 26, 2014 at 05:47:07PM +0000, Will Deacon wrote:
> Joerg, would you expect this to go via your tree or via something broader
> like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?

Hmm, I don't like the idea of storing private data in iommu_ops. But
given that this is already an improvement we can build on later, here is
my

	Acked-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

To further improve this we should probably introduce a seperate
iommu-descriptor data-structure later which then describes a single
hardware iommu device.


	Joerg

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Fri, Nov 28, 2014 at 01:03:36PM +0000, jroedel-l3A5Bk7waGM@public.gmane.org wrote:
> On Wed, Nov 26, 2014 at 05:47:07PM +0000, Will Deacon wrote:
> > Joerg, would you expect this to go via your tree or via something broader
> > like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?
> 
> Hmm, I don't like the idea of storing private data in iommu_ops. But
> given that this is already an improvement we can build on later, here is
> my
> 
> 	Acked-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Thanks, Joerg! I'll repost this series shortly with the relevant acks.

> To further improve this we should probably introduce a seperate
> iommu-descriptor data-structure later which then describes a single
> hardware iommu device.

Yes, I agree. I'd like to put the pgsize_bitmap in there too, as the page
table series I posted yesterday has to play games to keep the mask
restricted based on the tables in use by any given domain.

Will

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Laurent Pinchart]

Hi Will,

On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
> On Wed, Nov 19, 2014 at 11:21:26AM +0000, Marek Szyprowski wrote:
> > On 2014-11-14 19:56, Will Deacon wrote:
> > > Hello everybody,
> > > 
> > > Here is the fourth iteration of the RFC I've previously posted here:
> > >    RFCv1:
> > >    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/28
> > >    3023.html RFCv2:
> > >    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
> > >    /283752.html RFCv3:
> > >    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
> > >    /287031.html> > 
> > > Changes since RFCv3 include:
> > >    - Drastic simplification of the data structures, so that we no longer
> > >      pass around lists of domains. Instead, dma-mapping is expected to
> > >      allocate the domain (Joerg talked about adding a get_default_domain
> > >      operation to iommu_ops).
> > >    
> > >    - iommu_ops is used to hold the per-instance IOMMU data
> > >    
> > >    - Configuration of DMA segments added to of_dma_configure
> > > 
> > > All feedback welcome.
> > 
> > I've rebased my Exynos SYSMMU patches on top of this patchset and it
> > works fine,
> > You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU)
> > integration with DT and DMA-mapping subsystem" thread.
> 
> I just saw that and it looks great, thanks! FWIW, I'll take the first 3
> patches you have into my series in some shape or another.
> 
> > You can add to all your patches:
> > Acked-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> 
> Cheers.
> 
> > I'm also interested in adding get_default_domain() callback, but I
> > assume that this
> > can be done once the basic patchset get merged. Do you plan to work on
> > it, do you want
> > me to implement it?
> 
> If Joerg isn't working on it already (I don't think he is), then please
> do have a go if you have time. You'll probably want to avoid adding devices
> with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
> to the default domain, otherwise you'll run into issues initialising the
> iova allocator.
> 
> I had a go at getting ARM dma-mapping to use a hypothetical
> get_default_domain function, so I've included the diff I ended up with
> below, in case it's at all useful.
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h
> b/arch/arm/include/asm/dma-mapping.h index f3c0d953f6a2..5071553bf6b8
> 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
> *dev) }
>  #define dma_max_pfn(dev) dma_max_pfn(dev)
> 
> -static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
> -				      u64 size, struct iommu_ops *iommu,
> -				      bool coherent)
> -{
> -	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 dma_base, u64 size,
> +			       struct iommu_ops *iommu, bool coherent);
> 
>  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 c245d903927f..da2c2667bbb1 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = {
>   * arm_iommu_attach_device function.
>   */
>  struct dma_iommu_mapping *
> -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t
> size) +__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t
> base, +			   size_t size)
>  {
>  	unsigned int bits = size >> PAGE_SHIFT;
>  	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
> @@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus,
> dma_addr_t base, size_t size) mapping->extensions = extensions;
>  	mapping->base = base;
>  	mapping->bits = BITS_PER_BYTE * bitmap_size;
> +	mapping->domain = domain;
> 
>  	spin_lock_init(&mapping->lock);
> 
> -	mapping->domain = iommu_domain_alloc(bus);
> -	if (!mapping->domain)
> -		goto err4;
> -
>  	kref_init(&mapping->kref);
>  	return mapping;
> -err4:
> -	kfree(mapping->bitmaps[0]);
>  err3:
>  	kfree(mapping->bitmaps);
>  err2:
> @@ -1901,6 +1897,23 @@ err2:
>  err:
>  	return ERR_PTR(err);
>  }
> +
> +struct dma_iommu_mapping *
> +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t
> size) +{
> +	struct dma_iommu_mapping *mapping;
> +	struct iommu_domain *domain;
> +
> +	domain = iommu_domain_alloc(bus);
> +	if (!domain)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mapping = __arm_iommu_create_mapping(domain, base, size);
> +	if (IS_ERR(mapping))
> +		iommu_domain_free(domain);
> +
> +	return mapping;
> +}
>  EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
> 
>  static void release_iommu_mapping(struct kref *kref)
> @@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
>   *	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.
> + * 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)
> @@ -1963,7 +1975,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;
> @@ -1975,7 +1986,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)
>  {
> @@ -1990,10 +2000,141 @@ 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 struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
> +{
> +	return coherent ? &iommu_coherent_ops : &iommu_ops;
> +}
> +
> +struct dma_iommu_mapping_entry {
> +	struct list_head		list;
> +	struct dma_iommu_mapping	*mapping;
> +	struct iommu_domain		*domain;
> +	u64				dma_base;
> +	u64				size;
> +	struct kref			kref;
> +};
> +
> +static DEFINE_SPINLOCK(dma_iommu_mapping_lock);
> +static LIST_HEAD(dma_iommu_mapping_table);
> +
> +static void __remove_iommu_mapping_entry(struct kref *kref)
> +{
> +	struct dma_iommu_mapping_entry *entry;
> +
> +	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
> +	list_del(&entry->list);
> +}
> +
> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
> size,
> +				    struct iommu_ops *iommu)
> +{
> +	struct iommu_domain *domain;
> +	struct dma_iommu_mapping_entry *entry = NULL;
> +
> +	if (!iommu->get_default_domain)
> +		return false;
> +
> +	domain = iommu->get_default_domain(dev);
> +	if (!domain)
> +		return false;
> +
> +	spin_lock(&dma_iommu_mapping_lock);
> +
> +	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {
> +		if (entry->domain == domain)
> +			break;
> +	}

That might be a stupid question (fighting my impostor syndrome again here), 
but is there a fundamental reason why we can't store the VA allocation data 
(probably using iova) in the domain instead of having to go through hoops and 
loops here to associate that data to the domain ?

> +	/* Load entry->mapping after entry  -- not strictly necessary for ARM */
> +	smp_read_barrier_depends();
> +
> +	if (!entry) {
> +		struct dma_iommu_mapping *mapping;
> +
> +		entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
> +		if (!entry)
> +			goto err_unlock;
> +
> +		entry->domain	= domain;
> +		entry->dma_base	= dma_base;
> +		entry->size	= size;
> +		kref_init(&entry->kref);
> +		list_add(&entry->list, &dma_iommu_mapping_table);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +
> +		mapping = __arm_iommu_create_mapping(domain, dma_base, size);
> +		if (!IS_ERR(mapping))
> +			return false;
> +
> +		smp_wmb();
> +		entry->mapping = mapping;
> +	} else if (entry->mapping) {
> +		if (entry->dma_base > dma_base || entry->size > size)
> +			goto err_unlock;
> +
> +		kref_get(&entry->kref);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +	} else {
> +		/* Racing on the same IOMMU */
> +		goto err_unlock;
> +	}
> +
> +	if (arm_iommu_attach_device(dev, entry->mapping)) {
> +		int entry_dead;
> +
> +		pr_warn("Failed to attached device %s to IOMMU mapping\n",
> +				dev_name(dev));
> +		spin_lock(&dma_iommu_mapping_lock);
> +		entry_dead = kref_put(&entry->kref,
> +				      __remove_iommu_mapping_entry);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +
> +		if (entry_dead) {
> +			entry->mapping->domain = NULL;
> +			arm_iommu_release_mapping(entry->mapping);
> +		}
> +
> +		return false;
> +	}
> +
> +	return true;
> +
> +err_unlock:
> +	spin_unlock(&dma_iommu_mapping_lock);
> +	return false;
> +}
> +
> +#else
> +
> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
> size, +				    struct iommu_ops *iommu)
> +{
> +	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 dma_base, u64 size,
> +			struct iommu_ops *iommu, bool coherent)
> +{
> +	struct dma_map_ops *dma_ops;
> +
> +	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> +		dma_ops = arm_get_iommu_dma_map_ops(coherent);
> +	else
> +		dma_ops = arm_get_dma_map_ops(coherent);
> +
> +	set_dma_ops(dev, dma_ops);
> +}

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Will Deacon]

On Mon, Dec 15, 2014 at 05:21:16PM +0000, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
> > +static void __remove_iommu_mapping_entry(struct kref *kref)
> > +{
> > +	struct dma_iommu_mapping_entry *entry;
> > +
> > +	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
> > +	list_del(&entry->list);
> > +}
> > +
> > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
> > size,
> > +				    struct iommu_ops *iommu)
> > +{
> > +	struct iommu_domain *domain;
> > +	struct dma_iommu_mapping_entry *entry = NULL;
> > +
> > +	if (!iommu->get_default_domain)
> > +		return false;
> > +
> > +	domain = iommu->get_default_domain(dev);
> > +	if (!domain)
> > +		return false;
> > +
> > +	spin_lock(&dma_iommu_mapping_lock);
> > +
> > +	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {
> > +		if (entry->domain == domain)
> > +			break;
> > +	}
> 
> That might be a stupid question (fighting my impostor syndrome again here), 
> but is there a fundamental reason why we can't store the VA allocation data 
> (probably using iova) in the domain instead of having to go through hoops and 
> loops here to associate that data to the domain ?

Well, this was very much a work-in-progress that I ended up abandoning :)

Ultimately, I'd really like to see the default domain management moved into
core code, at which point extending struct iommu_domain seems perfectly
plausible to me (not sure if we'd have a new structure for that).

Then we'd have a fairly clean device -> group -> domain -> allocator
abstraction for each master.

Will

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Laurent Pinchart]

Hi Will,

On Monday 15 December 2014 17:34:16 Will Deacon wrote:
> On Mon, Dec 15, 2014 at 05:21:16PM +0000, Laurent Pinchart wrote:
> > On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
> > > +static void __remove_iommu_mapping_entry(struct kref *kref)
> > > +{
> > > +	struct dma_iommu_mapping_entry *entry;
> > > +
> > > +	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
> > > +	list_del(&entry->list);
> > > +}
> > > +
> > > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base,
> > > u64
> > > size,
> > > +				    struct iommu_ops *iommu)
> > > +{
> > > +	struct iommu_domain *domain;
> > > +	struct dma_iommu_mapping_entry *entry = NULL;
> > > +
> > > +	if (!iommu->get_default_domain)
> > > +		return false;
> > > +
> > > +	domain = iommu->get_default_domain(dev);
> > > +	if (!domain)
> > > +		return false;
> > > +
> > > +	spin_lock(&dma_iommu_mapping_lock);
> > > +
> > > +	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {
> > > +		if (entry->domain == domain)
> > > +			break;
> > > +	}
> > 
> > That might be a stupid question (fighting my impostor syndrome again
> > here), but is there a fundamental reason why we can't store the VA
> > allocation data (probably using iova) in the domain instead of having to
> > go through hoops and loops here to associate that data to the domain ?
> 
> Well, this was very much a work-in-progress that I ended up abandoning :)
> 
> Ultimately, I'd really like to see the default domain management moved into
> core code, at which point extending struct iommu_domain seems perfectly
> plausible to me (not sure if we'd have a new structure for that).
> 
> Then we'd have a fairly clean device -> group -> domain -> allocator
> abstraction for each master.

I like that. Thank you for confirming that some of my questions are sensible 
:-)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

[Robin Murphy]

Hi Will,

On 14/11/14 18:56, Will Deacon wrote:
> Hello everybody,
>
> Here is the fourth iteration of the RFC I've previously posted here:
>
>    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
>    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
>    RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
>
> Changes since RFCv3 include:
>
>    - Drastic simplification of the data structures, so that we no longer
>      pass around lists of domains. Instead, dma-mapping is expected to
>      allocate the domain (Joerg talked about adding a get_default_domain
>      operation to iommu_ops).
>
>    - iommu_ops is used to hold the per-instance IOMMU data
>
>    - Configuration of DMA segments added to of_dma_configure
>
> All feedback welcome.
>

This proposal has been really useful in getting IOMMU DMA mapping 
running on arm64, so with the proviso that patch 6 definitely causes 
problems, and patches 7 and 8 ported to arm64 rather than used directly, 
for the series:

Tested-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Hopefully it's now stable enough that I can finish cleaning up my 
hacked-up SMMU driver (oh, the rebasing pain :P)

Thanks,
Robin.

> Cheers,
>
> Will
>
> --->8
>
> Will Deacon (8):
>    iommu: provide early initialisation hook for IOMMU drivers
>    dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
>    iommu: add new iommu_ops callback for adding an OF device
>    iommu: provide helper function to configure an IOMMU for an of master
>    dma-mapping: detect and configure IOMMU in of_dma_configure
>    dma-mapping: set dma segment properties 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 | 12 +++---
>   arch/arm/kernel/setup.c            |  2 +
>   arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
>   drivers/iommu/Kconfig              |  2 +-
>   drivers/iommu/of_iommu.c           | 50 ++++++++++++++++++++++++
>   drivers/of/platform.c              | 54 +++++++++++++------------
>   include/asm-generic/vmlinux.lds.h  |  2 +
>   include/linux/amba/bus.h           |  1 +
>   include/linux/dma-mapping.h        | 13 ++++---
>   include/linux/iommu.h              |  8 ++++
>   include/linux/of_iommu.h           | 31 +++++++++++++++
>   include/linux/platform_device.h    |  2 +
>   12 files changed, 214 insertions(+), 43 deletions(-)
>