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 11:15:50AM -0800, Stefano Stabellini wrote:
> > 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.
> 
> But there are no Kconfig DEBUG - so you may as well just do
> #if 0
> 
> around the code.

Ah! Ops :-)

DEBUG_KERNEL?

> > > > +               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