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

[Geert Uytterhoeven]

Hi Robin,

On Thu, Mar 30, 2017 at 1:46 PM, Robin Murphy <robin.murphy at arm.com> wrote:
> On 30/03/17 12:16, Andrzej Hajda wrote:
> [...]
>>>>>> I guess Geert's proposition to create pages permanently is also not
>>>>>> acceptable[2]. So how to fix crashes which appeared after patch adding
>>>>> If I'm not mistaken, creating the pages permanently is what the
>>>>> !DMA_ATTR_FORCE_CONTIGUOUS does? The only difference is where
>>>>> the underlying memory is allocated from.
>>>>>
>>>>> Am I missing something?
>>>> Quoting Robin from his response:
>>>>> in general is is not
>>>>> safe to assume a coherent allocation is backed by struct pages at all
>>>> As I said before I am not familiar with the subject, so it is possible I
>>>> misunderstood something.
>>> That's from the perspective of external callers of
>>> dma_alloc_coherent()/dma_alloc_attrs(), wherein
>>> dma_alloc_from_coherent() may have returned device-specific memory
>>> without calling into the arch dma_map_ops implementation. Internal to
>>> the arm64 implementation, however, everything *we* allocate comes from
>>> either CMA or the normal page allocator, so should always be backed by
>>> real kernel pages.
>>>
>>> Robin.
>>
>> OK, so what do you exactly mean by "The general point still stands", my
>> patch fixes handling of allocations made internally?
>
> That since FORCE_CONTIGUOUS allocations always come from CMA, you can
> let the existing CMA-based implementations handle them just by fixing up
> dma_addr appropriately.
>
>> Anyway as I understand approach "creating the pages permanently" in
>> __iommu_alloc_attrs is OK. Is it worth to create patch adding it? It
>> should solve the issue as well and also looks saner (for me).
>
> Sure, you *could* - there's nothing technically wrong with that other
> than violating a strict interpretation of the iommu-dma API
> documentation if you pass it into iommu_dma_mmap() - it's just that the
> only point of using the pages array here in the first place is to cover
> the general case where an allocation might not be physically contiguous.
> If you have a guarantee that a given allocation definitely *is* both
> physically and virtually contiguous, then that's a bunch of extra work
> you simply have no need to do.

The same can be said for dma_common_contiguous_remap() below...

> If you do want to go down that route, then I would much rather we fix
> dma_common_contiguous_remap() to leave a valid array in area->pages in
> the first place, than be temporarily faking them up around individual calls.

The only point of using the pages array here in the first place is to cover
the general case in dma_common_pages_remap().

Providing a contiguous variant of map_vm_area() could resolve that.

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