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 <[email protected]>
---
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
Hi Boris,
On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon
<[email protected]> 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 <[email protected]>
> ---
> 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
+Christoph who introduced the CONFIG_ARCH_NO_COHERENT_DMA_MMAP option.
Hi Max,
On Wed, 29 Mar 2017 15:48:24 -0700
Max Filippov <[email protected]> wrote:
> Hi Boris,
>
> On Tue, Mar 28, 2017 at 1:20 AM, Boris Brezillon
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
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.
On Thu, Mar 30, 2017 at 1:30 AM, Boris Brezillon
<[email protected]> 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
On Thu, 30 Mar 2017 02:05:28 -0700
Max Filippov <[email protected]> wrote:
> On Thu, Mar 30, 2017 at 1:30 AM, Boris Brezillon
> <[email protected]> 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 <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
when submitting the patch.
Thanks,
Boris