Re: [PATCH] xtensa: Fix mmap() support

[Max Filippov]

Hi Boris,

On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon
<boris.brezillon@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
From e93a971e6a4802927f70a730fd3b71e6ae69fe39 Mon Sep 17 00:00:00 2001
From: Max Filippov <jcmvbkbc@xxxxxxxxx>
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@xxxxxxxxx>
---
 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