Re: [Xen-devel] [PATCH v2 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 16 Jan 2017, Stefano Stabellini wrote:
> On Mon, 16 Jan 2017, Andrii Anisov 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>
> > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > ---
> >  arch/arm/xen/mm.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index bd62d94..ff812a2 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -163,6 +163,19 @@ bool xen_arch_need_swiotlb(struct device *dev,
> >             !is_device_dma_coherent(dev));
> >  }
> >  
> > +/*
> > + * Create userspace mapping for the DMA-coherent memory.
> > + */
> > +static int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct 
> > *vma,
> > +                    void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > +                    unsigned long attrs)
> > +{

Only one suggestion more. For this to work correctly, we are assuming
that no foreging pages are involved here, which is a very reasonable
assumption given that mmap should be called on memory returned by
dma_alloc_coherent.  Please add an in-code comment here so that we'll
remember.


> > +   if (__generic_dma_ops(dev)->mmap)
> > +           return __generic_dma_ops(dev)->mmap(dev, vma, cpu_addr, 
> > dma_addr, size, attrs);
> > +
> > +   return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> > +}
> > +
> >  int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> >                              unsigned int address_bits,
> >                              dma_addr_t *dma_handle)
> > @@ -198,6 +211,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> >     .unmap_page = xen_swiotlb_unmap_page,
> >     .dma_supported = xen_swiotlb_dma_supported,
> >     .set_dma_mask = xen_swiotlb_set_dma_mask,
> > +   .mmap = xen_swiotlb_dma_mmap,
> >  };
> >  
> >  int __init xen_mm_init(void)
> 
> The patch should work fine and looks OK. It is better written like this,
> compared to the previous versions that reimplemented dma_common_mmap. I
> like the fact that we are reusing the arm specific generic mmap
> functions via __generic_dma_ops.
> 
> For consistency, I would prefer to have xen_swiotlb_dma_mmap in
> drivers/xen/swiotlb-xen.c, even if it needs to be #ifdef'ed CONFIG_ARM
> (at least the __generic_dma_ops calls need to be #ifdef'ed).
> 
> Konrad, what do you think?
> 

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