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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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@eu.citrix.com> 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@globallogic.com> wrote:
> > >> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >> >
> > >> > This function creates userspace mapping for the DMA-coherent memory.
> > >> >
> > >> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >> > Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> > >> > ---
> > >> >  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.

If it is used on x86 - we need to test that code path to make sure it does
what is expected.