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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Jan 16, 2017 at 05:09:24PM -0800, Stefano Stabellini wrote:
> On Mon, 16 Jan 2017, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > 
> > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> 
> Thanks for the patch!
> 
> 
> >  arch/arm/xen/mm.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index ff812a2..dc83a35 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -176,6 +176,16 @@ static int xen_swiotlb_dma_mmap(struct device *dev, 
> > struct vm_area_struct *vma,
> >     return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> >  }
> 
> As for the other patch, I would move xen_swiotlb_get_sgtable to
> drivers/xen/swiotlb-xen.c, if Konrad agrees.

That is fine.
> 
> 
> > +static 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 (__generic_dma_ops(dev)->get_sgtable)
> > +           return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr, 
> > handle,
> > +                                                            size, attrs);
> > +   return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> > +}
> > +
> 
> __generic_dma_ops(dev)->get_sgtable on ARM is implemented by
> arm_dma_get_sgtable, which doesn't work on foreign pages (pages for
> which bfn != pfn).
> 
> If get_sgtable is guaranteed to be always called passing references to
> pages previously allocated with dma_alloc_coherent, then we don't have
> any issues, because those can't be foreign pages. I suggest we add an
> in-code comment to explain why this is safe, as for the previous patch.
> I think this is the case, but I am not 100% sure.
> 
> On the other hand, if this function can be called passing as parameters
> cpu_addr and handle that could potentially refer to a foreign page, then
> we have a problem. On ARM, virt_to_phys doesn't work on some pages, in
> fact that is the reason why ARM has its own separate get_sgtable
> implementation (arm_dma_get_sgtable). But with Xen foreign pages,
> dma_to_pfn doesn't work either, because we have no way of finding out
> the pfn address corresponding to the mfn of the foreign page. Both
> arm_dma_get_sgtable and dma_common_get_sgtable wouldn't work. I have no
> solution to this problem, but maybe we could add a check like the
> following (also to the previous patch?). I haven't tested it, but I
> think it should work as long as page_is_ram is returns the correct value
> for the handle parameter.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index dc83a35..cd0441c 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -18,6 +18,7 @@
>  #include <xen/page.h>
>  #include <xen/swiotlb-xen.h>
>  
> +#include <asm/dma-mapping.h>
>  #include <asm/cacheflush.h>
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/interface.h>
> @@ -180,9 +181,18 @@ static 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 (__generic_dma_ops(dev)->get_sgtable)
> +
> +     if (__generic_dma_ops(dev)->get_sgtable) {
> +             /* We can't handle foreign pages here. */
> +#ifdef CONFIG_ARM
> +             unsigned long bfn = dma_to_pfn(dev, handle);
> +#else
> +             unsigned long bfn = handle >> PAGE_SHIFT;
> +#endif
> +             BUG_ON (!page_is_ram(bfn));
>               return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr, 
> handle,
>                                                                size, attrs);
> +     }
>       return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
>  }
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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