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

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

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@epam.com>
> > > 
> > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > ---
> > >  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.

> 
> 
> > > +		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@lists.xen.org
https://lists.xen.org/xen-devel