Re: [Xen-devel] [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > 
> > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> >  arch/arm/xen/mm.c         |  1 +
> >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
> >  include/xen/swiotlb-xen.h |  6 ++++++
> >  3 files changed, 29 insertions(+)
> > 
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index cd1684e..76ea48a 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> >     .dma_supported = xen_swiotlb_dma_supported,
> >     .set_dma_mask = xen_swiotlb_set_dma_mask,
> >     .mmap = xen_swiotlb_dma_mmap,
> > +   .get_sgtable = xen_swiotlb_get_sgtable,
> >  };
> >  
> >  int __init xen_mm_init(void)
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 8ac36b4..a809d43 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct 
> > vm_area_struct *vma,
> >     return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> >  }
> >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> > +
> > +/*
> > + * Following function should be called with the local pages only.
> 
> What does 'local pages' mean?
> 
> > + */
> > +int
> > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > +                   void *cpu_addr, dma_addr_t handle, size_t size,
> > +                   unsigned long attrs)
> > +{
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +   if (__generic_dma_ops(dev)->get_sgtable) {
> > +#ifdef DEBUG
> > +           unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
> > +           BUG_ON (!page_is_ram(bfn));
> > +#endif
> 
> Could you remove the above please?

I asked him to add it (see
http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
this function cannot work on foreign pages. From the commit message
(d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
to be called on buffers returned by dma_alloc_coherent, thus it should
be safe.  However, if that's not the case in any of the drivers, it
would lead to memory corruptions. The two lines under ifdef DEBUG are an
nice way to catch this kind of errors. However, given that they cost a
few cpu cycles more than necessary, I wouldn't enable them in
production, hence the ifdef DEBUG.


> > +           return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
> > +                                                      handle, size, attrs);
> > +   }
> > +#endif
> > +   return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> > +}
> > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
> > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> > index 5c8f4c8..c554c23 100644
> > --- a/include/xen/swiotlb-xen.h
> > +++ b/include/xen/swiotlb-xen.h
> > @@ -60,4 +60,10 @@ 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,
> >                     unsigned long attrs);
> > +
> > +extern int
> > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > +                   void *cpu_addr, dma_addr_t handle, size_t size,
> > +                   unsigned long attrs);
> 
> And perhaps fix this to be aligned properly?
> > +
> >  #endif /* __LINUX_SWIOTLB_XEN_H */
> > -- 
> > 2.7.4
> > 
> 

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