Re: [Xen-devel] swiotlb-xen question

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 4 Apr 2014, Oleksandr Dmytryshyn wrote:
> On Thu, Apr 3, 2014 at 1:51 PM, Stefano Stabellini
> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Tue, 1 Apr 2014, Oleksandr Dmytryshyn wrote:
> >> Hi to all. I've investigated my issue deeper.
> >>
> >> Currently we use mmap in 2 cases: video-in (camera) and video-out 
> >> (display).
> >>
> >> In case video-out all is working because mmap callback in the video-out 
> >> device
> >> leads to the remap_pfn_range() function (this function doesn't use 
> >> unimplemented
> >> .mmap callback in the swiotlb-xen.c file).
> >>
> >> In case video-out mmap callback in the video-in device leads to the .mmap
> >> callback from the dma ops (unimplemented callback in the swiotlb-xen.c 
> >> file)
> >>
> >> I've investigated kernel code. We have patch from the Stefano Stabellini
> >> (068a6e62 - arm/xen: get_dma_ops: return xen_dma_ops if we are running as
> >> xen_initial_domain)
> >>
> >> Before this patch the function *get_dma_ops() always returned
> >> __generic_dma_ops()
> >> which returned &arm_dma_ops (if archdata.dma_ops is absent). In this case
> >> for arm architecture .mmap callback is arm_dma_mmap() function.
> >>
> >> After this patch for domU all is the same as before this patch. But in case
> >> dom0 kernel uses xen_dma_ops with unimplemented .mmap callback. If some
> >> function tries to use .mmap callback in dom0 then kernel calls generic
> >> function dma_common_mmap() which doesn't work properly.
> >>
> >> May be some mechanism for swiotlb-xen should be implemented for 
> >> unimplemented
> >> callbacks: if callback is not implemented then right architectural callback
> >> should be used (.mmap - arm_dma_mmap() for ARM)?
> >
> > I understand now. Thanks for investingating!
> > I think that we'll have to introduce our own version of .mmap,
> > because dma_to_pfn doesn't work correctly on Xen on ARM.
> > The implementation would be something like (untested):
> >
> > 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;
> > #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(xen_bus_to_phys(dev, dma_addr));
> >         unsigned long off = vma->vm_pgoff;
> >
> >         vma->vm_page_prot = __get_dma_pgprot(attrs, 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);
> >         }
> > #endif  /* CONFIG_MMU */
> >
> >         return ret;
> > }
> >
> > then we can add the function to xen_swiotlb_dma_ops.
> > Would you be up for coming up with a proper patch?
> 
> Hi, Stefano.
> 
> Here is a slightly modified solution from You (I've only replaced a
> __get_dma_pgprot() function with its original implementation):

Looks fine.
Could you resend to the list as a proper patch and with a signed-off-by
line? Otherwise I can do it.


> 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;
> +#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(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);
> + }
> +#endif  /* CONFIG_MMU */
> +
> + 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,
> 
> 
> I've checked it on Xen 4.4 stable + some our architecture specific
> patches on top
> plus Dom0 and DomU Linux Kernel 3.8 on DRA7xxx based board. Camera application
> is working (I can see a captured data from the camera on my board). Can we use
> this implementation of the .mmap callback as the correct solution?
> 
> Oleksandr Dmytryshyn | Product Engineering and Development
> GlobalLogic
> M +38.067.382.2525
> www.globallogic.com
> 
> http://www.globallogic.com/email_disclaimer.txt
> 

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