| Message ID | alpine.DEB.2.10.1701161705430.2960@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@epam.com> > > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > 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@kernel.org> > > 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@lists.xen.org > https://lists.xen.org/xen-devel
Stefano, About this piece: > > - 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); > } Would it be in drivers/xen/swiotlb-xen.c as you suggested, the whole "if (__generic_dma_ops(dev)->get_sgtable) {}" should be under ifdef. IMO it would be better to avoid ifdefs in drivers/xen/swiotlb-xen.c, but I haven't find out how to do that. Sincerely, Andrii Anisov.
On Wed, 18 Jan 2017, Andrii Anisov wrote: > Stefano, > > About this piece: > > > > > - 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); > > } > > > Would it be in drivers/xen/swiotlb-xen.c as you suggested, the whole > "if (__generic_dma_ops(dev)->get_sgtable) {}" should be under ifdef. > > IMO it would be better to avoid ifdefs in drivers/xen/swiotlb-xen.c, > but I haven't find out how to do that. Yes, I dislike ifdef like everybody else, but sometimes they are unavoidable. But please test this code because I only compile tested it :-)
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); }