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

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 30/03/17 12:53, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Thu, Mar 30, 2017 at 1:46 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> 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...

Indeed it can. See if you can spot anything I've said in defence of that
particular function ;)

>> 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.

Isn't that what remap_pfn_range() already is? iommu_dma_mmap() had to
exist specifically because the likes of dma_common_mmap() *are* using
that on the assumption of physically contiguous ranges. I don't really
have the time or inclination to go digging into this myself, but there's
almost certainly room for some improvement and/or cleanup in the common
code too (and as I said, if something can be done there than I would
rather it were tackled head-on than worked around with point fixes in
the arch code).

Robin.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
>