Re: [Xen-devel] [PATCH v1] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 12 May 2014, Konrad Rzeszutek Wilk wrote:
> On Mon, May 12, 2014 at 10:45:04AM +0100, Stefano Stabellini wrote:
> > On Mon, 12 May 2014, Oleksandr Dmytryshyn wrote:
> > > On Fri, May 9, 2014 at 5:50 PM, Stefano Stabellini
> > > <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > > > On Mon, 28 Apr 2014, Oleksandr Dmytryshyn wrote:
> > > >> Hi, Stefano.
> > > >>
> > > >> Can You please confirm that this implementation of the
> > > >> xen_swiotlb_dma_mmap is correct?
> > > >
> > > > Hi Oleksandr,
> > > > sorry for the delay in my reply.
> > > >
> > > > The patch is OK. It needs an Ack from the other Xen x86 maintainers too.
> > > > See this thread as reference:
> > > >
> > > > http://marc.info/?l=xen-devel&m=139586321124533
> > > >
> > > >
> > > >> Oleksandr Dmytryshyn | Product Engineering and Development
> > > >> GlobalLogic
> > > >> M +38.067.382.2525
> > > >> www.globallogic.com
> > > >>
> > > >> http://www.globallogic.com/email_disclaimer.txt
> > > >>
> > > >>
> > > >> On Tue, Apr 8, 2014 at 3:08 PM, Oleksandr Dmytryshyn
> > > >> <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote:
> > > >> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > >> >
> > > >> > This function creates userspace mapping for the DMA-coherent memory.
> > > >> >
> > > >> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > >> > Signed-off-by: Oleksandr Dmytryshyn 
> > > >> > <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> > > >> > ---
> > > >> >  arch/arm/xen/mm.c         |  1 +
> > > >> >  drivers/xen/swiotlb-xen.c | 36 ++++++++++++++++++++++++++++++++++++
> > > >> >  include/xen/swiotlb-xen.h |  5 +++++
> > > >> >  3 files changed, 42 insertions(+)
> > > >> >
> > > >> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > > >> > index b0e77de..91408b1 100644
> > > >> > --- a/arch/arm/xen/mm.c
> > > >> > +++ b/arch/arm/xen/mm.c
> > > >> > @@ -48,6 +48,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> > > >> >         .sync_sg_for_device = xen_swiotlb_sync_sg_for_device,
> > > >> >         .map_sg = xen_swiotlb_map_sg_attrs,
> > > >> >         .unmap_sg = xen_swiotlb_unmap_sg_attrs,
> > > >> > +       .mmap = xen_swiotlb_dma_mmap,
> > > >> >         .map_page = xen_swiotlb_map_page,
> > > >> >         .unmap_page = xen_swiotlb_unmap_page,
> > > >> >         .dma_supported = xen_swiotlb_dma_supported,
> > > >> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > >> > index 5403855..acf0c06 100644
> > > >> > --- a/drivers/xen/swiotlb-xen.c
> > > >> > +++ b/drivers/xen/swiotlb-xen.c
> > > >> > @@ -407,6 +407,42 @@ dma_addr_t xen_swiotlb_map_page(struct device 
> > > >> > *dev, struct page *page,
> > > >> >  EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
> > > >> >
> > > >> >  /*
> > > >> > + * Create userspace mapping for the DMA-coherent memory.
> > > >> > + */
> > > >> > +int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct 
> > > >> > *vma,
> > > >> > +                        void *cpu_addr, dma_addr_t dma_addr, size_t 
> > > >> > size,
> > > >> > +                        struct dma_attrs *attrs)
> > > >> > +{
> > > >> > +       int ret = -ENXIO;
> > > >> > +       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(xen_bus_to_phys(dma_addr));
> > > >> > +       unsigned long off = vma->vm_pgoff;
> > > >> > +       pgprot_t prot = vma->vm_page_prot;
> > > >> > +
> > > >> > +       prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
> > > >> > +                           pgprot_writecombine(prot) :
> > > >> > +                           pgprot_dmacoherent(prot);
> > > >> > +
> > > >> > +       vma->vm_page_prot = prot;
> > > >> > +
> > > >> > +       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);
> > > >> > +       }
> > > >> > +
> > > >> > +       return ret;
> > > >> > +}
> > > >> > +EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> > > >> > +
> > > >> > +/*
> > > >> >   * Unmap a single streaming mode DMA translation.  The dma_addr and 
> > > >> > size must
> > > >> >   * match what was provided for in a previous xen_swiotlb_map_page 
> > > >> > call.  All
> > > >> >   * other usages are undefined.
> > > >> > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> > > >> > index 7b64465..930fa94 100644
> > > >> > --- a/include/xen/swiotlb-xen.h
> > > >> > +++ b/include/xen/swiotlb-xen.h
> > > >> > @@ -15,6 +15,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, 
> > > >> > size_t size,
> > > >> >                           void *vaddr, dma_addr_t dma_handle,
> > > >> >                           struct dma_attrs *attrs);
> > > >> >
> > > >> > +extern int
> > > >> > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > > >> > +                    void *cpu_addr, dma_addr_t dma_addr, size_t 
> > > >> > size,
> > > >> > +                    struct dma_attrs *attrs);
> > > >> > +
> > > >> >  extern dma_addr_t xen_swiotlb_map_page(struct device *dev, struct 
> > > >> > page *page,
> > > >> >                                        unsigned long offset, size_t 
> > > >> > size,
> > > >> >                                        enum dma_data_direction dir,
> > > >> > --
> > > >> > 1.8.2.rc2
> > > >> >
> > > >>
> > > 
> > > Hi, Stefano.
> > > 
> > > Thank You for reply.
> > > 
> > > I'll look into that thread.
> > 
> > Actually the thread I linked was for the other maintainers in CC, to
> > better understand why we need this :-)
> 
> I presume you meant me :-)
> 
> Looking back at the thread the concern I had (doing it in the generic ARM)
> is not done anymore. Instead it is done in the SWIOTLB generic implementation.
> 
> Conceptually it looks OK.
> 
> The part that worries me is x86 testing. Looking at the code .. it looks
> it it is not used at all on x86! Well that makes it rather easy.
> 
> So, if you can double-check that it is not used on x86, then it has
> my Acked-by.

Yep, the new function remains completely unused on x86.
However as it is, it doesn't compile on x86 because pgprot_dmacoherent
is not defined.

Oleksandr, would it be OK if we switch to:

vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

like it is done in dma_common_mmap?

pgprot_noncached and pgprot_dmacoherent should end up being the same set
of attributes on ARM, assuming that CONFIG_ARM_DMA_MEM_BUFFERABLE is not
defined. If it is defined, then we set the memory UNCACHED when it could
be BUFFERABLE, loosing performance but it shouldn't cause any troubles.
Thoughts?

Otherwise we could ifdef CONFIG_ARM pgprot_dmacoherent.


+ * Create userspace mapping for the DMA-coherent memory.
+ */
+int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
+                        void *cpu_addr, dma_addr_t dma_addr, size_t size,
+                        struct dma_attrs *attrs)
+{
+       int ret = -ENXIO;
+       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(xen_bus_to_phys(dma_addr));
+       unsigned long off = vma->vm_pgoff;
+
+       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+       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);
+       }
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel