[PATCH/RFC 0/7] iommu: fixes & extensions

[PATCH/RFC 0/7] iommu: fixes & extensions

[Ohad Ben-Cohen]

5 first patches are relatively small iommu fixes/cleanups.

2 last patches are proposals for core iommu extensions:
- Add fault report mechanism, needed for recovery of remote processors
  trying to access unmapped addresses.
- Add splitting of memory regions to pages in the iommu core itself
  (according to hardware capabilities as advertised by the iommu drivers).
  This is needed to prevent duplication of this logic by
  the iommu users/drivers themselves.

The patches are based on Joerg's arm/omap branch.

Tested with OMAP3 (omap3isp) + OMAP4 (rpmsg/remoteproc).

Laurent Pinchart (1):
  iommu/omap-iovmm: support non page-aligned buffers in iommu_vmap

Ohad Ben-Cohen (6):
  iommu/omap: cleanup: remove a redundant 'return' statement
  iommu/core: use the existing IS_ALIGNED macro
  iommu/omap: ->unmap() should return order of unmapped page
  iommu/msm: ->unmap() should return order of unmapped page
  iommu/core: add fault reporting
  iommu/core: split mapping to page sizes as supported by the hardware

 arch/arm/plat-omap/include/plat/iommu.h |    3 +-
 drivers/iommu/iommu.c                   |  142 +++++++++++++++++++++++++++----
 drivers/iommu/msm_iommu.c               |   15 +++-
 drivers/iommu/omap-iommu.c              |   56 +++---------
 drivers/iommu/omap-iovmm.c              |   50 +++++++-----
 drivers/media/video/omap3isp/isp.c      |    2 +-
 include/linux/iommu.h                   |   67 ++++++++++++++-
 virt/kvm/iommu.c                        |    6 +-
 8 files changed, 251 insertions(+), 90 deletions(-)

-- 
1.7.4.1

[PATCH 1/7] iommu/omap-iovmm: support non page-aligned buffers in iommu_vmap

[Ohad Ben-Cohen]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

omap_iovmm requires page-aligned buffers, and that sometimes causes
omap3isp failures (i.e. whenever the buffer passed from userspace is not
page-aligned).

Remove this limitation by rounding the address of the first page entry
down, and adding the offset back to the device address.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
[ohad at wizery.com: rebased, but tested only with aligned buffers]
[ohad at wizery.com: slightly edited the commit log]
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/iommu/omap-iovmm.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/omap-iovmm.c b/drivers/iommu/omap-iovmm.c
index 5e7f97d..39bdb92 100644
--- a/drivers/iommu/omap-iovmm.c
+++ b/drivers/iommu/omap-iovmm.c
@@ -27,6 +27,15 @@
 
 static struct kmem_cache *iovm_area_cachep;
 
+/* return the offset of the first scatterlist entry in a sg table */
+static unsigned int sgtable_offset(const struct sg_table *sgt)
+{
+	if (!sgt || !sgt->nents)
+		return 0;
+
+	return sgt->sgl->offset;
+}
+
 /* return total bytes of sg buffers */
 static size_t sgtable_len(const struct sg_table *sgt)
 {
@@ -39,11 +48,17 @@ static size_t sgtable_len(const struct sg_table *sgt)
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
 		size_t bytes;
 
-		bytes = sg->length;
+		bytes = sg->length + sg->offset;
 
 		if (!iopgsz_ok(bytes)) {
-			pr_err("%s: sg[%d] not iommu pagesize(%x)\n",
-			       __func__, i, bytes);
+			pr_err("%s: sg[%d] not iommu pagesize(%u %u)\n",
+			       __func__, i, bytes, sg->offset);
+			return 0;
+		}
+
+		if (i && sg->offset) {
+			pr_err("%s: sg[%d] offset not allowed in internal "
+					"entries\n", __func__, i);
 			return 0;
 		}
 
@@ -164,8 +179,8 @@ static void *vmap_sg(const struct sg_table *sgt)
 		u32 pa;
 		int err;
 
-		pa = sg_phys(sg);
-		bytes = sg->length;
+		pa = sg_phys(sg) - sg->offset;
+		bytes = sg->length + sg->offset;
 
 		BUG_ON(bytes != PAGE_SIZE);
 
@@ -405,8 +420,8 @@ static int map_iovm_area(struct iommu_domain *domain, struct iovm_struct *new,
 		u32 pa;
 		size_t bytes;
 
-		pa = sg_phys(sg);
-		bytes = sg->length;
+		pa = sg_phys(sg) - sg->offset;
+		bytes = sg->length + sg->offset;
 
 		flags &= ~IOVMF_PGSZ_MASK;
 
@@ -432,7 +447,7 @@ err_out:
 	for_each_sg(sgt->sgl, sg, i, j) {
 		size_t bytes;
 
-		bytes = sg->length;
+		bytes = sg->length + sg->offset;
 		order = get_order(bytes);
 
 		/* ignore failures.. we're already handling one */
@@ -461,7 +476,7 @@ static void unmap_iovm_area(struct iommu_domain *domain, struct omap_iommu *obj,
 		size_t bytes;
 		int order;
 
-		bytes = sg->length;
+		bytes = sg->length + sg->offset;
 		order = get_order(bytes);
 
 		err = iommu_unmap(domain, start, order);
@@ -600,7 +615,7 @@ u32 omap_iommu_vmap(struct iommu_domain *domain, struct omap_iommu *obj, u32 da,
 	if (IS_ERR_VALUE(da))
 		vunmap_sg(va);
 
-	return da;
+	return da + sgtable_offset(sgt);
 }
 EXPORT_SYMBOL_GPL(omap_iommu_vmap);
 
@@ -620,6 +635,7 @@ omap_iommu_vunmap(struct iommu_domain *domain, struct omap_iommu *obj, u32 da)
 	 * 'sgt' is allocated before 'omap_iommu_vmalloc()' is called.
 	 * Just returns 'sgt' to the caller to free
 	 */
+	da &= PAGE_MASK;
 	sgt = unmap_vm_area(domain, obj, da, vunmap_sg,
 					IOVMF_DISCONT | IOVMF_MMIO);
 	if (!sgt)
-- 
1.7.4.1

[PATCH 2/7] iommu/omap: cleanup: remove a redundant statement

[Ohad Ben-Cohen]

Tiny cleanup that removes a redundant 'return' statement.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/iommu/omap-iommu.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 90744af..4311bc3 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1069,12 +1069,10 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 	iotlb_init_entry(&e, da, pa, flags);
 
 	ret = omap_iopgtable_store_entry(oiommu, &e);
-	if (ret) {
+	if (ret)
 		dev_err(dev, "omap_iopgtable_store_entry failed: %d\n", ret);
-		return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int omap_iommu_unmap(struct iommu_domain *domain, unsigned long da,
-- 
1.7.4.1

[PATCH 3/7] iommu/core: use the existing IS_ALIGNED macro

[Ohad Ben-Cohen]

Replace iommu's alignment checks with the existing IS_ALIGNED macro,
to drop a few lines of code and utilize IS_ALIGNED's type safety.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/iommu/iommu.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6e6b6a1..e61a9ba 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -16,6 +16,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#include <linux/kernel.h>
 #include <linux/bug.h>
 #include <linux/types.h>
 #include <linux/module.h>
@@ -97,13 +98,11 @@ EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
 int iommu_map(struct iommu_domain *domain, unsigned long iova,
 	      phys_addr_t paddr, int gfp_order, int prot)
 {
-	unsigned long invalid_mask;
 	size_t size;
 
 	size         = 0x1000UL << gfp_order;
-	invalid_mask = size - 1;
 
-	BUG_ON((iova | paddr) & invalid_mask);
+	BUG_ON(!IS_ALIGNED(iova | paddr, size));
 
 	return iommu_ops->map(domain, iova, paddr, gfp_order, prot);
 }
@@ -111,13 +110,11 @@ EXPORT_SYMBOL_GPL(iommu_map);
 
 int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int gfp_order)
 {
-	unsigned long invalid_mask;
 	size_t size;
 
 	size         = 0x1000UL << gfp_order;
-	invalid_mask = size - 1;
 
-	BUG_ON(iova & invalid_mask);
+	BUG_ON(!IS_ALIGNED(iova, size));
 
 	return iommu_ops->unmap(domain, iova, gfp_order);
 }
-- 
1.7.4.1

[PATCH 4/7] iommu/omap: ->unmap() should return order of unmapped page

[Ohad Ben-Cohen]

Users of the IOMMU API (kvm specifically) assume that iommu_unmap()
returns the order of the unmapped page.

Fix omap_iommu_unmap() to do so and adopt omap-iovmm accordingly.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/iommu/omap-iommu.c |   13 ++++---------
 drivers/iommu/omap-iovmm.c |    2 +-
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 4311bc3..bd5f606 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1081,18 +1081,13 @@ static int omap_iommu_unmap(struct iommu_domain *domain, unsigned long da,
 	struct omap_iommu_domain *omap_domain = domain->priv;
 	struct omap_iommu *oiommu = omap_domain->iommu_dev;
 	struct device *dev = oiommu->dev;
-	size_t bytes = PAGE_SIZE << order;
-	size_t ret;
+	size_t unmap_size;
 
-	dev_dbg(dev, "unmapping da 0x%lx size 0x%x\n", da, bytes);
+	dev_dbg(dev, "unmapping da 0x%lx order %d\n", da, order);
 
-	ret = iopgtable_clear_entry(oiommu, da);
-	if (ret != bytes) {
-		dev_err(dev, "entry @ 0x%lx was %d; not %d\n", da, ret, bytes);
-		return -EINVAL;
-	}
+	unmap_size = iopgtable_clear_entry(oiommu, da);
 
-	return 0;
+	return unmap_size ? get_order(unmap_size) : -EINVAL;
 }
 
 static int
diff --git a/drivers/iommu/omap-iovmm.c b/drivers/iommu/omap-iovmm.c
index 39bdb92..e8fdb88 100644
--- a/drivers/iommu/omap-iovmm.c
+++ b/drivers/iommu/omap-iovmm.c
@@ -480,7 +480,7 @@ static void unmap_iovm_area(struct iommu_domain *domain, struct omap_iommu *obj,
 		order = get_order(bytes);
 
 		err = iommu_unmap(domain, start, order);
-		if (err)
+		if (err < 0)
 			break;
 
 		dev_dbg(obj->dev, "%s: unmap %08x(%x) %08x\n",
-- 
1.7.4.1

[PATCH 5/7] iommu/msm: ->unmap() should return order of unmapped page

[Ohad Ben-Cohen]

Users of the IOMMU API (kvm specifically) assume that iommu_unmap()
returns the order of the unmapped page (on success).

Fix msm_iommu_unmap() accordingly.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Stepan Moskovchenko <stepanm@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
---
 drivers/iommu/msm_iommu.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 1a584e0..d1733f6 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -543,6 +543,13 @@ static int msm_iommu_unmap(struct iommu_domain *domain, unsigned long va,
 	}
 
 	ret = __flush_iotlb(domain);
+
+	/*
+	 * the IOMMU API requires us to return the order of the unmapped
+	 * page (on success).
+	 */
+	if (!ret)
+		ret = order;
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
-- 
1.7.4.1

[PATCH 5/7] iommu/msm: ->unmap() should return order of unmapped page

[David Brown]

On Fri, Sep 02, 2011 at 08:32:34PM +0300, Ohad Ben-Cohen wrote:
> Users of the IOMMU API (kvm specifically) assume that iommu_unmap()
> returns the order of the unmapped page (on success).
> 
> Fix msm_iommu_unmap() accordingly.
> 
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Stepan Moskovchenko <stepanm@codeaurora.org>
> Cc: David Brown <davidb@codeaurora.org>
> ---
>  drivers/iommu/msm_iommu.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 1a584e0..d1733f6 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -543,6 +543,13 @@ static int msm_iommu_unmap(struct iommu_domain *domain, unsigned long va,
>  	}
>  
>  	ret = __flush_iotlb(domain);
> +
> +	/*
> +	 * the IOMMU API requires us to return the order of the unmapped
> +	 * page (on success).
> +	 */
> +	if (!ret)
> +		ret = order;
>  fail:
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  	return ret;

Acked-by: David Brown <davidb@codeaurora.org>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 6/7] iommu/core: add fault reporting

[Ohad Ben-Cohen]

Add iommu fault report mechanism to the IOMMU API, so implementations
could report about mmu faults (translation errors, hardware errors,
etc..).

Fault reports can be used in several ways:
- mere logging
- reset the device that accessed the faulting address (may be necessary
  in case the device is a remote processor for example)
- implement dynamic PTE/TLB loading

Currently the fault handler is installed when allocating a new domain
(less churn for users). Alternatively, we can also add a dedicated
iommu_set_fault_handler() API (presumably with notifiers).

Adopt OMAP's iommu driver (and remove its now-redundant
omap_iommu_set_isr API) and fix existing users of iommu_domain_alloc()
to pass NULL fault handlers.

OMAP's iommu driver will currently only pass the generic IOMMU_ERROR
fault, but in principle we can support dynamic PTE/TLB loading too
in the future.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 arch/arm/plat-omap/include/plat/iommu.h |    3 +-
 drivers/iommu/iommu.c                   |   10 +++++-
 drivers/iommu/omap-iommu.c              |   31 ++--------------
 drivers/media/video/omap3isp/isp.c      |    2 +-
 include/linux/iommu.h                   |   60 ++++++++++++++++++++++++++++++-
 virt/kvm/iommu.c                        |    2 +-
 6 files changed, 74 insertions(+), 34 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 7f1df0e..a1d79ee 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -32,6 +32,7 @@ struct omap_iommu {
 	void __iomem	*regbase;
 	struct device	*dev;
 	void		*isr_priv;
+	struct iommu_domain *domain;
 
 	unsigned int	refcount;
 	spinlock_t	iommu_lock;	/* global for this whole object */
@@ -48,8 +49,6 @@ struct omap_iommu {
 	struct list_head	mmap;
 	struct mutex		mmap_lock; /* protect mmap */
 
-	int (*isr)(struct omap_iommu *obj, u32 da, u32 iommu_errs, void *priv);
-
 	void *ctx; /* iommu context: registres saved area */
 	u32 da_start;
 	u32 da_end;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e61a9ba..c08f1a0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -40,7 +40,13 @@ bool iommu_found(void)
 }
 EXPORT_SYMBOL_GPL(iommu_found);
 
-struct iommu_domain *iommu_domain_alloc(void)
+/**
+ * iommu_domain_alloc() - allocate and initialize a new iommu domain
+ * @handler: an optional pointer to a fault handler, or NULL if not needed
+ *
+ * Returns the new domain, or NULL on error.
+ */
+struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler)
 {
 	struct iommu_domain *domain;
 	int ret;
@@ -49,6 +55,8 @@ struct iommu_domain *iommu_domain_alloc(void)
 	if (!domain)
 		return NULL;
 
+	domain->handler = handler;
+
 	ret = iommu_ops->domain_init(domain);
 	if (ret)
 		goto out_free;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bd5f606..089fddc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -775,6 +775,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
 	u32 da, errs;
 	u32 *iopgd, *iopte;
 	struct omap_iommu *obj = data;
+	struct iommu_domain *domain = obj->domain;
 
 	if (!obj->refcount)
 		return IRQ_NONE;
@@ -786,7 +787,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
 		return IRQ_HANDLED;
 
 	/* Fault callback or TLB/PTE Dynamic loading */
-	if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv))
+	if (!report_iommu_fault(domain, obj->dev, da, IOMMU_ERROR))
 		return IRQ_HANDLED;
 
 	iommu_disable(obj);
@@ -904,33 +905,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
 	dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
 }
 
-int omap_iommu_set_isr(const char *name,
-		  int (*isr)(struct omap_iommu *obj, u32 da, u32 iommu_errs,
-			     void *priv),
-		  void *isr_priv)
-{
-	struct device *dev;
-	struct omap_iommu *obj;
-
-	dev = driver_find_device(&omap_iommu_driver.driver, NULL, (void *)name,
-				 device_match_by_alias);
-	if (!dev)
-		return -ENODEV;
-
-	obj = to_iommu(dev);
-	spin_lock(&obj->iommu_lock);
-	if (obj->refcount != 0) {
-		spin_unlock(&obj->iommu_lock);
-		return -EBUSY;
-	}
-	obj->isr = isr;
-	obj->isr_priv = isr_priv;
-	spin_unlock(&obj->iommu_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(omap_iommu_set_isr);
-
 /*
  *	OMAP Device MMU(IOMMU) detection
  */
@@ -1115,6 +1089,7 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	omap_domain->iommu_dev = oiommu;
+	oiommu->domain = domain;
 
 out:
 	spin_unlock(&omap_domain->lock);
diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index a4baa61..5b06769 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -2141,7 +2141,7 @@ static int isp_probe(struct platform_device *pdev)
 	/* to be removed once iommu migration is complete */
 	isp->iommu = to_iommu(isp->iommu_dev);
 
-	isp->domain = iommu_domain_alloc();
+	isp->domain = iommu_domain_alloc(NULL);
 	if (!isp->domain) {
 		dev_err(isp->dev, "can't alloc iommu domain\n");
 		ret = -ENOMEM;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9940319..3cbea04 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -26,9 +26,27 @@
 #define IOMMU_CACHE	(4) /* DMA cache coherency */
 
 struct device;
+struct iommu_domain;
+
+/**
+ * enum iommu_fault_types - iommu fault types
+ *
+ * @IOMMU_ERROR: Unrecoverable error
+ * @IOMMU_TLBMISS: TLB miss while the page table walker is disabled
+ * @IOMMU_NOPTE: TLB miss and no PTE for the requested address
+ */
+enum iommu_fault_types {
+	IOMMU_ERROR,
+	IOMMU_TLBMISS,
+	IOMMU_NOPTE,
+};
+
+typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
+				struct device *, unsigned long, int);
 
 struct iommu_domain {
 	void *priv;
+	iommu_fault_handler_t handler;
 };
 
 #define IOMMU_CAP_CACHE_COHERENCY	0x1
@@ -53,7 +71,7 @@ struct iommu_ops {
 
 extern void register_iommu(struct iommu_ops *ops);
 extern bool iommu_found(void);
-extern struct iommu_domain *iommu_domain_alloc(void);
+extern struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler);
 extern void iommu_domain_free(struct iommu_domain *domain);
 extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
@@ -68,6 +86,40 @@ extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
 extern int iommu_domain_has_cap(struct iommu_domain *domain,
 				unsigned long cap);
 
+/**
+ * report_iommu_fault() - report about an IOMMU fault to the IOMMU framework
+ * @domain: the iommu domain where the fault has happened
+ * @dev: the device where the fault has happened
+ * @iova: the faulting address
+ * @event: the mmu fault type
+ *
+ * This function should be called by the low-level IOMMU implementations
+ * whenever IOMMU faults happen, to allow high-level users, that are
+ * interested in such events, to know about them.
+ *
+ * This event may be useful in case the device, generating the fault,
+ * needs to be restarted before it can be used again (e.g. this device
+ * may be a remote processor), or if dynamic TLB/PTE loading is desired.
+ *
+ * Returns 0 on success and an appropriate error code otherwise (if dynamic
+ * PTE/TLB loading will one day be supported, implementations will be able
+ * to tell whether it succeeded or not according to this return value).
+ */
+static inline int report_iommu_fault(struct iommu_domain *domain,
+		struct device *dev, unsigned long iova, int event)
+{
+	int ret = 0;
+
+	/*
+	 * if upper layers showed interest and installed a fault handler,
+	 * invoke it.
+	 */
+	if (domain->handler)
+		ret = domain->handler(domain, dev, iova, event);
+
+	return ret;
+}
+
 #else /* CONFIG_IOMMU_API */
 
 static inline void register_iommu(struct iommu_ops *ops)
@@ -123,6 +175,12 @@ static inline int domain_has_cap(struct iommu_domain *domain,
 	return 0;
 }
 
+static inline int report_iommu_fault(struct iommu_domain *domain,
+			struct device *dev, unsigned long iova, int event)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 78c80f6..2fd67e5 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -233,7 +233,7 @@ int kvm_iommu_map_guest(struct kvm *kvm)
 		return -ENODEV;
 	}
 
-	kvm->arch.iommu_domain = iommu_domain_alloc();
+	kvm->arch.iommu_domain = iommu_domain_alloc(NULL);
 	if (!kvm->arch.iommu_domain)
 		return -ENOMEM;
 
-- 
1.7.4.1

[RFC 6/7] iommu/core: add fault reporting

[Roedel, Joerg]

On Fri, Sep 02, 2011 at 01:32:35PM -0400, Ohad Ben-Cohen wrote:
> -struct iommu_domain *iommu_domain_alloc(void)
> +/**
> + * iommu_domain_alloc() - allocate and initialize a new iommu domain
> + * @handler: an optional pointer to a fault handler, or NULL if not needed
> + *
> + * Returns the new domain, or NULL on error.
> + */
> +struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler)

Please add a seperate function for setting the fault-handler. It is
optional, so no need to be a value of the alloc-function.

> +/**
> + * enum iommu_fault_types - iommu fault types
> + *
> + * @IOMMU_ERROR: Unrecoverable error
> + * @IOMMU_TLBMISS: TLB miss while the page table walker is disabled
> + * @IOMMU_NOPTE: TLB miss and no PTE for the requested address
> + */
> +enum iommu_fault_types {
> +	IOMMU_ERROR,
> +	IOMMU_TLBMISS,
> +	IOMMU_NOPTE,
> +};

Can you elaborate a bit on what the user of the api will do different
between IOMMU_TLBMISS and IOMMU_NOPTE?
My feeling is that those differences should be handled internally in the
IOMMU driver, but probably I miss a use-case.
Also, we need some flags to distinguish between the type of the fault
(read, write, ...).

	Joerg


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[RFC 6/7] iommu/core: add fault reporting

[Ohad Ben-Cohen]

On Mon, Sep 5, 2011 at 1:00 PM, Roedel, Joerg <Joerg.Roedel@amd.com> wrote:
> Please add a seperate function for setting the fault-handler. It is
> optional, so no need to be a value of the alloc-function.

Will do.

> Can you elaborate a bit on what the user of the api will do different
> between IOMMU_TLBMISS and IOMMU_NOPTE?
> My feeling is that those differences should be handled internally in the
> IOMMU driver, but probably I miss a use-case.

I actually agree. Moreover, since we're not planning on implementing
this (dynamic PTE/TLB loading is supported by the hardware, but we're
not really using it ATM), and I don't see any other user doing so at
this point, I'll just remove those two fault types altogether.

> Also, we need some flags to distinguish between the type of the fault
> (read, write, ...).

I'll add it (though it won't be used by OMAP since the hardware
doesn't tell us this info).

Thanks!
Ohad.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[Ohad Ben-Cohen]

When mapping a memory region, split it to page sizes as supported
by the iommu hardware. Always prefer bigger pages, when possible,
in order to reduce the TLB pressure.

Conversely, when unmapping a memory region, iterate through its pages,
until the region is completely unmapped.

The logic to do that is now added to the IOMMU core, so neither the iommu
drivers themselves nor users of the IOMMU API have to duplicate it.

This is needed by future users (generic DMA-API, remoteproc and tidspbridge
to name a few) and could potentially also simplify existing users
(amd_iommu's iommu_unmap_page, intel-iommu's hardware_largepage_caps, and 
possibly kvm_iommu_put_pages too).

This allows a more lenient granularity of mappings: traditionally the
IOMMU API took 'order' (of a page) as a mapping size, and directly let
the low level iommu drivers handle the mapping. Now that the IOMMU
core can split arbitrary memory regions into pages, we can remove this
limitation, so users don't have to split those regions by themselves.

Currently the supported page sizes are advertised once and they then
remain static. That works well for OMAP (and seemingly MSM too) but
it would probably not fly with intel's hardware, where the page size
capabilities seem to have the potential to be different between
several DMA remapping devices (so we might have to maintain this pgsize
bitmap information per IOMMU device).

Mainline users of the IOMMU API (kvm and omap-iovmm) are adopted
appropriately. Only OMAP and MSM were changed to advertise the supported
page sizes at this point (so this is definitely not merging material).

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/iommu/iommu.c      |  127 +++++++++++++++++++++++++++++++++++++++----
 drivers/iommu/msm_iommu.c  |    8 +++-
 drivers/iommu/omap-iommu.c |    6 ++-
 drivers/iommu/omap-iovmm.c |   12 +---
 include/linux/iommu.h      |    7 ++-
 virt/kvm/iommu.c           |    4 +-
 6 files changed, 136 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c08f1a0..b5c6d63 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -16,6 +16,8 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#define pr_fmt(fmt)    "%s: " fmt, __func__
+
 #include <linux/kernel.h>
 #include <linux/bug.h>
 #include <linux/types.h>
@@ -23,15 +25,41 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/iommu.h>
+#include <linux/bitmap.h>
 
 static struct iommu_ops *iommu_ops;
 
-void register_iommu(struct iommu_ops *ops)
+/* bitmap of supported page sizes */
+static unsigned long *iommu_pgsize_bitmap;
+
+/* number of bits used to represent the supported pages */
+static unsigned int iommu_nr_page_bits;
+
+/* size of the smallest supported page (in bytes) */
+static unsigned int iommu_min_pagesz;
+
+/* bit number of the smallest supported page */
+static unsigned int iommu_min_page_idx;
+
+/**
+ * register_iommu() - register an IOMMU hardware
+ * @ops: iommu handlers
+ * @pgsize_bitmap: bitmap of page sizes supported by the hardware
+ * @nr_page_bits: size of @pgsize_bitmap (in bits)
+ */
+void register_iommu(struct iommu_ops *ops, unsigned long *pgsize_bitmap,
+					unsigned int nr_page_bits)
 {
-	if (iommu_ops)
+	if (iommu_ops || iommu_pgsize_bitmap || !nr_page_bits)
 		BUG();
 
 	iommu_ops = ops;
+	iommu_pgsize_bitmap = pgsize_bitmap;
+	iommu_nr_page_bits = nr_page_bits;
+
+	/* find the minimum page size and its index only once */
+	iommu_min_page_idx = find_first_bit(pgsize_bitmap, nr_page_bits);
+	iommu_min_pagesz = 1 << iommu_min_page_idx;
 }
 
 bool iommu_found(void)
@@ -104,26 +132,101 @@ int iommu_domain_has_cap(struct iommu_domain *domain,
 EXPORT_SYMBOL_GPL(iommu_domain_has_cap);
 
 int iommu_map(struct iommu_domain *domain, unsigned long iova,
-	      phys_addr_t paddr, int gfp_order, int prot)
+	      phys_addr_t paddr, size_t size, int prot)
 {
-	size_t size;
+	int ret = 0;
+
+	/*
+	 * both the virtual address and the physical one, as well as
+	 * the size of the mapping, must be aligned (at least) to the
+	 * size of the smallest page supported by the hardware
+	 */
+	if (!IS_ALIGNED(iova | paddr | size, iommu_min_pagesz)) {
+		pr_err("unaligned: iova 0x%lx pa 0x%x size 0x%x min_pagesz "
+			"0x%x\n", iova, paddr, size, iommu_min_pagesz);
+		return -EINVAL;
+	}
+
+	pr_debug("map this: iova 0x%lx pa 0x%x size 0x%x\n", iova, paddr, size);
+
+	while (size) {
+		unsigned long pgsize = iommu_min_pagesz;
+		unsigned long idx = iommu_min_page_idx;
+		unsigned long addr_merge = iova | paddr;
+		int order;
+
+		/* find the max page size with which iova, paddr are aligned */
+		for (;;) {
+			unsigned long try_pgsize;
 
-	size         = 0x1000UL << gfp_order;
+			idx = find_next_bit(iommu_pgsize_bitmap,
+						iommu_nr_page_bits, idx + 1);
 
-	BUG_ON(!IS_ALIGNED(iova | paddr, size));
+			/* no more pages to check ? */
+			if (idx >= iommu_nr_page_bits)
+				break;
 
-	return iommu_ops->map(domain, iova, paddr, gfp_order, prot);
+			try_pgsize = 1 << idx;
+
+			/* page too big ? addresses not aligned ? */
+			if (size < try_pgsize ||
+					!IS_ALIGNED(addr_merge, try_pgsize))
+				break;
+
+			pgsize = try_pgsize;
+		}
+
+		order = get_order(pgsize);
+
+		pr_debug("mapping: iova 0x%lx pa 0x%x order %d\n", iova,
+							paddr, order);
+
+		ret = iommu_ops->map(domain, iova, paddr, order, prot);
+		if (ret)
+			break;
+
+		size -= pgsize;
+		iova += pgsize;
+		paddr += pgsize;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_map);
 
-int iommu_unmap(struct iommu_domain *domain, unsigned long iova, int gfp_order)
+int iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 {
-	size_t size;
+	int order, unmapped_size, unmapped_order, total_unmapped = 0;
+
+	/*
+	 * The virtual address, as well as the size of the mapping, must be
+	 * aligned (at least) to the size of the smallest page supported
+	 * by the hardware
+	 */
+	if (!IS_ALIGNED(iova | size, iommu_min_pagesz)) {
+		pr_err("unaligned: iova 0x%lx size 0x%x min_pagesz 0x%x\n",
+				iova, size, iommu_min_pagesz);
+		return -EINVAL;
+	}
+
+	pr_debug("unmap this: iova 0x%lx size 0x%x\n", iova, size);
+
+	while (size > total_unmapped) {
+		order = get_order(size - total_unmapped);
+
+		unmapped_order = iommu_ops->unmap(domain, iova, order);
+		if (unmapped_order < 0)
+			return unmapped_order;
+
+		pr_debug("unmapped: iova 0x%lx order %d\n", iova,
+							unmapped_order);
 
-	size         = 0x1000UL << gfp_order;
+		unmapped_size = 0x1000UL << unmapped_order;
 
-	BUG_ON(!IS_ALIGNED(iova, size));
+		iova += unmapped_size;
+		total_unmapped += unmapped_size;
+	}
 
-	return iommu_ops->unmap(domain, iova, gfp_order);
+	return get_order(total_unmapped);
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d1733f6..e59ced9 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -676,6 +676,9 @@ fail:
 	return 0;
 }
 
+/* bitmap of the page sizes currently supported */
+static unsigned long msm_iommu_pgsizes = SZ_4K | SZ_64K | SZ_1M | SZ_16M;
+
 static struct iommu_ops msm_iommu_ops = {
 	.domain_init = msm_iommu_domain_init,
 	.domain_destroy = msm_iommu_domain_destroy,
@@ -728,7 +731,10 @@ static void __init setup_iommu_tex_classes(void)
 static int __init msm_iommu_init(void)
 {
 	setup_iommu_tex_classes();
-	register_iommu(&msm_iommu_ops);
+
+	/* we're only using the first 25 bits of the pgsizes bitmap */
+	register_iommu(&msm_iommu_ops, &msm_iommu_pgsizes, 25);
+
 	return 0;
 }
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 089fddc..3e651f9 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1202,6 +1202,9 @@ static int omap_iommu_domain_has_cap(struct iommu_domain *domain,
 	return 0;
 }
 
+/* bitmap of the page sizes supported by the OMAP IOMMU hardware */
+static unsigned long omap_iommu_pgsizes = SZ_4K | SZ_64K | SZ_1M | SZ_16M;
+
 static struct iommu_ops omap_iommu_ops = {
 	.domain_init	= omap_iommu_domain_init,
 	.domain_destroy	= omap_iommu_domain_destroy,
@@ -1225,7 +1228,8 @@ static int __init omap_iommu_init(void)
 		return -ENOMEM;
 	iopte_cachep = p;
 
-	register_iommu(&omap_iommu_ops);
+	/* we're only using the first 25 bits of the pgsizes bitmap */
+	register_iommu(&omap_iommu_ops, &omap_iommu_pgsizes, 25);
 
 	return platform_driver_register(&omap_iommu_driver);
 }
diff --git a/drivers/iommu/omap-iovmm.c b/drivers/iommu/omap-iovmm.c
index e8fdb88..f4dea5a 100644
--- a/drivers/iommu/omap-iovmm.c
+++ b/drivers/iommu/omap-iovmm.c
@@ -409,7 +409,6 @@ static int map_iovm_area(struct iommu_domain *domain, struct iovm_struct *new,
 	unsigned int i, j;
 	struct scatterlist *sg;
 	u32 da = new->da_start;
-	int order;
 
 	if (!domain || !sgt)
 		return -EINVAL;
@@ -428,12 +427,10 @@ static int map_iovm_area(struct iommu_domain *domain, struct iovm_struct *new,
 		if (bytes_to_iopgsz(bytes) < 0)
 			goto err_out;
 
-		order = get_order(bytes);
-
 		pr_debug("%s: [%d] %08x %08x(%x)\n", __func__,
 			 i, da, pa, bytes);
 
-		err = iommu_map(domain, da, pa, order, flags);
+		err = iommu_map(domain, da, pa, bytes, flags);
 		if (err)
 			goto err_out;
 
@@ -448,10 +445,9 @@ err_out:
 		size_t bytes;
 
 		bytes = sg->length + sg->offset;
-		order = get_order(bytes);
 
 		/* ignore failures.. we're already handling one */
-		iommu_unmap(domain, da, order);
+		iommu_unmap(domain, da, bytes);
 
 		da += bytes;
 	}
@@ -474,12 +470,10 @@ static void unmap_iovm_area(struct iommu_domain *domain, struct omap_iommu *obj,
 	start = area->da_start;
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
 		size_t bytes;
-		int order;
 
 		bytes = sg->length + sg->offset;
-		order = get_order(bytes);
 
-		err = iommu_unmap(domain, start, order);
+		err = iommu_unmap(domain, start, bytes);
 		if (err < 0)
 			break;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3cbea04..116d207 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -69,7 +69,8 @@ struct iommu_ops {
 
 #ifdef CONFIG_IOMMU_API
 
-extern void register_iommu(struct iommu_ops *ops);
+extern void register_iommu(struct iommu_ops *ops, unsigned long *pgsize_bitmap,
+					unsigned int nr_page_bits);
 extern bool iommu_found(void);
 extern struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler);
 extern void iommu_domain_free(struct iommu_domain *domain);
@@ -78,9 +79,9 @@ extern int iommu_attach_device(struct iommu_domain *domain,
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
-		     phys_addr_t paddr, int gfp_order, int prot);
+		     phys_addr_t paddr, size_t size, int prot);
 extern int iommu_unmap(struct iommu_domain *domain, unsigned long iova,
-		       int gfp_order);
+		       size_t size);
 extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain,
 				      unsigned long iova);
 extern int iommu_domain_has_cap(struct iommu_domain *domain,
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index 2fd67e5..3d107b9 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -111,7 +111,7 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
 
 		/* Map into IO address space */
 		r = iommu_map(domain, gfn_to_gpa(gfn), pfn_to_hpa(pfn),
-			      get_order(page_size), flags);
+			      page_size, flags);
 		if (r) {
 			printk(KERN_ERR "kvm_iommu_map_address:"
 			       "iommu failed to map pfn=%llx\n", pfn);
@@ -293,7 +293,7 @@ static void kvm_iommu_put_pages(struct kvm *kvm,
 		pfn  = phys >> PAGE_SHIFT;
 
 		/* Unmap address from IO address space */
-		order       = iommu_unmap(domain, gfn_to_gpa(gfn), 0);
+		order       = iommu_unmap(domain, gfn_to_gpa(gfn), PAGE_SIZE);
 		unmap_pages = 1ULL << order;
 
 		/* Unpin all pages we just unmapped to not leak any memory */
-- 
1.7.4.1

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[KyongHo Cho]

Hi

On Sat, Sep 3, 2011 at 2:32 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> When mapping a memory region, split it to page sizes as supported
> by the iommu hardware. Always prefer bigger pages, when possible,
> in order to reduce the TLB pressure.
>
True. It is important for the peripheral devices that works with IOMMU.

> This allows a more lenient granularity of mappings: traditionally the
> IOMMU API took 'order' (of a page) as a mapping size, and directly let
> the low level iommu drivers handle the mapping. Now that the IOMMU
> core can split arbitrary memory regions into pages, we can remove this
> limitation, so users don't have to split those regions by themselves.
>

Please find the following link that I submitted for our IOMMU.
https://lkml.org/lkml/2011/7/3/152

s5p_iommu_map/unmap accepts any order of physical address and iova
without support of your suggestion if the order is not less than PAGE_SHIFT

Of course, an IO virtual memory manager must be careful when it allocates
IO virtual memory to lead best performance.
But I think IOMMU API must not expect that the caller of iommu_map() knows
about the restriction of IOMMU API implementation.

Regards,
Cho KyongHo.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[Ohad Ben-Cohen]

Hi Cho,

On Wed, Sep 7, 2011 at 4:30 AM, KyongHo Cho <pullip.cho@samsung.com> wrote:
> Please find the following link that I submitted for our IOMMU.
> https://lkml.org/lkml/2011/7/3/152
>
> s5p_iommu_map/unmap accepts any order of physical address and iova
> without support of your suggestion if the order is not less than PAGE_SHIFT

That's exactly what I'm trying to prevent; there's no reason to
duplicate the exact same logic in every iommu driver.

As a result, your map function is quite big (it even duplicates the
same logic for every page size it tries).

Try to rebase your patch on the "iommu/core: split mapping to page
sizes as supported by the hardware" patch I've just sent, and see how
significantly simpler your code becomes.

Relying on a single implementation, provided by the IOMMU core, means
less code to maintain (and debug) and a consistent behavior (across
all supported hardware) exposed to users of the IOMMU API.

> But I think IOMMU API must not expect that the caller of iommu_map() knows
> about the restriction of IOMMU API implementation.

Right. But I don't think there's any disagreement here ?

If any, then I think that s5p_iommu_map() is more limited than what I
propose: if it is given a 64KB region aligned only to a 4KB address,
it will BUG_ON() it. While not ideal, I don't think there's any reason
not to map it using 4KB pages (which is exactly what the new
iommu_map() I propose will do).

Thanks,
Ohad.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[KyongHo Cho]

On Wed, Sep 7, 2011 at 3:01 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Cho,
>
> On Wed, Sep 7, 2011 at 4:30 AM, KyongHo Cho <pullip.cho@samsung.com> wrote:
>> Please find the following link that I submitted for our IOMMU.
>> https://lkml.org/lkml/2011/7/3/152
>>
>> s5p_iommu_map/unmap accepts any order of physical address and iova
>> without support of your suggestion if the order is not less than PAGE_SHIFT
>
> That's exactly what I'm trying to prevent; there's no reason to
> duplicate the exact same logic in every iommu driver.
>
> As a result, your map function is quite big (it even duplicates the
> same logic for every page size it tries).
>
I agree that my map function is big :)
However, it is faster because it does not calculate the given order and
not call any function.

> Try to rebase your patch on the "iommu/core: split mapping to page
> sizes as supported by the hardware" patch I've just sent, and see how
> significantly simpler your code becomes.
>
> Relying on a single implementation, provided by the IOMMU core, means
> less code to maintain (and debug) and a consistent behavior (across
> all supported hardware) exposed to users of the IOMMU API.
>
>> But I think IOMMU API must not expect that the caller of iommu_map() knows
>> about the restriction of IOMMU API implementation.
>
> Right. But I don't think there's any disagreement here ?
>
Yes, sorry:)

> If any, then I think that s5p_iommu_map() is more limited than what I
> propose: if it is given a 64KB region aligned only to a 4KB address,
> it will BUG_ON() it. While not ideal, I don't think there's any reason
> not to map it using 4KB pages (which is exactly what the new
> iommu_map() I propose will do).
>
It is a logical error because the caller of iommu_map() gives
4KB aligned physical address while saying that it is 64KB aligned.
I think it must not be happened .

> Thanks,
> Ohad.

Thank you.

KyongHo.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[Ohad Ben-Cohen]

Hi KyongHo,

On Wed, Sep 7, 2011 at 11:05 AM, KyongHo Cho <pullip.cho@samsung.com> wrote:
> However, it is faster because it does not calculate the given order and
> not call any function.

Hmm this sounds a bit like a red herring to me; optimization of the
map function is not the main subject here. Especially not when we're
discussing mapping of large physically contiguous memory regions which
do not happen too often.

But generally I don't think that duplicating code and eliminating
function calls is a good optimization (the compiler can do that for
us..). Moreover, it looks like s5p_iommu_map() gives a higher priority
for bigger pages first, and only reaches 4KB pages after it tried all
other supported pages. IMHO that's sub-optimal, because small mappings
are more common than big ones.

Another advantage for migrating s5p_iommu_map() over to the subject
patch, is that s5p_iommu_map() doesn't support super sections yet. To
support it, you'd need to add more code (duplicate another while
loop). But if you migrated to the subject patch, then you would only
need to flip the 16MB bit when you advertise page size capabilities
and then that's it; you're done.

> It is a logical error because the caller of iommu_map() gives
> 4KB aligned physical address while saying that it is 64KB aligned.

The caller of iommu_map() doesn't say anything about alignments. It
just gives it a memory region to map, and expect things to just work.

Obviously it's better if big physically contiguous mapping will be
aligned to big pages, but things can still work even if it's not, by
using smaller pages. No reason not to provide this flexibility.

The only requirement the IOMMU API should have is the minimum 4KB
alignment, because IOMMU hardware does not support smaller pages.

Thanks,
Ohad.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[KyongHo Cho]

Hi Ohad,

On Wed, Sep 7, 2011 at 6:16 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hmm this sounds a bit like a red herring to me; optimization of the
:) I agree. sorry.

> map function is not the main subject here. Especially not when we're
> discussing mapping of large physically contiguous memory regions which
> do not happen too often.
>
I've got your point but I thought that it is really needed.

> Another advantage for migrating s5p_iommu_map() over to the subject
> patch, is that s5p_iommu_map() doesn't support super sections yet. To
> support it, you'd need to add more code (duplicate another while
> loop). But if you migrated to the subject patch, then you would only
> need to flip the 16MB bit when you advertise page size capabilities
> and then that's it; you're done.

I did not implement that.
16MB page is less practical in Linux because Linux kernel is unable
to allocated larger physically contiguous memory than 4MB by default.
But I also think that it is needed to support 16MB mapping for IO
virtualization someday
and it is just trivial job.

And you pointed correctly that s5p_iommu_map() has duplicate similar codes.

Actually, I think your idea is good and does not cause performance degradation.
But I wondered if it is really useful.

>
> The caller of iommu_map() doesn't say anything about alignments. It
> just gives it a memory region to map, and expect things to just work.
>
The caller of iommu_map() gives gfp_order that is the size of the physical
memory to map.
I thought that it also means alignment of the physical memory.
Isn't it?

Regards,
KyongHo

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[Ohad Ben-Cohen]

Hi KyongHo,

On Thu, Sep 8, 2011 at 3:51 PM, KyongHo Cho <pullip.cho@samsung.com> wrote:
> 16MB page is less practical in Linux because Linux kernel is unable
> to allocated larger physically contiguous memory than 4MB by default.
> But I also think that it is needed to support 16MB mapping for IO
> virtualization someday

Actually we need physically contiguous memory regions that are much
bigger (in the tens of megs), so we do utilize 16MB pages. Today we
reserve that memory on boot, but the plan is to move to CMA.

> Actually, I think your idea is good and does not cause performance degradation.
> But I wondered if it is really useful.

Well, the alternative is to duplicate this logic in every IOMMU driver.

Go ahead and try to rebase your driver on my recent patch set and see
if you like it; the result should significantly simplify your
map/unmap functions.

You only need to add this line:
static unsigned long s5p_iommu_pgsizes = SZ_4K | SZ_64K | SZ_1M | SZ_16M;

and then advertise it with iommu_register, and you're done. The IOMMU
core will only ask you to handle a single page from now on, and will
take care of the rest.

> The caller of iommu_map() gives gfp_order that is the size of the physical
> memory to map.

I've changed it in the patch :)

This way users are not bound to rigid mapping sizes (this allows us to
better utilize the physically-contiguous memory region we reserve on
boot).

Thanks,
Ohad.

[RFC 7/7] iommu/core: split mapping to page sizes as supported by the hardware

[Ohad Ben-Cohen]

[resending due to mailer-daemon errors I got, sorry]

Hi KyongHo,

On Wed, Sep 7, 2011 at 11:05 AM, KyongHo Cho <pullip.cho@samsung.com> wrote:
> However, it is faster because it does not calculate the given order and
> not call any function.

Hmm this sounds a bit like a red herring to me; optimization of the
map function is not the main subject here. Especially not when we're
discussing mapping of large physically contiguous memory regions which
do not happen too often.

But generally I don't think that duplicating code and eliminating
function calls is a good optimization (the compiler can do that for
us..). Moreover, it looks like s5p_iommu_map() gives a higher priority
for bigger pages first, and only reaches 4KB pages after it tried all
other supported pages. IMHO that's sub-optimal, because small mappings
are more common than big ones.

Another advantage for migrating s5p_iommu_map() over to the subject
patch, is that s5p_iommu_map() doesn't support super sections yet. To
support it, you'd need to add more code (duplicate another while
loop). But if you migrated to the subject patch, then you would only
need to flip the 16MB bit when you advertise page size capabilities
and then that's it; you're done.

> It is a logical error because the caller of iommu_map() gives
> 4KB aligned physical address while saying that it is 64KB aligned.

The caller of iommu_map() doesn't say anything about alignments. It
just gives it a memory region to map, and expect things to just work.

Obviously it's better if big physically contiguous mapping will be
aligned to big pages, but things can still work even if it's not, by
using smaller pages. No reason not to provide this flexibility.

The only requirement the IOMMU API should have is the minimum 4KB
alignment, because IOMMU hardware does not support smaller pages.

Thanks,
Ohad.

[PATCH/RFC 0/7] iommu: fixes & extensions

[Roedel, Joerg]

On Fri, Sep 02, 2011 at 01:32:29PM -0400, Ohad Ben-Cohen wrote:
> 5 first patches are relatively small iommu fixes/cleanups.
> 
> 2 last patches are proposals for core iommu extensions:
> - Add fault report mechanism, needed for recovery of remote processors
>   trying to access unmapped addresses.
> - Add splitting of memory regions to pages in the iommu core itself
>   (according to hardware capabilities as advertised by the iommu drivers).
>   This is needed to prevent duplication of this logic by
>   the iommu users/drivers themselves.
> 
> The patches are based on Joerg's arm/omap branch.
> 
> Tested with OMAP3 (omap3isp) + OMAP4 (rpmsg/remoteproc).
> 
> Laurent Pinchart (1):
>   iommu/omap-iovmm: support non page-aligned buffers in iommu_vmap
> 
> Ohad Ben-Cohen (6):
>   iommu/omap: cleanup: remove a redundant 'return' statement
>   iommu/core: use the existing IS_ALIGNED macro
>   iommu/omap: ->unmap() should return order of unmapped page
>   iommu/msm: ->unmap() should return order of unmapped page
>   iommu/core: add fault reporting
>   iommu/core: split mapping to page sizes as supported by the hardware

Ok, I applied 1-5 to their respective branches. Patch 6 needs some more
discussion to make sure the interface is generally usable. Patch 7 seems
to be a starting point for now. This definitly requires conversion of
the other IOMMU drivers too.
Please make individual patch-sets from patch 6 and 7 respectivly.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH/RFC 0/7] iommu: fixes & extensions

[Ohad Ben-Cohen]

On Tue, Sep 6, 2011 at 1:15 PM, Roedel, Joerg <Joerg.Roedel@amd.com> wrote:
> Ok, I applied 1-5 to their respective branches. Patch 6 needs some more
> discussion to make sure the interface is generally usable. Patch 7 seems
> to be a starting point for now. This definitly requires conversion of
> the other IOMMU drivers too.
> Please make individual patch-sets from patch 6 and 7 respectivly.

Will do, thanks !