[PATCH v5 0/1] Add iommu map_sg/unmap_sg API

[PATCH v5 0/1] Add iommu map_sg/unmap_sg API

[Olav Haugan]

Patch for adding map_sg/unmap_sg to the generic IOMMU API.

v4 -> v5:
* Simplified logic in map_sg on error
* Moved fallback to separate function for both map an unmap and pointed
  all drivers to these fallbacks.

v3 -> v4:
* Removed BUG_ON in both map_sg and unmap_sg
* Removed PAGE_ALIGN of length of mapping

v2 -> v3:
* Updated commit text
* Simplifed fallback code and fixed variable types in map_sg function.
* Renamed and changed arguments to map and unmap functions

v1 -> v2:
* Added support for "option" argument to unmap call. This can be used by
  IOMMU driver implentations to allow clients to signal to the driver not
  to do TLB invalidate for example.
* Added fallback in case iommu_{map,unmap}_range is called for an IOMMU driver
  that does not have these implemented.
* Rebased on top of Joerg's tree.
* Split out patch from the rest of the patches.


Olav Haugan (1):
  iommu-api: Add map_sg/unmap_sg functions

 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 ++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 0/1] Add iommu map_sg/unmap_sg API

[Olav Haugan]

Patch for adding map_sg/unmap_sg to the generic IOMMU API.

v4 -> v5:
* Simplified logic in map_sg on error
* Moved fallback to separate function for both map an unmap and pointed
  all drivers to these fallbacks.

v3 -> v4:
* Removed BUG_ON in both map_sg and unmap_sg
* Removed PAGE_ALIGN of length of mapping

v2 -> v3:
* Updated commit text
* Simplifed fallback code and fixed variable types in map_sg function.
* Renamed and changed arguments to map and unmap functions

v1 -> v2:
* Added support for "option" argument to unmap call. This can be used by
  IOMMU driver implentations to allow clients to signal to the driver not
  to do TLB invalidate for example.
* Added fallback in case iommu_{map,unmap}_range is called for an IOMMU driver
  that does not have these implemented.
* Rebased on top of Joerg's tree.
* Split out patch from the rest of the patches.


Olav Haugan (1):
  iommu-api: Add map_sg/unmap_sg functions

 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 ++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

Mapping and unmapping are more often than not in the critical path.
map_sg and unmap_sg allows IOMMU driver implementations to optimize
the process of mapping and unmapping buffers into the IOMMU page tables.

Instead of mapping a buffer one page at a time and requiring potentially
expensive TLB operations for each page, this function allows the driver
to map all pages in one go and defer TLB maintenance until after all
pages have been mapped.

Additionally, the mapping operation would be faster in general since
clients does not have to keep calling map API over and over again for
each physically contiguous chunk of memory that needs to be mapped to a
virtually contiguous region.

Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
---
 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 ++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1840531..3604f3c 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3402,6 +3402,8 @@ static const struct iommu_ops amd_iommu_ops = {
 	.detach_dev = amd_iommu_detach_device,
 	.map = amd_iommu_map,
 	.unmap = amd_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = amd_iommu_iova_to_phys,
 	.domain_has_cap = amd_iommu_domain_has_cap,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ca18d6d..231e006 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1604,6 +1604,8 @@ static const struct iommu_ops arm_smmu_ops = {
 	.detach_dev	= arm_smmu_detach_dev,
 	.map		= arm_smmu_map,
 	.unmap		= arm_smmu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= arm_smmu_iova_to_phys,
 	.domain_has_cap	= arm_smmu_domain_has_cap,
 	.add_device	= arm_smmu_add_device,
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index d037e87..8876508 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1177,6 +1177,8 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.detach_dev = exynos_iommu_detach_device,
 	.map = exynos_iommu_map,
 	.unmap = exynos_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = exynos_iommu_iova_to_phys,
 	.add_device = exynos_iommu_add_device,
 	.remove_device = exynos_iommu_remove_device,
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d1f5caa..4fc61ff 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4462,6 +4462,8 @@ static const struct iommu_ops intel_iommu_ops = {
 	.detach_dev	= intel_iommu_detach_device,
 	.map		= intel_iommu_map,
 	.unmap		= intel_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= intel_iommu_iova_to_phys,
 	.domain_has_cap = intel_iommu_domain_has_cap,
 	.add_device	= intel_iommu_add_device,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1698360..24cf727 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
 
+int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			 struct scatterlist *sg, unsigned int nents,
+			 int prot, unsigned long flags)
+{
+	int ret = 0;
+	unsigned long offset = 0;
+	unsigned int i;
+	struct scatterlist *s;
+
+	for_each_sg(sg, s, nents, i) {
+		phys_addr_t phys = page_to_phys(sg_page(s));
+		size_t page_len = s->offset + s->length;
+
+		ret = iommu_map(domain, iova + offset, phys, page_len,
+				prot);
+		if (ret) {
+			/* undo mappings already done */
+			iommu_unmap(domain, iova, offset);
+			break;
+		}
+		offset += page_len;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(default_iommu_map_sg);
+
+int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
+			       size_t size, unsigned long flags)
+{
+	return iommu_unmap(domain, iova, size);
+}
+EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);
 
 int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
 			       phys_addr_t paddr, u64 size, int prot)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 7dab5cb..1dc6f94 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1127,6 +1127,8 @@ static const struct iommu_ops ipmmu_ops = {
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
 	.unmap = ipmmu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = ipmmu_iova_to_phys,
 	.add_device = ipmmu_add_device,
 	.remove_device = ipmmu_remove_device,
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 49f41d6..2b3c8d3 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -681,6 +681,8 @@ static const struct iommu_ops msm_iommu_ops = {
 	.detach_dev = msm_iommu_detach_dev,
 	.map = msm_iommu_map,
 	.unmap = msm_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
 	.domain_has_cap = msm_iommu_domain_has_cap,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index e202b0c..7d4c920 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1285,6 +1285,8 @@ static const struct iommu_ops omap_iommu_ops = {
 	.detach_dev	= omap_iommu_detach_dev,
 	.map		= omap_iommu_map,
 	.unmap		= omap_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= omap_iommu_iova_to_phys,
 	.add_device	= omap_iommu_add_device,
 	.remove_device	= omap_iommu_remove_device,
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index 1333e6fb..3510f70 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -361,6 +361,8 @@ static const struct iommu_ops shmobile_iommu_ops = {
 	.detach_dev = shmobile_iommu_detach_device,
 	.map = shmobile_iommu_map,
 	.unmap = shmobile_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = shmobile_iommu_iova_to_phys,
 	.add_device = shmobile_iommu_add_device,
 	.pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 792da5e..ebd847b 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -954,6 +954,8 @@ static const struct iommu_ops smmu_iommu_ops = {
 	.detach_dev	= smmu_iommu_detach_dev,
 	.map		= smmu_iommu_map,
 	.unmap		= smmu_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= smmu_iommu_iova_to_phys,
 	.domain_has_cap	= smmu_iommu_domain_has_cap,
 	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a52..ee106ce 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -22,6 +22,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/types.h>
+#include <linux/scatterlist.h>
 #include <trace/events/iommu.h>
 
 #define IOMMU_READ	(1 << 0)
@@ -93,6 +94,10 @@ enum iommu_attr {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @map_sg: map a scatter-gather list of physically contiguous memory chunks
+ * to an iommu domain
+ * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
+ * chunks from an iommu domain
  * @iova_to_phys: translate iova to physical address
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping
@@ -110,6 +115,11 @@ struct iommu_ops {
 		   phys_addr_t paddr, size_t size, int prot);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
 		     size_t size);
+	int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
+			struct scatterlist *sg, unsigned int nents, int prot,
+			unsigned long flags);
+	int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
+			size_t size, unsigned long flags);
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
 	int (*domain_has_cap)(struct iommu_domain *domain,
 			      unsigned long cap);
@@ -153,6 +163,12 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);
 extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 		       size_t size);
+extern int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+				struct scatterlist *sg,unsigned int nents,
+				int prot, unsigned long flags);
+extern int default_iommu_unmap_sg(struct iommu_domain *domain,
+				  unsigned long iova, size_t size,
+				  unsigned long flags);
 extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova);
 extern int iommu_domain_has_cap(struct iommu_domain *domain,
 				unsigned long cap);
@@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct iommu_domain *domain,
 	return ret;
 }
 
+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -287,6 +317,20 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	return -ENODEV;
 }
 
+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return -ENODEV;
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_domain_window_enable(struct iommu_domain *domain,
 					     u32 wnd_nr, phys_addr_t paddr,
 					     u64 size, int prot)
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

Mapping and unmapping are more often than not in the critical path.
map_sg and unmap_sg allows IOMMU driver implementations to optimize
the process of mapping and unmapping buffers into the IOMMU page tables.

Instead of mapping a buffer one page at a time and requiring potentially
expensive TLB operations for each page, this function allows the driver
to map all pages in one go and defer TLB maintenance until after all
pages have been mapped.

Additionally, the mapping operation would be faster in general since
clients does not have to keep calling map API over and over again for
each physically contiguous chunk of memory that needs to be mapped to a
virtually contiguous region.

Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
---
 drivers/iommu/amd_iommu.c      |  2 ++
 drivers/iommu/arm-smmu.c       |  2 ++
 drivers/iommu/exynos-iommu.c   |  2 ++
 drivers/iommu/intel-iommu.c    |  2 ++
 drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
 drivers/iommu/ipmmu-vmsa.c     |  2 ++
 drivers/iommu/msm_iommu.c      |  2 ++
 drivers/iommu/omap-iommu.c     |  2 ++
 drivers/iommu/shmobile-iommu.c |  2 ++
 drivers/iommu/tegra-smmu.c     |  2 ++
 include/linux/iommu.h          | 44 ++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 95 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1840531..3604f3c 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3402,6 +3402,8 @@ static const struct iommu_ops amd_iommu_ops = {
 	.detach_dev = amd_iommu_detach_device,
 	.map = amd_iommu_map,
 	.unmap = amd_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = amd_iommu_iova_to_phys,
 	.domain_has_cap = amd_iommu_domain_has_cap,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ca18d6d..231e006 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1604,6 +1604,8 @@ static const struct iommu_ops arm_smmu_ops = {
 	.detach_dev	= arm_smmu_detach_dev,
 	.map		= arm_smmu_map,
 	.unmap		= arm_smmu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= arm_smmu_iova_to_phys,
 	.domain_has_cap	= arm_smmu_domain_has_cap,
 	.add_device	= arm_smmu_add_device,
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index d037e87..8876508 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1177,6 +1177,8 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.detach_dev = exynos_iommu_detach_device,
 	.map = exynos_iommu_map,
 	.unmap = exynos_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = exynos_iommu_iova_to_phys,
 	.add_device = exynos_iommu_add_device,
 	.remove_device = exynos_iommu_remove_device,
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d1f5caa..4fc61ff 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4462,6 +4462,8 @@ static const struct iommu_ops intel_iommu_ops = {
 	.detach_dev	= intel_iommu_detach_device,
 	.map		= intel_iommu_map,
 	.unmap		= intel_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= intel_iommu_iova_to_phys,
 	.domain_has_cap = intel_iommu_domain_has_cap,
 	.add_device	= intel_iommu_add_device,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1698360..24cf727 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 }
 EXPORT_SYMBOL_GPL(iommu_unmap);
 
+int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			 struct scatterlist *sg, unsigned int nents,
+			 int prot, unsigned long flags)
+{
+	int ret = 0;
+	unsigned long offset = 0;
+	unsigned int i;
+	struct scatterlist *s;
+
+	for_each_sg(sg, s, nents, i) {
+		phys_addr_t phys = page_to_phys(sg_page(s));
+		size_t page_len = s->offset + s->length;
+
+		ret = iommu_map(domain, iova + offset, phys, page_len,
+				prot);
+		if (ret) {
+			/* undo mappings already done */
+			iommu_unmap(domain, iova, offset);
+			break;
+		}
+		offset += page_len;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(default_iommu_map_sg);
+
+int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
+			       size_t size, unsigned long flags)
+{
+	return iommu_unmap(domain, iova, size);
+}
+EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);
 
 int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
 			       phys_addr_t paddr, u64 size, int prot)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 7dab5cb..1dc6f94 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1127,6 +1127,8 @@ static const struct iommu_ops ipmmu_ops = {
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
 	.unmap = ipmmu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = ipmmu_iova_to_phys,
 	.add_device = ipmmu_add_device,
 	.remove_device = ipmmu_remove_device,
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 49f41d6..2b3c8d3 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -681,6 +681,8 @@ static const struct iommu_ops msm_iommu_ops = {
 	.detach_dev = msm_iommu_detach_dev,
 	.map = msm_iommu_map,
 	.unmap = msm_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
 	.domain_has_cap = msm_iommu_domain_has_cap,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index e202b0c..7d4c920 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1285,6 +1285,8 @@ static const struct iommu_ops omap_iommu_ops = {
 	.detach_dev	= omap_iommu_detach_dev,
 	.map		= omap_iommu_map,
 	.unmap		= omap_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= omap_iommu_iova_to_phys,
 	.add_device	= omap_iommu_add_device,
 	.remove_device	= omap_iommu_remove_device,
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index 1333e6fb..3510f70 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -361,6 +361,8 @@ static const struct iommu_ops shmobile_iommu_ops = {
 	.detach_dev = shmobile_iommu_detach_device,
 	.map = shmobile_iommu_map,
 	.unmap = shmobile_iommu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.unmap_sg = default_iommu_unmap_sg,
 	.iova_to_phys = shmobile_iommu_iova_to_phys,
 	.add_device = shmobile_iommu_add_device,
 	.pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 792da5e..ebd847b 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -954,6 +954,8 @@ static const struct iommu_ops smmu_iommu_ops = {
 	.detach_dev	= smmu_iommu_detach_dev,
 	.map		= smmu_iommu_map,
 	.unmap		= smmu_iommu_unmap,
+	.map_sg		= default_iommu_map_sg,
+	.unmap_sg	= default_iommu_unmap_sg,
 	.iova_to_phys	= smmu_iommu_iova_to_phys,
 	.domain_has_cap	= smmu_iommu_domain_has_cap,
 	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a52..ee106ce 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -22,6 +22,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/types.h>
+#include <linux/scatterlist.h>
 #include <trace/events/iommu.h>
 
 #define IOMMU_READ	(1 << 0)
@@ -93,6 +94,10 @@ enum iommu_attr {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @map_sg: map a scatter-gather list of physically contiguous memory chunks
+ * to an iommu domain
+ * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
+ * chunks from an iommu domain
  * @iova_to_phys: translate iova to physical address
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping
@@ -110,6 +115,11 @@ struct iommu_ops {
 		   phys_addr_t paddr, size_t size, int prot);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
 		     size_t size);
+	int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
+			struct scatterlist *sg, unsigned int nents, int prot,
+			unsigned long flags);
+	int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
+			size_t size, unsigned long flags);
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
 	int (*domain_has_cap)(struct iommu_domain *domain,
 			      unsigned long cap);
@@ -153,6 +163,12 @@ extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);
 extern size_t iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 		       size_t size);
+extern int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+				struct scatterlist *sg,unsigned int nents,
+				int prot, unsigned long flags);
+extern int default_iommu_unmap_sg(struct iommu_domain *domain,
+				  unsigned long iova, size_t size,
+				  unsigned long flags);
 extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova);
 extern int iommu_domain_has_cap(struct iommu_domain *domain,
 				unsigned long cap);
@@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct iommu_domain *domain,
 	return ret;
 }
 
+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return domain->ops->unmap_sg(domain, iova, size, flags);
+}
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -287,6 +317,20 @@ static inline int iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	return -ENODEV;
 }
 
+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
+			       struct scatterlist *sg, unsigned int nents,
+			       int prot, unsigned long flags)
+{
+	return -ENODEV;
+}
+
+static inline int iommu_unmap_sg(struct iommu_domain *domain,
+				 unsigned long iova, size_t size,
+				 unsigned long flags)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_domain_window_enable(struct iommu_domain *domain,
 					     u32 wnd_nr, phys_addr_t paddr,
 					     u64 size, int prot)
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Konrad Rzeszutek Wilk]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> Mapping and unmapping are more often than not in the critical path.
> map_sg and unmap_sg allows IOMMU driver implementations to optimize
> the process of mapping and unmapping buffers into the IOMMU page tables.
> 
> Instead of mapping a buffer one page at a time and requiring potentially
> expensive TLB operations for each page, this function allows the driver
> to map all pages in one go and defer TLB maintenance until after all
> pages have been mapped.
> 
> Additionally, the mapping operation would be faster in general since
> clients does not have to keep calling map API over and over again for
> each physically contiguous chunk of memory that needs to be mapped to a
> virtually contiguous region.
> 
> Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

Thank you for changing it this way.


.. snip..
> +	for_each_sg(sg, s, nents, i) {
> +		phys_addr_t phys = page_to_phys(sg_page(s));
> +		size_t page_len = s->offset + s->length;
> +
> +		ret = iommu_map(domain, iova + offset, phys, page_len,
> +				prot);
> +		if (ret) {
> +			/* undo mappings already done */
> +			iommu_unmap(domain, iova, offset);

Don't we want then to unmap all of the scatter list instead of just
the last one?

> +			break;
> +		}
> +		offset += page_len;
> +	}
> +
> +	return ret;
> +}

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Konrad Rzeszutek Wilk]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> Mapping and unmapping are more often than not in the critical path.
> map_sg and unmap_sg allows IOMMU driver implementations to optimize
> the process of mapping and unmapping buffers into the IOMMU page tables.
> 
> Instead of mapping a buffer one page at a time and requiring potentially
> expensive TLB operations for each page, this function allows the driver
> to map all pages in one go and defer TLB maintenance until after all
> pages have been mapped.
> 
> Additionally, the mapping operation would be faster in general since
> clients does not have to keep calling map API over and over again for
> each physically contiguous chunk of memory that needs to be mapped to a
> virtually contiguous region.
> 
> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>

Thank you for changing it this way.


.. snip..
> +	for_each_sg(sg, s, nents, i) {
> +		phys_addr_t phys = page_to_phys(sg_page(s));
> +		size_t page_len = s->offset + s->length;
> +
> +		ret = iommu_map(domain, iova + offset, phys, page_len,
> +				prot);
> +		if (ret) {
> +			/* undo mappings already done */
> +			iommu_unmap(domain, iova, offset);

Don't we want then to unmap all of the scatter list instead of just
the last one?

> +			break;
> +		}
> +		offset += page_len;
> +	}
> +
> +	return ret;
> +}

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/11/2014 6:35 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> .. snip..
>> +	for_each_sg(sg, s, nents, i) {
>> +		phys_addr_t phys = page_to_phys(sg_page(s));
>> +		size_t page_len = s->offset + s->length;
>> +
>> +		ret = iommu_map(domain, iova + offset, phys, page_len,
>> +				prot);
>> +		if (ret) {
>> +			/* undo mappings already done */
>> +			iommu_unmap(domain, iova, offset);
> 
> Don't we want then to unmap all of the scatter list instead of just
> the last one?
> 

It reverts all the mapping that was already done up until the error
occurred. "offset" contains the amount we have mapped so far.

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/11/2014 6:35 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> .. snip..
>> +	for_each_sg(sg, s, nents, i) {
>> +		phys_addr_t phys = page_to_phys(sg_page(s));
>> +		size_t page_len = s->offset + s->length;
>> +
>> +		ret = iommu_map(domain, iova + offset, phys, page_len,
>> +				prot);
>> +		if (ret) {
>> +			/* undo mappings already done */
>> +			iommu_unmap(domain, iova, offset);
> 
> Don't we want then to unmap all of the scatter list instead of just
> the last one?
> 

It reverts all the mapping that was already done up until the error
occurred. "offset" contains the amount we have mapped so far.

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Hiroshi Doyu]

Hi Olav,

Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:

> @@ -93,6 +94,10 @@ enum iommu_attr {
>   * @detach_dev: detach device from an iommu domain
>   * @map: map a physically contiguous memory region to an iommu domain
>   * @unmap: unmap a physically contiguous memory region from an iommu domain
> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
> + * to an iommu domain
> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
> + * chunks from an iommu domain
>   * @iova_to_phys: translate iova to physical address
>   * @domain_has_cap: domain capabilities query
>   * @add_device: add device to iommu grouping
> @@ -110,6 +115,11 @@ struct iommu_ops {
>                    phys_addr_t paddr, size_t size, int prot);
>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>                      size_t size);
> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
> +                       struct scatterlist *sg, unsigned int nents, int prot,
> +                       unsigned long flags);
> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
> +                       size_t size, unsigned long flags);

Do you have any exmaple/explanation for the above "flags"?

Is this going to be used for iommu global/standard attribute or SoC
spcific one?

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Hiroshi Doyu]

Hi Olav,

Olav Haugan <ohaugan@codeaurora.org> writes:

> @@ -93,6 +94,10 @@ enum iommu_attr {
>   * @detach_dev: detach device from an iommu domain
>   * @map: map a physically contiguous memory region to an iommu domain
>   * @unmap: unmap a physically contiguous memory region from an iommu domain
> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
> + * to an iommu domain
> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
> + * chunks from an iommu domain
>   * @iova_to_phys: translate iova to physical address
>   * @domain_has_cap: domain capabilities query
>   * @add_device: add device to iommu grouping
> @@ -110,6 +115,11 @@ struct iommu_ops {
>                    phys_addr_t paddr, size_t size, int prot);
>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>                      size_t size);
> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
> +                       struct scatterlist *sg, unsigned int nents, int prot,
> +                       unsigned long flags);
> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
> +                       size_t size, unsigned long flags);

Do you have any exmaple/explanation for the above "flags"?

Is this going to be used for iommu global/standard attribute or SoC
spcific one?

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Rob Clark]

On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Hi Olav,
>
> Olav Haugan <ohaugan@codeaurora.org> writes:
>
>> @@ -93,6 +94,10 @@ enum iommu_attr {
>>   * @detach_dev: detach device from an iommu domain
>>   * @map: map a physically contiguous memory region to an iommu domain
>>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
>> + * to an iommu domain
>> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
>> + * chunks from an iommu domain
>>   * @iova_to_phys: translate iova to physical address
>>   * @domain_has_cap: domain capabilities query
>>   * @add_device: add device to iommu grouping
>> @@ -110,6 +115,11 @@ struct iommu_ops {
>>                    phys_addr_t paddr, size_t size, int prot);
>>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>>                      size_t size);
>> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
>> +                       struct scatterlist *sg, unsigned int nents, int prot,
>> +                       unsigned long flags);
>> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
>> +                       size_t size, unsigned long flags);
>
> Do you have any exmaple/explanation for the above "flags"?
>
> Is this going to be used for iommu global/standard attribute or SoC
> spcific one?

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
drivers which wanted to map/unmap N buffers with a single flush at the
end.  There might have been some other usages envisioned.

BR,
-R

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Rob Clark]

On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Hi Olav,
>
> Olav Haugan <ohaugan@codeaurora.org> writes:
>
>> @@ -93,6 +94,10 @@ enum iommu_attr {
>>   * @detach_dev: detach device from an iommu domain
>>   * @map: map a physically contiguous memory region to an iommu domain
>>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
>> + * to an iommu domain
>> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
>> + * chunks from an iommu domain
>>   * @iova_to_phys: translate iova to physical address
>>   * @domain_has_cap: domain capabilities query
>>   * @add_device: add device to iommu grouping
>> @@ -110,6 +115,11 @@ struct iommu_ops {
>>                    phys_addr_t paddr, size_t size, int prot);
>>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>>                      size_t size);
>> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
>> +                       struct scatterlist *sg, unsigned int nents, int prot,
>> +                       unsigned long flags);
>> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
>> +                       size_t size, unsigned long flags);
>
> Do you have any exmaple/explanation for the above "flags"?
>
> Is this going to be used for iommu global/standard attribute or SoC
> spcific one?

iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
drivers which wanted to map/unmap N buffers with a single flush at the
end.  There might have been some other usages envisioned.

BR,
-R

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/12/2014 3:48 AM, Rob Clark wrote:
> On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> Hi Olav,
>>
>> Olav Haugan <ohaugan@codeaurora.org> writes:
>>
>>> @@ -93,6 +94,10 @@ enum iommu_attr {
>>>   * @detach_dev: detach device from an iommu domain
>>>   * @map: map a physically contiguous memory region to an iommu domain
>>>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>>> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
>>> + * to an iommu domain
>>> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
>>> + * chunks from an iommu domain
>>>   * @iova_to_phys: translate iova to physical address
>>>   * @domain_has_cap: domain capabilities query
>>>   * @add_device: add device to iommu grouping
>>> @@ -110,6 +115,11 @@ struct iommu_ops {
>>>                    phys_addr_t paddr, size_t size, int prot);
>>>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>>>                      size_t size);
>>> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
>>> +                       struct scatterlist *sg, unsigned int nents, int prot,
>>> +                       unsigned long flags);
>>> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
>>> +                       size_t size, unsigned long flags);
>>
>> Do you have any exmaple/explanation for the above "flags"?
>>
>> Is this going to be used for iommu global/standard attribute or SoC
>> spcific one?
> 
> iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
> drivers which wanted to map/unmap N buffers with a single flush at the
> end.  There might have been some other usages envisioned.
> 

Yes, that was the original intent of the flags for now. I am sure we can
find other uses for this in the future.

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/12/2014 3:48 AM, Rob Clark wrote:
> On Mon, Aug 11, 2014 at 9:51 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> Hi Olav,
>>
>> Olav Haugan <ohaugan@codeaurora.org> writes:
>>
>>> @@ -93,6 +94,10 @@ enum iommu_attr {
>>>   * @detach_dev: detach device from an iommu domain
>>>   * @map: map a physically contiguous memory region to an iommu domain
>>>   * @unmap: unmap a physically contiguous memory region from an iommu domain
>>> + * @map_sg: map a scatter-gather list of physically contiguous memory chunks
>>> + * to an iommu domain
>>> + * @unmap_sg: unmap a scatter-gather list of physically contiguous memory
>>> + * chunks from an iommu domain
>>>   * @iova_to_phys: translate iova to physical address
>>>   * @domain_has_cap: domain capabilities query
>>>   * @add_device: add device to iommu grouping
>>> @@ -110,6 +115,11 @@ struct iommu_ops {
>>>                    phys_addr_t paddr, size_t size, int prot);
>>>         size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
>>>                      size_t size);
>>> +       int (*map_sg)(struct iommu_domain *domain, unsigned long iova,
>>> +                       struct scatterlist *sg, unsigned int nents, int prot,
>>> +                       unsigned long flags);
>>> +       int (*unmap_sg)(struct iommu_domain *domain, unsigned long iova,
>>> +                       size_t size, unsigned long flags);
>>
>> Do you have any exmaple/explanation for the above "flags"?
>>
>> Is this going to be used for iommu global/standard attribute or SoC
>> spcific one?
> 
> iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
> drivers which wanted to map/unmap N buffers with a single flush at the
> end.  There might have been some other usages envisioned.
> 

Yes, that was the original intent of the flags for now. I am sure we can
find other uses for this in the future.

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[joro-zLv9SwRftAIdnm+yROfE0A]

On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
> On 8/12/2014 3:48 AM, Rob Clark wrote:
> > iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
> > drivers which wanted to map/unmap N buffers with a single flush at the
> > end.  There might have been some other usages envisioned.
> 
> Yes, that was the original intent of the flags for now. I am sure we can
> find other uses for this in the future.

Do you have anything else in mind already besides the DONT_FLUSH_TLB
flag?

How is the IOTLB supposed to be flushed when this flag is used?


	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[joro at 8bytes.org]

On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
> On 8/12/2014 3:48 AM, Rob Clark wrote:
> > iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
> > drivers which wanted to map/unmap N buffers with a single flush at the
> > end.  There might have been some other usages envisioned.
> 
> Yes, that was the original intent of the flags for now. I am sure we can
> find other uses for this in the future.

Do you have anything else in mind already besides the DONT_FLUSH_TLB
flag?

How is the IOTLB supposed to be flushed when this flag is used?


	Joerg

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Rob Clark]

On Mon, Aug 18, 2014 at 10:07 AM, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
>> On 8/12/2014 3:48 AM, Rob Clark wrote:
>> > iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
>> > drivers which wanted to map/unmap N buffers with a single flush at the
>> > end.  There might have been some other usages envisioned.
>>
>> Yes, that was the original intent of the flags for now. I am sure we can
>> find other uses for this in the future.
>
> Do you have anything else in mind already besides the DONT_FLUSH_TLB
> flag?
>
> How is the IOTLB supposed to be flushed when this flag is used?
>

well, I was thinking one of two ways:

1) add new flush() vfunc.. this, I think, would be most convenient for
drivers using this feature
2) or driver simply doesn't set DONT_FLUSH_TLB flag on the last
{map,unmap}..  that would be slightly more awkward to use, but would
avoid adding a new vfunc

BR,
-R

>         Joerg
>

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Rob Clark]

On Mon, Aug 18, 2014 at 10:07 AM, joro at 8bytes.org <joro@8bytes.org> wrote:
> On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
>> On 8/12/2014 3:48 AM, Rob Clark wrote:
>> > iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
>> > drivers which wanted to map/unmap N buffers with a single flush at the
>> > end.  There might have been some other usages envisioned.
>>
>> Yes, that was the original intent of the flags for now. I am sure we can
>> find other uses for this in the future.
>
> Do you have anything else in mind already besides the DONT_FLUSH_TLB
> flag?
>
> How is the IOTLB supposed to be flushed when this flag is used?
>

well, I was thinking one of two ways:

1) add new flush() vfunc.. this, I think, would be most convenient for
drivers using this feature
2) or driver simply doesn't set DONT_FLUSH_TLB flag on the last
{map,unmap}..  that would be slightly more awkward to use, but would
avoid adding a new vfunc

BR,
-R

>         Joerg
>

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 11:32 AM, Rob Clark wrote:
> On Mon, Aug 18, 2014 at 10:07 AM, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
>> On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
>>> On 8/12/2014 3:48 AM, Rob Clark wrote:
>>>> iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
>>>> drivers which wanted to map/unmap N buffers with a single flush at the
>>>> end.  There might have been some other usages envisioned.
>>>
>>> Yes, that was the original intent of the flags for now. I am sure we can
>>> find other uses for this in the future.
>>
>> Do you have anything else in mind already besides the DONT_FLUSH_TLB
>> flag?

No, I do not have other uses right now. But could imagine use cases like
"force <insert minimum mapping size here> mapping" flag etc.

>> How is the IOTLB supposed to be flushed when this flag is used?
>>
> 
> well, I was thinking one of two ways:
> 
> 1) add new flush() vfunc.. this, I think, would be most convenient for
> drivers using this feature
> 2) or driver simply doesn't set DONT_FLUSH_TLB flag on the last
> {map,unmap}..  that would be slightly more awkward to use, but would
> avoid adding a new vfunc
> 

I agree and would support adding a public flush() API. It is cleaner.
However, it does allow clients to chose which method to use. Either an
extra call to flush() or just do not pass DONT_FLUSH_TLB flag on the
last invocation of map/unmap.


Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 11:32 AM, Rob Clark wrote:
> On Mon, Aug 18, 2014 at 10:07 AM, joro at 8bytes.org <joro@8bytes.org> wrote:
>> On Tue, Aug 12, 2014 at 09:56:11AM -0700, Olav Haugan wrote:
>>> On 8/12/2014 3:48 AM, Rob Clark wrote:
>>>> iirc, one plan for 'flags' was some sort of DONT_FLUSH_TLB flag for
>>>> drivers which wanted to map/unmap N buffers with a single flush at the
>>>> end.  There might have been some other usages envisioned.
>>>
>>> Yes, that was the original intent of the flags for now. I am sure we can
>>> find other uses for this in the future.
>>
>> Do you have anything else in mind already besides the DONT_FLUSH_TLB
>> flag?

No, I do not have other uses right now. But could imagine use cases like
"force <insert minimum mapping size here> mapping" flag etc.

>> How is the IOTLB supposed to be flushed when this flag is used?
>>
> 
> well, I was thinking one of two ways:
> 
> 1) add new flush() vfunc.. this, I think, would be most convenient for
> drivers using this feature
> 2) or driver simply doesn't set DONT_FLUSH_TLB flag on the last
> {map,unmap}..  that would be slightly more awkward to use, but would
> avoid adding a new vfunc
> 

I agree and would support adding a public flush() API. It is cleaner.
However, it does allow clients to chose which method to use. Either an
extra call to flush() or just do not pass DONT_FLUSH_TLB flag on the
last invocation of map/unmap.


Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[joro-zLv9SwRftAIdnm+yROfE0A]

On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote:
> On 8/18/2014 11:32 AM, Rob Clark wrote:

> No, I do not have other uses right now. But could imagine use cases like
> "force <insert minimum mapping size here> mapping" flag etc.

I think it is worth discussing to add a flush() function to the
IOMMU-API. I sent a patch-set myself a few years ago introducing an
iommu_commit() function with the same semantics, but this is
out-of-scope for this patch-set.

Also adding a flag parameter to iommu_map_sg would introduce an
asymentrie in the API to the iommu_map() function. So please leave it
out for this patch-set, when we really need a flag parameter someday we
can introduce it for iommu_map() and iommu_map_sg() together.


	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[joro at 8bytes.org]

On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote:
> On 8/18/2014 11:32 AM, Rob Clark wrote:

> No, I do not have other uses right now. But could imagine use cases like
> "force <insert minimum mapping size here> mapping" flag etc.

I think it is worth discussing to add a flush() function to the
IOMMU-API. I sent a patch-set myself a few years ago introducing an
iommu_commit() function with the same semantics, but this is
out-of-scope for this patch-set.

Also adding a flag parameter to iommu_map_sg would introduce an
asymentrie in the API to the iommu_map() function. So please leave it
out for this patch-set, when we really need a flag parameter someday we
can introduce it for iommu_map() and iommu_map_sg() together.


	Joerg

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 2:26 PM, joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org wrote:
> On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote:
>> On 8/18/2014 11:32 AM, Rob Clark wrote:
> 
>> No, I do not have other uses right now. But could imagine use cases like
>> "force <insert minimum mapping size here> mapping" flag etc.
> 
> I think it is worth discussing to add a flush() function to the
> IOMMU-API. I sent a patch-set myself a few years ago introducing an
> iommu_commit() function with the same semantics, but this is
> out-of-scope for this patch-set.
> 
> Also adding a flag parameter to iommu_map_sg would introduce an
> asymentrie in the API to the iommu_map() function. So please leave it
> out for this patch-set, when we really need a flag parameter someday we
> can introduce it for iommu_map() and iommu_map_sg() together.
> 

Sure, I can remove it. I was trying to make iommu_map_sg/iommu_unmap_sg
symmetric :-)

Anything else before I push v6?


Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 2:26 PM, joro at 8bytes.org wrote:
> On Mon, Aug 18, 2014 at 01:48:46PM -0700, Olav Haugan wrote:
>> On 8/18/2014 11:32 AM, Rob Clark wrote:
> 
>> No, I do not have other uses right now. But could imagine use cases like
>> "force <insert minimum mapping size here> mapping" flag etc.
> 
> I think it is worth discussing to add a flush() function to the
> IOMMU-API. I sent a patch-set myself a few years ago introducing an
> iommu_commit() function with the same semantics, but this is
> out-of-scope for this patch-set.
> 
> Also adding a flag parameter to iommu_map_sg would introduce an
> asymentrie in the API to the iommu_map() function. So please leave it
> out for this patch-set, when we really need a flag parameter someday we
> can introduce it for iommu_map() and iommu_map_sg() together.
> 

Sure, I can remove it. I was trying to make iommu_map_sg/iommu_unmap_sg
symmetric :-)

Anything else before I push v6?


Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

Hi Olav,

Thank you for the patch.

On Monday 11 August 2014 15:45:50 Olav Haugan wrote:
> Mapping and unmapping are more often than not in the critical path.
> map_sg and unmap_sg allows IOMMU driver implementations to optimize
> the process of mapping and unmapping buffers into the IOMMU page tables.
> 
> Instead of mapping a buffer one page at a time and requiring potentially
> expensive TLB operations for each page, this function allows the driver
> to map all pages in one go and defer TLB maintenance until after all
> pages have been mapped.
> 
> Additionally, the mapping operation would be faster in general since
> clients does not have to keep calling map API over and over again for
> each physically contiguous chunk of memory that needs to be mapped to a
> virtually contiguous region.
> 
> Signed-off-by: Olav Haugan <ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c      |  2 ++
>  drivers/iommu/arm-smmu.c       |  2 ++
>  drivers/iommu/exynos-iommu.c   |  2 ++
>  drivers/iommu/intel-iommu.c    |  2 ++
>  drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
>  drivers/iommu/ipmmu-vmsa.c     |  2 ++
>  drivers/iommu/msm_iommu.c      |  2 ++
>  drivers/iommu/omap-iommu.c     |  2 ++
>  drivers/iommu/shmobile-iommu.c |  2 ++
>  drivers/iommu/tegra-smmu.c     |  2 ++
>  include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
>  11 files changed, 95 insertions(+)

[snip]

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1698360..24cf727 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
> unsigned long iova, size_t size)

[snip]

> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
> +			       size_t size, unsigned long flags)

I would have called this iommu_default_unmap_sg (and same comment for 
default_iommu_map_sg) to keep the iommu_ prefix, but that's up to you.

> +{
> +	return iommu_unmap(domain, iova, size);
> +}
> +EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);

Do you expect drivers to need to override this ? What are the use cases for 
non-default implementation of unmap_sg different than this ?

[snip]

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a52..ee106ce 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h

[snip]

> @@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct
> iommu_domain *domain, return ret;
>  }
> 
> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long
> iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

Instead of having to modify all IOMMU drivers to set the map_sg operation to 
default_iommu_map_sg, how about calling it automatically as a fallback when 
map_sg is NULL ? Something like

	if (domain->ops->map_sg)
		return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

	return default_iommu_map_sg(domain, iova, sg, nents, prot, flags);

> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}
> +

-- 
Regards,

Laurent Pinchart

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

Hi Olav,

Thank you for the patch.

On Monday 11 August 2014 15:45:50 Olav Haugan wrote:
> Mapping and unmapping are more often than not in the critical path.
> map_sg and unmap_sg allows IOMMU driver implementations to optimize
> the process of mapping and unmapping buffers into the IOMMU page tables.
> 
> Instead of mapping a buffer one page at a time and requiring potentially
> expensive TLB operations for each page, this function allows the driver
> to map all pages in one go and defer TLB maintenance until after all
> pages have been mapped.
> 
> Additionally, the mapping operation would be faster in general since
> clients does not have to keep calling map API over and over again for
> each physically contiguous chunk of memory that needs to be mapped to a
> virtually contiguous region.
> 
> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
> ---
>  drivers/iommu/amd_iommu.c      |  2 ++
>  drivers/iommu/arm-smmu.c       |  2 ++
>  drivers/iommu/exynos-iommu.c   |  2 ++
>  drivers/iommu/intel-iommu.c    |  2 ++
>  drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
>  drivers/iommu/ipmmu-vmsa.c     |  2 ++
>  drivers/iommu/msm_iommu.c      |  2 ++
>  drivers/iommu/omap-iommu.c     |  2 ++
>  drivers/iommu/shmobile-iommu.c |  2 ++
>  drivers/iommu/tegra-smmu.c     |  2 ++
>  include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
>  11 files changed, 95 insertions(+)

[snip]

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 1698360..24cf727 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
> unsigned long iova, size_t size)

[snip]

> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
> +			       size_t size, unsigned long flags)

I would have called this iommu_default_unmap_sg (and same comment for 
default_iommu_map_sg) to keep the iommu_ prefix, but that's up to you.

> +{
> +	return iommu_unmap(domain, iova, size);
> +}
> +EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);

Do you expect drivers to need to override this ? What are the use cases for 
non-default implementation of unmap_sg different than this ?

[snip]

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a52..ee106ce 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h

[snip]

> @@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct
> iommu_domain *domain, return ret;
>  }
> 
> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long
> iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

Instead of having to modify all IOMMU drivers to set the map_sg operation to 
default_iommu_map_sg, how about calling it automatically as a fallback when 
map_sg is NULL ? Something like

	if (domain->ops->map_sg)
		return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);

	return default_iommu_map_sg(domain, iova, sg, nents, prot, flags);

> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}
> +

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

Hi Laurent,

On 8/12/2014 9:55 AM, Laurent Pinchart wrote:
> Hi Olav,
> 
> Thank you for the patch.
> 
> On Monday 11 August 2014 15:45:50 Olav Haugan wrote:
>> Mapping and unmapping are more often than not in the critical path.
>> map_sg and unmap_sg allows IOMMU driver implementations to optimize
>> the process of mapping and unmapping buffers into the IOMMU page tables.
>>
>> Instead of mapping a buffer one page at a time and requiring potentially
>> expensive TLB operations for each page, this function allows the driver
>> to map all pages in one go and defer TLB maintenance until after all
>> pages have been mapped.
>>
>> Additionally, the mapping operation would be faster in general since
>> clients does not have to keep calling map API over and over again for
>> each physically contiguous chunk of memory that needs to be mapped to a
>> virtually contiguous region.
>>
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>  drivers/iommu/amd_iommu.c      |  2 ++
>>  drivers/iommu/arm-smmu.c       |  2 ++
>>  drivers/iommu/exynos-iommu.c   |  2 ++
>>  drivers/iommu/intel-iommu.c    |  2 ++
>>  drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
>>  drivers/iommu/ipmmu-vmsa.c     |  2 ++
>>  drivers/iommu/msm_iommu.c      |  2 ++
>>  drivers/iommu/omap-iommu.c     |  2 ++
>>  drivers/iommu/shmobile-iommu.c |  2 ++
>>  drivers/iommu/tegra-smmu.c     |  2 ++
>>  include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
>>  11 files changed, 95 insertions(+)
> 
> [snip]
> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 1698360..24cf727 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
>> unsigned long iova, size_t size)
> 
> [snip]
> 
>> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       size_t size, unsigned long flags)
> 
> I would have called this iommu_default_unmap_sg (and same comment for 
> default_iommu_map_sg) to keep the iommu_ prefix, but that's up to you.
> 
>> +{
>> +	return iommu_unmap(domain, iova, size);
>> +}
>> +EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);
> 
> Do you expect drivers to need to override this ? What are the use cases for 
> non-default implementation of unmap_sg different than this ?

Good question. Yes, maybe some drivers does not need or want to override
this but a use case is to provide a more optimized version of the
map_sg/unmap_sg functions. For example a very simple way to optimize
this would be to have an implementation that unmaps everything and then
does a TLB invalidate instead of doing a TLB invalidate after every
single unmap (which happens with the default implementation if your
driver does TLB invalidate after unmapping).

> [snip]
> 
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 20f9a52..ee106ce 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
> 
> [snip]
> 
>> @@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct
>> iommu_domain *domain, return ret;
>>  }
>>
>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long
>> iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> 
> Instead of having to modify all IOMMU drivers to set the map_sg operation to 
> default_iommu_map_sg, how about calling it automatically as a fallback when 
> map_sg is NULL ? Something like
> 
> 	if (domain->ops->map_sg)
> 		return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> 
> 	return default_iommu_map_sg(domain, iova, sg, nents, prot, flags);

This was my original proposal but after some discussion on the list we
ended up with what we have now.

> 
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
>> +
> 

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

Hi Laurent,

On 8/12/2014 9:55 AM, Laurent Pinchart wrote:
> Hi Olav,
> 
> Thank you for the patch.
> 
> On Monday 11 August 2014 15:45:50 Olav Haugan wrote:
>> Mapping and unmapping are more often than not in the critical path.
>> map_sg and unmap_sg allows IOMMU driver implementations to optimize
>> the process of mapping and unmapping buffers into the IOMMU page tables.
>>
>> Instead of mapping a buffer one page at a time and requiring potentially
>> expensive TLB operations for each page, this function allows the driver
>> to map all pages in one go and defer TLB maintenance until after all
>> pages have been mapped.
>>
>> Additionally, the mapping operation would be faster in general since
>> clients does not have to keep calling map API over and over again for
>> each physically contiguous chunk of memory that needs to be mapped to a
>> virtually contiguous region.
>>
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>  drivers/iommu/amd_iommu.c      |  2 ++
>>  drivers/iommu/arm-smmu.c       |  2 ++
>>  drivers/iommu/exynos-iommu.c   |  2 ++
>>  drivers/iommu/intel-iommu.c    |  2 ++
>>  drivers/iommu/iommu.c          | 33 +++++++++++++++++++++++++++++++
>>  drivers/iommu/ipmmu-vmsa.c     |  2 ++
>>  drivers/iommu/msm_iommu.c      |  2 ++
>>  drivers/iommu/omap-iommu.c     |  2 ++
>>  drivers/iommu/shmobile-iommu.c |  2 ++
>>  drivers/iommu/tegra-smmu.c     |  2 ++
>>  include/linux/iommu.h          | 44 +++++++++++++++++++++++++++++++++++++++
>>  11 files changed, 95 insertions(+)
> 
> [snip]
> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 1698360..24cf727 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1088,6 +1088,39 @@ size_t iommu_unmap(struct iommu_domain *domain,
>> unsigned long iova, size_t size)
> 
> [snip]
> 
>> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       size_t size, unsigned long flags)
> 
> I would have called this iommu_default_unmap_sg (and same comment for 
> default_iommu_map_sg) to keep the iommu_ prefix, but that's up to you.
> 
>> +{
>> +	return iommu_unmap(domain, iova, size);
>> +}
>> +EXPORT_SYMBOL_GPL(default_iommu_unmap_sg);
> 
> Do you expect drivers to need to override this ? What are the use cases for 
> non-default implementation of unmap_sg different than this ?

Good question. Yes, maybe some drivers does not need or want to override
this but a use case is to provide a more optimized version of the
map_sg/unmap_sg functions. For example a very simple way to optimize
this would be to have an implementation that unmaps everything and then
does a TLB invalidate instead of doing a TLB invalidate after every
single unmap (which happens with the default implementation if your
driver does TLB invalidate after unmapping).

> [snip]
> 
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 20f9a52..ee106ce 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
> 
> [snip]
> 
>> @@ -240,6 +256,20 @@ static inline int report_iommu_fault(struct
>> iommu_domain *domain, return ret;
>>  }
>>
>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long
>> iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> 
> Instead of having to modify all IOMMU drivers to set the map_sg operation to 
> default_iommu_map_sg, how about calling it automatically as a fallback when 
> map_sg is NULL ? Something like
> 
> 	if (domain->ops->map_sg)
> 		return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> 
> 	return default_iommu_map_sg(domain, iova, sg, nents, prot, flags);

This was my original proposal but after some discussion on the list we
ended up with what we have now.

> 
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
>> +
> 

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> +int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			 struct scatterlist *sg, unsigned int nents,
> +			 int prot, unsigned long flags)
> +{
> +	int ret = 0;
> +	unsigned long offset = 0;
> +	unsigned int i;
> +	struct scatterlist *s;
> +
> +	for_each_sg(sg, s, nents, i) {
> +		phys_addr_t phys = page_to_phys(sg_page(s));
> +		size_t page_len = s->offset + s->length;
> +
> +		ret = iommu_map(domain, iova + offset, phys, page_len,
> +				prot);

This isn't going to work, iova + offset, phys and page_len need to be
aligned to the minimum page-size the given IOMMU implementation
supports. See the iommu_map implementation for details.

> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
> +			       size_t size, unsigned long flags)

Another asymmentry here, why don't you just pass a scatterlist and nents
like in the map_sg function? if you implement it like this it is just a
duplication of iommu_unmap().

> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}

These function pointers need to be checked for != NULL before calling
them.


	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> +int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			 struct scatterlist *sg, unsigned int nents,
> +			 int prot, unsigned long flags)
> +{
> +	int ret = 0;
> +	unsigned long offset = 0;
> +	unsigned int i;
> +	struct scatterlist *s;
> +
> +	for_each_sg(sg, s, nents, i) {
> +		phys_addr_t phys = page_to_phys(sg_page(s));
> +		size_t page_len = s->offset + s->length;
> +
> +		ret = iommu_map(domain, iova + offset, phys, page_len,
> +				prot);

This isn't going to work, iova + offset, phys and page_len need to be
aligned to the minimum page-size the given IOMMU implementation
supports. See the iommu_map implementation for details.

> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
> +			       size_t size, unsigned long flags)

Another asymmentry here, why don't you just pass a scatterlist and nents
like in the map_sg function? if you implement it like this it is just a
duplication of iommu_unmap().

> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}

These function pointers need to be checked for != NULL before calling
them.


	Joerg

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 2:55 PM, Joerg Roedel wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>> +int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			 struct scatterlist *sg, unsigned int nents,
>> +			 int prot, unsigned long flags)
>> +{
>> +	int ret = 0;
>> +	unsigned long offset = 0;
>> +	unsigned int i;
>> +	struct scatterlist *s;
>> +
>> +	for_each_sg(sg, s, nents, i) {
>> +		phys_addr_t phys = page_to_phys(sg_page(s));
>> +		size_t page_len = s->offset + s->length;
>> +
>> +		ret = iommu_map(domain, iova + offset, phys, page_len,
>> +				prot);
> 
> This isn't going to work, iova + offset, phys and page_len need to be
> aligned to the minimum page-size the given IOMMU implementation
> supports. See the iommu_map implementation for details.

If the alignment is not correct then iommu_map() will return error. Not
sure what other option we have here (and why make it different behavior
than iommu_map which just return error when it is not aligned properly).
I don't think we want to force any kind of alignment automatically. I
would rather have the API tell me I am doing something wrong than having
the function aligning the values and possibly undermap or overmap.

>> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       size_t size, unsigned long flags)
> 
> Another asymmentry here, why don't you just pass a scatterlist and nents
> like in the map_sg function? if you implement it like this it is just a
> duplication of iommu_unmap().

Yes, I am aware of that. However, several people prefer this than
passing in scatterlist. It is not very convenient to pass a scatterlist
in some use cases. Someone mentioned a use case where they would have to
create a dummy sg list and populate it with the iova just to do an
unmap. I believe we would have to do this also. There is no use for
sglist when unmapping. However, would like to keep separate API from
iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
> 
> These function pointers need to be checked for != NULL before calling
> them.

I thought that was why we added the default fallback and set all the
drivers to point to these fallback functions. Several people wanted this
so that we don't have to have NULL-check in these functions (and have
the functions be simple inline functions).

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/18/2014 2:55 PM, Joerg Roedel wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>> +int default_iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			 struct scatterlist *sg, unsigned int nents,
>> +			 int prot, unsigned long flags)
>> +{
>> +	int ret = 0;
>> +	unsigned long offset = 0;
>> +	unsigned int i;
>> +	struct scatterlist *s;
>> +
>> +	for_each_sg(sg, s, nents, i) {
>> +		phys_addr_t phys = page_to_phys(sg_page(s));
>> +		size_t page_len = s->offset + s->length;
>> +
>> +		ret = iommu_map(domain, iova + offset, phys, page_len,
>> +				prot);
> 
> This isn't going to work, iova + offset, phys and page_len need to be
> aligned to the minimum page-size the given IOMMU implementation
> supports. See the iommu_map implementation for details.

If the alignment is not correct then iommu_map() will return error. Not
sure what other option we have here (and why make it different behavior
than iommu_map which just return error when it is not aligned properly).
I don't think we want to force any kind of alignment automatically. I
would rather have the API tell me I am doing something wrong than having
the function aligning the values and possibly undermap or overmap.

>> +int default_iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       size_t size, unsigned long flags)
> 
> Another asymmentry here, why don't you just pass a scatterlist and nents
> like in the map_sg function? if you implement it like this it is just a
> duplication of iommu_unmap().

Yes, I am aware of that. However, several people prefer this than
passing in scatterlist. It is not very convenient to pass a scatterlist
in some use cases. Someone mentioned a use case where they would have to
create a dummy sg list and populate it with the iova just to do an
unmap. I believe we would have to do this also. There is no use for
sglist when unmapping. However, would like to keep separate API from
iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
> 
> These function pointers need to be checked for != NULL before calling
> them.

I thought that was why we added the default fallback and set all the
drivers to point to these fallback functions. Several people wanted this
so that we don't have to have NULL-check in these functions (and have
the functions be simple inline functions).

Thanks,

Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> If the alignment is not correct then iommu_map() will return error. Not
> sure what other option we have here (and why make it different behavior
> than iommu_map which just return error when it is not aligned properly).
> I don't think we want to force any kind of alignment automatically. I
> would rather have the API tell me I am doing something wrong than having
> the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way
in the DMA-API and since you do 'page_len = s->offset + s->length' you
use it the same way).
So when you pass iova + offset the result will no longer be
page-aligned. You should force sg->offset == 0 and sg->length to be
page-aligned instead. This makes more sense because the IOMMU-API works
on (io)-page granularity and not on arbitrary phys-addr ranges like the
DMA-API.

> Yes, I am aware of that. However, several people prefer this than
> passing in scatterlist. It is not very convenient to pass a scatterlist
> in some use cases. Someone mentioned a use case where they would have to
> create a dummy sg list and populate it with the iova just to do an
> unmap. I believe we would have to do this also. There is no use for
> sglist when unmapping. However, would like to keep separate API from
> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Keeping it symetric is not more complicated, the caller just needs to
keep the sg-list used for mapping around. I prefer the unmap_sg call to
work in sg-lists too.

> I thought that was why we added the default fallback and set all the
> drivers to point to these fallback functions. Several people wanted this
> so that we don't have to have NULL-check in these functions (and have
> the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live
with not doing a pointer check here.


	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> If the alignment is not correct then iommu_map() will return error. Not
> sure what other option we have here (and why make it different behavior
> than iommu_map which just return error when it is not aligned properly).
> I don't think we want to force any kind of alignment automatically. I
> would rather have the API tell me I am doing something wrong than having
> the function aligning the values and possibly undermap or overmap.

But sg->offset is an offset into the page (at least it is used that way
in the DMA-API and since you do 'page_len = s->offset + s->length' you
use it the same way).
So when you pass iova + offset the result will no longer be
page-aligned. You should force sg->offset == 0 and sg->length to be
page-aligned instead. This makes more sense because the IOMMU-API works
on (io)-page granularity and not on arbitrary phys-addr ranges like the
DMA-API.

> Yes, I am aware of that. However, several people prefer this than
> passing in scatterlist. It is not very convenient to pass a scatterlist
> in some use cases. Someone mentioned a use case where they would have to
> create a dummy sg list and populate it with the iova just to do an
> unmap. I believe we would have to do this also. There is no use for
> sglist when unmapping. However, would like to keep separate API from
> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).

Keeping it symetric is not more complicated, the caller just needs to
keep the sg-list used for mapping around. I prefer the unmap_sg call to
work in sg-lists too.

> I thought that was why we added the default fallback and set all the
> drivers to point to these fallback functions. Several people wanted this
> so that we don't have to have NULL-check in these functions (and have
> the functions be simple inline functions).

Okay, since you add these call-backs to all drivers I think I can live
with not doing a pointer check here.


	Joerg

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > If the alignment is not correct then iommu_map() will return error. Not
> > sure what other option we have here (and why make it different behavior
> > than iommu_map which just return error when it is not aligned properly).
> > I don't think we want to force any kind of alignment automatically. I
> > would rather have the API tell me I am doing something wrong than having
> > the function aligning the values and possibly undermap or overmap.
> 
> But sg->offset is an offset into the page (at least it is used that way
> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> use it the same way).
> So when you pass iova + offset the result will no longer be
> page-aligned. You should force sg->offset == 0 and sg->length to be
> page-aligned instead. This makes more sense because the IOMMU-API works
> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> DMA-API.
> 
> > Yes, I am aware of that. However, several people prefer this than
> > passing in scatterlist. It is not very convenient to pass a scatterlist
> > in some use cases. Someone mentioned a use case where they would have to
> > create a dummy sg list and populate it with the iova just to do an
> > unmap. I believe we would have to do this also. There is no use for
> > sglist when unmapping. However, would like to keep separate API from
> > iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
> 
> Keeping it symetric is not more complicated, the caller just needs to
> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different 
than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() 
completely.

> > I thought that was why we added the default fallback and set all the
> > drivers to point to these fallback functions. Several people wanted this
> > so that we don't have to have NULL-check in these functions (and have
> > the functions be simple inline functions).
> 
> Okay, since you add these call-backs to all drivers I think I can live
> with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)
	return ops();
else
	return default_ops();

to avoid modifying all drivers. I'm not sure why that wasn't received with 
much enthusiasm.

-- 
Regards,

Laurent Pinchart

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > If the alignment is not correct then iommu_map() will return error. Not
> > sure what other option we have here (and why make it different behavior
> > than iommu_map which just return error when it is not aligned properly).
> > I don't think we want to force any kind of alignment automatically. I
> > would rather have the API tell me I am doing something wrong than having
> > the function aligning the values and possibly undermap or overmap.
> 
> But sg->offset is an offset into the page (at least it is used that way
> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> use it the same way).
> So when you pass iova + offset the result will no longer be
> page-aligned. You should force sg->offset == 0 and sg->length to be
> page-aligned instead. This makes more sense because the IOMMU-API works
> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> DMA-API.
> 
> > Yes, I am aware of that. However, several people prefer this than
> > passing in scatterlist. It is not very convenient to pass a scatterlist
> > in some use cases. Someone mentioned a use case where they would have to
> > create a dummy sg list and populate it with the iova just to do an
> > unmap. I believe we would have to do this also. There is no use for
> > sglist when unmapping. However, would like to keep separate API from
> > iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
> 
> Keeping it symetric is not more complicated, the caller just needs to
> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> work in sg-lists too.

Do we have a use case where the unmap_sg() implementation would be different 
than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() 
completely.

> > I thought that was why we added the default fallback and set all the
> > drivers to point to these fallback functions. Several people wanted this
> > so that we don't have to have NULL-check in these functions (and have
> > the functions be simple inline functions).
> 
> Okay, since you add these call-backs to all drivers I think I can live
> with not doing a pointer check here.

I suggested doing a

if (ops is not NULL)
	return ops();
else
	return default_ops();

to avoid modifying all drivers. I'm not sure why that wasn't received with 
much enthusiasm.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
>> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
>>> If the alignment is not correct then iommu_map() will return error. Not
>>> sure what other option we have here (and why make it different behavior
>>> than iommu_map which just return error when it is not aligned properly).
>>> I don't think we want to force any kind of alignment automatically. I
>>> would rather have the API tell me I am doing something wrong than having
>>> the function aligning the values and possibly undermap or overmap.
>>
>> But sg->offset is an offset into the page (at least it is used that way
>> in the DMA-API and since you do 'page_len = s->offset + s->length' you
>> use it the same way).
>> So when you pass iova + offset the result will no longer be
>> page-aligned. You should force sg->offset == 0 and sg->length to be
>> page-aligned instead. This makes more sense because the IOMMU-API works
>> on (io)-page granularity and not on arbitrary phys-addr ranges like the
>> DMA-API.
>>
>>> Yes, I am aware of that. However, several people prefer this than
>>> passing in scatterlist. It is not very convenient to pass a scatterlist
>>> in some use cases. Someone mentioned a use case where they would have to
>>> create a dummy sg list and populate it with the iova just to do an
>>> unmap. I believe we would have to do this also. There is no use for
>>> sglist when unmapping. However, would like to keep separate API from
>>> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
>>
>> Keeping it symetric is not more complicated, the caller just needs to
>> keep the sg-list used for mapping around. I prefer the unmap_sg call to
>> work in sg-lists too.
> 
> Do we have a use case where the unmap_sg() implementation would be different 
> than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() 
> completely.
> 
>>> I thought that was why we added the default fallback and set all the
>>> drivers to point to these fallback functions. Several people wanted this
>>> so that we don't have to have NULL-check in these functions (and have
>>> the functions be simple inline functions).
>>
>> Okay, since you add these call-backs to all drivers I think I can live
>> with not doing a pointer check here.
> 
> I suggested doing a
> 
> if (ops is not NULL)
> 	return ops();
> else
> 	return default_ops();
> 
> to avoid modifying all drivers. I'm not sure why that wasn't received with 
> much enthusiasm.
> 

Both Thierry R. and Konrad W. argued for modifying the drivers instead
so I implemented what the majority wanted. :-)


Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
>> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
>>> If the alignment is not correct then iommu_map() will return error. Not
>>> sure what other option we have here (and why make it different behavior
>>> than iommu_map which just return error when it is not aligned properly).
>>> I don't think we want to force any kind of alignment automatically. I
>>> would rather have the API tell me I am doing something wrong than having
>>> the function aligning the values and possibly undermap or overmap.
>>
>> But sg->offset is an offset into the page (at least it is used that way
>> in the DMA-API and since you do 'page_len = s->offset + s->length' you
>> use it the same way).
>> So when you pass iova + offset the result will no longer be
>> page-aligned. You should force sg->offset == 0 and sg->length to be
>> page-aligned instead. This makes more sense because the IOMMU-API works
>> on (io)-page granularity and not on arbitrary phys-addr ranges like the
>> DMA-API.
>>
>>> Yes, I am aware of that. However, several people prefer this than
>>> passing in scatterlist. It is not very convenient to pass a scatterlist
>>> in some use cases. Someone mentioned a use case where they would have to
>>> create a dummy sg list and populate it with the iova just to do an
>>> unmap. I believe we would have to do this also. There is no use for
>>> sglist when unmapping. However, would like to keep separate API from
>>> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
>>
>> Keeping it symetric is not more complicated, the caller just needs to
>> keep the sg-list used for mapping around. I prefer the unmap_sg call to
>> work in sg-lists too.
> 
> Do we have a use case where the unmap_sg() implementation would be different 
> than a plain iommu_unmap() call ? If not I'd rather remove unmap_sg() 
> completely.
> 
>>> I thought that was why we added the default fallback and set all the
>>> drivers to point to these fallback functions. Several people wanted this
>>> so that we don't have to have NULL-check in these functions (and have
>>> the functions be simple inline functions).
>>
>> Okay, since you add these call-backs to all drivers I think I can live
>> with not doing a pointer check here.
> 
> I suggested doing a
> 
> if (ops is not NULL)
> 	return ops();
> else
> 	return default_ops();
> 
> to avoid modifying all drivers. I'm not sure why that wasn't received with 
> much enthusiasm.
> 

Both Thierry R. and Konrad W. argued for modifying the drivers instead
so I implemented what the majority wanted. :-)


Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> >>> If the alignment is not correct then iommu_map() will return error. Not
> >>> sure what other option we have here (and why make it different behavior
> >>> than iommu_map which just return error when it is not aligned properly).
> >>> I don't think we want to force any kind of alignment automatically. I
> >>> would rather have the API tell me I am doing something wrong than having
> >>> the function aligning the values and possibly undermap or overmap.
> >> 
> >> But sg->offset is an offset into the page (at least it is used that way
> >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> >> use it the same way).
> >> So when you pass iova + offset the result will no longer be
> >> page-aligned. You should force sg->offset == 0 and sg->length to be
> >> page-aligned instead. This makes more sense because the IOMMU-API works
> >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> >> DMA-API.
> >> 
> >>> Yes, I am aware of that. However, several people prefer this than
> >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> >>> in some use cases. Someone mentioned a use case where they would have to
> >>> create a dummy sg list and populate it with the iova just to do an
> >>> unmap. I believe we would have to do this also. There is no use for
> >>> sglist when unmapping. However, would like to keep separate API from
> >>> iommu_unmap() to keep the API function names symmetric
> >>> (map_sg/unmap_sg).
> >> 
> >> Keeping it symetric is not more complicated, the caller just needs to
> >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> >> work in sg-lists too.
> > 
> > Do we have a use case where the unmap_sg() implementation would be
> > different than a plain iommu_unmap() call ? If not I'd rather remove
> > unmap_sg() completely.
> > 
> >>> I thought that was why we added the default fallback and set all the
> >>> drivers to point to these fallback functions. Several people wanted this
> >>> so that we don't have to have NULL-check in these functions (and have
> >>> the functions be simple inline functions).
> >> 
> >> Okay, since you add these call-backs to all drivers I think I can live
> >> with not doing a pointer check here.
> > 
> > I suggested doing a
> > 
> > if (ops is not NULL)
> > 
> > 	return ops();
> > 
> > else
> > 
> > 	return default_ops();
> > 
> > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > much enthusiasm.
> 
> Both Thierry R. and Konrad W. argued for modifying the drivers instead
> so I implemented what the majority wanted. :-)

I'm not blaming you :-) I was just wondering what their rationale was.

-- 
Regards,

Laurent Pinchart

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> >>> If the alignment is not correct then iommu_map() will return error. Not
> >>> sure what other option we have here (and why make it different behavior
> >>> than iommu_map which just return error when it is not aligned properly).
> >>> I don't think we want to force any kind of alignment automatically. I
> >>> would rather have the API tell me I am doing something wrong than having
> >>> the function aligning the values and possibly undermap or overmap.
> >> 
> >> But sg->offset is an offset into the page (at least it is used that way
> >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> >> use it the same way).
> >> So when you pass iova + offset the result will no longer be
> >> page-aligned. You should force sg->offset == 0 and sg->length to be
> >> page-aligned instead. This makes more sense because the IOMMU-API works
> >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> >> DMA-API.
> >> 
> >>> Yes, I am aware of that. However, several people prefer this than
> >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> >>> in some use cases. Someone mentioned a use case where they would have to
> >>> create a dummy sg list and populate it with the iova just to do an
> >>> unmap. I believe we would have to do this also. There is no use for
> >>> sglist when unmapping. However, would like to keep separate API from
> >>> iommu_unmap() to keep the API function names symmetric
> >>> (map_sg/unmap_sg).
> >> 
> >> Keeping it symetric is not more complicated, the caller just needs to
> >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> >> work in sg-lists too.
> > 
> > Do we have a use case where the unmap_sg() implementation would be
> > different than a plain iommu_unmap() call ? If not I'd rather remove
> > unmap_sg() completely.
> > 
> >>> I thought that was why we added the default fallback and set all the
> >>> drivers to point to these fallback functions. Several people wanted this
> >>> so that we don't have to have NULL-check in these functions (and have
> >>> the functions be simple inline functions).
> >> 
> >> Okay, since you add these call-backs to all drivers I think I can live
> >> with not doing a pointer check here.
> > 
> > I suggested doing a
> > 
> > if (ops is not NULL)
> > 
> > 	return ops();
> > 
> > else
> > 
> > 	return default_ops();
> > 
> > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > much enthusiasm.
> 
> Both Thierry R. and Konrad W. argued for modifying the drivers instead
> so I implemented what the majority wanted. :-)

I'm not blaming you :-) I was just wondering what their rationale was.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3854 bytes --]

On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > >>> If the alignment is not correct then iommu_map() will return error. Not
> > >>> sure what other option we have here (and why make it different behavior
> > >>> than iommu_map which just return error when it is not aligned properly).
> > >>> I don't think we want to force any kind of alignment automatically. I
> > >>> would rather have the API tell me I am doing something wrong than having
> > >>> the function aligning the values and possibly undermap or overmap.
> > >> 
> > >> But sg->offset is an offset into the page (at least it is used that way
> > >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> > >> use it the same way).
> > >> So when you pass iova + offset the result will no longer be
> > >> page-aligned. You should force sg->offset == 0 and sg->length to be
> > >> page-aligned instead. This makes more sense because the IOMMU-API works
> > >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> > >> DMA-API.
> > >> 
> > >>> Yes, I am aware of that. However, several people prefer this than
> > >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> > >>> in some use cases. Someone mentioned a use case where they would have to
> > >>> create a dummy sg list and populate it with the iova just to do an
> > >>> unmap. I believe we would have to do this also. There is no use for
> > >>> sglist when unmapping. However, would like to keep separate API from
> > >>> iommu_unmap() to keep the API function names symmetric
> > >>> (map_sg/unmap_sg).
> > >> 
> > >> Keeping it symetric is not more complicated, the caller just needs to
> > >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> > >> work in sg-lists too.
> > > 
> > > Do we have a use case where the unmap_sg() implementation would be
> > > different than a plain iommu_unmap() call ? If not I'd rather remove
> > > unmap_sg() completely.
> > > 
> > >>> I thought that was why we added the default fallback and set all the
> > >>> drivers to point to these fallback functions. Several people wanted this
> > >>> so that we don't have to have NULL-check in these functions (and have
> > >>> the functions be simple inline functions).
> > >> 
> > >> Okay, since you add these call-backs to all drivers I think I can live
> > >> with not doing a pointer check here.
> > > 
> > > I suggested doing a
> > > 
> > > if (ops is not NULL)
> > > 
> > > 	return ops();
> > > 
> > > else
> > > 
> > > 	return default_ops();
> > > 
> > > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > > much enthusiasm.
> > 
> > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > so I implemented what the majority wanted. :-)
> 
> I'm not blaming you :-) I was just wondering what their rationale was.

In my opinion it's much more direct that way. It means that if a driver
doesn't implement it, it won't fall back to some default implementation
instead. Providing an explicit helper like this makes it obvious that
the driver is using a default implementation rather than making things
work "magically". It's easier to see in the driver that there's the
potential to optimize.

It also has the side-effect of keeping the core code cleaner in my
opinion, since the core iommu_map_sg() and iommu_unmap_sg() functions
can now blindly call into drivers directly rather than performing the
various checks to see if they implement the required functionality.

Thierry

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

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Thierry Reding]

On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > >>> If the alignment is not correct then iommu_map() will return error. Not
> > >>> sure what other option we have here (and why make it different behavior
> > >>> than iommu_map which just return error when it is not aligned properly).
> > >>> I don't think we want to force any kind of alignment automatically. I
> > >>> would rather have the API tell me I am doing something wrong than having
> > >>> the function aligning the values and possibly undermap or overmap.
> > >> 
> > >> But sg->offset is an offset into the page (at least it is used that way
> > >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> > >> use it the same way).
> > >> So when you pass iova + offset the result will no longer be
> > >> page-aligned. You should force sg->offset == 0 and sg->length to be
> > >> page-aligned instead. This makes more sense because the IOMMU-API works
> > >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> > >> DMA-API.
> > >> 
> > >>> Yes, I am aware of that. However, several people prefer this than
> > >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> > >>> in some use cases. Someone mentioned a use case where they would have to
> > >>> create a dummy sg list and populate it with the iova just to do an
> > >>> unmap. I believe we would have to do this also. There is no use for
> > >>> sglist when unmapping. However, would like to keep separate API from
> > >>> iommu_unmap() to keep the API function names symmetric
> > >>> (map_sg/unmap_sg).
> > >> 
> > >> Keeping it symetric is not more complicated, the caller just needs to
> > >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> > >> work in sg-lists too.
> > > 
> > > Do we have a use case where the unmap_sg() implementation would be
> > > different than a plain iommu_unmap() call ? If not I'd rather remove
> > > unmap_sg() completely.
> > > 
> > >>> I thought that was why we added the default fallback and set all the
> > >>> drivers to point to these fallback functions. Several people wanted this
> > >>> so that we don't have to have NULL-check in these functions (and have
> > >>> the functions be simple inline functions).
> > >> 
> > >> Okay, since you add these call-backs to all drivers I think I can live
> > >> with not doing a pointer check here.
> > > 
> > > I suggested doing a
> > > 
> > > if (ops is not NULL)
> > > 
> > > 	return ops();
> > > 
> > > else
> > > 
> > > 	return default_ops();
> > > 
> > > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > > much enthusiasm.
> > 
> > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > so I implemented what the majority wanted. :-)
> 
> I'm not blaming you :-) I was just wondering what their rationale was.

In my opinion it's much more direct that way. It means that if a driver
doesn't implement it, it won't fall back to some default implementation
instead. Providing an explicit helper like this makes it obvious that
the driver is using a default implementation rather than making things
work "magically". It's easier to see in the driver that there's the
potential to optimize.

It also has the side-effect of keeping the core code cleaner in my
opinion, since the core iommu_map_sg() and iommu_unmap_sg() functions
can now blindly call into drivers directly rather than performing the
various checks to see if they implement the required functionality.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/192e9789/attachment.sig>

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Konrad Rzeszutek Wilk]

On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > >>> If the alignment is not correct then iommu_map() will return error. Not
> > >>> sure what other option we have here (and why make it different behavior
> > >>> than iommu_map which just return error when it is not aligned properly).
> > >>> I don't think we want to force any kind of alignment automatically. I
> > >>> would rather have the API tell me I am doing something wrong than having
> > >>> the function aligning the values and possibly undermap or overmap.
> > >> 
> > >> But sg->offset is an offset into the page (at least it is used that way
> > >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> > >> use it the same way).
> > >> So when you pass iova + offset the result will no longer be
> > >> page-aligned. You should force sg->offset == 0 and sg->length to be
> > >> page-aligned instead. This makes more sense because the IOMMU-API works
> > >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> > >> DMA-API.
> > >> 
> > >>> Yes, I am aware of that. However, several people prefer this than
> > >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> > >>> in some use cases. Someone mentioned a use case where they would have to
> > >>> create a dummy sg list and populate it with the iova just to do an
> > >>> unmap. I believe we would have to do this also. There is no use for
> > >>> sglist when unmapping. However, would like to keep separate API from
> > >>> iommu_unmap() to keep the API function names symmetric
> > >>> (map_sg/unmap_sg).
> > >> 
> > >> Keeping it symetric is not more complicated, the caller just needs to
> > >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> > >> work in sg-lists too.
> > > 
> > > Do we have a use case where the unmap_sg() implementation would be
> > > different than a plain iommu_unmap() call ? If not I'd rather remove
> > > unmap_sg() completely.
> > > 
> > >>> I thought that was why we added the default fallback and set all the
> > >>> drivers to point to these fallback functions. Several people wanted this
> > >>> so that we don't have to have NULL-check in these functions (and have
> > >>> the functions be simple inline functions).
> > >> 
> > >> Okay, since you add these call-backs to all drivers I think I can live
> > >> with not doing a pointer check here.
> > > 
> > > I suggested doing a
> > > 
> > > if (ops is not NULL)
> > > 
> > > 	return ops();
> > > 
> > > else
> > > 
> > > 	return default_ops();
> > > 
> > > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > > much enthusiasm.
> > 
> > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > so I implemented what the majority wanted. :-)
> 
> I'm not blaming you :-) I was just wondering what their rationale was.


a). To be inline with the usage of this API.

    For this particular case the other functions (but maybe I missed some)
    follow the idea that they implement the ops->func for every operation
    and they don't have an fallback.

b)  Follow what x86 maintainers prefer (based on other API calls,
    such as x86_init or any other platform overrides).

c). When there are new IOMMUs it has to take in-to account all of the
    function ops. If they fail to implement all of them the kernel
    crashes instead of working (but maybe working incorrectly).

d). Lastly, we also want the bloat of kernel. If the compiler decides
    to roll in the fallback implementation for 'iommu_map_sg' in - it will
    needlessly expand the kernel wherever we use 'iommu_map_sg' call with
    a fallback implementation (which might not be called 99% of time).

    Having the little inline static just do 'func_ops->func' will
    stop the compiler from optimizing too much and convert this just
    to a simple function.

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Konrad Rzeszutek Wilk]

On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:
> > >> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
> > >>> If the alignment is not correct then iommu_map() will return error. Not
> > >>> sure what other option we have here (and why make it different behavior
> > >>> than iommu_map which just return error when it is not aligned properly).
> > >>> I don't think we want to force any kind of alignment automatically. I
> > >>> would rather have the API tell me I am doing something wrong than having
> > >>> the function aligning the values and possibly undermap or overmap.
> > >> 
> > >> But sg->offset is an offset into the page (at least it is used that way
> > >> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> > >> use it the same way).
> > >> So when you pass iova + offset the result will no longer be
> > >> page-aligned. You should force sg->offset == 0 and sg->length to be
> > >> page-aligned instead. This makes more sense because the IOMMU-API works
> > >> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> > >> DMA-API.
> > >> 
> > >>> Yes, I am aware of that. However, several people prefer this than
> > >>> passing in scatterlist. It is not very convenient to pass a scatterlist
> > >>> in some use cases. Someone mentioned a use case where they would have to
> > >>> create a dummy sg list and populate it with the iova just to do an
> > >>> unmap. I believe we would have to do this also. There is no use for
> > >>> sglist when unmapping. However, would like to keep separate API from
> > >>> iommu_unmap() to keep the API function names symmetric
> > >>> (map_sg/unmap_sg).
> > >> 
> > >> Keeping it symetric is not more complicated, the caller just needs to
> > >> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> > >> work in sg-lists too.
> > > 
> > > Do we have a use case where the unmap_sg() implementation would be
> > > different than a plain iommu_unmap() call ? If not I'd rather remove
> > > unmap_sg() completely.
> > > 
> > >>> I thought that was why we added the default fallback and set all the
> > >>> drivers to point to these fallback functions. Several people wanted this
> > >>> so that we don't have to have NULL-check in these functions (and have
> > >>> the functions be simple inline functions).
> > >> 
> > >> Okay, since you add these call-backs to all drivers I think I can live
> > >> with not doing a pointer check here.
> > > 
> > > I suggested doing a
> > > 
> > > if (ops is not NULL)
> > > 
> > > 	return ops();
> > > 
> > > else
> > > 
> > > 	return default_ops();
> > > 
> > > to avoid modifying all drivers. I'm not sure why that wasn't received with
> > > much enthusiasm.
> > 
> > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > so I implemented what the majority wanted. :-)
> 
> I'm not blaming you :-) I was just wondering what their rationale was.


a). To be inline with the usage of this API.

    For this particular case the other functions (but maybe I missed some)
    follow the idea that they implement the ops->func for every operation
    and they don't have an fallback.

b)  Follow what x86 maintainers prefer (based on other API calls,
    such as x86_init or any other platform overrides).

c). When there are new IOMMUs it has to take in-to account all of the
    function ops. If they fail to implement all of them the kernel
    crashes instead of working (but maybe working incorrectly).

d). Lastly, we also want the bloat of kernel. If the compiler decides
    to roll in the fallback implementation for 'iommu_map_sg' in - it will
    needlessly expand the kernel wherever we use 'iommu_map_sg' call with
    a fallback implementation (which might not be called 99% of time).

    Having the little inline static just do 'func_ops->func' will
    stop the compiler from optimizing too much and convert this just
    to a simple function.

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

Hi Konrad,

On Wednesday 20 August 2014 09:02:50 Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

[snip]

> > > >> Okay, since you add these call-backs to all drivers I think I can
> > > >> live with not doing a pointer check here.
> > > > 
> > > > I suggested doing a
> > > > 
> > > > if (ops is not NULL)
> > > > 	return ops();
> > > > else
> > > > 	return default_ops();
> > > > 
> > > > to avoid modifying all drivers. I'm not sure why that wasn't received
> > > > with much enthusiasm.
> > > 
> > > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > > so I implemented what the majority wanted. :-)
> > 
> > I'm not blaming you :-) I was just wondering what their rationale was.
> 
> a). To be inline with the usage of this API.
> 
>     For this particular case the other functions (but maybe I missed some)
>     follow the idea that they implement the ops->func for every operation
>     and they don't have an fallback.

That's because the other functions are mandatory, while this particular one is 
just an optimization and can be implemented generically.

> b)  Follow what x86 maintainers prefer (based on other API calls,
>     such as x86_init or any other platform overrides).
> 
> c). When there are new IOMMUs it has to take in-to account all of the
>     function ops. If they fail to implement all of them the kernel
>     crashes instead of working (but maybe working incorrectly).

I don't think that applies in this case, the generic implementation should 
work in all cases.

> d). Lastly, we also want the bloat of kernel. If the compiler decides
>     to roll in the fallback implementation for 'iommu_map_sg' in - it will
>     needlessly expand the kernel wherever we use 'iommu_map_sg' call with
>     a fallback implementation (which might not be called 99% of time).
> 
>     Having the little inline static just do 'func_ops->func' will
>     stop the compiler from optimizing too much and convert this just
>     to a simple function.

That's an argument I can buy.

-- 
Regards,

Laurent Pinchart

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Laurent Pinchart]

Hi Konrad,

On Wednesday 20 August 2014 09:02:50 Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 19, 2014 at 10:52:46PM +0200, Laurent Pinchart wrote:
> > On Tuesday 19 August 2014 11:40:24 Olav Haugan wrote:
> > > On 8/19/2014 9:11 AM, Laurent Pinchart wrote:
> > > > On Tuesday 19 August 2014 13:59:54 Joerg Roedel wrote:

[snip]

> > > >> Okay, since you add these call-backs to all drivers I think I can
> > > >> live with not doing a pointer check here.
> > > > 
> > > > I suggested doing a
> > > > 
> > > > if (ops is not NULL)
> > > > 	return ops();
> > > > else
> > > > 	return default_ops();
> > > > 
> > > > to avoid modifying all drivers. I'm not sure why that wasn't received
> > > > with much enthusiasm.
> > > 
> > > Both Thierry R. and Konrad W. argued for modifying the drivers instead
> > > so I implemented what the majority wanted. :-)
> > 
> > I'm not blaming you :-) I was just wondering what their rationale was.
> 
> a). To be inline with the usage of this API.
> 
>     For this particular case the other functions (but maybe I missed some)
>     follow the idea that they implement the ops->func for every operation
>     and they don't have an fallback.

That's because the other functions are mandatory, while this particular one is 
just an optimization and can be implemented generically.

> b)  Follow what x86 maintainers prefer (based on other API calls,
>     such as x86_init or any other platform overrides).
> 
> c). When there are new IOMMUs it has to take in-to account all of the
>     function ops. If they fail to implement all of them the kernel
>     crashes instead of working (but maybe working incorrectly).

I don't think that applies in this case, the generic implementation should 
work in all cases.

> d). Lastly, we also want the bloat of kernel. If the compiler decides
>     to roll in the fallback implementation for 'iommu_map_sg' in - it will
>     needlessly expand the kernel wherever we use 'iommu_map_sg' call with
>     a fallback implementation (which might not be called 99% of time).
> 
>     Having the little inline static just do 'func_ops->func' will
>     stop the compiler from optimizing too much and convert this just
>     to a simple function.

That's an argument I can buy.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/19/2014 4:59 AM, Joerg Roedel wrote:
> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
>> If the alignment is not correct then iommu_map() will return error. Not
>> sure what other option we have here (and why make it different behavior
>> than iommu_map which just return error when it is not aligned properly).
>> I don't think we want to force any kind of alignment automatically. I
>> would rather have the API tell me I am doing something wrong than having
>> the function aligning the values and possibly undermap or overmap.
> 
> But sg->offset is an offset into the page (at least it is used that way
> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> use it the same way).
> So when you pass iova + offset the result will no longer be
> page-aligned. You should force sg->offset == 0 and sg->length to be
> page-aligned instead. This makes more sense because the IOMMU-API works
> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> DMA-API.

Ok, but I don't think we want to force a specific alignment here (as I
discussed earlier with Will D.). So I would just do something like
	
	iommu_map(domain, iova + offset, phys, s->length, prot);
	offset += s->length;

>> Yes, I am aware of that. However, several people prefer this than
>> passing in scatterlist. It is not very convenient to pass a scatterlist
>> in some use cases. Someone mentioned a use case where they would have to
>> create a dummy sg list and populate it with the iova just to do an
>> unmap. I believe we would have to do this also. There is no use for
>> sglist when unmapping. However, would like to keep separate API from
>> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
> 
> Keeping it symetric is not more complicated, the caller just needs to
> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> work in sg-lists too.

So are you looking for something like this then:

int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
			 struct scatterlist *sg, unsigned int nents,
			 int prot, unsigned long flags)

int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
			 struct scatterlist *sg, unsigned int nents,
			 unsigned long flags)

This makes unmapping code more complicated (and slow) though. For
example the way I would implement the fallback would be to iterate
through the sglist first to calculate the total length to unmap and then
call iommu_unmap().

I know we talked about removing the flag argument in the map call.
However, the TLB_NO_INV usage is actually needed for both map and unmap.
We have HW that requires TLB invalidate on MAP and TLB inv. is also
required if you implement your mapping function to support replacing an
existing mapping...

Rob, did you have an issue with this API before or was it just an issue
with not having the iova in the unmap call?

>> I thought that was why we added the default fallback and set all the
>> drivers to point to these fallback functions. Several people wanted this
>> so that we don't have to have NULL-check in these functions (and have
>> the functions be simple inline functions).
> 
> Okay, since you add these call-backs to all drivers I think I can live
> with not doing a pointer check here.
> 
> 
> 	Joerg
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 8/19/2014 4:59 AM, Joerg Roedel wrote:
> On Mon, Aug 18, 2014 at 03:47:56PM -0700, Olav Haugan wrote:
>> If the alignment is not correct then iommu_map() will return error. Not
>> sure what other option we have here (and why make it different behavior
>> than iommu_map which just return error when it is not aligned properly).
>> I don't think we want to force any kind of alignment automatically. I
>> would rather have the API tell me I am doing something wrong than having
>> the function aligning the values and possibly undermap or overmap.
> 
> But sg->offset is an offset into the page (at least it is used that way
> in the DMA-API and since you do 'page_len = s->offset + s->length' you
> use it the same way).
> So when you pass iova + offset the result will no longer be
> page-aligned. You should force sg->offset == 0 and sg->length to be
> page-aligned instead. This makes more sense because the IOMMU-API works
> on (io)-page granularity and not on arbitrary phys-addr ranges like the
> DMA-API.

Ok, but I don't think we want to force a specific alignment here (as I
discussed earlier with Will D.). So I would just do something like
	
	iommu_map(domain, iova + offset, phys, s->length, prot);
	offset += s->length;

>> Yes, I am aware of that. However, several people prefer this than
>> passing in scatterlist. It is not very convenient to pass a scatterlist
>> in some use cases. Someone mentioned a use case where they would have to
>> create a dummy sg list and populate it with the iova just to do an
>> unmap. I believe we would have to do this also. There is no use for
>> sglist when unmapping. However, would like to keep separate API from
>> iommu_unmap() to keep the API function names symmetric (map_sg/unmap_sg).
> 
> Keeping it symetric is not more complicated, the caller just needs to
> keep the sg-list used for mapping around. I prefer the unmap_sg call to
> work in sg-lists too.

So are you looking for something like this then:

int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
			 struct scatterlist *sg, unsigned int nents,
			 int prot, unsigned long flags)

int iommu_unmap_sg(struct iommu_domain *domain, unsigned long iova,
			 struct scatterlist *sg, unsigned int nents,
			 unsigned long flags)

This makes unmapping code more complicated (and slow) though. For
example the way I would implement the fallback would be to iterate
through the sglist first to calculate the total length to unmap and then
call iommu_unmap().

I know we talked about removing the flag argument in the map call.
However, the TLB_NO_INV usage is actually needed for both map and unmap.
We have HW that requires TLB invalidate on MAP and TLB inv. is also
required if you implement your mapping function to support replacing an
existing mapping...

Rob, did you have an issue with this API before or was it just an issue
with not having the iova in the unmap call?

>> I thought that was why we added the default fallback and set all the
>> drivers to point to these fallback functions. Several people wanted this
>> so that we don't have to have NULL-check in these functions (and have
>> the functions be simple inline functions).
> 
> Okay, since you add these call-backs to all drivers I think I can live
> with not doing a pointer check here.
> 
> 
> 	Joerg
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}

I have thought a little bit more about this interface and think that it
would be better to just return a size_t from iommu_map_sg().  The
function returns the amount of address space mapped by it, 0 in the
worst case.

This makes it easy to unmap the region just with
iommu_unmap(domain, iova, size) in the end and removing the need for a
new iommu_unmap_sg() function. Also the error-path of the map_sg
call-backs becomes easier as the function then just returns the amount
of address-space already mapped before the error happened.

So the prototype would be:

	size_t iommu_map_sg(struct iommu_domain *domain,
			    unsigned long iova,
			    struct scatterlist *sg,
			    unsigned int nents,
			    int prot);

(as I said before, the flags parameter should not be added by this
 patch-set).


	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> +			       struct scatterlist *sg, unsigned int nents,
> +			       int prot, unsigned long flags)
> +{
> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> +}
> +
> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size,
> +				 unsigned long flags)
> +{
> +	return domain->ops->unmap_sg(domain, iova, size, flags);
> +}

I have thought a little bit more about this interface and think that it
would be better to just return a size_t from iommu_map_sg().  The
function returns the amount of address space mapped by it, 0 in the
worst case.

This makes it easy to unmap the region just with
iommu_unmap(domain, iova, size) in the end and removing the need for a
new iommu_unmap_sg() function. Also the error-path of the map_sg
call-backs becomes easier as the function then just returns the amount
of address-space already mapped before the error happened.

So the prototype would be:

	size_t iommu_map_sg(struct iommu_domain *domain,
			    unsigned long iova,
			    struct scatterlist *sg,
			    unsigned int nents,
			    int prot);

(as I said before, the flags parameter should not be added by this
 patch-set).


	Joerg

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 9/25/2014 10:01 AM, Joerg Roedel wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
>
> I have thought a little bit more about this interface and think that it
> would be better to just return a size_t from iommu_map_sg().  The
> function returns the amount of address space mapped by it, 0 in the
> worst case.
>
> This makes it easy to unmap the region just with
> iommu_unmap(domain, iova, size) in the end and removing the need for a
> new iommu_unmap_sg() function. Also the error-path of the map_sg
> call-backs becomes easier as the function then just returns the amount
> of address-space already mapped before the error happened.
>
> So the prototype would be:
>
> 	size_t iommu_map_sg(struct iommu_domain *domain,
> 			    unsigned long iova,
> 			    struct scatterlist *sg,
> 			    unsigned int nents,
> 			    int prot);
>
> (as I said before, the flags parameter should not be added by this
>   patch-set).
>

Ok, sounds good. I'll post v6 soon.

.Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 9/25/2014 10:01 AM, Joerg Roedel wrote:
> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>> +			       struct scatterlist *sg, unsigned int nents,
>> +			       int prot, unsigned long flags)
>> +{
>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>> +}
>> +
>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>> +				 unsigned long iova, size_t size,
>> +				 unsigned long flags)
>> +{
>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>> +}
>
> I have thought a little bit more about this interface and think that it
> would be better to just return a size_t from iommu_map_sg().  The
> function returns the amount of address space mapped by it, 0 in the
> worst case.
>
> This makes it easy to unmap the region just with
> iommu_unmap(domain, iova, size) in the end and removing the need for a
> new iommu_unmap_sg() function. Also the error-path of the map_sg
> call-backs becomes easier as the function then just returns the amount
> of address-space already mapped before the error happened.
>
> So the prototype would be:
>
> 	size_t iommu_map_sg(struct iommu_domain *domain,
> 			    unsigned long iova,
> 			    struct scatterlist *sg,
> 			    unsigned int nents,
> 			    int prot);
>
> (as I said before, the flags parameter should not be added by this
>   patch-set).
>

Ok, sounds good. I'll post v6 soon.

.Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote:
> On 9/25/2014 10:01 AM, Joerg Roedel wrote:
> >On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> >>+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> >>+			       struct scatterlist *sg, unsigned int nents,
> >>+			       int prot, unsigned long flags)
> >>+{
> >>+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> >>+}
> >>+
> >>+static inline int iommu_unmap_sg(struct iommu_domain *domain,
> >>+				 unsigned long iova, size_t size,
> >>+				 unsigned long flags)
> >>+{
> >>+	return domain->ops->unmap_sg(domain, iova, size, flags);
> >>+}
> >
> >I have thought a little bit more about this interface and think that it
> >would be better to just return a size_t from iommu_map_sg().  The
> >function returns the amount of address space mapped by it, 0 in the
> >worst case.
> >
> >This makes it easy to unmap the region just with
> >iommu_unmap(domain, iova, size) in the end and removing the need for a
> >new iommu_unmap_sg() function. Also the error-path of the map_sg
> >call-backs becomes easier as the function then just returns the amount
> >of address-space already mapped before the error happened.
> >
> >So the prototype would be:
> >
> >	size_t iommu_map_sg(struct iommu_domain *domain,
> >			    unsigned long iova,
> >			    struct scatterlist *sg,
> >			    unsigned int nents,
> >			    int prot);
> >
> >(as I said before, the flags parameter should not be added by this
> >  patch-set).
> >
> 
> Ok, sounds good. I'll post v6 soon.

Perhaps make the return value ssize_t so that we can propagate errors?

Thierry

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

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Thierry Reding]

On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote:
> On 9/25/2014 10:01 AM, Joerg Roedel wrote:
> >On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
> >>+static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
> >>+			       struct scatterlist *sg, unsigned int nents,
> >>+			       int prot, unsigned long flags)
> >>+{
> >>+	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
> >>+}
> >>+
> >>+static inline int iommu_unmap_sg(struct iommu_domain *domain,
> >>+				 unsigned long iova, size_t size,
> >>+				 unsigned long flags)
> >>+{
> >>+	return domain->ops->unmap_sg(domain, iova, size, flags);
> >>+}
> >
> >I have thought a little bit more about this interface and think that it
> >would be better to just return a size_t from iommu_map_sg().  The
> >function returns the amount of address space mapped by it, 0 in the
> >worst case.
> >
> >This makes it easy to unmap the region just with
> >iommu_unmap(domain, iova, size) in the end and removing the need for a
> >new iommu_unmap_sg() function. Also the error-path of the map_sg
> >call-backs becomes easier as the function then just returns the amount
> >of address-space already mapped before the error happened.
> >
> >So the prototype would be:
> >
> >	size_t iommu_map_sg(struct iommu_domain *domain,
> >			    unsigned long iova,
> >			    struct scatterlist *sg,
> >			    unsigned int nents,
> >			    int prot);
> >
> >(as I said before, the flags parameter should not be added by this
> >  patch-set).
> >
> 
> Ok, sounds good. I'll post v6 soon.

Perhaps make the return value ssize_t so that we can propagate errors?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141015/7248bdde/attachment-0001.sig>

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 10/15/2014 2:16 AM, Thierry Reding wrote:
> On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote:
>> On 9/25/2014 10:01 AM, Joerg Roedel wrote:
>>> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>>>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>>>> +			       struct scatterlist *sg, unsigned int nents,
>>>> +			       int prot, unsigned long flags)
>>>> +{
>>>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>>>> +}
>>>> +
>>>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>>>> +				 unsigned long iova, size_t size,
>>>> +				 unsigned long flags)
>>>> +{
>>>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>>>> +}
>>>
>>> I have thought a little bit more about this interface and think that it
>>> would be better to just return a size_t from iommu_map_sg().  The
>>> function returns the amount of address space mapped by it, 0 in the
>>> worst case.
>>>
>>> This makes it easy to unmap the region just with
>>> iommu_unmap(domain, iova, size) in the end and removing the need for a
>>> new iommu_unmap_sg() function. Also the error-path of the map_sg
>>> call-backs becomes easier as the function then just returns the amount
>>> of address-space already mapped before the error happened.
>>>
>>> So the prototype would be:
>>>
>>> 	size_t iommu_map_sg(struct iommu_domain *domain,
>>> 			    unsigned long iova,
>>> 			    struct scatterlist *sg,
>>> 			    unsigned int nents,
>>> 			    int prot);
>>>
>>> (as I said before, the flags parameter should not be added by this
>>>   patch-set).
>>>
>>
>> Ok, sounds good. I'll post v6 soon.
>
> Perhaps make the return value ssize_t so that we can propagate errors?
>

I am fine with that. Joerg?

Thanks,

.Olav

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Olav Haugan]

On 10/15/2014 2:16 AM, Thierry Reding wrote:
> On Mon, Oct 06, 2014 at 12:02:47PM -0700, Olav Haugan wrote:
>> On 9/25/2014 10:01 AM, Joerg Roedel wrote:
>>> On Mon, Aug 11, 2014 at 03:45:50PM -0700, Olav Haugan wrote:
>>>> +static inline int iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
>>>> +			       struct scatterlist *sg, unsigned int nents,
>>>> +			       int prot, unsigned long flags)
>>>> +{
>>>> +	return domain->ops->map_sg(domain, iova, sg, nents, prot, flags);
>>>> +}
>>>> +
>>>> +static inline int iommu_unmap_sg(struct iommu_domain *domain,
>>>> +				 unsigned long iova, size_t size,
>>>> +				 unsigned long flags)
>>>> +{
>>>> +	return domain->ops->unmap_sg(domain, iova, size, flags);
>>>> +}
>>>
>>> I have thought a little bit more about this interface and think that it
>>> would be better to just return a size_t from iommu_map_sg().  The
>>> function returns the amount of address space mapped by it, 0 in the
>>> worst case.
>>>
>>> This makes it easy to unmap the region just with
>>> iommu_unmap(domain, iova, size) in the end and removing the need for a
>>> new iommu_unmap_sg() function. Also the error-path of the map_sg
>>> call-backs becomes easier as the function then just returns the amount
>>> of address-space already mapped before the error happened.
>>>
>>> So the prototype would be:
>>>
>>> 	size_t iommu_map_sg(struct iommu_domain *domain,
>>> 			    unsigned long iova,
>>> 			    struct scatterlist *sg,
>>> 			    unsigned int nents,
>>> 			    int prot);
>>>
>>> (as I said before, the flags parameter should not be added by this
>>>   patch-set).
>>>
>>
>> Ok, sounds good. I'll post v6 soon.
>
> Perhaps make the return value ssize_t so that we can propagate errors?
>

I am fine with that. Joerg?

Thanks,

.Olav

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Thu, Oct 16, 2014 at 10:23:07AM -0700, Olav Haugan wrote:
> On 10/15/2014 2:16 AM, Thierry Reding wrote:
> >Perhaps make the return value ssize_t so that we can propagate errors?
> >
> 
> I am fine with that. Joerg?

I am not so sure this is a good idea. On error the function should
return the size that is already mapped, so that the caller can check
that against the expected size and just call iommu_unmap with the
returned size when return value does not match expected size.

The benefit of ssize_t is that we can return more detailed error
information when the function did not map anything yet. But this
complicates the error handling for the caller and implementers might get
it wrong and return an error-value even when some space was already
mapped, so that the caller can not unmap the addresses.

So taking this into mind, I prefer it to be size_t for now. We can
always change it later if it turns out to be wrong.

Regards,

	Joerg

[PATCH v5 1/1] iommu-api: Add map_sg/unmap_sg functions

[Joerg Roedel]

On Thu, Oct 16, 2014 at 10:23:07AM -0700, Olav Haugan wrote:
> On 10/15/2014 2:16 AM, Thierry Reding wrote:
> >Perhaps make the return value ssize_t so that we can propagate errors?
> >
> 
> I am fine with that. Joerg?

I am not so sure this is a good idea. On error the function should
return the size that is already mapped, so that the caller can check
that against the expected size and just call iommu_unmap with the
returned size when return value does not match expected size.

The benefit of ssize_t is that we can return more detailed error
information when the function did not map anything yet. But this
complicates the error handling for the caller and implementers might get
it wrong and return an error-value even when some space was already
mapped, so that the caller can not unmap the addresses.

So taking this into mind, I prefer it to be size_t for now. We can
always change it later if it turns out to be wrong.

Regards,

	Joerg