[PATCH v2] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code

[Catalin Marinas]

Hi Geert,

On Fri, Apr 21, 2017 at 07:39:43PM +0200, Geert Uytterhoeven wrote:
> On Fri, Apr 21, 2017 at 7:32 PM, Catalin Marinas
> <catalin.marinas at arm.com> wrote:
> > On Fri, Mar 31, 2017 at 01:02:51PM +0200, Andrzej Hajda wrote:
> >> In case of DMA_ATTR_FORCE_CONTIGUOUS allocations vm_area->pages
> >> is invalid. __iommu_mmap_attrs and __iommu_get_sgtable should not
> >> use it and take advantage of contiguous nature of the allocation.
> >
> > As I replied to your original patch, I think
> > dma_common_contiguous_remap() should be fixed not to leave a dangling
> > area->pages pointer.
> >
> >>  arch/arm64/mm/dma-mapping.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> >> index f7b5401..41c7c36 100644
> >> --- a/arch/arm64/mm/dma-mapping.c
> >> +++ b/arch/arm64/mm/dma-mapping.c
> >> @@ -703,6 +703,10 @@ 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)
> >> +             return vm_iomap_memory(vma,
> >> +                             page_to_phys(vmalloc_to_page(cpu_addr)), size);
> >
> > I replied to Geert's patch on whether we actually need to call
> > dma_common_contiguous_remap() in __iommu_alloc_attrs() if the device is
> > coherent. We don't do this for the swiotlb allocation. If we are going
> > to change this, cpu_addr may or may not be in the vmalloc range and
> > vmalloc_to_page() would fail (I guess we could add an is_vmalloc_addr()
> > check).
> >
> > Alternatively, just open-code dma_common_contiguous_remap() in
> > __iommu_alloc_attrs() to keep a valid area->pages around (I think Geert
> > already suggested this). AFAICT, arch/arm does this already in its own
> > __iommu_alloc_buffer().
> >
> > Yet another option would be for iommu_dma_alloc() to understand
> > DMA_ATTR_FORCE_CONTIGUOUS and remove the arm64-specific handling.
> 
> That was actually the approach I took in my v1.
> V2 changed that to provide standalone iommu_dma_{alloc,free}_contiguous()
> functions.
> V3 changed that to call everything directly from the arm64 code.
> ...

I now read through the past discussions (as I ignored the threads
previously). I got Robin's point of not extending iommu_dma_alloc()
(though it looked simpler) and open-coding dma_common_contiguous_remap()
wouldn't make sense either as a way to pass this buffer to
iommu_dma_mmap() since it wasn't returned by iommu_dma_alloc().

So I think we should just fall back to the swiotlb ops for the mmap and
get_sgtable but since we don't have a dma_addr_t handle (we only have
the one of the other side of the IOMMU), we'd need to factor out the
common code from __swiotlb_mmap into a __swiotlb_mmap_page (similarly
for __swiotlb_get_sgtable). The __iommu_* functions would call these
with the correct page (from vmalloc_to_page) if
DMA_ATTR_FORCE_CONTIGUOUS and the buffer is not a candidate for
*_from_coherent.

While fixing/removing dma_get_sgtable() is a nice goal, we first need to
address DMA_ATTR_FORCE_CONTIGUOUS on arm64 since the patch breaks
existing use-cases (and I'd like to avoid reverting this patch).

-- 
Catalin