* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS @ 2017-04-25 18:00 ` Catalin Marinas 2017-05-04 11:29 ` Robin Murphy 2017-05-05 10:01 ` Andrzej Hajda 0 siblings, 2 replies; 6+ messages in thread From: Catalin Marinas @ 2017-04-25 18:00 UTC (permalink / raw) To: linux-arm-kernel While honouring the DMA_ATTR_FORCE_CONTIGUOUS on arm64 (commit 44176bb38fa4: "arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU"), the existing uses of dma_mmap_attrs() and dma_get_sgtable() have been broken by passing a physically contiguous vm_struct with an invalid pages pointer through the common iommu API. Since the coherent allocation with DMA_ATTR_FORCE_CONTIGUOUS uses CMA, this patch simply reuses the existing swiotlb logic for mmap and get_sgtable. Note that the current implementation of get_sgtable (both swiotlb and iommu) is broken if dma_declare_coherent_memory() is used since such memory does not have a corresponding struct page. To be addressed in a separate patch. Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU") Reported-by: Andrzej Hajda <a.hajda@samsung.com> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- Here's my attempt of fixing this, though I'd like to wait for Robin's review (who's currently on holiday until next week). Not tested. arch/arm64/mm/dma-mapping.c | 65 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index f7b54019ef55..c9e53dec3695 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -308,24 +308,15 @@ static void __swiotlb_sync_sg_for_device(struct device *dev, sg->length, dir); } -static int __swiotlb_mmap(struct device *dev, - struct vm_area_struct *vma, - void *cpu_addr, dma_addr_t dma_addr, size_t size, - unsigned long attrs) +static int __swiotlb_mmap_pfn(struct vm_area_struct *vma, + unsigned long pfn, size_t size) { int ret = -ENXIO; unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; - unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT; unsigned long off = vma->vm_pgoff; - vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot, - is_device_dma_coherent(dev)); - - if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) - return ret; - if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { ret = remap_pfn_range(vma, vma->vm_start, pfn + off, @@ -336,19 +327,43 @@ static int __swiotlb_mmap(struct device *dev, return ret; } -static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, - void *cpu_addr, dma_addr_t handle, size_t size, - unsigned long attrs) +static int __swiotlb_mmap(struct device *dev, + struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + unsigned long attrs) +{ + int ret; + unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT; + + vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot, + is_device_dma_coherent(dev)); + + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) + return ret; + + return __swiotlb_mmap_pfn(vma, pfn, size); +} + +static int __swiotlb_get_sgtable_page(struct sg_table *sgt, + struct page *page, size_t size) { int ret = sg_alloc_table(sgt, 1, GFP_KERNEL); if (!ret) - sg_set_page(sgt->sgl, phys_to_page(dma_to_phys(dev, handle)), - PAGE_ALIGN(size), 0); + sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0); return ret; } +static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, + void *cpu_addr, dma_addr_t handle, size_t size, + unsigned long attrs) +{ + struct page *page = phys_to_page(dma_to_phys(dev, handle)); + + return __swiotlb_get_sgtable_page(sgt, page, size); +} + static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) { if (swiotlb) @@ -703,6 +718,15 @@ static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma, if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) return ret; + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { + /* + * DMA_ATTR_FORCE_CONTIGUOUS allocations are always remapped, + * hence in the vmalloc space. + */ + unsigned long pfn = vmalloc_to_pfn(cpu_addr); + return __swiotlb_mmap_pfn(vma, pfn, size); + } + area = find_vm_area(cpu_addr); if (WARN_ON(!area || !area->pages)) return -ENXIO; @@ -717,6 +741,15 @@ static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt, unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT; struct vm_struct *area = find_vm_area(cpu_addr); + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { + /* + * DMA_ATTR_FORCE_CONTIGUOUS allocations are always remapped, + * hence in the vmalloc space. + */ + struct page *page = vmalloc_to_page(cpu_addr); + return __swiotlb_get_sgtable_page(sgt, page, size); + } + if (WARN_ON(!area || !area->pages)) return -ENXIO; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS 2017-04-25 18:00 ` [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS Catalin Marinas @ 2017-05-04 11:29 ` Robin Murphy 2017-05-04 14:03 ` Catalin Marinas 2017-05-05 10:01 ` Andrzej Hajda 1 sibling, 1 reply; 6+ messages in thread From: Robin Murphy @ 2017-05-04 11:29 UTC (permalink / raw) To: linux-arm-kernel On 25/04/17 19:00, Catalin Marinas wrote: > While honouring the DMA_ATTR_FORCE_CONTIGUOUS on arm64 (commit > 44176bb38fa4: "arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to > IOMMU"), the existing uses of dma_mmap_attrs() and dma_get_sgtable() > have been broken by passing a physically contiguous vm_struct with an > invalid pages pointer through the common iommu API. > > Since the coherent allocation with DMA_ATTR_FORCE_CONTIGUOUS uses CMA, > this patch simply reuses the existing swiotlb logic for mmap and > get_sgtable. > > Note that the current implementation of get_sgtable (both swiotlb and > iommu) is broken if dma_declare_coherent_memory() is used since such > memory does not have a corresponding struct page. To be addressed in a > separate patch. > > Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU") > Reported-by: Andrzej Hajda <a.hajda@samsung.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > > Here's my attempt of fixing this, though I'd like to wait for Robin's review > (who's currently on holiday until next week). > > Not tested. The general approach is sound, and I can't see anything that looks obviously wrong with the implementation, although I have no easy way of testing it either: Acked-by: Robin Murphy <robin.murphy@arm.com> As for all the other failings of get_sgtable, it seems those have been wrong from its inception, so can't be particularly critical as they're presumably not being hit in practice - if this fixes all the actual regressions I'd be inclined to leave it at that until the dma_buf folks can cook up a suitable replacement for the whole mess. Robin. > arch/arm64/mm/dma-mapping.c | 65 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 49 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > index f7b54019ef55..c9e53dec3695 100644 > --- a/arch/arm64/mm/dma-mapping.c > +++ b/arch/arm64/mm/dma-mapping.c > @@ -308,24 +308,15 @@ static void __swiotlb_sync_sg_for_device(struct device *dev, > sg->length, dir); > } > > -static int __swiotlb_mmap(struct device *dev, > - struct vm_area_struct *vma, > - void *cpu_addr, dma_addr_t dma_addr, size_t size, > - unsigned long attrs) > +static int __swiotlb_mmap_pfn(struct vm_area_struct *vma, > + unsigned long pfn, size_t size) > { > int ret = -ENXIO; > unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> > PAGE_SHIFT; > unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT; > unsigned long off = vma->vm_pgoff; > > - vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot, > - is_device_dma_coherent(dev)); > - > - if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > - return ret; > - > if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) { > ret = remap_pfn_range(vma, vma->vm_start, > pfn + off, > @@ -336,19 +327,43 @@ static int __swiotlb_mmap(struct device *dev, > return ret; > } > > -static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, > - void *cpu_addr, dma_addr_t handle, size_t size, > - unsigned long attrs) > +static int __swiotlb_mmap(struct device *dev, > + struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + unsigned long attrs) > +{ > + int ret; > + unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT; > + > + vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot, > + is_device_dma_coherent(dev)); > + > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > + return ret; > + > + return __swiotlb_mmap_pfn(vma, pfn, size); > +} > + > +static int __swiotlb_get_sgtable_page(struct sg_table *sgt, > + struct page *page, size_t size) > { > int ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > > if (!ret) > - sg_set_page(sgt->sgl, phys_to_page(dma_to_phys(dev, handle)), > - PAGE_ALIGN(size), 0); > + sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0); > > return ret; > } > > +static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, > + void *cpu_addr, dma_addr_t handle, size_t size, > + unsigned long attrs) > +{ > + struct page *page = phys_to_page(dma_to_phys(dev, handle)); > + > + return __swiotlb_get_sgtable_page(sgt, page, size); > +} > + > static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) > { > if (swiotlb) > @@ -703,6 +718,15 @@ static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma, > if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > return ret; > > + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { > + /* > + * DMA_ATTR_FORCE_CONTIGUOUS allocations are always remapped, > + * hence in the vmalloc space. > + */ > + unsigned long pfn = vmalloc_to_pfn(cpu_addr); > + return __swiotlb_mmap_pfn(vma, pfn, size); > + } > + > area = find_vm_area(cpu_addr); > if (WARN_ON(!area || !area->pages)) > return -ENXIO; > @@ -717,6 +741,15 @@ static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt, > unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT; > struct vm_struct *area = find_vm_area(cpu_addr); > > + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { > + /* > + * DMA_ATTR_FORCE_CONTIGUOUS allocations are always remapped, > + * hence in the vmalloc space. > + */ > + struct page *page = vmalloc_to_page(cpu_addr); > + return __swiotlb_get_sgtable_page(sgt, page, size); > + } > + > if (WARN_ON(!area || !area->pages)) > return -ENXIO; > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS 2017-05-04 11:29 ` Robin Murphy @ 2017-05-04 14:03 ` Catalin Marinas 2017-05-04 14:08 ` Geert Uytterhoeven 0 siblings, 1 reply; 6+ messages in thread From: Catalin Marinas @ 2017-05-04 14:03 UTC (permalink / raw) To: linux-arm-kernel On Thu, May 04, 2017 at 12:29:37PM +0100, Robin Murphy wrote: > On 25/04/17 19:00, Catalin Marinas wrote: > > While honouring the DMA_ATTR_FORCE_CONTIGUOUS on arm64 (commit > > 44176bb38fa4: "arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to > > IOMMU"), the existing uses of dma_mmap_attrs() and dma_get_sgtable() > > have been broken by passing a physically contiguous vm_struct with an > > invalid pages pointer through the common iommu API. > > > > Since the coherent allocation with DMA_ATTR_FORCE_CONTIGUOUS uses CMA, > > this patch simply reuses the existing swiotlb logic for mmap and > > get_sgtable. > > > > Note that the current implementation of get_sgtable (both swiotlb and > > iommu) is broken if dma_declare_coherent_memory() is used since such > > memory does not have a corresponding struct page. To be addressed in a > > separate patch. > > > > Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU") > > Reported-by: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > --- > > > > Here's my attempt of fixing this, though I'd like to wait for Robin's review > > (who's currently on holiday until next week). > > > > Not tested. > > The general approach is sound, and I can't see anything that looks > obviously wrong with the implementation, although I have no easy way of > testing it either: > > Acked-by: Robin Murphy <robin.murphy@arm.com> Thanks for looking at this patch. Going through the past threads on this, I wonder whether the attribute is actually used anywhere (in which case we may be better off reverting the original patch). If improved IOMMU performance is required, there are likely better ways to achieve this like using a different page size at the IOMMU level. -- Catalin ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS 2017-05-04 14:03 ` Catalin Marinas @ 2017-05-04 14:08 ` Geert Uytterhoeven 2017-05-04 17:58 ` Catalin Marinas 0 siblings, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2017-05-04 14:08 UTC (permalink / raw) To: linux-arm-kernel Hi Catalin, On Thu, May 4, 2017 at 4:03 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > Going through the past threads on this, I wonder whether the attribute > is actually used anywhere (in which case we may be better off reverting > the original patch). If improved IOMMU performance is required, there AFAIK, the users are somewhere in the pipeline. > are likely better ways to achieve this like using a different page size > at the IOMMU level. If I'm not mistaken, it's intended to be used when one device cannot use the IOMMU for whatever reason. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS 2017-05-04 14:08 ` Geert Uytterhoeven @ 2017-05-04 17:58 ` Catalin Marinas 0 siblings, 0 replies; 6+ messages in thread From: Catalin Marinas @ 2017-05-04 17:58 UTC (permalink / raw) To: linux-arm-kernel On Thu, May 04, 2017 at 04:08:29PM +0200, Geert Uytterhoeven wrote: > On Thu, May 4, 2017 at 4:03 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > Going through the past threads on this, I wonder whether the attribute > > is actually used anywhere (in which case we may be better off reverting > > the original patch). If improved IOMMU performance is required, there > > AFAIK, the users are somewhere in the pipeline. Let's hope we are going to see some users soon. I'll push the fix as well to avoid a regression (though I cannot fully test your patch, nor my fix). -- Catalin ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS 2017-04-25 18:00 ` [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS Catalin Marinas 2017-05-04 11:29 ` Robin Murphy @ 2017-05-05 10:01 ` Andrzej Hajda 1 sibling, 0 replies; 6+ messages in thread From: Andrzej Hajda @ 2017-05-05 10:01 UTC (permalink / raw) To: linux-arm-kernel On 25.04.2017 20:00, Catalin Marinas wrote: > While honouring the DMA_ATTR_FORCE_CONTIGUOUS on arm64 (commit > 44176bb38fa4: "arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to > IOMMU"), the existing uses of dma_mmap_attrs() and dma_get_sgtable() > have been broken by passing a physically contiguous vm_struct with an > invalid pages pointer through the common iommu API. > > Since the coherent allocation with DMA_ATTR_FORCE_CONTIGUOUS uses CMA, > this patch simply reuses the existing swiotlb logic for mmap and > get_sgtable. > > Note that the current implementation of get_sgtable (both swiotlb and > iommu) is broken if dma_declare_coherent_memory() is used since such > memory does not have a corresponding struct page. To be addressed in a > separate patch. > > Fixes: 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU") > Reported-by: Andrzej Hajda <a.hajda@samsung.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Tested-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-05 10:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170425180040epcas4p3d96f58d43c973feefbb31e97b9fb2ace@epcas4p3.samsung.com>
2017-04-25 18:00 ` [PATCH] arm64: Fix the DMA mmap and get_sgtable API with DMA_ATTR_FORCE_CONTIGUOUS Catalin Marinas
2017-05-04 11:29 ` Robin Murphy
2017-05-04 14:03 ` Catalin Marinas
2017-05-04 14:08 ` Geert Uytterhoeven
2017-05-04 17:58 ` Catalin Marinas
2017-05-05 10:01 ` Andrzej Hajda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).