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

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

[Will Deacon]

Hi all,

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

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

Changes since RFCv2 include:

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

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

Cheers,

Will

--->8

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

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

-- 
2.1.0

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

[Will Deacon]

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

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

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

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

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e550ccb7634e..13d800c4ce25 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -89,3 +89,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match, *matches = &__iommu_of_table;
+
+	for_each_matching_node_and_match(np, matches, &match) {
+		const 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 5ba0360663a7..b75ede8f99ae 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -162,6 +162,7 @@
 #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
 #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
 #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
 #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
@@ -495,6 +496,7 @@
 	CLK_OF_TABLES()							\
 	RESERVEDMEM_OF_TABLES()						\
 	CLKSRC_OF_TABLES()						\
+	IOMMU_OF_TABLES()						\
 	CPU_METHOD_OF_TABLES()						\
 	KERNEL_DTB()							\
 	IRQCHIP_OF_MATCH_TABLE()					\
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a527922a..4256f3ce1673 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -57,6 +57,16 @@ struct iommu_domain {
 	struct iommu_domain_geometry geometry;
 };
 
+struct iommu_data {
+	/* Initialised by DMA mapping */
+	struct iommu_domain *domain;
+	struct iova_domain *iovad;
+
+	/* Initialised by the IOMMU driver */
+	const struct iommu_ops *ops;
+	void *priv;
+};
+
 #define IOMMU_CAP_CACHE_COHERENCY	0x1
 #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
 
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f34bca..0a685e0ab33e 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_data(struct device_node *np,
+				     struct iommu_data *data)
+{
+	np->data = data;
+}
+
+static inline struct iommu_data *of_iommu_get_data(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.0

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

[Robin Murphy]

Hi Will,

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

Robin.

--->8---

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 8656b63..1927691 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -24,6 +24,9 @@
  #include <linux/of_iommu.h>
  #include <linux/slab.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.
   *

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

[Will Deacon]

On Thu, Sep 18, 2014 at 03:31:54PM +0100, Robin Murphy wrote:
> After some fun times wondering why on Earth of_iommu_init was trying to 
> instantiate a GIC, I think we may need one of these as part of this 
> patch, too ;)

Oops, thanks!

Will

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

[Will Deacon]

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

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping
ops too, this patch replaces the function with a broader
arch_setup_dma_ops callback which 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 c45b61a4b4a5..7e9ac4f604c3 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 0197725e033a..946dd7ae0394 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -164,6 +164,8 @@ static void of_dma_configure(struct platform_device *pdev)
 {
 	u64 dma_addr, paddr, size;
 	int ret;
+	bool coherent;
+	unsigned long offset;
 	struct device *dev = &pdev->dev;
 
 	/*
@@ -179,28 +181,21 @@ static void of_dma_configure(struct platform_device *pdev)
 	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 931b70986272..1e944e77d38d 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.0

[RFC PATCH v3 3/7] 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4256f3ce1673..821eb0bd9f6c 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>
 
@@ -140,6 +141,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;
 };
 
-- 
2.1.0

Re: [RFC PATCH v3 3/7] iommu: add new iommu_ops callback for adding an OF device

[Marek Szyprowski]

Hello,

On 2014-09-12 18:34, Will Deacon wrote:
> 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 | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 4256f3ce1673..821eb0bd9f6c 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>
>   
> @@ -140,6 +141,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


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

> +
>   	unsigned long pgsize_bitmap;
>   };

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

Re: [RFC PATCH v3 3/7] iommu: add new iommu_ops callback for adding an OF device

[Will Deacon]

On Mon, Sep 15, 2014 at 12:57:38PM +0100, Marek Szyprowski wrote:
> Hello,

Hi Marek,

Thanks for looking again at this -- I'll take at look at your exynos series
when I'm back in the UK next week.

> On 2014-09-12 18:34, Will Deacon wrote:
> > 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 | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 4256f3ce1673..821eb0bd9f6c 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>
> >   
> > @@ -140,6 +141,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
> 
> 
> If I understand correctly, this callback is intended to do per-master 
> initialization
> of the iommu structures required by the given iommu driver (I stored them in
> dev->archdata.iommu). However I really don't get what is the meaning of 
> the return
> value. Is it a boolean value? It is used only by of_iommu_configure to 
> check if
> the parse loop should be terminated...

It should probably return 0 on success, < 0 otherwise. I'll fix
of_iommu_configure to check for < 0 and only exit the loop then. The idea is
that we don't swizzle the DMA ops for a device to IOMMU ops if of_xlate
failed for any of its IDs.

Will

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

[Will Deacon]

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

This patch introduces of_iommu_configure, which does exactly that. A
list of iommu_dma_mapping structures are created for each device, which
represent the set of IOMMU instances through which the device can
master. The list is protected by a kref count and freed when no users
remain. It is expected that DMA-mapping code will take a reference if it
wishes to make use of the IOMMU information.

Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/of_iommu.c    | 70 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-mapping.h |  8 ++++++
 include/linux/of_iommu.h    | 10 +++++++
 4 files changed, 89 insertions(+), 1 deletion(-)

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 13d800c4ce25..8656b63f27ee 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -18,9 +18,11 @@
  */
 
 #include <linux/export.h>
+#include <linux/iommu.h>
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/slab.h>
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
@@ -90,6 +92,74 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	struct of_phandle_args iommu_spec;
+	struct iommu_dma_mapping *mapping;
+	struct device_node *np;
+	struct iommu_data *iommu = NULL;
+	int idx = 0;
+
+	/*
+	 * We don't currently walk up the tree looking for a parent IOMMU.
+	 * See the `Notes:' section of
+	 * Documentation/devicetree/bindings/iommu/iommu.txt
+	 */
+	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+					   "#iommu-cells", idx,
+					   &iommu_spec)) {
+		struct iommu_data *data;
+
+		np = iommu_spec.np;
+		data = of_iommu_get_data(np);
+
+		if (!data || !data->ops || !data->ops->of_xlate)
+			goto err_put_node;
+
+		if (!iommu) {
+			iommu = data;
+		} else if (iommu != data) {
+			/* We don't currently support multi-IOMMU masters */
+			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
+				dev_name(dev));
+			goto err_put_node;
+		}
+
+		if (!data->ops->of_xlate(dev, &iommu_spec))
+			goto err_put_node;
+
+		of_node_put(np);
+		idx++;
+	}
+
+	if (!iommu)
+		return NULL;
+
+	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)
+		return NULL;
+
+	kref_init(&mapping->kref);
+	INIT_LIST_HEAD(&mapping->node);
+	mapping->iommu = iommu;
+	return mapping;
+
+err_put_node:
+	of_node_put(np);
+	return NULL;
+}
+
+void of_iommu_deconfigure(struct kref *kref)
+{
+	struct iommu_dma_mapping *mapping, *curr, *next;
+
+	mapping = container_of(kref, struct iommu_dma_mapping, kref);
+	list_for_each_entry_safe(curr, next, &mapping->node, node) {
+		list_del(&curr->node);
+		kfree(curr);
+	}
+}
+
 void __init of_iommu_init(void)
 {
 	struct device_node *np;
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 1e944e77d38d..e60e52d82db9 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -62,6 +62,14 @@ struct dma_map_ops {
 	int is_phys;
 };
 
+struct iommu_data;
+
+struct iommu_dma_mapping {
+	struct iommu_data *iommu;
+	struct list_head node;
+	struct kref kref;
+};
+
 #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
 
 #define DMA_MASK_NONE	0x0ULL
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 0a685e0ab33e..af6179557005 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -1,9 +1,12 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/device.h>
 #include <linux/iommu.h>
 #include <linux/of.h>
 
+struct iommu_dma_mapping;
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -11,6 +14,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     size_t *size);
 
 extern void of_iommu_init(void);
+extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev);
+extern void of_iommu_deconfigure(struct kref *kref);
 
 #else
 
@@ -22,6 +27,11 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 }
 
 static inline void of_iommu_init(void) { }
+static inline struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
+{
+	return NULL;
+}
+static inline void of_iommu_deconfigure(struct kref *kref) { }
 
 #endif	/* CONFIG_OF_IOMMU */
 
-- 
2.1.0

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

[Laurent Pinchart]

Hi Will,

Thank you for the patch.

On Friday 12 September 2014 17:34:52 Will Deacon wrote:
> The generic IOMMU device-tree bindings can be used to add arbitrary OF
> masters to an IOMMU with a compliant binding.
> 
> This patch introduces of_iommu_configure, which does exactly that. A
> list of iommu_dma_mapping structures are created for each device, which
> represent the set of IOMMU instances through which the device can
> master. The list is protected by a kref count and freed when no users
> remain. It is expected that DMA-mapping code will take a reference if it
> wishes to make use of the IOMMU information.
> 
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/Kconfig       |  2 +-
>  drivers/iommu/of_iommu.c    | 70 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-mapping.h |  8 ++++++
>  include/linux/of_iommu.h    | 10 +++++++
>  4 files changed, 89 insertions(+), 1 deletion(-)
> 
> 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 13d800c4ce25..8656b63f27ee 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -18,9 +18,11 @@
>   */
> 
>  #include <linux/export.h>
> +#include <linux/iommu.h>
>  #include <linux/limits.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> +#include <linux/slab.h>
> 
>  /**
>   * of_get_dma_window - Parse *dma-window property and returns 0 if found.
> @@ -90,6 +92,74 @@ int of_get_dma_window(struct device_node *dn, const char
> *prefix, int index, }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
> 
> +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> +{
> +	struct of_phandle_args iommu_spec;
> +	struct iommu_dma_mapping *mapping;
> +	struct device_node *np;
> +	struct iommu_data *iommu = NULL;
> +	int idx = 0;
> +
> +	/*
> +	 * We don't currently walk up the tree looking for a parent IOMMU.
> +	 * See the `Notes:' section of
> +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> +	 */
> +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +					   "#iommu-cells", idx,
> +					   &iommu_spec)) {
> +		struct iommu_data *data;
> +
> +		np = iommu_spec.np;
> +		data = of_iommu_get_data(np);
> +
> +		if (!data || !data->ops || !data->ops->of_xlate)
> +			goto err_put_node;
> +
> +		if (!iommu) {
> +			iommu = data;
> +		} else if (iommu != data) {
> +			/* We don't currently support multi-IOMMU masters */
> +			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> +				dev_name(dev));
> +			goto err_put_node;
> +		}
> +
> +		if (!data->ops->of_xlate(dev, &iommu_spec))
> +			goto err_put_node;
> +
> +		of_node_put(np);
> +		idx++;
> +	}
> +
> +	if (!iommu)
> +		return NULL;
> +
> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> +	if (!mapping)
> +		return NULL;
> +
> +	kref_init(&mapping->kref);
> +	INIT_LIST_HEAD(&mapping->node);

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

> +	mapping->iommu = iommu;
> +	return mapping;
> +
> +err_put_node:
> +	of_node_put(np);
> +	return NULL;
> +}
> +
> +void of_iommu_deconfigure(struct kref *kref)
> +{
> +	struct iommu_dma_mapping *mapping, *curr, *next;
> +
> +	mapping = container_of(kref, struct iommu_dma_mapping, kref);
> +	list_for_each_entry_safe(curr, next, &mapping->node, node) {
> +		list_del(&curr->node);
> +		kfree(curr);
> +	}

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

> +}
> +

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

>  void __init of_iommu_init(void)
>  {
>  	struct device_node *np;
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 1e944e77d38d..e60e52d82db9 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -62,6 +62,14 @@ struct dma_map_ops {
>  	int is_phys;
>  };
> 
> +struct iommu_data;
> +
> +struct iommu_dma_mapping {
> +	struct iommu_data *iommu;
> +	struct list_head node;
> +	struct kref kref;
> +};

Could you please document the structure with kerneldoc ?

> +
>  #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> 
>  #define DMA_MASK_NONE	0x0ULL
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 0a685e0ab33e..af6179557005 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -1,9 +1,12 @@
>  #ifndef __OF_IOMMU_H
>  #define __OF_IOMMU_H
> 
> +#include <linux/device.h>
>  #include <linux/iommu.h>
>  #include <linux/of.h>
> 
> +struct iommu_dma_mapping;
> +
>  #ifdef CONFIG_OF_IOMMU
> 
>  extern int of_get_dma_window(struct device_node *dn, const char *prefix,
> @@ -11,6 +14,8 @@ extern int of_get_dma_window(struct device_node *dn, const
> char *prefix, size_t *size);
> 
>  extern void of_iommu_init(void);
> +extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev);
> +extern void of_iommu_deconfigure(struct kref *kref);
> 
>  #else
> 
> @@ -22,6 +27,11 @@ static inline int of_get_dma_window(struct device_node
> *dn, const char *prefix, }
> 
>  static inline void of_iommu_init(void) { }
> +static inline struct iommu_dma_mapping *of_iommu_configure(struct device
> *dev) +{
> +	return NULL;
> +}
> +static inline void of_iommu_deconfigure(struct kref *kref) { }
> 
>  #endif	/* CONFIG_OF_IOMMU */

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

On Thu, Sep 18, 2014 at 12:13:13PM +0100, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> Thank you for the patch.

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

> On Friday 12 September 2014 17:34:52 Will Deacon wrote:
> > The generic IOMMU device-tree bindings can be used to add arbitrary OF
> > masters to an IOMMU with a compliant binding.
> > 
> > This patch introduces of_iommu_configure, which does exactly that. A
> > list of iommu_dma_mapping structures are created for each device, which
> > represent the set of IOMMU instances through which the device can
> > master. The list is protected by a kref count and freed when no users
> > remain. It is expected that DMA-mapping code will take a reference if it
> > wishes to make use of the IOMMU information.

[...]

> > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> > +{
> > +	struct of_phandle_args iommu_spec;
> > +	struct iommu_dma_mapping *mapping;
> > +	struct device_node *np;
> > +	struct iommu_data *iommu = NULL;
> > +	int idx = 0;
> > +
> > +	/*
> > +	 * We don't currently walk up the tree looking for a parent IOMMU.
> > +	 * See the `Notes:' section of
> > +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> > +	 */
> > +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> > +					   "#iommu-cells", idx,
> > +					   &iommu_spec)) {
> > +		struct iommu_data *data;
> > +
> > +		np = iommu_spec.np;
> > +		data = of_iommu_get_data(np);
> > +
> > +		if (!data || !data->ops || !data->ops->of_xlate)
> > +			goto err_put_node;
> > +
> > +		if (!iommu) {
> > +			iommu = data;
> > +		} else if (iommu != data) {
> > +			/* We don't currently support multi-IOMMU masters */
> > +			pr_warn("Rejecting device %s with multiple IOMMU instances\n",
> > +				dev_name(dev));
> > +			goto err_put_node;
> > +		}
> > +
> > +		if (!data->ops->of_xlate(dev, &iommu_spec))
> > +			goto err_put_node;
> > +
> > +		of_node_put(np);
> > +		idx++;
> > +	}
> > +
> > +	if (!iommu)
> > +		return NULL;
> > +
> > +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> > +	if (!mapping)
> > +		return NULL;
> > +
> > +	kref_init(&mapping->kref);
> > +	INIT_LIST_HEAD(&mapping->node);
> 
> I might be missing something, but that doesn't seem to match the commit 
> message. This creates a single iommu_dma_mapping per device, where is the list 
> supposed to be populated ?

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

> > +void of_iommu_deconfigure(struct kref *kref)
> > +{
> > +	struct iommu_dma_mapping *mapping, *curr, *next;
> > +
> > +	mapping = container_of(kref, struct iommu_dma_mapping, kref);
> > +	list_for_each_entry_safe(curr, next, &mapping->node, node) {
> > +		list_del(&curr->node);
> > +		kfree(curr);
> > +	}
> 
> Don't you need to also kfree(mapping) here ?

Yup, thanks.

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

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

> >  void __init of_iommu_init(void)
> >  {
> >  	struct device_node *np;
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 1e944e77d38d..e60e52d82db9 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -62,6 +62,14 @@ struct dma_map_ops {
> >  	int is_phys;
> >  };
> > 
> > +struct iommu_data;
> > +
> > +struct iommu_dma_mapping {
> > +	struct iommu_data *iommu;
> > +	struct list_head node;
> > +	struct kref kref;
> > +};
> 
> Could you please document the structure with kerneldoc ?

Ok, I'll try!

Will

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

[Laurent Pinchart]

Hi Will,

On Monday 22 September 2014 18:13:52 Will Deacon wrote:
> On Thu, Sep 18, 2014 at 12:13:13PM +0100, Laurent Pinchart wrote:
> > Hi Will,
> 
> Hi Laurent,
> 
> > Thank you for the patch.
> 
> Sorry for the delay in replying, I was at Connect last week and the email
> has backed up.

No worries.

> > On Friday 12 September 2014 17:34:52 Will Deacon wrote:
> >> The generic IOMMU device-tree bindings can be used to add arbitrary OF
> >> masters to an IOMMU with a compliant binding.
> >> 
> >> This patch introduces of_iommu_configure, which does exactly that. A
> >> list of iommu_dma_mapping structures are created for each device, which
> >> represent the set of IOMMU instances through which the device can
> >> master. The list is protected by a kref count and freed when no users
> >> remain. It is expected that DMA-mapping code will take a reference if it
> >> wishes to make use of the IOMMU information.
> 
> [...]
> 
> >> +struct iommu_dma_mapping *of_iommu_configure(struct device *dev)
> >> +{
> >> +	struct of_phandle_args iommu_spec;
> >> +	struct iommu_dma_mapping *mapping;
> >> +	struct device_node *np;
> >> +	struct iommu_data *iommu = NULL;
> >> +	int idx = 0;
> >> +
> >> +	/*
> >> +	 * We don't currently walk up the tree looking for a parent IOMMU.
> >> +	 * See the `Notes:' section of
> >> +	 * Documentation/devicetree/bindings/iommu/iommu.txt
> >> +	 */
> >> +	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> >> +					   "#iommu-cells", idx,
> >> +					   &iommu_spec)) {
> >> +		struct iommu_data *data;
> >> +
> >> +		np = iommu_spec.np;
> >> +		data = of_iommu_get_data(np);
> >> +
> >> +		if (!data || !data->ops || !data->ops->of_xlate)
> >> +			goto err_put_node;
> >> +
> >> +		if (!iommu) {
> >> +			iommu = data;
> >> +		} else if (iommu != data) {
> >> +			/* We don't currently support multi-IOMMU masters */
> >> +			pr_warn("Rejecting device %s with multiple IOMMU 
instances\n",
> >> +				dev_name(dev));
> >> +			goto err_put_node;
> >> +		}
> >> +
> >> +		if (!data->ops->of_xlate(dev, &iommu_spec))
> >> +			goto err_put_node;
> >> +
> >> +		of_node_put(np);
> >> +		idx++;
> >> +	}
> >> +
> >> +	if (!iommu)
> >> +		return NULL;
> >> +
> >> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> >> +	if (!mapping)
> >> +		return NULL;
> >> +
> >> +	kref_init(&mapping->kref);
> >> +	INIT_LIST_HEAD(&mapping->node);
> > 
> > I might be missing something, but that doesn't seem to match the commit
> > message. This creates a single iommu_dma_mapping per device, where is the
> > list supposed to be populated ?
> 
> Right. I currently only populate the first node in the list, and the loop
> above barfs if we find multiple IOMMUs. I was hoping you'd add support for
> that later on, as you mentioned the need for this so I guess you can test it
> too?

I can, I was just puzzled by the mismatch between the code and commit message. 
As your patch series doesn't provide a complete end-to-end implementation it's 
not always easy to understand how you envision that implementation to work, 
and thus difficult to implement the required modifications to the IOMMU 
driver.

> >> +void of_iommu_deconfigure(struct kref *kref)
> >> +{
> >> +	struct iommu_dma_mapping *mapping, *curr, *next;
> >> +
> >> +	mapping = container_of(kref, struct iommu_dma_mapping, kref);
> >> +	list_for_each_entry_safe(curr, next, &mapping->node, node) {
> >> +		list_del(&curr->node);
> >> +		kfree(curr);
> >> +	}
> > 
> > Don't you need to also kfree(mapping) here ?
> 
> Yup, thanks.
> 
> >> +}
> >> +
> > 
> > Do you expect other users of of_iommu_deconfigure() than kref_put() ? If
> > not, how about exposing an of_iommu_put(struct iommu_dma_mapping *) that
> > would call kref_put() internally, and hiding of_iommu_deconfigure() ?
> 
> That's a good idea, I'll do that.
> 
> >>  void __init of_iommu_init(void)
> >>  {
> >>  	struct device_node *np;
> >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> >> index 1e944e77d38d..e60e52d82db9 100644
> >> --- a/include/linux/dma-mapping.h
> >> +++ b/include/linux/dma-mapping.h
> >> @@ -62,6 +62,14 @@ struct dma_map_ops {
> >>  	int is_phys;
> >>  };
> >> 
> >> +struct iommu_data;
> >> +
> >> +struct iommu_dma_mapping {
> >> +	struct iommu_data *iommu;
> >> +	struct list_head node;
> >> +	struct kref kref;
> >> +};
> > 
> > Could you please document the structure with kerneldoc ?
> 
> Ok, I'll try!

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

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

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

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 7e9ac4f604c3..a8bb0c494bb3 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_dma_mapping *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 946dd7ae0394..95ebd38db545 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 platform_device *pdev)
 	int ret;
 	bool coherent;
 	unsigned long offset;
+	struct iommu_dma_mapping *iommu;
 	struct device *dev = &pdev->dev;
 
 	/*
@@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device *pdev)
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	arch_setup_dma_ops(dev, coherent);
+	iommu = of_iommu_configure(dev);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+
+	if (iommu)
+		kref_put(&iommu->kref, of_iommu_deconfigure);
+}
+
+static void of_dma_deconfigure(struct platform_device *pdev)
+{
+	arch_teardown_dma_ops(&pdev->dev);
 }
 
 /**
@@ -224,16 +238,12 @@ static struct platform_device *of_platform_device_create_pdata(
 	if (!dev)
 		goto err_clear_flag;
 
-	of_dma_configure(dev);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
-
-	/* We do not fill the DMA ops for platform devices by default.
-	 * This is currently the responsibility of the platform code
-	 * to do such, possibly using a device notifier
-	 */
+	of_dma_configure(dev);
 
 	if (of_device_add(dev) != 0) {
+		of_dma_deconfigure(dev);
 		platform_device_put(dev);
 		goto err_clear_flag;
 	}
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e60e52d82db9..47e1ac30e300 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -138,7 +138,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { }
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+				      u64 size, struct iommu_dma_mapping *iommu,
+				      bool coherent) { }
+#endif
+
+#ifndef arch_teardown_dma_ops
+static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
-- 
2.1.0

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

[Laurent Pinchart]

Hi Will,

Thank you for the patch.

On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> This patch extends of_dma_configure so that it sets up the IOMMU for a
> device, as well as the coherent/non-coherent DMA mapping ops.
> 
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>  arch/arm/include/asm/dma-mapping.h |  4 +++-
>  drivers/of/platform.c              | 24 +++++++++++++++++-------
>  include/linux/dma-mapping.h        |  8 +++++++-
>  3 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/include/asm/dma-mapping.h
> b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
> 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_dma_mapping *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 946dd7ae0394..95ebd38db545 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 platform_device
> *pdev) int ret;
>  	bool coherent;
>  	unsigned long offset;
> +	struct iommu_dma_mapping *iommu;
>  	struct device *dev = &pdev->dev;
> 
>  	/*
> @@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
> *pdev) dev_dbg(dev, "device is%sdma coherent\n",
>  		coherent ? " " : " not ");
> 
> -	arch_setup_dma_ops(dev, coherent);
> +	iommu = of_iommu_configure(dev);
> +	dev_dbg(dev, "device is%sbehind an iommu\n",
> +		iommu ? " " : " not ");
> +
> +	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> +
> +	if (iommu)
> +		kref_put(&iommu->kref, of_iommu_deconfigure);

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

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

> +}
> +
> +static void of_dma_deconfigure(struct platform_device *pdev)
> +{
> +	arch_teardown_dma_ops(&pdev->dev);
>  }
> 
>  /**
> @@ -224,16 +238,12 @@ static struct platform_device
> *of_platform_device_create_pdata( if (!dev)
>  		goto err_clear_flag;
> 
> -	of_dma_configure(dev);
>  	dev->dev.bus = &platform_bus_type;
>  	dev->dev.platform_data = platform_data;
> -
> -	/* We do not fill the DMA ops for platform devices by default.
> -	 * This is currently the responsibility of the platform code
> -	 * to do such, possibly using a device notifier
> -	 */
> +	of_dma_configure(dev);
> 
>  	if (of_device_add(dev) != 0) {
> +		of_dma_deconfigure(dev);
>  		platform_device_put(dev);
>  		goto err_clear_flag;
>  	}
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index e60e52d82db9..47e1ac30e300 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -138,7 +138,13 @@ static inline int dma_coerce_mask_and_coherent(struct
> device *dev, u64 mask) extern u64 dma_get_required_mask(struct device
> *dev);
> 
>  #ifndef arch_setup_dma_ops
> -static inline void arch_setup_dma_ops(struct device *dev, bool coherent) {
> } +static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
> +				      u64 size, struct iommu_dma_mapping *iommu,
> +				      bool coherent) { }
> +#endif
> +
> +#ifndef arch_teardown_dma_ops
> +static inline void arch_teardown_dma_ops(struct device *dev) { }
>  #endif
> 
>  static inline unsigned int dma_get_max_seg_size(struct device *dev)

-- 
Regards,

Laurent Pinchart

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3417 bytes --]

On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote:
> Hi Will,
> 
> Thank you for the patch.
> 
> On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> > This patch extends of_dma_configure so that it sets up the IOMMU for a
> > device, as well as the coherent/non-coherent DMA mapping ops.
> > 
> > Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  arch/arm/include/asm/dma-mapping.h |  4 +++-
> >  drivers/of/platform.c              | 24 +++++++++++++++++-------
> >  include/linux/dma-mapping.h        |  8 +++++++-
> >  3 files changed, 27 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/dma-mapping.h
> > b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
> > 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_dma_mapping *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 946dd7ae0394..95ebd38db545 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 platform_device
> > *pdev) int ret;
> >  	bool coherent;
> >  	unsigned long offset;
> > +	struct iommu_dma_mapping *iommu;
> >  	struct device *dev = &pdev->dev;
> > 
> >  	/*
> > @@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
> > *pdev) dev_dbg(dev, "device is%sdma coherent\n",
> >  		coherent ? " " : " not ");
> > 
> > -	arch_setup_dma_ops(dev, coherent);
> > +	iommu = of_iommu_configure(dev);
> > +	dev_dbg(dev, "device is%sbehind an iommu\n",
> > +		iommu ? " " : " not ");
> > +
> > +	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> > +
> > +	if (iommu)
> > +		kref_put(&iommu->kref, of_iommu_deconfigure);
> 
> What's the expected life cycle of the iommu_dma_mapping structure ? It gets 
> created by of_iommu_configure() and the initial reference gets dropped here. I 
> suppose you expect arch code to need to keep a reference to the structure, but 
> your implementation in patch 7/7 doesn't. As far as I can see, you don't even 
> use the contents of the structure in the ARM arch_setup_dma_ops() 
> implementation. Do you expect that to change later, or other architectures to 
> need it ?
> 
> By the way, now that I think about it, I find struct iommu_dma_mapping and 
> struct dma_iommu_mapping very confusing.

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Will Deacon]

On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote:
> > On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> > > This patch extends of_dma_configure so that it sets up the IOMMU for a
> > > device, as well as the coherent/non-coherent DMA mapping ops.
> > > 
> > > Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >  arch/arm/include/asm/dma-mapping.h |  4 +++-
> > >  drivers/of/platform.c              | 24 +++++++++++++++++-------
> > >  include/linux/dma-mapping.h        |  8 +++++++-
> > >  3 files changed, 27 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/dma-mapping.h
> > > b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
> > > 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_dma_mapping *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 946dd7ae0394..95ebd38db545 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 platform_device
> > > *pdev) int ret;
> > >  	bool coherent;
> > >  	unsigned long offset;
> > > +	struct iommu_dma_mapping *iommu;
> > >  	struct device *dev = &pdev->dev;
> > > 
> > >  	/*
> > > @@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
> > > *pdev) dev_dbg(dev, "device is%sdma coherent\n",
> > >  		coherent ? " " : " not ");
> > > 
> > > -	arch_setup_dma_ops(dev, coherent);
> > > +	iommu = of_iommu_configure(dev);
> > > +	dev_dbg(dev, "device is%sbehind an iommu\n",
> > > +		iommu ? " " : " not ");
> > > +
> > > +	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> > > +
> > > +	if (iommu)
> > > +		kref_put(&iommu->kref, of_iommu_deconfigure);
> > 
> > What's the expected life cycle of the iommu_dma_mapping structure ? It gets 
> > created by of_iommu_configure() and the initial reference gets dropped here. I 
> > suppose you expect arch code to need to keep a reference to the structure, but 
> > your implementation in patch 7/7 doesn't. As far as I can see, you don't even 
> > use the contents of the structure in the ARM arch_setup_dma_ops() 
> > implementation. Do you expect that to change later, or other architectures to 
> > need it ?
> > 
> > By the way, now that I think about it, I find struct iommu_dma_mapping and 
> > struct dma_iommu_mapping very confusing.
> 
> Agreed. I wonder how useful it is to know the set of IOMMU instances
> that each device can master through. Wouldn't it be more useful to keep
> a list of master interfaces for each device? The set of IOMMU instances
> can trivially be derived from that.

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

Will

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

[Laurent Pinchart]

Hi Will,

On Monday 22 September 2014 18:50:27 Will Deacon wrote:
> On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> > On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote:
> >> On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> >>> This patch extends of_dma_configure so that it sets up the IOMMU for a
> >>> device, as well as the coherent/non-coherent DMA mapping ops.
> >>> 
> >>> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> >>> ---
> >>> 
> >>>  arch/arm/include/asm/dma-mapping.h |  4 +++-
> >>>  drivers/of/platform.c              | 24 +++++++++++++++++-------
> >>>  include/linux/dma-mapping.h        |  8 +++++++-
> >>>  3 files changed, 27 insertions(+), 9 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/include/asm/dma-mapping.h
> >>> b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
> >>> 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_dma_mapping *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 946dd7ae0394..95ebd38db545 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
> >>> platform_device *pdev)
> >>>  	int ret;
> >>>  	bool coherent;
> >>>  	unsigned long offset;
> >>> +	struct iommu_dma_mapping *iommu;
> >>>  	struct device *dev = &pdev->dev;
> >>>  	
> >>>  	/*
> >>> @@ -195,7 +197,19 @@ static void of_dma_configure(struct
> >>> platform_device *pdev)
> >>>  	dev_dbg(dev, "device is%sdma coherent\n",
> >>>  		coherent ? " " : " not ");
> >>> 
> >>> -	arch_setup_dma_ops(dev, coherent);
> >>> +	iommu = of_iommu_configure(dev);
> >>> +	dev_dbg(dev, "device is%sbehind an iommu\n",
> >>> +		iommu ? " " : " not ");
> >>> +
> >>> +	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> >>> +
> >>> +	if (iommu)
> >>> +		kref_put(&iommu->kref, of_iommu_deconfigure);
> >> 
> >> What's the expected life cycle of the iommu_dma_mapping structure ? It
> >> gets created by of_iommu_configure() and the initial reference gets
> >> dropped here. I suppose you expect arch code to need to keep a reference
> >> to the structure, but your implementation in patch 7/7 doesn't. As far as
> >> I can see, you don't even use the contents of the structure in the ARM
> >> arch_setup_dma_ops() implementation. Do you expect that to change later,
> >> or other architectures to need it ?
> >> 
> >> By the way, now that I think about it, I find struct iommu_dma_mapping
> >> and struct dma_iommu_mapping very confusing.
> > 
> > Agreed. I wonder how useful it is to know the set of IOMMU instances
> > that each device can master through. Wouldn't it be more useful to keep
> > a list of master interfaces for each device? The set of IOMMU instances
> > can trivially be derived from that.
> 
> I'm struggling to think how that would look. What do you mean by `master
> interfaces' in terms of the code we have in Linux? At the end of the day,
> the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you
> and Laurent have use-cases involving devices mastering through multiple
> IOMMUs. If it doesn't work for you, it might be best for you to send me
> the patch ;)

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

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote:
> Hi Will,

Hi Laurent,

> On Monday 22 September 2014 18:50:27 Will Deacon wrote:
> > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> > > Agreed. I wonder how useful it is to know the set of IOMMU instances
> > > that each device can master through. Wouldn't it be more useful to keep
> > > a list of master interfaces for each device? The set of IOMMU instances
> > > can trivially be derived from that.
> > 
> > I'm struggling to think how that would look. What do you mean by `master
> > interfaces' in terms of the code we have in Linux? At the end of the day,
> > the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you
> > and Laurent have use-cases involving devices mastering through multiple
> > IOMMUs. If it doesn't work for you, it might be best for you to send me
> > the patch ;)
> 
> Just for the record, I've brought up the topic of masters being served by 
> multiple IOMMUs, but don't have a use case for it (yet at least). I do have 
> masters served through multiple streams with separate stream IDs, but all by 
> the same IOMMU.

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

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

Will

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

[Marek Szyprowski]

Hello,

On 2014-10-27 11:51, Will Deacon wrote:
> On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote:
>> On Monday 22 September 2014 18:50:27 Will Deacon wrote:
>>> On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
>>>> Agreed. I wonder how useful it is to know the set of IOMMU instances
>>>> that each device can master through. Wouldn't it be more useful to keep
>>>> a list of master interfaces for each device? The set of IOMMU instances
>>>> can trivially be derived from that.
>>> I'm struggling to think how that would look. What do you mean by `master
>>> interfaces' in terms of the code we have in Linux? At the end of the day,
>>> the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you
>>> and Laurent have use-cases involving devices mastering through multiple
>>> IOMMUs. If it doesn't work for you, it might be best for you to send me
>>> the patch ;)
>> Just for the record, I've brought up the topic of masters being served by
>> multiple IOMMUs, but don't have a use case for it (yet at least). I do have
>> masters served through multiple streams with separate stream IDs, but all by
>> the same IOMMU.
> Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the
> DMA-mapping API should *not* be exposed to the details of masters that
> master through multiple IOMMUs. Instead, that should be abstracted by the
> device API by exposing that device as a single struct device.
>
> So, that's certainly an area that needs more work and I'll drop the limited
> support I'd cooked up from this patch set in the next version.

Great! That's more or less something I've already implemented on top of your
previous patchset, as I didn't have any good idea how to manage multiple 
masters
separately. I'm waiting for your next update and I will rebase my 
patches soon.

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

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

[Laurent Pinchart]

Hi Will,

On Monday 27 October 2014 10:51:59 Will Deacon wrote:
> On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote:
> > On Monday 22 September 2014 18:50:27 Will Deacon wrote:
> > > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> > > > Agreed. I wonder how useful it is to know the set of IOMMU instances
> > > > that each device can master through. Wouldn't it be more useful to
> > > > keep a list of master interfaces for each device? The set of IOMMU
> > > > instances can trivially be derived from that.
> > > 
> > > I'm struggling to think how that would look. What do you mean by `master
> > > interfaces' in terms of the code we have in Linux? At the end of the
> > > day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because
> > > you and Laurent have use-cases involving devices mastering through
> > > multiple IOMMUs. If it doesn't work for you, it might be best for you to
> > > send me the patch ;)
> > 
> > Just for the record, I've brought up the topic of masters being served by
> > multiple IOMMUs, but don't have a use case for it (yet at least). I do
> > have masters served through multiple streams with separate stream IDs, but
> > all by the same IOMMU.
> 
> Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the
> DMA-mapping API should *not* be exposed to the details of masters that
> master through multiple IOMMUs. Instead, that should be abstracted by the
> device API by exposing that device as a single struct device.

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

On Mon, Oct 27, 2014 at 11:30:33AM +0000, Laurent Pinchart wrote:
> Hi Will,

Hey Laurent,

> On Monday 27 October 2014 10:51:59 Will Deacon wrote:
> > On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote:
> > > On Monday 22 September 2014 18:50:27 Will Deacon wrote:
> > > > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> > > > > Agreed. I wonder how useful it is to know the set of IOMMU instances
> > > > > that each device can master through. Wouldn't it be more useful to
> > > > > keep a list of master interfaces for each device? The set of IOMMU
> > > > > instances can trivially be derived from that.
> > > > 
> > > > I'm struggling to think how that would look. What do you mean by `master
> > > > interfaces' in terms of the code we have in Linux? At the end of the
> > > > day, the list of IOMMU instances (i.e. iommu_dma_mapping) exists because
> > > > you and Laurent have use-cases involving devices mastering through
> > > > multiple IOMMUs. If it doesn't work for you, it might be best for you to
> > > > send me the patch ;)
> > > 
> > > Just for the record, I've brought up the topic of masters being served by
> > > multiple IOMMUs, but don't have a use case for it (yet at least). I do
> > > have masters served through multiple streams with separate stream IDs, but
> > > all by the same IOMMU.
> > 
> > Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the
> > DMA-mapping API should *not* be exposed to the details of masters that
> > master through multiple IOMMUs. Instead, that should be abstracted by the
> > device API by exposing that device as a single struct device.
> 
> I'm not sure to follow you here. Aren't we already exposing masters that 
> master through multiple IOMMUs as single instances of struct device ?

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

Joerg?

> > So, that's certainly an area that needs more work and I'll drop the limited
> > support I'd cooked up from this patch set in the next version.
> 
> How about masters connected to multiple stream IDs of the same IOMMU ?

That should still be handled, as I believe that will be a common case.

Will

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

[jroedel-l3A5Bk7waGM]

On Mon, Oct 27, 2014 at 04:02:16PM +0000, Will Deacon wrote:
> On Mon, Oct 27, 2014 at 11:30:33AM +0000, Laurent Pinchart wrote:
> > I'm not sure to follow you here. Aren't we already exposing masters that 
> > master through multiple IOMMUs as single instances of struct device ?
> 
> Hmm, yes, now you've confused me too! The conclusion was certainly that
> dma-mapping should not be the one dealing with the I/O topology. Domain
> allocation would then be an iommu callback (something like
> ->get_default_domain), but the rest of the details weren't fleshed out.

The idea is that the IOMMU core code will allocate a default domain for
each iommu-group at initialization time. This domain can be requested
later by a new iommu-api function and used for DMA-API mappings.

A device still can be assigned to another domain by driver code (like
VFIO). But if the device is later de-assigned the IOMMU core-code
automatically puts it back into the default domain.


	Joerg

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

[Will Deacon]

On Thu, Sep 18, 2014 at 12:17:33PM +0100, Laurent Pinchart wrote:
> Hi Will,

Hello again,

> On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> > @@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
> > *pdev) dev_dbg(dev, "device is%sdma coherent\n",
> >  		coherent ? " " : " not ");
> > 
> > -	arch_setup_dma_ops(dev, coherent);
> > +	iommu = of_iommu_configure(dev);
> > +	dev_dbg(dev, "device is%sbehind an iommu\n",
> > +		iommu ? " " : " not ");
> > +
> > +	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> > +
> > +	if (iommu)
> > +		kref_put(&iommu->kref, of_iommu_deconfigure);
> 
> What's the expected life cycle of the iommu_dma_mapping structure ? It gets 
> created by of_iommu_configure() and the initial reference gets dropped here. I 
> suppose you expect arch code to need to keep a reference to the structure, but 
> your implementation in patch 7/7 doesn't. As far as I can see, you don't even 
> use the contents of the structure in the ARM arch_setup_dma_ops() 
> implementation. Do you expect that to change later, or other architectures to 
> need it ?

Indeed, I've not done anything to the ARM dma-mapping ops other than plug-in
the existing code, which doesn't use these new features. I think Marek was
going to look at that.

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

Yup; I'd like to see some generic code that uses the per-IOMMU-instance
domain and allocator which is passed to arch_setup_dma_ops. Then we could
simply move arch/arm/ (and arm64) over to that, which would get rid of
dma_iommu_mapping entirely.

Will

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

[Will Deacon]

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

This patch adds a call to of_iommu_init before of_platform_populate.

Signed-off-by: Will Deacon <will.deacon-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 84db893dedc2..495e29697a67 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>
@@ -800,6 +801,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.0

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 854 bytes --]

On Fri, Sep 12, 2014 at 05:34:54PM +0100, Will Deacon wrote:
> We need to ensure that the IOMMUs in the system have a chance to perform
> some basic initialisation before we start adding masters to them.
> 
> This patch adds a call to of_iommu_init before of_platform_populate.

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

I also don't like how this uses what is essentially initcall ordering
when solutions have been proposed in the past to properly integrate this
with deferred probe and allow IOMMU drivers to be proper drivers. We
keep adding these special cases for devices that need to be probed early
and I think it's turning into a mess.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Arnd Bergmann]

On Monday 22 September 2014 11:36:15 Thierry Reding wrote:
> On Fri, Sep 12, 2014 at 05:34:54PM +0100, Will Deacon wrote:
> > We need to ensure that the IOMMUs in the system have a chance to perform
> > some basic initialisation before we start adding masters to them.
> > 
> > This patch adds a call to of_iommu_init before of_platform_populate.
> 
> Why can't you call it from of_platform_populate() directly? That way it
> would be usable for all architectures rather than just ARM. Eventually
> we're going to need the same thing for 64-bit ARM (and possibly others
> as well).

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

	Arnd

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1027 bytes --]

On Mon, Sep 22, 2014 at 01:08:27PM +0200, Arnd Bergmann wrote:
> On Monday 22 September 2014 11:36:15 Thierry Reding wrote:
> > On Fri, Sep 12, 2014 at 05:34:54PM +0100, Will Deacon wrote:
> > > We need to ensure that the IOMMUs in the system have a chance to perform
> > > some basic initialisation before we start adding masters to them.
> > > 
> > > This patch adds a call to of_iommu_init before of_platform_populate.
> > 
> > Why can't you call it from of_platform_populate() directly? That way it
> > would be usable for all architectures rather than just ARM. Eventually
> > we're going to need the same thing for 64-bit ARM (and possibly others
> > as well).
> 
> IIRC, of_platform_populate can be called multiple times, even recursively
> be drivers that populate their own child devices.

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Arnd Bergmann]

On Monday 22 September 2014 13:40:40 Thierry Reding wrote:
> On Mon, Sep 22, 2014 at 01:08:27PM +0200, Arnd Bergmann wrote:
> > On Monday 22 September 2014 11:36:15 Thierry Reding wrote:
> > > On Fri, Sep 12, 2014 at 05:34:54PM +0100, Will Deacon wrote:
> > > > We need to ensure that the IOMMUs in the system have a chance to perform
> > > > some basic initialisation before we start adding masters to them.
> > > > 
> > > > This patch adds a call to of_iommu_init before of_platform_populate.
> > > 
> > > Why can't you call it from of_platform_populate() directly? That way it
> > > would be usable for all architectures rather than just ARM. Eventually
> > > we're going to need the same thing for 64-bit ARM (and possibly others
> > > as well).
> > 
> > IIRC, of_platform_populate can be called multiple times, even recursively
> > be drivers that populate their own child devices.
> 
> Indeed. Perhaps it could be conditionally called when root == NULL. But
> perhaps that's not safe either. Anyway, I still think we shouldn't be
> making this some randomly placed early initcall anyway.

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

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

I see two problems with using deferred probing here: 

- we don't actually need to defer the probing but the binding to the driver
  when no dma ops are set, but it seems silly to even create the device
  before we can find out which ops it should use.
  The reason is that a driver does not actively ask for an IOMMU as it would
  for other subsystems (gpio, led, dmaengine, ...).

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

	Arnd

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4956 bytes --]

On Mon, Sep 22, 2014 at 06:03:47PM +0200, Arnd Bergmann wrote:
> On Monday 22 September 2014 13:40:40 Thierry Reding wrote:
> > On Mon, Sep 22, 2014 at 01:08:27PM +0200, Arnd Bergmann wrote:
> > > On Monday 22 September 2014 11:36:15 Thierry Reding wrote:
> > > > On Fri, Sep 12, 2014 at 05:34:54PM +0100, Will Deacon wrote:
> > > > > We need to ensure that the IOMMUs in the system have a chance to perform
> > > > > some basic initialisation before we start adding masters to them.
> > > > > 
> > > > > This patch adds a call to of_iommu_init before of_platform_populate.
> > > > 
> > > > Why can't you call it from of_platform_populate() directly? That way it
> > > > would be usable for all architectures rather than just ARM. Eventually
> > > > we're going to need the same thing for 64-bit ARM (and possibly others
> > > > as well).
> > > 
> > > IIRC, of_platform_populate can be called multiple times, even recursively
> > > be drivers that populate their own child devices.
> > 
> > Indeed. Perhaps it could be conditionally called when root == NULL. But
> > perhaps that's not safe either. Anyway, I still think we shouldn't be
> > making this some randomly placed early initcall anyway.
> 
> I disagree. IOMMUs are special in the same sense that irqchips, clocks and
> timers are. This is not just a random call, it is being explicit about a
> base functionality that is needed for all devices attached to it.

Why do you say IOMMUs are special? I can understand how clocks and
timers are somewhat special because they are required at early boot. But
IOMMUs are not special in that way. I don't think there are any IOMMU
users that early in the boot process. The only reason why IOMMU is
special is because of the way it's currently hooked into the driver
model.

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

They aren't really device drivers at all. They don't bind to devices but
to device tree nodes. And that comes at a high cost. Writing drivers for
these subsystems is weird, because all of a sudden you can no longer use
all the goodies that we've become used to (dev_log(), devres, ...) over
the years.

> I see two problems with using deferred probing here: 
> 
> - we don't actually need to defer the probing but the binding to the driver
>   when no dma ops are set, but it seems silly to even create the device
>   before we can find out which ops it should use.

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

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

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

Currently the specifics of how that works is all mostly hidden within
the IOMMU driver which will decide what IOMMU to attach to. That's
problematic for cases where a device has multiple master interfaces,
since it won't be able to decide which one to attach to.

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

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

Why? The patches that I (and Hiroshi before me) posted a while ago did
exactly that and it worked just fine. The only objection to that was
that Greg (and you I think) didn't want to have that code in the core
and therefore -EPROBE_DEFER can't properly be propagated. There's no
special magic required beyond that. The IOMMU becomes a resource or
service provider, just like any other driver.

As for the overhead I think that's negligible. Hopefully the IOMMU would
be standalone enough not to defer probing itself, so at worst all IOMMU
users would be deferred once. Many of them will already defer because of
other resources such as GPIOs, clocks or regulators anyway. But that's a
problem for which a solution could be implemented. Dependency-based
probe ordering was discussed not so long ago. With that in place the
IOMMU would be probed before any of its users.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Arnd Bergmann]

On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:

> > I see two problems with using deferred probing here: 
> > 
> > - we don't actually need to defer the probing but the binding to the driver
> >   when no dma ops are set, but it seems silly to even create the device
> >   before we can find out which ops it should use.
> 
> What does device creation have to do with anything? Surely a device
> won't need IOMMU services before the device is bound to a driver.

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

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

> >   The reason is that a driver does not actively ask for an IOMMU as it would
> >   for other subsystems (gpio, led, dmaengine, ...).
> 
> Actually it does. At least in some cases. If you want to use the IOMMU
> API directly you'd call something like iommu_present() on the bus type
> and then allocate an IOMMU domain and attach to it. Unfortunately the
> API seems to have been designed under the assumption that IOMMU will
> have been registered before any users, so the API doesn't propagate any
> meaningful errors.

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

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

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


	Arnd

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3328 bytes --]

On Tue, Sep 23, 2014 at 09:44:25AM +0200, Arnd Bergmann wrote:
> On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> 
> > > I see two problems with using deferred probing here: 
> > > 
> > > - we don't actually need to defer the probing but the binding to the driver
> > >   when no dma ops are set, but it seems silly to even create the device
> > >   before we can find out which ops it should use.
> > 
> > What does device creation have to do with anything? Surely a device
> > won't need IOMMU services before the device is bound to a driver.
> 
> The problem is that the driver will start using the IOMMU as soon
> as it calls dma_map_*, but that happens at runtime, not necessarily
> during the probe function.

But it won't call dma_map_*() before probe, right? If this is done in
the core we can defer probing before the driver's probe is called, so
there shouldn't be an issue.

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

Of course any such solution would only work under the assumption that
the driver (or core) knows what it's doing and doesn't call dma_map_*()
until IOMMU support has properly been set up.

For DMA/IOMMU integration users I'd expect the core to set everything up
before proceeding to calling the driver's .probe() function. For others
they will need a way to either explicitly check for the IOMMU master
interface or have the core do that for them (similar to how it would do
that for DMA/IOMMU integration users).

> > >   The reason is that a driver does not actively ask for an IOMMU as it would
> > >   for other subsystems (gpio, led, dmaengine, ...).
> > 
> > Actually it does. At least in some cases. If you want to use the IOMMU
> > API directly you'd call something like iommu_present() on the bus type
> > and then allocate an IOMMU domain and attach to it. Unfortunately the
> > API seems to have been designed under the assumption that IOMMU will
> > have been registered before any users, so the API doesn't propagate any
> > meaningful errors.
> 
> That's just a special case that does not even work as it should yet,

Of course it works. I count at least 3 drivers in mainline (and 1 local)
that do exactly this.

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

It has everything to do with it. If we go and implement everything in
terms of DMA/IOMMU then we leave out all other users. Depending on who
you talk to the direct IOMMU users are the actually important ones.

> > Also, even if in other cases the drivers don't actively ask for an IOMMU
> > that doesn't mean that they couldn't be made to. For drivers that use
> > the DMA/IOMMU integration layer this is probably not practicable, but
> > there is no reason the core couldn't do it.
> 
> We intentionally have an abstraction that is meant to let you write drivers
> without knowing about iommu, swiotlb or coherency, these are all hidden
> behind the dma_map_ops implementation.

Perhaps I'm missing something then. How can you associate two devices
with the same domain with dma_map_ops?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Laurent Pinchart]

Hi Arnd,

On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> > > I see two problems with using deferred probing here:
> > > 
> > > - we don't actually need to defer the probing but the binding to the
> > >   driver when no dma ops are set, but it seems silly to even create the
> > >   device before we can find out which ops it should use.
> > 
> > What does device creation have to do with anything? Surely a device
> > won't need IOMMU services before the device is bound to a driver.
> 
> The problem is that the driver will start using the IOMMU as soon
> as it calls dma_map_*, but that happens at runtime, not necessarily
> during the probe function.
> 
> So we can get into the weird situation that probe() returns success,
> but then you can't use the device yet because you don't know whether
> it is supposed to use an IOMMU or not.

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

> > >   The reason is that a driver does not actively ask for an IOMMU as it
> > >   would for other subsystems (gpio, led, dmaengine, ...).
> > 
> > Actually it does. At least in some cases. If you want to use the IOMMU
> > API directly you'd call something like iommu_present() on the bus type
> > and then allocate an IOMMU domain and attach to it. Unfortunately the
> > API seems to have been designed under the assumption that IOMMU will
> > have been registered before any users, so the API doesn't propagate any
> > meaningful errors.
> 
> That's just a special case that does not even work as it should yet,
> please don't confuse the matter more by talking about drivers that
> use the IOMMU API explicitly, this series has very little to do with
> that.
> 
> > Also, even if in other cases the drivers don't actively ask for an IOMMU
> > that doesn't mean that they couldn't be made to. For drivers that use
> > the DMA/IOMMU integration layer this is probably not practicable, but
> > there is no reason the core couldn't do it.
> 
> We intentionally have an abstraction that is meant to let you write drivers
> without knowing about iommu, swiotlb or coherency, these are all hidden
> behind the dma_map_ops implementation.

-- 
Regards,

Laurent Pinchart

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

[Arnd Bergmann]

On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:
> On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> > On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> > > > I see two problems with using deferred probing here:
> > > > 
> > > > - we don't actually need to defer the probing but the binding to the
> > > >   driver when no dma ops are set, but it seems silly to even create the
> > > >   device before we can find out which ops it should use.
> > > 
> > > What does device creation have to do with anything? Surely a device
> > > won't need IOMMU services before the device is bound to a driver.
> > 
> > The problem is that the driver will start using the IOMMU as soon
> > as it calls dma_map_*, but that happens at runtime, not necessarily
> > during the probe function.
> > 
> > So we can get into the weird situation that probe() returns success,
> > but then you can't use the device yet because you don't know whether
> > it is supposed to use an IOMMU or not.
> 
> If we want IOMMU devices to be supported by common device drivers we need to 
> defer probing of the master devices, there's no doubt about that. Earlier 
> approaches that hooked up into the device core code were rejected, but it 
> should be possible to use bus notifiers to achieve the same result (with the 
> drawback of having to register one notifier per bus). The 
> BUS_NOTIFY_BIND_DRIVER notifier can then just return -EPROBE_DEFER when a 
> iommus property is available and points to an IOMMU not registered yet. I'm 
> not saying we have to do this, but I believe that at least from a technical 
> point of view it could be done.

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

	Arnd

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3010 bytes --]

On Tue, Oct 14, 2014 at 03:20:46PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:
> > On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> > > On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> > > > > I see two problems with using deferred probing here:
> > > > > 
> > > > > - we don't actually need to defer the probing but the binding to the
> > > > >   driver when no dma ops are set, but it seems silly to even create the
> > > > >   device before we can find out which ops it should use.
> > > > 
> > > > What does device creation have to do with anything? Surely a device
> > > > won't need IOMMU services before the device is bound to a driver.
> > > 
> > > The problem is that the driver will start using the IOMMU as soon
> > > as it calls dma_map_*, but that happens at runtime, not necessarily
> > > during the probe function.
> > > 
> > > So we can get into the weird situation that probe() returns success,
> > > but then you can't use the device yet because you don't know whether
> > > it is supposed to use an IOMMU or not.
> > 
> > If we want IOMMU devices to be supported by common device drivers we need to 
> > defer probing of the master devices, there's no doubt about that. Earlier 
> > approaches that hooked up into the device core code were rejected, but it 
> > should be possible to use bus notifiers to achieve the same result (with the 
> > drawback of having to register one notifier per bus). The 
> > BUS_NOTIFY_BIND_DRIVER notifier can then just return -EPROBE_DEFER when a 
> > iommus property is available and points to an IOMMU not registered yet. I'm 
> > not saying we have to do this, but I believe that at least from a technical 
> > point of view it could be done.
> 
> I think that fundamentally speaking, relying on notifiers for something like
> this is very problematic, both in terms of maintainability and reliability.
> We should really try to get the notifiers out of the iommu handling, not put
> more of them in.

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

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

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Laurent Pinchart]

Hi Thierry,

On Tuesday 14 October 2014 15:37:59 Thierry Reding wrote:
> On Tue, Oct 14, 2014 at 03:20:46PM +0200, Arnd Bergmann wrote:
> > On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:
> >> On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> >>> On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> >>>>> I see two problems with using deferred probing here:
> >>>>> 
> >>>>> - we don't actually need to defer the probing but the binding to
> >>>>>   the driver when no dma ops are set, but it seems silly to even
> >>>>>   create the device before we can find out which ops it should use.
> >>>> 
> >>>> What does device creation have to do with anything? Surely a device
> >>>> won't need IOMMU services before the device is bound to a driver.
> >>> 
> >>> The problem is that the driver will start using the IOMMU as soon
> >>> as it calls dma_map_*, but that happens at runtime, not necessarily
> >>> during the probe function.
> >>> 
> >>> So we can get into the weird situation that probe() returns success,
> >>> but then you can't use the device yet because you don't know whether
> >>> it is supposed to use an IOMMU or not.
> >> 
> >> If we want IOMMU devices to be supported by common device drivers we
> >> need to defer probing of the master devices, there's no doubt about
> >> that. Earlier approaches that hooked up into the device core code were
> >> rejected, but it should be possible to use bus notifiers to achieve the
> >> same result (with the drawback of having to register one notifier per
> >> bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return -
> >> EPROBE_DEFER when a iommus property is available and points to an IOMMU
> >> not registered yet. I'm not saying we have to do this, but I believe that
> >> at least from a technical point of view it could be done.
> > 
> > I think that fundamentally speaking, relying on notifiers for something
> > like this is very problematic, both in terms of maintainability and
> > reliability. We should really try to get the notifiers out of the iommu
> > handling, not put more of them in.
> 
> Agreed. Also last time I checked the driver core simply ignored the
> return value from notifiers, therefore this wouldn't work without
> changing the core either.
> 
> Still, I agree with Laurent that we really should be relying on probe
> deferral for probe ordering.

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

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

> And while it's true that earlier attempts to put this into the core were
> rejected, I think there's still value in proposing it again. The alternative
> proposed here is similarly close to the core and needs to duplicated for
> every architecture. That itself is to me a strong indication that this
> really does belong in the core.
> 
> I think initially this was proposed to become part of really_probe() and
> I still think that's where it belongs. There's precedent for it with the
> pinctrl_bind_pins() call, though it seems like Greg regrets allowing
> that into the core. Perhaps if really_probe() is "too core", then
> platform_drv_probe() would be a better candidate?

I've CC'ed Greg to this e-mail as he will likely have the last word on this 
topic.

-- 
Regards,

Laurent Pinchart

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3112 bytes --]

On Tue, Oct 14, 2014 at 06:01:58PM +0300, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Tuesday 14 October 2014 15:37:59 Thierry Reding wrote:
> > On Tue, Oct 14, 2014 at 03:20:46PM +0200, Arnd Bergmann wrote:
> > > On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:
> > >> On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> > >>> On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> > >>>>> I see two problems with using deferred probing here:
> > >>>>> 
> > >>>>> - we don't actually need to defer the probing but the binding to
> > >>>>>   the driver when no dma ops are set, but it seems silly to even
> > >>>>>   create the device before we can find out which ops it should use.
> > >>>> 
> > >>>> What does device creation have to do with anything? Surely a device
> > >>>> won't need IOMMU services before the device is bound to a driver.
> > >>> 
> > >>> The problem is that the driver will start using the IOMMU as soon
> > >>> as it calls dma_map_*, but that happens at runtime, not necessarily
> > >>> during the probe function.
> > >>> 
> > >>> So we can get into the weird situation that probe() returns success,
> > >>> but then you can't use the device yet because you don't know whether
> > >>> it is supposed to use an IOMMU or not.
> > >> 
> > >> If we want IOMMU devices to be supported by common device drivers we
> > >> need to defer probing of the master devices, there's no doubt about
> > >> that. Earlier approaches that hooked up into the device core code were
> > >> rejected, but it should be possible to use bus notifiers to achieve the
> > >> same result (with the drawback of having to register one notifier per
> > >> bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just return -
> > >> EPROBE_DEFER when a iommus property is available and points to an IOMMU
> > >> not registered yet. I'm not saying we have to do this, but I believe that
> > >> at least from a technical point of view it could be done.
> > > 
> > > I think that fundamentally speaking, relying on notifiers for something
> > > like this is very problematic, both in terms of maintainability and
> > > reliability. We should really try to get the notifiers out of the iommu
> > > handling, not put more of them in.
> > 
> > Agreed. Also last time I checked the driver core simply ignored the
> > return value from notifiers, therefore this wouldn't work without
> > changing the core either.
> > 
> > Still, I agree with Laurent that we really should be relying on probe
> > deferral for probe ordering.
> 
> *If* we decide to support IOMMUs with real devices ;-)
> 
> I don't have a strong opinion on the subject. I initially thought it would be 
> a shame not to be able to use devres, until realizing that binding to a DT 
> node instead of a device meant that no unbind can ever occur. Loosing dev_* 
> support is also an annoyance though.

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Laurent Pinchart]

Hi Thierry,

On Tuesday 14 October 2014 17:05:29 Thierry Reding wrote:
> On Tue, Oct 14, 2014 at 06:01:58PM +0300, Laurent Pinchart wrote:
> > On Tuesday 14 October 2014 15:37:59 Thierry Reding wrote:
> >> On Tue, Oct 14, 2014 at 03:20:46PM +0200, Arnd Bergmann wrote:
> >>> On Tuesday 14 October 2014 16:07:38 Laurent Pinchart wrote:
> >>>> On Tuesday 23 September 2014 09:44:25 Arnd Bergmann wrote:
> >>>>> On Tuesday 23 September 2014 09:02:39 Thierry Reding wrote:
> >>>>>>> I see two problems with using deferred probing here:
> >>>>>>> 
> >>>>>>> - we don't actually need to defer the probing but the binding to
> >>>>>>>   the driver when no dma ops are set, but it seems silly to even
> >>>>>>>   create the device before we can find out which ops it should
> >>>>>>>   use.
> >>>>>> 
> >>>>>> What does device creation have to do with anything? Surely a device
> >>>>>> won't need IOMMU services before the device is bound to a driver.
> >>>>> 
> >>>>> The problem is that the driver will start using the IOMMU as soon
> >>>>> as it calls dma_map_*, but that happens at runtime, not necessarily
> >>>>> during the probe function.
> >>>>> 
> >>>>> So we can get into the weird situation that probe() returns success,
> >>>>> but then you can't use the device yet because you don't know whether
> >>>>> it is supposed to use an IOMMU or not.
> >>>> 
> >>>> If we want IOMMU devices to be supported by common device drivers we
> >>>> need to defer probing of the master devices, there's no doubt about
> >>>> that. Earlier approaches that hooked up into the device core code
> >>>> were rejected, but it should be possible to use bus notifiers to
> >>>> achieve the same result (with the drawback of having to register one
> >>>> notifier per bus). The BUS_NOTIFY_BIND_DRIVER notifier can then just
> >>>> return -EPROBE_DEFER when a iommus property is available and points to
> >>>> an IOMMU not registered yet. I'm not saying we have to do this, but I
> >>>> believe that at least from a technical point of view it could be done.
> >>> 
> >>> I think that fundamentally speaking, relying on notifiers for
> >>> something like this is very problematic, both in terms of
> >>> maintainability and reliability. We should really try to get the
> >>> notifiers out of the iommu handling, not put more of them in.
> >> 
> >> Agreed. Also last time I checked the driver core simply ignored the
> >> return value from notifiers, therefore this wouldn't work without
> >> changing the core either.
> >> 
> >> Still, I agree with Laurent that we really should be relying on probe
> >> deferral for probe ordering.
> > 
> > *If* we decide to support IOMMUs with real devices ;-)
> > 
> > I don't have a strong opinion on the subject. I initially thought it would
> > be a shame not to be able to use devres, until realizing that binding to
> > a DT node instead of a device meant that no unbind can ever occur.
> > Loosing dev_* support is also an annoyance though.
> 
> Binding to a DT node then also means that you can't build the driver as
> a module. And in particular for multiplatform kernels this is going to
> be a problem eventually.

It will, but other dependencies will make multiplatform kernels unpractical 
with or without IOMMUs anyway. Many platforms need clocks, timers, pin 
control, gpio, i2c and pmic support to boot to userspace. Agreed, that's not a 
valid reason to make the problem worse just for the sake of it.

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

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

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

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

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 929 bytes --]

On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
[...]
> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
> +{
> +	struct dma_iommu_mapping *mapping;
> +
> +	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);

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

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Laurent Pinchart]

[-- Attachment #1.1: Type: text/plain, Size: 1166 bytes --]

On Monday 22 September 2014 11:19:35 Thierry Reding wrote:
> On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
> [...]
> 
> > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
> > size) +{
> > +	struct dma_iommu_mapping *mapping;
> > +
> > +	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
> 
> If I understand correctly this will be called for each device that has
> an IOMMU master interface and will end up creating a new mapping for
> each of the devices. Each of these mappings will translate to a domain
> in the IOMMU API, which in turn is a separate address space.
> 
> How do you envision to support use-cases where a set of devices need to
> share a single domain? This is needed for example in DRM where SoCs
> often have a set of hardware blocks (each with its own master interface)
> that compose the display device. On Tegra for example there are two
> display controllers that need access to the same IOVA domain so that
> they can scan out framebuffers.

Or simply for IOMMUs that serve multiple masters and support a single domain 
only.

-- 
Regards,

Laurent Pinchart

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Will Deacon]

Hi Thierry,

On Mon, Sep 22, 2014 at 10:19:35AM +0100, Thierry Reding wrote:
> On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
> [...]
> > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
> > +{
> > +	struct dma_iommu_mapping *mapping;
> > +
> > +	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
> 
> If I understand correctly this will be called for each device that has
> an IOMMU master interface and will end up creating a new mapping for
> each of the devices. Each of these mappings will translate to a domain
> in the IOMMU API, which in turn is a separate address space.

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

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

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

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

Will

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2825 bytes --]

On Mon, Sep 22, 2014 at 06:43:37PM +0100, Will Deacon wrote:
> Hi Thierry,
> 
> On Mon, Sep 22, 2014 at 10:19:35AM +0100, Thierry Reding wrote:
> > On Fri, Sep 12, 2014 at 05:34:55PM +0100, Will Deacon wrote:
> > [...]
> > > +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size)
> > > +{
> > > +	struct dma_iommu_mapping *mapping;
> > > +
> > > +	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
> > 
> > If I understand correctly this will be called for each device that has
> > an IOMMU master interface and will end up creating a new mapping for
> > each of the devices. Each of these mappings will translate to a domain
> > in the IOMMU API, which in turn is a separate address space.
> 
> Correct, although that's largely because I've bolted on the existing ARM
> IOMMU code.
> 
> > How do you envision to support use-cases where a set of devices need to
> > share a single domain? This is needed for example in DRM where SoCs
> > often have a set of hardware blocks (each with its own master interface)
> > that compose the display device. On Tegra for example there are two
> > display controllers that need access to the same IOVA domain so that
> > they can scan out framebuffers.
> 
> Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops
> contains a domain and an allocator for each IOMMU instance in the system.
> It would then be up to the architecture how it makes use of those, but
> the most obvious thing to do would be to attach devices mastering through
> an IOMMU instance to that per-instance domain.
> 
> The other use-case is isolation (one domain per device), which I guess
> matches what the ARM code is doing at the moment.

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

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Will Deacon]

On Tue, Sep 23, 2014 at 08:14:01AM +0100, Thierry Reding wrote:
> On Mon, Sep 22, 2014 at 06:43:37PM +0100, Will Deacon wrote:
> > Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops
> > contains a domain and an allocator for each IOMMU instance in the system.
> > It would then be up to the architecture how it makes use of those, but
> > the most obvious thing to do would be to attach devices mastering through
> > an IOMMU instance to that per-instance domain.
> > 
> > The other use-case is isolation (one domain per device), which I guess
> > matches what the ARM code is doing at the moment.
> 
> I think there are two cases here. You can have a composite device that
> wants to manage a single domain (using its own allocator) for a set of
> hardware devices. At the same time a set of devices (think 2D and 3D
> engines) could want to use a multiple domains for process separation.
> In that case I'd expect a logical DRM device to allocate one domain per
> process and then associate the 2D and 3D engines with that same domain
> on process switch.

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

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

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

Will

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4218 bytes --]

On Wed, Sep 24, 2014 at 05:33:38PM +0100, Will Deacon wrote:
> On Tue, Sep 23, 2014 at 08:14:01AM +0100, Thierry Reding wrote:
> > On Mon, Sep 22, 2014 at 06:43:37PM +0100, Will Deacon wrote:
> > > Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops
> > > contains a domain and an allocator for each IOMMU instance in the system.
> > > It would then be up to the architecture how it makes use of those, but
> > > the most obvious thing to do would be to attach devices mastering through
> > > an IOMMU instance to that per-instance domain.
> > > 
> > > The other use-case is isolation (one domain per device), which I guess
> > > matches what the ARM code is doing at the moment.
> > 
> > I think there are two cases here. You can have a composite device that
> > wants to manage a single domain (using its own allocator) for a set of
> > hardware devices. At the same time a set of devices (think 2D and 3D
> > engines) could want to use a multiple domains for process separation.
> > In that case I'd expect a logical DRM device to allocate one domain per
> > process and then associate the 2D and 3D engines with that same domain
> > on process switch.
> 
> Sure, but that's well outside of what the dma-mapping API is going to setup
> as a default domain. These specialist setups are certainly possible, but I
> think they should be driven by, for example, the DRM code as opposed to
> being in the core dma-mapping code.

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

So I think what we're going to need is a way to prevent the default
attachment to DMA/IOMMU. Or alternatively not associate devices with
IOMMU domains by default but let drivers explicitly make the decision.
Either of those two alternatives would require driver-specific
knowledge, which would be another strong argument against doing the
whole IOMMU initialization at device creation time.

> > What I proposed a while back was to leave it up to the IOMMU driver to
> > choose an allocator for the device. Or rather, choose whether to use a
> > custom allocator or the DMA/IOMMU integration allocator. The way this
> > worked was to keep a list of devices in the IOMMU driver. Devices in
> > this list would be added to domain reserved for DMA/IOMMU integration.
> > Those would typically be devices such as SD/MMC, audio, ... devices that
> > are in-kernel and need no per-process separation. By default devices
> > wouldn't be added to a domain, so devices forming a composite DRM device
> > would be able to manage their own domain.
> 
> I'd live to have as little of this as possible in the IOMMU drivers, as we
> should leave those to deal with the IOMMU hardware and not domain
> management. Having subsystems manage their own dma ops is an extension to
> the dma-mapping API.

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

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

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

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Will Deacon]

Hi Thierry,

On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
> On Wed, Sep 24, 2014 at 05:33:38PM +0100, Will Deacon wrote:
> > On Tue, Sep 23, 2014 at 08:14:01AM +0100, Thierry Reding wrote:
> > > On Mon, Sep 22, 2014 at 06:43:37PM +0100, Will Deacon wrote:
> > > > Yup. In this case, the iommu_dma_mapping passed to arch_setup_dma_ops
> > > > contains a domain and an allocator for each IOMMU instance in the system.
> > > > It would then be up to the architecture how it makes use of those, but
> > > > the most obvious thing to do would be to attach devices mastering through
> > > > an IOMMU instance to that per-instance domain.
> > > > 
> > > > The other use-case is isolation (one domain per device), which I guess
> > > > matches what the ARM code is doing at the moment.
> > > 
> > > I think there are two cases here. You can have a composite device that
> > > wants to manage a single domain (using its own allocator) for a set of
> > > hardware devices. At the same time a set of devices (think 2D and 3D
> > > engines) could want to use a multiple domains for process separation.
> > > In that case I'd expect a logical DRM device to allocate one domain per
> > > process and then associate the 2D and 3D engines with that same domain
> > > on process switch.
> > 
> > Sure, but that's well outside of what the dma-mapping API is going to setup
> > as a default domain. These specialist setups are certainly possible, but I
> > think they should be driven by, for example, the DRM code as opposed to
> > being in the core dma-mapping code.
> 
> I completely agree that these special cases should be driven by the
> drivers that need them. However the problem here is that the current
> patch will already attach the device to an IOMMU domain by default.

Sure, but that's not an unfixable problem if somebody cares enough to do it.
Right now, I see a small handful of callers for the IOMMU API and nearly all
of them would work perfectly well with a default domain. The big exception
to that is VFIO, but that requires the device to be unbound from the host
driver, so we could detach the mapping at that point.

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

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

> > > What I proposed a while back was to leave it up to the IOMMU driver to
> > > choose an allocator for the device. Or rather, choose whether to use a
> > > custom allocator or the DMA/IOMMU integration allocator. The way this
> > > worked was to keep a list of devices in the IOMMU driver. Devices in
> > > this list would be added to domain reserved for DMA/IOMMU integration.
> > > Those would typically be devices such as SD/MMC, audio, ... devices that
> > > are in-kernel and need no per-process separation. By default devices
> > > wouldn't be added to a domain, so devices forming a composite DRM device
> > > would be able to manage their own domain.
> > 
> > I'd live to have as little of this as possible in the IOMMU drivers, as we
> > should leave those to deal with the IOMMU hardware and not domain
> > management. Having subsystems manage their own dma ops is an extension to
> > the dma-mapping API.
> 
> It's not an extension, really. It's more that both need to be able to
> coexist. For some devices you may want to create an IOMMU domain and
> hook it up with the DMA mapping functions, for others you don't and
> handle mapping to IOVA space explicitly.

I think it's an extension in the sense that mainline doesn't currently do
what you want, regardless of this patch series. My motivation is to enable
IOMMU-backed DMA-mapping so that I can continue implementing the virtual
SMMU work I started a while back. Patches welcome to enable any other
use-cases -- I don't think they're mutually exclusive.

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

I agree here, the ARM DMA-mapping code should really be doing one domain
per SMMU instance; all I've done is hook up the existing code which wasn't
previously being called.

> Doing so would leave a large number of address spaces available for
> things like a GPU driver to keep per-process address spaces for
> isolation.
> 
> I don't see how we'd be able to do that with the approach that you
> propose in this series since it assumes that each device will be
> associated with a separate domain.

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

Will

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3846 bytes --]

On Tue, Sep 30, 2014 at 05:00:35PM +0100, Will Deacon wrote:
> On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
[...]
> > So I think what we're going to need is a way to prevent the default
> > attachment to DMA/IOMMU. Or alternatively not associate devices with
> > IOMMU domains by default but let drivers explicitly make the decision.
> 
> Which drivers and how would they know what to do? I think you might be
> jumping the gun a bit here, given where mainline is with using the IOMMU
> for anything at all.

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

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

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

> > > > What I proposed a while back was to leave it up to the IOMMU driver to
> > > > choose an allocator for the device. Or rather, choose whether to use a
> > > > custom allocator or the DMA/IOMMU integration allocator. The way this
> > > > worked was to keep a list of devices in the IOMMU driver. Devices in
> > > > this list would be added to domain reserved for DMA/IOMMU integration.
> > > > Those would typically be devices such as SD/MMC, audio, ... devices that
> > > > are in-kernel and need no per-process separation. By default devices
> > > > wouldn't be added to a domain, so devices forming a composite DRM device
> > > > would be able to manage their own domain.
> > > 
> > > I'd live to have as little of this as possible in the IOMMU drivers, as we
> > > should leave those to deal with the IOMMU hardware and not domain
> > > management. Having subsystems manage their own dma ops is an extension to
> > > the dma-mapping API.
> > 
> > It's not an extension, really. It's more that both need to be able to
> > coexist. For some devices you may want to create an IOMMU domain and
> > hook it up with the DMA mapping functions, for others you don't and
> > handle mapping to IOVA space explicitly.
> 
> I think it's an extension in the sense that mainline doesn't currently do
> what you want, regardless of this patch series.

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

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

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

> > Doing so would leave a large number of address spaces available for
> > things like a GPU driver to keep per-process address spaces for
> > isolation.
> > 
> > I don't see how we'd be able to do that with the approach that you
> > propose in this series since it assumes that each device will be
> > associated with a separate domain.
> 
> No, that's an artifact of the existing code on ARM. My series adds a list of
> domains to each device, but those domains are per-IOMMU instance and can
> appear in multiple lists.

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Will Deacon]

Hi Thierry,

On Wed, Oct 01, 2014 at 09:46:10AM +0100, Thierry Reding wrote:
> On Tue, Sep 30, 2014 at 05:00:35PM +0100, Will Deacon wrote:
> > On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
> [...]
> > > So I think what we're going to need is a way to prevent the default
> > > attachment to DMA/IOMMU. Or alternatively not associate devices with
> > > IOMMU domains by default but let drivers explicitly make the decision.
> > 
> > Which drivers and how would they know what to do? I think you might be
> > jumping the gun a bit here, given where mainline is with using the IOMMU
> > for anything at all.
> 
> I don't think I am. I've been working on patches to enable IOMMU on
> Tegra, with the specific use-case that we want to use it to allow
> physically non-contiguous framebuffers to be used for scan out.
> 
> In order to do so the DRM driver allocates an IOMMU domain and adds both
> display controllers to it. When a framebuffer is created or imported
> from DMA-BUF, it gets mapped into this domain and both display
> controllers can use the IOVA address as the framebuffer base address.

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

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

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

> > > > > What I proposed a while back was to leave it up to the IOMMU driver to
> > > > > choose an allocator for the device. Or rather, choose whether to use a
> > > > > custom allocator or the DMA/IOMMU integration allocator. The way this
> > > > > worked was to keep a list of devices in the IOMMU driver. Devices in
> > > > > this list would be added to domain reserved for DMA/IOMMU integration.
> > > > > Those would typically be devices such as SD/MMC, audio, ... devices that
> > > > > are in-kernel and need no per-process separation. By default devices
> > > > > wouldn't be added to a domain, so devices forming a composite DRM device
> > > > > would be able to manage their own domain.
> > > > 
> > > > I'd live to have as little of this as possible in the IOMMU drivers, as we
> > > > should leave those to deal with the IOMMU hardware and not domain
> > > > management. Having subsystems manage their own dma ops is an extension to
> > > > the dma-mapping API.
> > > 
> > > It's not an extension, really. It's more that both need to be able to
> > > coexist. For some devices you may want to create an IOMMU domain and
> > > hook it up with the DMA mapping functions, for others you don't and
> > > handle mapping to IOVA space explicitly.
> > 
> > I think it's an extension in the sense that mainline doesn't currently do
> > what you want, regardless of this patch series.
> 
> It's interesting since you're now the second person to say this. Can you
> please elaborate why you think that's the case?

Because the only way to set up DMA through an IOMMU on ARM is via the
arm_iommu_* functions, which are currently called from a subset of the
IOMMU drivers themselves:

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

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

> I do have local patches that allow precisely this use-case to work
> without changes to the IOMMU core or requiring any extra ARM-specific
> glue.
> 
> There's a fair bit of jumping through hoops, because for example you
> don't know what IOMMU instance a domain belongs to at .domain_init()
> time, so I have to defer most of the actual domain initalization until a
> device is actually attached to it, but I digress.
> 
> > > Doing so would leave a large number of address spaces available for
> > > things like a GPU driver to keep per-process address spaces for
> > > isolation.
> > > 
> > > I don't see how we'd be able to do that with the approach that you
> > > propose in this series since it assumes that each device will be
> > > associated with a separate domain.
> > 
> > No, that's an artifact of the existing code on ARM. My series adds a list of
> > domains to each device, but those domains are per-IOMMU instance and can
> > appear in multiple lists.
> 
> So you're saying the end result will be that there's a single domain per
> IOMMU device that will be associated with all devices that have a master
> interface to it?

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

Will

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6871 bytes --]

On Fri, Oct 03, 2014 at 04:08:50PM +0100, Will Deacon wrote:
> Hi Thierry,
> 
> On Wed, Oct 01, 2014 at 09:46:10AM +0100, Thierry Reding wrote:
> > On Tue, Sep 30, 2014 at 05:00:35PM +0100, Will Deacon wrote:
> > > On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
> > [...]
> > > > So I think what we're going to need is a way to prevent the default
> > > > attachment to DMA/IOMMU. Or alternatively not associate devices with
> > > > IOMMU domains by default but let drivers explicitly make the decision.
> > > 
> > > Which drivers and how would they know what to do? I think you might be
> > > jumping the gun a bit here, given where mainline is with using the IOMMU
> > > for anything at all.
> > 
> > I don't think I am. I've been working on patches to enable IOMMU on
> > Tegra, with the specific use-case that we want to use it to allow
> > physically non-contiguous framebuffers to be used for scan out.
> > 
> > In order to do so the DRM driver allocates an IOMMU domain and adds both
> > display controllers to it. When a framebuffer is created or imported
> > from DMA-BUF, it gets mapped into this domain and both display
> > controllers can use the IOVA address as the framebuffer base address.
> 
> Does that mean you manually swizzle the dma_map_ops for the device in the
> DRM driver?

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

> > Given that a device can only be attached to a single domain at a time
> > this will cause breakage when the ARM glue code starts automatically
> > attaching the display controllers to a default domain.
> 
> Why couldn't you just re-use the domain already allocated by the DMA mapping
> API?

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

> > > > > > What I proposed a while back was to leave it up to the IOMMU driver to
> > > > > > choose an allocator for the device. Or rather, choose whether to use a
> > > > > > custom allocator or the DMA/IOMMU integration allocator. The way this
> > > > > > worked was to keep a list of devices in the IOMMU driver. Devices in
> > > > > > this list would be added to domain reserved for DMA/IOMMU integration.
> > > > > > Those would typically be devices such as SD/MMC, audio, ... devices that
> > > > > > are in-kernel and need no per-process separation. By default devices
> > > > > > wouldn't be added to a domain, so devices forming a composite DRM device
> > > > > > would be able to manage their own domain.
> > > > > 
> > > > > I'd live to have as little of this as possible in the IOMMU drivers, as we
> > > > > should leave those to deal with the IOMMU hardware and not domain
> > > > > management. Having subsystems manage their own dma ops is an extension to
> > > > > the dma-mapping API.
> > > > 
> > > > It's not an extension, really. It's more that both need to be able to
> > > > coexist. For some devices you may want to create an IOMMU domain and
> > > > hook it up with the DMA mapping functions, for others you don't and
> > > > handle mapping to IOVA space explicitly.
> > > 
> > > I think it's an extension in the sense that mainline doesn't currently do
> > > what you want, regardless of this patch series.
> > 
> > It's interesting since you're now the second person to say this. Can you
> > please elaborate why you think that's the case?
> 
> Because the only way to set up DMA through an IOMMU on ARM is via the
> arm_iommu_* functions,

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

> which are currently called from a subset of the IOMMU drivers themselves:
> 
>   drivers/gpu/drm/exynos/exynos_drm_iommu.c
>   drivers/iommu/ipmmu-vmsa.c
>   drivers/iommu/shmobile-iommu.c
>   drivers/media/platform/omap3isp/isp.c
> 
> Of these, ipmmu-vmsa.c and shmobile.c both allocate a domain per device.
> The omap3 code seems to do something similar. That just leaves the exynos
> driver, which Marek has been reworking anyway.

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

> > I do have local patches that allow precisely this use-case to work
> > without changes to the IOMMU core or requiring any extra ARM-specific
> > glue.
> > 
> > There's a fair bit of jumping through hoops, because for example you
> > don't know what IOMMU instance a domain belongs to at .domain_init()
> > time, so I have to defer most of the actual domain initalization until a
> > device is actually attached to it, but I digress.
> > 
> > > > Doing so would leave a large number of address spaces available for
> > > > things like a GPU driver to keep per-process address spaces for
> > > > isolation.
> > > > 
> > > > I don't see how we'd be able to do that with the approach that you
> > > > propose in this series since it assumes that each device will be
> > > > associated with a separate domain.
> > > 
> > > No, that's an artifact of the existing code on ARM. My series adds a list of
> > > domains to each device, but those domains are per-IOMMU instance and can
> > > appear in multiple lists.
> > 
> > So you're saying the end result will be that there's a single domain per
> > IOMMU device that will be associated with all devices that have a master
> > interface to it?
> 
> Yes, that's the plan. Having thought about it some more (after your
> comments), subsystems can still call of_dma_deconfigure if they want to do
> their own IOMMU domain management. That may well be useful for things like
> VFIO, for example.

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

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

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

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Laurent Pinchart]

Hi Thierry and Will,

On Monday 06 October 2014 11:52:50 Thierry Reding wrote:
> On Fri, Oct 03, 2014 at 04:08:50PM +0100, Will Deacon wrote:
> > On Wed, Oct 01, 2014 at 09:46:10AM +0100, Thierry Reding wrote:
> >> On Tue, Sep 30, 2014 at 05:00:35PM +0100, Will Deacon wrote:
> >>> On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
> >> [...]
> >> 
> >>>> So I think what we're going to need is a way to prevent the default
> >>>> attachment to DMA/IOMMU. Or alternatively not associate devices with
> >>>> IOMMU domains by default but let drivers explicitly make the
> >>>> decision.
> >>> 
> >>> Which drivers and how would they know what to do? I think you might be
> >>> jumping the gun a bit here, given where mainline is with using the
> >>> IOMMU for anything at all.
> >> 
> >> I don't think I am. I've been working on patches to enable IOMMU on
> >> Tegra, with the specific use-case that we want to use it to allow
> >> physically non-contiguous framebuffers to be used for scan out.
> >> 
> >> In order to do so the DRM driver allocates an IOMMU domain and adds both
> >> display controllers to it. When a framebuffer is created or imported
> >> from DMA-BUF, it gets mapped into this domain and both display
> >> controllers can use the IOVA address as the framebuffer base address.
> > 
> > Does that mean you manually swizzle the dma_map_ops for the device in the
> > DRM driver?
> 
> No. It means we use the IOMMU API directly instead of the DMA mapping
> API.

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

> >> Given that a device can only be attached to a single domain at a time
> >> this will cause breakage when the ARM glue code starts automatically
> >> attaching the display controllers to a default domain.
> > 
> > Why couldn't you just re-use the domain already allocated by the DMA
> > mapping API?
> 
> Because I don't see how you'd get access to it. And provided that we
> could do that it would also mean that there'd be at least two domains
> (one for each display controller) and we'd need to decide on using a
> single one of them. Which one do we choose? And what about the unused
> one? If there's no way to detach it we loose a precious resource.

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

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

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

> >>>>> I'd live to have as little of this as possible in the IOMMU
> >>>>> drivers, as we should leave those to deal with the IOMMU hardware
> >>>>> and not domain management. Having subsystems manage their own dma
> >>>>> ops is an extension to the dma-mapping API.
> >>>> 
> >>>> It's not an extension, really. It's more that both need to be able
> >>>> to coexist. For some devices you may want to create an IOMMU domain
> >>>> and hook it up with the DMA mapping functions, for others you don't
> >>>> and handle mapping to IOVA space explicitly.
> >>> 
> >>> I think it's an extension in the sense that mainline doesn't currently
> >>> do what you want, regardless of this patch series.
> >> 
> >> It's interesting since you're now the second person to say this. Can you
> >> please elaborate why you think that's the case?
> > 
> > Because the only way to set up DMA through an IOMMU on ARM is via the
> > arm_iommu_* functions,
> 
> No, you can use the IOMMU API directly just fine.
> 
> > which are currently called from a subset of the IOMMU drivers themselves:
> >   drivers/gpu/drm/exynos/exynos_drm_iommu.c
> >   drivers/iommu/ipmmu-vmsa.c
> >   drivers/iommu/shmobile-iommu.c
> >   drivers/media/platform/omap3isp/isp.c
> > 
> > Of these, ipmmu-vmsa.c and shmobile.c both allocate a domain per device.
> > The omap3 code seems to do something similar. That just leaves the exynos
> > driver, which Marek has been reworking anyway.
> 
> Right, and as I remember one of the things that Marek did was introduce
> a flag to mark drivers as doing their own IOMMU domain management so
> that they wouldn't be automatically associated with a "mapping".
> 
> >> I do have local patches that allow precisely this use-case to work
> >> without changes to the IOMMU core or requiring any extra ARM-specific
> >> glue.
> >> 
> >> There's a fair bit of jumping through hoops, because for example you
> >> don't know what IOMMU instance a domain belongs to at .domain_init()
> >> time, so I have to defer most of the actual domain initalization until a
> >> device is actually attached to it, but I digress.
> >> 
> >>>> Doing so would leave a large number of address spaces available for
> >>>> things like a GPU driver to keep per-process address spaces for
> >>>> isolation.
> >>>> 
> >>>> I don't see how we'd be able to do that with the approach that you
> >>>> propose in this series since it assumes that each device will be
> >>>> associated with a separate domain.
> >>> 
> >>> No, that's an artifact of the existing code on ARM. My series adds a
> >>> list of domains to each device, but those domains are per-IOMMU
> >>> instance and can appear in multiple lists.
> >> 
> >> So you're saying the end result will be that there's a single domain per
> >> IOMMU device that will be associated with all devices that have a master
> >> interface to it?
> > 
> > Yes, that's the plan. Having thought about it some more (after your
> > comments), subsystems can still call of_dma_deconfigure if they want to do
> > their own IOMMU domain management. That may well be useful for things like
> > VFIO, for example.
> 
> I think it's really weird to set up some complicated data structures at
> early boot without knowing that they'll ever be used and then require
> drivers to undo that if they decide not to use it.
> 
> As mentioned in an earlier reply I don't see why we need to set this all
> up that early in the boot in the first place. It only becomes important
> right before a driver's .probe() is called because the device can't
> perform any DMA-related operations before that point in time.
> 
> Now if we postpone initialization of the IOMMU masters and swizzling of
> the DMA operations until driver probe time we get rid of a lot of
> problems. For example we could use deferred probing if the IOMMU driver
> hasn't loaded yet. That in turn would allow IOMMU drivers to be built as
> modules rather than built-in. And especially with multi-platform kernels
> I think we really want to build as much as possible as modules.

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

-- 
Regards,

Laurent Pinchart

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7064 bytes --]

On Mon, Oct 06, 2014 at 01:50:40PM +0300, Laurent Pinchart wrote:
> Hi Thierry and Will,
> 
> On Monday 06 October 2014 11:52:50 Thierry Reding wrote:
> > On Fri, Oct 03, 2014 at 04:08:50PM +0100, Will Deacon wrote:
> > > On Wed, Oct 01, 2014 at 09:46:10AM +0100, Thierry Reding wrote:
> > >> On Tue, Sep 30, 2014 at 05:00:35PM +0100, Will Deacon wrote:
> > >>> On Thu, Sep 25, 2014 at 07:40:23AM +0100, Thierry Reding wrote:
> > >> [...]
> > >> 
> > >>>> So I think what we're going to need is a way to prevent the default
> > >>>> attachment to DMA/IOMMU. Or alternatively not associate devices with
> > >>>> IOMMU domains by default but let drivers explicitly make the
> > >>>> decision.
> > >>> 
> > >>> Which drivers and how would they know what to do? I think you might be
> > >>> jumping the gun a bit here, given where mainline is with using the
> > >>> IOMMU for anything at all.
> > >> 
> > >> I don't think I am. I've been working on patches to enable IOMMU on
> > >> Tegra, with the specific use-case that we want to use it to allow
> > >> physically non-contiguous framebuffers to be used for scan out.
> > >> 
> > >> In order to do so the DRM driver allocates an IOMMU domain and adds both
> > >> display controllers to it. When a framebuffer is created or imported
> > >> from DMA-BUF, it gets mapped into this domain and both display
> > >> controllers can use the IOVA address as the framebuffer base address.
> > > 
> > > Does that mean you manually swizzle the dma_map_ops for the device in the
> > > DRM driver?
> > 
> > No. It means we use the IOMMU API directly instead of the DMA mapping
> > API.
> 
> Is there a reason why you can't use the DMA mapping API for this, assuming of 
> course that it would provide a way to attach both display controllers to the 
> same domain ? Do you need to have explicit control over the VA at which the 
> buffers are mapped ?

I suppose I could use the DMA mapping API at least for the display parts
if both controllers could be attached to the same domain. However when
we get to the 2D and 3D parts we will probably want to switch the IOMMU
domain depending on the userspace context to prevent applications from
stepping on each other's toes.

And no, I don't need to have explicit control over which VA the buffers
get mapped to.

> > >> Given that a device can only be attached to a single domain at a time
> > >> this will cause breakage when the ARM glue code starts automatically
> > >> attaching the display controllers to a default domain.
> > > 
> > > Why couldn't you just re-use the domain already allocated by the DMA
> > > mapping API?
> > 
> > Because I don't see how you'd get access to it. And provided that we
> > could do that it would also mean that there'd be at least two domains
> > (one for each display controller) and we'd need to decide on using a
> > single one of them. Which one do we choose? And what about the unused
> > one? If there's no way to detach it we loose a precious resource.
> 
> This would also be an issue for my Renesas IOMMU (ipmmu-vmsa) use cases. The 
> IOMMU supports up to four domains (each of them having its own hardware TLB) 
> and shares them between all the bus masters connected to the IOMMU. The 
> connections between bus master and TLBs are configurable. I thus can't live 
> with one domain being created per device.

I suppose one could fake this behind the curtains by making several
domains correspond to the same TLB (it sounds like pretty much the same
concept as an address space on Tegra). But that's just really nasty in
my opinion.

> > >>>>>> What I proposed a while back was to leave it up to the IOMMU
> > >>>>>> driver to choose an allocator for the device. Or rather, choose
> > >>>>>> whether to use a custom allocator or the DMA/IOMMU integration
> > >>>>>> allocator. The way this worked was to keep a list of devices in
> > >>>>>> the IOMMU driver. Devices in this list would be added to domain
> > >>>>>> reserved for DMA/IOMMU integration. Those would typically be
> > >>>>>> devices such as SD/MMC, audio, ... devices that are in-kernel
> > >>>>>> and need no per-process separation. By default devices wouldn't
> > >>>>>> be added to a domain, so devices forming a composite DRM device
> > >>>>>> would be able to manage their own domain.
> 
> The problem with your solution is that it requires knowledge of all bus master 
> devices in the IOMMU driver. That's not where that knowledge belongs, as it's 
> a property of a particular SoC integration, not of the IOMMU itself.

Right. It will work nicely on Tegra where the IOMMU is closely tied to
the memory controller and therefore does in fact know about all of the
masters. It won't work for something more generic like the ARM SMMU
where the SoC integration really isn't a property of the IOMMU itself.

So Marek's proposal to mark drivers that don't need or want the DMA API
integration sounds like a pretty good alternative to me.

> > > Yes, that's the plan. Having thought about it some more (after your
> > > comments), subsystems can still call of_dma_deconfigure if they want to do
> > > their own IOMMU domain management. That may well be useful for things like
> > > VFIO, for example.
> > 
> > I think it's really weird to set up some complicated data structures at
> > early boot without knowing that they'll ever be used and then require
> > drivers to undo that if they decide not to use it.
> > 
> > As mentioned in an earlier reply I don't see why we need to set this all
> > up that early in the boot in the first place. It only becomes important
> > right before a driver's .probe() is called because the device can't
> > perform any DMA-related operations before that point in time.
> > 
> > Now if we postpone initialization of the IOMMU masters and swizzling of
> > the DMA operations until driver probe time we get rid of a lot of
> > problems. For example we could use deferred probing if the IOMMU driver
> > hasn't loaded yet. That in turn would allow IOMMU drivers to be built as
> > modules rather than built-in. And especially with multi-platform kernels
> > I think we really want to build as much as possible as modules.
> 
> For what it's worth, given that I have no code to show, I was about to try 
> implementing that when Will sent his patch set. My idea was to use a bus 
> notifier in the IOMMU core to defer probing of devices for which the IOMMU 
> isn't available yet based on the DT iommus property.

I already implemented something like that as part of an RFC that I sent
a while back[0] and specifically [1]. One variant of that had been
posted earlier by Hiroshi and it hooked nicely into the core so that it
would all happen transparently to the drivers. That earlier variant had
been NAK'ed because "we don't want that in the core", and my later
attempt was discussed a little and then ignored.

Thierry

[0]: https://lkml.org/lkml/2014/6/26/470
[1]: https://lkml.org/lkml/2014/6/26/476

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Robin Murphy]

Hi Will,

On 12/09/14 17:34, Will Deacon wrote:
> Hi all,
>
> Here is version three of the RFC I've previously posted here:
>
>    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
>    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
>
> Changes since RFCv2 include:
>
>    - Put the iommu_ops in iommu_data so of_iommu_configure can avoid using
>      the bus_type
>    - Initialise the offset and DMA masks on the dev in of_dma_configure
>      instead of in the arch callback (as this would cause a regression on
>      some architectures)
>    - Added deconfigure/teardown code based on ref counting the iommu_dma_mapping
>    - A bunch of small fixes (_OF_DECLARE,  some code shuffling, fix multiple
>      IOMMU parsing)
>
> All feedback welcome. Hopefully this is now at a point where people can
> start looking to port dma-mapping and/or IOMMU drivers to it.
>

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

Robin.

> Cheers,
>
> Will
>
> --->8
>
> Will Deacon (7):
>    iommu: provide early initialisation hook for IOMMU drivers
>    dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
>    iommu: add new iommu_ops callback for adding an OF device
>    iommu: provide helper function to configure an IOMMU for an of master
>    dma-mapping: detect and configure IOMMU in of_dma_configure
>    arm: call iommu_init before of_platform_populate
>    arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops
>
>   arch/arm/include/asm/dma-mapping.h |  9 ++--
>   arch/arm/kernel/setup.c            |  2 +
>   arch/arm/mm/dma-mapping.c          | 68 ++++++++++++++++++++++++++----
>   drivers/iommu/Kconfig              |  2 +-
>   drivers/iommu/of_iommu.c           | 84 ++++++++++++++++++++++++++++++++++++++
>   drivers/of/platform.c              | 53 +++++++++++++-----------
>   include/asm-generic/vmlinux.lds.h  |  2 +
>   include/linux/dma-mapping.h        | 21 +++++++---
>   include/linux/iommu.h              | 15 +++++++
>   include/linux/of_iommu.h           | 35 ++++++++++++++++
>   10 files changed, 247 insertions(+), 44 deletions(-)
>

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

[Will Deacon]

On Tue, Sep 16, 2014 at 12:40:27PM +0100, Robin Murphy wrote:
> On 12/09/14 17:34, Will Deacon wrote:
> > Here is version three of the RFC I've previously posted here:
> >
> >    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
> >    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
> >
> > Changes since RFCv2 include:
> >
> >    - Put the iommu_ops in iommu_data so of_iommu_configure can avoid using
> >      the bus_type
> >    - Initialise the offset and DMA masks on the dev in of_dma_configure
> >      instead of in the arch callback (as this would cause a regression on
> >      some architectures)
> >    - Added deconfigure/teardown code based on ref counting the iommu_dma_mapping
> >    - A bunch of small fixes (_OF_DECLARE,  some code shuffling, fix multiple
> >      IOMMU parsing)
> >
> > All feedback welcome. Hopefully this is now at a point where people can
> > start looking to port dma-mapping and/or IOMMU drivers to it.
> >
> 
> What about AMBA devices? Playing with this on Juno and wondering why my 
> PL330 doesn't get any of_xlate callbacks, I see that 
> of_amba_device_create doesn't call of_dma_configure or anything from 
> that path. It's easy to work around by removing the arm,primecell 
> compatible, but that feels pretty dirty.

Yeah, that's just a bug in mainline. I'll look at fixing it if you don't
beat me to it.

Will