Re: [PATCH 16/19] dma-iommu: don't depend on CONFIG_DMA_DIRECT_REMAP

[Robin Murphy]

On 14/01/2019 09:41, Christoph Hellwig wrote:
> For entirely dma coherent architectures there is no good reason to ever
> remap dma coherent allocation.

Yes there is, namely assembling large buffers without the need for 
massive CMA areas and compaction overhead under memory fragmentation. 
That has always been a distinct concern from the DMA_DIRECT_REMAP cases; 
they've just been able to share a fair few code paths.

>  Move all the remap and pool code under
> CONFIG_DMA_DIRECT_REMAP ifdefs, and drop the Kconfig dependency.

As far as I'm concerned that splits things the wrong way. Logically, 
iommu_dma_alloc() should always have done its own vmap() instead of just 
returning the bare pages array, but that was tricky to resolve with the 
design of having the caller handle everything to do with coherency 
(forcing the caller to unpick that mapping just to remap it yet again in 
the noncoherent case didn't seem sensible).

Robin.

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>   drivers/iommu/Kconfig     |  1 -
>   drivers/iommu/dma-iommu.c | 10 ++++++++++
>   2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 8b13fb7d0263..d9a25715650e 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -94,7 +94,6 @@ config IOMMU_DMA
>   	select IOMMU_API
>   	select IOMMU_IOVA
>   	select NEED_SG_DMA_LENGTH
> -	depends on DMA_DIRECT_REMAP
>   
>   config FSL_PAMU
>   	bool "Freescale IOMMU support"
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index fd25c995bde4..e27909771d55 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -502,6 +502,7 @@ static void *iommu_dma_alloc_contiguous(struct device *dev, size_t size,
>   	return page_address(page);
>   }
>   
> +#ifdef CONFIG_DMA_DIRECT_REMAP
>   static void __iommu_dma_free_pages(struct page **pages, int count)
>   {
>   	while (count--)
> @@ -775,6 +776,7 @@ static void *iommu_dma_alloc_noncoherent(struct device *dev, size_t size,
>   				gfp, attrs);
>   	return iommu_dma_alloc_remap(dev, size, dma_handle, gfp, attrs);
>   }
> +#endif /* CONFIG_DMA_DIRECT_REMAP */
>   
>   static void iommu_dma_sync_single_for_cpu(struct device *dev,
>   		dma_addr_t dma_handle, size_t size, enum dma_data_direction dir)
> @@ -1057,6 +1059,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
>   	 */
>   	gfp |= __GFP_ZERO;
>   
> +#ifdef CONFIG_DMA_DIRECT_REMAP
>   	if (!dev_is_dma_coherent(dev))
>   		return iommu_dma_alloc_noncoherent(dev, size, dma_handle, gfp,
>   				attrs);
> @@ -1064,6 +1067,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t size,
>   	if (gfpflags_allow_blocking(gfp) &&
>   	    !(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
>   		return iommu_dma_alloc_remap(dev, size, dma_handle, gfp, attrs);
> +#endif
>   
>   	return iommu_dma_alloc_contiguous(dev, size, dma_handle, gfp, attrs);
>   }
> @@ -1083,6 +1087,7 @@ static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr,
>   	 *
>   	 * Hence how dodgy the below logic looks...
>   	 */
> +#ifdef CONFIG_DMA_DIRECT_REMAP
>   	if (dma_in_atomic_pool(cpu_addr, PAGE_ALIGN(size))) {
>   		iommu_dma_free_pool(dev, size, cpu_addr, dma_handle);
>   		return;
> @@ -1096,6 +1101,7 @@ static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr,
>   		page = vmalloc_to_page(cpu_addr);
>   		dma_common_free_remap(cpu_addr, PAGE_ALIGN(size), VM_USERMAP);
>   	} else
> +#endif
>   		page = virt_to_page(cpu_addr);
>   
>   	iommu_dma_free_contiguous(dev, size, page, dma_handle);
> @@ -1119,11 +1125,13 @@ static int iommu_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>   	if (off >= count || user_count > count - off)
>   		return -ENXIO;
>   
> +#ifdef CONFIG_DMA_DIRECT_REMAP
>   	if (is_vmalloc_addr(cpu_addr)) {
>   		if (!(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
>   			return iommu_dma_mmap_remap(cpu_addr, size, vma);
>   		pfn = vmalloc_to_pfn(cpu_addr);
>   	} else
> +#endif
>   		pfn = page_to_pfn(virt_to_page(cpu_addr));
>   
>   	return remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,
> @@ -1137,11 +1145,13 @@ static int iommu_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
>   	struct page *page;
>   	int ret;
>   
> +#ifdef CONFIG_DMA_DIRECT_REMAP
>   	if (is_vmalloc_addr(cpu_addr)) {
>   		if (!(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
>   			return iommu_dma_get_sgtable_remap(sgt, cpu_addr, size);
>   		page = vmalloc_to_page(cpu_addr);
>   	} else
> +#endif
>   		page = virt_to_page(cpu_addr);
>   
>   	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);