[PATCH/RFC 0/4] iommu: add workaround for R-Car Gen3 IPMMU

[PATCH/RFC 0/4] iommu: add workaround for R-Car Gen3 IPMMU

[Yoshihiro Shimoda]

This patch set is based on:
 iommu.git / next branch and the following patch that Magnus-san sent:
 [patch v6 00/07] iommu/ipmmu-vmsa: ipmmu multi-arch update v6

R-Car Gen3 IPMMU has an issue that will mistake an address translation
if IMCTR.FLUSH is set while some related devices that on the same domain
are running.

To avoid this, we have some choices but any ideas are not perfect...
 1) IMUCTR.FLUSH of the connected device is set after the device stopped.
  - For now, IPMMU driver cannot know the dev pointer in map/unmap timing.
    So, we cannot implement such a code.
 2) IMCTR.FLUSH is set after all devices are unmapped on the domain.

 - In any cases, before DMA API's "unmap" calling, the device should be stopped.
   However, some device drivers (e.g. USB EHCI) don't do so.
 - If we have a special API to set the register and call the API on each
   device drivers, maybe we can avoid the issue. But, I think it is not
   acceptable in upstream...

Anyway, this patch set has the idea 2) above to avoid the issue.
But, I'm not sure, this is good or not. So, I marked this patch set as "RFC".

Magnus Damm (2):
  iommu: dma: track mapped iova
  iommu: dma: iommu iova domain reset

Yoshihiro Shimoda (2):
  iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in
    init_iova_rcaches()
  iommu: ipmmu-vmsa: enable force_reset_when_empty

 drivers/iommu/dma-iommu.c  | 61 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/iommu/iova.c       | 13 +++++++++-
 drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++--
 include/linux/iommu.h      |  4 +++
 include/linux/iova.h       |  1 +
 5 files changed, 86 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH/RFC 0/4] iommu: add workaround for R-Car Gen3 IPMMU

[Yoshihiro Shimoda]

This patch set is based on:
 iommu.git / next branch and the following patch that Magnus-san sent:
 [patch v6 00/07] iommu/ipmmu-vmsa: ipmmu multi-arch update v6

R-Car Gen3 IPMMU has an issue that will mistake an address translation
if IMCTR.FLUSH is set while some related devices that on the same domain
are running.

To avoid this, we have some choices but any ideas are not perfect...
 1) IMUCTR.FLUSH of the connected device is set after the device stopped.
  - For now, IPMMU driver cannot know the dev pointer in map/unmap timing.
    So, we cannot implement such a code.
 2) IMCTR.FLUSH is set after all devices are unmapped on the domain.

 - In any cases, before DMA API's "unmap" calling, the device should be stopped.
   However, some device drivers (e.g. USB EHCI) don't do so.
 - If we have a special API to set the register and call the API on each
   device drivers, maybe we can avoid the issue. But, I think it is not
   acceptable in upstream...

Anyway, this patch set has the idea 2) above to avoid the issue.
But, I'm not sure, this is good or not. So, I marked this patch set as "RFC".

Magnus Damm (2):
  iommu: dma: track mapped iova
  iommu: dma: iommu iova domain reset

Yoshihiro Shimoda (2):
  iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in
    init_iova_rcaches()
  iommu: ipmmu-vmsa: enable force_reset_when_empty

 drivers/iommu/dma-iommu.c  | 61 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/iommu/iova.c       | 13 +++++++++-
 drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++--
 include/linux/iommu.h      |  4 +++
 include/linux/iova.h       |  1 +
 5 files changed, 86 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH/RFC 1/4] iommu: dma: track mapped iova

[Yoshihiro Shimoda]

From: Magnus Damm <damm+renesas@opensource.se>

To track mapped iova for a workaround code in the future.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++------
 include/linux/iommu.h     |  2 ++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d64..a0b8c0f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/dma-iommu.h>
 #include <linux/gfp.h>
@@ -147,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	order = __ffs(domain->pgsize_bitmap);
 	base_pfn = max_t(unsigned long, 1, base >> order);
 	end_pfn = (base + size - 1) >> order;
+	atomic_set(&domain->iova_pfns_mapped, 0);
 
 	/* Check the domain allows at least some access to the device... */
 	if (domain->geometry.force_aperture) {
@@ -209,6 +211,7 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	struct iova_domain *iovad = cookie_iovad(domain);
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
+	struct iova *iova;
 
 	if (domain->geometry.force_aperture)
 		dma_limit = min(dma_limit, domain->geometry.aperture_end);
@@ -216,9 +219,23 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
 	 */
-	return alloc_iova(iovad, length, dma_limit >> shift, true);
+	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+	if (iova)
+		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+
+	return iova;
 }
 
+void
+__free_iova_domain(struct iommu_domain *domain, struct iova *iova)
+{
+	struct iova_domain *iovad = cookie_iovad(domain);
+
+	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
+	__free_iova(iovad, iova);
+}
+
+
 /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
 static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
 {
@@ -235,7 +252,7 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
 	size -= iommu_unmap(domain, pfn << shift, size);
 	/* ...and if we can't, then something is horribly, horribly wrong */
 	WARN_ON(size > 0);
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 }
 
 static void __iommu_dma_free_pages(struct page **pages, int count)
@@ -401,7 +418,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
 out_free_sg:
 	sg_free_table(&sgt);
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_free_pages:
 	__iommu_dma_free_pages(pages, count);
 	return NULL;
@@ -447,7 +464,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 
 	dma_addr = iova_dma_addr(iovad, iova);
 	if (iommu_map(domain, dma_addr, phys - iova_off, len, prot)) {
-		__free_iova(iovad, iova);
+		__free_iova_domain(domain, iova);
 		return DMA_ERROR_CODE;
 	}
 	return dma_addr + iova_off;
@@ -613,7 +630,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	return __finalise_sg(dev, sg, nents, dma_addr);
 
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_restore_sg:
 	__invalidate_sg(sg, nents);
 	return 0;
@@ -689,7 +706,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	return msi_page;
 
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_free_page:
 	kfree(msi_page);
 	return NULL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..91d0159 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -19,6 +19,7 @@
 #ifndef __LINUX_IOMMU_H
 #define __LINUX_IOMMU_H
 
+#include <linux/atomic.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/of.h>
@@ -84,6 +85,7 @@ struct iommu_domain {
 	void *handler_token;
 	struct iommu_domain_geometry geometry;
 	void *iova_cookie;
+	atomic_t iova_pfns_mapped;
 };
 
 enum iommu_cap {
-- 
1.9.1

[PATCH/RFC 1/4] iommu: dma: track mapped iova

[Yoshihiro Shimoda]

From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>

To track mapped iova for a workaround code in the future.

Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++------
 include/linux/iommu.h     |  2 ++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d64..a0b8c0f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/dma-iommu.h>
 #include <linux/gfp.h>
@@ -147,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	order = __ffs(domain->pgsize_bitmap);
 	base_pfn = max_t(unsigned long, 1, base >> order);
 	end_pfn = (base + size - 1) >> order;
+	atomic_set(&domain->iova_pfns_mapped, 0);
 
 	/* Check the domain allows at least some access to the device... */
 	if (domain->geometry.force_aperture) {
@@ -209,6 +211,7 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	struct iova_domain *iovad = cookie_iovad(domain);
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
+	struct iova *iova;
 
 	if (domain->geometry.force_aperture)
 		dma_limit = min(dma_limit, domain->geometry.aperture_end);
@@ -216,9 +219,23 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
 	 */
-	return alloc_iova(iovad, length, dma_limit >> shift, true);
+	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+	if (iova)
+		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+
+	return iova;
 }
 
+void
+__free_iova_domain(struct iommu_domain *domain, struct iova *iova)
+{
+	struct iova_domain *iovad = cookie_iovad(domain);
+
+	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
+	__free_iova(iovad, iova);
+}
+
+
 /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
 static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
 {
@@ -235,7 +252,7 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
 	size -= iommu_unmap(domain, pfn << shift, size);
 	/* ...and if we can't, then something is horribly, horribly wrong */
 	WARN_ON(size > 0);
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 }
 
 static void __iommu_dma_free_pages(struct page **pages, int count)
@@ -401,7 +418,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
 out_free_sg:
 	sg_free_table(&sgt);
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_free_pages:
 	__iommu_dma_free_pages(pages, count);
 	return NULL;
@@ -447,7 +464,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 
 	dma_addr = iova_dma_addr(iovad, iova);
 	if (iommu_map(domain, dma_addr, phys - iova_off, len, prot)) {
-		__free_iova(iovad, iova);
+		__free_iova_domain(domain, iova);
 		return DMA_ERROR_CODE;
 	}
 	return dma_addr + iova_off;
@@ -613,7 +630,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	return __finalise_sg(dev, sg, nents, dma_addr);
 
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_restore_sg:
 	__invalidate_sg(sg, nents);
 	return 0;
@@ -689,7 +706,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	return msi_page;
 
 out_free_iova:
-	__free_iova(iovad, iova);
+	__free_iova_domain(domain, iova);
 out_free_page:
 	kfree(msi_page);
 	return NULL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..91d0159 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -19,6 +19,7 @@
 #ifndef __LINUX_IOMMU_H
 #define __LINUX_IOMMU_H
 
+#include <linux/atomic.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/of.h>
@@ -84,6 +85,7 @@ struct iommu_domain {
 	void *handler_token;
 	struct iommu_domain_geometry geometry;
 	void *iova_cookie;
+	atomic_t iova_pfns_mapped;
 };
 
 enum iommu_cap {
-- 
1.9.1

[PATCH/RFC 2/4] iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in init_iova_rcaches()

[Yoshihiro Shimoda]

In the future, the init_iova_rcaches will be called in atomic.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/iova.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b7268a1..866ad65 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -723,7 +723,9 @@ static void init_iova_rcaches(struct iova_domain *iovad)
 		rcache = &iovad->rcaches[i];
 		spin_lock_init(&rcache->lock);
 		rcache->depot_size = 0;
-		rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());
+		rcache->cpu_rcaches = __alloc_percpu_gfp(sizeof(*cpu_rcache),
+							 cache_line_size(),
+							 GFP_NOWAIT);
 		if (WARN_ON(!rcache->cpu_rcaches))
 			continue;
 		for_each_possible_cpu(cpu) {
-- 
1.9.1

[PATCH/RFC 2/4] iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in init_iova_rcaches()

[Yoshihiro Shimoda]

In the future, the init_iova_rcaches will be called in atomic.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/iova.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b7268a1..866ad65 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -723,7 +723,9 @@ static void init_iova_rcaches(struct iova_domain *iovad)
 		rcache = &iovad->rcaches[i];
 		spin_lock_init(&rcache->lock);
 		rcache->depot_size = 0;
-		rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());
+		rcache->cpu_rcaches = __alloc_percpu_gfp(sizeof(*cpu_rcache),
+							 cache_line_size(),
+							 GFP_NOWAIT);
 		if (WARN_ON(!rcache->cpu_rcaches))
 			continue;
 		for_each_possible_cpu(cpu) {
-- 
1.9.1

[PATCH/RFC 3/4] iommu: dma: iommu iova domain reset

[Yoshihiro Shimoda]

From: Magnus Damm <damm+renesas@opensource.se>

To add a workaround code for ipmmu-vmsa driver, this patch adds
a new geometry "force_reset_when_empty" not to reuse iova space.
When all pfns happen to get unmapped then ask the IOMMU driver to
flush the state followed by starting from an empty iova space.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/dma-iommu.c | 42 +++++++++++++++++++++++++++++++++++++-----
 drivers/iommu/iova.c      |  9 +++++++++
 include/linux/iommu.h     |  2 ++
 include/linux/iova.h      |  1 +
 4 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index a0b8c0f..d0fa0b1 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -42,6 +42,7 @@ struct iommu_dma_cookie {
 	struct iova_domain	iovad;
 	struct list_head	msi_page_list;
 	spinlock_t		msi_lock;
+	spinlock_t		reset_lock;
 };
 
 static inline struct iova_domain *cookie_iovad(struct iommu_domain *domain)
@@ -74,6 +75,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
 
 	spin_lock_init(&cookie->msi_lock);
 	INIT_LIST_HEAD(&cookie->msi_page_list);
+	spin_lock_init(&cookie->reset_lock);
 	domain->iova_cookie = cookie;
 	return 0;
 }
@@ -208,9 +210,11 @@ int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
 static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 		dma_addr_t dma_limit)
 {
+	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = cookie_iovad(domain);
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
+	unsigned long flags;
 	struct iova *iova;
 
 	if (domain->geometry.force_aperture)
@@ -219,9 +223,19 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
 	 */
-	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
-	if (iova)
-		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+	if (domain->geometry.force_reset_when_empty) {
+		spin_lock_irqsave(&cookie->reset_lock, flags);
+
+		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+		if (iova)
+			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+
+		spin_unlock_irqrestore(&cookie->reset_lock, flags);
+	} else {
+		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+		if (iova)
+			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+	}
 
 	return iova;
 }
@@ -229,10 +243,28 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 void
 __free_iova_domain(struct iommu_domain *domain, struct iova *iova)
 {
+	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = cookie_iovad(domain);
+	unsigned long flags;
 
-	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
-	__free_iova(iovad, iova);
+	/* In case force_reset_when_empty is set, do not reuse iova space
+	 * but instead simply keep on expanding seemingly forever.
+	 * When all pfns happen to get unmapped then ask the IOMMU driver to
+	 * flush the state followed by starting from an empty iova space.
+	 */
+	if (domain->geometry.force_reset_when_empty) {
+		spin_lock_irqsave(&cookie->reset_lock, flags);
+		if (atomic_sub_return(iova_size(iova),
+				      &domain->iova_pfns_mapped) == 0) {
+			reset_iova_domain(iovad);
+			if (domain->ops->domain_reset)
+				domain->ops->domain_reset(domain);
+		}
+		spin_unlock_irqrestore(&cookie->reset_lock, flags);
+	} else {
+		atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
+		__free_iova(iovad, iova);
+	}
 }
 
 
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 866ad65..50aaa46 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -464,6 +464,7 @@ void put_iova_domain(struct iova_domain *iovad)
 	while (node) {
 		struct iova *iova = rb_entry(node, struct iova, node);
 
+		__cached_rbnode_delete_update(iovad, iova);
 		rb_erase(node, &iovad->rbroot);
 		free_iova_mem(iova);
 		node = rb_first(&iovad->rbroot);
@@ -472,6 +473,14 @@ void put_iova_domain(struct iova_domain *iovad)
 }
 EXPORT_SYMBOL_GPL(put_iova_domain);
 
+void
+reset_iova_domain(struct iova_domain *iovad)
+{
+	put_iova_domain(iovad);
+	init_iova_rcaches(iovad);
+}
+EXPORT_SYMBOL_GPL(reset_iova_domain);
+
 static int
 __is_range_overlap(struct rb_node *node,
 	unsigned long pfn_lo, unsigned long pfn_hi)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 91d0159..bd9ba1b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -51,6 +51,7 @@ struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */
 	dma_addr_t aperture_end;   /* Last address that can be mapped     */
 	bool force_aperture;       /* DMA only allowed in mappable range? */
+	bool force_reset_when_empty;
 };
 
 /* Domain feature flags */
@@ -168,6 +169,7 @@ struct iommu_ops {
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
 	void (*domain_free)(struct iommu_domain *);
+	void (*domain_reset)(struct iommu_domain *);
 
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
diff --git a/include/linux/iova.h b/include/linux/iova.h
index f27bb2c..31c8496 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -103,6 +103,7 @@ void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn, unsigned long pfn_32bit);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
+void reset_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
 	struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
 void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
-- 
1.9.1

[PATCH/RFC 3/4] iommu: dma: iommu iova domain reset

[Yoshihiro Shimoda]

From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>

To add a workaround code for ipmmu-vmsa driver, this patch adds
a new geometry "force_reset_when_empty" not to reuse iova space.
When all pfns happen to get unmapped then ask the IOMMU driver to
flush the state followed by starting from an empty iova space.

Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/dma-iommu.c | 42 +++++++++++++++++++++++++++++++++++++-----
 drivers/iommu/iova.c      |  9 +++++++++
 include/linux/iommu.h     |  2 ++
 include/linux/iova.h      |  1 +
 4 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index a0b8c0f..d0fa0b1 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -42,6 +42,7 @@ struct iommu_dma_cookie {
 	struct iova_domain	iovad;
 	struct list_head	msi_page_list;
 	spinlock_t		msi_lock;
+	spinlock_t		reset_lock;
 };
 
 static inline struct iova_domain *cookie_iovad(struct iommu_domain *domain)
@@ -74,6 +75,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
 
 	spin_lock_init(&cookie->msi_lock);
 	INIT_LIST_HEAD(&cookie->msi_page_list);
+	spin_lock_init(&cookie->reset_lock);
 	domain->iova_cookie = cookie;
 	return 0;
 }
@@ -208,9 +210,11 @@ int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
 static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 		dma_addr_t dma_limit)
 {
+	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = cookie_iovad(domain);
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
+	unsigned long flags;
 	struct iova *iova;
 
 	if (domain->geometry.force_aperture)
@@ -219,9 +223,19 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
 	 */
-	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
-	if (iova)
-		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+	if (domain->geometry.force_reset_when_empty) {
+		spin_lock_irqsave(&cookie->reset_lock, flags);
+
+		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+		if (iova)
+			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+
+		spin_unlock_irqrestore(&cookie->reset_lock, flags);
+	} else {
+		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
+		if (iova)
+			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
+	}
 
 	return iova;
 }
@@ -229,10 +243,28 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 void
 __free_iova_domain(struct iommu_domain *domain, struct iova *iova)
 {
+	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = cookie_iovad(domain);
+	unsigned long flags;
 
-	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
-	__free_iova(iovad, iova);
+	/* In case force_reset_when_empty is set, do not reuse iova space
+	 * but instead simply keep on expanding seemingly forever.
+	 * When all pfns happen to get unmapped then ask the IOMMU driver to
+	 * flush the state followed by starting from an empty iova space.
+	 */
+	if (domain->geometry.force_reset_when_empty) {
+		spin_lock_irqsave(&cookie->reset_lock, flags);
+		if (atomic_sub_return(iova_size(iova),
+				      &domain->iova_pfns_mapped) == 0) {
+			reset_iova_domain(iovad);
+			if (domain->ops->domain_reset)
+				domain->ops->domain_reset(domain);
+		}
+		spin_unlock_irqrestore(&cookie->reset_lock, flags);
+	} else {
+		atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
+		__free_iova(iovad, iova);
+	}
 }
 
 
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 866ad65..50aaa46 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -464,6 +464,7 @@ void put_iova_domain(struct iova_domain *iovad)
 	while (node) {
 		struct iova *iova = rb_entry(node, struct iova, node);
 
+		__cached_rbnode_delete_update(iovad, iova);
 		rb_erase(node, &iovad->rbroot);
 		free_iova_mem(iova);
 		node = rb_first(&iovad->rbroot);
@@ -472,6 +473,14 @@ void put_iova_domain(struct iova_domain *iovad)
 }
 EXPORT_SYMBOL_GPL(put_iova_domain);
 
+void
+reset_iova_domain(struct iova_domain *iovad)
+{
+	put_iova_domain(iovad);
+	init_iova_rcaches(iovad);
+}
+EXPORT_SYMBOL_GPL(reset_iova_domain);
+
 static int
 __is_range_overlap(struct rb_node *node,
 	unsigned long pfn_lo, unsigned long pfn_hi)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 91d0159..bd9ba1b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -51,6 +51,7 @@ struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */
 	dma_addr_t aperture_end;   /* Last address that can be mapped     */
 	bool force_aperture;       /* DMA only allowed in mappable range? */
+	bool force_reset_when_empty;
 };
 
 /* Domain feature flags */
@@ -168,6 +169,7 @@ struct iommu_ops {
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
 	void (*domain_free)(struct iommu_domain *);
+	void (*domain_reset)(struct iommu_domain *);
 
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
diff --git a/include/linux/iova.h b/include/linux/iova.h
index f27bb2c..31c8496 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -103,6 +103,7 @@ void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn, unsigned long pfn_32bit);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
+void reset_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
 	struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
 void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
-- 
1.9.1

[PATCH/RFC 4/4] iommu: ipmmu-vmsa: enable force_reset_when_empty

[Yoshihiro Shimoda]

The IPMMU of R-Car Gen3 will mistake an address translation if
IMCTR.FLUSH is set while some related devices that on the same doamin
are running. To avoid this, this patch uses the force_reset_when_empty
feature.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 11550ac..4b62969 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -302,7 +302,8 @@ static void ipmmu_tlb_flush_all(void *cookie)
 {
 	struct ipmmu_vmsa_domain *domain = cookie;
 
-	ipmmu_tlb_invalidate(domain);
+	if (!domain->io_domain.geometry.force_reset_when_empty)
+		ipmmu_tlb_invalidate(domain);
 }
 
 static void ipmmu_tlb_add_flush(unsigned long iova, size_t size,
@@ -555,6 +556,13 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
 	kfree(domain);
 }
 
+static void ipmmu_domain_reset(struct iommu_domain *io_domain)
+{
+	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
+
+	ipmmu_tlb_invalidate(domain);
+}
+
 static int ipmmu_attach_device(struct iommu_domain *io_domain,
 			       struct device *dev)
 {
@@ -832,6 +840,7 @@ static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.domain_free = ipmmu_domain_free,
+	.domain_reset = ipmmu_domain_reset,
 	.attach_dev = ipmmu_attach_device,
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
@@ -858,8 +867,10 @@ static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
 
 	case IOMMU_DOMAIN_DMA:
 		io_domain = __ipmmu_domain_alloc(type);
-		if (io_domain)
+		if (io_domain) {
 			iommu_get_dma_cookie(io_domain);
+			io_domain->geometry.force_reset_when_empty = true;
+		}
 		break;
 	}
 
@@ -927,6 +938,7 @@ static int ipmmu_of_xlate_dma(struct device *dev,
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc_dma,
 	.domain_free = ipmmu_domain_free_dma,
+	.domain_reset = ipmmu_domain_reset,
 	.attach_dev = ipmmu_attach_device,
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
-- 
1.9.1

[PATCH/RFC 4/4] iommu: ipmmu-vmsa: enable force_reset_when_empty

[Yoshihiro Shimoda]

The IPMMU of R-Car Gen3 will mistake an address translation if
IMCTR.FLUSH is set while some related devices that on the same doamin
are running. To avoid this, this patch uses the force_reset_when_empty
feature.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 11550ac..4b62969 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -302,7 +302,8 @@ static void ipmmu_tlb_flush_all(void *cookie)
 {
 	struct ipmmu_vmsa_domain *domain = cookie;
 
-	ipmmu_tlb_invalidate(domain);
+	if (!domain->io_domain.geometry.force_reset_when_empty)
+		ipmmu_tlb_invalidate(domain);
 }
 
 static void ipmmu_tlb_add_flush(unsigned long iova, size_t size,
@@ -555,6 +556,13 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
 	kfree(domain);
 }
 
+static void ipmmu_domain_reset(struct iommu_domain *io_domain)
+{
+	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
+
+	ipmmu_tlb_invalidate(domain);
+}
+
 static int ipmmu_attach_device(struct iommu_domain *io_domain,
 			       struct device *dev)
 {
@@ -832,6 +840,7 @@ static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.domain_free = ipmmu_domain_free,
+	.domain_reset = ipmmu_domain_reset,
 	.attach_dev = ipmmu_attach_device,
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
@@ -858,8 +867,10 @@ static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
 
 	case IOMMU_DOMAIN_DMA:
 		io_domain = __ipmmu_domain_alloc(type);
-		if (io_domain)
+		if (io_domain) {
 			iommu_get_dma_cookie(io_domain);
+			io_domain->geometry.force_reset_when_empty = true;
+		}
 		break;
 	}
 
@@ -927,6 +938,7 @@ static int ipmmu_of_xlate_dma(struct device *dev,
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc_dma,
 	.domain_free = ipmmu_domain_free_dma,
+	.domain_reset = ipmmu_domain_reset,
 	.attach_dev = ipmmu_attach_device,
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
-- 
1.9.1

Re: [PATCH/RFC 1/4] iommu: dma: track mapped iova

[Robin Murphy]

On 25/01/17 12:53, Yoshihiro Shimoda wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> To track mapped iova for a workaround code in the future.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++------
>  include/linux/iommu.h     |  2 ++

So what's being added here is a counter of allocations within the
iova_domain held by an iommu_dma_cookie? Then why is it all the way down
in the iommu_domain and not in the cookie? That's needlessly invasive -
it would be almost understandable (but still horrible) if you needed to
refer to it directly from the IOMMU driver, but as far as I can see you
don't.

Robin.

>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 2db0d64..a0b8c0f 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -19,6 +19,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/device.h>
>  #include <linux/dma-iommu.h>
>  #include <linux/gfp.h>
> @@ -147,6 +148,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>  	order = __ffs(domain->pgsize_bitmap);
>  	base_pfn = max_t(unsigned long, 1, base >> order);
>  	end_pfn = (base + size - 1) >> order;
> +	atomic_set(&domain->iova_pfns_mapped, 0);
>  
>  	/* Check the domain allows at least some access to the device... */
>  	if (domain->geometry.force_aperture) {
> @@ -209,6 +211,7 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  	struct iova_domain *iovad = cookie_iovad(domain);
>  	unsigned long shift = iova_shift(iovad);
>  	unsigned long length = iova_align(iovad, size) >> shift;
> +	struct iova *iova;
>  
>  	if (domain->geometry.force_aperture)
>  		dma_limit = min(dma_limit, domain->geometry.aperture_end);
> @@ -216,9 +219,23 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  	 * Enforce size-alignment to be safe - there could perhaps be an
>  	 * attribute to control this per-device, or at least per-domain...
>  	 */
> -	return alloc_iova(iovad, length, dma_limit >> shift, true);
> +	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
> +	if (iova)
> +		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
> +
> +	return iova;
>  }
>  
> +void
> +__free_iova_domain(struct iommu_domain *domain, struct iova *iova)
> +{
> +	struct iova_domain *iovad = cookie_iovad(domain);
> +
> +	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
> +	__free_iova(iovad, iova);
> +}
> +
> +
>  /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
>  static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
>  {
> @@ -235,7 +252,7 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr)
>  	size -= iommu_unmap(domain, pfn << shift, size);
>  	/* ...and if we can't, then something is horribly, horribly wrong */
>  	WARN_ON(size > 0);
> -	__free_iova(iovad, iova);
> +	__free_iova_domain(domain, iova);
>  }
>  
>  static void __iommu_dma_free_pages(struct page **pages, int count)
> @@ -401,7 +418,7 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
>  out_free_sg:
>  	sg_free_table(&sgt);
>  out_free_iova:
> -	__free_iova(iovad, iova);
> +	__free_iova_domain(domain, iova);
>  out_free_pages:
>  	__iommu_dma_free_pages(pages, count);
>  	return NULL;
> @@ -447,7 +464,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
>  
>  	dma_addr = iova_dma_addr(iovad, iova);
>  	if (iommu_map(domain, dma_addr, phys - iova_off, len, prot)) {
> -		__free_iova(iovad, iova);
> +		__free_iova_domain(domain, iova);
>  		return DMA_ERROR_CODE;
>  	}
>  	return dma_addr + iova_off;
> @@ -613,7 +630,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>  	return __finalise_sg(dev, sg, nents, dma_addr);
>  
>  out_free_iova:
> -	__free_iova(iovad, iova);
> +	__free_iova_domain(domain, iova);
>  out_restore_sg:
>  	__invalidate_sg(sg, nents);
>  	return 0;
> @@ -689,7 +706,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>  	return msi_page;
>  
>  out_free_iova:
> -	__free_iova(iovad, iova);
> +	__free_iova_domain(domain, iova);
>  out_free_page:
>  	kfree(msi_page);
>  	return NULL;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0ff5111..91d0159 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -19,6 +19,7 @@
>  #ifndef __LINUX_IOMMU_H
>  #define __LINUX_IOMMU_H
>  
> +#include <linux/atomic.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/of.h>
> @@ -84,6 +85,7 @@ struct iommu_domain {
>  	void *handler_token;
>  	struct iommu_domain_geometry geometry;
>  	void *iova_cookie;
> +	atomic_t iova_pfns_mapped;
>  };
>  
>  enum iommu_cap {
> 

Re: [PATCH/RFC 2/4] iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in init_iova_rcaches()

[Robin Murphy]

On 25/01/17 12:54, Yoshihiro Shimoda wrote:
> In the future, the init_iova_rcaches will be called in atomic.

That screams "doing the wrong thing". The sole point of the rcaches is
to reuse IOVAs, whereas the main point of this series seems to involve
not reusing IOVAs. The fact that we have to affect the allocation of
something we explicitly want to avoid using, rather than, say, not
allocating it at all, is a bit of a red flag.

Also, the rcaches are rather big. Allocating all this lot in atomic
context is not necessarily going to be the best idea.

Robin.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/iommu/iova.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index b7268a1..866ad65 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -723,7 +723,9 @@ static void init_iova_rcaches(struct iova_domain *iovad)
>  		rcache = &iovad->rcaches[i];
>  		spin_lock_init(&rcache->lock);
>  		rcache->depot_size = 0;
> -		rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());
> +		rcache->cpu_rcaches = __alloc_percpu_gfp(sizeof(*cpu_rcache),
> +							 cache_line_size(),
> +							 GFP_NOWAIT);
>  		if (WARN_ON(!rcache->cpu_rcaches))
>  			continue;
>  		for_each_possible_cpu(cpu) {
> 

Re: [PATCH/RFC 2/4] iommu: iova: use __alloc_percpu_gfp() with GFP_NOWAIT in init_iova_rcaches()

[Robin Murphy]

On 25/01/17 12:54, Yoshihiro Shimoda wrote:
> In the future, the init_iova_rcaches will be called in atomic.

That screams "doing the wrong thing". The sole point of the rcaches is
to reuse IOVAs, whereas the main point of this series seems to involve
not reusing IOVAs. The fact that we have to affect the allocation of
something we explicitly want to avoid using, rather than, say, not
allocating it at all, is a bit of a red flag.

Also, the rcaches are rather big. Allocating all this lot in atomic
context is not necessarily going to be the best idea.

Robin.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/iommu/iova.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index b7268a1..866ad65 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -723,7 +723,9 @@ static void init_iova_rcaches(struct iova_domain *iovad)
>  		rcache = &iovad->rcaches[i];
>  		spin_lock_init(&rcache->lock);
>  		rcache->depot_size = 0;
> -		rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());
> +		rcache->cpu_rcaches = __alloc_percpu_gfp(sizeof(*cpu_rcache),
> +							 cache_line_size(),
> +							 GFP_NOWAIT);
>  		if (WARN_ON(!rcache->cpu_rcaches))
>  			continue;
>  		for_each_possible_cpu(cpu) {
> 

Re: [PATCH/RFC 3/4] iommu: dma: iommu iova domain reset

[Robin Murphy]

On 25/01/17 12:54, Yoshihiro Shimoda wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> To add a workaround code for ipmmu-vmsa driver, this patch adds
> a new geometry "force_reset_when_empty" not to reuse iova space.

The domain geometry is absolutely not the appropriate place for that. If
anything, it could possibly be a domain attribute, but it has nothing to
do with describing the range of input addresses the hardware is able to
translate.

> When all pfns happen to get unmapped then ask the IOMMU driver to
> flush the state followed by starting from an empty iova space.

And what happens if all the PFNs are never unmapped? Many devices (USB
being among them) use a coherent allocation for some kind of descriptor
buffer which exists for the lifetime of the device, then use streaming
mappings for data transfers - the net result of that is that the number
of PFNs mapped will always be >=1, and eventually streaming mapping will
fail because you've exhausted the address space. And if the device *is*
a USB controller, at that point the thread will hang because the USB
core's response to a DMA mapping failure happens to be "keep trying
indefinitely".

Essentially, however long this allocation workaround postpones it, one
or other failure mode is unavoidable with certain devices unless you can
do something drastic like periodically suspend and resume the entire
system to reset everything.

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/iommu/dma-iommu.c | 42 +++++++++++++++++++++++++++++++++++++-----
>  drivers/iommu/iova.c      |  9 +++++++++
>  include/linux/iommu.h     |  2 ++
>  include/linux/iova.h      |  1 +
>  4 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index a0b8c0f..d0fa0b1 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -42,6 +42,7 @@ struct iommu_dma_cookie {
>  	struct iova_domain	iovad;
>  	struct list_head	msi_page_list;
>  	spinlock_t		msi_lock;
> +	spinlock_t		reset_lock;

So now we do get something in the cookie, but it's protecting a bunch of
machinery that's accessible from a wider scope? That doesn't seem like a
good design.

>  };
>  
>  static inline struct iova_domain *cookie_iovad(struct iommu_domain *domain)
> @@ -74,6 +75,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>  
>  	spin_lock_init(&cookie->msi_lock);
>  	INIT_LIST_HEAD(&cookie->msi_page_list);
> +	spin_lock_init(&cookie->reset_lock);
>  	domain->iova_cookie = cookie;
>  	return 0;
>  }
> @@ -208,9 +210,11 @@ int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
>  static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  		dma_addr_t dma_limit)
>  {
> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iova_domain *iovad = cookie_iovad(domain);
>  	unsigned long shift = iova_shift(iovad);
>  	unsigned long length = iova_align(iovad, size) >> shift;
> +	unsigned long flags;
>  	struct iova *iova;
>  
>  	if (domain->geometry.force_aperture)
> @@ -219,9 +223,19 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  	 * Enforce size-alignment to be safe - there could perhaps be an
>  	 * attribute to control this per-device, or at least per-domain...
>  	 */
> -	iova = alloc_iova(iovad, length, dma_limit >> shift, true);
> -	if (iova)
> -		atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
> +	if (domain->geometry.force_reset_when_empty) {
> +		spin_lock_irqsave(&cookie->reset_lock, flags);

Is this locking definitely safe against the potential concurrent callers
of __free_iova() on the same domain who won't touch reset_lock? (It may
well be, it's just not clear)

> +
> +		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
> +		if (iova)
> +			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
> +
> +		spin_unlock_irqrestore(&cookie->reset_lock, flags);
> +	} else {
> +		iova = alloc_iova(iovad, length, dma_limit >> shift, true);
> +		if (iova)
> +			atomic_add(iova_size(iova), &domain->iova_pfns_mapped);

Why would we even need to keep track of this on non-broken IOMMUS?

> +	}
>  
>  	return iova;
>  }
> @@ -229,10 +243,28 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  void
>  __free_iova_domain(struct iommu_domain *domain, struct iova *iova)
>  {
> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iova_domain *iovad = cookie_iovad(domain);
> +	unsigned long flags;
>  
> -	atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
> -	__free_iova(iovad, iova);
> +	/* In case force_reset_when_empty is set, do not reuse iova space
> +	 * but instead simply keep on expanding seemingly forever.
> +	 * When all pfns happen to get unmapped then ask the IOMMU driver to
> +	 * flush the state followed by starting from an empty iova space.
> +	 */
> +	if (domain->geometry.force_reset_when_empty) {
> +		spin_lock_irqsave(&cookie->reset_lock, flags);
> +		if (atomic_sub_return(iova_size(iova),
> +				      &domain->iova_pfns_mapped) == 0) {
> +			reset_iova_domain(iovad);
> +			if (domain->ops->domain_reset)
> +				domain->ops->domain_reset(domain);
> +		}
> +		spin_unlock_irqrestore(&cookie->reset_lock, flags);
> +	} else {
> +		atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
> +		__free_iova(iovad, iova);
> +	}
>  }
>  
>  
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 866ad65..50aaa46 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -464,6 +464,7 @@ void put_iova_domain(struct iova_domain *iovad)
>  	while (node) {
>  		struct iova *iova = rb_entry(node, struct iova, node);
>  
> +		__cached_rbnode_delete_update(iovad, iova);
>  		rb_erase(node, &iovad->rbroot);
>  		free_iova_mem(iova);
>  		node = rb_first(&iovad->rbroot);
> @@ -472,6 +473,14 @@ void put_iova_domain(struct iova_domain *iovad)
>  }
>  EXPORT_SYMBOL_GPL(put_iova_domain);
>  
> +void
> +reset_iova_domain(struct iova_domain *iovad)
> +{
> +	put_iova_domain(iovad);
> +	init_iova_rcaches(iovad);

And we have to nuke the entire domain rather than simply freeing all the
IOVAs because...?

In summary, this is really ugly. If you must implement this workaround,
it would be infinitely preferable to use a more appropriate allocator in
the first place, at which point you then need none of the invasive
hacks. I've already proposed support for multiple allocator types for
the sake of MSI pages[1], and it's not all that complicated to move the
abstraction further up into the alloc and free helpers themselves so it
applies everywhere (I have some work in progress to convert alloc/free
to be PFN-based, which effectively requires most of the same changes, so
they're going to get done either way at some point). With that,
implementing a one-way wraparound allocator as a third type would be
pretty simple - the IPMMU driver could request that somehow upon getting
its cookie, then simply skip TLB syncs on unmaps internally (and maybe
still refcount maps vs. unmaps there if you think there's any chance of
stale TLB entries lasting long enough to be problematic).

I'd still be inclined to simply not pretend to support managed DMA
domains on this IOMMU if at all possible, though.

Robin.

[1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1314675.html

> +}
> +EXPORT_SYMBOL_GPL(reset_iova_domain);
> +
>  static int
>  __is_range_overlap(struct rb_node *node,
>  	unsigned long pfn_lo, unsigned long pfn_hi)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 91d0159..bd9ba1b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -51,6 +51,7 @@ struct iommu_domain_geometry {
>  	dma_addr_t aperture_start; /* First address that can be mapped    */
>  	dma_addr_t aperture_end;   /* Last address that can be mapped     */
>  	bool force_aperture;       /* DMA only allowed in mappable range? */
> +	bool force_reset_when_empty;
>  };
>  
>  /* Domain feature flags */
> @@ -168,6 +169,7 @@ struct iommu_ops {
>  	/* Domain allocation and freeing by the iommu driver */
>  	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
>  	void (*domain_free)(struct iommu_domain *);
> +	void (*domain_reset)(struct iommu_domain *);
>  
>  	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>  	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
> diff --git a/include/linux/iova.h b/include/linux/iova.h
> index f27bb2c..31c8496 100644
> --- a/include/linux/iova.h
> +++ b/include/linux/iova.h
> @@ -103,6 +103,7 @@ void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>  	unsigned long start_pfn, unsigned long pfn_32bit);
>  struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>  void put_iova_domain(struct iova_domain *iovad);
> +void reset_iova_domain(struct iova_domain *iovad);
>  struct iova *split_and_remove_iova(struct iova_domain *iovad,
>  	struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
>  void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
> 

Re: [PATCH/RFC 1/4] iommu: dma: track mapped iova

[Magnus Damm]

Hi Robin, Shimoda-san, everyone,

On Thu, Jan 26, 2017 at 1:27 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 25/01/17 12:53, Yoshihiro Shimoda wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> To track mapped iova for a workaround code in the future.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>  drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++------
>>  include/linux/iommu.h     |  2 ++
>
> So what's being added here is a counter of allocations within the
> iova_domain held by an iommu_dma_cookie? Then why is it all the way down
> in the iommu_domain and not in the cookie? That's needlessly invasive -
> it would be almost understandable (but still horrible) if you needed to
> refer to it directly from the IOMMU driver, but as far as I can see you
> don't.

You are very correct about all points. =)

As you noticed, the counter is kept per-domain. History is a bit
blurry but I think the reason for my abstraction-breaking is that i
decided to implement the simplest possible code to get the reset
callback to near the iova code and the domain level seemed suitable as
a first step. Moving it to a different level is of course no problem.

My main concern for this series is however if a subsystem-level
intrusive workaround like this ever could be beaten into acceptable
form for upstream merge.The code is pretty simple - the main portion
of the workaround is this counter that used to detect when the number
of mapped pages drops back to zero together with the callback. But
unless the code can be made pluggable it introduces overhead for all
users.

In your other email you asked what happens if the counter never drops
to zero. You are right that this workaround is not a general-purpose
fix suitable for all kinds of devices. Because of that the workaround
is designed to be enabled on only some of the IPMMU instances on the
system. It is also most likely an errata workaround for a particular
ES version, so we would most likely have to enable it with
soc_device_match().

I'll reply to patch 3/4 with some more details.

Thanks!

/ magnus

Re: [PATCH/RFC 3/4] iommu: dma: iommu iova domain reset

[Magnus Damm]

Hi Robin, Shimoda-san, everyone,

Thanks for your feedback!

On Thu, Jan 26, 2017 at 1:38 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 25/01/17 12:54, Yoshihiro Shimoda wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> To add a workaround code for ipmmu-vmsa driver, this patch adds
>> a new geometry "force_reset_when_empty" not to reuse iova space.
>
> The domain geometry is absolutely not the appropriate place for that. If
> anything, it could possibly be a domain attribute, but it has nothing to
> do with describing the range of input addresses the hardware is able to
> translate.

I agree, this flag was added without too much consideration about
correct location.

>> When all pfns happen to get unmapped then ask the IOMMU driver to
>> flush the state followed by starting from an empty iova space.
>
> And what happens if all the PFNs are never unmapped? Many devices (USB
> being among them) use a coherent allocation for some kind of descriptor
> buffer which exists for the lifetime of the device, then use streaming
> mappings for data transfers - the net result of that is that the number
> of PFNs mapped will always be >=1, and eventually streaming mapping will
> fail because you've exhausted the address space. And if the device *is*
> a USB controller, at that point the thread will hang because the USB
> core's response to a DMA mapping failure happens to be "keep trying
> indefinitely".

You are absolutely right. My apologies for not providing more
information in the first place.

Like you mention, the workaround can not be made into something
general purpose, however for some restricted use case it might be
helpful. For instance, we have some MMC controllers that are able to
perform on-chip bus mastering but they lack scatter gather support.
>From the hardware design point of view the scatter gather feature was
decided to be put into the IPMMU hardware. Because of that we would
like to use the IPMMU hardware for this purpose, however with some ES
specific errata we need to handle unmapping in a special way.
Fortunately the MMC controllers are grouped together using the same
IPMMU and we are able to make sure that all the buffers are unmapped
now and then from a workaround in the MMC device driver. Not pretty
but not super intrusive.

> Essentially, however long this allocation workaround postpones it, one
> or other failure mode is unavoidable with certain devices unless you can
> do something drastic like periodically suspend and resume the entire
> system to reset everything.

Yeah, suspend/resume or unbind/bind might work.

>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>  drivers/iommu/dma-iommu.c | 42 +++++++++++++++++++++++++++++++++++++-----
>>  drivers/iommu/iova.c      |  9 +++++++++
>>  include/linux/iommu.h     |  2 ++
>>  include/linux/iova.h      |  1 +
>>  4 files changed, 49 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index a0b8c0f..d0fa0b1 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -42,6 +42,7 @@ struct iommu_dma_cookie {
>>       struct iova_domain      iovad;
>>       struct list_head        msi_page_list;
>>       spinlock_t              msi_lock;
>> +     spinlock_t              reset_lock;
>
> So now we do get something in the cookie, but it's protecting a bunch of
> machinery that's accessible from a wider scope? That doesn't seem like a
> good design.

It's not. =)

There is room for improvement in the implementation for sure! We also
had other ideas of tracking mapped pages per device instead of per
domain. We have other requirements from the hardware to serialize some
MMC data handling, so grouping the MMC devices together into a single
IOMMU domain seemed light weight and simple for a first shot!

>>  };
>>
>>  static inline struct iova_domain *cookie_iovad(struct iommu_domain *domain)
>> @@ -74,6 +75,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>>
>>       spin_lock_init(&cookie->msi_lock);
>>       INIT_LIST_HEAD(&cookie->msi_page_list);
>> +     spin_lock_init(&cookie->reset_lock);
>>       domain->iova_cookie = cookie;
>>       return 0;
>>  }
>> @@ -208,9 +210,11 @@ int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
>>  static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>>               dma_addr_t dma_limit)
>>  {
>> +     struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>       struct iova_domain *iovad = cookie_iovad(domain);
>>       unsigned long shift = iova_shift(iovad);
>>       unsigned long length = iova_align(iovad, size) >> shift;
>> +     unsigned long flags;
>>       struct iova *iova;
>>
>>       if (domain->geometry.force_aperture)
>> @@ -219,9 +223,19 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>>        * Enforce size-alignment to be safe - there could perhaps be an
>>        * attribute to control this per-device, or at least per-domain...
>>        */
>> -     iova = alloc_iova(iovad, length, dma_limit >> shift, true);
>> -     if (iova)
>> -             atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
>> +     if (domain->geometry.force_reset_when_empty) {
>> +             spin_lock_irqsave(&cookie->reset_lock, flags);
>
> Is this locking definitely safe against the potential concurrent callers
> of __free_iova() on the same domain who won't touch reset_lock? (It may
> well be, it's just not clear)

I think the code assumes that __free_iova_domain() is used instead of
__free_iova(), and in such case I believe it should be safe as long as
the flag force_reset_when_empty is set early in the IPMMU code and
left unchanged.

>> +
>> +             iova = alloc_iova(iovad, length, dma_limit >> shift, true);
>> +             if (iova)
>> +                     atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
>> +
>> +             spin_unlock_irqrestore(&cookie->reset_lock, flags);
>> +     } else {
>> +             iova = alloc_iova(iovad, length, dma_limit >> shift, true);
>> +             if (iova)
>> +                     atomic_add(iova_size(iova), &domain->iova_pfns_mapped);
>
> Why would we even need to keep track of this on non-broken IOMMUS?

I agree it would be better to not keep track of it.

>> +     }
>>
>>       return iova;
>>  }
>> @@ -229,10 +243,28 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>>  void
>>  __free_iova_domain(struct iommu_domain *domain, struct iova *iova)
>>  {
>> +     struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>       struct iova_domain *iovad = cookie_iovad(domain);
>> +     unsigned long flags;
>>
>> -     atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
>> -     __free_iova(iovad, iova);
>> +     /* In case force_reset_when_empty is set, do not reuse iova space
>> +      * but instead simply keep on expanding seemingly forever.
>> +      * When all pfns happen to get unmapped then ask the IOMMU driver to
>> +      * flush the state followed by starting from an empty iova space.
>> +      */
>> +     if (domain->geometry.force_reset_when_empty) {
>> +             spin_lock_irqsave(&cookie->reset_lock, flags);
>> +             if (atomic_sub_return(iova_size(iova),
>> +                                   &domain->iova_pfns_mapped) == 0) {
>> +                     reset_iova_domain(iovad);
>> +                     if (domain->ops->domain_reset)
>> +                             domain->ops->domain_reset(domain);
>> +             }
>> +             spin_unlock_irqrestore(&cookie->reset_lock, flags);
>> +     } else {
>> +             atomic_sub(iova_size(iova), &domain->iova_pfns_mapped);
>> +             __free_iova(iovad, iova);
>> +     }
>>  }
>>
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 866ad65..50aaa46 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -464,6 +464,7 @@ void put_iova_domain(struct iova_domain *iovad)
>>       while (node) {
>>               struct iova *iova = rb_entry(node, struct iova, node);
>>
>> +             __cached_rbnode_delete_update(iovad, iova);
>>               rb_erase(node, &iovad->rbroot);
>>               free_iova_mem(iova);
>>               node = rb_first(&iovad->rbroot);
>> @@ -472,6 +473,14 @@ void put_iova_domain(struct iova_domain *iovad)
>>  }
>>  EXPORT_SYMBOL_GPL(put_iova_domain);
>>
>> +void
>> +reset_iova_domain(struct iova_domain *iovad)
>> +{
>> +     put_iova_domain(iovad);
>> +     init_iova_rcaches(iovad);
>
> And we have to nuke the entire domain rather than simply freeing all the
> IOVAs because...?
>
> In summary, this is really ugly. If you must implement this workaround,
> it would be infinitely preferable to use a more appropriate allocator in
> the first place, at which point you then need none of the invasive
> hacks. I've already proposed support for multiple allocator types for
> the sake of MSI pages[1], and it's not all that complicated to move the
> abstraction further up into the alloc and free helpers themselves so it
> applies everywhere (I have some work in progress to convert alloc/free
> to be PFN-based, which effectively requires most of the same changes, so
> they're going to get done either way at some point). With that,
> implementing a one-way wraparound allocator as a third type would be
> pretty simple - the IPMMU driver could request that somehow upon getting
> its cookie, then simply skip TLB syncs on unmaps internally (and maybe
> still refcount maps vs. unmaps there if you think there's any chance of
> stale TLB entries lasting long enough to be problematic).

Thanks for your suggestion. Sounds much cleaner.

> I'd still be inclined to simply not pretend to support managed DMA
> domains on this IOMMU if at all possible, though.

We can fortunately mix and match what to enable between different
IPMMU instances, but this particular workaround is specific to some ES
version so once the errata is fixed it should be possible to fall back
to regular behaviour.

> [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1314675.html

Will have a look! Let me know if you happen to attend FOSDEM, would be
good to discuss more if you have time!

Thanks for your help!

/ magnus