[PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hello,

This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API. The
biggest issue is fixed by patch 5/5, while the other patches fix smaller
problems that I've noticed when reading the code, without experiencing them at
runtime.

I'd like to take this as an opportunity to discuss OMAP IOMMU integration with
the ARM DMA mapping implementation. The idea is to hide the IOMMU completely
behind the DMA mapping API, making it completely transparent to drivers.

A drivers will only need to allocate memory with dma_alloc_*, and behind the
scene the ARM DMA mapping implementation will find out that the device is
behind an IOMMU and will map the buffers through the IOMMU, returning an I/O
VA address to the driver. No direct call to the OMAP IOMMU driver or to the
IOMMU core should be necessary anymore.

To use the IOMMU the ARM DMA implementation requires a VA mapping to be
created with a call to arm_iommu_create_mapping() and to then be attached to
the device with arm_iommu_attach_device(). This is currently not performed by
the OMAP IOMMU driver, I have thus added that code to the OMAP3 ISP driver for
now. I believe this to still be an improvement compared to the current
situation, as it allows getting rid of custom memory allocation code in the
OMAP3 ISP driver and custom I/O VA space management in omap-iovmm. However,
that code should ideally be removed from the driver. The question is, where
should it be moved to ?

One possible solution would be to add the code to the OMAP IOMMU driver.
However, this would require all OMAP IOMMU users to be converted to the ARM
DMA API. I assume there would be issues that I don't foresee though.

I'm not even sure whether the OMAP IOMMU driver would be the best place to put
that code. Ideally VA spaces should be created by the platform somehow, and
mapping of devices to IOMMUs should be handled by the IOMMU core instead of
the IOMMU drivers. We're not there yet, and the path might not be
straightforward, hence this attempt to start a constructive discussion.

A second completely unrelated problem that I'd like to get feedback on is the
suspend/resume support in the OMAP IOMMU driver, or rather the lack thereof.
The driver exports omap_iommu_save_ctx() and omap_iommu_restore_ctx()
functions and expects the IOMMU users to call them directly. This is really
hackish to say the least. A proper suspend/resume implementation shouldn't be
difficult to implement, and I'm wondering whether the lack of proper power
management support comes from historical reasons only, or if there are
problems I might not have considered.

Last but not least, the patches are available at

	git://linuxtv.org/pinchartl/media.git omap3isp/dma

along with a patch series that ports the OMAP3 ISP driver to the DMA API. I
will submit that one for review once the IOMMU patches get accepted and after
fixing a couple of remaining bugs (I'm aware that I have broken userspace
PFNMAP buffers).

Laurent Pinchart (5):
  iommu/omap: Use the cache cleaning API
  iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
  iommu/omap: Flush the TLB only after updating translation table
    entries
  iommu/omap: Remove comment about supporting single page mappings only
  iommu/omap: Fix map protection value handling

 drivers/iommu/omap-iommu.c | 68 ++++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 45 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/5] iommu/omap: Use the cache cleaning API

[Laurent Pinchart]

The page table entries must be cleaned from the cache before being
accessed by the IOMMU. Instead of implementing cache management manually
(and ignoring L2 cache), use clean_dcache_area() to make sure the
entries are visible to the device.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/iommu/omap-iommu.c | 41 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index a893eca..bb605c9 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
 /*
  *	H/W pagetable operations
  */
-static void flush_iopgd_range(u32 *first, u32 *last)
+static void flush_pgtable(void *addr, size_t size)
 {
-	/* FIXME: L2 cache should be taken care of if it exists */
-	do {
-		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
-		    : : "r" (first));
-		first += L1_CACHE_BYTES / sizeof(*first);
-	} while (first <= last);
-}
-
-static void flush_iopte_range(u32 *first, u32 *last)
-{
-	/* FIXME: L2 cache should be taken care of if it exists */
-	do {
-		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
-		    : : "r" (first));
-		first += L1_CACHE_BYTES / sizeof(*first);
-	} while (first <= last);
+	clean_dcache_area(addr, size);
 }
 
 static void iopte_free(u32 *iopte)
@@ -546,7 +531,7 @@ static u32 *iopte_alloc(struct omap_iommu *obj, u32 *iopgd, u32 da)
 			return ERR_PTR(-ENOMEM);
 
 		*iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
-		flush_iopgd_range(iopgd, iopgd);
+		flush_pgtable(iopgd, sizeof(*iopgd));
 
 		dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
 	} else {
@@ -575,7 +560,7 @@ static int iopgd_alloc_section(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
 	}
 
 	*iopgd = (pa & IOSECTION_MASK) | prot | IOPGD_SECTION;
-	flush_iopgd_range(iopgd, iopgd);
+	flush_pgtable(iopgd, sizeof(*iopgd));
 	return 0;
 }
 
@@ -592,7 +577,7 @@ static int iopgd_alloc_super(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
 
 	for (i = 0; i < 16; i++)
 		*(iopgd + i) = (pa & IOSUPER_MASK) | prot | IOPGD_SUPER;
-	flush_iopgd_range(iopgd, iopgd + 15);
+	flush_pgtable(iopgd, sizeof(*iopgd) * 16);
 	return 0;
 }
 
@@ -605,7 +590,7 @@ static int iopte_alloc_page(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
 		return PTR_ERR(iopte);
 
 	*iopte = (pa & IOPAGE_MASK) | prot | IOPTE_SMALL;
-	flush_iopte_range(iopte, iopte);
+	flush_pgtable(iopte, sizeof(*iopte));
 
 	dev_vdbg(obj->dev, "%s: da:%08x pa:%08x pte:%p *pte:%08x\n",
 		 __func__, da, pa, iopte, *iopte);
@@ -619,18 +604,12 @@ static int iopte_alloc_large(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
 	u32 *iopte = iopte_alloc(obj, iopgd, da);
 	int i;
 
-	if ((da | pa) & ~IOLARGE_MASK) {
-		dev_err(obj->dev, "%s: %08x:%08x should aligned on %08lx\n",
-			__func__, da, pa, IOLARGE_SIZE);
-		return -EINVAL;
-	}
-
 	if (IS_ERR(iopte))
 		return PTR_ERR(iopte);
 
 	for (i = 0; i < 16; i++)
 		*(iopte + i) = (pa & IOLARGE_MASK) | prot | IOPTE_LARGE;
-	flush_iopte_range(iopte, iopte + 15);
+	flush_pgtable(iopte, sizeof(*iopte) * 16);
 	return 0;
 }
 
@@ -733,7 +712,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
 		}
 		bytes *= nent;
 		memset(iopte, 0, nent * sizeof(*iopte));
-		flush_iopte_range(iopte, iopte + (nent - 1) * sizeof(*iopte));
+		flush_pgtable(iopte, sizeof(*iopte) * nent);
 
 		/*
 		 * do table walk to check if this table is necessary or not
@@ -755,7 +734,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
 		bytes *= nent;
 	}
 	memset(iopgd, 0, nent * sizeof(*iopgd));
-	flush_iopgd_range(iopgd, iopgd + (nent - 1) * sizeof(*iopgd));
+	flush_pgtable(iopgd, sizeof(*iopgd) * nent);
 out:
 	return bytes;
 }
@@ -799,7 +778,7 @@ static void iopgtable_clear_entry_all(struct omap_iommu *obj)
 			iopte_free(iopte_offset(iopgd, 0));
 
 		*iopgd = 0;
-		flush_iopgd_range(iopgd, iopgd);
+		flush_pgtable(iopgd, sizeof(*iopgd));
 	}
 
 	flush_iotlb_all(obj);
-- 
1.8.3.2

[PATCH 2/5] iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()

[Laurent Pinchart]

The flush_iotlb_page() function prints a debug message when no
corresponding page was found in the TLB. That condition is incorrectly
checked and always resolves to true, given that the for_each_iotlb_cr()
loop is never interrupted and always reaches obj->nr_tlb_entries.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/iommu/omap-iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bb605c9..cb1e1de 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -376,6 +376,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
 {
 	int i;
 	struct cr_regs cr;
+	bool found = false;
 
 	pm_runtime_get_sync(obj->dev);
 
@@ -394,11 +395,12 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
 				__func__, start, da, bytes);
 			iotlb_load_cr(obj, &cr);
 			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
+			found = true;
 		}
 	}
 	pm_runtime_put_sync(obj->dev);
 
-	if (i == obj->nr_tlb_entries)
+	if (!found)
 		dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);
 }
 
-- 
1.8.3.2

[PATCH 3/5] iommu/omap: Flush the TLB only after updating translation table entries

[Laurent Pinchart]

Flushing the TLB before updating translation entries creates a race
condition and can lead to stale TLB entries if a translation request
arrives between flushing the TLB and updating the translation entries.
As there's no requirement to flush the TLB before updating the entries,
flush it after only.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/iommu/omap-iommu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index cb1e1de..fedd303 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e)
 {
 	int err;
 
-	flush_iotlb_page(obj, e->da);
 	err = iopgtable_store_entry_core(obj, e);
-	if (!err)
+	if (!err) {
+		flush_iotlb_page(obj, e->da);
 		prefetch_iotlb_entry(obj, e);
+	}
 	return err;
 }
 EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);
-- 
1.8.3.2

[PATCH 4/5] iommu/omap: Remove comment about supporting single page mappings only

[Laurent Pinchart]

The IOMMU core breaks out mappings into pages already, there's no
need to support mapping multiple pages in one go.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/iommu/omap-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index fedd303..59de3fc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1043,7 +1043,6 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 	int omap_pgsz;
 	u32 ret, flags;
 
-	/* we only support mapping a single iommu page for now */
 	omap_pgsz = bytes_to_iopgsz(bytes);
 	if (omap_pgsz < 0) {
 		dev_err(dev, "invalid size to map: %d\n", bytes);
-- 
1.8.3.2

[PATCH 5/5] iommu/omap: Fix map protection value handling

[Laurent Pinchart]

The prot flags passed to the IOMMU map handler are defined in
include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
IOMMU flags being interpreted as page sizes, leading to failures.

Hardcode the OMAP mapping parameters to little-endian, 8-bits and
non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
support read-only or write-only mappings, ignore the prot value.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/iommu/omap-iommu.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 59de3fc..9f7f1d4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1016,8 +1016,7 @@ static void iopte_cachep_ctor(void *iopte)
 	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
 }
 
-static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
-				   u32 flags)
+static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int pgsz)
 {
 	memset(e, 0, sizeof(*e));
 
@@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
 	e->pa		= pa;
 	e->valid	= 1;
 	/* FIXME: add OMAP1 support */
-	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
-	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
-	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
-	e->mixed	= flags & MMU_RAM_MIXED_MASK;
+	e->pgsz		= pgsz;
+	e->endian	= MMU_RAM_ENDIAN_LITTLE;
+	e->elsz		= MMU_RAM_ELSZ_8;
+	e->mixed	= 0;
 
 	return iopgsz_to_bytes(e->pgsz);
 }
@@ -1041,7 +1040,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 	struct device *dev = oiommu->dev;
 	struct iotlb_entry e;
 	int omap_pgsz;
-	u32 ret, flags;
+	u32 ret;
 
 	omap_pgsz = bytes_to_iopgsz(bytes);
 	if (omap_pgsz < 0) {
@@ -1051,9 +1050,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 
 	dev_dbg(dev, "mapping da 0x%lx to pa 0x%x size 0x%x\n", da, pa, bytes);
 
-	flags = omap_pgsz | prot;
-
-	iotlb_init_entry(&e, da, pa, flags);
+	iotlb_init_entry(&e, da, pa, omap_pgsz);
 
 	ret = omap_iopgtable_store_entry(oiommu, &e);
 	if (ret)
-- 
1.8.3.2

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> along with a patch series that ports the OMAP3 ISP driver to the DMA API. I
> will submit that one for review once the IOMMU patches get accepted and after
> fixing a couple of remaining bugs (I'm aware that I have broken userspace
> PFNMAP buffers).
>
> Laurent Pinchart (5):
>    iommu/omap: Use the cache cleaning API
>    iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
>    iommu/omap: Flush the TLB only after updating translation table
>      entries
>    iommu/omap: Remove comment about supporting single page mappings only
>    iommu/omap: Fix map protection value handling
>
>   drivers/iommu/omap-iommu.c | 68 ++++++++++++++++------------------------------
>   1 file changed, 23 insertions(+), 45 deletions(-)

Until I have time to read and think about the rest, for the series,

Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hello,

Suman, Florian, any opinion regarding this patch series ? Your opinion on the 
architecture-related question below would be appreciated as well, but I'd like 
to get this merged first (and especially patch 5/5) in the not too distant 
future to push OMAP3 ISP patches that depend on it.

On Saturday 08 March 2014 01:46:09 Laurent Pinchart wrote:
> Hello,
> 
> This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
> when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API.
> The biggest issue is fixed by patch 5/5, while the other patches fix
> smaller problems that I've noticed when reading the code, without
> experiencing them at runtime.
> 
> I'd like to take this as an opportunity to discuss OMAP IOMMU integration
> with the ARM DMA mapping implementation. The idea is to hide the IOMMU
> completely behind the DMA mapping API, making it completely transparent to
> drivers.
> 
> A drivers will only need to allocate memory with dma_alloc_*, and behind the
> scene the ARM DMA mapping implementation will find out that the device is
> behind an IOMMU and will map the buffers through the IOMMU, returning an
> I/O VA address to the driver. No direct call to the OMAP IOMMU driver or to
> the IOMMU core should be necessary anymore.
> 
> To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> created with a call to arm_iommu_create_mapping() and to then be attached to
> the device with arm_iommu_attach_device(). This is currently not performed
> by the OMAP IOMMU driver, I have thus added that code to the OMAP3 ISP
> driver for now. I believe this to still be an improvement compared to the
> current situation, as it allows getting rid of custom memory allocation
> code in the OMAP3 ISP driver and custom I/O VA space management in
> omap-iovmm. However, that code should ideally be removed from the driver.
> The question is, where should it be moved to ?
> 
> One possible solution would be to add the code to the OMAP IOMMU driver.
> However, this would require all OMAP IOMMU users to be converted to the ARM
> DMA API. I assume there would be issues that I don't foresee though.
> 
> I'm not even sure whether the OMAP IOMMU driver would be the best place to
> put that code. Ideally VA spaces should be created by the platform somehow,
> and mapping of devices to IOMMUs should be handled by the IOMMU core
> instead of the IOMMU drivers. We're not there yet, and the path might not
> be straightforward, hence this attempt to start a constructive discussion.
> 
> A second completely unrelated problem that I'd like to get feedback on is
> the suspend/resume support in the OMAP IOMMU driver, or rather the lack
> thereof. The driver exports omap_iommu_save_ctx() and
> omap_iommu_restore_ctx() functions and expects the IOMMU users to call them
> directly. This is really hackish to say the least. A proper suspend/resume
> implementation shouldn't be difficult to implement, and I'm wondering
> whether the lack of proper power management support comes from historical
> reasons only, or if there are problems I might not have considered.
> 
> Last but not least, the patches are available at
> 
> 	git://linuxtv.org/pinchartl/media.git omap3isp/dma
> 
> along with a patch series that ports the OMAP3 ISP driver to the DMA API. I
> will submit that one for review once the IOMMU patches get accepted and
> after fixing a couple of remaining bugs (I'm aware that I have broken
> userspace PFNMAP buffers).
> 
> Laurent Pinchart (5):
>   iommu/omap: Use the cache cleaning API
>   iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
>   iommu/omap: Flush the TLB only after updating translation table
>     entries
>   iommu/omap: Remove comment about supporting single page mappings only
>   iommu/omap: Fix map protection value handling
> 
>  drivers/iommu/omap-iommu.c | 68 +++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 45 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()

[Suman Anna]

Hi Laurent,

On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> The flush_iotlb_page() function prints a debug message when no
> corresponding page was found in the TLB. That condition is incorrectly
> checked and always resolves to true, given that the for_each_iotlb_cr()
> loop is never interrupted and always reaches obj->nr_tlb_entries.

Nice catch.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index bb605c9..cb1e1de 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -376,6 +376,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
>   {
>   	int i;
>   	struct cr_regs cr;
> +	bool found = false;
>
>   	pm_runtime_get_sync(obj->dev);
>
> @@ -394,11 +395,12 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
>   				__func__, start, da, bytes);
>   			iotlb_load_cr(obj, &cr);
>   			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
> +			found = true;

The patch is fine as it is, but I think the loop can be breaked when an 
entry is found for simplification. I do not expect that we will have 
multiple entries with the same da in the TLBs (it is a multi-hit fault 
scenario) to continue looping through all entries. The only means for 
the multi-hit scenario to happen is user error with programming TLBs 
directly. This function call seems to be added in general to take care 
of any prefetching. I don't have the complete history on the 
PREFETCH_IOTLB on why it was added, it doesn't seem to be enabled but 
this should ideally be left to the h/w unless you are locking a TLB 
entry. DSP/Bridge is the only one that uses locked TLB entries at the 
moment, but it is not using the OMAP IOMMU driver yet.

regards
Suman

>   		}
>   	}
>   	pm_runtime_put_sync(obj->dev);
>
> -	if (i == obj->nr_tlb_entries)
> +	if (!found)
>   		dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);
>   }
>
>

Re: [PATCH 3/5] iommu/omap: Flush the TLB only after updating translation table entries

[Suman Anna]

Hi Laurent,

On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> Flushing the TLB before updating translation entries creates a race
> condition and can lead to stale TLB entries if a translation request
> arrives between flushing the TLB and updating the translation entries.
> As there's no requirement to flush the TLB before updating the entries,
> flush it after only.

I do not expect a device to be actively using a region if we are about 
to change its mapping, we expect the access from the device to be only 
between a map and an unmap. The race condition (if any in such 
scenarios) would exist even after this patch, like after you programmed 
the entry, and the translation request comes before you flush the page. 
Then it is still operating on an older address, while you have already 
programmed a new one. An unmap currently clears the entry and flushes 
any TLB as well, so I don't think this patch makes a big difference.

regards
Suman

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index cb1e1de..fedd303 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>   {
>   	int err;
>
> -	flush_iotlb_page(obj, e->da);
>   	err = iopgtable_store_entry_core(obj, e);
> -	if (!err)
> +	if (!err) {
> +		flush_iotlb_page(obj, e->da);
>   		prefetch_iotlb_entry(obj, e);
> +	}
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);
>

Re: [PATCH 5/5] iommu/omap: Fix map protection value handling

[Suman Anna]

Hi Laurent,

On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> The prot flags passed to the IOMMU map handler are defined in
> include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
> driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
> IOMMU flags being interpreted as page sizes, leading to failures.
>
> Hardcode the OMAP mapping parameters to little-endian, 8-bits and
> non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
> support read-only or write-only mappings, ignore the prot value.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 59de3fc..9f7f1d4 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1016,8 +1016,7 @@ static void iopte_cachep_ctor(void *iopte)
>   	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
>   }
>
> -static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
> -				   u32 flags)
> +static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int pgsz)
>   {
>   	memset(e, 0, sizeof(*e));
>
> @@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
>   	e->pa		= pa;
>   	e->valid	= 1;
As I was looking through this, I found a small bug here. This is 
unrelated to this patch, but you can fix it in the same patch. The 
e->valid value is directly used in omap2_alloc_cr, so it should be 
initialized to MMU_CAM_V. It is not a problem currently as 
PREFETCH_IOTLB is not used, if used, this is overriding the size 
bit-fields when it prefetches the entry, which may lead to an MMU fault.


>   	/* FIXME: add OMAP1 support */
> -	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
> -	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
> -	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
> -	e->mixed	= flags & MMU_RAM_MIXED_MASK;
> +	e->pgsz		= pgsz;
> +	e->endian	= MMU_RAM_ENDIAN_LITTLE;
> +	e->elsz		= MMU_RAM_ELSZ_8;
> +	e->mixed	= 0;

Thanks, the change of these settings looks good overall. The only ones 
for which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per 
TRM, endianness conversion supported only on writes anyway, so I am 
assuming that we never use the combination), and OMAP2420 DSP, for which 
there is no driver currently nor is the MMU being instantiated. All 
other processors are pure little endian, and they even ignore the values 
written in these fields.

Please recheck the Camera subsystem once, and see if the e->elsz should 
be set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera 
usage, so you are probably in a better place than me).

If you are good, then with the e->valid change,
Acked-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>

regards
Suman

>
>   	return iopgsz_to_bytes(e->pgsz);
>   }
> @@ -1041,7 +1040,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
>   	struct device *dev = oiommu->dev;
>   	struct iotlb_entry e;
>   	int omap_pgsz;
> -	u32 ret, flags;
> +	u32 ret;
>
>   	omap_pgsz = bytes_to_iopgsz(bytes);
>   	if (omap_pgsz < 0) {
> @@ -1051,9 +1050,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
>
>   	dev_dbg(dev, "mapping da 0x%lx to pa 0x%x size 0x%x\n", da, pa, bytes);
>
> -	flags = omap_pgsz | prot;
> -
> -	iotlb_init_entry(&e, da, pa, flags);
> +	iotlb_init_entry(&e, da, pa, omap_pgsz);
>
>   	ret = omap_iopgtable_store_entry(oiommu, &e);
>   	if (ret)
>

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Suman Anna]

Hi Laurent,

On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> Hello,
>
> This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
> when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API. The
> biggest issue is fixed by patch 5/5, while the other patches fix smaller
> problems that I've noticed when reading the code, without experiencing them at
> runtime.
>
> I'd like to take this as an opportunity to discuss OMAP IOMMU integration with
> the ARM DMA mapping implementation. The idea is to hide the IOMMU completely
> behind the DMA mapping API, making it completely transparent to drivers.

Thanks for starting the discussion.

>
> A drivers will only need to allocate memory with dma_alloc_*, and behind the
> scene the ARM DMA mapping implementation will find out that the device is
> behind an IOMMU and will map the buffers through the IOMMU, returning an I/O
> VA address to the driver. No direct call to the OMAP IOMMU driver or to the
> IOMMU core should be necessary anymore.
>
> To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> created with a call to arm_iommu_create_mapping() and to then be attached to
> the device with arm_iommu_attach_device(). This is currently not performed by
> the OMAP IOMMU driver, I have thus added that code to the OMAP3 ISP driver for
> now. I believe this to still be an improvement compared to the current
> situation, as it allows getting rid of custom memory allocation code in the
> OMAP3 ISP driver and custom I/O VA space management in omap-iovmm. However,
> that code should ideally be removed from the driver. The question is, where
> should it be moved to ?
>
> One possible solution would be to add the code to the OMAP IOMMU driver.
> However, this would require all OMAP IOMMU users to be converted to the ARM
> DMA API. I assume there would be issues that I don't foresee though.

Yeah, I think this will pose some problems for the other main user of 
IOMMUs - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in 
OMAP4 and beyond). A remoteproc device also needs to map memory at 
specific addresses for its code and data sections, and not rely on a I/O 
VA address being given to it. The firmware sections are already linked 
at specific addresses, and so remoteproc needs to allocate memory, load 
the firmware and map into appropriate device addresses. We are doing 
this currently usage a combination of CMA memory to get contiguous 
memory (there are some restrictions for certain sections) and 
iommu_map/unmap API to program the MMU with these pages. This usage is 
different from what is expected from exchanging buffers, which can be 
allocated from a predefined mapping range. Even that one is tricky if we 
need to support different cache properties/attributes as the cache 
configuration is in general local to these processors.

>
> I'm not even sure whether the OMAP IOMMU driver would be the best place to put
> that code. Ideally VA spaces should be created by the platform somehow, and
> mapping of devices to IOMMUs should be handled by the IOMMU core instead of
> the IOMMU drivers. We're not there yet, and the path might not be
> straightforward, hence this attempt to start a constructive discussion.
>
> A second completely unrelated problem that I'd like to get feedback on is the
> suspend/resume support in the OMAP IOMMU driver, or rather the lack thereof.
> The driver exports omap_iommu_save_ctx() and omap_iommu_restore_ctx()
> functions and expects the IOMMU users to call them directly. This is really
> hackish to say the least. A proper suspend/resume implementation shouldn't be
> difficult to implement, and I'm wondering whether the lack of proper power
> management support comes from historical reasons only, or if there are
> problems I might not have considered.

Agreed, the current code definitely needs a cleanup and better 
organization (like folding into runtime suspend ops). The main thing is 
that we need the IOMMU to be activated as long as its parent device is 
active (like the omap3isp or a remoteproc device). And this behaviour 
needs separation from configuring it and programming for the first time. 
Only a user can dictate when an IOMMU is not needed. So we either need 
an API to activate or have access to the iommu's dev object somehow so 
that we can use the pm_runtime_get/put API to hold a usage counter and 
let the runtime suspend ops take care of save/restore without tearing 
down the domain or detaching the device.

>
> Last but not least, the patches are available at
>
> 	git://linuxtv.org/pinchartl/media.git omap3isp/dma
>
> along with a patch series that ports the OMAP3 ISP driver to the DMA API. I
> will submit that one for review once the IOMMU patches get accepted and after
> fixing a couple of remaining bugs (I'm aware that I have broken userspace
> PFNMAP buffers).

I have looked at your tree, and as you already stated, it seems to be 
right intermediate solution for now to get rid of omap-iovmm.

regards
Suman

>
> Laurent Pinchart (5):
>    iommu/omap: Use the cache cleaning API
>    iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
>    iommu/omap: Flush the TLB only after updating translation table
>      entries
>    iommu/omap: Remove comment about supporting single page mappings only
>    iommu/omap: Fix map protection value handling
>
>   drivers/iommu/omap-iommu.c | 68 ++++++++++++++++------------------------------
>   1 file changed, 23 insertions(+), 45 deletions(-)
>

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Suman Anna]

Hi Laurent,

On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> The page table entries must be cleaned from the cache before being
> accessed by the IOMMU. Instead of implementing cache management manually
> (and ignoring L2 cache), use clean_dcache_area() to make sure the
> entries are visible to the device.

Thanks for fixing this, this has been long pending. There have been some 
previous attempts at this as well by Ramesh Gupta, with the last thread 
(a long time now) being
http://marc.info/?t=134752035400001&r=1&w=2

Santosh,
Can you please take a look at this patch and provide your comments?

regards
Suman

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 41 ++++++++++-------------------------------
>   1 file changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index a893eca..bb605c9 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>   /*
>    *	H/W pagetable operations
>    */
> -static void flush_iopgd_range(u32 *first, u32 *last)
> +static void flush_pgtable(void *addr, size_t size)
>   {
> -	/* FIXME: L2 cache should be taken care of if it exists */
> -	do {
> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
> -		    : : "r" (first));
> -		first += L1_CACHE_BYTES / sizeof(*first);
> -	} while (first <= last);
> -}
> -
> -static void flush_iopte_range(u32 *first, u32 *last)
> -{
> -	/* FIXME: L2 cache should be taken care of if it exists */
> -	do {
> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
> -		    : : "r" (first));
> -		first += L1_CACHE_BYTES / sizeof(*first);
> -	} while (first <= last);
> +	clean_dcache_area(addr, size);
>   }
>
>   static void iopte_free(u32 *iopte)
> @@ -546,7 +531,7 @@ static u32 *iopte_alloc(struct omap_iommu *obj, u32 *iopgd, u32 da)
>   			return ERR_PTR(-ENOMEM);
>
>   		*iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
> -		flush_iopgd_range(iopgd, iopgd);
> +		flush_pgtable(iopgd, sizeof(*iopgd));
>
>   		dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
>   	} else {
> @@ -575,7 +560,7 @@ static int iopgd_alloc_section(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>   	}
>
>   	*iopgd = (pa & IOSECTION_MASK) | prot | IOPGD_SECTION;
> -	flush_iopgd_range(iopgd, iopgd);
> +	flush_pgtable(iopgd, sizeof(*iopgd));
>   	return 0;
>   }
>
> @@ -592,7 +577,7 @@ static int iopgd_alloc_super(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>
>   	for (i = 0; i < 16; i++)
>   		*(iopgd + i) = (pa & IOSUPER_MASK) | prot | IOPGD_SUPER;
> -	flush_iopgd_range(iopgd, iopgd + 15);
> +	flush_pgtable(iopgd, sizeof(*iopgd) * 16);
>   	return 0;
>   }
>
> @@ -605,7 +590,7 @@ static int iopte_alloc_page(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>   		return PTR_ERR(iopte);
>
>   	*iopte = (pa & IOPAGE_MASK) | prot | IOPTE_SMALL;
> -	flush_iopte_range(iopte, iopte);
> +	flush_pgtable(iopte, sizeof(*iopte));
>
>   	dev_vdbg(obj->dev, "%s: da:%08x pa:%08x pte:%p *pte:%08x\n",
>   		 __func__, da, pa, iopte, *iopte);
> @@ -619,18 +604,12 @@ static int iopte_alloc_large(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>   	u32 *iopte = iopte_alloc(obj, iopgd, da);
>   	int i;
>
> -	if ((da | pa) & ~IOLARGE_MASK) {
> -		dev_err(obj->dev, "%s: %08x:%08x should aligned on %08lx\n",
> -			__func__, da, pa, IOLARGE_SIZE);
> -		return -EINVAL;
> -	}
> -
>   	if (IS_ERR(iopte))
>   		return PTR_ERR(iopte);
>
>   	for (i = 0; i < 16; i++)
>   		*(iopte + i) = (pa & IOLARGE_MASK) | prot | IOPTE_LARGE;
> -	flush_iopte_range(iopte, iopte + 15);
> +	flush_pgtable(iopte, sizeof(*iopte) * 16);
>   	return 0;
>   }
>
> @@ -733,7 +712,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
>   		}
>   		bytes *= nent;
>   		memset(iopte, 0, nent * sizeof(*iopte));
> -		flush_iopte_range(iopte, iopte + (nent - 1) * sizeof(*iopte));
> +		flush_pgtable(iopte, sizeof(*iopte) * nent);
>
>   		/*
>   		 * do table walk to check if this table is necessary or not
> @@ -755,7 +734,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
>   		bytes *= nent;
>   	}
>   	memset(iopgd, 0, nent * sizeof(*iopgd));
> -	flush_iopgd_range(iopgd, iopgd + (nent - 1) * sizeof(*iopgd));
> +	flush_pgtable(iopgd, sizeof(*iopgd) * nent);
>   out:
>   	return bytes;
>   }
> @@ -799,7 +778,7 @@ static void iopgtable_clear_entry_all(struct omap_iommu *obj)
>   			iopte_free(iopte_offset(iopgd, 0));
>
>   		*iopgd = 0;
> -		flush_iopgd_range(iopgd, iopgd);
> +		flush_pgtable(iopgd, sizeof(*iopgd));
>   	}
>
>   	flush_iotlb_all(obj);
>

Re: [PATCH 5/5] iommu/omap: Fix map protection value handling

[Laurent Pinchart]

Hi Suman,

Thank you for the review.

On Thursday 13 March 2014 19:07:33 Suman Anna wrote:
> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > The prot flags passed to the IOMMU map handler are defined in
> > include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
> > driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
> > IOMMU flags being interpreted as page sizes, leading to failures.
> > 
> > Hardcode the OMAP mapping parameters to little-endian, 8-bits and
> > non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
> > support read-only or write-only mappings, ignore the prot value.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >   drivers/iommu/omap-iommu.c | 17 +++++++----------
> >   1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> > index 59de3fc..9f7f1d4 100644
> > --- a/drivers/iommu/omap-iommu.c
> > +++ b/drivers/iommu/omap-iommu.c
> > @@ -1016,8 +1016,7 @@ static void iopte_cachep_ctor(void *iopte)
> >   	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
> >   }
> > 
> > -static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
> > -				   u32 flags)
> > +static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int
> > pgsz)
> >   {
> >   	memset(e, 0, sizeof(*e));
> > 
> > @@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e,
> > u32 da, u32 pa,
> >   	e->pa		= pa;
> >   	e->valid	= 1;
> 
> As I was looking through this, I found a small bug here. This is unrelated
> to this patch, but you can fix it in the same patch. The e->valid value is
> directly used in omap2_alloc_cr, so it should be initialized to MMU_CAM_V.
> It is not a problem currently as PREFETCH_IOTLB is not used, if used, this
> is overriding the size bit-fields when it prefetches the entry, which may
> lead to an MMU fault.

Good catch. As it's a separate issue, do you mind if I fix that in a separate 
patch in v2 ?

> >   	/* FIXME: add OMAP1 support */
> > 
> > -	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
> > -	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
> > -	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
> > -	e->mixed	= flags & MMU_RAM_MIXED_MASK;
> > +	e->pgsz		= pgsz;
> > +	e->endian	= MMU_RAM_ENDIAN_LITTLE;
> > +	e->elsz		= MMU_RAM_ELSZ_8;
> > +	e->mixed	= 0;
> 
> Thanks, the change of these settings looks good overall. The only ones for
> which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per TRM,
> endianness conversion supported only on writes anyway, so I am assuming that
> we never use the combination), and OMAP2420 DSP, for which there is no
> driver currently nor is the MMU being instantiated. All other processors are
> pure little endian, and they even ignore the values written in these fields.
> 
> Please recheck the Camera subsystem once, and see if the e->elsz should be
> set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera usage, so
> you are probably in a better place than me).

I've tested MMU_RAM_ELSZ_NONE and it works fine. I'll update the patch in v2.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()

[Laurent Pinchart]

Hi Suman,

On Thursday 13 March 2014 17:16:07 Suman Anna wrote:
> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > The flush_iotlb_page() function prints a debug message when no
> > corresponding page was found in the TLB. That condition is incorrectly
> > checked and always resolves to true, given that the for_each_iotlb_cr()
> > loop is never interrupted and always reaches obj->nr_tlb_entries.
> 
> Nice catch.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >   drivers/iommu/omap-iommu.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> > index bb605c9..cb1e1de 100644
> > --- a/drivers/iommu/omap-iommu.c
> > +++ b/drivers/iommu/omap-iommu.c
> > @@ -376,6 +376,7 @@ static void flush_iotlb_page(struct omap_iommu *obj,
> > u32 da)
> >   {
> >   	int i;
> >   	struct cr_regs cr;
> > +	bool found = false;
> > 
> >   	pm_runtime_get_sync(obj->dev);
> > 
> > @@ -394,11 +395,12 @@ static void flush_iotlb_page(struct omap_iommu *obj,
> > u32 da)
> >   				__func__, start, da, bytes);
> >   			iotlb_load_cr(obj, &cr);
> >   			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
> > +			found = true;
> 
> The patch is fine as it is, but I think the loop can be breaked when an
> entry is found for simplification. I do not expect that we will have
> multiple entries with the same da in the TLBs (it is a multi-hit fault
> scenario) to continue looping through all entries. The only means for the
> multi-hit scenario to happen is user error with programming TLBs directly.

OK. I wasn't sure whether we could have two TLB entries for the same VA. I'll 
update the patch accordingly.

> This function call seems to be added in general to take care of any
> prefetching. I don't have the complete history on the PREFETCH_IOTLB on why
> it was added, it doesn't seem to be enabled but this should ideally be left
> to the h/w unless you are locking a TLB entry. DSP/Bridge is the only one
> that uses locked TLB entries at the moment, but it is not using the OMAP
> IOMMU driver yet.
>
> >   		}
> >   	}
> >   	pm_runtime_put_sync(obj->dev);
> > 
> > -	if (i == obj->nr_tlb_entries)
> > +	if (!found)
> >   		dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);
> >   }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] iommu/omap: Flush the TLB only after updating translation table entries

[Laurent Pinchart]

Hi Suman,

On Thursday 13 March 2014 17:27:38 Suman Anna wrote:
> Hi Laurent,
> 
> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > Flushing the TLB before updating translation entries creates a race
> > condition and can lead to stale TLB entries if a translation request
> > arrives between flushing the TLB and updating the translation entries.
> > As there's no requirement to flush the TLB before updating the entries,
> > flush it after only.
> 
> I do not expect a device to be actively using a region if we are about
> to change its mapping, we expect the access from the device to be only
> between a map and an unmap. The race condition (if any in such
> scenarios) would exist even after this patch, like after you programmed
> the entry, and the translation request comes before you flush the page.
> Then it is still operating on an older address, while you have already
> programmed a new one. An unmap currently clears the entry and flushes
> any TLB as well, so I don't think this patch makes a big difference.

I agree that the patch won't make a difference in practice. Should I drop it 
from v2 ? 

> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> > 
> >   drivers/iommu/omap-iommu.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> > index cb1e1de..fedd303 100644
> > --- a/drivers/iommu/omap-iommu.c
> > +++ b/drivers/iommu/omap-iommu.c
> > @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu
> > *obj, struct iotlb_entry *e)> 
> >   {
> >   	int err;
> > 
> > -	flush_iotlb_page(obj, e->da);
> > 
> >   	err = iopgtable_store_entry_core(obj, e);
> > -	if (!err)
> > +	if (!err) {
> > +		flush_iotlb_page(obj, e->da);
> >   		prefetch_iotlb_entry(obj, e);
> > +	}
> > 
> >   	return err;
> >   }
> >   EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Suman,

(CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU discussion)

On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
> > found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
> > DMA API. The biggest issue is fixed by patch 5/5, while the other patches
> > fix smaller problems that I've noticed when reading the code, without
> > experiencing them at runtime.
> > 
> > I'd like to take this as an opportunity to discuss OMAP IOMMU integration
> > with the ARM DMA mapping implementation. The idea is to hide the IOMMU
> > completely behind the DMA mapping API, making it completely transparent
> > to drivers.
>
> Thanks for starting the discussion.
> 
> > A drivers will only need to allocate memory with dma_alloc_*, and behind
> > the scene the ARM DMA mapping implementation will find out that the
> > device is behind an IOMMU and will map the buffers through the IOMMU,
> > returning an I/O VA address to the driver. No direct call to the OMAP
> > IOMMU driver or to the IOMMU core should be necessary anymore.
> > 
> > To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> > created with a call to arm_iommu_create_mapping() and to then be attached
> > to the device with arm_iommu_attach_device(). This is currently not
> > performed by the OMAP IOMMU driver, I have thus added that code to the
> > OMAP3 ISP driver for now. I believe this to still be an improvement
> > compared to the current situation, as it allows getting rid of custom
> > memory allocation code in the OMAP3 ISP driver and custom I/O VA space
> > management in omap-iovmm. However, that code should ideally be removed
> > from the driver. The question is, where should it be moved to ?
> > 
> > One possible solution would be to add the code to the OMAP IOMMU driver.
> > However, this would require all OMAP IOMMU users to be converted to the
> > ARM DMA API. I assume there would be issues that I don't foresee though.
> 
> Yeah, I think this will pose some problems for the other main user of IOMMUs
> - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in OMAP4 and
> beyond). A remoteproc device also needs to map memory at specific addresses
> for its code and data sections, and not rely on a I/O VA address being given
> to it. The firmware sections are already linked at specific addresses, and
> so remoteproc needs to allocate memory, load the firmware and map into
> appropriate device addresses. We are doing this currently usage a
> combination of CMA memory to get contiguous memory (there are some
> restrictions for certain sections) and iommu_map/unmap API to program the
> MMU with these pages. This usage is different from what is expected from
> exchanging buffers, which can be allocated from a predefined mapping range.
> Even that one is tricky if we need to support different cache
> properties/attributes as the cache configuration is in general local to
> these processors.

Right, we indeed need two levels of API, one for drivers such as remoteproc 
that need direct control of the IOMMU, and one for drivers that only need to 
map buffers without any additional requirement. In the second case the drivers 
should ideally use the DMA mapping API not even be aware that an IOMMU is 
present. This would require moving the ARM mapping allocation out of the 
client driver.

The IOMMU core or the IOMMU driver will need to know whether the driver 
expects to control the IOMMU directly or to have it managed transparently. As 
this is a software configuration I don't think the information belongs to DT. 
The question is, how should this information be conveyed ?

> > I'm not even sure whether the OMAP IOMMU driver would be the best place to
> > put that code. Ideally VA spaces should be created by the platform
> > somehow, and mapping of devices to IOMMUs should be handled by the IOMMU
> > core instead of the IOMMU drivers. We're not there yet, and the path
> > might not be straightforward, hence this attempt to start a constructive
> > discussion.
> > 
> > A second completely unrelated problem that I'd like to get feedback on is
> > the suspend/resume support in the OMAP IOMMU driver, or rather the lack
> > thereof. The driver exports omap_iommu_save_ctx() and
> > omap_iommu_restore_ctx() functions and expects the IOMMU users to call
> > them directly. This is really hackish to say the least. A proper
> > suspend/resume implementation shouldn't be difficult to implement, and
> > I'm wondering whether the lack of proper power management support comes
> > from historical reasons only, or if there are problems I might not have
> > considered.
> 
> Agreed, the current code definitely needs a cleanup and better organization
> (like folding into runtime suspend ops). The main thing is that we need the
> IOMMU to be activated as long as its parent device is active (like the
> omap3isp or a remoteproc device). And this behaviour needs separation from
> configuring it and programming for the first time. Only a user can dictate
> when an IOMMU is not needed. So we either need an API to activate or have
> access to the iommu's dev object somehow so that we can use the
> pm_runtime_get/put API to hold a usage counter and let the runtime suspend
> ops take care of save/restore without tearing down the domain or detaching
> the device.

The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), and 
pm_runtime_put_sync() in iommu_disable(). The enable/disable functions are 
called in the attach and detach handlers (as well as in the fault handler for 
the disable function). As long as a device is attached the IOMMU will thus not 
be suspended by runtime PM, but could be suspended by system PM.

This leads to two separate issues. The first one is that the IOMMU will be 
powered on as long as a device is attached, even if the device isn't in use. 
This consumes power needlessly. It would probably be better to more the 
enable/disable calls to the map/unmap handlers, as the IOMMU should be powered 
off when no mapping exists (or am I missing something ?).

The second issue is that the IOMMU and IOMMU users system PM suspend/resume 
callbacks are not synchronized. We need a guarantee that the IOMMU will be 
suspended after its users, and resumed before them. One option to fix this 
could be to use the suspend_late and resume_early PM operations (although I 
haven't checked whether that would work), but it's probably a bit hackish.

> > Last but not least, the patches are available at
> > 
> > 	git://linuxtv.org/pinchartl/media.git omap3isp/dma
> > 
> > along with a patch series that ports the OMAP3 ISP driver to the DMA API.
> > I will submit that one for review once the IOMMU patches get accepted and
> > after fixing a couple of remaining bugs (I'm aware that I have broken
> > userspace PFNMAP buffers).
> 
> I have looked at your tree, and as you already stated, it seems to be
> right intermediate solution for now to get rid of omap-iovmm.

OK, thank you. I'll push those patches upstream as soon as this series gets 
merged.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Santosh Shilimkar]

+ Russell, Arnd

On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
> Hi Laurent,
> 
> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>> The page table entries must be cleaned from the cache before being
>> accessed by the IOMMU. Instead of implementing cache management manually
>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>> entries are visible to the device.
> 
> Thanks for fixing this, this has been long pending. There have been some 
> previous attempts at this as well by Ramesh Gupta, with the last thread 
> (a long time now) being
> http://marc.info/?t=134752035400001&r=1&w=2
> 
> Santosh,
> Can you please take a look at this patch and provide your comments?
> 
> regards
> Suman
> 
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/iommu/omap-iommu.c | 41 ++++++++++-------------------------------
>>   1 file changed, 10 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>> index a893eca..bb605c9 100644
>> --- a/drivers/iommu/omap-iommu.c
>> +++ b/drivers/iommu/omap-iommu.c
>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>   /*
>>    *	H/W pagetable operations
>>    */
>> -static void flush_iopgd_range(u32 *first, u32 *last)
>> +static void flush_pgtable(void *addr, size_t size)
>>   {
>> -	/* FIXME: L2 cache should be taken care of if it exists */
>> -	do {
>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
>> -		    : : "r" (first));
>> -		first += L1_CACHE_BYTES / sizeof(*first);
>> -	} while (first <= last);
>> -}
>> -
>> -static void flush_iopte_range(u32 *first, u32 *last)
>> -{
>> -	/* FIXME: L2 cache should be taken care of if it exists */
>> -	do {
>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
>> -		    : : "r" (first));
>> -		first += L1_CACHE_BYTES / sizeof(*first);
>> -	} while (first <= last);
>> +	clean_dcache_area(addr, size);
I remember NAKing this approach in past and my stand remains same.
The cache APIs which you are trying to use here are not suppose
to be used outside.

I think the right way to fix this is to make use of streaming APIs.
If needed, IOMMU can have its own dma_ops for special case
handling if any.

Russell, Arnd might have more ideas.

>>   }
>>
>>   static void iopte_free(u32 *iopte)
>> @@ -546,7 +531,7 @@ static u32 *iopte_alloc(struct omap_iommu *obj, u32 *iopgd, u32 da)
>>   			return ERR_PTR(-ENOMEM);
>>
>>   		*iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
>> -		flush_iopgd_range(iopgd, iopgd);
>> +		flush_pgtable(iopgd, sizeof(*iopgd));
>>
>>   		dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
>>   	} else {
>> @@ -575,7 +560,7 @@ static int iopgd_alloc_section(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>>   	}
>>
>>   	*iopgd = (pa & IOSECTION_MASK) | prot | IOPGD_SECTION;
>> -	flush_iopgd_range(iopgd, iopgd);
>> +	flush_pgtable(iopgd, sizeof(*iopgd));
>>   	return 0;
>>   }
>>
>> @@ -592,7 +577,7 @@ static int iopgd_alloc_super(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>>
>>   	for (i = 0; i < 16; i++)
>>   		*(iopgd + i) = (pa & IOSUPER_MASK) | prot | IOPGD_SUPER;
>> -	flush_iopgd_range(iopgd, iopgd + 15);
>> +	flush_pgtable(iopgd, sizeof(*iopgd) * 16);
>>   	return 0;
>>   }
>>
>> @@ -605,7 +590,7 @@ static int iopte_alloc_page(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>>   		return PTR_ERR(iopte);
>>
>>   	*iopte = (pa & IOPAGE_MASK) | prot | IOPTE_SMALL;
>> -	flush_iopte_range(iopte, iopte);
>> +	flush_pgtable(iopte, sizeof(*iopte));
>>
>>   	dev_vdbg(obj->dev, "%s: da:%08x pa:%08x pte:%p *pte:%08x\n",
>>   		 __func__, da, pa, iopte, *iopte);
>> @@ -619,18 +604,12 @@ static int iopte_alloc_large(struct omap_iommu *obj, u32 da, u32 pa, u32 prot)
>>   	u32 *iopte = iopte_alloc(obj, iopgd, da);
>>   	int i;
>>
>> -	if ((da | pa) & ~IOLARGE_MASK) {
>> -		dev_err(obj->dev, "%s: %08x:%08x should aligned on %08lx\n",
>> -			__func__, da, pa, IOLARGE_SIZE);
>> -		return -EINVAL;
>> -	}
>> -
>>   	if (IS_ERR(iopte))
>>   		return PTR_ERR(iopte);
>>
>>   	for (i = 0; i < 16; i++)
>>   		*(iopte + i) = (pa & IOLARGE_MASK) | prot | IOPTE_LARGE;
>> -	flush_iopte_range(iopte, iopte + 15);
>> +	flush_pgtable(iopte, sizeof(*iopte) * 16);
>>   	return 0;
>>   }
>>
>> @@ -733,7 +712,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
>>   		}
>>   		bytes *= nent;
>>   		memset(iopte, 0, nent * sizeof(*iopte));
>> -		flush_iopte_range(iopte, iopte + (nent - 1) * sizeof(*iopte));
>> +		flush_pgtable(iopte, sizeof(*iopte) * nent);
>>
>>   		/*
>>   		 * do table walk to check if this table is necessary or not
>> @@ -755,7 +734,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu *obj, u32 da)
>>   		bytes *= nent;
>>   	}
>>   	memset(iopgd, 0, nent * sizeof(*iopgd));
>> -	flush_iopgd_range(iopgd, iopgd + (nent - 1) * sizeof(*iopgd));
>> +	flush_pgtable(iopgd, sizeof(*iopgd) * nent);
>>   out:
>>   	return bytes;
>>   }
>> @@ -799,7 +778,7 @@ static void iopgtable_clear_entry_all(struct omap_iommu *obj)
>>   			iopte_free(iopte_offset(iopgd, 0));
>>
>>   		*iopgd = 0;
>> -		flush_iopgd_range(iopgd, iopgd);
>> +		flush_pgtable(iopgd, sizeof(*iopgd));
>>   	}
>>
>>   	flush_iotlb_all(obj);
>>
> 

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Laurent Pinchart]

Hi Santosh,

On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
> + Russell, Arnd
> 
> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
> > On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> >> The page table entries must be cleaned from the cache before being
> >> accessed by the IOMMU. Instead of implementing cache management manually
> >> (and ignoring L2 cache), use clean_dcache_area() to make sure the
> >> entries are visible to the device.
> > 
> > Thanks for fixing this, this has been long pending. There have been some
> > previous attempts at this as well by Ramesh Gupta, with the last thread
> > (a long time now) being
> > http://marc.info/?t=134752035400001&r=1&w=2
> > 
> > Santosh,
> > Can you please take a look at this patch and provide your comments?
> > 
> > regards
> > Suman
> > 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> 
> >>   drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
> >>   1 file changed, 10 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> >> index a893eca..bb605c9 100644
> >> --- a/drivers/iommu/omap-iommu.c
> >> +++ b/drivers/iommu/omap-iommu.c
> >> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
> >>   /*
> >>    *	H/W pagetable operations
> >>    */
> >> -static void flush_iopgd_range(u32 *first, u32 *last)
> >> +static void flush_pgtable(void *addr, size_t size)
> >>   {
> >> -	/* FIXME: L2 cache should be taken care of if it exists */
> >> -	do {
> >> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
> >> -		    : : "r" (first));
> >> -		first += L1_CACHE_BYTES / sizeof(*first);
> >> -	} while (first <= last);
> >> -}
> >> -
> >> -static void flush_iopte_range(u32 *first, u32 *last)
> >> -{
> >> -	/* FIXME: L2 cache should be taken care of if it exists */
> >> -	do {
> >> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
> >> -		    : : "r" (first));
> >> -		first += L1_CACHE_BYTES / sizeof(*first);
> >> -	} while (first <= last);
> >> +	clean_dcache_area(addr, size);
> 
> I remember NAKing this approach in past and my stand remains same.
> The cache APIs which you are trying to use here are not suppose
> to be used outside.

Please note that the omap-iommu driver already uses clean_dcache_area(). 
That's where I got the idea :-)

> I think the right way to fix this is to make use of streaming APIs.
> If needed, IOMMU can have its own dma_ops for special case
> handling if any.

I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu 
driver. If that's fine I'll modify this patch accordingly in v2.

> Russell, Arnd might have more ideas.
> 
> >>   }
> >>   
> >>   static void iopte_free(u32 *iopte)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Arnd Bergmann]

On Friday 14 March 2014, Santosh Shilimkar wrote:
> I remember NAKing this approach in past and my stand remains same.
> The cache APIs which you are trying to use here are not suppose
> to be used outside.
> 
> I think the right way to fix this is to make use of streaming APIs.
> If needed, IOMMU can have its own dma_ops for special case
> handling if any.
> 
> Russell, Arnd might have more ideas.

I have a bad feeling about using the dma-mapping API within the
IOMMU code, because that driver is also used to implement the
dma-mapping API for devices attached to the IOMMU. It's possible
that it just works.

Is the IOMMU actually designed to have page tables in noncoherent
memory? I would have expected that the iopt accesses must all be
done on dma_alloc_coherent() provided memory to guarantee proper
accesses.

	Arnd

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Santosh Shilimkar]

On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
> Hi Santosh,
> 
> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
>> + Russell, Arnd
>>
>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>> The page table entries must be cleaned from the cache before being
>>>> accessed by the IOMMU. Instead of implementing cache management manually
>>>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>>>> entries are visible to the device.
>>>
>>> Thanks for fixing this, this has been long pending. There have been some
>>> previous attempts at this as well by Ramesh Gupta, with the last thread
>>> (a long time now) being
>>> http://marc.info/?t=134752035400001&r=1&w=2
>>>
>>> Santosh,
>>> Can you please take a look at this patch and provide your comments?
>>>
>>> regards
>>> Suman
>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> ---
>>>>
>>>>   drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
>>>>   1 file changed, 10 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>>> index a893eca..bb605c9 100644
>>>> --- a/drivers/iommu/omap-iommu.c
>>>> +++ b/drivers/iommu/omap-iommu.c
>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>>>   /*
>>>>    *	H/W pagetable operations
>>>>    */
>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
>>>> +static void flush_pgtable(void *addr, size_t size)
>>>>   {
>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
>>>> -	do {
>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
>>>> -		    : : "r" (first));
>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
>>>> -	} while (first <= last);
>>>> -}
>>>> -
>>>> -static void flush_iopte_range(u32 *first, u32 *last)
>>>> -{
>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
>>>> -	do {
>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
>>>> -		    : : "r" (first));
>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
>>>> -	} while (first <= last);
>>>> +	clean_dcache_area(addr, size);
>>
>> I remember NAKing this approach in past and my stand remains same.
>> The cache APIs which you are trying to use here are not suppose
>> to be used outside.
> 
> Please note that the omap-iommu driver already uses clean_dcache_area(). 
> That's where I got the idea :-)
> 
So that fall through the cracks then ;-)

>> I think the right way to fix this is to make use of streaming APIs.
>> If needed, IOMMU can have its own dma_ops for special case
>> handling if any.
> 
> I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu 
> driver. If that's fine I'll modify this patch accordingly in v2.
> 
That will be definitely a better option than the APIs used. Those streaming
APIs will take care of L2 cache as well if present.

Regards,
Santosh

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Santosh Shilimkar]

On Friday 14 March 2014 12:57 PM, Arnd Bergmann wrote:
> On Friday 14 March 2014, Santosh Shilimkar wrote:
>> I remember NAKing this approach in past and my stand remains same.
>> The cache APIs which you are trying to use here are not suppose
>> to be used outside.
>>
>> I think the right way to fix this is to make use of streaming APIs.
>> If needed, IOMMU can have its own dma_ops for special case
>> handling if any.
>>
>> Russell, Arnd might have more ideas.
> 
> I have a bad feeling about using the dma-mapping API within the
> IOMMU code, because that driver is also used to implement the
> dma-mapping API for devices attached to the IOMMU. It's possible
> that it just works.
> 
Thats a good point.

> Is the IOMMU actually designed to have page tables in noncoherent
> memory? I would have expected that the iopt accesses must all be
> done on dma_alloc_coherent() provided memory to guarantee proper
> accesses.
> 
At least the OMAP one seems to use non-coherent memory which will
need CMO.

Regards,
Santosh

Re: [PATCH 5/5] iommu/omap: Fix map protection value handling

[Suman Anna]

Hi Laurent,

On 03/14/2014 04:46 AM, Laurent Pinchart wrote:
> Hi Suman,
>
> Thank you for the review.
>
> On Thursday 13 March 2014 19:07:33 Suman Anna wrote:
>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>> The prot flags passed to the IOMMU map handler are defined in
>>> include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
>>> driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
>>> IOMMU flags being interpreted as page sizes, leading to failures.
>>>
>>> Hardcode the OMAP mapping parameters to little-endian, 8-bits and
>>> non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
>>> support read-only or write-only mappings, ignore the prot value.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>> ---
>>>
>>>    drivers/iommu/omap-iommu.c | 17 +++++++----------
>>>    1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>> index 59de3fc..9f7f1d4 100644
>>> --- a/drivers/iommu/omap-iommu.c
>>> +++ b/drivers/iommu/omap-iommu.c
>>> @@ -1016,8 +1016,7 @@ static void iopte_cachep_ctor(void *iopte)
>>>    	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
>>>    }
>>>
>>> -static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
>>> -				   u32 flags)
>>> +static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int
>>> pgsz)
>>>    {
>>>    	memset(e, 0, sizeof(*e));
>>>
>>> @@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e,
>>> u32 da, u32 pa,
>>>    	e->pa		= pa;
>>>    	e->valid	= 1;
>>
>> As I was looking through this, I found a small bug here. This is unrelated
>> to this patch, but you can fix it in the same patch. The e->valid value is
>> directly used in omap2_alloc_cr, so it should be initialized to MMU_CAM_V.
>> It is not a problem currently as PREFETCH_IOTLB is not used, if used, this
>> is overriding the size bit-fields when it prefetches the entry, which may
>> lead to an MMU fault.
>
> Good catch. As it's a separate issue, do you mind if I fix that in a separate
> patch in v2 ?

I actually have couple of minor cleanup patches as well. I will send 
them out and you can include them with your series when you post v2.

>
>>>    	/* FIXME: add OMAP1 support */
>>>
>>> -	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
>>> -	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
>>> -	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
>>> -	e->mixed	= flags & MMU_RAM_MIXED_MASK;
>>> +	e->pgsz		= pgsz;
>>> +	e->endian	= MMU_RAM_ENDIAN_LITTLE;
>>> +	e->elsz		= MMU_RAM_ELSZ_8;
>>> +	e->mixed	= 0;
>>
>> Thanks, the change of these settings looks good overall. The only ones for
>> which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per TRM,
>> endianness conversion supported only on writes anyway, so I am assuming that
>> we never use the combination), and OMAP2420 DSP, for which there is no
>> driver currently nor is the MMU being instantiated. All other processors are
>> pure little endian, and they even ignore the values written in these fields.
>>
>> Please recheck the Camera subsystem once, and see if the e->elsz should be
>> set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera usage, so
>> you are probably in a better place than me).
>
> I've tested MMU_RAM_ELSZ_NONE and it works fine. I'll update the patch in v2.
>
Thanks for checking and confirming this.

regards
Suman

Re: [PATCH 3/5] iommu/omap: Flush the TLB only after updating translation table entries

[Suman Anna]

Hi Laurent,

 >>
>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>> Flushing the TLB before updating translation entries creates a race
>>> condition and can lead to stale TLB entries if a translation request
>>> arrives between flushing the TLB and updating the translation entries.
>>> As there's no requirement to flush the TLB before updating the entries,
>>> flush it after only.
>>
>> I do not expect a device to be actively using a region if we are about
>> to change its mapping, we expect the access from the device to be only
>> between a map and an unmap. The race condition (if any in such
>> scenarios) would exist even after this patch, like after you programmed
>> the entry, and the translation request comes before you flush the page.
>> Then it is still operating on an older address, while you have already
>> programmed a new one. An unmap currently clears the entry and flushes
>> any TLB as well, so I don't think this patch makes a big difference.
>
> I agree that the patch won't make a difference in practice. Should I drop it
> from v2 ?

Yes, that should be safe.

regards
Suman

>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>> ---
>>>
>>>    drivers/iommu/omap-iommu.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>> index cb1e1de..fedd303 100644
>>> --- a/drivers/iommu/omap-iommu.c
>>> +++ b/drivers/iommu/omap-iommu.c
>>> @@ -662,10 +662,11 @@ int omap_iopgtable_store_entry(struct omap_iommu
>>> *obj, struct iotlb_entry *e)>
>>>    {
>>>    	int err;
>>>
>>> -	flush_iotlb_page(obj, e->da);
>>>
>>>    	err = iopgtable_store_entry_core(obj, e);
>>> -	if (!err)
>>> +	if (!err) {
>>> +		flush_iotlb_page(obj, e->da);
>>>    		prefetch_iotlb_entry(obj, e);
>>> +	}
>>>
>>>    	return err;
>>>    }
>>>    EXPORT_SYMBOL_GPL(omap_iopgtable_store_entry);
>

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Suman Anna]

Hi Santosh, Laurent, Russell, Arnd,

On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
> On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
>> Hi Santosh,
>>
>> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
>>> + Russell, Arnd
>>>
>>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
>>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>>> The page table entries must be cleaned from the cache before being
>>>>> accessed by the IOMMU. Instead of implementing cache management manually
>>>>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>>>>> entries are visible to the device.
>>>>
>>>> Thanks for fixing this, this has been long pending. There have been some
>>>> previous attempts at this as well by Ramesh Gupta, with the last thread
>>>> (a long time now) being
>>>> http://marc.info/?t=134752035400001&r=1&w=2
>>>>
>>>> Santosh,
>>>> Can you please take a look at this patch and provide your comments?
>>>>
>>>> regards
>>>> Suman
>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>>> ---
>>>>>
>>>>>    drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
>>>>>    1 file changed, 10 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>>>> index a893eca..bb605c9 100644
>>>>> --- a/drivers/iommu/omap-iommu.c
>>>>> +++ b/drivers/iommu/omap-iommu.c
>>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>>>>    /*
>>>>>     *	H/W pagetable operations
>>>>>     */
>>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
>>>>> +static void flush_pgtable(void *addr, size_t size)
>>>>>    {
>>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
>>>>> -	do {
>>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
>>>>> -		    : : "r" (first));
>>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
>>>>> -	} while (first <= last);
>>>>> -}
>>>>> -
>>>>> -static void flush_iopte_range(u32 *first, u32 *last)
>>>>> -{
>>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
>>>>> -	do {
>>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
>>>>> -		    : : "r" (first));
>>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
>>>>> -	} while (first <= last);
>>>>> +	clean_dcache_area(addr, size);
>>>
>>> I remember NAKing this approach in past and my stand remains same.
>>> The cache APIs which you are trying to use here are not suppose
>>> to be used outside.

So this particular change has a long history (have to dig through to 
educate myself). I have not seen clean_dcache_area attempted before, and 
I wasn't sure myself it it can be used here. Ramesh and Fernando 
definitely did start out with dmac_flush_range and outer_flush_range 
which was definitely nacked [1].

>>
>> Please note that the omap-iommu driver already uses clean_dcache_area().
>> That's where I got the idea :-)
>>
> So that fall through the cracks then ;-)
>
>>> I think the right way to fix this is to make use of streaming APIs.
>>> If needed, IOMMU can have its own dma_ops for special case
>>> handling if any.
>>
>> I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu
>> driver. If that's fine I'll modify this patch accordingly in v2.
>>

Ramesh had also attempted to use dma_page_single() previously [2], and 
Russell has instead suggested to extend the cpu cache operations [3].
Ramesh had worked based on this suggestion and the series reached v6 [4] 
(same as link I mentioned above) and this is the last attempt on this, 
but the thread went silent.

I am wondering if that is still valid and is the right way to go, as 
this seems to be a common problem. I do see dmac_flush_range being used 
for similar purposes in msm_iommu.c and exynos-iommu.c, so looks like 
they also fell through the cracks.

Laurent,
Can you drop this patch out from v2 so that it is not clubbed with the 
small cleanup patches, and we can track this separately?

regards
Suman

[1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
[2] http://marc.info/?t=130281804900005&r=1&w=2
[3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
[4] http://marc.info/?t=134752035400001&r=1&w=2

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Santosh Shilimkar]

On Friday 14 March 2014 09:49 PM, Suman Anna wrote:
> Hi Santosh, Laurent, Russell, Arnd,
> 
> On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
>> On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
>>> Hi Santosh,
>>>
>>> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
>>>> + Russell, Arnd
>>>>
>>>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
>>>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>>>> The page table entries must be cleaned from the cache before being
>>>>>> accessed by the IOMMU. Instead of implementing cache management manually
>>>>>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>>>>>> entries are visible to the device.
>>>>>
>>>>> Thanks for fixing this, this has been long pending. There have been some
>>>>> previous attempts at this as well by Ramesh Gupta, with the last thread
>>>>> (a long time now) being
>>>>> http://marc.info/?t=134752035400001&r=1&w=2
>>>>>
>>>>> Santosh,
>>>>> Can you please take a look at this patch and provide your comments?
>>>>>
>>>>> regards
>>>>> Suman
>>>>>
>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>> ---
>>>>>>
>>>>>>    drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
>>>>>>    1 file changed, 10 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>>>>> index a893eca..bb605c9 100644
>>>>>> --- a/drivers/iommu/omap-iommu.c
>>>>>> +++ b/drivers/iommu/omap-iommu.c
>>>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>>>>>    /*
>>>>>>     *    H/W pagetable operations
>>>>>>     */
>>>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
>>>>>> +static void flush_pgtable(void *addr, size_t size)
>>>>>>    {
>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>> -    do {
>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pgd"
>>>>>> -            : : "r" (first));
>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>> -    } while (first <= last);
>>>>>> -}
>>>>>> -
>>>>>> -static void flush_iopte_range(u32 *first, u32 *last)
>>>>>> -{
>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>> -    do {
>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pte"
>>>>>> -            : : "r" (first));
>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>> -    } while (first <= last);
>>>>>> +    clean_dcache_area(addr, size);
>>>>
>>>> I remember NAKing this approach in past and my stand remains same.
>>>> The cache APIs which you are trying to use here are not suppose
>>>> to be used outside.
> 
> So this particular change has a long history (have to dig through to educate myself). I have not seen clean_dcache_area attempted before, and I wasn't sure myself it it can be used here. Ramesh and Fernando definitely did start out with dmac_flush_range and outer_flush_range which was definitely nacked [1].
> 
OK. Please wrap the lines appropriately while replying.

>>>
>>> Please note that the omap-iommu driver already uses clean_dcache_area().
>>> That's where I got the idea :-)
>>>
>> So that fall through the cracks then ;-)
>>
>>>> I think the right way to fix this is to make use of streaming APIs.
>>>> If needed, IOMMU can have its own dma_ops for special case
>>>> handling if any.
>>>
>>> I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu
>>> driver. If that's fine I'll modify this patch accordingly in v2.
>>>
> 
> Ramesh had also attempted to use dma_page_single() previously [2], and Russell has instead suggested to extend the cpu cache operations [3].
> Ramesh had worked based on this suggestion and the series reached v6 [4] (same as link I mentioned above) and this is the last attempt on this, but the thread went silent.
> 
> I am wondering if that is still valid and is the right way to go, as this seems to be a common problem. I do see dmac_flush_range being used for similar purposes in msm_iommu.c and exynos-iommu.c, so looks like they also fell through the cracks.
> 
Thanks for the links. I think you should revive the v6 series unless and until
RMK has some other suggestion. This can also help to remove the wrong usages
from other IOMMU drivers as you pointed out.

> Laurent,
> Can you drop this patch out from v2 so that it is not clubbed with the small cleanup patches, and we can track this separately?
> 
Agree

Regards,
Santosh

> 
> [1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
> [2] http://marc.info/?t=130281804900005&r=1&w=2
> [3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
> [4] http://marc.info/?t=134752035400001&r=1&w=2
> 
> 

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Sakari Ailus]

Hi Laurent and Suman,

On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> Hi Suman,
> 
> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU discussion)
> 
> On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
> > On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
> > > found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
> > > DMA API. The biggest issue is fixed by patch 5/5, while the other patches
> > > fix smaller problems that I've noticed when reading the code, without
> > > experiencing them at runtime.
> > > 
> > > I'd like to take this as an opportunity to discuss OMAP IOMMU integration
> > > with the ARM DMA mapping implementation. The idea is to hide the IOMMU
> > > completely behind the DMA mapping API, making it completely transparent
> > > to drivers.
> >
> > Thanks for starting the discussion.
> > 
> > > A drivers will only need to allocate memory with dma_alloc_*, and behind
> > > the scene the ARM DMA mapping implementation will find out that the
> > > device is behind an IOMMU and will map the buffers through the IOMMU,
> > > returning an I/O VA address to the driver. No direct call to the OMAP
> > > IOMMU driver or to the IOMMU core should be necessary anymore.
> > > 
> > > To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> > > created with a call to arm_iommu_create_mapping() and to then be attached
> > > to the device with arm_iommu_attach_device(). This is currently not
> > > performed by the OMAP IOMMU driver, I have thus added that code to the
> > > OMAP3 ISP driver for now. I believe this to still be an improvement
> > > compared to the current situation, as it allows getting rid of custom
> > > memory allocation code in the OMAP3 ISP driver and custom I/O VA space
> > > management in omap-iovmm. However, that code should ideally be removed
> > > from the driver. The question is, where should it be moved to ?
> > > 
> > > One possible solution would be to add the code to the OMAP IOMMU driver.
> > > However, this would require all OMAP IOMMU users to be converted to the
> > > ARM DMA API. I assume there would be issues that I don't foresee though.
> > 
> > Yeah, I think this will pose some problems for the other main user of IOMMUs
> > - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in OMAP4 and
> > beyond). A remoteproc device also needs to map memory at specific addresses
> > for its code and data sections, and not rely on a I/O VA address being given
> > to it. The firmware sections are already linked at specific addresses, and
> > so remoteproc needs to allocate memory, load the firmware and map into
> > appropriate device addresses. We are doing this currently usage a
> > combination of CMA memory to get contiguous memory (there are some
> > restrictions for certain sections) and iommu_map/unmap API to program the
> > MMU with these pages. This usage is different from what is expected from
> > exchanging buffers, which can be allocated from a predefined mapping range.
> > Even that one is tricky if we need to support different cache
> > properties/attributes as the cache configuration is in general local to
> > these processors.
> 
> Right, we indeed need two levels of API, one for drivers such as remoteproc 
> that need direct control of the IOMMU, and one for drivers that only need to 
> map buffers without any additional requirement. In the second case the drivers 
> should ideally use the DMA mapping API not even be aware that an IOMMU is 
> present. This would require moving the ARM mapping allocation out of the 
> client driver.
> 
> The IOMMU core or the IOMMU driver will need to know whether the driver 
> expects to control the IOMMU directly or to have it managed transparently. As 
> this is a software configuration I don't think the information belongs to DT. 
> The question is, how should this information be conveyed ?

The driver would need to know that, I think.

Currently the DMA mapping API doesn't allow explicit addressing to IOVA
address space AFAIK. The IOMMU API does. It's a good question how to do this
as I don't think there's even a way for the driver to explicitly obtain
access to the IOMMU.

The virtual address space allocation would need to take into account that
some of the address space is actually mapped outside it. The iova library
can do this already.

> > > I'm not even sure whether the OMAP IOMMU driver would be the best place to
> > > put that code. Ideally VA spaces should be created by the platform
> > > somehow, and mapping of devices to IOMMUs should be handled by the IOMMU
> > > core instead of the IOMMU drivers. We're not there yet, and the path
> > > might not be straightforward, hence this attempt to start a constructive
> > > discussion.
> > > 
> > > A second completely unrelated problem that I'd like to get feedback on is
> > > the suspend/resume support in the OMAP IOMMU driver, or rather the lack
> > > thereof. The driver exports omap_iommu_save_ctx() and
> > > omap_iommu_restore_ctx() functions and expects the IOMMU users to call
> > > them directly. This is really hackish to say the least. A proper
> > > suspend/resume implementation shouldn't be difficult to implement, and
> > > I'm wondering whether the lack of proper power management support comes
> > > from historical reasons only, or if there are problems I might not have
> > > considered.
> > 
> > Agreed, the current code definitely needs a cleanup and better organization
> > (like folding into runtime suspend ops). The main thing is that we need the
> > IOMMU to be activated as long as its parent device is active (like the
> > omap3isp or a remoteproc device). And this behaviour needs separation from
> > configuring it and programming for the first time. Only a user can dictate
> > when an IOMMU is not needed. So we either need an API to activate or have
> > access to the iommu's dev object somehow so that we can use the
> > pm_runtime_get/put API to hold a usage counter and let the runtime suspend
> > ops take care of save/restore without tearing down the domain or detaching
> > the device.
> 
> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), and 
> pm_runtime_put_sync() in iommu_disable(). The enable/disable functions are 
> called in the attach and detach handlers (as well as in the fault handler for 
> the disable function). As long as a device is attached the IOMMU will thus not 
> be suspended by runtime PM, but could be suspended by system PM.
> 
> This leads to two separate issues. The first one is that the IOMMU will be 
> powered on as long as a device is attached, even if the device isn't in use. 
> This consumes power needlessly. It would probably be better to more the 
> enable/disable calls to the map/unmap handlers, as the IOMMU should be powered 
> off when no mapping exists (or am I missing something ?).

In general, I wouldn't make the assumption that no mappings can exist if the
IOMMU is powered off. This would mean that just keeping buffers around for
later use would keep hardware powered. Memory allocation can be a very
time-consuming task which often is done when the entire system is booted up
(depending on the user space naturally), not when a particular device node
is opened.

That said, I don't have a good, obviously acceptable solution to propose to
this problem right now. I think the IOMMU power state should be rather bound
to the device's power state, but DMA mapping API doesn't know anything about
IOMMUs as such. I wonder what kind of a reception would such an addition to
the DMA mapping would receive. There are two things I could think of: either
make the potential IOMMU visible to the driver so the driver can use runtime
PM as usual, or add a function to the DMA mapping API to allow whatever
hardware used in the DMA accesses to power down (and up again when needed).

The latter is hackish in my opinion but the former would also change the DMA
mapping API to a direction which not everyone would probably agree on.

I'm personally fine with the above limitation on OMAP3 but in general it'd
be better to get around it.

> The second issue is that the IOMMU and IOMMU users system PM suspend/resume 
> callbacks are not synchronized. We need a guarantee that the IOMMU will be 
> suspended after its users, and resumed before them. One option to fix this 
> could be to use the suspend_late and resume_early PM operations (although I 
> haven't checked whether that would work), but it's probably a bit hackish.

I agree.

The IOMMUs would indeed need to be resumed first. Hiroshi had a aptchset to
initialise and associate IOMMUs to devices using DT but it seems it's not
exactly going somewhere right now. I just mention this since basically the
same problem exists there.

<URL:http://lists.linuxfoundation.org/pipermail/iommu/2013-November/index.html>

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Suman Anna]

Hi Santosh,

On 03/15/2014 12:54 PM, Santosh Shilimkar wrote:
> On Friday 14 March 2014 09:49 PM, Suman Anna wrote:
>> Hi Santosh, Laurent, Russell, Arnd,
>>
>> On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
>>> On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
>>>> Hi Santosh,
>>>>
>>>> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
>>>>> + Russell, Arnd
>>>>>
>>>>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
>>>>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>>>>> The page table entries must be cleaned from the cache before being
>>>>>>> accessed by the IOMMU. Instead of implementing cache management manually
>>>>>>> (and ignoring L2 cache), use clean_dcache_area() to make sure the
>>>>>>> entries are visible to the device.
>>>>>>
>>>>>> Thanks for fixing this, this has been long pending. There have been some
>>>>>> previous attempts at this as well by Ramesh Gupta, with the last thread
>>>>>> (a long time now) being
>>>>>> http://marc.info/?t=134752035400001&r=1&w=2
>>>>>>
>>>>>> Santosh,
>>>>>> Can you please take a look at this patch and provide your comments?
>>>>>>
>>>>>> regards
>>>>>> Suman
>>>>>>
>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>>>>> ---
>>>>>>>
>>>>>>>     drivers/iommu/omap-iommu.c | 41 ++++++++++-----------------------------
>>>>>>>     1 file changed, 10 insertions(+), 31 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>>>>>>> index a893eca..bb605c9 100644
>>>>>>> --- a/drivers/iommu/omap-iommu.c
>>>>>>> +++ b/drivers/iommu/omap-iommu.c
>>>>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
>>>>>>>     /*
>>>>>>>      *    H/W pagetable operations
>>>>>>>      */
>>>>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
>>>>>>> +static void flush_pgtable(void *addr, size_t size)
>>>>>>>     {
>>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>>> -    do {
>>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pgd"
>>>>>>> -            : : "r" (first));
>>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>>> -    } while (first <= last);
>>>>>>> -}
>>>>>>> -
>>>>>>> -static void flush_iopte_range(u32 *first, u32 *last)
>>>>>>> -{
>>>>>>> -    /* FIXME: L2 cache should be taken care of if it exists */
>>>>>>> -    do {
>>>>>>> -        asm("mcr    p15, 0, %0, c7, c10, 1 @ flush_pte"
>>>>>>> -            : : "r" (first));
>>>>>>> -        first += L1_CACHE_BYTES / sizeof(*first);
>>>>>>> -    } while (first <= last);
>>>>>>> +    clean_dcache_area(addr, size);
>>>>>
>>>>> I remember NAKing this approach in past and my stand remains same.
>>>>> The cache APIs which you are trying to use here are not suppose
>>>>> to be used outside.
>>
>> So this particular change has a long history (have to dig through to educate myself). I have not seen clean_dcache_area attempted before, and I wasn't sure myself it it can be used here. Ramesh and Fernando definitely did start out with dmac_flush_range and outer_flush_range which was definitely nacked [1].
>>
> OK. Please wrap the lines appropriately while replying.

Hmm, weird. I don't see this with my other responses. But sorry for
the trouble.

>
>>>>
>>>> Please note that the omap-iommu driver already uses clean_dcache_area().
>>>> That's where I got the idea :-)
>>>>
>>> So that fall through the cracks then ;-)
>>>
>>>>> I think the right way to fix this is to make use of streaming APIs.
>>>>> If needed, IOMMU can have its own dma_ops for special case
>>>>> handling if any.
>>>>
>>>> I can replace clean_dcache_area() with dma_map_page() as done by the arm-smmu
>>>> driver. If that's fine I'll modify this patch accordingly in v2.
>>>>
>>
>> Ramesh had also attempted to use dma_page_single() previously [2], and Russell has instead suggested to extend the cpu cache operations [3].
>> Ramesh had worked based on this suggestion and the series reached v6 [4] (same as link I mentioned above) and this is the last attempt on this, but the thread went silent.
>>
>> I am wondering if that is still valid and is the right way to go, as this seems to be a common problem. I do see dmac_flush_range being used for similar purposes in msm_iommu.c and exynos-iommu.c, so looks like they also fell through the cracks.
>>
> Thanks for the links. I think you should revive the v6 series unless and until
> RMK has some other suggestion. This can also help to remove the wrong usages
> from other IOMMU drivers as you pointed out.

OK, will do.

regards
Suman

>
>> Laurent,
>> Can you drop this patch out from v2 so that it is not clubbed with the small cleanup patches, and we can track this separately?
>>
> Agree
>
> Regards,
> Santosh
>
>>
>> [1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
>> [2] http://marc.info/?t=130281804900005&r=1&w=2
>> [3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
>> [4] http://marc.info/?t=134752035400001&r=1&w=2
>>
>>
>

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Suman Anna]

Hi Laurent, Sakari,

On 03/16/2014 04:54 PM, Sakari Ailus wrote:
> Hi Laurent and Suman,
>
> On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
>> Hi Suman,
>>
>> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU discussion)
>>
>> On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
>>>> Hello,
>>>>
>>>> This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
>>>> found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
>>>> DMA API. The biggest issue is fixed by patch 5/5, while the other patches
>>>> fix smaller problems that I've noticed when reading the code, without
>>>> experiencing them at runtime.
>>>>
>>>> I'd like to take this as an opportunity to discuss OMAP IOMMU integration
>>>> with the ARM DMA mapping implementation. The idea is to hide the IOMMU
>>>> completely behind the DMA mapping API, making it completely transparent
>>>> to drivers.
>>>
>>> Thanks for starting the discussion.
>>>
>>>> A drivers will only need to allocate memory with dma_alloc_*, and behind
>>>> the scene the ARM DMA mapping implementation will find out that the
>>>> device is behind an IOMMU and will map the buffers through the IOMMU,
>>>> returning an I/O VA address to the driver. No direct call to the OMAP
>>>> IOMMU driver or to the IOMMU core should be necessary anymore.
>>>>
>>>> To use the IOMMU the ARM DMA implementation requires a VA mapping to be
>>>> created with a call to arm_iommu_create_mapping() and to then be attached
>>>> to the device with arm_iommu_attach_device(). This is currently not
>>>> performed by the OMAP IOMMU driver, I have thus added that code to the
>>>> OMAP3 ISP driver for now. I believe this to still be an improvement
>>>> compared to the current situation, as it allows getting rid of custom
>>>> memory allocation code in the OMAP3 ISP driver and custom I/O VA space
>>>> management in omap-iovmm. However, that code should ideally be removed
>>>> from the driver. The question is, where should it be moved to ?
>>>>
>>>> One possible solution would be to add the code to the OMAP IOMMU driver.
>>>> However, this would require all OMAP IOMMU users to be converted to the
>>>> ARM DMA API. I assume there would be issues that I don't foresee though.
>>>
>>> Yeah, I think this will pose some problems for the other main user of IOMMUs
>>> - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in OMAP4 and
>>> beyond). A remoteproc device also needs to map memory at specific addresses
>>> for its code and data sections, and not rely on a I/O VA address being given
>>> to it. The firmware sections are already linked at specific addresses, and
>>> so remoteproc needs to allocate memory, load the firmware and map into
>>> appropriate device addresses. We are doing this currently usage a
>>> combination of CMA memory to get contiguous memory (there are some
>>> restrictions for certain sections) and iommu_map/unmap API to program the
>>> MMU with these pages. This usage is different from what is expected from
>>> exchanging buffers, which can be allocated from a predefined mapping range.
>>> Even that one is tricky if we need to support different cache
>>> properties/attributes as the cache configuration is in general local to
>>> these processors.
>>
>> Right, we indeed need two levels of API, one for drivers such as remoteproc
>> that need direct control of the IOMMU, and one for drivers that only need to
>> map buffers without any additional requirement. In the second case the drivers
>> should ideally use the DMA mapping API not even be aware that an IOMMU is
>> present. This would require moving the ARM mapping allocation out of the
>> client driver.

Actually, I think there is one another use case, even with remoteprocs 
which is to runtime map buffers. This is different from the firmware 
management. The memory for buffers could have been allocated from other 
subsystems, but remoteproc would need just to manage the VA space and map.

>>
>> The IOMMU core or the IOMMU driver will need to know whether the driver
>> expects to control the IOMMU directly or to have it managed transparently. As
>> this is a software configuration I don't think the information belongs to DT.
>> The question is, how should this information be conveyed?

Can this be done through iommu_domain_set_attr? But that means the 
client driver has to dictate this. The iommu driver can be configured 
appropriately based on this.

>
> The driver would need to know that, I think.
>
> Currently the DMA mapping API doesn't allow explicit addressing to IOVA
> address space AFAIK. The IOMMU API does. It's a good question how to do this
> as I don't think there's even a way for the driver to explicitly obtain
> access to the IOMMU.
>
> The virtual address space allocation would need to take into account that
> some of the address space is actually mapped outside it. The iova library
> can do this already.
>
>>>> I'm not even sure whether the OMAP IOMMU driver would be the best place to
>>>> put that code. Ideally VA spaces should be created by the platform
>>>> somehow, and mapping of devices to IOMMUs should be handled by the IOMMU
>>>> core instead of the IOMMU drivers. We're not there yet, and the path
>>>> might not be straightforward, hence this attempt to start a constructive
>>>> discussion.
>>>>
>>>> A second completely unrelated problem that I'd like to get feedback on is
>>>> the suspend/resume support in the OMAP IOMMU driver, or rather the lack
>>>> thereof. The driver exports omap_iommu_save_ctx() and
>>>> omap_iommu_restore_ctx() functions and expects the IOMMU users to call
>>>> them directly. This is really hackish to say the least. A proper
>>>> suspend/resume implementation shouldn't be difficult to implement, and
>>>> I'm wondering whether the lack of proper power management support comes
>>>> from historical reasons only, or if there are problems I might not have
>>>> considered.
>>>
>>> Agreed, the current code definitely needs a cleanup and better organization
>>> (like folding into runtime suspend ops). The main thing is that we need the
>>> IOMMU to be activated as long as its parent device is active (like the
>>> omap3isp or a remoteproc device). And this behaviour needs separation from
>>> configuring it and programming for the first time. Only a user can dictate
>>> when an IOMMU is not needed. So we either need an API to activate or have
>>> access to the iommu's dev object somehow so that we can use the
>>> pm_runtime_get/put API to hold a usage counter and let the runtime suspend
>>> ops take care of save/restore without tearing down the domain or detaching
>>> the device.
>>
>> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), and
>> pm_runtime_put_sync() in iommu_disable(). The enable/disable functions are
>> called in the attach and detach handlers (as well as in the fault handler for
>> the disable function). As long as a device is attached the IOMMU will thus not
>> be suspended by runtime PM, but could be suspended by system PM.

This would all need some rearranging of course. It is also not about 
just disabling the clocks, we may have to assert the resets as well, due 
to couple of issues on OMAP4 and beyond.

>>
>> This leads to two separate issues. The first one is that the IOMMU will be
>> powered on as long as a device is attached, even if the device isn't in use.
>> This consumes power needlessly. It would probably be better to more the
>> enable/disable calls to the map/unmap handlers, as the IOMMU should be powered
>> off when no mapping exists (or am I missing something ?).
>
> In general, I wouldn't make the assumption that no mappings can exist if the
> IOMMU is powered off. This would mean that just keeping buffers around for
> later use would keep hardware powered. Memory allocation can be a very
> time-consuming task which often is done when the entire system is booted up
> (depending on the user space naturally), not when a particular device node
> is opened.

Yeah, agreed. remoteproc firmware memory will stay mapped in even when 
we power off IOMMU, we just preserve the required IOMMU context. I 
expect the suspend/resume code to preserve any locked TLBs and the TTB 
address. The page table entries would be auto-preserved as long as we 
don't free the table.

>
> That said, I don't have a good, obviously acceptable solution to propose to
> this problem right now. I think the IOMMU power state should be rather bound
> to the device's power state, but DMA mapping API doesn't know anything about
> IOMMUs as such. I wonder what kind of a reception would such an addition to
> the DMA mapping would receive. There are two things I could think of: either
> make the potential IOMMU visible to the driver so the driver can use runtime
> PM as usual, or add a function to the DMA mapping API to allow whatever
> hardware used in the DMA accesses to power down (and up again when needed).
>
> The latter is hackish in my opinion but the former would also change the DMA
> mapping API to a direction which not everyone would probably agree on.
>
> I'm personally fine with the above limitation on OMAP3 but in general it'd
> be better to get around it.

Once we add the iommus property to the client DT nodes, I think the 
drivers should be able to retrieve the iommu dev object. So that should 
allow both the IOMMU to be visible, as well as give access to invoke 
runtime_pm functions on the iommu device.

>
>> The second issue is that the IOMMU and IOMMU users system PM suspend/resume
>> callbacks are not synchronized. We need a guarantee that the IOMMU will be
>> suspended after its users, and resumed before them. One option to fix this
>> could be to use the suspend_late and resume_early PM operations (although I
>> haven't checked whether that would work), but it's probably a bit hackish.
>
> I agree.
>
> The IOMMUs would indeed need to be resumed first. Hiroshi had a aptchset to
> initialise and associate IOMMUs to devices using DT but it seems it's not
> exactly going somewhere right now. I just mention this since basically the
> same problem exists there.
>
> <URL:http://lists.linuxfoundation.org/pipermail/iommu/2013-November/index.html>

Yeah, we haven't converted the current OMAP iommu users to DT yet, and I 
would expect some changes in the OMAP IOMMU code once that series is 
finalized.

regards
Suman

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Suman and Sakari,

On Monday 17 March 2014 14:58:24 Suman Anna wrote:
> On 03/16/2014 04:54 PM, Sakari Ailus wrote:
> > On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> >> Hi Suman,
> >> 
> >> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU discussion)
> >> 
> >> On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
> >>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> >>>> Hello,
> >>>> 
> >>>> This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
> >>>> found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
> >>>> DMA API. The biggest issue is fixed by patch 5/5, while the other
> >>>> patches fix smaller problems that I've noticed when reading the code,
> >>>> without experiencing them at runtime.
> >>>> 
> >>>> I'd like to take this as an opportunity to discuss OMAP IOMMU
> >>>> integration with the ARM DMA mapping implementation. The idea is to
> >>>> hide the IOMMU completely behind the DMA mapping API, making it
> >>>> completely transparent to drivers.
> >>> 
> >>> Thanks for starting the discussion.
> >>> 
> >>>> A drivers will only need to allocate memory with dma_alloc_*, and
> >>>> behind the scene the ARM DMA mapping implementation will find out that
> >>>> the device is behind an IOMMU and will map the buffers through the
> >>>> IOMMU, returning an I/O VA address to the driver. No direct call to the
> >>>> OMAP IOMMU driver or to the IOMMU core should be necessary anymore.
> >>>> 
> >>>> To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> >>>> created with a call to arm_iommu_create_mapping() and to then be
> >>>> attached to the device with arm_iommu_attach_device(). This is
> >>>> currently not performed by the OMAP IOMMU driver, I have thus added
> >>>> that code to the OMAP3 ISP driver for now. I believe this to still be
> >>>> an improvement compared to the current situation, as it allows getting
> >>>> rid of custom memory allocation code in the OMAP3 ISP driver and custom
> >>>> I/O VA space management in omap-iovmm. However, that code should
> >>>> ideally be removed from the driver. The question is, where should it be
> >>>> moved to ?
> >>>> 
> >>>> One possible solution would be to add the code to the OMAP IOMMU
> >>>> driver. However, this would require all OMAP IOMMU users to be
> >>>> converted to the ARM DMA API. I assume there would be issues that I
> >>>> don't foresee though.
> >>> 
> >>> Yeah, I think this will pose some problems for the other main user of
> >>> IOMMUs - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in
> >>> OMAP4 and beyond). A remoteproc device also needs to map memory at
> >>> specific addresses for its code and data sections, and not rely on a
> >>> I/O VA address being given to it. The firmware sections are already
> >>> linked at specific addresses, and so remoteproc needs to allocate
> >>> memory, load the firmware and map into appropriate device addresses. We
> >>> are doing this currently usage a combination of CMA memory to get
> >>> contiguous memory (there are some restrictions for certain sections) and
> >>> iommu_map/unmap API to program the MMU with these pages. This usage is
> >>> different from what is expected from exchanging buffers, which can be
> >>> allocated from a predefined mapping range. Even that one is tricky if we
> >>> need to support different cache properties/attributes as the cache
> >>> configuration is in general local to these processors.
> >> 
> >> Right, we indeed need two levels of API, one for drivers such as
> >> remoteproc that need direct control of the IOMMU, and one for drivers
> >> that only need to map buffers without any additional requirement. In the
> >> second case the drivers should ideally use the DMA mapping API not even
> >> be aware that an IOMMU is present. This would require moving the ARM
> >> mapping allocation out of the client driver.
> 
> Actually, I think there is one another use case, even with remoteprocs which
> is to runtime map buffers. This is different from the firmware management.
> The memory for buffers could have been allocated from other subsystems, but
> remoteproc would need just to manage the VA space and map.

Right, although you might not always have to manage the VA space in that case. 
Anyway, if your driver needs to manage the VA space for the firmware, it 
should be able to manage the VA space for the buffers using the same IOMMU 
API.

> >> The IOMMU core or the IOMMU driver will need to know whether the driver
> >> expects to control the IOMMU directly or to have it managed
> >> transparently. As this is a software configuration I don't think the
> >> information belongs to DT. The question is, how should this information
> >> be conveyed?
> 
> Can this be done through iommu_domain_set_attr? But that means the client
> driver has to dictate this. The iommu driver can be configured appropriately
> based on this.

The problem with that approach is that IOMMU client (bus master) drivers would 
need a pointer to the IOMMU domain. That's what we want to avoid in the first 
place :-)

> > The driver would need to know that, I think.

The IOMMU client driver would know that. Or, to be accurate, it should either 
know that it wants to manage the IOMMU directly, or should ignore the IOMMU 
completely.

> > Currently the DMA mapping API doesn't allow explicit addressing to IOVA
> > address space AFAIK. The IOMMU API does. It's a good question how to do
> > this as I don't think there's even a way for the driver to explicitly
> > obtain access to the IOMMU.

A driver can't access the IOMMU device directly, but the IOMMU API uses IOMMU 
domains, not IOMMU devices. Drivers can create domains and then access the 
IOMMU API. The association with a particular IOMMU instance is currently left 
to the IOMMU driver, that usually gets that information from platform data or 
DT in a driver-specific way. I think this should be standardized, especially 
in the DT case.

What drivers can't do at the moment is accessing the IOMMU API using a domain 
they haven't created themselves. A two drivers requiring direct access to an 
IOMMU sharing the same domain wouldn't be possible. That might not be a 
problem we need to solve now though.

> > The virtual address space allocation would need to take into account that
> > some of the address space is actually mapped outside it. The iova library
> > can do this already.

This only needs to be taken into account for drivers that require direct 
control of the IOMMU. Those drivers are currently expected to manage the whole 
VA space themselves. They can do so internally, or using a library such as 
iova, yes, but they won't use the DMA API IOMMU integration.

> >>>> I'm not even sure whether the OMAP IOMMU driver would be the best place
> >>>> to put that code. Ideally VA spaces should be created by the platform
> >>>> somehow, and mapping of devices to IOMMUs should be handled by the
> >>>> IOMMU core instead of the IOMMU drivers. We're not there yet, and the
> >>>> path might not be straightforward, hence this attempt to start a
> >>>> constructive discussion.
> >>>> 
> >>>> A second completely unrelated problem that I'd like to get feedback on
> >>>> is the suspend/resume support in the OMAP IOMMU driver, or rather the
> >>>> lack thereof. The driver exports omap_iommu_save_ctx() and
> >>>> omap_iommu_restore_ctx() functions and expects the IOMMU users to call
> >>>> them directly. This is really hackish to say the least. A proper
> >>>> suspend/resume implementation shouldn't be difficult to implement, and
> >>>> I'm wondering whether the lack of proper power management support comes
> >>>> from historical reasons only, or if there are problems I might not have
> >>>> considered.
> >>> 
> >>> Agreed, the current code definitely needs a cleanup and better
> >>> organization (like folding into runtime suspend ops). The main thing is
> >>> that we need the IOMMU to be activated as long as its parent device is
> >>> active (like the omap3isp or a remoteproc device). And this behaviour
> >>> needs separation from configuring it and programming for the first time.
> >>> Only a user can dictate when an IOMMU is not needed. So we either need
> >>> an API to activate or have access to the iommu's dev object somehow so
> >>> that we can use the pm_runtime_get/put API to hold a usage counter and
> >>> let the runtime suspend ops take care of save/restore without tearing
> >>> down the domain or detaching the device.
> >> 
> >> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), and
> >> pm_runtime_put_sync() in iommu_disable(). The enable/disable functions
> >> are called in the attach and detach handlers (as well as in the fault
> >> handler for the disable function). As long as a device is attached the
> >> IOMMU will thus not be suspended by runtime PM, but could be suspended by
> >> system PM.
> 
> This would all need some rearranging of course. It is also not about just
> disabling the clocks, we may have to assert the resets as well, due to
> couple of issues on OMAP4 and beyond.

Right.
 
> >> This leads to two separate issues. The first one is that the IOMMU will
> >> be powered on as long as a device is attached, even if the device isn't
> >> in use. This consumes power needlessly. It would probably be better to
> >> more the enable/disable calls to the map/unmap handlers, as the IOMMU
> >> should be powered off when no mapping exists (or am I missing something
> >> ?).
> > 
> > In general, I wouldn't make the assumption that no mappings can exist if
> > the IOMMU is powered off. This would mean that just keeping buffers
> > around for later use would keep hardware powered. Memory allocation can
> > be a very time-consuming task which often is done when the entire system
> > is booted up (depending on the user space naturally), not when a
> > particular device node is opened.
> 
> Yeah, agreed. remoteproc firmware memory will stay mapped in even when we
> power off IOMMU, we just preserve the required IOMMU context. I expect the
> suspend/resume code to preserve any locked TLBs and the TTB address. The
> page table entries would be auto-preserved as long as we don't free the
> table.

I don't want to force the IOMMU to be powered on when a mapping exists, but at 
the moment the driver powers it on when a device is attached, which is even 
worse. Moving the pm_runtime_get/set calls to map/unmap would be an 
improvement compared to the current situation, but is obviously not the 
perfect solution.

> > That said, I don't have a good, obviously acceptable solution to propose
> > to this problem right now. I think the IOMMU power state should be rather
> > bound to the device's power state, but DMA mapping API doesn't know
> > anything about IOMMUs as such. I wonder what kind of a reception would
> > such an addition to the DMA mapping would receive. There are two things I
> > could think of: either make the potential IOMMU visible to the driver so
> > the driver can use runtime PM as usual, or add a function to the DMA
> > mapping API to allow whatever hardware used in the DMA accesses to power
> > down (and up again when needed).
> > 
> > The latter is hackish in my opinion but the former would also change the
> > DMA mapping API to a direction which not everyone would probably agree
> > on.

I don't like the first option, as drivers should not be aware of the IOMMU at 
all in the general case. The second option is pretty hackish as well.

As the IOMMU driver is notified when a device is attached and detached, I 
think the right solution would be to then ask the PM core to notify the IOMMU 
driver when the attached devices are suspended and resumed, and block suspend 
as long as a user is awake.

I'm not sure whether we have any PM notification mechanism though, and whether 
we can ask suspend to be deferred similarly to deferred probing.

> > I'm personally fine with the above limitation on OMAP3 but in general it'd
> > be better to get around it.
> 
> Once we add the iommus property to the client DT nodes, I think the drivers
> should be able to retrieve the iommu dev object. So that should allow both
> the IOMMU to be visible, as well as give access to invoke runtime_pm
> functions on the iommu device.

I'd like to avoid that. On one family of SoCs I work with several IOMMU 
instances serve dozens of devices each. I don't want to modify each device 
driver to take the IOMMU into account. If one driver needs to handle the IOMMU 
and IOMMU client power dependency, it should be the IOMMU driver.

> >> The second issue is that the IOMMU and IOMMU users system PM
> >> suspend/resume callbacks are not synchronized. We need a guarantee that
> >> the IOMMU will be suspended after its users, and resumed before them. One
> >> option to fix this could be to use the suspend_late and resume_early PM
> >> operations (although I haven't checked whether that would work), but it's
> >> probably a bit hackish.
> > 
> > I agree.
> > 
> > The IOMMUs would indeed need to be resumed first. Hiroshi had a aptchset
> > to initialise and associate IOMMUs to devices using DT but it seems it's
> > not exactly going somewhere right now. I just mention this since basically
> > the> same problem exists there.
> > 
> > <URL:http://lists.linuxfoundation.org/pipermail/iommu/2013-November/index.
> > html>
>
> Yeah, we haven't converted the current OMAP iommu users to DT yet, and I
> would expect some changes in the OMAP IOMMU code once that series is
> finalized.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Laurent Pinchart]

Hi all,

On Friday 14 March 2014 20:49:56 Suman Anna wrote:
> On 03/14/2014 12:51 PM, Santosh Shilimkar wrote:
> > On Friday 14 March 2014 12:38 PM, Laurent Pinchart wrote:
> >> On Friday 14 March 2014 12:15:11 Santosh Shilimkar wrote:
> >>> On Thursday 13 March 2014 10:47 PM, Anna, Suman wrote:
> >>>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> >>>>> The page table entries must be cleaned from the cache before being
> >>>>> accessed by the IOMMU. Instead of implementing cache management
> >>>>> manually (and ignoring L2 cache), use clean_dcache_area() to make sure
> >>>>> the entries are visible to the device.
> >>>> 
> >>>> Thanks for fixing this, this has been long pending. There have been
> >>>> some previous attempts at this as well by Ramesh Gupta, with the last
> >>>> thread (a long time now) being
> >>>> http://marc.info/?t=134752035400001&r=1&w=2
> >>>> 
> >>>> Santosh,
> >>>> Can you please take a look at this patch and provide your comments?
> >>>> 
> >>>> regards
> >>>> Suman
> >>>> 
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/iommu/omap-iommu.c | 41 ++++++++++---------------------------
> >>>>>  1 file changed, 10 insertions(+), 31 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> >>>>> index a893eca..bb605c9 100644
> >>>>> --- a/drivers/iommu/omap-iommu.c
> >>>>> +++ b/drivers/iommu/omap-iommu.c
> >>>>> @@ -500,24 +500,9 @@ EXPORT_SYMBOL_GPL(omap_foreach_iommu_device);
> >>>>>    /*
> >>>>>     *	H/W pagetable operations
> >>>>>     */
> >>>>> -static void flush_iopgd_range(u32 *first, u32 *last)
> >>>>> +static void flush_pgtable(void *addr, size_t size)
> >>>>>  {
> >>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
> >>>>> -	do {
> >>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pgd"
> >>>>> -		    : : "r" (first));
> >>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
> >>>>> -	} while (first <= last);
> >>>>> -}
> >>>>> -
> >>>>> -static void flush_iopte_range(u32 *first, u32 *last)
> >>>>> -{
> >>>>> -	/* FIXME: L2 cache should be taken care of if it exists */
> >>>>> -	do {
> >>>>> -		asm("mcr	p15, 0, %0, c7, c10, 1 @ flush_pte"
> >>>>> -		    : : "r" (first));
> >>>>> -		first += L1_CACHE_BYTES / sizeof(*first);
> >>>>> -	} while (first <= last);
> >>>>> +	clean_dcache_area(addr, size);
> >>> 
> >>> I remember NAKing this approach in past and my stand remains same.
> >>> The cache APIs which you are trying to use here are not suppose
> >>> to be used outside.
> 
> So this particular change has a long history (have to dig through to educate
> myself).

Thank you for doing so, the result is pretty informative.

> I have not seen clean_dcache_area attempted before, and I wasn't sure myself
> it it can be used here. Ramesh and Fernando definitely did start out with
> dmac_flush_range and outer_flush_range which was definitely nacked [1].
> 
> >> Please note that the omap-iommu driver already uses clean_dcache_area().
> >> That's where I got the idea :-)
> > 
> > So that fall through the cracks then ;-)
> > 
> >>> I think the right way to fix this is to make use of streaming APIs.
> >>> If needed, IOMMU can have its own dma_ops for special case
> >>> handling if any.
> >> 
> >> I can replace clean_dcache_area() with dma_map_page() as done by the
> >> arm-smmu driver. If that's fine I'll modify this patch accordingly in
> >> v2.
> 
> Ramesh had also attempted to use dma_page_single() previously [2], and
> Russell has instead suggested to extend the cpu cache operations [3].
> Ramesh had worked based on this suggestion and the series reached v6 [4]
> (same as link I mentioned above) and this is the last attempt on this,
> but the thread went silent.

I'm not sure yet whether I totally agree with Russell here, not on the 
rationale of his opinion, but on what we're trying to achieve. Isn't the IOMMU 
just a device that can perform direct memory access to fetch page table 
entries ? It seems to me that what the driver needs is to make sure that 
changes to the page tables are visible to the device when performing direct 
memory access. That doesn't really differ from other DMA use cases, so why 
would it need an IOMMU-specific API ? Or does the OMAP IOMMU fetch page table 
entries using a special kind of DMA ?

> I am wondering if that is still valid and is the right way to go, as this
> seems to be a common problem. I do see dmac_flush_range being used for
> similar purposes in msm_iommu.c and exynos-iommu.c, so looks like they also
> fell through the cracks.
> 
> Laurent,
> Can you drop this patch out from v2 so that it is not clubbed with the small
> cleanup patches, and we can track this separately?

Sure, I'll do that.

> [1] http://marc.info/?l=linux-omap&m=129907009019355&w=2
> [2] http://marc.info/?t=130281804900005&r=1&w=2
> [3] http://marc.info/?l=linux-omap&m=131310179423214&w=2
> [4] http://marc.info/?t=134752035400001&r=1&w=2

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Laurent Pinchart]

Hi Arnd,

On Friday 14 March 2014 17:57:48 Arnd Bergmann wrote:
> On Friday 14 March 2014, Santosh Shilimkar wrote:
> > I remember NAKing this approach in past and my stand remains same. The
> > cache APIs which you are trying to use here are not suppose to be used
> > outside.
> > 
> > I think the right way to fix this is to make use of streaming APIs.
> > If needed, IOMMU can have its own dma_ops for special case handling if
> > any.
> > 
> > Russell, Arnd might have more ideas.
> 
> I have a bad feeling about using the dma-mapping API within the IOMMU code,
> because that driver is also used to implement the dma-mapping API for
> devices attached to the IOMMU. It's possible that it just works.

Right, but I hope the memory port used by the IOMMU to fetch page table 
entries will not go through itself the same IOMMU :-)

> Is the IOMMU actually designed to have page tables in noncoherent memory? I
> would have expected that the iopt accesses must all be done on
> dma_alloc_coherent() provided memory to guarantee proper accesses.

I'm not knowledgeable enough about the OMAP IOMMU, but at least the Renesas 
IOMMU doesn't need coherent memory as long as the driver makes sure that 
changes to the page tables are made visible to the device. That IOMMU can also 
use the DVM hardware coherency protocols on a cache coherent interconnect, but 
I haven't investigated that yet.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] iommu/omap: Use the cache cleaning API

[Suman Anna]

On 03/17/2014 06:12 PM, Laurent Pinchart wrote:
> Hi Arnd,
> 
> On Friday 14 March 2014 17:57:48 Arnd Bergmann wrote:
>> On Friday 14 March 2014, Santosh Shilimkar wrote:
>>> I remember NAKing this approach in past and my stand remains same. The
>>> cache APIs which you are trying to use here are not suppose to be used
>>> outside.
>>>
>>> I think the right way to fix this is to make use of streaming APIs.
>>> If needed, IOMMU can have its own dma_ops for special case handling if
>>> any.
>>>
>>> Russell, Arnd might have more ideas.
>>
>> I have a bad feeling about using the dma-mapping API within the IOMMU code,
>> because that driver is also used to implement the dma-mapping API for
>> devices attached to the IOMMU. It's possible that it just works.
> 
> Right, but I hope the memory port used by the IOMMU to fetch page table 
> entries will not go through itself the same IOMMU :-)
> 
>> Is the IOMMU actually designed to have page tables in noncoherent memory? I
>> would have expected that the iopt accesses must all be done on
>> dma_alloc_coherent() provided memory to guarantee proper accesses.
> 
> I'm not knowledgeable enough about the OMAP IOMMU, but at least the Renesas 
> IOMMU doesn't need coherent memory as long as the driver makes sure that 
> changes to the page tables are made visible to the device. 

That is the case with OMAP IOMMU as well, we have all along been using
non-coherent memory. Switching to coherent memory will probably simplify
things in the IOMMU driver (and eliminate this discussion on OMAP :))

regards
Suman

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Joerg Roedel]

On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> Right, we indeed need two levels of API, one for drivers such as remoteproc 
> that need direct control of the IOMMU, and one for drivers that only need to 
> map buffers without any additional requirement. In the second case the drivers 
> should ideally use the DMA mapping API not even be aware that an IOMMU is 
> present. This would require moving the ARM mapping allocation out of the 
> client driver.

These two levels exist in the form of the DMA-API and the IOMMU-API. In
fact, the IOMMU-API was created to provide a way to drivers to specifiy
the IOVA->PHYS mappings on its own.

> The IOMMU core or the IOMMU driver will need to know whether the driver 
> expects to control the IOMMU directly or to have it managed transparently. As 
> this is a software configuration I don't think the information belongs to DT. 
> The question is, how should this information be conveyed ?

The way this works on x86 is that a device driver can use the DMA-API
per default. If it wants to use the IOMMU-API it has to allocate a
domain and add the device it handles to this domain (it can't use
DMA-API anymore then).


	Joerg

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Marek Szyprowski]

Hello,

I'm sorry for a delay, I've got back from my holidays and I'm still busy 
trying
to get thought all the emails.

On 2014-03-17 23:44, Laurent Pinchart wrote:
> Hi Suman and Sakari,
>
> On Monday 17 March 2014 14:58:24 Suman Anna wrote:
> > On 03/16/2014 04:54 PM, Sakari Ailus wrote:
> > > On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> > >> Hi Suman,
> > >>
> > >> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU discussion)
> > >>
> > >> On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
> > >>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > >>>> Hello,
> > >>>>
> > >>>> This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
> > >>>> found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
> > >>>> DMA API. The biggest issue is fixed by patch 5/5, while the other
> > >>>> patches fix smaller problems that I've noticed when reading the code,
> > >>>> without experiencing them at runtime.
> > >>>>
> > >>>> I'd like to take this as an opportunity to discuss OMAP IOMMU
> > >>>> integration with the ARM DMA mapping implementation. The idea is to
> > >>>> hide the IOMMU completely behind the DMA mapping API, making it
> > >>>> completely transparent to drivers.
> > >>>
> > >>> Thanks for starting the discussion.
> > >>>
> > >>>> A drivers will only need to allocate memory with dma_alloc_*, and
> > >>>> behind the scene the ARM DMA mapping implementation will find out that
> > >>>> the device is behind an IOMMU and will map the buffers through the
> > >>>> IOMMU, returning an I/O VA address to the driver. No direct call to the
> > >>>> OMAP IOMMU driver or to the IOMMU core should be necessary anymore.
> > >>>>
> > >>>> To use the IOMMU the ARM DMA implementation requires a VA mapping to be
> > >>>> created with a call to arm_iommu_create_mapping() and to then be
> > >>>> attached to the device with arm_iommu_attach_device(). This is
> > >>>> currently not performed by the OMAP IOMMU driver, I have thus added
> > >>>> that code to the OMAP3 ISP driver for now. I believe this to still be
> > >>>> an improvement compared to the current situation, as it allows getting
> > >>>> rid of custom memory allocation code in the OMAP3 ISP driver and custom
> > >>>> I/O VA space management in omap-iovmm. However, that code should
> > >>>> ideally be removed from the driver. The question is, where should it be
> > >>>> moved to ?
> > >>>>
> > >>>> One possible solution would be to add the code to the OMAP IOMMU
> > >>>> driver. However, this would require all OMAP IOMMU users to be
> > >>>> converted to the ARM DMA API. I assume there would be issues that I
> > >>>> don't foresee though.
> > >>>
> > >>> Yeah, I think this will pose some problems for the other main user of
> > >>> IOMMUs - the remoteproc devices (DSP, Dual-Cortex M3/M4 processors in
> > >>> OMAP4 and beyond). A remoteproc device also needs to map memory at
> > >>> specific addresses for its code and data sections, and not rely on a
> > >>> I/O VA address being given to it. The firmware sections are already
> > >>> linked at specific addresses, and so remoteproc needs to allocate
> > >>> memory, load the firmware and map into appropriate device addresses. We
> > >>> are doing this currently usage a combination of CMA memory to get
> > >>> contiguous memory (there are some restrictions for certain sections) and
> > >>> iommu_map/unmap API to program the MMU with these pages. This usage is
> > >>> different from what is expected from exchanging buffers, which can be
> > >>> allocated from a predefined mapping range. Even that one is tricky if we
> > >>> need to support different cache properties/attributes as the cache
> > >>> configuration is in general local to these processors.
> > >>
> > >> Right, we indeed need two levels of API, one for drivers such as
> > >> remoteproc that need direct control of the IOMMU, and one for drivers
> > >> that only need to map buffers without any additional requirement. In the
> > >> second case the drivers should ideally use the DMA mapping API not even
> > >> be aware that an IOMMU is present. This would require moving the ARM
> > >> mapping allocation out of the client driver.
> >
> > Actually, I think there is one another use case, even with remoteprocs which
> > is to runtime map buffers. This is different from the firmware management.
> > The memory for buffers could have been allocated from other subsystems, but
> > remoteproc would need just to manage the VA space and map.
>
> Right, although you might not always have to manage the VA space in that case.
> Anyway, if your driver needs to manage the VA space for the firmware, it
> should be able to manage the VA space for the buffers using the same IOMMU
> API.

I've already seen some use cases which require to give a client device 
limited
access to DMA mapping IOMMU related structures. If we agree on API, I see no
problems to add such calls to dma-mapping. However in the most common case I
would like to hide the presence of IOMMU from the client device at all.

> > >> The IOMMU core or the IOMMU driver will need to know whether the driver
> > >> expects to control the IOMMU directly or to have it managed
> > >> transparently. As this is a software configuration I don't think the
> > >> information belongs to DT. The question is, how should this information
> > >> be conveyed?
> >
> > Can this be done through iommu_domain_set_attr? But that means the client
> > driver has to dictate this. The iommu driver can be configured appropriately
> > based on this.
>
> The problem with that approach is that IOMMU client (bus master) drivers would
> need a pointer to the IOMMU domain. That's what we want to avoid in the first
> place :-)
>
> > > The driver would need to know that, I think.
>
> The IOMMU client driver would know that. Or, to be accurate, it should either
> know that it wants to manage the IOMMU directly, or should ignore the IOMMU
> completely.
>
> > > Currently the DMA mapping API doesn't allow explicit addressing to IOVA
> > > address space AFAIK. The IOMMU API does. It's a good question how to do
> > > this as I don't think there's even a way for the driver to explicitly
> > > obtain access to the IOMMU.
>
> A driver can't access the IOMMU device directly, but the IOMMU API uses IOMMU
> domains, not IOMMU devices. Drivers can create domains and then access the
> IOMMU API. The association with a particular IOMMU instance is currently left
> to the IOMMU driver, that usually gets that information from platform data or
> DT in a driver-specific way. I think this should be standardized, especially
> in the DT case.
>
> What drivers can't do at the moment is accessing the IOMMU API using a domain
> they haven't created themselves. A two drivers requiring direct access to an
> IOMMU sharing the same domain wouldn't be possible. That might not be a
> problem we need to solve now though.
>
> > > The virtual address space allocation would need to take into account that
> > > some of the address space is actually mapped outside it. The iova library
> > > can do this already.
>
> This only needs to be taken into account for drivers that require direct
> control of the IOMMU. Those drivers are currently expected to manage the whole
> VA space themselves. They can do so internally, or using a library such as
> iova, yes, but they won't use the DMA API IOMMU integration.
>
> > >>>> I'm not even sure whether the OMAP IOMMU driver would be the best place
> > >>>> to put that code. Ideally VA spaces should be created by the platform
> > >>>> somehow, and mapping of devices to IOMMUs should be handled by the
> > >>>> IOMMU core instead of the IOMMU drivers. We're not there yet, and the
> > >>>> path might not be straightforward, hence this attempt to start a
> > >>>> constructive discussion.
> > >>>>
> > >>>> A second completely unrelated problem that I'd like to get feedback on
> > >>>> is the suspend/resume support in the OMAP IOMMU driver, or rather the
> > >>>> lack thereof. The driver exports omap_iommu_save_ctx() and
> > >>>> omap_iommu_restore_ctx() functions and expects the IOMMU users to call
> > >>>> them directly. This is really hackish to say the least. A proper
> > >>>> suspend/resume implementation shouldn't be difficult to implement, and
> > >>>> I'm wondering whether the lack of proper power management support comes
> > >>>> from historical reasons only, or if there are problems I might not have
> > >>>> considered.
> > >>>
> > >>> Agreed, the current code definitely needs a cleanup and better
> > >>> organization (like folding into runtime suspend ops). The main thing is
> > >>> that we need the IOMMU to be activated as long as its parent device is
> > >>> active (like the omap3isp or a remoteproc device). And this behaviour
> > >>> needs separation from configuring it and programming for the first time.
> > >>> Only a user can dictate when an IOMMU is not needed. So we either need
> > >>> an API to activate or have access to the iommu's dev object somehow so
> > >>> that we can use the pm_runtime_get/put API to hold a usage counter and
> > >>> let the runtime suspend ops take care of save/restore without tearing
> > >>> down the domain or detaching the device.
> > >>
> > >> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), and
> > >> pm_runtime_put_sync() in iommu_disable(). The enable/disable functions
> > >> are called in the attach and detach handlers (as well as in the fault
> > >> handler for the disable function). As long as a device is attached the
> > >> IOMMU will thus not be suspended by runtime PM, but could be suspended by
> > >> system PM.
> >
> > This would all need some rearranging of course. It is also not about just
> > disabling the clocks, we may have to assert the resets as well, due to
> > couple of issues on OMAP4 and beyond.
>
> Right.
>   
> > >> This leads to two separate issues. The first one is that the IOMMU will
> > >> be powered on as long as a device is attached, even if the device isn't
> > >> in use. This consumes power needlessly. It would probably be better to
> > >> more the enable/disable calls to the map/unmap handlers, as the IOMMU
> > >> should be powered off when no mapping exists (or am I missing something
> > >> ?).
> > >
> > > In general, I wouldn't make the assumption that no mappings can exist if
> > > the IOMMU is powered off. This would mean that just keeping buffers
> > > around for later use would keep hardware powered. Memory allocation can
> > > be a very time-consuming task which often is done when the entire system
> > > is booted up (depending on the user space naturally), not when a
> > > particular device node is opened.
> >
> > Yeah, agreed. remoteproc firmware memory will stay mapped in even when we
> > power off IOMMU, we just preserve the required IOMMU context. I expect the
> > suspend/resume code to preserve any locked TLBs and the TTB address. The
> > page table entries would be auto-preserved as long as we don't free the
> > table.
>
> I don't want to force the IOMMU to be powered on when a mapping exists, but at
> the moment the driver powers it on when a device is attached, which is even
> worse. Moving the pm_runtime_get/set calls to map/unmap would be an
> improvement compared to the current situation, but is obviously not the
> perfect solution.

Basically the IOMMU should be enabled when client device called 
pm_runtime_get_sync()
and can disabled after client's pm_runtime_put(). Too bad that this 
cannot be
easily done. Initially I hooked IOMMU device as a parent device for the 
client
device. This worked quite fine until pm_domain has been introduced. With 
pm_domains
things are much more complicated, because we cannot use parent relationship
(it won't work). For internal use a hack proposed by Cho KyongHo, where 
IOMMU driver
replaces pm_domain_ops of the client device. I'm working on finding a 
less hacky
solution.

For a reference, please check drivers/iommu/exynos-iommu.c file from the 
following repo:
https://review.tizen.org/git/?p=platform/kernel/linux-3.10.git;a=shortlog;h=refs/heads/tizen

(...)

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

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Joerg,

On Friday 04 April 2014 12:18:11 Joerg Roedel wrote:
> On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> > Right, we indeed need two levels of API, one for drivers such as
> > remoteproc that need direct control of the IOMMU, and one for drivers that
> > only need to map buffers without any additional requirement. In the second
> > case the drivers should ideally use the DMA mapping API not even be aware
> > that an IOMMU is present. This would require moving the ARM mapping
> > allocation out of the client driver.
> 
> These two levels exist in the form of the DMA-API and the IOMMU-API. In
> fact, the IOMMU-API was created to provide a way to drivers to specifiy
> the IOVA->PHYS mappings on its own.

I agree with you, the two levels are already present, but there's still rough 
edges that we need to soften. 

The ARM DMA API implementation requires someone to create the VA space mapping 
by calling arm_iommu_create_mapping() and attach devices to mappings by 
calling arm_iommu_attach_device(). This must only be performed for devices 
that use the DMA API, devices that manage their IOMMU directly will call to 
the IOMMU API directly.

One obvious possibility is to call those functions in the IOMMU bus masters 
device drivers. This is pretty easy, but makes the drivers IOMMU-aware (which 
I'd like to avoid) and doesn't allow multiple bus masters to share a VA space 
mapping. Another possibility is to place the calls in the IOMMU drivers. This 
has the advantage of removing any IOMMU knowledge from bus masters device 
drivers and allowing multiple bus masters per IOMMU, but makes IOMMU 
management by the DMA API unconditional.

Has anyone given this problem a though ?

> > The IOMMU core or the IOMMU driver will need to know whether the driver
> > expects to control the IOMMU directly or to have it managed transparently.
> > As this is a software configuration I don't think the information belongs
> > to DT. The question is, how should this information be conveyed ?
> 
> The way this works on x86 is that a device driver can use the DMA-API per
> default. If it wants to use the IOMMU-API it has to allocate a domain and
> add the device it handles to this domain (it can't use DMA-API anymore
> then).

Are drivers supposed to reimplement the DMA API internally in that case ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Marek,

On Friday 04 April 2014 14:23:57 Marek Szyprowski wrote:
> Hello,
> 
> I'm sorry for a delay, I've got back from my holidays and I'm still busy
> trying to get thought all the emails.

No worries.

> On 2014-03-17 23:44, Laurent Pinchart wrote:
> > On Monday 17 March 2014 14:58:24 Suman Anna wrote:
> > > On 03/16/2014 04:54 PM, Sakari Ailus wrote:
> > > > On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote:
> > > >> Hi Suman,
> > > >> 
> > > >> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU
> > > >> discussion)
> > > >> 
> > > >> On Thursday 13 March 2014 21:33:37 Suman Anna wrote:
> > > >>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
> > > >>>> Hello,
> > > >>>> 
> > > >>>> This patch set fixes miscellaneous issues with the OMAP IOMMU
> > > >>>> driver, found when trying to port the OMAP3 ISP away from omap-
> > > >>>> iovmm to the ARM DMA API. The biggest issue is fixed by patch 5/5,
> > > >>>> while the other patches fix smaller problems that I've noticed when
> > > >>>> reading the code, without experiencing them at runtime.
> > > >>>> 
> > > >>>> I'd like to take this as an opportunity to discuss OMAP IOMMU
> > > >>>> integration with the ARM DMA mapping implementation. The idea is to
> > > >>>> hide the IOMMU completely behind the DMA mapping API, making it
> > > >>>> completely transparent to drivers.
> > > >>> 
> > > >>> Thanks for starting the discussion.
> > > >>> 
> > > >>>> A drivers will only need to allocate memory with dma_alloc_*, and
> > > >>>> behind the scene the ARM DMA mapping implementation will find out
> > > >>>> that the device is behind an IOMMU and will map the buffers through
> > > >>>> the IOMMU, returning an I/O VA address to the driver. No direct
> > > >>>> call to the OMAP IOMMU driver or to the IOMMU core should be
> > > >>>> necessary anymore.
> > > >>>> 
> > > >>>> To use the IOMMU the ARM DMA implementation requires a VA mapping
> > > >>>> to be created with a call to arm_iommu_create_mapping() and to then
> > > >>>> be attached to the device with arm_iommu_attach_device(). This is
> > > >>>> currently not performed by the OMAP IOMMU driver, I have thus added
> > > >>>> that code to the OMAP3 ISP driver for now. I believe this to still
> > > >>>> be an improvement compared to the current situation, as it allows
> > > >>>> getting rid of custom memory allocation code in the OMAP3 ISP
> > > >>>> driver and custom I/O VA space management in omap-iovmm. However,
> > > >>>> that code should ideally be removed from the driver. The question
> > > >>>> is, where should it be moved to ?
> > > >>>> 
> > > >>>> One possible solution would be to add the code to the OMAP IOMMU
> > > >>>> driver. However, this would require all OMAP IOMMU users to be
> > > >>>> converted to the ARM DMA API. I assume there would be issues that I
> > > >>>> don't foresee though.
> > > >>> 
> > > >>> Yeah, I think this will pose some problems for the other main user
> > > >>> of IOMMUs - the remoteproc devices (DSP, Dual-Cortex M3/M4
> > > >>> processors in OMAP4 and beyond). A remoteproc device also needs to
> > > >>> map memory at specific addresses for its code and data sections, and
> > > >>> not rely on a I/O VA address being given to it. The firmware
> > > >>> sections are already linked at specific addresses, and so remoteproc
> > > >>> needs to allocate memory, load the firmware and map into appropriate
> > > >>> device addresses. We are doing this currently usage a combination of
> > > >>> CMA memory to get contiguous memory (there are some restrictions for
> > > >>> certain sections) and iommu_map/unmap API to program the MMU with
> > > >>> these pages. This usage is different from what is expected from
> > > >>> exchanging buffers, which can be allocated from a predefined mapping
> > > >>> range. Even that one is tricky if we need to support different cache
> > > >>> properties/attributes as the cache configuration is in general local
> > > >>> to these processors.
> > > >> 
> > > >> Right, we indeed need two levels of API, one for drivers such as
> > > >> remoteproc that need direct control of the IOMMU, and one for drivers
> > > >> that only need to map buffers without any additional requirement. In
> > > >> the second case the drivers should ideally use the DMA mapping API
> > > >> not even be aware that an IOMMU is present. This would require moving
> > > >> the ARM mapping allocation out of the client driver.
> > > 
> > > Actually, I think there is one another use case, even with remoteprocs
> > > which is to runtime map buffers. This is different from the firmware
> > > management. The memory for buffers could have been allocated from other
> > > subsystems, but remoteproc would need just to manage the VA space and
> > > map.
> > 
> > Right, although you might not always have to manage the VA space in that
> > case. Anyway, if your driver needs to manage the VA space for the
> > firmware, it should be able to manage the VA space for the buffers using
> > the same IOMMU API.
> 
> I've already seen some use cases which require to give a client device
> limited access to DMA mapping IOMMU related structures. If we agree on API,
> I see no problems to add such calls to dma-mapping. However in the most
> common case I would like to hide the presence of IOMMU from the client
> device at all.

I agree with you. I'd like to hide the IOMMU completely by default, but some 
drivers will need fine-grained control over the IOMMU and will thus likely 
need direct access. I really see two ways to solve this, either allowing 
drivers to manage the IOMMU directly when required, or extending the DMA API 
to allow some degree of IOMMU management in cooperation with the DMA API 
implementation. I'm not sure which is best, as I don't know all our current 
uses cases for direct IOMMU access in details, and I can't obviously foresee 
all the future use cases.

To achieve transparent IOMMU handling for drivers we need to move the 
arm_iommu_create_mapping() and arm_iommu_attach_device() calls from bus 
masters (IOMMU clients) device drivers to a more central location. This could 
be the IOMMU drivers, the IOMMU core, the bus drivers, ... However, if we 
decide to allow drivers direct control over the IOMMU, we also need to make 
those calls conditional, and thus give a way to device drivers to opt-out.

> > > >> The IOMMU core or the IOMMU driver will need to know whether the
> > > >> driver expects to control the IOMMU directly or to have it managed
> > > >> transparently. As this is a software configuration I don't think the
> > > >> information belongs to DT. The question is, how should this 
> > > >> information be conveyed?
> > > 
> > > Can this be done through iommu_domain_set_attr? But that means the
> > > client driver has to dictate this. The iommu driver can be configured
> > > appropriately based on this.
> > 
> > The problem with that approach is that IOMMU client (bus master) drivers
> > would need a pointer to the IOMMU domain. That's what we want to avoid in
> > the first place :-)
> > 
> > > > The driver would need to know that, I think.
> > 
> > The IOMMU client driver would know that. Or, to be accurate, it should
> > either know that it wants to manage the IOMMU directly, or should ignore
> > the IOMMU completely.
> > 
> > > > Currently the DMA mapping API doesn't allow explicit addressing to
> > > > IOVA address space AFAIK. The IOMMU API does. It's a good question how
> > > > to do this as I don't think there's even a way for the driver to
> > > > explicitly obtain access to the IOMMU.
> > 
> > A driver can't access the IOMMU device directly, but the IOMMU API uses
> > IOMMU domains, not IOMMU devices. Drivers can create domains and then
> > access the IOMMU API. The association with a particular IOMMU instance is
> > currently left to the IOMMU driver, that usually gets that information
> > from platform data or DT in a driver-specific way. I think this should be
> > standardized, especially in the DT case.
> > 
> > What drivers can't do at the moment is accessing the IOMMU API using a
> > domain they haven't created themselves. A two drivers requiring direct
> > access to an IOMMU sharing the same domain wouldn't be possible. That
> > might not be a problem we need to solve now though.
> > 
> > > > The virtual address space allocation would need to take into account
> > > > that some of the address space is actually mapped outside it. The iova
> > > > library can do this already.
> > 
> > This only needs to be taken into account for drivers that require direct
> > control of the IOMMU. Those drivers are currently expected to manage the
> > whole VA space themselves. They can do so internally, or using a library
> > such as iova, yes, but they won't use the DMA API IOMMU integration.
> > 
> > > >>>> I'm not even sure whether the OMAP IOMMU driver would be the best
> > > >>>> place to put that code. Ideally VA spaces should be created by the
> > > >>>> platform somehow, and mapping of devices to IOMMUs should be
> > > >>>> handled by the IOMMU core instead of the IOMMU drivers. We're not
> > > >>>> there yet, and the path might not be straightforward, hence this
> > > >>>> attempt to start a constructive discussion.
> > > >>>> 
> > > >>>> A second completely unrelated problem that I'd like to get feedback
> > > >>>> on is the suspend/resume support in the OMAP IOMMU driver, or
> > > >>>> rather the lack thereof. The driver exports omap_iommu_save_ctx()
> > > >>>> and omap_iommu_restore_ctx() functions and expects the IOMMU users
> > > >>>> to call them directly. This is really hackish to say the least. A
> > > >>>> proper suspend/resume implementation shouldn't be difficult to
> > > >>>> implement, and I'm wondering whether the lack of proper power
> > > >>>> management support comes from historical reasons only, or if there
> > > >>>> are problems I might not have considered.
> > > >>> 
> > > >>> Agreed, the current code definitely needs a cleanup and better
> > > >>> organization (like folding into runtime suspend ops). The main thing
> > > >>> is that we need the IOMMU to be activated as long as its parent
> > > >>> device is active (like the omap3isp or a remoteproc device). And
> > > >>> this behaviour needs separation from configuring it and programming
> > > >>> for the first time. Only a user can dictate when an IOMMU is not
> > > >>> needed. So we either need an API to activate or have access to the
> > > >>> iommu's dev object somehow so that we can use the pm_runtime_get/put
> > > >>> API to hold a usage counter and let the runtime suspend ops take
> > > >>> care of save/restore without tearing down the domain or detaching
> > > >>> the device.
> > > >> 
> > > >> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(),
> > > >> and pm_runtime_put_sync() in iommu_disable(). The enable/disable
> > > >> functions are called in the attach and detach handlers (as well as in
> > > >> the fault handler for the disable function). As long as a device is
> > > >> attached the IOMMU will thus not be suspended by runtime PM, but
> > > >>> could be suspended by system PM.
> > > 
> > > This would all need some rearranging of course. It is also not about
> > > just disabling the clocks, we may have to assert the resets as well, due
> > > to couple of issues on OMAP4 and beyond.
> > 
> > Right.
> > 
> > > >> This leads to two separate issues. The first one is that the IOMMU
> > > >> will be powered on as long as a device is attached, even if the
> > > >> device isn't in use. This consumes power needlessly. It would
> > > >> probably be better to more the enable/disable calls to the map/unmap
> > > >> handlers, as the IOMMU should be powered off when no mapping exists
> > > >> (or am I missing something ?).
> > > > 
> > > > In general, I wouldn't make the assumption that no mappings can exist
> > > > if the IOMMU is powered off. This would mean that just keeping buffers
> > > > around for later use would keep hardware powered. Memory allocation 
> > > > can be a very time-consuming task which often is done when the entire
> > > > system is booted up (depending on the user space naturally), not when
> > > > a particular device node is opened.
> > > 
> > > Yeah, agreed. remoteproc firmware memory will stay mapped in even when
> > > we power off IOMMU, we just preserve the required IOMMU context. I
> > > expect the suspend/resume code to preserve any locked TLBs and the TTB
> > > address. The page table entries would be auto-preserved as long as we
> > > don't free the table.
> > 
> > I don't want to force the IOMMU to be powered on when a mapping exists,
> > but at the moment the driver powers it on when a device is attached,
> > which is even worse. Moving the pm_runtime_get/set calls to map/unmap
> > would be an improvement compared to the current situation, but is
> > obviously not the perfect solution.
> 
> Basically the IOMMU should be enabled when client device called
> pm_runtime_get_sync() and can disabled after client's pm_runtime_put(). Too
> bad that this cannot be easily done.

Too bad indeed :-)

> Initially I hooked IOMMU device as a parent device for the client device.
> This worked quite fine until pm_domain has been introduced. With pm_domains
> things are much more complicated, because we cannot use parent relationship
> (it won't work). For internal use a hack proposed by Cho KyongHo, where
> IOMMU driver replaces pm_domain_ops of the client device. I'm working on
> finding a less hacky solution.

I was thinking about using some kind of PM notifier, but that might be a very 
naive approach. Have you considered it ?

> For a reference, please check drivers/iommu/exynos-iommu.c file from the
> following repo:
> https://review.tizen.org/git/?p=platform/kernel/linux-3.10.git;a=shortlog;h=
> refs/heads/tizen

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Joerg Roedel]

Hi Laurent,

On Tue, Apr 08, 2014 at 02:50:38PM +0200, Laurent Pinchart wrote:
> I agree with you, the two levels are already present, but there's still rough 
> edges that we need to soften. 
> 
> The ARM DMA API implementation requires someone to create the VA space mapping 
> by calling arm_iommu_create_mapping() and attach devices to mappings by 
> calling arm_iommu_attach_device().

Who is "someone" in this case?

> This must only be performed for devices that use the DMA API, devices
> that manage their IOMMU directly will call to the IOMMU API directly.
> 
> One obvious possibility is to call those functions in the IOMMU bus masters 
> device drivers. This is pretty easy, but makes the drivers IOMMU-aware (which 
> I'd like to avoid) and doesn't allow multiple bus masters to share a VA space 
> mapping. Another possibility is to place the calls in the IOMMU drivers. This 
> has the advantage of removing any IOMMU knowledge from bus masters device 
> drivers and allowing multiple bus masters per IOMMU, but makes IOMMU 
> management by the DMA API unconditional.

Why does that make IOMMU management by the DMA API unconditional? On x86
it works this way: The IOMMU drivers create DMA-API mappings for the
devices by default. So any driver can use the DMA-API just
out-of-the-box without being aware of an IOMMU. If the driver wants to
deal with the IOMMU directly, it creates its own iommu-domain and
attaches the device to it. The device uses the drivers domain then and
not the DMA-API domain setup by the IOMMU driver. On iommu-detach the
device is assigned back to its DMA-API domain.

> > The way this works on x86 is that a device driver can use the DMA-API per
> > default. If it wants to use the IOMMU-API it has to allocate a domain and
> > add the device it handles to this domain (it can't use DMA-API anymore
> > then).
> 
> Are drivers supposed to reimplement the DMA API internally in that case?

Usually not, but possible. Device drivers use the IOMMU-API when they
want to control the io-addresses themselves. This is the case for VFIO
for example, where a given range from a process address space is mapped
into a device address space. In those cases the device driver usally
does not need to implement its own address allocator.


Regards,

	Joerg

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Joerg,

On Tuesday 08 April 2014 15:43:22 Joerg Roedel wrote:
> On Tue, Apr 08, 2014 at 02:50:38PM +0200, Laurent Pinchart wrote:
> > I agree with you, the two levels are already present, but there's still
> > rough edges that we need to soften.
> > 
> > The ARM DMA API implementation requires someone to create the VA space
> > mapping by calling arm_iommu_create_mapping() and attach devices to
> > mappings by calling arm_iommu_attach_device().
> 
> Who is "someone" in this case?

That's exactly the problem :-) The ARM DMA API implementation doesn't care who 
that "someone" is. Existing implementations call those functions either from 
the bus masters device drivers (in which case the drivers need to be IOMMU-
aware, even if they use the DMA API and don't need to handle the IOMMU 
directly) or from the IOMMU drivers (in which case the bus masters device 
drivers don't have to care about the IOMMU, but without a way for drivers to 
handle the IOMMU directly when they need to). Possible other candidates are 
core IOMMU code or bus code.

> > This must only be performed for devices that use the DMA API, devices
> > that manage their IOMMU directly will call to the IOMMU API directly.
> > 
> > One obvious possibility is to call those functions in the IOMMU bus
> > masters device drivers. This is pretty easy, but makes the drivers IOMMU-
> > aware (which I'd like to avoid) and doesn't allow multiple bus masters to
> > share a VA space mapping. Another possibility is to place the calls in the
> > IOMMU drivers. This has the advantage of removing any IOMMU knowledge
> > from bus masters device drivers and allowing multiple bus masters per
> > IOMMU, but makes IOMMU management by the DMA API unconditional.
> 
> Why does that make IOMMU management by the DMA API unconditional? On x86 it
> works this way: The IOMMU drivers create DMA-API mappings for the devices by
> default. So any driver can use the DMA-API just out-of-the-box without being
> aware of an IOMMU. If the driver wants to deal with the IOMMU directly, it
> creates its own iommu-domain and attaches the device to it. The device uses
> the drivers domain then and not the DMA-API domain setup by the IOMMU
> driver. On iommu-detach the device is assigned back to its DMA-API domain.

If we call arm_iommu_attach_device() from the IOMMU driver to get default 
transparent IOMMU handling, the function will then attach the device to the 
default domain with a call to iommu_attach_device(). If a driver needs to 
handle the IOMMU directly, should it start by detaching the device from the 
ARM IOMMU domain ? We would need to be very careful with the assumptions made 
by the different layers, as they might not support a driver attaching a new 
domain to a device that already has a domain attached. I'd feel more 
comfortable with avoiding to attach the default domain to the device in the 
first place, but that might not be easily doable.

> > > The way this works on x86 is that a device driver can use the DMA-API
> > > per default. If it wants to use the IOMMU-API it has to allocate a
> > > domain and add the device it handles to this domain (it can't use DMA-
> > > API anymore then).
> > 
> > Are drivers supposed to reimplement the DMA API internally in that case?
> 
> Usually not, but possible. Device drivers use the IOMMU-API when they want
> to control the io-addresses themselves. This is the case for VFIO for
> example, where a given range from a process address space is mapped into a
> device address space. In those cases the device driver usally does not need
> to implement its own address allocator.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Joerg Roedel]

On Tue, Apr 08, 2014 at 05:02:37PM +0200, Laurent Pinchart wrote:
> On Tuesday 08 April 2014 15:43:22 Joerg Roedel wrote:
> > Who is "someone" in this case?
> 
> That's exactly the problem :-) The ARM DMA API implementation doesn't care who 
> that "someone" is. Existing implementations call those functions either from 
> the bus masters device drivers (in which case the drivers need to be IOMMU-
> aware, even if they use the DMA API and don't need to handle the IOMMU 
> directly) or from the IOMMU drivers (in which case the bus masters device 
> drivers don't have to care about the IOMMU, but without a way for drivers to 
> handle the IOMMU directly when they need to). Possible other candidates are 
> core IOMMU code or bus code.

That doesn't sound very transparent for the device drivers. All what you
describe above (attaching a device to its default domain for DMA-API)
should happen in the IOMMU driver. For the device driver it should make
no difference if there is an IOMMU or not.

> If we call arm_iommu_attach_device() from the IOMMU driver to get default 
> transparent IOMMU handling, the function will then attach the device to the 
> default domain with a call to iommu_attach_device().

If you have to call a function it is not transparent anymore.

> If a driver needs to handle the IOMMU directly, should it start by
> detaching the device from the ARM IOMMU domain ? We would need to be
> very careful with the assumptions made by the different layers, as
> they might not support a driver attaching a new domain to a device
> that already has a domain attached. I'd feel more comfortable with
> avoiding to attach the default domain to the device in the first
> place, but that might not be easily doable.

The way this is solved by others is that iommu_attach_device()
automatically detaches the device from its default (DMA-API) domain and
attach it to the device drivers own domain. On iommu_detach_device() the
device is attached back to the default domain.


	Joerg

Re: [PATCH 0/5] OMAP IOMMU fixes and IOMMU architecture questions

[Laurent Pinchart]

Hi Joerg,

On Wednesday 09 April 2014 17:08:31 Joerg Roedel wrote:
> On Tue, Apr 08, 2014 at 05:02:37PM +0200, Laurent Pinchart wrote:
> > On Tuesday 08 April 2014 15:43:22 Joerg Roedel wrote:
> > > Who is "someone" in this case?
> > 
> > That's exactly the problem :-) The ARM DMA API implementation doesn't care
> > who that "someone" is. Existing implementations call those functions
> > either from the bus masters device drivers (in which case the drivers
> > need to be IOMMU- aware, even if they use the DMA API and don't need to
> > handle the IOMMU directly) or from the IOMMU drivers (in which case the
> > bus masters device drivers don't have to care about the IOMMU, but
> > without a way for drivers to handle the IOMMU directly when they need
> > to). Possible other candidates are core IOMMU code or bus code.
> 
> That doesn't sound very transparent for the device drivers. All what you
> describe above (attaching a device to its default domain for DMA-API)
> should happen in the IOMMU driver. For the device driver it should make
> no difference if there is an IOMMU or not.

Creating the DMA API mapping and attaching the device to the domain are 
definitely not transparent when performed by bus master device drivers. That's 
just the current situation for several drivers, and we obviously want to 
change it.

> > If we call arm_iommu_attach_device() from the IOMMU driver to get default
> > transparent IOMMU handling, the function will then attach the device to
> > the default domain with a call to iommu_attach_device().
> 
> If you have to call a function it is not transparent anymore.

Sure it is, IOMMU handling is transparent for the bus master device (which is 
the goal) when arm_iommu_attach_device() is called from the IOMMU driver.

> > If a driver needs to handle the IOMMU directly, should it start by
> > detaching the device from the ARM IOMMU domain ? We would need to be
> > very careful with the assumptions made by the different layers, as
> > they might not support a driver attaching a new domain to a device
> > that already has a domain attached. I'd feel more comfortable with
> > avoiding to attach the default domain to the device in the first
> > place, but that might not be easily doable.
> 
> The way this is solved by others is that iommu_attach_device() automatically
> detaches the device from its default (DMA-API) domain and attach it to the
> device drivers own domain. On iommu_detach_device() the device is attached
> back to the default domain.

That might work, I'll need to give it a try. It will make the attach/detach 
operations pretty complex though, with copies of the same code in all IOMMU 
drivers. Refactoring will probably be needed, but I'll first see if I can get 
it working properly without modifying the IOMMU core.

-- 
Regards,

Laurent Pinchart