| Message ID | 20190114094159.27326-17-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/19] dma-mapping: add a Kconfig symbol to indicated arch_dma_prep_coherent presence | expand |
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@lst.de> > --- > 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); >
On Wed, Feb 06, 2019 at 11:55:49AM +0000, Robin Murphy wrote: > 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. Well, I guess I need to reword this - there is no _requirement_ to remap. And x86 has been happy to not remap so far and I see absolutely no reason to force anyone to remap. >> 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). I don't parse this. In the old code base before this series iommu_dma_alloc is a relatively low-level helper allocating and mapping pages. And that one should have done the remapping, and in fact does so since ("dma-iommu: refactor page array remap helpers"). It just happens that the function is now called iommu_dma_alloc_remap. The new iommu_dma_alloc is the high level entry point that handles every possible case of different allocations, including those where we do not have a virtual mapping.
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);
For entirely dma coherent architectures there is no good reason to ever remap dma coherent allocation. Move all the remap and pool code under CONFIG_DMA_DIRECT_REMAP ifdefs, and drop the Kconfig dependency. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/iommu/Kconfig | 1 - drivers/iommu/dma-iommu.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)