* [PATCH] xtensa: Fix mmap() support @ 2017-03-28 8:20 Boris Brezillon 2017-03-29 22:48 ` Max Filippov 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2017-03-28 8:20 UTC (permalink / raw) To: Chris Zankel, Max Filippov, linux-xtensa Cc: Maxime Ripard, Thomas Petazzoni, linux-kernel, Boris Brezillon The xtensa architecture does not implement the dma_map_ops->mmap() hook, thus relying on the default dma_common_mmap() implementation. This implementation is only safe on DMA-coherent architecture (hence the !defined(CONFIG_ARCH_NO_COHERENT_DMA_MMAP) condition), but xtensa does not fall in this case. This lead to bad pfn calculation when someone tries to mmap() one or several pages that are not part of the identity mapping because the dma_common_mmap() extract the pfn value from the virt address using virt_to_page() which is only applicable on DMA-coherent platforms (on other platforms, DMA coherent pages are mapped in a different region). Implement xtensa_dma_mmap() (loosely based on __arm_dma_mmap()) in which pfn is extracted from the DMA address using PFN_DOWN(). While we're at it, select ARCH_NO_COHERENT_DMA_MMAP from the XTENSA entry so that we never silently fallback to dma_common_mmap() if someone decides to drop the xtensa_dma_mmap() implementation. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- Hello, This bug has been detected while developping a DRM driver on an FPGA containing an Xtensa CPU. The DRM driver is using the generic CMA GEM implementation which is allocating DMA coherent buffers in kernel space and then allows userspace programs to mmap() these buffers. Whith the existing implementation, the userspace pointer was pointing to a completely different physical region, thus leading to bad display output and memory corruptions. I'm not sure the xtensa_dma_mmap() implementation is correct, but it seems to solve my problem. Don't hesitate to propose a different implementation. Thanks, Boris --- arch/xtensa/Kconfig | 1 + arch/xtensa/kernel/pci-dma.c | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig index f4126cf997a4..2e672a5e9fdf 100644 --- a/arch/xtensa/Kconfig +++ b/arch/xtensa/Kconfig @@ -3,6 +3,7 @@ config ZONE_DMA config XTENSA def_bool y + select ARCH_NO_COHERENT_DMA_MMAP select ARCH_WANT_FRAME_POINTERS select ARCH_WANT_IPC_PARSE_VERSION select BUILDTIME_EXTABLE_SORT diff --git a/arch/xtensa/kernel/pci-dma.c b/arch/xtensa/kernel/pci-dma.c index 70e362e6038e..8f3ef49ceba6 100644 --- a/arch/xtensa/kernel/pci-dma.c +++ b/arch/xtensa/kernel/pci-dma.c @@ -249,9 +249,32 @@ int xtensa_dma_mapping_error(struct device *dev, dma_addr_t dma_addr) return 0; } +static int xtensa_dma_mmap(struct device *dev, struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + unsigned long attrs) +{ + int ret = -ENXIO; +#ifdef CONFIG_MMU + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; + unsigned long pfn = PFN_DOWN(dma_addr); + unsigned long off = vma->vm_pgoff; + + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) + return ret; + + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) + ret = remap_pfn_range(vma, vma->vm_start, pfn + off, + vma->vm_end - vma->vm_start, + vma->vm_page_prot); +#endif /* CONFIG_MMU */ + return ret; +} + struct dma_map_ops xtensa_dma_map_ops = { .alloc = xtensa_dma_alloc, .free = xtensa_dma_free, + .mmap = xtensa_dma_mmap, .map_page = xtensa_map_page, .unmap_page = xtensa_unmap_page, .map_sg = xtensa_map_sg, -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xtensa: Fix mmap() support 2017-03-28 8:20 [PATCH] xtensa: Fix mmap() support Boris Brezillon @ 2017-03-29 22:48 ` Max Filippov 2017-03-30 8:30 ` Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Max Filippov @ 2017-03-29 22:48 UTC (permalink / raw) To: Boris Brezillon Cc: Chris Zankel, linux-xtensa, Maxime Ripard, Thomas Petazzoni, LKML [-- Attachment #1: Type: text/plain, Size: 3787 bytes --] Hi Boris, On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > The xtensa architecture does not implement the dma_map_ops->mmap() hook, > thus relying on the default dma_common_mmap() implementation. > This implementation is only safe on DMA-coherent architecture (hence the > !defined(CONFIG_ARCH_NO_COHERENT_DMA_MMAP) condition), but xtensa does not > fall in this case. Why not? AFAIU "DMA-coherent" may mean "having DMA memory accessible w/o caching", is that right? We have all low memory mapped in the uncached KSEG region, it may be mapped to the userspace w/o caching as well, that's what dma_common_mmap does. > This lead to bad pfn calculation when someone tries to mmap() one or > several pages that are not part of the identity mapping because the > dma_common_mmap() extract the pfn value from the virt address using > virt_to_page() which is only applicable on DMA-coherent platforms (on > other platforms, DMA coherent pages are mapped in a different region). Oh, yes. Our __pa implementation is buggy in a sense that it doesn't work correctly with addresses from the uncached KSEG. > Implement xtensa_dma_mmap() (loosely based on __arm_dma_mmap()) in which > pfn is extracted from the DMA address using PFN_DOWN(). > > While we're at it, select ARCH_NO_COHERENT_DMA_MMAP from the XTENSA > entry so that we never silently fallback to dma_common_mmap() if someone > decides to drop the xtensa_dma_mmap() implementation. I don't think this is right. > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > Hello, > > This bug has been detected while developping a DRM driver on an FPGA > containing an Xtensa CPU. The DRM driver is using the generic CMA GEM > implementation which is allocating DMA coherent buffers in kernel space > and then allows userspace programs to mmap() these buffers. > > Whith the existing implementation, the userspace pointer was pointing > to a completely different physical region, thus leading to bad display > output and memory corruptions. > > I'm not sure the xtensa_dma_mmap() implementation is correct, but it > seems to solve my problem. > > Don't hesitate to propose a different implementation. Could you please instead check if the dma_common_mmap works for you with the attached patch? [...] > diff --git a/arch/xtensa/kernel/pci-dma.c b/arch/xtensa/kernel/pci-dma.c > index 70e362e6038e..8f3ef49ceba6 100644 > --- a/arch/xtensa/kernel/pci-dma.c > +++ b/arch/xtensa/kernel/pci-dma.c > @@ -249,9 +249,32 @@ int xtensa_dma_mapping_error(struct device *dev, dma_addr_t dma_addr) > return 0; > } > > +static int xtensa_dma_mmap(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > + unsigned long attrs) > +{ > + int ret = -ENXIO; > +#ifdef CONFIG_MMU > + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > + unsigned long pfn = PFN_DOWN(dma_addr); > + unsigned long off = vma->vm_pgoff; > + > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > + return ret; > + > + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) > + ret = remap_pfn_range(vma, vma->vm_start, pfn + off, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); You use vma->vm_page_prot directly here, isn't it required to do vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); to make the mapping uncached? Because otherwise I guess you get a mapping with cache which on Xtensa is not going to be coherent. -- Thanks. -- Max [-- Attachment #2: 0001-WIP-xtensa-make-__pa-work-with-uncached-KSEG.patch --] [-- Type: text/x-patch, Size: 1126 bytes --] From e93a971e6a4802927f70a730fd3b71e6ae69fe39 Mon Sep 17 00:00:00 2001 From: Max Filippov <jcmvbkbc@gmail.com> Date: Wed, 29 Mar 2017 15:44:47 -0700 Subject: [PATCH] WIP: xtensa: make __pa work with uncached KSEG Signed-off-by: Max Filippov <jcmvbkbc@gmail.com> --- arch/xtensa/include/asm/page.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/arch/xtensa/include/asm/page.h b/arch/xtensa/include/asm/page.h index 976b1d7..ca02161 100644 --- a/arch/xtensa/include/asm/page.h +++ b/arch/xtensa/include/asm/page.h @@ -164,8 +164,21 @@ void copy_user_highpage(struct page *to, struct page *from, #define ARCH_PFN_OFFSET (PHYS_OFFSET >> PAGE_SHIFT) +#ifdef CONFIG_MMU +static inline unsigned long ___pa(unsigned long va) +{ + unsigned long off = va - PAGE_OFFSET; + + if (off > XCHAL_KSEG_SIZE) + off -= XCHAL_KSEG_SIZE; + + return off + PHYS_OFFSET; +} +#define __pa(x) ___pa((unsigned long)(x)) +#else #define __pa(x) \ ((unsigned long) (x) - PAGE_OFFSET + PHYS_OFFSET) +#endif #define __va(x) \ ((void *)((unsigned long) (x) - PHYS_OFFSET + PAGE_OFFSET)) #define pfn_valid(pfn) \ -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xtensa: Fix mmap() support 2017-03-29 22:48 ` Max Filippov @ 2017-03-30 8:30 ` Boris Brezillon 2017-03-30 8:45 ` Christoph Hellwig 2017-03-30 9:05 ` Max Filippov 0 siblings, 2 replies; 6+ messages in thread From: Boris Brezillon @ 2017-03-30 8:30 UTC (permalink / raw) To: Max Filippov Cc: Christoph Hellwig, Chris Zankel, linux-xtensa, Maxime Ripard, Thomas Petazzoni, LKML +Christoph who introduced the CONFIG_ARCH_NO_COHERENT_DMA_MMAP option. Hi Max, On Wed, 29 Mar 2017 15:48:24 -0700 Max Filippov <jcmvbkbc@gmail.com> wrote: > Hi Boris, > > On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > The xtensa architecture does not implement the dma_map_ops->mmap() hook, > > thus relying on the default dma_common_mmap() implementation. > > This implementation is only safe on DMA-coherent architecture (hence the > > !defined(CONFIG_ARCH_NO_COHERENT_DMA_MMAP) condition), but xtensa does not > > fall in this case. > > Why not? AFAIU "DMA-coherent" may mean "having DMA memory accessible > w/o caching", is that right? I think I misunderstood what CONFIG_ARCH_NO_COHERENT_DMA_MMAP means. My understanding was that, if CONFIG_ARCH_NO_COHERENT_DMA_MMAP is not set, the architecture is assumed to be cache-coherent, but re-reading the option name I realize you're probably right. > We have all low memory mapped in the uncached > KSEG region, it may be mapped to the userspace w/o caching as well, that's > what dma_common_mmap does. > > > This lead to bad pfn calculation when someone tries to mmap() one or > > several pages that are not part of the identity mapping because the > > dma_common_mmap() extract the pfn value from the virt address using > > virt_to_page() which is only applicable on DMA-coherent platforms (on > > other platforms, DMA coherent pages are mapped in a different region). > > Oh, yes. Our __pa implementation is buggy in a sense that it doesn't work > correctly with addresses from the uncached KSEG. I'm new to the xtensa architecture, and on ARM, virt_to_phys(), __pa() and co are known to be unsafe when used on DMA-coherent memory. Here it seems that you have twice the identity mapping in your virtual address space: once with the uncached flag set, and another one with cache activated, so it is indeed easier to patch __pa() to do the right thing. > > > Implement xtensa_dma_mmap() (loosely based on __arm_dma_mmap()) in which > > pfn is extracted from the DMA address using PFN_DOWN(). > > > > While we're at it, select ARCH_NO_COHERENT_DMA_MMAP from the XTENSA > > entry so that we never silently fallback to dma_common_mmap() if someone > > decides to drop the xtensa_dma_mmap() implementation. > > I don't think this is right. Yep, as said above, you're probably right. > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > Hello, > > > > This bug has been detected while developping a DRM driver on an FPGA > > containing an Xtensa CPU. The DRM driver is using the generic CMA GEM > > implementation which is allocating DMA coherent buffers in kernel space > > and then allows userspace programs to mmap() these buffers. > > > > Whith the existing implementation, the userspace pointer was pointing > > to a completely different physical region, thus leading to bad display > > output and memory corruptions. > > > > I'm not sure the xtensa_dma_mmap() implementation is correct, but it > > seems to solve my problem. > > > > Don't hesitate to propose a different implementation. > > Could you please instead check if the dma_common_mmap works for you > with the attached patch? I will. BTW, shouldn't it be if (off >= XCHAL_KSEG_SIZE) instead of if (off > XCHAL_KSEG_SIZE) ? > > [...] > > > diff --git a/arch/xtensa/kernel/pci-dma.c b/arch/xtensa/kernel/pci-dma.c > > index 70e362e6038e..8f3ef49ceba6 100644 > > --- a/arch/xtensa/kernel/pci-dma.c > > +++ b/arch/xtensa/kernel/pci-dma.c > > @@ -249,9 +249,32 @@ int xtensa_dma_mapping_error(struct device *dev, dma_addr_t dma_addr) > > return 0; > > } > > > > +static int xtensa_dma_mmap(struct device *dev, struct vm_area_struct *vma, > > + void *cpu_addr, dma_addr_t dma_addr, size_t size, > > + unsigned long attrs) > > +{ > > + int ret = -ENXIO; > > +#ifdef CONFIG_MMU > > + unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; > > + unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; > > + unsigned long pfn = PFN_DOWN(dma_addr); > > + unsigned long off = vma->vm_pgoff; > > + > > + if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret)) > > + return ret; > > + > > + if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) > > + ret = remap_pfn_range(vma, vma->vm_start, pfn + off, > > + vma->vm_end - vma->vm_start, > > + vma->vm_page_prot); > > You use vma->vm_page_prot directly here, isn't it required to do > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > to make the mapping uncached? Because otherwise I guess you > get a mapping with cache which on Xtensa is not going to be > coherent. Indeed. Anyway, we'll probably keep relying on dma_common_mmap() which is already doing the right thing. Thanks for the review. Regards, Boris ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xtensa: Fix mmap() support 2017-03-30 8:30 ` Boris Brezillon @ 2017-03-30 8:45 ` Christoph Hellwig 2017-03-30 9:05 ` Max Filippov 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2017-03-30 8:45 UTC (permalink / raw) To: Boris Brezillon Cc: Max Filippov, Christoph Hellwig, Chris Zankel, linux-xtensa, Maxime Ripard, Thomas Petazzoni, LKML On Thu, Mar 30, 2017 at 10:30:32AM +0200, Boris Brezillon wrote: > I think I misunderstood what CONFIG_ARCH_NO_COHERENT_DMA_MMAP means. > My understanding was that, if CONFIG_ARCH_NO_COHERENT_DMA_MMAP is not > set, the architecture is assumed to be cache-coherent, but re-reading > the option name I realize you're probably right. It just says that the architecture did not support dma_common_mmap at the point in time when I switched all architectures to use the common dma_ops or implementing the various dma calls. There is no inherent logic behind the symbol, and if the few remaining architectures could support it with a bit work the option could just go away. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xtensa: Fix mmap() support 2017-03-30 8:30 ` Boris Brezillon 2017-03-30 8:45 ` Christoph Hellwig @ 2017-03-30 9:05 ` Max Filippov 2017-03-30 10:05 ` Boris Brezillon 1 sibling, 1 reply; 6+ messages in thread From: Max Filippov @ 2017-03-30 9:05 UTC (permalink / raw) To: Boris Brezillon Cc: Christoph Hellwig, Chris Zankel, linux-xtensa, Maxime Ripard, Thomas Petazzoni, LKML On Thu, Mar 30, 2017 at 1:30 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> Could you please instead check if the dma_common_mmap works for you >> with the attached patch? > > I will. BTW, shouldn't it be > > if (off >= XCHAL_KSEG_SIZE) > > instead of > > if (off > XCHAL_KSEG_SIZE) > ? Oops. Yes, you're right. -- Thanks. -- Max ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xtensa: Fix mmap() support 2017-03-30 9:05 ` Max Filippov @ 2017-03-30 10:05 ` Boris Brezillon 0 siblings, 0 replies; 6+ messages in thread From: Boris Brezillon @ 2017-03-30 10:05 UTC (permalink / raw) To: Max Filippov Cc: Christoph Hellwig, Chris Zankel, linux-xtensa, Maxime Ripard, Thomas Petazzoni, LKML On Thu, 30 Mar 2017 02:05:28 -0700 Max Filippov <jcmvbkbc@gmail.com> wrote: > On Thu, Mar 30, 2017 at 1:30 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > >> Could you please instead check if the dma_common_mmap works for you > >> with the attached patch? > > > > I will. BTW, shouldn't it be > > > > if (off >= XCHAL_KSEG_SIZE) > > > > instead of > > > > if (off > XCHAL_KSEG_SIZE) > > ? > > Oops. Yes, you're right. > It works, you can add my Tested-by: Boris Brezillon <boris.brezillon@free-electrons.com> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> when submitting the patch. Thanks, Boris ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-30 10:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-28 8:20 [PATCH] xtensa: Fix mmap() support Boris Brezillon 2017-03-29 22:48 ` Max Filippov 2017-03-30 8:30 ` Boris Brezillon 2017-03-30 8:45 ` Christoph Hellwig 2017-03-30 9:05 ` Max Filippov 2017-03-30 10:05 ` Boris Brezillon
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.