[PATCH v2 0/2] iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS

[PATCH v2 0/2] iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS

[Geert Uytterhoeven]

	Hi all,

Tis patch series adds helpers for DMA_ATTR_FORCE_CONTIGUOUS to the
generic IOMMU DMA code, and support for allocating physically contiguous
DMA buffers on arm64 systems with an IOMMU.  This can be useful when two
or more devices with different memory requirements are involved in
buffer sharing.

Changes compared to v1:
  - Provide standalone iommu_dma_{alloc,free}_contiguous() functions, as
    requested by Robin Murphy,
  - Handle dispatching in the arch (arm64) code, as requested by Robin
    Murphy,
  - Simplify operations by getting rid of the page array/scatterlist
    dance, as the buffer is contiguous,
  - Move CPU cache magement into the caller, which is much simpler with
    a single contiguous buffer.

Thanks for your comments!

Geert Uytterhoeven (2):
  iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS
  arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU

 arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++---------
 drivers/iommu/dma-iommu.c   | 72 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h   |  4 +++
 3 files changed, 113 insertions(+), 14 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/2] iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS

[Geert Uytterhoeven]

Add helpers for allocating physically contiguous DMA buffers to the
generic IOMMU DMA code.  This can be useful when two or more devices
with different memory requirements are involved in buffer sharing.

The iommu_dma_{alloc,free}_contiguous() functions complement the existing
iommu_dma_{alloc,free}() functions, and allow architecture-specific code
to implement support for the DMA_ATTR_FORCE_CONTIGUOUS attribute on
systems with an IOMMU.  As this uses the CMA allocator, setting this
attribute has a runtime-dependency on CONFIG_DMA_CMA.

Note that unlike the existing iommu_dma_alloc() helper,
iommu_dma_alloc_contiguous() has no callback to flush pages.
Ensuring the returned region is made visible to a non-coherent device is
the responsibility of the caller.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Provide standalone iommu_dma_{alloc,free}_contiguous() functions, as
    requested by Robin Murphy,
  - Simplify operations by getting rid of the page array/scatterlist
    dance, as the buffer is contiguous,
  - Move CPU cache magement into the caller, which is much simpler with
    a single contiguous buffer.
---
 drivers/iommu/dma-iommu.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |  4 +++
 2 files changed, 76 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d641cf4505b5..8f8ed4426f9a3a12 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -30,6 +30,7 @@
 #include <linux/pci.h>
 #include <linux/scatterlist.h>
 #include <linux/vmalloc.h>
+#include <linux/dma-contiguous.h>
 
 struct iommu_dma_msi_page {
 	struct list_head	list;
@@ -408,6 +409,77 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
 }
 
 /**
+ * iommu_dma_free_contiguous - Free a buffer allocated by
+ *			       iommu_dma_alloc_contiguous()
+ * @dev: Device which owns this buffer
+ * @page: Buffer page pointer as returned by iommu_dma_alloc_contiguous()
+ * @size: Size of buffer in bytes
+ * @handle: DMA address of buffer
+ *
+ * Frees the pages associated with the buffer.
+ */
+void iommu_dma_free_contiguous(struct device *dev, struct page *page,
+		size_t size, dma_addr_t *handle)
+{
+	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), *handle);
+	dma_release_from_contiguous(dev, page, PAGE_ALIGN(size) >> PAGE_SHIFT);
+	*handle = DMA_ERROR_CODE;
+}
+
+/**
+ * iommu_dma_alloc_contiguous - Allocate and map a buffer contiguous in IOVA
+ *				and physical space
+ * @dev: Device to allocate memory for. Must be a real device attached to an
+ *	 iommu_dma_domain
+ * @size: Size of buffer in bytes
+ * @prot: IOMMU mapping flags
+ * @handle: Out argument for allocated DMA handle
+ *
+ * Return: Buffer page pointer, or NULL on failure.
+ *
+ * Note that unlike iommu_dma_alloc(), it's the caller's responsibility to
+ * ensure the returned region is made visible to the given non-coherent device.
+ */
+struct page *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
+		int prot, dma_addr_t *handle)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iova_domain *iovad = cookie_iovad(domain);
+	dma_addr_t dma_addr;
+	unsigned int count;
+	struct page *page;
+	struct iova *iova;
+	int ret;
+
+	*handle = DMA_ERROR_CODE;
+
+	size = PAGE_ALIGN(size);
+	count = size >> PAGE_SHIFT;
+	page = dma_alloc_from_contiguous(dev, count, get_order(size));
+	if (!page)
+		return NULL;
+
+	iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
+	if (!iova)
+		goto out_free_pages;
+
+	size = iova_align(iovad, size);
+	dma_addr = iova_dma_addr(iovad, iova);
+	ret = iommu_map(domain, dma_addr, page_to_phys(page), size, prot);
+	if (ret < 0)
+		goto out_free_iova;
+
+	*handle = dma_addr;
+	return page;
+
+out_free_iova:
+	__free_iova(iovad, iova);
+out_free_pages:
+	dma_release_from_contiguous(dev, page, count);
+	return NULL;
+}
+
+/**
  * iommu_dma_mmap - Map a buffer into provided user VMA
  * @pages: Array representing buffer from iommu_dma_alloc()
  * @size: Size of buffer in bytes
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 7f7e9a7e3839966c..7eee62c2b0e752f7 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -45,6 +45,10 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
 		void (*flush_page)(struct device *, const void *, phys_addr_t));
 void iommu_dma_free(struct device *dev, struct page **pages, size_t size,
 		dma_addr_t *handle);
+struct page *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
+		int prot, dma_addr_t *handle);
+void iommu_dma_free_contiguous(struct device *dev, struct page *page,
+		size_t size, dma_addr_t *handle);
 
 int iommu_dma_mmap(struct page **pages, size_t size, struct vm_area_struct *vma);
 
-- 
1.9.1

[PATCH v2 2/2] arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU

[Geert Uytterhoeven]

Add support for allocation physically contiguous DMA buffers on arm64
systems with an IOMMU, by dispatching DMA buffer allocations with the
DMA_ATTR_FORCE_CONTIGUOUS attribute to the appropriate IOMMU DMA
helpers.

Note that as this uses the CMA allocator, setting this attribute has a
runtime-dependency on CONFIG_DMA_CMA, just like on arm32.

For arm64 systems using swiotlb, no changes are needed to support the
allocation of physically contiguous DMA buffers:
  - swiotlb always uses physically contiguous buffers (up to
    IO_TLB_SEGSIZE = 128 pages),
  - arm64's __dma_alloc_coherent() already calls
    dma_alloc_from_contiguous() when CMA is available.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New, handle dispatching in the arch (arm64) code, as requested by
    Robin Murphy.
---
 arch/arm64/mm/dma-mapping.c | 51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 1d7d5d2881db7c19..325803e0ba79ef26 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -577,20 +577,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 	 */
 	gfp |= __GFP_ZERO;
 
-	if (gfpflags_allow_blocking(gfp)) {
-		struct page **pages;
-		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
-
-		pages = iommu_dma_alloc(dev, iosize, gfp, attrs, ioprot,
-					handle, flush_page);
-		if (!pages)
-			return NULL;
-
-		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
-					      __builtin_return_address(0));
-		if (!addr)
-			iommu_dma_free(dev, pages, iosize, handle);
-	} else {
+	if (!gfpflags_allow_blocking(gfp)) {
 		struct page *page;
 		/*
 		 * In atomic context we can't remap anything, so we'll only
@@ -614,6 +601,35 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 				__free_from_pool(addr, size);
 			addr = NULL;
 		}
+	} else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
+		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+		struct page *page;
+
+		page = iommu_dma_alloc_contiguous(dev, iosize, ioprot, handle);
+		if (!page)
+			return NULL;
+
+		if (!coherent)
+			__dma_flush_area(page_to_virt(page), iosize);
+
+		addr = dma_common_contiguous_remap(page, size, VM_USERMAP,
+						   prot,
+						   __builtin_return_address(0));
+		if (!addr)
+			iommu_dma_free_contiguous(dev, page, iosize, handle);
+	} else {
+		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+		struct page **pages;
+
+		pages = iommu_dma_alloc(dev, iosize, gfp, attrs, ioprot,
+					handle, flush_page);
+		if (!pages)
+			return NULL;
+
+		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
+					      __builtin_return_address(0));
+		if (!addr)
+			iommu_dma_free(dev, pages, iosize, handle);
 	}
 	return addr;
 }
@@ -626,6 +642,8 @@ static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 	size = PAGE_ALIGN(size);
 	/*
 	 * @cpu_addr will be one of 3 things depending on how it was allocated:
+	 * - A remapped array of pages from iommu_dma_alloc_contiguous()
+	 *   for contiguous allocations.
 	 * - A remapped array of pages from iommu_dma_alloc(), for all
 	 *   non-atomic allocations.
 	 * - A non-cacheable alias from the atomic pool, for atomic
@@ -637,6 +655,11 @@ static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 	if (__in_atomic_pool(cpu_addr, size)) {
 		iommu_dma_unmap_page(dev, handle, iosize, 0, 0);
 		__free_from_pool(cpu_addr, size);
+	} else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
+		struct page *page = phys_to_page(dma_to_phys(dev, handle));
+
+		iommu_dma_free_contiguous(dev, page, iosize, &handle);
+		dma_common_free_remap(cpu_addr, size, VM_USERMAP);
 	} else if (is_vmalloc_addr(cpu_addr)){
 		struct vm_struct *area = find_vm_area(cpu_addr);
 
-- 
1.9.1

Re: [PATCH v2 1/2] iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS

[Robin Murphy]

Hi Geert,

On 27/01/17 15:34, Geert Uytterhoeven wrote:
> Add helpers for allocating physically contiguous DMA buffers to the
> generic IOMMU DMA code.  This can be useful when two or more devices
> with different memory requirements are involved in buffer sharing.
> 
> The iommu_dma_{alloc,free}_contiguous() functions complement the existing
> iommu_dma_{alloc,free}() functions, and allow architecture-specific code
> to implement support for the DMA_ATTR_FORCE_CONTIGUOUS attribute on
> systems with an IOMMU.  As this uses the CMA allocator, setting this
> attribute has a runtime-dependency on CONFIG_DMA_CMA.
> 
> Note that unlike the existing iommu_dma_alloc() helper,
> iommu_dma_alloc_contiguous() has no callback to flush pages.
> Ensuring the returned region is made visible to a non-coherent device is
> the responsibility of the caller.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Provide standalone iommu_dma_{alloc,free}_contiguous() functions, as
>     requested by Robin Murphy,
>   - Simplify operations by getting rid of the page array/scatterlist
>     dance, as the buffer is contiguous,
>   - Move CPU cache magement into the caller, which is much simpler with
>     a single contiguous buffer.

Thanks for the rework, that's a lot easier to make sense of! Now, please
don't hate me, but...

> ---
>  drivers/iommu/dma-iommu.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-iommu.h |  4 +++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 2db0d641cf4505b5..8f8ed4426f9a3a12 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -30,6 +30,7 @@
>  #include <linux/pci.h>
>  #include <linux/scatterlist.h>
>  #include <linux/vmalloc.h>
> +#include <linux/dma-contiguous.h>
>  
>  struct iommu_dma_msi_page {
>  	struct list_head	list;
> @@ -408,6 +409,77 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
>  }
>  
>  /**
> + * iommu_dma_free_contiguous - Free a buffer allocated by
> + *			       iommu_dma_alloc_contiguous()
> + * @dev: Device which owns this buffer
> + * @page: Buffer page pointer as returned by iommu_dma_alloc_contiguous()
> + * @size: Size of buffer in bytes
> + * @handle: DMA address of buffer
> + *
> + * Frees the pages associated with the buffer.
> + */
> +void iommu_dma_free_contiguous(struct device *dev, struct page *page,
> +		size_t size, dma_addr_t *handle)
> +{
> +	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), *handle);
> +	dma_release_from_contiguous(dev, page, PAGE_ALIGN(size) >> PAGE_SHIFT);
> +	*handle = DMA_ERROR_CODE;
> +}
> +
> +/**
> + * iommu_dma_alloc_contiguous - Allocate and map a buffer contiguous in IOVA
> + *				and physical space
> + * @dev: Device to allocate memory for. Must be a real device attached to an
> + *	 iommu_dma_domain
> + * @size: Size of buffer in bytes
> + * @prot: IOMMU mapping flags
> + * @handle: Out argument for allocated DMA handle
> + *
> + * Return: Buffer page pointer, or NULL on failure.
> + *
> + * Note that unlike iommu_dma_alloc(), it's the caller's responsibility to
> + * ensure the returned region is made visible to the given non-coherent device.
> + */
> +struct page *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
> +		int prot, dma_addr_t *handle)
> +{
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +	struct iova_domain *iovad = cookie_iovad(domain);
> +	dma_addr_t dma_addr;
> +	unsigned int count;
> +	struct page *page;
> +	struct iova *iova;
> +	int ret;
> +
> +	*handle = DMA_ERROR_CODE;
> +
> +	size = PAGE_ALIGN(size);
> +	count = size >> PAGE_SHIFT;
> +	page = dma_alloc_from_contiguous(dev, count, get_order(size));
> +	if (!page)
> +		return NULL;
> +
> +	iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
> +	if (!iova)
> +		goto out_free_pages;
> +
> +	size = iova_align(iovad, size);
> +	dma_addr = iova_dma_addr(iovad, iova);
> +	ret = iommu_map(domain, dma_addr, page_to_phys(page), size, prot);
> +	if (ret < 0)
> +		goto out_free_iova;
> +
> +	*handle = dma_addr;
> +	return page;
> +
> +out_free_iova:
> +	__free_iova(iovad, iova);
> +out_free_pages:
> +	dma_release_from_contiguous(dev, page, count);
> +	return NULL;
> +}

...now that I can see it clearly, isn't this more or less just:

	page = dma_alloc_from_contiguous(dev, ...);
	if (page)
		dma_addr = iommu_dma_map_page(dev, page, ...);
?

Would it not be even simpler to just make those two calls directly from
the arm64 code?

Robin.

> +
> +/**
>   * iommu_dma_mmap - Map a buffer into provided user VMA
>   * @pages: Array representing buffer from iommu_dma_alloc()
>   * @size: Size of buffer in bytes
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 7f7e9a7e3839966c..7eee62c2b0e752f7 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -45,6 +45,10 @@ struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t gfp,
>  		void (*flush_page)(struct device *, const void *, phys_addr_t));
>  void iommu_dma_free(struct device *dev, struct page **pages, size_t size,
>  		dma_addr_t *handle);
> +struct page *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
> +		int prot, dma_addr_t *handle);
> +void iommu_dma_free_contiguous(struct device *dev, struct page *page,
> +		size_t size, dma_addr_t *handle);
>  
>  int iommu_dma_mmap(struct page **pages, size_t size, struct vm_area_struct *vma);
>  
> 

Re: [PATCH v2 2/2] arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 27 Jan 2017 16:34:19 Geert Uytterhoeven wrote:
> Add support for allocation physically contiguous DMA buffers on arm64
> systems with an IOMMU, by dispatching DMA buffer allocations with the
> DMA_ATTR_FORCE_CONTIGUOUS attribute to the appropriate IOMMU DMA
> helpers.
> 
> Note that as this uses the CMA allocator, setting this attribute has a
> runtime-dependency on CONFIG_DMA_CMA, just like on arm32.
> 
> For arm64 systems using swiotlb, no changes are needed to support the
> allocation of physically contiguous DMA buffers:
>   - swiotlb always uses physically contiguous buffers (up to
>     IO_TLB_SEGSIZE = 128 pages),
>   - arm64's __dma_alloc_coherent() already calls
>     dma_alloc_from_contiguous() when CMA is available.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - New, handle dispatching in the arch (arm64) code, as requested by
>     Robin Murphy.
> ---
>  arch/arm64/mm/dma-mapping.c | 51 +++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 1d7d5d2881db7c19..325803e0ba79ef26 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -577,20 +577,7 @@ static void *__iommu_alloc_attrs(struct device *dev,
> size_t size, */
>  	gfp |= __GFP_ZERO;
> 
> -	if (gfpflags_allow_blocking(gfp)) {
> -		struct page **pages;
> -		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, 
coherent);
> -
> -		pages = iommu_dma_alloc(dev, iosize, gfp, attrs, ioprot,
> -					handle, flush_page);
> -		if (!pages)
> -			return NULL;
> -
> -		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
> -					      __builtin_return_address(0));
> -		if (!addr)
> -			iommu_dma_free(dev, pages, iosize, handle);
> -	} else {
> +	if (!gfpflags_allow_blocking(gfp)) {
>  		struct page *page;
>  		/*
>  		 * In atomic context we can't remap anything, so we'll only
> @@ -614,6 +601,35 @@ static void *__iommu_alloc_attrs(struct device *dev,
> size_t size, __free_from_pool(addr, size);
>  			addr = NULL;
>  		}
> +	} else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
> +		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, 
coherent);
> +		struct page *page;
> +
> +		page = iommu_dma_alloc_contiguous(dev, iosize, ioprot, 
handle);
> +		if (!page)
> +			return NULL;
> +
> +		if (!coherent)
> +			__dma_flush_area(page_to_virt(page), iosize);
> +
> +		addr = dma_common_contiguous_remap(page, size, VM_USERMAP,
> +						   prot,
> +						   
__builtin_return_address(0));
> +		if (!addr)
> +			iommu_dma_free_contiguous(dev, page, iosize, handle);
> +	} else {
> +		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, 
coherent);
> +		struct page **pages;
> +
> +		pages = iommu_dma_alloc(dev, iosize, gfp, attrs, ioprot,
> +					handle, flush_page);
> +		if (!pages)
> +			return NULL;
> +
> +		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
> +					      __builtin_return_address(0));
> +		if (!addr)
> +			iommu_dma_free(dev, pages, iosize, handle);
>  	}
>  	return addr;
>  }
> @@ -626,6 +642,8 @@ static void __iommu_free_attrs(struct device *dev,
> size_t size, void *cpu_addr, size = PAGE_ALIGN(size);
>  	/*
>  	 * @cpu_addr will be one of 3 things depending on how it was 
allocated:

s/one of 3/one of 4/

Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * - A remapped array of pages from iommu_dma_alloc_contiguous()
> +	 *   for contiguous allocations.
>  	 * - A remapped array of pages from iommu_dma_alloc(), for all
>  	 *   non-atomic allocations.
>  	 * - A non-cacheable alias from the atomic pool, for atomic
> @@ -637,6 +655,11 @@ static void __iommu_free_attrs(struct device *dev,
> size_t size, void *cpu_addr, if (__in_atomic_pool(cpu_addr, size)) {
>  		iommu_dma_unmap_page(dev, handle, iosize, 0, 0);
>  		__free_from_pool(cpu_addr, size);
> +	} else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
> +		struct page *page = phys_to_page(dma_to_phys(dev, handle));
> +
> +		iommu_dma_free_contiguous(dev, page, iosize, &handle);
> +		dma_common_free_remap(cpu_addr, size, VM_USERMAP);
>  	} else if (is_vmalloc_addr(cpu_addr)){
>  		struct vm_struct *area = find_vm_area(cpu_addr);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] iommu/dma: Add support for DMA_ATTR_FORCE_CONTIGUOUS

[Geert Uytterhoeven]

Hi Robin,

On Fri, Jan 27, 2017 at 6:50 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 27/01/17 15:34, Geert Uytterhoeven wrote:
>> Add helpers for allocating physically contiguous DMA buffers to the
>> generic IOMMU DMA code.  This can be useful when two or more devices
>> with different memory requirements are involved in buffer sharing.
>>
>> The iommu_dma_{alloc,free}_contiguous() functions complement the existing
>> iommu_dma_{alloc,free}() functions, and allow architecture-specific code
>> to implement support for the DMA_ATTR_FORCE_CONTIGUOUS attribute on
>> systems with an IOMMU.  As this uses the CMA allocator, setting this
>> attribute has a runtime-dependency on CONFIG_DMA_CMA.
>>
>> Note that unlike the existing iommu_dma_alloc() helper,
>> iommu_dma_alloc_contiguous() has no callback to flush pages.
>> Ensuring the returned region is made visible to a non-coherent device is
>> the responsibility of the caller.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v2:
>>   - Provide standalone iommu_dma_{alloc,free}_contiguous() functions, as
>>     requested by Robin Murphy,
>>   - Simplify operations by getting rid of the page array/scatterlist
>>     dance, as the buffer is contiguous,
>>   - Move CPU cache magement into the caller, which is much simpler with
>>     a single contiguous buffer.
>
> Thanks for the rework, that's a lot easier to make sense of! Now, please
> don't hate me, but...

I don't hate you at all. I'm just a drivers/iommu rookie ;-)

>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c

>> +/**
>> + * iommu_dma_alloc_contiguous - Allocate and map a buffer contiguous in IOVA
>> + *                           and physical space
>> + * @dev: Device to allocate memory for. Must be a real device attached to an
>> + *    iommu_dma_domain
>> + * @size: Size of buffer in bytes
>> + * @prot: IOMMU mapping flags
>> + * @handle: Out argument for allocated DMA handle
>> + *
>> + * Return: Buffer page pointer, or NULL on failure.
>> + *
>> + * Note that unlike iommu_dma_alloc(), it's the caller's responsibility to
>> + * ensure the returned region is made visible to the given non-coherent device.
>> + */
>> +struct page *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
>> +             int prot, dma_addr_t *handle)
>> +{
>> +     struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> +     struct iova_domain *iovad = cookie_iovad(domain);
>> +     dma_addr_t dma_addr;
>> +     unsigned int count;
>> +     struct page *page;
>> +     struct iova *iova;
>> +     int ret;
>> +
>> +     *handle = DMA_ERROR_CODE;
>> +
>> +     size = PAGE_ALIGN(size);
>> +     count = size >> PAGE_SHIFT;
>> +     page = dma_alloc_from_contiguous(dev, count, get_order(size));
>> +     if (!page)
>> +             return NULL;
>> +
>> +     iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
>> +     if (!iova)
>> +             goto out_free_pages;
>> +
>> +     size = iova_align(iovad, size);
>> +     dma_addr = iova_dma_addr(iovad, iova);
>> +     ret = iommu_map(domain, dma_addr, page_to_phys(page), size, prot);
>> +     if (ret < 0)
>> +             goto out_free_iova;
>> +
>> +     *handle = dma_addr;
>> +     return page;
>> +
>> +out_free_iova:
>> +     __free_iova(iovad, iova);
>> +out_free_pages:
>> +     dma_release_from_contiguous(dev, page, count);
>> +     return NULL;
>> +}
>
> ...now that I can see it clearly, isn't this more or less just:
>
>         page = dma_alloc_from_contiguous(dev, ...);
>         if (page)
>                 dma_addr = iommu_dma_map_page(dev, page, ...);
> ?
>
> Would it not be even simpler to just make those two calls directly from
> the arm64 code?

You're right. Will update. Thanks a lot!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds