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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


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