diff mbox series

[v2,10/11] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*

Message ID 20200603222247.11681-10-sstabellini@kernel.org (mailing list archive)
State Accepted
Commit df0991c1e41a20daa0f59c68c0318330986895f3
Headers show
Series [v2,01/11] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses | expand

Commit Message

Stefano Stabellini June 3, 2020, 10:22 p.m. UTC 
From: Stefano Stabellini <stefano.stabellini@xilinx.com>

xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
getting called passing dma addresses. On some platforms dma addresses
could be different from physical addresses. Before doing any operations
on these addresses we need to convert them back to physical addresses
using dma_to_phys.

Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
and xen_arch_need_swiotlb.

dma_cache_maint is fixed by the next patch.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Tested-by: Corey Minyard <cminyard@mvista.com>
Tested-by: Roman Shaposhnik <roman@zededa.com>

---
Changes in v2:
- improve commit message
- don't use pfn_valid
---
 arch/arm/xen/mm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig June 8, 2020, 7:12 a.m. UTC | #1 
On Wed, Jun 03, 2020 at 03:22:46PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
> getting called passing dma addresses. On some platforms dma addresses
> could be different from physical addresses. Before doing any operations
> on these addresses we need to convert them back to physical addresses
> using dma_to_phys.
> 
> Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
> and xen_arch_need_swiotlb.
> 
> dma_cache_maint is fixed by the next patch.

The calling conventions because really weird now because
xen_dma_sync_for_{device,cpu} already get both a phys_addr_t and
a dma_addr_t.  

> 
> -	if (pfn_valid(PFN_DOWN(handle)))
> +	if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))

But here we translate the dma address to a phys addr

>  		arch_sync_dma_for_cpu(paddr, size, dir);

While this still uses the passed in paddr.  I think the uses of
addresses in this code really needs a major rethink.
Stefano Stabellini June 9, 2020, 12:38 a.m. UTC | #2 
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:46PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
> > getting called passing dma addresses. On some platforms dma addresses
> > could be different from physical addresses. Before doing any operations
> > on these addresses we need to convert them back to physical addresses
> > using dma_to_phys.
> > 
> > Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
> > and xen_arch_need_swiotlb.
> > 
> > dma_cache_maint is fixed by the next patch.
> 
> The calling conventions because really weird now because
> xen_dma_sync_for_{device,cpu} already get both a phys_addr_t and
> a dma_addr_t.  
> 
> > 
> > -	if (pfn_valid(PFN_DOWN(handle)))
> > +	if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
> 
> But here we translate the dma address to a phys addr
> 
> >  		arch_sync_dma_for_cpu(paddr, size, dir);
> 
> While this still uses the passed in paddr.  I think the uses of
> addresses in this code really needs a major rethink.


Yeah, the pfn_valid check is a bit weird by definition because we are
using it to understand whether the address belong to us or to another
VM. To do the pfn_valid check we need to translate the dma address into
something the CPU understands, hence, the dma_to_phys call.

Why can't we use the already-provided paddr? Because paddr has been
translated twice:
1) from dma to maybe-foreign phys address (could be ours, or another VM)
2) from maybe-foreign address to local (using our local mapping of the foreign page)

In fact, it would be clearer if we had all three addresses as parameters
of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
address (we tend to call it xenbus address, baddr), the local physical
address. Something like:


void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
			  phys_addr_t baddr, phys_addr_t paddr, size_t size,
			  enum dma_data_direction dir)
{
	if (pfn_valid(baddr))
		arch_sync_dma_for_cpu(paddr, size, dir);
	else if (dir != DMA_TO_DEVICE)
		dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
Christoph Hellwig June 9, 2020, 5:38 a.m. UTC | #3 
On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> Yeah, the pfn_valid check is a bit weird by definition because we are
> using it to understand whether the address belong to us or to another
> VM. To do the pfn_valid check we need to translate the dma address into
> something the CPU understands, hence, the dma_to_phys call.
> 
> Why can't we use the already-provided paddr? Because paddr has been
> translated twice:
> 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> 2) from maybe-foreign address to local (using our local mapping of the foreign page)
> 
> In fact, it would be clearer if we had all three addresses as parameters
> of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> address (we tend to call it xenbus address, baddr), the local physical
> address. Something like:

I think instead we should move the arch_sync_dma_for_{device,cpu}
calls from xen_dma_sync_for_{device,cpu} into the callers, as they
are provided by the generic dma-noncoherent.h and optimized out for
coherent architectures like x86.  Then the swiotlb-xen.c code only
need to call dma_cache_maint as the interface (which would have to
grow a better name), which should then only need a single kind of
address.
Christoph Hellwig June 9, 2020, 5:39 a.m. UTC | #4 
On Mon, Jun 08, 2020 at 10:38:02PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> > Yeah, the pfn_valid check is a bit weird by definition because we are
> > using it to understand whether the address belong to us or to another
> > VM. To do the pfn_valid check we need to translate the dma address into
> > something the CPU understands, hence, the dma_to_phys call.
> > 
> > Why can't we use the already-provided paddr? Because paddr has been
> > translated twice:
> > 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> > 2) from maybe-foreign address to local (using our local mapping of the foreign page)
> > 
> > In fact, it would be clearer if we had all three addresses as parameters
> > of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> > address (we tend to call it xenbus address, baddr), the local physical
> > address. Something like:
> 
> I think instead we should move the arch_sync_dma_for_{device,cpu}
> calls from xen_dma_sync_for_{device,cpu} into the callers, as they
> are provided by the generic dma-noncoherent.h and optimized out for
> coherent architectures like x86.  Then the swiotlb-xen.c code only
> need to call dma_cache_maint as the interface (which would have to
> grow a better name), which should then only need a single kind of
> address.

... actually I'd keep the xen_dma_sync_for_{device,cpu} names for the
low-level interface, just move the arch_sync_dma_for_{device,cpu}
calls up.
Stefano Stabellini June 9, 2020, 9:50 p.m. UTC | #5 
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 10:38:02PM -0700, Christoph Hellwig wrote:
> > On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> > > Yeah, the pfn_valid check is a bit weird by definition because we are
> > > using it to understand whether the address belong to us or to another
> > > VM. To do the pfn_valid check we need to translate the dma address into
> > > something the CPU understands, hence, the dma_to_phys call.
> > > 
> > > Why can't we use the already-provided paddr? Because paddr has been
> > > translated twice:
> > > 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> > > 2) from maybe-foreign address to local (using our local mapping of the foreign page)
> > > 
> > > In fact, it would be clearer if we had all three addresses as parameters
> > > of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> > > address (we tend to call it xenbus address, baddr), the local physical
> > > address. Something like:
> > 
> > I think instead we should move the arch_sync_dma_for_{device,cpu}
> > calls from xen_dma_sync_for_{device,cpu} into the callers, as they
> > are provided by the generic dma-noncoherent.h and optimized out for
> > coherent architectures like x86.  Then the swiotlb-xen.c code only
> > need to call dma_cache_maint as the interface (which would have to
> > grow a better name), which should then only need a single kind of
> > address.
> 
> ... actually I'd keep the xen_dma_sync_for_{device,cpu} names for the
> low-level interface, just move the arch_sync_dma_for_{device,cpu}
> calls up.

I can do that.
diff mbox series

Patch

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index f2414ea40a79..bbad712a890d 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 #include <linux/cpu.h>
+#include <linux/dma-direct.h>
 #include <linux/dma-noncoherent.h>
 #include <linux/gfp.h>
 #include <linux/highmem.h>
@@ -75,7 +76,7 @@  void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
 			  phys_addr_t paddr, size_t size,
 			  enum dma_data_direction dir)
 {
-	if (pfn_valid(PFN_DOWN(handle)))
+	if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
 		arch_sync_dma_for_cpu(paddr, size, dir);
 	else if (dir != DMA_TO_DEVICE)
 		dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -85,7 +86,7 @@  void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
 			     phys_addr_t paddr, size_t size,
 			     enum dma_data_direction dir)
 {
-	if (pfn_valid(PFN_DOWN(handle)))
+	if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle))))
 		arch_sync_dma_for_device(paddr, size, dir);
 	else if (dir == DMA_FROM_DEVICE)
 		dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -98,7 +99,7 @@  bool xen_arch_need_swiotlb(struct device *dev,
 			   dma_addr_t dev_addr)
 {
 	unsigned int xen_pfn = XEN_PFN_DOWN(phys);
-	unsigned int bfn = XEN_PFN_DOWN(dev_addr);
+	unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
 
 	/*
 	 * The swiotlb buffer should be used if