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.
Robin.
>
>
> Regards
> Andrzej
>
>>> + kfree(pages);
>>> +
>>> + return ret;
>>> + }
>>> +
>>> + if (WARN_ON(!area->pages))
>>> return -ENXIO;
>>>
>>> return iommu_dma_mmap(area->pages, size, vma);
>>> @@ -717,7 +740,20 @@ static int __iommu_get_sgtable(struct device *dev,
>>> struct sg_table *sgt,
>>> unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>>> struct vm_struct *area = find_vm_area(cpu_addr);
>>>
>>> - if (WARN_ON(!area || !area->pages))
>>> + if (WARN_ON(!area))
>>> + return -ENXIO;
>>> +
>>> + if (attrs & DMA_ATTR_FORCE_CONTIGUOUS) {
>>> + int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
>>> +
>>> + if (!ret)
>>> + sg_set_page(sgt->sgl, vmalloc_to_page(cpu_addr),
>>> + PAGE_ALIGN(size), 0);
>>> +
>>> + return ret;
>>> + }
>> As above, this may as well just go straight to the non-IOMMU version,
>> although I agree with Russell's concerns[1] that in general is is not
>> safe to assume a coherent allocation is backed by struct pages at all.
>>
>> Robin.
>>
>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/496068.html
>>
>>> +
>>> + if (WARN_ON(!area->pages))
>>> return -ENXIO;
>>>
>>> return sg_alloc_table_from_pages(sgt, area->pages, count, 0, size,
>>>
>>
>>
>>
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
