| Message ID | 20190422175942.18788-15-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] arm64/iommu: handle non-remapped addresses in ->mmap and ->get_sgtable | expand |
On 22/04/2019 18:59, Christoph Hellwig wrote: > From: Robin Murphy <robin.murphy@arm.com> > > The freeing logic was made particularly horrible by part of it being > opaque to the arch wrapper, which led to a lot of convoluted repetition > to ensure each path did everything in the right order. Now that it's > all private, we can pick apart and consolidate the logically-distinct > steps of freeing the IOMMU mapping, the underlying pages, and the CPU > remap (if necessary) into something much more manageable. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > [various cosmetic changes to the code flow] Hmm, I do still prefer my original flow with the dma_common_free_remap() call right out of the way at the end rather than being a special case in the middle of all the page-freeing (which is the kind of existing complexity I was trying to eliminate). I guess you've done this to avoid having "if (!dma_release_from_contiguous(...))..." twice like I ended up with, which is fair enough I suppose - once we manage to solve the new dma_{alloc,free}_contiguous() interface that may tip the balance so I can always revisit this then. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/iommu/dma-iommu.c | 75 ++++++++++++++++++--------------------- > 1 file changed, 35 insertions(+), 40 deletions(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 4632b9d301a1..9658c4cc3cfe 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -916,6 +916,41 @@ static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, > __iommu_dma_unmap(dev, handle, size); > } > > +static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, > + dma_addr_t handle, unsigned long attrs) > +{ > + size_t alloc_size = PAGE_ALIGN(size); > + int count = alloc_size >> PAGE_SHIFT; > + struct page *page = NULL; > + > + __iommu_dma_unmap(dev, handle, size); > + > + /* Non-coherent atomic allocation? Easy */ > + if (dma_free_from_pool(cpu_addr, alloc_size)) > + return; > + > + if (is_vmalloc_addr(cpu_addr)) { > + /* > + * If it the address is remapped, then it's either non-coherent s/If it/If/ My "easy; more involved; most complex" narrative definitely doesn't translate very well to a different order :) Otherwise, Reluctantly-Acked-by: Robin Murphy <robin.murphy@arm.com> > + * or highmem CMA, or an iommu_dma_alloc_remap() construction. > + */ > + struct page **pages = __iommu_dma_get_pages(cpu_addr); > + > + if (pages) > + __iommu_dma_free_pages(pages, count); > + else > + page = vmalloc_to_page(cpu_addr); > + > + dma_common_free_remap(cpu_addr, alloc_size, VM_USERMAP); > + } else { > + /* Lowmem means a coherent atomic or CMA allocation */ > + page = virt_to_page(cpu_addr); > + } > + > + if (page && !dma_release_from_contiguous(dev, page, count)) > + __free_pages(page, get_order(alloc_size)); > +} > + > static void *iommu_dma_alloc(struct device *dev, size_t size, > dma_addr_t *handle, gfp_t gfp, unsigned long attrs) > { > @@ -985,46 +1020,6 @@ static void *iommu_dma_alloc(struct device *dev, size_t size, > return addr; > } > > -static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, > - dma_addr_t handle, unsigned long attrs) > -{ > - size_t iosize = size; > - > - size = PAGE_ALIGN(size); > - /* > - * @cpu_addr will be one of 4 things depending on how it was allocated: > - * - A remapped array of pages for contiguous allocations. > - * - A remapped array of pages from iommu_dma_alloc_remap(), for all > - * non-atomic allocations. > - * - A non-cacheable alias from the atomic pool, for atomic > - * allocations by non-coherent devices. > - * - A normal lowmem address, for atomic allocations by > - * coherent devices. > - * Hence how dodgy the below logic looks... > - */ > - if (dma_in_atomic_pool(cpu_addr, size)) { > - __iommu_dma_unmap(dev, handle, iosize); > - dma_free_from_pool(cpu_addr, size); > - } else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { > - struct page *page = vmalloc_to_page(cpu_addr); > - > - __iommu_dma_unmap(dev, handle, iosize); > - dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT); > - dma_common_free_remap(cpu_addr, size, VM_USERMAP); > - } else if (is_vmalloc_addr(cpu_addr)){ > - struct page **pages = __iommu_dma_get_pages(cpu_addr); > - > - if (WARN_ON(!pages)) > - return; > - __iommu_dma_unmap(dev, handle, iosize); > - __iommu_dma_free_pages(pages, size >> PAGE_SHIFT); > - dma_common_free_remap(cpu_addr, size, VM_USERMAP); > - } else { > - __iommu_dma_unmap(dev, handle, iosize); > - __free_pages(virt_to_page(cpu_addr), get_order(size)); > - } > -} > - > static int __iommu_dma_mmap_pfn(struct vm_area_struct *vma, > unsigned long pfn, size_t size) > { >
On Mon, Apr 29, 2019 at 02:59:43PM +0100, Robin Murphy wrote: > Hmm, I do still prefer my original flow with the dma_common_free_remap() > call right out of the way at the end rather than being a special case in > the middle of all the page-freeing (which is the kind of existing > complexity I was trying to eliminate). I guess you've done this to avoid > having "if (!dma_release_from_contiguous(...))..." twice like I ended up > with, which is fair enough I suppose - once we manage to solve the new > dma_{alloc,free}_contiguous() interface that may tip the balance so I can > always revisit this then. Ok, I'll try to accomodate that with a minor rework.
On Mon, Apr 29, 2019 at 09:03:48PM +0200, Christoph Hellwig wrote: > On Mon, Apr 29, 2019 at 02:59:43PM +0100, Robin Murphy wrote: > > Hmm, I do still prefer my original flow with the dma_common_free_remap() > > call right out of the way at the end rather than being a special case in > > the middle of all the page-freeing (which is the kind of existing > > complexity I was trying to eliminate). I guess you've done this to avoid > > having "if (!dma_release_from_contiguous(...))..." twice like I ended up > > with, which is fair enough I suppose - once we manage to solve the new > > dma_{alloc,free}_contiguous() interface that may tip the balance so I can > > always revisit this then. > > Ok, I'll try to accomodate that with a minor rework. Does this look reasonable? diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 5b2a2bf44078..f884d22b1388 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -921,7 +921,7 @@ static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, { size_t alloc_size = PAGE_ALIGN(size); int count = alloc_size >> PAGE_SHIFT; - struct page *page = NULL; + struct page *page = NULL, **pages = NULL; __iommu_dma_unmap(dev, handle, size); @@ -934,19 +934,17 @@ static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, * If it the address is remapped, then it's either non-coherent * or highmem CMA, or an iommu_dma_alloc_remap() construction. */ - struct page **pages = __iommu_dma_get_pages(cpu_addr); - - if (pages) - __iommu_dma_free_pages(pages, count); - else + pages = __iommu_dma_get_pages(cpu_addr); + if (!pages) page = vmalloc_to_page(cpu_addr); - dma_common_free_remap(cpu_addr, alloc_size, VM_USERMAP); } else { /* Lowmem means a coherent atomic or CMA allocation */ page = virt_to_page(cpu_addr); } + if (pages) + __iommu_dma_free_pages(pages, count); if (page && !dma_release_from_contiguous(dev, page, count)) __free_pages(page, get_order(alloc_size)); }
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 4632b9d301a1..9658c4cc3cfe 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -916,6 +916,41 @@ static void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle, __iommu_dma_unmap(dev, handle, size); } +static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, + dma_addr_t handle, unsigned long attrs) +{ + size_t alloc_size = PAGE_ALIGN(size); + int count = alloc_size >> PAGE_SHIFT; + struct page *page = NULL; + + __iommu_dma_unmap(dev, handle, size); + + /* Non-coherent atomic allocation? Easy */ + if (dma_free_from_pool(cpu_addr, alloc_size)) + return; + + if (is_vmalloc_addr(cpu_addr)) { + /* + * If it the address is remapped, then it's either non-coherent + * or highmem CMA, or an iommu_dma_alloc_remap() construction. + */ + struct page **pages = __iommu_dma_get_pages(cpu_addr); + + if (pages) + __iommu_dma_free_pages(pages, count); + else + page = vmalloc_to_page(cpu_addr); + + dma_common_free_remap(cpu_addr, alloc_size, VM_USERMAP); + } else { + /* Lowmem means a coherent atomic or CMA allocation */ + page = virt_to_page(cpu_addr); + } + + if (page && !dma_release_from_contiguous(dev, page, count)) + __free_pages(page, get_order(alloc_size)); +} + static void *iommu_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle, gfp_t gfp, unsigned long attrs) { @@ -985,46 +1020,6 @@ static void *iommu_dma_alloc(struct device *dev, size_t size, return addr; } -static void iommu_dma_free(struct device *dev, size_t size, void *cpu_addr, - dma_addr_t handle, unsigned long attrs) -{ - size_t iosize = size; - - size = PAGE_ALIGN(size); - /* - * @cpu_addr will be one of 4 things depending on how it was allocated: - * - A remapped array of pages for contiguous allocations. - * - A remapped array of pages from iommu_dma_alloc_remap(), for all - * non-atomic allocations. - * - A non-cacheable alias from the atomic pool, for atomic - * allocations by non-coherent devices. - * - A normal lowmem address, for atomic allocations by - * coherent devices. - * Hence how dodgy the below logic looks... - */ - if (dma_in_atomic_pool(cpu_addr, size)) { - __iommu_dma_unmap(dev, handle, iosize); - dma_free_from_pool(cpu_addr, size); - } else if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) { - struct page *page = vmalloc_to_page(cpu_addr); - - __iommu_dma_unmap(dev, handle, iosize); - dma_release_from_contiguous(dev, page, size >> PAGE_SHIFT); - dma_common_free_remap(cpu_addr, size, VM_USERMAP); - } else if (is_vmalloc_addr(cpu_addr)){ - struct page **pages = __iommu_dma_get_pages(cpu_addr); - - if (WARN_ON(!pages)) - return; - __iommu_dma_unmap(dev, handle, iosize); - __iommu_dma_free_pages(pages, size >> PAGE_SHIFT); - dma_common_free_remap(cpu_addr, size, VM_USERMAP); - } else { - __iommu_dma_unmap(dev, handle, iosize); - __free_pages(virt_to_page(cpu_addr), get_order(size)); - } -} - static int __iommu_dma_mmap_pfn(struct vm_area_struct *vma, unsigned long pfn, size_t size) {