From: Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> To: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>, Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>, Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>, Bartlomiej Zolnierkiewicz <b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Subject: Re: [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code Date: Wed, 29 Mar 2017 17:12:28 +0200 [thread overview] Message-ID: <073f1feb-d6da-c93f-8e67-0910befec27b@samsung.com> (raw) In-Reply-To: <15b1be13-625f-db74-d213-ad1df86f7eb5-5wv7dgnIgG8@public.gmane.org> 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.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> >> --- >> 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? 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, >> > > >
WARNING: multiple messages have this Message-ID (diff)
From: a.hajda@samsung.com (Andrzej Hajda) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code Date: Wed, 29 Mar 2017 17:12:28 +0200 [thread overview] Message-ID: <073f1feb-d6da-c93f-8e67-0910befec27b@samsung.com> (raw) In-Reply-To: <15b1be13-625f-db74-d213-ad1df86f7eb5@arm.com> 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.hajda@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? 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, >> > > >
next prev parent reply other threads:[~2017-03-29 15:12 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <CGME20170329100540eucas1p22d428e33aa678cbef0a24dd249820451@eucas1p2.samsung.com> 2017-03-29 10:05 ` [PATCH] arm64/dma-mapping: fix DMA_ATTR_FORCE_CONTIGUOUS mmaping code Andrzej Hajda 2017-03-29 10:05 ` Andrzej Hajda [not found] ` <1490781926-6209-1-git-send-email-a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2017-03-29 12:54 ` Geert Uytterhoeven 2017-03-29 12:54 ` Geert Uytterhoeven 2017-03-29 12:55 ` Robin Murphy 2017-03-29 12:55 ` Robin Murphy [not found] ` <15b1be13-625f-db74-d213-ad1df86f7eb5-5wv7dgnIgG8@public.gmane.org> 2017-03-29 15:12 ` Andrzej Hajda [this message] 2017-03-29 15:12 ` Andrzej Hajda [not found] ` <073f1feb-d6da-c93f-8e67-0910befec27b-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2017-03-29 15:33 ` Robin Murphy 2017-03-29 15:33 ` Robin Murphy [not found] ` <c13f6475-f503-238f-c101-4f9cdf1b0ae2-5wv7dgnIgG8@public.gmane.org> 2017-03-30 6:51 ` Andrzej Hajda 2017-03-30 6:51 ` Andrzej Hajda [not found] ` <45fde94b-9487-c28e-9d4e-3b36c4b4a934-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2017-03-30 7:40 ` Geert Uytterhoeven 2017-03-30 7:40 ` Geert Uytterhoeven [not found] ` <CAMuHMdWkOdAD=d0DAy7cJTHcQc7oq5HVDvTNtjfQETMynx1_+g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-30 8:30 ` Andrzej Hajda 2017-03-30 8:30 ` Andrzej Hajda [not found] ` <8dcbb0b3-dd13-d2b3-90f5-64b900d67778-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2017-03-30 10:44 ` Robin Murphy 2017-03-30 10:44 ` Robin Murphy [not found] ` <41097b2c-9bbc-31e4-d17d-8c3f638ec1a6-5wv7dgnIgG8@public.gmane.org> 2017-03-30 11:16 ` Andrzej Hajda 2017-03-30 11:16 ` Andrzej Hajda [not found] ` <bd77ea57-96a8-bbc4-8184-9ddfd443f3c6-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2017-03-30 11:46 ` Robin Murphy 2017-03-30 11:46 ` Robin Murphy [not found] ` <868d0c3c-6b85-0709-1686-66b484ede46d-5wv7dgnIgG8@public.gmane.org> 2017-03-30 11:53 ` Geert Uytterhoeven 2017-03-30 11:53 ` Geert Uytterhoeven [not found] ` <CAMuHMdUp3-pdZ+w+Y-GRfHbkdnjHbP=+H=A_ukTwDD5q_WMi1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-30 14:10 ` Robin Murphy 2017-03-30 14:10 ` Robin Murphy 2017-04-21 16:12 ` Catalin Marinas 2017-04-21 16:12 ` Catalin Marinas [not found] ` <20170421161234.GD5312-M2fw3Uu6cmfZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org> 2017-04-24 16:58 ` Laura Abbott 2017-04-24 16:58 ` Laura Abbott [not found] ` <a344c9c8-8a1f-cfc4-4dcd-1b49f3e1ac6c-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-04-25 14:11 ` Catalin Marinas 2017-04-25 14:11 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=073f1feb-d6da-c93f-8e67-0910befec27b@samsung.com \ --to=a.hajda-sze3o3uu22jbdgjk7y7tuq@public.gmane.org \ --cc=b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \ --cc=catalin.marinas-5wv7dgnIgG8@public.gmane.org \ --cc=geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org \ --cc=iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=robin.murphy-5wv7dgnIgG8@public.gmane.org \ --cc=will.deacon-5wv7dgnIgG8@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.