[PATCH V2 0/6] IOMMU-DMA - support DMA_ATTR_LOW_ADDRESS attribute

[PATCH V2 0/6] IOMMU-DMA - support DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

This patchset is a rebase of original patches from Marek Szyprowski:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2321261.html

The patches have been rebased on Joro's IOMMU tree "next" branch:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git

This patchset is needed to address the IOVA address dependency issue between
firmware buffers and other buffers in Samsung s5p-mfc driver.

There have been few discussions in the past on how to find a generic
soultion for this issue, ranging from adding an entirely new API to choose
IOVA window[1], to adding a DMA attribute DMA_ATTR_LOW_ADDRESS which handles
buffer allocation from lower address[2].
This is a continuation of initial work from Marek for approach [2].
Patches have been tested with latest version of Samsung s5p-mfc driver.

Changes since V1:
[PATCH V2 1/6]
- Rebase on latest tree.

[PATCH V2 2/6]
- Rebase on latest tree.
  Added a missing check for iova_pfn in __iova_rcache_get()
  Discard changes from drivers/iommu/intel/iommu.c which are not necessary

[PATCH V2 3/6]
- Rebase on latest tree.

[PATCH V2 4/6]
- Rebase on latest tree

[PATCH V2 5/6]
- Rebase on latest tree

[PATCH V2 6/6]
- Rebase on latest tree.

Marek Szyprowski (6):
  dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute
  iommu: iova: properly handle 0 as a valid IOVA address
  iommu: iova: add support for 'first-fit' algorithm
  iommu: dma-iommu: refactor iommu_dma_alloc_iova()
  iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS
  media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS

References:
[1]
https://lore.kernel.org/linux-iommu/20200811054912.GA301@infradead.org/

[2]
https://lore.kernel.org/linux-mm/bff57cbe-2247-05e1-9059-d9c66d64c407@arm.com

 drivers/iommu/dma-iommu.c                     | 77 +++++++++++-----
 drivers/iommu/iova.c                          | 91 ++++++++++++++++++-
 .../media/platform/samsung/s5p-mfc/s5p_mfc.c  |  8 +-
 include/linux/dma-mapping.h                   |  6 ++
 include/linux/iova.h                          |  3 +
 5 files changed, 156 insertions(+), 29 deletions(-)


base-commit: faf93cfaadfaaff2a5c35d6301b45aa2f6e4ddb2
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 0/6] IOMMU-DMA - support DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

This patchset is a rebase of original patches from Marek Szyprowski:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2321261.html

The patches have been rebased on Joro's IOMMU tree "next" branch:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git

This patchset is needed to address the IOVA address dependency issue between
firmware buffers and other buffers in Samsung s5p-mfc driver.

There have been few discussions in the past on how to find a generic
soultion for this issue, ranging from adding an entirely new API to choose
IOVA window[1], to adding a DMA attribute DMA_ATTR_LOW_ADDRESS which handles
buffer allocation from lower address[2].
This is a continuation of initial work from Marek for approach [2].
Patches have been tested with latest version of Samsung s5p-mfc driver.

Changes since V1:
[PATCH V2 1/6]
- Rebase on latest tree.

[PATCH V2 2/6]
- Rebase on latest tree.
  Added a missing check for iova_pfn in __iova_rcache_get()
  Discard changes from drivers/iommu/intel/iommu.c which are not necessary

[PATCH V2 3/6]
- Rebase on latest tree.

[PATCH V2 4/6]
- Rebase on latest tree

[PATCH V2 5/6]
- Rebase on latest tree

[PATCH V2 6/6]
- Rebase on latest tree.

Marek Szyprowski (6):
  dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute
  iommu: iova: properly handle 0 as a valid IOVA address
  iommu: iova: add support for 'first-fit' algorithm
  iommu: dma-iommu: refactor iommu_dma_alloc_iova()
  iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS
  media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS

References:
[1]
https://lore.kernel.org/linux-iommu/20200811054912.GA301@infradead.org/

[2]
https://lore.kernel.org/linux-mm/bff57cbe-2247-05e1-9059-d9c66d64c407@arm.com

 drivers/iommu/dma-iommu.c                     | 77 +++++++++++-----
 drivers/iommu/iova.c                          | 91 ++++++++++++++++++-
 .../media/platform/samsung/s5p-mfc/s5p_mfc.c  |  8 +-
 include/linux/dma-mapping.h                   |  6 ++
 include/linux/iova.h                          |  3 +
 5 files changed, 156 insertions(+), 29 deletions(-)


base-commit: faf93cfaadfaaff2a5c35d6301b45aa2f6e4ddb2
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 1/6] dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Some devices require to allocate a special buffer (usually for the
firmware) just at the beginning of the address space to ensure that all
further allocations can be expressed as a positive offset from that
special buffer. When IOMMU is used for managing the DMA address space,
such requirement can be easily fulfilled, simply by enforcing the
'first-fit' IOVA allocation algorithm.

This patch adds a DMA attribute for such case.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 include/linux/dma-mapping.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index dca2b1355bb1..3cbdf857edd1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -60,6 +60,12 @@
  * at least read-only at lesser-privileged levels).
  */
 #define DMA_ATTR_PRIVILEGED		(1UL << 9)
+/*
+ * DMA_ATTR_LOW_ADDRESS: used to indicate that the buffer should be allocated
+ * at the lowest possible DMA address, usually just at the beginning of the
+ * DMA/IOVA address space ('first-fit' allocation algorithm).
+ */
+#define DMA_ATTR_LOW_ADDRESS		(1UL << 10)
 
 /*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.  It can
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 1/6] dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Some devices require to allocate a special buffer (usually for the
firmware) just at the beginning of the address space to ensure that all
further allocations can be expressed as a positive offset from that
special buffer. When IOMMU is used for managing the DMA address space,
such requirement can be easily fulfilled, simply by enforcing the
'first-fit' IOVA allocation algorithm.

This patch adds a DMA attribute for such case.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 include/linux/dma-mapping.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index dca2b1355bb1..3cbdf857edd1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -60,6 +60,12 @@
  * at least read-only at lesser-privileged levels).
  */
 #define DMA_ATTR_PRIVILEGED		(1UL << 9)
+/*
+ * DMA_ATTR_LOW_ADDRESS: used to indicate that the buffer should be allocated
+ * at the lowest possible DMA address, usually just at the beginning of the
+ * DMA/IOVA address space ('first-fit' allocation algorithm).
+ */
+#define DMA_ATTR_LOW_ADDRESS		(1UL << 10)
 
 /*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.  It can
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Zero is a valid DMA and IOVA address on many architectures, so adjust the
IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
(~0UL) is introduced as a generic value for the error case. Adjust all
callers of the alloc_iova_fast() function for the new return value.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 16 +++++++++-------
 drivers/iommu/iova.c      | 13 +++++++++----
 include/linux/iova.h      |  1 +
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1ca85d37eeab..16218d6a0703 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 {
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-	unsigned long shift, iova_len, iova = 0;
+	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
 
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
 		cookie->msi_iova += size;
@@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		iova = alloc_iova_fast(iovad, iova_len,
 				       DMA_BIT_MASK(32) >> shift, false);
 
-	if (!iova)
+	if (iova == IOVA_BAD_ADDR)
 		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
 				       true);
 
-	return (dma_addr_t)iova << shift;
+	if (iova != IOVA_BAD_ADDR)
+		return (dma_addr_t)iova << shift;
+	return DMA_MAPPING_ERROR;
 }
 
 static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
@@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 	size = iova_align(iovad, size + iova_off);
 
 	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 
 	if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
@@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 
 	size = iova_align(iovad, size);
 	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
 	if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
@@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	}
 
 	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
-	if (!iova) {
+	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
 	}
@@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		return NULL;
 
 	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_page;
 
 	if (iommu_map(domain, iova, msi_addr, size, prot))
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index db77aa675145..ae0fe0a6714e 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
  * This function tries to satisfy an iova allocation from the rcache,
  * and falls back to regular allocation on failure. If regular allocation
  * fails too and the flush_rcache flag is set then the rcache will be flushed.
+ * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
+ * of a failure.
 */
 unsigned long
 alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
@@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 		size = roundup_pow_of_two(size);
 
 	iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
-	if (iova_pfn)
+	if (iova_pfn != IOVA_BAD_ADDR)
 		return iova_pfn;
 
 retry:
@@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 		unsigned int cpu;
 
 		if (!flush_rcache)
-			return 0;
+			return IOVA_BAD_ADDR;
 
 		/* Try replenishing IOVAs by flushing rcache. */
 		flush_rcache = false;
@@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
 				       unsigned long limit_pfn)
 {
 	struct iova_cpu_rcache *cpu_rcache;
-	unsigned long iova_pfn = 0;
+	unsigned long iova_pfn = IOVA_BAD_ADDR;
 	bool has_pfn = false;
 	unsigned long flags;
 
@@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
 
 	spin_unlock_irqrestore(&cpu_rcache->lock, flags);
 
+	if (!iova_pfn)
+		return IOVA_BAD_ADDR;
+
 	return iova_pfn;
 }
 
@@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
 	unsigned int log_size = order_base_2(size);
 
 	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
-		return 0;
+		return IOVA_BAD_ADDR;
 
 	return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
 }
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 320a70e40233..46b5b10c532b 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -21,6 +21,7 @@ struct iova {
 	unsigned long	pfn_lo; /* Lowest allocated pfn */
 };
 
+#define IOVA_BAD_ADDR	(~0UL)
 
 struct iova_rcache;
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Zero is a valid DMA and IOVA address on many architectures, so adjust the
IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
(~0UL) is introduced as a generic value for the error case. Adjust all
callers of the alloc_iova_fast() function for the new return value.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 16 +++++++++-------
 drivers/iommu/iova.c      | 13 +++++++++----
 include/linux/iova.h      |  1 +
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1ca85d37eeab..16218d6a0703 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 {
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-	unsigned long shift, iova_len, iova = 0;
+	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
 
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
 		cookie->msi_iova += size;
@@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		iova = alloc_iova_fast(iovad, iova_len,
 				       DMA_BIT_MASK(32) >> shift, false);
 
-	if (!iova)
+	if (iova == IOVA_BAD_ADDR)
 		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
 				       true);
 
-	return (dma_addr_t)iova << shift;
+	if (iova != IOVA_BAD_ADDR)
+		return (dma_addr_t)iova << shift;
+	return DMA_MAPPING_ERROR;
 }
 
 static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
@@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 	size = iova_align(iovad, size + iova_off);
 
 	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 
 	if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
@@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 
 	size = iova_align(iovad, size);
 	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
 	if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
@@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	}
 
 	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
-	if (!iova) {
+	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
 	}
@@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		return NULL;
 
 	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
-	if (!iova)
+	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_page;
 
 	if (iommu_map(domain, iova, msi_addr, size, prot))
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index db77aa675145..ae0fe0a6714e 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
  * This function tries to satisfy an iova allocation from the rcache,
  * and falls back to regular allocation on failure. If regular allocation
  * fails too and the flush_rcache flag is set then the rcache will be flushed.
+ * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
+ * of a failure.
 */
 unsigned long
 alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
@@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 		size = roundup_pow_of_two(size);
 
 	iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
-	if (iova_pfn)
+	if (iova_pfn != IOVA_BAD_ADDR)
 		return iova_pfn;
 
 retry:
@@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 		unsigned int cpu;
 
 		if (!flush_rcache)
-			return 0;
+			return IOVA_BAD_ADDR;
 
 		/* Try replenishing IOVAs by flushing rcache. */
 		flush_rcache = false;
@@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
 				       unsigned long limit_pfn)
 {
 	struct iova_cpu_rcache *cpu_rcache;
-	unsigned long iova_pfn = 0;
+	unsigned long iova_pfn = IOVA_BAD_ADDR;
 	bool has_pfn = false;
 	unsigned long flags;
 
@@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
 
 	spin_unlock_irqrestore(&cpu_rcache->lock, flags);
 
+	if (!iova_pfn)
+		return IOVA_BAD_ADDR;
+
 	return iova_pfn;
 }
 
@@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
 	unsigned int log_size = order_base_2(size);
 
 	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
-		return 0;
+		return IOVA_BAD_ADDR;
 
 	return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
 }
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 320a70e40233..46b5b10c532b 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -21,6 +21,7 @@ struct iova {
 	unsigned long	pfn_lo; /* Lowest allocated pfn */
 };
 
+#define IOVA_BAD_ADDR	(~0UL)
 
 struct iova_rcache;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 3/6] iommu: iova: add support for 'first-fit' algorithm

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Add support for the 'first-fit' allocation algorithm. It will be used for
the special case of implementing DMA_ATTR_LOW_ADDRESS, so this path
doesn't use IOVA cache.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/iova.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iova.h |  2 ++
 2 files changed, 80 insertions(+)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index ae0fe0a6714e..89f9338f83a3 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -231,6 +231,59 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 	return -ENOMEM;
 }
 
+static unsigned long
+__iova_get_aligned_start(unsigned long start, unsigned long size)
+{
+	unsigned long mask = __roundup_pow_of_two(size) - 1;
+
+	return (start + mask) & ~mask;
+}
+
+static int __alloc_and_insert_iova_range_forward(struct iova_domain *iovad,
+			unsigned long size, unsigned long limit_pfn,
+			struct iova *new)
+{
+	struct rb_node *curr;
+	unsigned long flags;
+	unsigned long start, limit;
+
+	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+
+	curr = rb_first(&iovad->rbroot);
+	limit = limit_pfn;
+	start = __iova_get_aligned_start(iovad->start_pfn, size);
+
+	while (curr) {
+		struct iova *curr_iova = rb_entry(curr, struct iova, node);
+		struct rb_node *next = rb_next(curr);
+
+		start = __iova_get_aligned_start(curr_iova->pfn_hi + 1, size);
+		if (next) {
+			struct iova *next_iova = rb_entry(next, struct iova, node);
+			limit = next_iova->pfn_lo - 1;
+		} else {
+			limit = limit_pfn;
+		}
+
+		if ((start + size) <= limit)
+			break;	/* found a free slot */
+		curr = next;
+	}
+
+	if (!curr && start + size > limit) {
+		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+		return -ENOMEM;
+	}
+
+	new->pfn_lo = start;
+	new->pfn_hi = new->pfn_lo + size - 1;
+	iova_insert_rbtree(&iovad->rbroot, new, curr);
+
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+
+	return 0;
+}
+
 static struct kmem_cache *iova_cache;
 static unsigned int iova_cache_users;
 static DEFINE_MUTEX(iova_cache_mutex);
@@ -420,6 +473,31 @@ free_iova(struct iova_domain *iovad, unsigned long pfn)
 }
 EXPORT_SYMBOL_GPL(free_iova);
 
+/**
+ * alloc_iova_first_fit - allocates an iova from the beginning of address space
+ * @iovad: - iova domain in question
+ * @size: - size of page frames to allocate
+ * @limit_pfn: - max limit address
+ * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
+ * of a failure.
+ */
+unsigned long
+alloc_iova_first_fit(struct iova_domain *iovad, unsigned long size,
+		     unsigned long limit_pfn)
+{
+	struct iova *new_iova = alloc_iova_mem();
+
+	if (!new_iova)
+		return IOVA_BAD_ADDR;
+
+	if (__alloc_and_insert_iova_range_forward(iovad, size, limit_pfn, new_iova)) {
+		free_iova_mem(new_iova);
+		return IOVA_BAD_ADDR;
+	}
+	return new_iova->pfn_lo;
+}
+EXPORT_SYMBOL_GPL(alloc_iova_first_fit);
+
 /**
  * alloc_iova_fast - allocates an iova from rcache
  * @iovad: - iova domain in question
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 46b5b10c532b..45ed6d41490a 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -89,6 +89,8 @@ void free_iova_fast(struct iova_domain *iovad, unsigned long pfn,
 		    unsigned long size);
 unsigned long alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 			      unsigned long limit_pfn, bool flush_rcache);
+unsigned long alloc_iova_first_fit(struct iova_domain *iovad, unsigned long size,
+				   unsigned long limit_pfn);
 struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 3/6] iommu: iova: add support for 'first-fit' algorithm

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Add support for the 'first-fit' allocation algorithm. It will be used for
the special case of implementing DMA_ATTR_LOW_ADDRESS, so this path
doesn't use IOVA cache.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/iova.c | 78 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iova.h |  2 ++
 2 files changed, 80 insertions(+)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index ae0fe0a6714e..89f9338f83a3 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -231,6 +231,59 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 	return -ENOMEM;
 }
 
+static unsigned long
+__iova_get_aligned_start(unsigned long start, unsigned long size)
+{
+	unsigned long mask = __roundup_pow_of_two(size) - 1;
+
+	return (start + mask) & ~mask;
+}
+
+static int __alloc_and_insert_iova_range_forward(struct iova_domain *iovad,
+			unsigned long size, unsigned long limit_pfn,
+			struct iova *new)
+{
+	struct rb_node *curr;
+	unsigned long flags;
+	unsigned long start, limit;
+
+	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+
+	curr = rb_first(&iovad->rbroot);
+	limit = limit_pfn;
+	start = __iova_get_aligned_start(iovad->start_pfn, size);
+
+	while (curr) {
+		struct iova *curr_iova = rb_entry(curr, struct iova, node);
+		struct rb_node *next = rb_next(curr);
+
+		start = __iova_get_aligned_start(curr_iova->pfn_hi + 1, size);
+		if (next) {
+			struct iova *next_iova = rb_entry(next, struct iova, node);
+			limit = next_iova->pfn_lo - 1;
+		} else {
+			limit = limit_pfn;
+		}
+
+		if ((start + size) <= limit)
+			break;	/* found a free slot */
+		curr = next;
+	}
+
+	if (!curr && start + size > limit) {
+		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+		return -ENOMEM;
+	}
+
+	new->pfn_lo = start;
+	new->pfn_hi = new->pfn_lo + size - 1;
+	iova_insert_rbtree(&iovad->rbroot, new, curr);
+
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+
+	return 0;
+}
+
 static struct kmem_cache *iova_cache;
 static unsigned int iova_cache_users;
 static DEFINE_MUTEX(iova_cache_mutex);
@@ -420,6 +473,31 @@ free_iova(struct iova_domain *iovad, unsigned long pfn)
 }
 EXPORT_SYMBOL_GPL(free_iova);
 
+/**
+ * alloc_iova_first_fit - allocates an iova from the beginning of address space
+ * @iovad: - iova domain in question
+ * @size: - size of page frames to allocate
+ * @limit_pfn: - max limit address
+ * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
+ * of a failure.
+ */
+unsigned long
+alloc_iova_first_fit(struct iova_domain *iovad, unsigned long size,
+		     unsigned long limit_pfn)
+{
+	struct iova *new_iova = alloc_iova_mem();
+
+	if (!new_iova)
+		return IOVA_BAD_ADDR;
+
+	if (__alloc_and_insert_iova_range_forward(iovad, size, limit_pfn, new_iova)) {
+		free_iova_mem(new_iova);
+		return IOVA_BAD_ADDR;
+	}
+	return new_iova->pfn_lo;
+}
+EXPORT_SYMBOL_GPL(alloc_iova_first_fit);
+
 /**
  * alloc_iova_fast - allocates an iova from rcache
  * @iovad: - iova domain in question
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 46b5b10c532b..45ed6d41490a 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -89,6 +89,8 @@ void free_iova_fast(struct iova_domain *iovad, unsigned long pfn,
 		    unsigned long size);
 unsigned long alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
 			      unsigned long limit_pfn, bool flush_rcache);
+unsigned long alloc_iova_first_fit(struct iova_domain *iovad, unsigned long size,
+				   unsigned long limit_pfn);
 struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 4/6] iommu: dma-iommu: refactor iommu_dma_alloc_iova()

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Change the parameters passed to iommu_dma_alloc_iova(): the dma_limit can
be easily extracted from the parameters of the passed struct device, so
replace it with a flags parameter, which can later hold more information
about the way the IOVA allocator should do it job. While touching the
parameter list, move struct device to the second position to better match
the convention of the DMA-mapping related functions.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 16218d6a0703..cb235b40303c 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -600,12 +600,16 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 	}
 }
 
+#define DMA_ALLOC_IOVA_COHERENT		BIT(0)
+
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
-		size_t size, u64 dma_limit, struct device *dev)
+		struct device *dev, size_t size, unsigned int flags)
 {
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
 	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
+	u64 dma_limit = (flags & DMA_ALLOC_IOVA_COHERENT) ?
+			dev->coherent_dma_mask : dma_get_mask(dev);
 
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
 		cookie->msi_iova += size;
@@ -675,7 +679,7 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
 }
 
 static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
-		size_t size, int prot, u64 dma_mask)
+		size_t size, int prot, unsigned int flags)
 {
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
@@ -689,7 +693,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 
 	size = iova_align(iovad, size + iova_off);
 
-	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, flags);
 	if (iova == DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 
@@ -800,7 +804,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 		return NULL;
 
 	size = iova_align(iovad, size);
-	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, DMA_ALLOC_IOVA_COHERENT);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
@@ -963,7 +967,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-	dma_addr_t iova, dma_mask = dma_get_mask(dev);
+	dma_addr_t iova;
 
 	/*
 	 * If both the physical buffer start address and size are
@@ -1001,7 +1005,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 		arch_sync_dma_for_device(phys, size, dir);
 
-	iova = __iommu_dma_map(dev, phys, size, prot, dma_mask);
+	iova = __iommu_dma_map(dev, phys, size, prot, 0);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
 		swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
 	return iova;
@@ -1205,7 +1209,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 		prev = s;
 	}
 
-	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
+	iova = iommu_dma_alloc_iova(domain, dev, iova_len, 0);
 	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
@@ -1264,8 +1268,7 @@ static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	return __iommu_dma_map(dev, phys, size,
-			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO,
-			dma_get_mask(dev));
+			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO, 0);
 }
 
 static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
@@ -1375,7 +1378,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 		return NULL;
 
 	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot,
-			dev->coherent_dma_mask);
+			DMA_ALLOC_IOVA_COHERENT);
 	if (*handle == DMA_MAPPING_ERROR) {
 		__iommu_dma_free(dev, size, cpu_addr);
 		return NULL;
@@ -1517,7 +1520,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	if (!msi_page)
 		return NULL;
 
-	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, 0);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_page;
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 4/6] iommu: dma-iommu: refactor iommu_dma_alloc_iova()

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Change the parameters passed to iommu_dma_alloc_iova(): the dma_limit can
be easily extracted from the parameters of the passed struct device, so
replace it with a flags parameter, which can later hold more information
about the way the IOVA allocator should do it job. While touching the
parameter list, move struct device to the second position to better match
the convention of the DMA-mapping related functions.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 16218d6a0703..cb235b40303c 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -600,12 +600,16 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 	}
 }
 
+#define DMA_ALLOC_IOVA_COHERENT		BIT(0)
+
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
-		size_t size, u64 dma_limit, struct device *dev)
+		struct device *dev, size_t size, unsigned int flags)
 {
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
 	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
+	u64 dma_limit = (flags & DMA_ALLOC_IOVA_COHERENT) ?
+			dev->coherent_dma_mask : dma_get_mask(dev);
 
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
 		cookie->msi_iova += size;
@@ -675,7 +679,7 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
 }
 
 static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
-		size_t size, int prot, u64 dma_mask)
+		size_t size, int prot, unsigned int flags)
 {
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
@@ -689,7 +693,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
 
 	size = iova_align(iovad, size + iova_off);
 
-	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, flags);
 	if (iova == DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 
@@ -800,7 +804,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 		return NULL;
 
 	size = iova_align(iovad, size);
-	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, DMA_ALLOC_IOVA_COHERENT);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
@@ -963,7 +967,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-	dma_addr_t iova, dma_mask = dma_get_mask(dev);
+	dma_addr_t iova;
 
 	/*
 	 * If both the physical buffer start address and size are
@@ -1001,7 +1005,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 		arch_sync_dma_for_device(phys, size, dir);
 
-	iova = __iommu_dma_map(dev, phys, size, prot, dma_mask);
+	iova = __iommu_dma_map(dev, phys, size, prot, 0);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
 		swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
 	return iova;
@@ -1205,7 +1209,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 		prev = s;
 	}
 
-	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
+	iova = iommu_dma_alloc_iova(domain, dev, iova_len, 0);
 	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
@@ -1264,8 +1268,7 @@ static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	return __iommu_dma_map(dev, phys, size,
-			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO,
-			dma_get_mask(dev));
+			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO, 0);
 }
 
 static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
@@ -1375,7 +1378,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 		return NULL;
 
 	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot,
-			dev->coherent_dma_mask);
+			DMA_ALLOC_IOVA_COHERENT);
 	if (*handle == DMA_MAPPING_ERROR) {
 		__iommu_dma_free(dev, size, cpu_addr);
 		return NULL;
@@ -1517,7 +1520,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 	if (!msi_page)
 		return NULL;
 
-	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
+	iova = iommu_dma_alloc_iova(domain, dev, size, 0);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_page;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 5/6] iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Implement support for the DMA_ATTR_LOW_ADDRESS DMA attribute. If it has
been set, call alloc_iova_first_fit() instead of the alloc_iova_fast() to
allocate the new IOVA from the beginning of the address space.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 50 +++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index cb235b40303c..553c5b863e19 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -601,6 +601,18 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 }
 
 #define DMA_ALLOC_IOVA_COHERENT		BIT(0)
+#define DMA_ALLOC_IOVA_FIRST_FIT	BIT(1)
+
+static unsigned int dma_attrs_to_alloc_flags(unsigned long attrs, bool coherent)
+{
+	unsigned int flags = 0;
+
+	if (coherent)
+		flags |= DMA_ALLOC_IOVA_COHERENT;
+	if (attrs & DMA_ATTR_LOW_ADDRESS)
+		flags |= DMA_ALLOC_IOVA_FIRST_FIT;
+	return flags;
+}
 
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		struct device *dev, size_t size, unsigned int flags)
@@ -625,13 +637,23 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
 
 	/* Try to get PCI devices a SAC address */
-	if (dma_limit > DMA_BIT_MASK(32) && !iommu_dma_forcedac && dev_is_pci(dev))
-		iova = alloc_iova_fast(iovad, iova_len,
-				       DMA_BIT_MASK(32) >> shift, false);
+	if (dma_limit > DMA_BIT_MASK(32) && !iommu_dma_forcedac && dev_is_pci(dev)) {
+		if (unlikely(flags & DMA_ALLOC_IOVA_FIRST_FIT))
+			iova = alloc_iova_first_fit(iovad, iova_len,
+						    DMA_BIT_MASK(32) >> shift);
+		else
+			iova = alloc_iova_fast(iovad, iova_len,
+					      DMA_BIT_MASK(32) >> shift, false);
+	}
 
-	if (iova == IOVA_BAD_ADDR)
-		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
-				       true);
+	if (iova == IOVA_BAD_ADDR) {
+		if (unlikely(flags & DMA_ALLOC_IOVA_FIRST_FIT))
+			iova = alloc_iova_first_fit(iovad, iova_len,
+						    dma_limit >> shift);
+		else
+			iova = alloc_iova_fast(iovad, iova_len,
+						dma_limit >> shift, true);
+	}
 
 	if (iova != IOVA_BAD_ADDR)
 		return (dma_addr_t)iova << shift;
@@ -779,6 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 	struct iova_domain *iovad = &cookie->iovad;
 	bool coherent = dev_is_dma_coherent(dev);
 	int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, true);
 	unsigned int count, min_size, alloc_sizes = domain->pgsize_bitmap;
 	struct page **pages;
 	dma_addr_t iova;
@@ -804,7 +827,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 		return NULL;
 
 	size = iova_align(iovad, size);
-	iova = iommu_dma_alloc_iova(domain, dev, size, DMA_ALLOC_IOVA_COHERENT);
+	iova = iommu_dma_alloc_iova(domain, dev, size, flags);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
@@ -964,6 +987,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	phys_addr_t phys = page_to_phys(page) + offset;
 	bool coherent = dev_is_dma_coherent(dev);
 	int prot = dma_info_to_prot(dir, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, false);
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
@@ -1005,7 +1029,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 		arch_sync_dma_for_device(phys, size, dir);
 
-	iova = __iommu_dma_map(dev, phys, size, prot, 0);
+	iova = __iommu_dma_map(dev, phys, size, prot, flags);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
 		swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
 	return iova;
@@ -1152,6 +1176,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	struct iova_domain *iovad = &cookie->iovad;
 	struct scatterlist *s, *prev = NULL;
 	int prot = dma_info_to_prot(dir, dev_is_dma_coherent(dev), attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, false);
 	dma_addr_t iova;
 	size_t iova_len = 0;
 	unsigned long mask = dma_get_seg_boundary(dev);
@@ -1209,7 +1234,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 		prev = s;
 	}
 
-	iova = iommu_dma_alloc_iova(domain, dev, iova_len, 0);
+	iova = iommu_dma_alloc_iova(domain, dev, iova_len, flags);
 	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
@@ -1268,7 +1293,8 @@ static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	return __iommu_dma_map(dev, phys, size,
-			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO, 0);
+			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO,
+			dma_attrs_to_alloc_flags(attrs, false));
 }
 
 static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
@@ -1357,6 +1383,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 {
 	bool coherent = dev_is_dma_coherent(dev);
 	int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, true);
 	struct page *page = NULL;
 	void *cpu_addr;
 
@@ -1377,8 +1404,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 	if (!cpu_addr)
 		return NULL;
 
-	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot,
-			DMA_ALLOC_IOVA_COHERENT);
+	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot, flags);
 	if (*handle == DMA_MAPPING_ERROR) {
 		__iommu_dma_free(dev, size, cpu_addr);
 		return NULL;
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 5/6] iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Implement support for the DMA_ATTR_LOW_ADDRESS DMA attribute. If it has
been set, call alloc_iova_first_fit() instead of the alloc_iova_fast() to
allocate the new IOVA from the beginning of the address space.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/iommu/dma-iommu.c | 50 +++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index cb235b40303c..553c5b863e19 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -601,6 +601,18 @@ static int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
 }
 
 #define DMA_ALLOC_IOVA_COHERENT		BIT(0)
+#define DMA_ALLOC_IOVA_FIRST_FIT	BIT(1)
+
+static unsigned int dma_attrs_to_alloc_flags(unsigned long attrs, bool coherent)
+{
+	unsigned int flags = 0;
+
+	if (coherent)
+		flags |= DMA_ALLOC_IOVA_COHERENT;
+	if (attrs & DMA_ATTR_LOW_ADDRESS)
+		flags |= DMA_ALLOC_IOVA_FIRST_FIT;
+	return flags;
+}
 
 static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		struct device *dev, size_t size, unsigned int flags)
@@ -625,13 +637,23 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
 		dma_limit = min(dma_limit, (u64)domain->geometry.aperture_end);
 
 	/* Try to get PCI devices a SAC address */
-	if (dma_limit > DMA_BIT_MASK(32) && !iommu_dma_forcedac && dev_is_pci(dev))
-		iova = alloc_iova_fast(iovad, iova_len,
-				       DMA_BIT_MASK(32) >> shift, false);
+	if (dma_limit > DMA_BIT_MASK(32) && !iommu_dma_forcedac && dev_is_pci(dev)) {
+		if (unlikely(flags & DMA_ALLOC_IOVA_FIRST_FIT))
+			iova = alloc_iova_first_fit(iovad, iova_len,
+						    DMA_BIT_MASK(32) >> shift);
+		else
+			iova = alloc_iova_fast(iovad, iova_len,
+					      DMA_BIT_MASK(32) >> shift, false);
+	}
 
-	if (iova == IOVA_BAD_ADDR)
-		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
-				       true);
+	if (iova == IOVA_BAD_ADDR) {
+		if (unlikely(flags & DMA_ALLOC_IOVA_FIRST_FIT))
+			iova = alloc_iova_first_fit(iovad, iova_len,
+						    dma_limit >> shift);
+		else
+			iova = alloc_iova_fast(iovad, iova_len,
+						dma_limit >> shift, true);
+	}
 
 	if (iova != IOVA_BAD_ADDR)
 		return (dma_addr_t)iova << shift;
@@ -779,6 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 	struct iova_domain *iovad = &cookie->iovad;
 	bool coherent = dev_is_dma_coherent(dev);
 	int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, true);
 	unsigned int count, min_size, alloc_sizes = domain->pgsize_bitmap;
 	struct page **pages;
 	dma_addr_t iova;
@@ -804,7 +827,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 		return NULL;
 
 	size = iova_align(iovad, size);
-	iova = iommu_dma_alloc_iova(domain, dev, size, DMA_ALLOC_IOVA_COHERENT);
+	iova = iommu_dma_alloc_iova(domain, dev, size, flags);
 	if (iova == DMA_MAPPING_ERROR)
 		goto out_free_pages;
 
@@ -964,6 +987,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	phys_addr_t phys = page_to_phys(page) + offset;
 	bool coherent = dev_is_dma_coherent(dev);
 	int prot = dma_info_to_prot(dir, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, false);
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
@@ -1005,7 +1029,7 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 		arch_sync_dma_for_device(phys, size, dir);
 
-	iova = __iommu_dma_map(dev, phys, size, prot, 0);
+	iova = __iommu_dma_map(dev, phys, size, prot, flags);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(dev, phys))
 		swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
 	return iova;
@@ -1152,6 +1176,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	struct iova_domain *iovad = &cookie->iovad;
 	struct scatterlist *s, *prev = NULL;
 	int prot = dma_info_to_prot(dir, dev_is_dma_coherent(dev), attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, false);
 	dma_addr_t iova;
 	size_t iova_len = 0;
 	unsigned long mask = dma_get_seg_boundary(dev);
@@ -1209,7 +1234,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 		prev = s;
 	}
 
-	iova = iommu_dma_alloc_iova(domain, dev, iova_len, 0);
+	iova = iommu_dma_alloc_iova(domain, dev, iova_len, flags);
 	if (iova == DMA_MAPPING_ERROR) {
 		ret = -ENOMEM;
 		goto out_restore_sg;
@@ -1268,7 +1293,8 @@ static dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	return __iommu_dma_map(dev, phys, size,
-			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO, 0);
+			dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO,
+			dma_attrs_to_alloc_flags(attrs, false));
 }
 
 static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
@@ -1357,6 +1383,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 {
 	bool coherent = dev_is_dma_coherent(dev);
 	int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
+	unsigned int flags = dma_attrs_to_alloc_flags(attrs, true);
 	struct page *page = NULL;
 	void *cpu_addr;
 
@@ -1377,8 +1404,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
 	if (!cpu_addr)
 		return NULL;
 
-	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot,
-			DMA_ALLOC_IOVA_COHERENT);
+	*handle = __iommu_dma_map(dev, page_to_phys(page), size, ioprot, flags);
 	if (*handle == DMA_MAPPING_ERROR) {
 		__iommu_dma_free(dev, size, cpu_addr);
 		return NULL;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 6/6] media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

S5P-MFC driver relied on the way the ARM DMA-IOMMU glue code worked -
mainly it relied on the fact that the allocator used first-fit algorithm
and the first allocated buffer were at 0x0 DMA/IOVA address. This is not
true for the generic IOMMU-DMA glue code that will be used for ARM
architecture soon, so limit the dma_mask to size of the DMA window the
hardware can use and add the needed DMA attribute to force proper IOVA
allocation of the firmware buffer.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
index 761341934925..15c9c2273561 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
@@ -1196,8 +1196,12 @@ static int s5p_mfc_configure_common_memory(struct s5p_mfc_dev *mfc_dev)
 	if (!mfc_dev->mem_bitmap)
 		return -ENOMEM;
 
-	mfc_dev->mem_virt = dma_alloc_coherent(dev, mem_size,
-					       &mfc_dev->mem_base, GFP_KERNEL);
+	/* MFC v5 can access memory only via the 256M window */
+	if (exynos_is_iommu_available(dev) && !IS_MFCV6_PLUS(mfc_dev))
+		dma_set_mask_and_coherent(dev, SZ_256M - 1);
+
+	mfc_dev->mem_virt = dma_alloc_attrs(dev, mem_size, &mfc_dev->mem_base,
+					    GFP_KERNEL, DMA_ATTR_LOW_ADDRESS);
 	if (!mfc_dev->mem_virt) {
 		bitmap_free(mfc_dev->mem_bitmap);
 		dev_err(dev, "failed to preallocate %ld MiB for the firmware and context buffers\n",
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V2 6/6] media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS

[Ajay Kumar]

From: Marek Szyprowski <m.szyprowski@samsung.com>

S5P-MFC driver relied on the way the ARM DMA-IOMMU glue code worked -
mainly it relied on the fact that the allocator used first-fit algorithm
and the first allocated buffer were at 0x0 DMA/IOVA address. This is not
true for the generic IOMMU-DMA glue code that will be used for ARM
architecture soon, so limit the dma_mask to size of the DMA window the
hardware can use and add the needed DMA attribute to force proper IOVA
allocation of the firmware buffer.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
index 761341934925..15c9c2273561 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c
@@ -1196,8 +1196,12 @@ static int s5p_mfc_configure_common_memory(struct s5p_mfc_dev *mfc_dev)
 	if (!mfc_dev->mem_bitmap)
 		return -ENOMEM;
 
-	mfc_dev->mem_virt = dma_alloc_coherent(dev, mem_size,
-					       &mfc_dev->mem_base, GFP_KERNEL);
+	/* MFC v5 can access memory only via the 256M window */
+	if (exynos_is_iommu_available(dev) && !IS_MFCV6_PLUS(mfc_dev))
+		dma_set_mask_and_coherent(dev, SZ_256M - 1);
+
+	mfc_dev->mem_virt = dma_alloc_attrs(dev, mem_size, &mfc_dev->mem_base,
+					    GFP_KERNEL, DMA_ATTR_LOW_ADDRESS);
 	if (!mfc_dev->mem_virt) {
 		bitmap_free(mfc_dev->mem_bitmap);
 		dev_err(dev, "failed to preallocate %ld MiB for the firmware and context buffers\n",
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 0/6] IOMMU-DMA - support DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

Ping!

On Thu, May 12, 2022 at 9:09 AM Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>
> This patchset is a rebase of original patches from Marek Szyprowski:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2321261.html
>
> The patches have been rebased on Joro's IOMMU tree "next" branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git
>
> This patchset is needed to address the IOVA address dependency issue between
> firmware buffers and other buffers in Samsung s5p-mfc driver.
>
> There have been few discussions in the past on how to find a generic
> soultion for this issue, ranging from adding an entirely new API to choose
> IOVA window[1], to adding a DMA attribute DMA_ATTR_LOW_ADDRESS which handles
> buffer allocation from lower address[2].
> This is a continuation of initial work from Marek for approach [2].
> Patches have been tested with latest version of Samsung s5p-mfc driver.
>
> Changes since V1:
> [PATCH V2 1/6]
> - Rebase on latest tree.
>
> [PATCH V2 2/6]
> - Rebase on latest tree.
>   Added a missing check for iova_pfn in __iova_rcache_get()
>   Discard changes from drivers/iommu/intel/iommu.c which are not necessary
>
> [PATCH V2 3/6]
> - Rebase on latest tree.
>
> [PATCH V2 4/6]
> - Rebase on latest tree
>
> [PATCH V2 5/6]
> - Rebase on latest tree
>
> [PATCH V2 6/6]
> - Rebase on latest tree.
>
> Marek Szyprowski (6):
>   dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute
>   iommu: iova: properly handle 0 as a valid IOVA address
>   iommu: iova: add support for 'first-fit' algorithm
>   iommu: dma-iommu: refactor iommu_dma_alloc_iova()
>   iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS
>   media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS
>
> References:
> [1]
> https://lore.kernel.org/linux-iommu/20200811054912.GA301@infradead.org/
>
> [2]
> https://lore.kernel.org/linux-mm/bff57cbe-2247-05e1-9059-d9c66d64c407@arm.com
>
>  drivers/iommu/dma-iommu.c                     | 77 +++++++++++-----
>  drivers/iommu/iova.c                          | 91 ++++++++++++++++++-
>  .../media/platform/samsung/s5p-mfc/s5p_mfc.c  |  8 +-
>  include/linux/dma-mapping.h                   |  6 ++
>  include/linux/iova.h                          |  3 +
>  5 files changed, 156 insertions(+), 29 deletions(-)
>
>
> base-commit: faf93cfaadfaaff2a5c35d6301b45aa2f6e4ddb2
> --
> 2.17.1
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 0/6] IOMMU-DMA - support DMA_ATTR_LOW_ADDRESS attribute

[Ajay Kumar]

Ping!

On Thu, May 12, 2022 at 9:09 AM Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>
> This patchset is a rebase of original patches from Marek Szyprowski:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2321261.html
>
> The patches have been rebased on Joro's IOMMU tree "next" branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git
>
> This patchset is needed to address the IOVA address dependency issue between
> firmware buffers and other buffers in Samsung s5p-mfc driver.
>
> There have been few discussions in the past on how to find a generic
> soultion for this issue, ranging from adding an entirely new API to choose
> IOVA window[1], to adding a DMA attribute DMA_ATTR_LOW_ADDRESS which handles
> buffer allocation from lower address[2].
> This is a continuation of initial work from Marek for approach [2].
> Patches have been tested with latest version of Samsung s5p-mfc driver.
>
> Changes since V1:
> [PATCH V2 1/6]
> - Rebase on latest tree.
>
> [PATCH V2 2/6]
> - Rebase on latest tree.
>   Added a missing check for iova_pfn in __iova_rcache_get()
>   Discard changes from drivers/iommu/intel/iommu.c which are not necessary
>
> [PATCH V2 3/6]
> - Rebase on latest tree.
>
> [PATCH V2 4/6]
> - Rebase on latest tree
>
> [PATCH V2 5/6]
> - Rebase on latest tree
>
> [PATCH V2 6/6]
> - Rebase on latest tree.
>
> Marek Szyprowski (6):
>   dma-mapping: add DMA_ATTR_LOW_ADDRESS attribute
>   iommu: iova: properly handle 0 as a valid IOVA address
>   iommu: iova: add support for 'first-fit' algorithm
>   iommu: dma-iommu: refactor iommu_dma_alloc_iova()
>   iommu: dma-iommu: add support for DMA_ATTR_LOW_ADDRESS
>   media: platform: s5p-mfc: use DMA_ATTR_LOW_ADDRESS
>
> References:
> [1]
> https://lore.kernel.org/linux-iommu/20200811054912.GA301@infradead.org/
>
> [2]
> https://lore.kernel.org/linux-mm/bff57cbe-2247-05e1-9059-d9c66d64c407@arm.com
>
>  drivers/iommu/dma-iommu.c                     | 77 +++++++++++-----
>  drivers/iommu/iova.c                          | 91 ++++++++++++++++++-
>  .../media/platform/samsung/s5p-mfc/s5p_mfc.c  |  8 +-
>  include/linux/dma-mapping.h                   |  6 ++
>  include/linux/iova.h                          |  3 +
>  5 files changed, 156 insertions(+), 29 deletions(-)
>
>
> base-commit: faf93cfaadfaaff2a5c35d6301b45aa2f6e4ddb2
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Robin Murphy]

On 2022-05-11 13:15, Ajay Kumar wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Zero is a valid DMA and IOVA address on many architectures, so adjust the
> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
> (~0UL) is introduced as a generic value for the error case. Adjust all
> callers of the alloc_iova_fast() function for the new return value.

And when does anything actually need this? In fact if you were to stop 
iommu-dma from reserving IOVA 0 - which you don't - it would only show 
how patch #3 is broken.

Also note that it's really nothing to do with architectures either way; 
iommu-dma simply chooses to reserve IOVA 0 for its own convenience, 
mostly because it can. Much the same way that 0 is typically a valid CPU 
VA, but mapping something meaningful there is just asking for a world of 
pain debugging NULL-dereference bugs.

Robin.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>   drivers/iommu/dma-iommu.c | 16 +++++++++-------
>   drivers/iommu/iova.c      | 13 +++++++++----
>   include/linux/iova.h      |  1 +
>   3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1ca85d37eeab..16218d6a0703 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>   {
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>   	struct iova_domain *iovad = &cookie->iovad;
> -	unsigned long shift, iova_len, iova = 0;
> +	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
>   
>   	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
>   		cookie->msi_iova += size;
> @@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>   		iova = alloc_iova_fast(iovad, iova_len,
>   				       DMA_BIT_MASK(32) >> shift, false);
>   
> -	if (!iova)
> +	if (iova == IOVA_BAD_ADDR)
>   		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
>   				       true);
>   
> -	return (dma_addr_t)iova << shift;
> +	if (iova != IOVA_BAD_ADDR)
> +		return (dma_addr_t)iova << shift;
> +	return DMA_MAPPING_ERROR;
>   }
>   
>   static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
> @@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
>   	size = iova_align(iovad, size + iova_off);
>   
>   	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		return DMA_MAPPING_ERROR;
>   
>   	if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
> @@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
>   
>   	size = iova_align(iovad, size);
>   	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		goto out_free_pages;
>   
>   	if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
> @@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	}
>   
>   	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
> -	if (!iova) {
> +	if (iova == DMA_MAPPING_ERROR) {
>   		ret = -ENOMEM;
>   		goto out_restore_sg;
>   	}
> @@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>   		return NULL;
>   
>   	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		goto out_free_page;
>   
>   	if (iommu_map(domain, iova, msi_addr, size, prot))
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index db77aa675145..ae0fe0a6714e 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
>    * This function tries to satisfy an iova allocation from the rcache,
>    * and falls back to regular allocation on failure. If regular allocation
>    * fails too and the flush_rcache flag is set then the rcache will be flushed.
> + * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
> + * of a failure.
>   */
>   unsigned long
>   alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> @@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
>   		size = roundup_pow_of_two(size);
>   
>   	iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
> -	if (iova_pfn)
> +	if (iova_pfn != IOVA_BAD_ADDR)
>   		return iova_pfn;
>   
>   retry:
> @@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
>   		unsigned int cpu;
>   
>   		if (!flush_rcache)
> -			return 0;
> +			return IOVA_BAD_ADDR;
>   
>   		/* Try replenishing IOVAs by flushing rcache. */
>   		flush_rcache = false;
> @@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
>   				       unsigned long limit_pfn)
>   {
>   	struct iova_cpu_rcache *cpu_rcache;
> -	unsigned long iova_pfn = 0;
> +	unsigned long iova_pfn = IOVA_BAD_ADDR;
>   	bool has_pfn = false;
>   	unsigned long flags;
>   
> @@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
>   
>   	spin_unlock_irqrestore(&cpu_rcache->lock, flags);
>   
> +	if (!iova_pfn)
> +		return IOVA_BAD_ADDR;
> +
>   	return iova_pfn;
>   }
>   
> @@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>   	unsigned int log_size = order_base_2(size);
>   
>   	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
> -		return 0;
> +		return IOVA_BAD_ADDR;
>   
>   	return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
>   }
> diff --git a/include/linux/iova.h b/include/linux/iova.h
> index 320a70e40233..46b5b10c532b 100644
> --- a/include/linux/iova.h
> +++ b/include/linux/iova.h
> @@ -21,6 +21,7 @@ struct iova {
>   	unsigned long	pfn_lo; /* Lowest allocated pfn */
>   };
>   
> +#define IOVA_BAD_ADDR	(~0UL)
>   
>   struct iova_rcache;
>   
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Robin Murphy]

On 2022-05-11 13:15, Ajay Kumar wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Zero is a valid DMA and IOVA address on many architectures, so adjust the
> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
> (~0UL) is introduced as a generic value for the error case. Adjust all
> callers of the alloc_iova_fast() function for the new return value.

And when does anything actually need this? In fact if you were to stop 
iommu-dma from reserving IOVA 0 - which you don't - it would only show 
how patch #3 is broken.

Also note that it's really nothing to do with architectures either way; 
iommu-dma simply chooses to reserve IOVA 0 for its own convenience, 
mostly because it can. Much the same way that 0 is typically a valid CPU 
VA, but mapping something meaningful there is just asking for a world of 
pain debugging NULL-dereference bugs.

Robin.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>   drivers/iommu/dma-iommu.c | 16 +++++++++-------
>   drivers/iommu/iova.c      | 13 +++++++++----
>   include/linux/iova.h      |  1 +
>   3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1ca85d37eeab..16218d6a0703 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>   {
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>   	struct iova_domain *iovad = &cookie->iovad;
> -	unsigned long shift, iova_len, iova = 0;
> +	unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
>   
>   	if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
>   		cookie->msi_iova += size;
> @@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
>   		iova = alloc_iova_fast(iovad, iova_len,
>   				       DMA_BIT_MASK(32) >> shift, false);
>   
> -	if (!iova)
> +	if (iova == IOVA_BAD_ADDR)
>   		iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
>   				       true);
>   
> -	return (dma_addr_t)iova << shift;
> +	if (iova != IOVA_BAD_ADDR)
> +		return (dma_addr_t)iova << shift;
> +	return DMA_MAPPING_ERROR;
>   }
>   
>   static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
> @@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
>   	size = iova_align(iovad, size + iova_off);
>   
>   	iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		return DMA_MAPPING_ERROR;
>   
>   	if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
> @@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
>   
>   	size = iova_align(iovad, size);
>   	iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		goto out_free_pages;
>   
>   	if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
> @@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	}
>   
>   	iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
> -	if (!iova) {
> +	if (iova == DMA_MAPPING_ERROR) {
>   		ret = -ENOMEM;
>   		goto out_restore_sg;
>   	}
> @@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
>   		return NULL;
>   
>   	iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> -	if (!iova)
> +	if (iova == DMA_MAPPING_ERROR)
>   		goto out_free_page;
>   
>   	if (iommu_map(domain, iova, msi_addr, size, prot))
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index db77aa675145..ae0fe0a6714e 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
>    * This function tries to satisfy an iova allocation from the rcache,
>    * and falls back to regular allocation on failure. If regular allocation
>    * fails too and the flush_rcache flag is set then the rcache will be flushed.
> + * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
> + * of a failure.
>   */
>   unsigned long
>   alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> @@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
>   		size = roundup_pow_of_two(size);
>   
>   	iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
> -	if (iova_pfn)
> +	if (iova_pfn != IOVA_BAD_ADDR)
>   		return iova_pfn;
>   
>   retry:
> @@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
>   		unsigned int cpu;
>   
>   		if (!flush_rcache)
> -			return 0;
> +			return IOVA_BAD_ADDR;
>   
>   		/* Try replenishing IOVAs by flushing rcache. */
>   		flush_rcache = false;
> @@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
>   				       unsigned long limit_pfn)
>   {
>   	struct iova_cpu_rcache *cpu_rcache;
> -	unsigned long iova_pfn = 0;
> +	unsigned long iova_pfn = IOVA_BAD_ADDR;
>   	bool has_pfn = false;
>   	unsigned long flags;
>   
> @@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
>   
>   	spin_unlock_irqrestore(&cpu_rcache->lock, flags);
>   
> +	if (!iova_pfn)
> +		return IOVA_BAD_ADDR;
> +
>   	return iova_pfn;
>   }
>   
> @@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>   	unsigned int log_size = order_base_2(size);
>   
>   	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
> -		return 0;
> +		return IOVA_BAD_ADDR;
>   
>   	return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
>   }
> diff --git a/include/linux/iova.h b/include/linux/iova.h
> index 320a70e40233..46b5b10c532b 100644
> --- a/include/linux/iova.h
> +++ b/include/linux/iova.h
> @@ -21,6 +21,7 @@ struct iova {
>   	unsigned long	pfn_lo; /* Lowest allocated pfn */
>   };
>   
> +#define IOVA_BAD_ADDR	(~0UL)
>   
>   struct iova_rcache;
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Ajay Kumar]

Hi Robin

On Mon, May 23, 2022 at 11:00 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-05-11 13:15, Ajay Kumar wrote:
> > From: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> > Zero is a valid DMA and IOVA address on many architectures, so adjust the
> > IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
> > (~0UL) is introduced as a generic value for the error case. Adjust all
> > callers of the alloc_iova_fast() function for the new return value.
>
> And when does anything actually need this? In fact if you were to stop
> iommu-dma from reserving IOVA 0 - which you don't - it would only show
> how patch #3 is broken.
Right! Since the IOVA allocation happens from higher addr to lower addr,
hitting this (IOVA==0) case means out of IOVA space which is highly unlikely.

> Also note that it's really nothing to do with architectures either way;
> iommu-dma simply chooses to reserve IOVA 0 for its own convenience,
> mostly because it can. Much the same way that 0 is typically a valid CPU
> VA, but mapping something meaningful there is just asking for a world of
> pain debugging NULL-dereference bugs.
>
> Robin.
This makes sense, let me think about managing the PFN at lowest address
in some other way.

Thanks,
Ajay Kumar

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> > ---
> >   drivers/iommu/dma-iommu.c | 16 +++++++++-------
> >   drivers/iommu/iova.c      | 13 +++++++++----
> >   include/linux/iova.h      |  1 +
> >   3 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 1ca85d37eeab..16218d6a0703 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
> >   {
> >       struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >       struct iova_domain *iovad = &cookie->iovad;
> > -     unsigned long shift, iova_len, iova = 0;
> > +     unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
> >
> >       if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
> >               cookie->msi_iova += size;
> > @@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
> >               iova = alloc_iova_fast(iovad, iova_len,
> >                                      DMA_BIT_MASK(32) >> shift, false);
> >
> > -     if (!iova)
> > +     if (iova == IOVA_BAD_ADDR)
> >               iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
> >                                      true);
> >
> > -     return (dma_addr_t)iova << shift;
> > +     if (iova != IOVA_BAD_ADDR)
> > +             return (dma_addr_t)iova << shift;
> > +     return DMA_MAPPING_ERROR;
> >   }
> >
> >   static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
> > @@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
> >       size = iova_align(iovad, size + iova_off);
> >
> >       iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               return DMA_MAPPING_ERROR;
> >
> >       if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
> > @@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
> >
> >       size = iova_align(iovad, size);
> >       iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               goto out_free_pages;
> >
> >       if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
> > @@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> >       }
> >
> >       iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
> > -     if (!iova) {
> > +     if (iova == DMA_MAPPING_ERROR) {
> >               ret = -ENOMEM;
> >               goto out_restore_sg;
> >       }
> > @@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
> >               return NULL;
> >
> >       iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               goto out_free_page;
> >
> >       if (iommu_map(domain, iova, msi_addr, size, prot))
> > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> > index db77aa675145..ae0fe0a6714e 100644
> > --- a/drivers/iommu/iova.c
> > +++ b/drivers/iommu/iova.c
> > @@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
> >    * This function tries to satisfy an iova allocation from the rcache,
> >    * and falls back to regular allocation on failure. If regular allocation
> >    * fails too and the flush_rcache flag is set then the rcache will be flushed.
> > + * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
> > + * of a failure.
> >   */
> >   unsigned long
> >   alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> > @@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> >               size = roundup_pow_of_two(size);
> >
> >       iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
> > -     if (iova_pfn)
> > +     if (iova_pfn != IOVA_BAD_ADDR)
> >               return iova_pfn;
> >
> >   retry:
> > @@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> >               unsigned int cpu;
> >
> >               if (!flush_rcache)
> > -                     return 0;
> > +                     return IOVA_BAD_ADDR;
> >
> >               /* Try replenishing IOVAs by flushing rcache. */
> >               flush_rcache = false;
> > @@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
> >                                      unsigned long limit_pfn)
> >   {
> >       struct iova_cpu_rcache *cpu_rcache;
> > -     unsigned long iova_pfn = 0;
> > +     unsigned long iova_pfn = IOVA_BAD_ADDR;
> >       bool has_pfn = false;
> >       unsigned long flags;
> >
> > @@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
> >
> >       spin_unlock_irqrestore(&cpu_rcache->lock, flags);
> >
> > +     if (!iova_pfn)
> > +             return IOVA_BAD_ADDR;
> > +
> >       return iova_pfn;
> >   }
> >
> > @@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
> >       unsigned int log_size = order_base_2(size);
> >
> >       if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
> > -             return 0;
> > +             return IOVA_BAD_ADDR;
> >
> >       return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
> >   }
> > diff --git a/include/linux/iova.h b/include/linux/iova.h
> > index 320a70e40233..46b5b10c532b 100644
> > --- a/include/linux/iova.h
> > +++ b/include/linux/iova.h
> > @@ -21,6 +21,7 @@ struct iova {
> >       unsigned long   pfn_lo; /* Lowest allocated pfn */
> >   };
> >
> > +#define IOVA_BAD_ADDR        (~0UL)
> >
> >   struct iova_rcache;
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Ajay Kumar]

Hi Robin

On Mon, May 23, 2022 at 11:00 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-05-11 13:15, Ajay Kumar wrote:
> > From: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> > Zero is a valid DMA and IOVA address on many architectures, so adjust the
> > IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
> > (~0UL) is introduced as a generic value for the error case. Adjust all
> > callers of the alloc_iova_fast() function for the new return value.
>
> And when does anything actually need this? In fact if you were to stop
> iommu-dma from reserving IOVA 0 - which you don't - it would only show
> how patch #3 is broken.
Right! Since the IOVA allocation happens from higher addr to lower addr,
hitting this (IOVA==0) case means out of IOVA space which is highly unlikely.

> Also note that it's really nothing to do with architectures either way;
> iommu-dma simply chooses to reserve IOVA 0 for its own convenience,
> mostly because it can. Much the same way that 0 is typically a valid CPU
> VA, but mapping something meaningful there is just asking for a world of
> pain debugging NULL-dereference bugs.
>
> Robin.
This makes sense, let me think about managing the PFN at lowest address
in some other way.

Thanks,
Ajay Kumar

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> > ---
> >   drivers/iommu/dma-iommu.c | 16 +++++++++-------
> >   drivers/iommu/iova.c      | 13 +++++++++----
> >   include/linux/iova.h      |  1 +
> >   3 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 1ca85d37eeab..16218d6a0703 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -605,7 +605,7 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
> >   {
> >       struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >       struct iova_domain *iovad = &cookie->iovad;
> > -     unsigned long shift, iova_len, iova = 0;
> > +     unsigned long shift, iova_len, iova = IOVA_BAD_ADDR;
> >
> >       if (cookie->type == IOMMU_DMA_MSI_COOKIE) {
> >               cookie->msi_iova += size;
> > @@ -625,11 +625,13 @@ static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
> >               iova = alloc_iova_fast(iovad, iova_len,
> >                                      DMA_BIT_MASK(32) >> shift, false);
> >
> > -     if (!iova)
> > +     if (iova == IOVA_BAD_ADDR)
> >               iova = alloc_iova_fast(iovad, iova_len, dma_limit >> shift,
> >                                      true);
> >
> > -     return (dma_addr_t)iova << shift;
> > +     if (iova != IOVA_BAD_ADDR)
> > +             return (dma_addr_t)iova << shift;
> > +     return DMA_MAPPING_ERROR;
> >   }
> >
> >   static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
> > @@ -688,7 +690,7 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
> >       size = iova_align(iovad, size + iova_off);
> >
> >       iova = iommu_dma_alloc_iova(domain, size, dma_mask, dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               return DMA_MAPPING_ERROR;
> >
> >       if (iommu_map_atomic(domain, iova, phys - iova_off, size, prot)) {
> > @@ -799,7 +801,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
> >
> >       size = iova_align(iovad, size);
> >       iova = iommu_dma_alloc_iova(domain, size, dev->coherent_dma_mask, dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               goto out_free_pages;
> >
> >       if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
> > @@ -1204,7 +1206,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> >       }
> >
> >       iova = iommu_dma_alloc_iova(domain, iova_len, dma_get_mask(dev), dev);
> > -     if (!iova) {
> > +     if (iova == DMA_MAPPING_ERROR) {
> >               ret = -ENOMEM;
> >               goto out_restore_sg;
> >       }
> > @@ -1516,7 +1518,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
> >               return NULL;
> >
> >       iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> > -     if (!iova)
> > +     if (iova == DMA_MAPPING_ERROR)
> >               goto out_free_page;
> >
> >       if (iommu_map(domain, iova, msi_addr, size, prot))
> > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> > index db77aa675145..ae0fe0a6714e 100644
> > --- a/drivers/iommu/iova.c
> > +++ b/drivers/iommu/iova.c
> > @@ -429,6 +429,8 @@ EXPORT_SYMBOL_GPL(free_iova);
> >    * This function tries to satisfy an iova allocation from the rcache,
> >    * and falls back to regular allocation on failure. If regular allocation
> >    * fails too and the flush_rcache flag is set then the rcache will be flushed.
> > + * Returns a pfn the allocated iova starts at or IOVA_BAD_ADDR in the case
> > + * of a failure.
> >   */
> >   unsigned long
> >   alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> > @@ -447,7 +449,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> >               size = roundup_pow_of_two(size);
> >
> >       iova_pfn = iova_rcache_get(iovad, size, limit_pfn + 1);
> > -     if (iova_pfn)
> > +     if (iova_pfn != IOVA_BAD_ADDR)
> >               return iova_pfn;
> >
> >   retry:
> > @@ -456,7 +458,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long size,
> >               unsigned int cpu;
> >
> >               if (!flush_rcache)
> > -                     return 0;
> > +                     return IOVA_BAD_ADDR;
> >
> >               /* Try replenishing IOVAs by flushing rcache. */
> >               flush_rcache = false;
> > @@ -831,7 +833,7 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
> >                                      unsigned long limit_pfn)
> >   {
> >       struct iova_cpu_rcache *cpu_rcache;
> > -     unsigned long iova_pfn = 0;
> > +     unsigned long iova_pfn = IOVA_BAD_ADDR;
> >       bool has_pfn = false;
> >       unsigned long flags;
> >
> > @@ -858,6 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache,
> >
> >       spin_unlock_irqrestore(&cpu_rcache->lock, flags);
> >
> > +     if (!iova_pfn)
> > +             return IOVA_BAD_ADDR;
> > +
> >       return iova_pfn;
> >   }
> >
> > @@ -873,7 +878,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
> >       unsigned int log_size = order_base_2(size);
> >
> >       if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
> > -             return 0;
> > +             return IOVA_BAD_ADDR;
> >
> >       return __iova_rcache_get(&iovad->rcaches[log_size], limit_pfn - size);
> >   }
> > diff --git a/include/linux/iova.h b/include/linux/iova.h
> > index 320a70e40233..46b5b10c532b 100644
> > --- a/include/linux/iova.h
> > +++ b/include/linux/iova.h
> > @@ -21,6 +21,7 @@ struct iova {
> >       unsigned long   pfn_lo; /* Lowest allocated pfn */
> >   };
> >
> > +#define IOVA_BAD_ADDR        (~0UL)
> >
> >   struct iova_rcache;
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Marek Szyprowski]

Hi Robin,

On 23.05.2022 19:30, Robin Murphy wrote:
> On 2022-05-11 13:15, Ajay Kumar wrote:
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Zero is a valid DMA and IOVA address on many architectures, so adjust 
>> the
>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>> (~0UL) is introduced as a generic value for the error case. Adjust all
>> callers of the alloc_iova_fast() function for the new return value.
>
> And when does anything actually need this? In fact if you were to stop 
> iommu-dma from reserving IOVA 0 - which you don't - it would only show 
> how patch #3 is broken.

I don't get why you says that patch #3 is broken. Them main issue with 
address 0 being reserved appears when one uses first fit allocation 
algorithm. In such case the first allocation will be at the 0 address, 
what causes some issues. This patch simply makes the whole iova related 
code capable of such case. This mimics the similar code already used on 
ARM 32bit. There are drivers that rely on the way the IOVAs are 
allocated. This is especially important for the drivers for devices with 
limited addressing capabilities. And there exists such and they can be 
even behind the IOMMU.

Lets focus on the s5p-mfc driver and the way one of the supported 
hardware does the DMA. The hardware has very limited DMA window (256M) 
and addresses all memory buffers as an offset from the firmware base. 
Currently it has been assumed that the first allocated buffer will be on 
the beginning of the IOVA space. This worked fine on ARM 32bit and 
supporting such case is a target of this patchset.


> Also note that it's really nothing to do with architecture either way; 
> iommu-dma simply chooses to reserve IOVA 0 for its own convenience, 
> mostly because it can. Much the same way that 0 is typically a valid 
> CPU VA, but mapping something meaningful there is just asking for a 
> world of pain debugging NULL-dereference bugs.

Right that it is not directly related to the architecture, but it is 
much more common case for some architectures where DMA (IOVA) address = 
physical address + some offset. The commit message can be rephrased, 
though.

I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also 
uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation 
algorithm is used no one has ever need to consider such case so far.


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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Marek Szyprowski]

Hi Robin,

On 23.05.2022 19:30, Robin Murphy wrote:
> On 2022-05-11 13:15, Ajay Kumar wrote:
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Zero is a valid DMA and IOVA address on many architectures, so adjust 
>> the
>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>> (~0UL) is introduced as a generic value for the error case. Adjust all
>> callers of the alloc_iova_fast() function for the new return value.
>
> And when does anything actually need this? In fact if you were to stop 
> iommu-dma from reserving IOVA 0 - which you don't - it would only show 
> how patch #3 is broken.

I don't get why you says that patch #3 is broken. Them main issue with 
address 0 being reserved appears when one uses first fit allocation 
algorithm. In such case the first allocation will be at the 0 address, 
what causes some issues. This patch simply makes the whole iova related 
code capable of such case. This mimics the similar code already used on 
ARM 32bit. There are drivers that rely on the way the IOVAs are 
allocated. This is especially important for the drivers for devices with 
limited addressing capabilities. And there exists such and they can be 
even behind the IOMMU.

Lets focus on the s5p-mfc driver and the way one of the supported 
hardware does the DMA. The hardware has very limited DMA window (256M) 
and addresses all memory buffers as an offset from the firmware base. 
Currently it has been assumed that the first allocated buffer will be on 
the beginning of the IOVA space. This worked fine on ARM 32bit and 
supporting such case is a target of this patchset.


> Also note that it's really nothing to do with architecture either way; 
> iommu-dma simply chooses to reserve IOVA 0 for its own convenience, 
> mostly because it can. Much the same way that 0 is typically a valid 
> CPU VA, but mapping something meaningful there is just asking for a 
> world of pain debugging NULL-dereference bugs.

Right that it is not directly related to the architecture, but it is 
much more common case for some architectures where DMA (IOVA) address = 
physical address + some offset. The commit message can be rephrased, 
though.

I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also 
uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation 
algorithm is used no one has ever need to consider such case so far.


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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Robin Murphy]

On 2022-06-02 16:58, Marek Szyprowski wrote:
> Hi Robin,
> 
> On 23.05.2022 19:30, Robin Murphy wrote:
>> On 2022-05-11 13:15, Ajay Kumar wrote:
>>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>> Zero is a valid DMA and IOVA address on many architectures, so adjust
>>> the
>>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>>> (~0UL) is introduced as a generic value for the error case. Adjust all
>>> callers of the alloc_iova_fast() function for the new return value.
>>
>> And when does anything actually need this? In fact if you were to stop
>> iommu-dma from reserving IOVA 0 - which you don't - it would only show
>> how patch #3 is broken.
> 
> I don't get why you says that patch #3 is broken.

My mistake, in fact it's already just as broken either way - I forgot 
that that's done implicitly via iovad->start_pfn rather than an actual 
reserve_iova() entry. I see there is an initial calculation attempting 
to honour start_pfn in patch #3, but it's always immediately 
overwritten. More critically, the rb_first()/rb_next walk means the 
starting point can only creep inevitably upwards as older allocations 
are freed, so will end up pathologically stuck at or above limit_pfn and 
unable to recover. At best it's more "next-fit" than "first-fit", and 
TBH the fact that it could ever even allocate anything at all in an 
empty domain makes me want to change the definition of IOVA_ANCHOR ;)

> Them main issue with
> address 0 being reserved appears when one uses first fit allocation
> algorithm. In such case the first allocation will be at the 0 address,
> what causes some issues. This patch simply makes the whole iova related
> code capable of such case. This mimics the similar code already used on
> ARM 32bit. There are drivers that rely on the way the IOVAs are
> allocated. This is especially important for the drivers for devices with
> limited addressing capabilities. And there exists such and they can be
> even behind the IOMMU.
> 
> Lets focus on the s5p-mfc driver and the way one of the supported
> hardware does the DMA. The hardware has very limited DMA window (256M)
> and addresses all memory buffers as an offset from the firmware base.
> Currently it has been assumed that the first allocated buffer will be on
> the beginning of the IOVA space. This worked fine on ARM 32bit and
> supporting such case is a target of this patchset.

I still understand the use-case; I object to a solution where PFN 0 is 
always reserved yet sometimes allocated. Not to mention the slightly 
more fundamental thing of the whole lot not actually working the way 
it's supposed to.

I also remain opposed to baking in secret ABIs where allocators are 
expected to honour a particular undocumented behaviour. FWIW I've had 
plans for a while now to make the IOVA walk bidirectional to make the 
existing retry case more efficient, at which point it's then almost 
trivial to implement "bottom-up" allocation, which I'm thinking I might 
then force on by default for CONFIG_ARM to minimise any further 
surprises for v2 of the default domain conversion. However I'm 
increasingly less convinced that it's really sufficient for your case, 
and am leaning back towards the opinion that any driver with really 
specific constraints on how its DMA addresses are laid out should not be 
passively relying on a generic allocator to do exactly what it wants. A 
simple flag saying "try to allocate DMA addresses bottom-up" doesn't 
actually mean "allocate this thing on a 256MB-aligned address and 
everything subsequent within a 256MB window above it", so let's not 
build a fragile house of cards on top of pretending that it does.

>> Also note that it's really nothing to do with architecture either way;
>> iommu-dma simply chooses to reserve IOVA 0 for its own convenience,
>> mostly because it can. Much the same way that 0 is typically a valid
>> CPU VA, but mapping something meaningful there is just asking for a
>> world of pain debugging NULL-dereference bugs.
> 
> Right that it is not directly related to the architecture, but it is
> much more common case for some architectures where DMA (IOVA) address =
> physical address + some offset. The commit message can be rephrased,
> though.
> 
> I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also
> uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation
> algorithm is used no one has ever need to consider such case so far.

Because it's the most convenient and efficient thing to do. Remember we 
tried to use 0 for DMA_MAPPING_ERROR too, but that turned out to be a 
usable physical RAM address on some systems. With a virtual address 
space, on the other hand, we're free to do whatever we want; that's kind 
of the point.

Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Robin Murphy]

On 2022-06-02 16:58, Marek Szyprowski wrote:
> Hi Robin,
> 
> On 23.05.2022 19:30, Robin Murphy wrote:
>> On 2022-05-11 13:15, Ajay Kumar wrote:
>>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>> Zero is a valid DMA and IOVA address on many architectures, so adjust
>>> the
>>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>>> (~0UL) is introduced as a generic value for the error case. Adjust all
>>> callers of the alloc_iova_fast() function for the new return value.
>>
>> And when does anything actually need this? In fact if you were to stop
>> iommu-dma from reserving IOVA 0 - which you don't - it would only show
>> how patch #3 is broken.
> 
> I don't get why you says that patch #3 is broken.

My mistake, in fact it's already just as broken either way - I forgot 
that that's done implicitly via iovad->start_pfn rather than an actual 
reserve_iova() entry. I see there is an initial calculation attempting 
to honour start_pfn in patch #3, but it's always immediately 
overwritten. More critically, the rb_first()/rb_next walk means the 
starting point can only creep inevitably upwards as older allocations 
are freed, so will end up pathologically stuck at or above limit_pfn and 
unable to recover. At best it's more "next-fit" than "first-fit", and 
TBH the fact that it could ever even allocate anything at all in an 
empty domain makes me want to change the definition of IOVA_ANCHOR ;)

> Them main issue with
> address 0 being reserved appears when one uses first fit allocation
> algorithm. In such case the first allocation will be at the 0 address,
> what causes some issues. This patch simply makes the whole iova related
> code capable of such case. This mimics the similar code already used on
> ARM 32bit. There are drivers that rely on the way the IOVAs are
> allocated. This is especially important for the drivers for devices with
> limited addressing capabilities. And there exists such and they can be
> even behind the IOMMU.
> 
> Lets focus on the s5p-mfc driver and the way one of the supported
> hardware does the DMA. The hardware has very limited DMA window (256M)
> and addresses all memory buffers as an offset from the firmware base.
> Currently it has been assumed that the first allocated buffer will be on
> the beginning of the IOVA space. This worked fine on ARM 32bit and
> supporting such case is a target of this patchset.

I still understand the use-case; I object to a solution where PFN 0 is 
always reserved yet sometimes allocated. Not to mention the slightly 
more fundamental thing of the whole lot not actually working the way 
it's supposed to.

I also remain opposed to baking in secret ABIs where allocators are 
expected to honour a particular undocumented behaviour. FWIW I've had 
plans for a while now to make the IOVA walk bidirectional to make the 
existing retry case more efficient, at which point it's then almost 
trivial to implement "bottom-up" allocation, which I'm thinking I might 
then force on by default for CONFIG_ARM to minimise any further 
surprises for v2 of the default domain conversion. However I'm 
increasingly less convinced that it's really sufficient for your case, 
and am leaning back towards the opinion that any driver with really 
specific constraints on how its DMA addresses are laid out should not be 
passively relying on a generic allocator to do exactly what it wants. A 
simple flag saying "try to allocate DMA addresses bottom-up" doesn't 
actually mean "allocate this thing on a 256MB-aligned address and 
everything subsequent within a 256MB window above it", so let's not 
build a fragile house of cards on top of pretending that it does.

>> Also note that it's really nothing to do with architecture either way;
>> iommu-dma simply chooses to reserve IOVA 0 for its own convenience,
>> mostly because it can. Much the same way that 0 is typically a valid
>> CPU VA, but mapping something meaningful there is just asking for a
>> world of pain debugging NULL-dereference bugs.
> 
> Right that it is not directly related to the architecture, but it is
> much more common case for some architectures where DMA (IOVA) address =
> physical address + some offset. The commit message can be rephrased,
> though.
> 
> I see no reason to forbid the 0 as a valid IOVA. The DMA-mapping also
> uses DMA_MAPPING_ERROR as ~0. It looks that when last fit allocation
> algorithm is used no one has ever need to consider such case so far.

Because it's the most convenient and efficient thing to do. Remember we 
tried to use 0 for DMA_MAPPING_ERROR too, but that turned out to be a 
usable physical RAM address on some systems. With a virtual address 
space, on the other hand, we're free to do whatever we want; that's kind 
of the point.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Marek Szyprowski]

Hi Robin,

On 06.06.2022 14:38, Robin Murphy wrote:
> On 2022-06-02 16:58, Marek Szyprowski wrote:
>> On 23.05.2022 19:30, Robin Murphy wrote:
>>> On 2022-05-11 13:15, Ajay Kumar wrote:
>>>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>
>>>> Zero is a valid DMA and IOVA address on many architectures, so adjust
>>>> the
>>>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>>>> (~0UL) is introduced as a generic value for the error case. Adjust all
>>>> callers of the alloc_iova_fast() function for the new return value.
>>>
>>> And when does anything actually need this? In fact if you were to stop
>>> iommu-dma from reserving IOVA 0 - which you don't - it would only show
>>> how patch #3 is broken.
>>
>> I don't get why you says that patch #3 is broken.
>
> My mistake, in fact it's already just as broken either way - I forgot 
> that that's done implicitly via iovad->start_pfn rather than an actual 
> reserve_iova() entry. I see there is an initial calculation attempting 
> to honour start_pfn in patch #3, but it's always immediately 
> overwritten. More critically, the rb_first()/rb_next walk means the 
> starting point can only creep inevitably upwards as older allocations 
> are freed, so will end up pathologically stuck at or above limit_pfn 
> and unable to recover. At best it's more "next-fit" than "first-fit", 
> and TBH the fact that it could ever even allocate anything at all in 
> an empty domain makes me want to change the definition of IOVA_ANCHOR ;)
>
>> Them main issue with
>> address 0 being reserved appears when one uses first fit allocation
>> algorithm. In such case the first allocation will be at the 0 address,
>> what causes some issues. This patch simply makes the whole iova related
>> code capable of such case. This mimics the similar code already used on
>> ARM 32bit. There are drivers that rely on the way the IOVAs are
>> allocated. This is especially important for the drivers for devices with
>> limited addressing capabilities. And there exists such and they can be
>> even behind the IOMMU.
>>
>> Lets focus on the s5p-mfc driver and the way one of the supported
>> hardware does the DMA. The hardware has very limited DMA window (256M)
>> and addresses all memory buffers as an offset from the firmware base.
>> Currently it has been assumed that the first allocated buffer will be on
>> the beginning of the IOVA space. This worked fine on ARM 32bit and
>> supporting such case is a target of this patchset.
>
> I still understand the use-case; I object to a solution where PFN 0 is 
> always reserved yet sometimes allocated. Not to mention the slightly 
> more fundamental thing of the whole lot not actually working the way 
> it's supposed to.
>
> I also remain opposed to baking in secret ABIs where allocators are 
> expected to honour a particular undocumented behaviour. FWIW I've had 
> plans for a while now to make the IOVA walk bidirectional to make the 
> existing retry case more efficient, at which point it's then almost 
> trivial to implement "bottom-up" allocation, which I'm thinking I 
> might then force on by default for CONFIG_ARM to minimise any further 
> surprises for v2 of the default domain conversion. However I'm 
> increasingly less convinced that it's really sufficient for your case, 
> and am leaning back towards the opinion that any driver with really 
> specific constraints on how its DMA addresses are laid out should not 
> be passively relying on a generic allocator to do exactly what it 
> wants. A simple flag saying "try to allocate DMA addresses bottom-up" 
> doesn't actually mean "allocate this thing on a 256MB-aligned address 
> and everything subsequent within a 256MB window above it", so let's 
> not build a fragile house of cards on top of pretending that it does.


Okay, I see your point. I'm fine with adding more of the IOVA allocation 
logic to the respective driver (s5p-mfc), however I would still like to 
use the dma-mapping framework and helpers, which depend on it (like 
v4l2-videobuf2, dma-buf, and so on). Would it be fine for you to let 
device drivers to access the IOVA domains hidden by the DMA-IOMMU glue 
code to mark certain parts of IOVA space as reserved or to manually 
remap the buffer (like firmware) with specific address requirements?


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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V2 2/6] iommu: iova: properly handle 0 as a valid IOVA address

[Marek Szyprowski]

Hi Robin,

On 06.06.2022 14:38, Robin Murphy wrote:
> On 2022-06-02 16:58, Marek Szyprowski wrote:
>> On 23.05.2022 19:30, Robin Murphy wrote:
>>> On 2022-05-11 13:15, Ajay Kumar wrote:
>>>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>
>>>> Zero is a valid DMA and IOVA address on many architectures, so adjust
>>>> the
>>>> IOVA management code to properly handle it. A new value IOVA_BAD_ADDR
>>>> (~0UL) is introduced as a generic value for the error case. Adjust all
>>>> callers of the alloc_iova_fast() function for the new return value.
>>>
>>> And when does anything actually need this? In fact if you were to stop
>>> iommu-dma from reserving IOVA 0 - which you don't - it would only show
>>> how patch #3 is broken.
>>
>> I don't get why you says that patch #3 is broken.
>
> My mistake, in fact it's already just as broken either way - I forgot 
> that that's done implicitly via iovad->start_pfn rather than an actual 
> reserve_iova() entry. I see there is an initial calculation attempting 
> to honour start_pfn in patch #3, but it's always immediately 
> overwritten. More critically, the rb_first()/rb_next walk means the 
> starting point can only creep inevitably upwards as older allocations 
> are freed, so will end up pathologically stuck at or above limit_pfn 
> and unable to recover. At best it's more "next-fit" than "first-fit", 
> and TBH the fact that it could ever even allocate anything at all in 
> an empty domain makes me want to change the definition of IOVA_ANCHOR ;)
>
>> Them main issue with
>> address 0 being reserved appears when one uses first fit allocation
>> algorithm. In such case the first allocation will be at the 0 address,
>> what causes some issues. This patch simply makes the whole iova related
>> code capable of such case. This mimics the similar code already used on
>> ARM 32bit. There are drivers that rely on the way the IOVAs are
>> allocated. This is especially important for the drivers for devices with
>> limited addressing capabilities. And there exists such and they can be
>> even behind the IOMMU.
>>
>> Lets focus on the s5p-mfc driver and the way one of the supported
>> hardware does the DMA. The hardware has very limited DMA window (256M)
>> and addresses all memory buffers as an offset from the firmware base.
>> Currently it has been assumed that the first allocated buffer will be on
>> the beginning of the IOVA space. This worked fine on ARM 32bit and
>> supporting such case is a target of this patchset.
>
> I still understand the use-case; I object to a solution where PFN 0 is 
> always reserved yet sometimes allocated. Not to mention the slightly 
> more fundamental thing of the whole lot not actually working the way 
> it's supposed to.
>
> I also remain opposed to baking in secret ABIs where allocators are 
> expected to honour a particular undocumented behaviour. FWIW I've had 
> plans for a while now to make the IOVA walk bidirectional to make the 
> existing retry case more efficient, at which point it's then almost 
> trivial to implement "bottom-up" allocation, which I'm thinking I 
> might then force on by default for CONFIG_ARM to minimise any further 
> surprises for v2 of the default domain conversion. However I'm 
> increasingly less convinced that it's really sufficient for your case, 
> and am leaning back towards the opinion that any driver with really 
> specific constraints on how its DMA addresses are laid out should not 
> be passively relying on a generic allocator to do exactly what it 
> wants. A simple flag saying "try to allocate DMA addresses bottom-up" 
> doesn't actually mean "allocate this thing on a 256MB-aligned address 
> and everything subsequent within a 256MB window above it", so let's 
> not build a fragile house of cards on top of pretending that it does.


Okay, I see your point. I'm fine with adding more of the IOVA allocation 
logic to the respective driver (s5p-mfc), however I would still like to 
use the dma-mapping framework and helpers, which depend on it (like 
v4l2-videobuf2, dma-buf, and so on). Would it be fine for you to let 
device drivers to access the IOVA domains hidden by the DMA-IOMMU glue 
code to mark certain parts of IOVA space as reserved or to manually 
remap the buffer (like firmware) with specific address requirements?


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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel