Hi Andrzej,
On Thu, Mar 30, 2017 at 8:51 AM, Andrzej Hajda <a.ha...@samsung.com> wrote:
> On 29.03.2017 17:33, Robin Murphy wrote:
>> On 29/03/17 16:12, Andrzej Hajda wrote:
>>> On 29.03.2017 14:55, Robin Murphy wrote:
>>>> On 29/03/17 11:05, Andrzej Hajda wrote:
>>>>> In case of DMA_ATTR_FORCE_CONTIGUOUS allocations vm_area->pages
>>>>> is invalid. __iommu_mmap_attrs and __iommu_get_sgtable cannot use
>>>>> it. In first case temporary pages array is passed to iommu_dma_mmap,
>>>>> in 2nd case single entry sg table is created directly instead
>>>>> of calling helper.
>>>>>
>>>>> Fixes: 44176bb ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to
>>>>> IOMMU")
>>>>> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
>>>>> ---
>>>>> Hi,
>>>>>
>>>>> I am not familiar with this framework so please don't be too cruel ;)
>>>>> Alternative solution I see is to always create vm_area->pages,
>>>>> I do not know which approach is preferred.
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>> ---
>>>>> arch/arm64/mm/dma-mapping.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 38 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>>>> index f7b5401..bba2bc8 100644
>>>>> --- a/arch/arm64/mm/dma-mapping.c
>>>>> +++ b/arch/arm64/mm/dma-mapping.c
>>>>> @@ -704,7 +704,30 @@ static int __iommu_mmap_attrs(struct device *dev,
>>>>> struct vm_area_struct *vma,
>>>>> return ret;
>>>>>
>>>>> area = find_vm_area(cpu_addr);
>>>>> - if (WARN_ON(!area || !area->pages))
>>>>> + if (WARN_ON(!area))
>>>> >From the look of things, it doesn't seem strictly necessary to change
>>>> this, but whether that's a good thing is another matter. I'm not sure
>>>> that dma_common_contiguous_remap() should really be leaving a dangling
>>>> pointer in area->pages as it apparently does... :/
>>>>
>>>>> + return -ENXIO;
>>>>> +
>>>>> + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
>>>>> + struct page *page = vmalloc_to_page(cpu_addr);
>>>>> + unsigned int count = size >> PAGE_SHIFT;
>>>>> + struct page **pages;
>>>>> + unsigned long pfn;
>>>>> + int i;
>>>>> +
>>>>> + pages = kmalloc_array(count, sizeof(*pages), GFP_KERNEL);
>>>>> + if (!pages)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + for (i = 0, pfn = page_to_pfn(page); i < count; i++)
>>>>> + pages[i] = pfn_to_page(pfn + i);
>>>>> +
>>>>> + ret = iommu_dma_mmap(pages, size, vma);
>>>> /**
>>>> * iommu_dma_mmap - Map a buffer into provided user VMA
>>>> * @pages: Array representing buffer from iommu_dma_alloc()
>>>> ...
>>>>
>>>> In this case, the buffer has not come from iommu_dma_alloc(), so passing
>>>> into iommu_dma_mmap() is wrong by contract, even if having to fake up a
>>>> page array wasn't enough of a red flag. Given that a FORCE_CONTIGUOUS
>>>> allocation is essentially the same as for the non-IOMMU case, I think it
>>>> should be sufficient to defer to that path, i.e.:
>>>>
>>>> if (attrs & DMA_ATTR_FORCE_CONTIGUOUS)
>>>> return __swiotlb_mmap(dev, vma, cpu_addr,
>>>> phys_to_dma(virt_to_phys(cpu_addr)),
>>>> size, attrs);
>>> Maybe I have make mistake somewhere but it does not work, here and below
>>> (hangs or crashes). I suppose it can be due to different address
>>> translations, my patch uses
>>> page = vmalloc_to_page(cpu_addr).
>>> And here we have:
>>> handle = phys_to_dma(dev, virt_to_phys(cpu_addr)); // in
>>> __iommu_mmap_attrs
>>> page = phys_to_page(dma_to_phys(dev, handle)); // in
>>> __swiotlb_get_sgtable
>>> I guess it is similarly in __swiotlb_mmap.
>>>
>>> Are these translations equivalent?
>> Ah, my mistake, sorry - I managed to forget that cpu_addr is always
>> remapped for FORCE_CONTIGUOUS (*and* somehow overlook the use of
>> vmalloc_to_page() in the patch itself), so the virt_to_phys() part of my
>> example is indeed bogus. The general point still stands, though.
>
> 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?
Thanks!
> support to DMA_ATTR_FORCE_CONTIGUOUS in IOMMU on arm64 [1]?
> Maybe temporary solution is to drop the patch until proper handling of
> mmapping is proposed, without it the patch looks incomplete and causes
> regression.
> Moreover __iommu_alloc_attrs uses also dma_common_contiguous_remap which
> also assumes existence of struct pages.
>
> [1]: https://patchwork.kernel.org/patch/9609551/
> [2]: https://www.spinics.net/lists/arm-kernel/msg572688.html
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
