[PATCH V2 0/7] swiotlb: changes for powerpc/highmem

[PATCH V2 0/7] swiotlb: changes for powerpc/highmem

[Becky Bruce]

This is v2 of a series of fairly minor patches that get swiotlb
working on 32-bit powerpc systems with HIGHMEM, plus some cleanup
of the outdated comments in the code. I've made a couple of things
weak that ppc needs to override, and have changed the prototypes
for a couple of functions to include the hwdev pointer, which
we need to ppc to convert bus addresses to and from phys/virt
addresses. I've also fixed a build warning I've been seeing on
ppc.

In response to commentary on the previous series, I've also
refactored the code a bit, altough I did this slightly
differently than was suggested because I noticed we could use the
new helper function in 2 places instead of one. I've reformatted
a bit of code based on commentary as well.

I have not tested this in any way on any non-ppc platforms,
so commentary/testing from x86/ia64 folks is, once again,
greatly appreciated.

I'm going to be offline for the next week, but will respond to
commentary as soon as I return.

Cheers,
Becky

diffstat:
arch/x86/kernel/pci-swiotlb.c |    2 +-
include/linux/swiotlb.h       |    3 +-
lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------
3 files changed, 64 insertions(+), 56 deletions(-)

[PATCH 1/7] swiotlb: comment corrections (no code changes)

[Becky Bruce]

swiotlb_map/unmap_single are now swiotlb_map/unmap_page;
trivially change all the comments to reference new names.
Also, there were some comments that should have been
referring to just plain old map_single, not swiotlb_map_single;
fix those as well. Also change a use of the word "pointer", when
what is referred to is actually a dma/physical address.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 32e2bd3..f59cf30 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -60,8 +60,8 @@ enum dma_sync_target {
 int swiotlb_force;
 
 /*
- * Used to do a quick range check in swiotlb_unmap_single and
- * swiotlb_sync_single_*, to see if the memory was in fact allocated by this
+ * Used to do a quick range check in unmap_single and
+ * sync_single_*, to see if the memory was in fact allocated by this
  * API.
  */
 static char *io_tlb_start, *io_tlb_end;
@@ -560,7 +560,6 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 				   size)) {
 		/*
 		 * The allocated memory isn't reachable by the device.
-		 * Fall back on swiotlb_map_single().
 		 */
 		free_pages((unsigned long) ret, order);
 		ret = NULL;
@@ -568,9 +567,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	if (!ret) {
 		/*
 		 * We are either out of memory or the device can't DMA
-		 * to GFP_DMA memory; fall back on
-		 * swiotlb_map_single(), which will grab memory from
-		 * the lowest available address range.
+		 * to GFP_DMA memory; fall back on map_single(), which
+		 * will grab memory from the lowest available address range.
 		 */
 		ret = map_single(hwdev, 0, size, DMA_FROM_DEVICE);
 		if (!ret)
@@ -634,7 +632,7 @@ swiotlb_full(struct device *dev, size_t size, int dir, int do_panic)
  * physical address to use is returned.
  *
  * Once the device is given the dma address, the device owns this memory until
- * either swiotlb_unmap_single or swiotlb_dma_sync_single is performed.
+ * either swiotlb_unmap_page or swiotlb_dma_sync_single is performed.
  */
 dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 			    unsigned long offset, size_t size,
@@ -648,7 +646,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 
 	BUG_ON(dir == DMA_NONE);
 	/*
-	 * If the pointer passed in happens to be in the device's DMA window,
+	 * If the address happens to be in the device's DMA window,
 	 * we can safely return the device addr and not worry about bounce
 	 * buffering it.
 	 */
@@ -679,7 +677,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
 
 /*
  * Unmap a single streaming mode DMA translation.  The dma_addr and size must
- * match what was provided for in a previous swiotlb_map_single call.  All
+ * match what was provided for in a previous swiotlb_map_page call.  All
  * other usages are undefined.
  *
  * After this call, reads by the cpu to the buffer are guaranteed to see
@@ -703,7 +701,7 @@ EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
  * Make physical memory consistent for a single streaming mode DMA translation
  * after a transfer.
  *
- * If you perform a swiotlb_map_single() but wish to interrogate the buffer
+ * If you perform a swiotlb_map_page() but wish to interrogate the buffer
  * using the cpu, yet do not wish to teardown the dma mapping, you must
  * call this function before doing so.  At the next point you give the dma
  * address back to the card, you must first perform a
@@ -777,7 +775,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device);
 
 /*
  * Map a set of buffers described by scatterlist in streaming mode for DMA.
- * This is the scatter-gather version of the above swiotlb_map_single
+ * This is the scatter-gather version of the above swiotlb_map_page
  * interface.  Here the scatter gather list elements are each tagged with the
  * appropriate dma address and length.  They are obtained via
  * sg_dma_{address,length}(SG).
@@ -788,7 +786,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device);
  *       The routine returns the number of addr/length pairs actually
  *       used, at most nents.
  *
- * Device ownership issues as mentioned above for swiotlb_map_single are the
+ * Device ownership issues as mentioned above for swiotlb_map_page are the
  * same here.
  */
 int
@@ -836,7 +834,7 @@ EXPORT_SYMBOL(swiotlb_map_sg);
 
 /*
  * Unmap a set of streaming mode DMA translations.  Again, cpu read rules
- * concerning calls here are the same as for swiotlb_unmap_single() above.
+ * concerning calls here are the same as for swiotlb_unmap_page() above.
  */
 void
 swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
-- 
1.5.6.6

[PATCH 2/7] swiotlb: fix compile warning

[Becky Bruce]

Squash a build warning seen on 32-bit powerpc caused by calling min()
with 2 different types. Use min_t() instead

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index f59cf30..fa62498 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size,
 		unsigned long flags;
 
 		while (size) {
-			sz = min(PAGE_SIZE - offset, size);
+			sz = min_t(size_t, PAGE_SIZE - offset, size);
 
 			local_irq_save(flags);
 			buffer = kmap_atomic(pfn_to_page(pfn),
-- 
1.5.6.6

[PATCH 3/7] swiotlb: map_page fix for highmem systems

[Becky Bruce]

The current code calls virt_to_phys() on address that might
be in highmem, which is bad.  This wasn't needed, anyway, because
we already have the physical address we need. Get rid of the
now-unused virtual address as well.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index fa62498..eb6dbbd 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -640,7 +640,6 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 			    struct dma_attrs *attrs)
 {
 	phys_addr_t phys = page_to_phys(page) + offset;
-	void *ptr = page_address(page) + offset;
 	dma_addr_t dev_addr = swiotlb_phys_to_bus(dev, phys);
 	void *map;
 
@@ -651,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 * buffering it.
 	 */
 	if (!address_needs_mapping(dev, dev_addr, size) &&
-	    !range_needs_mapping(virt_to_phys(ptr), size))
+	    !range_needs_mapping(phys, size))
 		return dev_addr;
 
 	/*
-- 
1.5.6.6

[PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Becky Bruce]

Some architectures require additional checking to determine
if a device can dma to an address and need to provide their
own address_needs_mapping..

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index eb6dbbd..af2ec25 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -145,6 +145,12 @@ static void *swiotlb_bus_to_virt(dma_addr_t address)
 	return phys_to_virt(swiotlb_bus_to_phys(address));
 }
 
+int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
+					       dma_addr_t addr, size_t size)
+{
+	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+}
+
 int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size)
 {
 	return 0;
@@ -309,10 +315,10 @@ cleanup1:
 	return -ENOMEM;
 }
 
-static int
+static inline int
 address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size)
 {
-	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+	return swiotlb_arch_address_needs_mapping(hwdev, addr, size);
 }
 
 static inline int range_needs_mapping(phys_addr_t paddr, size_t size)
-- 
1.5.6.6

[PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Becky Bruce]

This mirrors the current swiotlb_sync_single() setup
where the swiotlb_unmap_single() function is static to this
file and contains the logic required to determine if we need
to call actual sync_single.  Previously, swiotlb_unmap_page
and swiotlb_unmap_sg were duplicating very similar code.
The duplicated code has also been reformatted for
readability.

Note that the swiotlb_unmap_sg code was previously doing
a complicated comparison to determine if an addresses needed
to be unmapped where a simple is_swiotlb_buffer() call
would have sufficed.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index af2ec25..602315b 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -688,17 +688,30 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
  * After this call, reads by the cpu to the buffer are guaranteed to see
  * whatever the device wrote there.
  */
-void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
-			size_t size, enum dma_data_direction dir,
-			struct dma_attrs *attrs)
+static void
+swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
+		     size_t size, int dir)
 {
 	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
+
+	if (is_swiotlb_buffer(dma_addr)) {
 		unmap_single(hwdev, dma_addr, size, dir);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+		return;
+	}
+
+	if (dir != DMA_FROM_DEVICE)
+		return;
+
+	dma_mark_clean(dma_addr, size);
+}
+
+void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
+			size_t size, enum dma_data_direction dir,
+			struct dma_attrs *attrs)
+{
+	swiotlb_unmap_single(hwdev, dev_addr, size, dir);
 }
 EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
 
@@ -850,13 +863,10 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
 
 	BUG_ON(dir == DMA_NONE);
 
-	for_each_sg(sgl, sg, nelems, i) {
-		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
-			unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
-				     sg->dma_length, dir);
-		else if (dir == DMA_FROM_DEVICE)
-			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
-	}
+	for_each_sg(sgl, sg, nelems, i)
+		swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length,
+				     dir);
+
 }
 EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
 
-- 
1.5.6.6

[PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code

[Becky Bruce]

Right now both swiotlb_sync_single_range and swiotlb_sync_sg
were duplicating the code in swiotlb_sync_single.  Just call it
instead.  Also rearrange the sync_single code for readability.

Note that the swiotlb_sync_sg code was previously doing
a complicated comparison to determine if an addresses needed
to be unmapped where a simple is_swiotlb_buffer() call
would have sufficed.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 602315b..c0f185a 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -732,10 +732,16 @@ swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
 	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
+
+	if (is_swiotlb_buffer(dma_addr)) {
 		sync_single(hwdev, dma_addr, size, dir, target);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+		return;
+	}
+
+	if (dir != DMA_FROM_DEVICE)
+		return;
+
+	dma_mark_clean(dma_addr, size);
 }
 
 void
@@ -762,13 +768,7 @@ swiotlb_sync_single_range(struct device *hwdev, dma_addr_t dev_addr,
 			  unsigned long offset, size_t size,
 			  int dir, int target)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr) + offset;
-
-	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
-		sync_single(hwdev, dma_addr, size, dir, target);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+	swiotlb_sync_single(hwdev, dev_addr + offset, size, dir, target);
 }
 
 void
@@ -892,15 +892,9 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
 	struct scatterlist *sg;
 	int i;
 
-	BUG_ON(dir == DMA_NONE);
-
-	for_each_sg(sgl, sg, nelems, i) {
-		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
-			sync_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
+	for_each_sg(sgl, sg, nelems, i)
+		swiotlb_sync_single(hwdev, sg->dma_address,
 				    sg->dma_length, dir, target);
-		else if (dir == DMA_FROM_DEVICE)
-			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
-	}
 }
 
 void
-- 
1.5.6.6

[PATCH 7/7] swiotlb: Change swiotlb_bus_to[phys,virt] prototypes

[Becky Bruce]

Add a hwdev argument that is needed on some architectures
in order to access a per-device offset that is taken into
account when producing a physical address (also needed to
get from bus address to virtual address because the physical
address is an intermediate step).

Also make swiotlb_bus_to_virt weak so architectures can
override it.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    3 ++-
 lib/swiotlb.c                 |   10 +++++-----
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index 34f12e9..887388a 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -28,7 +28,7 @@ dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 	return paddr;
 }
 
-phys_addr_t swiotlb_bus_to_phys(dma_addr_t baddr)
+phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
 {
 	return baddr;
 }
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index ac9ff54..cb1a663 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -29,7 +29,8 @@ extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
 
 extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
 				      phys_addr_t address);
-extern phys_addr_t swiotlb_bus_to_phys(dma_addr_t address);
+extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
+				       dma_addr_t address);
 
 extern int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size);
 
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index c0f185a..d006f6d 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -129,7 +129,7 @@ dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 	return paddr;
 }
 
-phys_addr_t __weak swiotlb_bus_to_phys(dma_addr_t baddr)
+phys_addr_t __weak swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
 {
 	return baddr;
 }
@@ -140,9 +140,9 @@ static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 	return swiotlb_phys_to_bus(hwdev, virt_to_phys(address));
 }
 
-static void *swiotlb_bus_to_virt(dma_addr_t address)
+void * __weak swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
 {
-	return phys_to_virt(swiotlb_bus_to_phys(address));
+	return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
 }
 
 int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
@@ -692,7 +692,7 @@ static void
 swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
 		     size_t size, int dir)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 
@@ -729,7 +729,7 @@ static void
 swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
 		    size_t size, int dir, int target)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 
-- 
1.5.6.6

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Fri,  3 Apr 2009 20:56:47 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> This mirrors the current swiotlb_sync_single() setup
> where the swiotlb_unmap_single() function is static to this
> file and contains the logic required to determine if we need
> to call actual sync_single.  Previously, swiotlb_unmap_page
> and swiotlb_unmap_sg were duplicating very similar code.
> The duplicated code has also been reformatted for
> readability.
> 
> Note that the swiotlb_unmap_sg code was previously doing
> a complicated comparison to determine if an addresses needed
> to be unmapped where a simple is_swiotlb_buffer() call
> would have sufficed.
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>  1 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index af2ec25..602315b 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c

I don't think 'swiotlb_unmap_single' name is appropriate.

swiotlb_unmap_single sounds like an exported function that IOMMUs can
use (and it was) however it should not be.

Re: [PATCH V2 0/7] swiotlb: changes for powerpc/highmem

[FUJITA Tomonori]

On Fri,  3 Apr 2009 20:56:42 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> This is v2 of a series of fairly minor patches that get swiotlb
> working on 32-bit powerpc systems with HIGHMEM, plus some cleanup
> of the outdated comments in the code. I've made a couple of things
> weak that ppc needs to override, and have changed the prototypes
> for a couple of functions to include the hwdev pointer, which
> we need to ppc to convert bus addresses to and from phys/virt
> addresses. I've also fixed a build warning I've been seeing on
> ppc.
> 
> In response to commentary on the previous series, I've also
> refactored the code a bit, altough I did this slightly
> differently than was suggested because I noticed we could use the
> new helper function in 2 places instead of one. I've reformatted
> a bit of code based on commentary as well.
> 
> I have not tested this in any way on any non-ppc platforms,
> so commentary/testing from x86/ia64 folks is, once again,
> greatly appreciated.
> 
> I'm going to be offline for the next week, but will respond to
> commentary as soon as I return.
> 
> Cheers,
> Becky
> 
> diffstat:
> arch/x86/kernel/pci-swiotlb.c |    2 +-
> include/linux/swiotlb.h       |    3 +-
> lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------
> 3 files changed, 64 insertions(+), 56 deletions(-)

I have a minor comment on 5/7 but the rest looks fine to me.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:

> On Fri,  3 Apr 2009 20:56:47 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>
>> This mirrors the current swiotlb_sync_single() setup
>> where the swiotlb_unmap_single() function is static to this
>> file and contains the logic required to determine if we need
>> to call actual sync_single.  Previously, swiotlb_unmap_page
>> and swiotlb_unmap_sg were duplicating very similar code.
>> The duplicated code has also been reformatted for
>> readability.
>>
>> Note that the swiotlb_unmap_sg code was previously doing
>> a complicated comparison to determine if an addresses needed
>> to be unmapped where a simple is_swiotlb_buffer() call
>> would have sufficed.
>>
>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>> ---
>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>> index af2ec25..602315b 100644
>> --- a/lib/swiotlb.c
>> +++ b/lib/swiotlb.c
>
> I don't think 'swiotlb_unmap_single' name is appropriate.
>
> swiotlb_unmap_single sounds like an exported function that IOMMUs can
> use (and it was) however it should not be.

What do you suggest we call it?  __swiotlb_unmap_single.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 01:34:44 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> 
> > On Fri,  3 Apr 2009 20:56:47 -0500
> > Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >
> >> This mirrors the current swiotlb_sync_single() setup
> >> where the swiotlb_unmap_single() function is static to this
> >> file and contains the logic required to determine if we need
> >> to call actual sync_single.  Previously, swiotlb_unmap_page
> >> and swiotlb_unmap_sg were duplicating very similar code.
> >> The duplicated code has also been reformatted for
> >> readability.
> >>
> >> Note that the swiotlb_unmap_sg code was previously doing
> >> a complicated comparison to determine if an addresses needed
> >> to be unmapped where a simple is_swiotlb_buffer() call
> >> would have sufficed.
> >>
> >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >> ---
> >> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >> index af2ec25..602315b 100644
> >> --- a/lib/swiotlb.c
> >> +++ b/lib/swiotlb.c
> >
> > I don't think 'swiotlb_unmap_single' name is appropriate.
> >
> > swiotlb_unmap_single sounds like an exported function that IOMMUs can
> > use (and it was) however it should not be.
> 
> What do you suggest we call it?  __swiotlb_unmap_single.

I think that __swiotlb_unmap_single is better because the name implies
that it's an internal function. It's fine by me.

If it is odd that __swiotlb_unmap_single() is just a wrapper function
of unmap_single(), which does the real job to unmap a dma mapping, it
might be another possible option to rename unmap_single to
do_unamp_single and use unmap_single.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 01:34:44 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>
>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>
>>>> This mirrors the current swiotlb_sync_single() setup
>>>> where the swiotlb_unmap_single() function is static to this
>>>> file and contains the logic required to determine if we need
>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>> The duplicated code has also been reformatted for
>>>> readability.
>>>>
>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>> a complicated comparison to determine if an addresses needed
>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>> would have sufficed.
>>>>
>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>> ---
>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>> index af2ec25..602315b 100644
>>>> --- a/lib/swiotlb.c
>>>> +++ b/lib/swiotlb.c
>>>
>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>
>>> swiotlb_unmap_single sounds like an exported function that IOMMUs  
>>> can
>>> use (and it was) however it should not be.
>>
>> What do you suggest we call it?  __swiotlb_unmap_single.
>
> I think that __swiotlb_unmap_single is better because the name implies
> that it's an internal function. It's fine by me.
>
> If it is odd that __swiotlb_unmap_single() is just a wrapper function
> of unmap_single(), which does the real job to unmap a dma mapping, it
> might be another possible option to rename unmap_single to
> do_unamp_single and use unmap_single.

I think you lost me here.  I'd prefer to just use  
__swiotlb_unmap_single at this point and get this code into the tree  
and work on such renaming after the fact (if that's ok).

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 10:32:20 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 01:34:44 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>
> >>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>
> >>>> This mirrors the current swiotlb_sync_single() setup
> >>>> where the swiotlb_unmap_single() function is static to this
> >>>> file and contains the logic required to determine if we need
> >>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>> The duplicated code has also been reformatted for
> >>>> readability.
> >>>>
> >>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>> a complicated comparison to determine if an addresses needed
> >>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>> would have sufficed.
> >>>>
> >>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>> ---
> >>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>> index af2ec25..602315b 100644
> >>>> --- a/lib/swiotlb.c
> >>>> +++ b/lib/swiotlb.c
> >>>
> >>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>
> >>> swiotlb_unmap_single sounds like an exported function that IOMMUs  
> >>> can
> >>> use (and it was) however it should not be.
> >>
> >> What do you suggest we call it?  __swiotlb_unmap_single.
> >
> > I think that __swiotlb_unmap_single is better because the name implies
> > that it's an internal function. It's fine by me.
> >
> > If it is odd that __swiotlb_unmap_single() is just a wrapper function
> > of unmap_single(), which does the real job to unmap a dma mapping, it
> > might be another possible option to rename unmap_single to
> > do_unamp_single and use unmap_single.
> 
> I think you lost me here.  I'd prefer to just use  
> __swiotlb_unmap_single at this point and get this code into the tree  
> and work on such renaming after the fact (if that's ok).

If you are rushing to merge this right now, the original patchset is
fine by me (I thought that you missed this merge window). I'll rename
it later.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 10:32:20 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
>>
>>> On Tue, 7 Apr 2009 01:34:44 -0500
>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>
>>>>
>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>>>
>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>>>
>>>>>> This mirrors the current swiotlb_sync_single() setup
>>>>>> where the swiotlb_unmap_single() function is static to this
>>>>>> file and contains the logic required to determine if we need
>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>>>> The duplicated code has also been reformatted for
>>>>>> readability.
>>>>>>
>>>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>>>> a complicated comparison to determine if an addresses needed
>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>>>> would have sufficed.
>>>>>>
>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>>>> ---
>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>>> index af2ec25..602315b 100644
>>>>>> --- a/lib/swiotlb.c
>>>>>> +++ b/lib/swiotlb.c
>>>>>
>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>>>
>>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs
>>>>> can
>>>>> use (and it was) however it should not be.
>>>>
>>>> What do you suggest we call it?  __swiotlb_unmap_single.
>>>
>>> I think that __swiotlb_unmap_single is better because the name  
>>> implies
>>> that it's an internal function. It's fine by me.
>>>
>>> If it is odd that __swiotlb_unmap_single() is just a wrapper  
>>> function
>>> of unmap_single(), which does the real job to unmap a dma mapping,  
>>> it
>>> might be another possible option to rename unmap_single to
>>> do_unamp_single and use unmap_single.
>>
>> I think you lost me here.  I'd prefer to just use
>> __swiotlb_unmap_single at this point and get this code into the tree
>> and work on such renaming after the fact (if that's ok).
>
> If you are rushing to merge this right now, the original patchset is
> fine by me (I thought that you missed this merge window). I'll rename
> it later.

We probably did, but one can never tell with these things.  It seemed  
like Ingo merged and pushed some swiotlb changes late in the game for . 
29

I'm still not clear on what you are suggesting... "rename unmap_single  
to do_unamp_single and use unmap_single".

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 11:50:56 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 10:32:20 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> >>
> >>> On Tue, 7 Apr 2009 01:34:44 -0500
> >>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>
> >>>>
> >>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>>>
> >>>>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>>>
> >>>>>> This mirrors the current swiotlb_sync_single() setup
> >>>>>> where the swiotlb_unmap_single() function is static to this
> >>>>>> file and contains the logic required to determine if we need
> >>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>>>> The duplicated code has also been reformatted for
> >>>>>> readability.
> >>>>>>
> >>>>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>>>> a complicated comparison to determine if an addresses needed
> >>>>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>>>> would have sufficed.
> >>>>>>
> >>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>>>> ---
> >>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>>>> index af2ec25..602315b 100644
> >>>>>> --- a/lib/swiotlb.c
> >>>>>> +++ b/lib/swiotlb.c
> >>>>>
> >>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>>>
> >>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs
> >>>>> can
> >>>>> use (and it was) however it should not be.
> >>>>
> >>>> What do you suggest we call it?  __swiotlb_unmap_single.
> >>>
> >>> I think that __swiotlb_unmap_single is better because the name  
> >>> implies
> >>> that it's an internal function. It's fine by me.
> >>>
> >>> If it is odd that __swiotlb_unmap_single() is just a wrapper  
> >>> function
> >>> of unmap_single(), which does the real job to unmap a dma mapping,  
> >>> it
> >>> might be another possible option to rename unmap_single to
> >>> do_unamp_single and use unmap_single.
> >>
> >> I think you lost me here.  I'd prefer to just use
> >> __swiotlb_unmap_single at this point and get this code into the tree
> >> and work on such renaming after the fact (if that's ok).
> >
> > If you are rushing to merge this right now, the original patchset is
> > fine by me (I thought that you missed this merge window). I'll rename
> > it later.
> 
> We probably did, but one can never tell with these things.  It seemed  
> like Ingo merged and pushed some swiotlb changes late in the game for . 
> 29

Well, merging patches that have not been tested linux-next late is
what we should not do, I guess. I like to see Becky's patch in 2.6.30
because I have some swiotlb changes for 2.6.31 though.


> I'm still not clear on what you are suggesting... "rename unmap_single  
> to do_unamp_single and use unmap_single".

This can be applied after Becky's patchset.


diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 1c86553..bffe6d7 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -482,7 +482,7 @@ found:
  * dma_addr is the kernel virtual address of the bounce buffer to unmap.
  */
 static void
-unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir)
+do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir)
 {
 	unsigned long flags;
 	int i, count, nslots = ALIGN(size, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT;
@@ -591,7 +591,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 		       (unsigned long long)dev_addr);
 
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
-		unmap_single(hwdev, ret, size, DMA_TO_DEVICE);
+		do_unmap_single(hwdev, ret, size, DMA_TO_DEVICE);
 		return NULL;
 	}
 	*dma_handle = dev_addr;
@@ -608,7 +608,7 @@ swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
 		free_pages((unsigned long) vaddr, get_order(size));
 	else
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
-		unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE);
+		do_unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE);
 }
 EXPORT_SYMBOL(swiotlb_free_coherent);
 
@@ -688,16 +688,15 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
  * After this call, reads by the cpu to the buffer are guaranteed to see
  * whatever the device wrote there.
  */
-static void
-swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
-		     size_t size, int dir)
+static void unmap_single(struct device *hwdev, dma_addr_t dev_addr,
+			 size_t size, int dir)
 {
 	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 
 	if (is_swiotlb_buffer(dma_addr)) {
-		unmap_single(hwdev, dma_addr, size, dir);
+		do_unmap_single(hwdev, dma_addr, size, dir);
 		return;
 	}
 
@@ -711,7 +710,7 @@ void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
 			size_t size, enum dma_data_direction dir,
 			struct dma_attrs *attrs)
 {
-	swiotlb_unmap_single(hwdev, dev_addr, size, dir);
+	unmap_single(hwdev, dev_addr, size, dir);
 }
 EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
 
@@ -864,8 +863,7 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
 	BUG_ON(dir == DMA_NONE);
 
 	for_each_sg(sgl, sg, nelems, i)
-		swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length,
-				     dir);
+		unmap_single(hwdev, sg->dma_address, sg->dma_length, dir);
 
 }
 EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 11:50:56 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
>>
>>> On Tue, 7 Apr 2009 10:32:20 -0500
>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>
>>>>
>>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
>>>>
>>>>> On Tue, 7 Apr 2009 01:34:44 -0500
>>>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>>>
>>>>>>
>>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>>>>>
>>>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>>>>>
>>>>>>>> This mirrors the current swiotlb_sync_single() setup
>>>>>>>> where the swiotlb_unmap_single() function is static to this
>>>>>>>> file and contains the logic required to determine if we need
>>>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>>>>>> The duplicated code has also been reformatted for
>>>>>>>> readability.
>>>>>>>>
>>>>>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>>>>>> a complicated comparison to determine if an addresses needed
>>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>>>>>> would have sufficed.
>>>>>>>>
>>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>>>>>> ---
>>>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>>>>> index af2ec25..602315b 100644
>>>>>>>> --- a/lib/swiotlb.c
>>>>>>>> +++ b/lib/swiotlb.c
>>>>>>>
>>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>>>>>
>>>>>>> swiotlb_unmap_single sounds like an exported function that  
>>>>>>> IOMMUs
>>>>>>> can
>>>>>>> use (and it was) however it should not be.
>>>>>>
>>>>>> What do you suggest we call it?  __swiotlb_unmap_single.
>>>>>
>>>>> I think that __swiotlb_unmap_single is better because the name
>>>>> implies
>>>>> that it's an internal function. It's fine by me.
>>>>>
>>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper
>>>>> function
>>>>> of unmap_single(), which does the real job to unmap a dma mapping,
>>>>> it
>>>>> might be another possible option to rename unmap_single to
>>>>> do_unamp_single and use unmap_single.
>>>>
>>>> I think you lost me here.  I'd prefer to just use
>>>> __swiotlb_unmap_single at this point and get this code into the  
>>>> tree
>>>> and work on such renaming after the fact (if that's ok).
>>>
>>> If you are rushing to merge this right now, the original patchset is
>>> fine by me (I thought that you missed this merge window). I'll  
>>> rename
>>> it later.
>>
>> We probably did, but one can never tell with these things.  It seemed
>> like Ingo merged and pushed some swiotlb changes late in the game  
>> for .
>> 29
>
> Well, merging patches that have not been tested linux-next late is
> what we should not do, I guess. I like to see Becky's patch in 2.6.30
> because I have some swiotlb changes for 2.6.31 though.

Same here.  It makes it easier for us to work on the powerpc arch  
specific changes for .31 if we can get these into .30.  What are you  
looking at for .31?

Ingo, any comments on that?

>> I'm still not clear on what you are suggesting... "rename  
>> unmap_single
>> to do_unamp_single and use unmap_single".
>
> This can be applied after Becky's patchset.

thanks. I'll merge this into her patch set and repost it.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 12:32:12 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 11:50:56 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
> >>
> >>> On Tue, 7 Apr 2009 10:32:20 -0500
> >>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>
> >>>>
> >>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> >>>>
> >>>>> On Tue, 7 Apr 2009 01:34:44 -0500
> >>>>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>>>
> >>>>>>
> >>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>>>>>
> >>>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>>>>>
> >>>>>>>> This mirrors the current swiotlb_sync_single() setup
> >>>>>>>> where the swiotlb_unmap_single() function is static to this
> >>>>>>>> file and contains the logic required to determine if we need
> >>>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>>>>>> The duplicated code has also been reformatted for
> >>>>>>>> readability.
> >>>>>>>>
> >>>>>>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>>>>>> a complicated comparison to determine if an addresses needed
> >>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>>>>>> would have sufficed.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>>>>>> ---
> >>>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>>>>>> index af2ec25..602315b 100644
> >>>>>>>> --- a/lib/swiotlb.c
> >>>>>>>> +++ b/lib/swiotlb.c
> >>>>>>>
> >>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>>>>>
> >>>>>>> swiotlb_unmap_single sounds like an exported function that  
> >>>>>>> IOMMUs
> >>>>>>> can
> >>>>>>> use (and it was) however it should not be.
> >>>>>>
> >>>>>> What do you suggest we call it?  __swiotlb_unmap_single.
> >>>>>
> >>>>> I think that __swiotlb_unmap_single is better because the name
> >>>>> implies
> >>>>> that it's an internal function. It's fine by me.
> >>>>>
> >>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper
> >>>>> function
> >>>>> of unmap_single(), which does the real job to unmap a dma mapping,
> >>>>> it
> >>>>> might be another possible option to rename unmap_single to
> >>>>> do_unamp_single and use unmap_single.
> >>>>
> >>>> I think you lost me here.  I'd prefer to just use
> >>>> __swiotlb_unmap_single at this point and get this code into the  
> >>>> tree
> >>>> and work on such renaming after the fact (if that's ok).
> >>>
> >>> If you are rushing to merge this right now, the original patchset is
> >>> fine by me (I thought that you missed this merge window). I'll  
> >>> rename
> >>> it later.
> >>
> >> We probably did, but one can never tell with these things.  It seemed
> >> like Ingo merged and pushed some swiotlb changes late in the game  
> >> for .
> >> 29
> >
> > Well, merging patches that have not been tested linux-next late is
> > what we should not do, I guess. I like to see Becky's patch in 2.6.30
> > because I have some swiotlb changes for 2.6.31 though.
> 
> Same here.  It makes it easier for us to work on the powerpc arch  
> specific changes for .31 if we can get these into .30.  What are you  
> looking at for .31?

I need to finish the dma_mapping_ops cleanups and cross-arch
unification stuff:

http://marc.info/?l=linux-kernel&m=123827903216314&w=2

But the changes to swiotlb is minor. BTW, I have the patches for
powerpc too.


> Ingo, any comments on that?

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> > Same here.  It makes it easier for us to work on the powerpc arch  
> > specific changes for .31 if we can get these into .30.  What are you  
> > looking at for .31?
> 
> I need to finish the dma_mapping_ops cleanups and cross-arch 
> unification stuff:
> 
> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
> 
> But the changes to swiotlb is minor. BTW, I have the patches for 
> powerpc too.
> 
> > Ingo, any comments on that?

if it's genuine fixes we can do it post-rc1, but it looks a tad wide 
for that:

 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    3 +-
 lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------

the patches came right in the merge window - that's too late for 
IOMMU bits, the patch cutoff is generally the beginning of the merge 
window. (Which was on March 23 in this window)

But i can queue them up in the .31 queue if it has all the Acks from 
you folks. Becky, mind resending the latest version, with all the 
acks in place?

Thanks,

	Ingo

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote:

>
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>
>>> Same here.  It makes it easier for us to work on the powerpc arch
>>> specific changes for .31 if we can get these into .30.  What are you
>>> looking at for .31?
>>
>> I need to finish the dma_mapping_ops cleanups and cross-arch
>> unification stuff:
>>
>> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
>>
>> But the changes to swiotlb is minor. BTW, I have the patches for
>> powerpc too.
>>
>>> Ingo, any comments on that?
>
> if it's genuine fixes we can do it post-rc1, but it looks a tad wide
> for that:
>
> arch/x86/kernel/pci-swiotlb.c |    2 +-
> include/linux/swiotlb.h       |    3 +-
> lib/swiotlb.c                 |  115 +++++++++++++++++++++ 
> +-------------------
>
> the patches came right in the merge window - that's too late for
> IOMMU bits, the patch cutoff is generally the beginning of the merge
> window. (Which was on March 23 in this window)

Fair.

> But i can queue them up in the .31 queue if it has all the Acks from
> you folks. Becky, mind resending the latest version, with all the
> acks in place?

I can do this in Becky's abscense this week.  The only "acks" I've  
seen are from FUJITA Tomonori.

Can we ask there be a unique swiotlb branch in your tree to make it  
easier for us to continue additional swiotlb work as well as arch  
specific work.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Ingo Molnar]

* Kumar Gala <galak@kernel.crashing.org> wrote:

>
> On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote:
>
>>
>> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>
>>>> Same here.  It makes it easier for us to work on the powerpc arch
>>>> specific changes for .31 if we can get these into .30.  What are you
>>>> looking at for .31?
>>>
>>> I need to finish the dma_mapping_ops cleanups and cross-arch
>>> unification stuff:
>>>
>>> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
>>>
>>> But the changes to swiotlb is minor. BTW, I have the patches for
>>> powerpc too.
>>>
>>>> Ingo, any comments on that?
>>
>> if it's genuine fixes we can do it post-rc1, but it looks a tad wide
>> for that:
>>
>> arch/x86/kernel/pci-swiotlb.c |    2 +-
>> include/linux/swiotlb.h       |    3 +-
>> lib/swiotlb.c                 |  115 +++++++++++++++++++++ 
>> +-------------------
>>
>> the patches came right in the merge window - that's too late for
>> IOMMU bits, the patch cutoff is generally the beginning of the merge
>> window. (Which was on March 23 in this window)
>
> Fair.
>
>> But i can queue them up in the .31 queue if it has all the Acks from
>> you folks. Becky, mind resending the latest version, with all the
>> acks in place?
>
> I can do this in Becky's abscense this week.  The only "acks" I've 
> seen are from FUJITA Tomonori.

_your_ ack (or signoff) counts too - so to me it's plural :-)

> Can we ask there be a unique swiotlb branch in your tree to make 
> it easier for us to continue additional swiotlb work as well as 
> arch specific work.

yes - tip:core/iommu is already distinct from tip:x86/iommu and is 
intended for such more generic patches. It has no patches pending 
right now (it just got all merged upstream) - so please use an 
upstream base - 2.6.30-rc1 would be a good starting point.

Thanks,

	Ingo

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

>>>
>>> the patches came right in the merge window - that's too late for
>>> IOMMU bits, the patch cutoff is generally the beginning of the merge
>>> window. (Which was on March 23 in this window)
>>
>> Fair.
>>
>>> But i can queue them up in the .31 queue if it has all the Acks from
>>> you folks. Becky, mind resending the latest version, with all the
>>> acks in place?
>>
>> I can do this in Becky's abscense this week.  The only "acks" I've
>> seen are from FUJITA Tomonori.
>
> _your_ ack (or signoff) counts too - so to me it's plural :-)

I've added my signoff :)

>> Can we ask there be a unique swiotlb branch in your tree to make
>> it easier for us to continue additional swiotlb work as well as
>> arch specific work.
>
> yes - tip:core/iommu is already distinct from tip:x86/iommu and is
> intended for such more generic patches. It has no patches pending
> right now (it just got all merged upstream) - so please use an
> upstream base - 2.6.30-rc1 would be a good starting point.

Just reposted v3 based on 2.6.30-rc1

- k

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

Here's the WIP patch for the PPC changes to give some sense of what we
need out of swiotlb for PPC.

- k

---
 arch/powerpc/include/asm/dma-mapping.h     |   18 ++++
 arch/powerpc/include/asm/scatterlist.h     |    6 +-
 arch/powerpc/include/asm/swiotlb.h         |   15 +++
 arch/powerpc/kernel/Makefile               |    1 +
 arch/powerpc/kernel/dma-swiotlb.c          |  154 ++++++++++++++++++++++++++++
 arch/powerpc/kernel/dma.c                  |    2 +-
 arch/powerpc/kernel/pci-common.c           |    2 +
 arch/powerpc/kernel/setup_32.c             |    4 +
 10 files changed, 211 insertions(+), 7 deletions(-)
 create mode 100644 arch/powerpc/include/asm/swiotlb.h
 create mode 100644 arch/powerpc/kernel/dma-swiotlb.c

diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
index c69f2b5..db6edfe 100644
--- a/arch/powerpc/include/asm/dma-mapping.h
+++ b/arch/powerpc/include/asm/dma-mapping.h
@@ -16,8 +16,18 @@
 #include <linux/dma-attrs.h>
 #include <asm/io.h>

+#ifdef CONFIG_SWIOTLB
+#include <asm/swiotlb.h>
+#endif
+
 #define DMA_ERROR_CODE		(~(dma_addr_t)0x0)

+/* Some dma direct funcs must be visible for use in other dma_ops */
+extern void *dma_direct_alloc_coherent(struct device *dev, size_t size,
+				       dma_addr_t *dma_handle, gfp_t flag);
+extern void dma_direct_free_coherent(struct device *dev, size_t size,
+				     void *vaddr, dma_addr_t dma_handle);
+
 #ifdef CONFIG_NOT_COHERENT_CACHE
 /*
  * DMA-consistent mapping functions for PowerPCs that don't support
@@ -112,11 +122,19 @@ static inline struct dma_mapping_ops *get_dma_ops(struct device *dev)
 	if (unlikely(dev == NULL))
 		return NULL;

+	/* BGILL - temp code to look for problems */
+	if(dev->archdata.dma_ops == NULL)
+		printk(KERN_EMERG "dev %s has null dma_ops\n", dev->bus->name);
+
 	return dev->archdata.dma_ops;
 }

 static inline void set_dma_ops(struct device *dev, struct dma_mapping_ops *ops)
 {
+	printk(KERN_EMERG "Setting dma_ops to %s\n",
+	       (ops == &dma_direct_ops) ? "dma_direct_ops" :
+	       ((ops == &swiotlb_dma_ops) ? "swiotlb_dma_ops" : "unknown"));
+
 	dev->archdata.dma_ops = ops;
 }

diff --git a/arch/powerpc/include/asm/scatterlist.h b/arch/powerpc/include/asm/scatterlist.h
index fcf7d55..912bf59 100644
--- a/arch/powerpc/include/asm/scatterlist.h
+++ b/arch/powerpc/include/asm/scatterlist.h
@@ -21,7 +21,7 @@ struct scatterlist {
 	unsigned int offset;
 	unsigned int length;

-	/* For TCE support */
+	/* For TCE or SWIOTLB support */
 	dma_addr_t dma_address;
 	u32 dma_length;
 };
@@ -34,11 +34,7 @@ struct scatterlist {
  * is 0.
  */
 #define sg_dma_address(sg)	((sg)->dma_address)
-#ifdef __powerpc64__
 #define sg_dma_len(sg)		((sg)->dma_length)
-#else
-#define sg_dma_len(sg)		((sg)->length)
-#endif

 #ifdef __powerpc64__
 #define ISA_DMA_THRESHOLD	(~0UL)
diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/include/asm/swiotlb.h
new file mode 100644
index 0000000..cbfce6c
--- /dev/null
+++ b/arch/powerpc/include/asm/swiotlb.h
@@ -0,0 +1,15 @@
+#ifndef __ASM_SWIOTLB_H
+#define __ASM_SWIOTLB_H
+
+#include <linux/swiotlb.h>
+
+extern int swiotlb_force;
+extern int swiotlb;
+extern struct dma_mapping_ops swiotlb_dma_ops;
+
+int swiotlb_arch_address_needs_mapping(struct device *, dma_addr_t,
+				       size_t size);
+
+static inline void dma_mark_clean(void *addr, size_t size) {}
+
+#endif /* __ASM_SWIOTLB_H */
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 71901fb..34c0a95 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_SMP)		+= smp.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_PPC_UDBG_16550)	+= legacy_serial.o udbg_16550.o
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
+obj-$(CONFIG_SWIOTLB)		+= dma-swiotlb.o

 pci64-$(CONFIG_PPC64)		+= pci_dn.o isa-bridge.o
 obj-$(CONFIG_PCI)		+= pci_$(CONFIG_WORD_SIZE).o $(pci64-y) \
diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c
new file mode 100644
index 0000000..639f3bc
--- /dev/null
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -0,0 +1,154 @@
+/*
+ * Copyright (C) 2009 Becky Bruce, Freescale Semiconductor
+ *
+ * swiotlb dma ops and functions required by the swiotlb code.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/pfn.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include <asm/machdep.h>
+#include <asm/swiotlb.h>
+#include <asm/dma.h>
+#include <asm/abs_addr.h>
+
+int swiotlb __read_mostly;
+
+unsigned long get_dma_direct_offset(struct device *dev);
+
+void *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t addr)
+{
+	unsigned long pfn = PFN_DOWN(swiotlb_bus_to_phys(hwdev, addr));
+	void *pageaddr = page_address(pfn_to_page(pfn));
+
+	if(pageaddr != NULL)
+		return pageaddr + (addr % PAGE_SIZE);
+	return NULL;
+}
+
+#if 0 /* BGILL - don't need */
+/* This can only be called on pages with a kernel mapping */
+dma_addr_t swiotlb_virt_to_bus(struct device *hwdev, void *addr)
+{
+	return swiotlb_phys_to_bus(hwdev, virt_to_abs((unsigned long)addr));
+}
+#endif
+
+dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
+{
+	return (paddr + get_dma_direct_offset(hwdev));
+}
+
+phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
+
+{
+	return (baddr - get_dma_direct_offset(hwdev));
+}
+
+/*
+ * Eventually, we should really be looking at some per-device
+ * quantity stored in archdata, and not a global value, since this is
+ * only needed for devices like PCI that use part of their 32 bit address
+ * space for something other than mapping memory.
+ *
+ * For now, require swiotlb mapping above MAX_32B_DIRECT_DMA_ADDR for devs
+ * that are not 64-bit capable.  This should be determined using the PCI mem
+ * allocations in the devtree. -beckyb
+ */
+#define MAX_32B_DIRECT_DMA_ADDR 0x80000000
+
+/* determine if an address needs bounce buffering via swiotlb. */
+int
+swiotlb_arch_address_needs_mapping(struct device *hwdev, dma_addr_t addr,
+				   size_t size)
+{
+	dma_addr_t mask = DMA_BIT_MASK(32);
+
+	/* Max dma_address we can access without bounce buffering */
+	dma_addr_t max = swiotlb_phys_to_bus(hwdev, MAX_32B_DIRECT_DMA_ADDR);
+
+	/* BGILL - can we get rid of this? */
+	if (hwdev && hwdev->dma_mask)
+		mask = *hwdev->dma_mask;
+
+	if ((mask <= DMA_BIT_MASK(36)) && (addr + size > max))
+		return 1;
+
+	return !is_buffer_dma_capable(mask, addr, size);
+}
+
+/*
+ * At the moment, all platforms that use this code only require
+ * swiotlb to be used if we're operating on HIGHMEM.  Since
+ * we don't ever call anything other than map_sg, unmap_sg,
+ * map_page, and unmap_page on highmem, use normal dma_ops
+ * for everything else.
+ */
+struct dma_mapping_ops swiotlb_dma_ops = {
+#if 0
+	.alloc_coherent = dma_direct_alloc_coherent,
+	.free_coherent = dma_direct_free_coherent,
+#else
+	.alloc_coherent = swiotlb_alloc_coherent,
+	.free_coherent = swiotlb_free_coherent,
+#endif
+	.map_sg = swiotlb_map_sg_attrs,
+	.unmap_sg = swiotlb_unmap_sg_attrs,
+	.dma_supported = swiotlb_dma_supported,
+	.map_page = swiotlb_map_page,
+	.unmap_page = swiotlb_unmap_page,
+	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
+	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
+	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
+	.sync_sg_for_device = swiotlb_sync_sg_for_device
+};
+
+static int ppc_swiotlb_bus_notify(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	/* We are only intereted in device addition */
+	if (action != BUS_NOTIFY_ADD_DEVICE)
+		return 0;
+
+#if 0 /* BGILL - should look like this; use other form for debug */
+	if(dma_get_mask(dev) < DMA_BIT_MASK(36))
+		set_dma_ops(dev, &swiotlb_dma_ops);
+#else
+	if(!dev->dma_mask) {
+		printk(KERN_EMERG "ERROR: no dma_mask set for dev\n");
+		set_dma_ops(dev, &swiotlb_dma_ops);
+	}
+	else if(*dev->dma_mask < DMA_BIT_MASK(36)) {
+		set_dma_ops(dev, &swiotlb_dma_ops);
+	}
+	else
+		printk(KERN_EMERG "ERROR; fall thru with direct_ops\n");
+#endif
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block ppc_swiotlb_plat_bus_notifier = {
+	.notifier_call = ppc_swiotlb_bus_notify,
+	.priority = 0,
+};
+
+static struct notifier_block ppc_swiotlb_of_bus_notifier = {
+	.notifier_call = ppc_swiotlb_bus_notify,
+	.priority = 0,
+};
+
+static int __init setup_bus_notifier(void)
+{
+	bus_register_notifier(&platform_bus_type, &ppc_swiotlb_plat_bus_notifier);
+	bus_register_notifier(&of_platform_bus_type, &ppc_swiotlb_of_bus_notifier);
+
+	return 0;
+}
+
+machine_arch_initcall(mpc86xx_hpcn, setup_bus_notifier);
+
diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
index 53c7788..62d80c4 100644
--- a/arch/powerpc/kernel/dma.c
+++ b/arch/powerpc/kernel/dma.c
@@ -19,7 +19,7 @@
  * default the offset is PCI_DRAM_OFFSET.
  */

-static unsigned long get_dma_direct_offset(struct device *dev)
+unsigned long get_dma_direct_offset(struct device *dev)
 {
 	if (dev)
 		return (unsigned long)dev->archdata.dma_data;
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 9e1ca74..f038b13 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -332,6 +332,10 @@ void __init setup_arch(char **cmdline_p)
 		ppc_md.setup_arch();
 	if ( ppc_md.progress ) ppc_md.progress("arch: exit", 0x3eab);

+	/* Allow iotlb to do it's setup */
+#ifdef CONFIG_SWIOTLB
+	swiotlb_init();
+#endif
 	paging_init();

 	/* Initialize the MMU context management stuff */

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

On Apr 9, 2009, at 1:50 PM, FUJITA Tomonori wrote:

> On Wed, 8 Apr 2009 23:59:18 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote:
>>
>>>> Becky's patches of last week also added __weak annotations to
>>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev
>>>> parameter to swiotlb_bus_to_phys; and added a weak
>>>> swiotlb_arch_address_needs_mapping.  I assume that was needed  
>>>> because
>>>> powerpc needs non-trivial implementations for those functions.
>>>
>>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
>>> can remove these and directly use virt_to_bus and bus_to_virt.
>>>
>>> About __weak address_needs_mapping function, as I said, removing it
>>> and using dma_map_ops is a proper solution.
>>
>> Is this something you are looking at doing in the .31 timeframe?
>>
>> I'm looking at the fact that we need to switch over to using struct
>> dma_map_ops on ppc.  (I'm guessing this might be the patches you
>> mentioned the other day).  If so did you add set_dma_mask() to the
>> generic dma_map_ops?
>
> Yeah, I'll send patches to convert ppc to use dma_map_ops. In .31
> timeframe, I plan to:
>
> - add a generic dma-mapping.h and convert ia64, x86, and ppc to use it
>
> - clean up swiotlb.
>
> - try to convert archs supporting multiple dma ops to use dma_map_ops
>
> - rewrite ia64 and x86 dma ops initialization
>
>
> BTW, the ppc specific swiotlb patchset is available?

Still cleaning it up, but I'll post a WIP patch so people have a sense  
of what the changes look like.

- k

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
> Hmm, I think that ia64 use swiotlb_alloc_boot too.
>
> ia64 uses swiotlb in two ways; using only swiotlb, using swiotlb and
> hw iommu. The latter is similar what x86 Calgary does.
>   

Yes, I see.  It also calls swiotlb_init() -> 
swiotlb_init_with_default_size(), though I got lost in the 
machvec/platform setup, so I couldn't quite work out where 
platform_dma_init() gets called from.

But I couldn't see any x86 references to 
swiotlb_late_init_with_default_size().

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Thu, 09 Apr 2009 12:19:19 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> >> Well, Becky's patches also added the hwdev argument to them, so 
> >> presumably the powerpc implementation needs that (different 
> >> devices/buses have differing views of physical memory, I guess).
> >>     
> >
> > Until I see the ppc specific swiotlb patchset, I'm not sure but I
> > think that we can remove phys_to_bus in swiotlb.
> >   
> 
> Kumar's comment was: "For our SoC chips we don't need any mapping 
> between phys & bus.  However something like PCI does have a mapping (a 
> simple offset)."
> 
> Kumar, could a single system have different phys<->bus mappings on a 
> single system, or could it differ from device to device (or bus to bus)?
> 
> > Even if we need phys_to_bus, we can remove the rest of __weak tricks
> > for only dom0. And we can make phys_to_bus arch-specific. Then we
> > don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0
> > support adds many hacks to swiotlb.
> >   
> 
> Well, we'd still need a way to do hook the swiotlb_alloc(_boot) 
> allocation.  At the moment its effectively arch-specific because x86 
> only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc().  One 

Hmm, I think that ia64 use swiotlb_alloc_boot too.

ia64 uses swiotlb in two ways; using only swiotlb, using swiotlb and
hw iommu. The latter is similar what x86 Calgary does.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Thu, 09 Apr 2009 12:19:19 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> >> Well, Becky's patches also added the hwdev argument to them, so 
> >> presumably the powerpc implementation needs that (different 
> >> devices/buses have differing views of physical memory, I guess).
> >>     
> >
> > Until I see the ppc specific swiotlb patchset, I'm not sure but I
> > think that we can remove phys_to_bus in swiotlb.
> >   
> 
> Kumar's comment was: "For our SoC chips we don't need any mapping 
> between phys & bus.  However something like PCI does have a mapping (a 
> simple offset)."

I meant that swiotlb doesn't need to use phys_to_bus stuff. But I as
said, I'm not sure until I see the ppc swiotlb code.


> Kumar, could a single system have different phys<->bus mappings on a 
> single system, or could it differ from device to device (or bus to bus)?
> 
> > Even if we need phys_to_bus, we can remove the rest of __weak tricks
> > for only dom0. And we can make phys_to_bus arch-specific. Then we
> > don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0
> > support adds many hacks to swiotlb.
> >   
> 
> Well, we'd still need a way to do hook the swiotlb_alloc(_boot) 
> allocation.  At the moment its effectively arch-specific because x86 
> only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc().  One 
> option would be to simply make that function arch-defined, which would 
> remove the need for any kind of override mechanism in lib/swiotlb; that 
> would match the handling of phys_to_bus.  And its more appealing if we 
> manage to drop swiotlb_alloc_boot, so there's only a single function for 
> the arches to worry about.
> 
> > Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it
> > doesn't need to handle it because the block layer can thanks to
> > the bouncing (the network layer does the similar, I think).
> >
> > As you said, we could remove the latter though I'm not sure.
> >   
> 
> It would take a bit of rearranging the x86 swiotlb/iommu init sequence, 
> but I don't think it would be too complex.  I'll look into it.

Unfortunately, not that simple. IA64_64 and x86 share the iommu init
sequence. So you need to look at both. I put some cleanups on it in
30-rc1. But I need to do more cleanups.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
>> Well, Becky's patches also added the hwdev argument to them, so 
>> presumably the powerpc implementation needs that (different 
>> devices/buses have differing views of physical memory, I guess).
>>     
>
> Until I see the ppc specific swiotlb patchset, I'm not sure but I
> think that we can remove phys_to_bus in swiotlb.
>   

Kumar's comment was: "For our SoC chips we don't need any mapping 
between phys & bus.  However something like PCI does have a mapping (a 
simple offset)."

Kumar, could a single system have different phys<->bus mappings on a 
single system, or could it differ from device to device (or bus to bus)?

> Even if we need phys_to_bus, we can remove the rest of __weak tricks
> for only dom0. And we can make phys_to_bus arch-specific. Then we
> don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0
> support adds many hacks to swiotlb.
>   

Well, we'd still need a way to do hook the swiotlb_alloc(_boot) 
allocation.  At the moment its effectively arch-specific because x86 
only uses swiotlb_alloc_boot(), and ia64 only uses swiotlb_alloc().  One 
option would be to simply make that function arch-defined, which would 
remove the need for any kind of override mechanism in lib/swiotlb; that 
would match the handling of phys_to_bus.  And its more appealing if we 
manage to drop swiotlb_alloc_boot, so there's only a single function for 
the arches to worry about.

> Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it
> doesn't need to handle it because the block layer can thanks to
> the bouncing (the network layer does the similar, I think).
>
> As you said, we could remove the latter though I'm not sure.
>   

It would take a bit of rearranging the x86 swiotlb/iommu init sequence, 
but I don't think it would be too complex.  I'll look into it.

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 8 Apr 2009 23:59:18 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote:
> 
> >> Becky's patches of last week also added __weak annotations to
> >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev
> >> parameter to swiotlb_bus_to_phys; and added a weak
> >> swiotlb_arch_address_needs_mapping.  I assume that was needed because
> >> powerpc needs non-trivial implementations for those functions.
> >
> > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
> > can remove these and directly use virt_to_bus and bus_to_virt.
> >
> > About __weak address_needs_mapping function, as I said, removing it
> > and using dma_map_ops is a proper solution.
> 
> Is this something you are looking at doing in the .31 timeframe?
> 
> I'm looking at the fact that we need to switch over to using struct  
> dma_map_ops on ppc.  (I'm guessing this might be the patches you  
> mentioned the other day).  If so did you add set_dma_mask() to the  
> generic dma_map_ops?

Yeah, I'll send patches to convert ppc to use dma_map_ops. In .31
timeframe, I plan to:

- add a generic dma-mapping.h and convert ia64, x86, and ppc to use it

- clean up swiotlb.

- try to convert archs supporting multiple dma ops to use dma_map_ops

- rewrite ia64 and x86 dma ops initialization


BTW, the ppc specific swiotlb patchset is available?

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 08 Apr 2009 17:09:00 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 08 Apr 2009 16:16:17 -0700
> > Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> >
> >   
> >> FUJITA Tomonori wrote:
> >>     
> >>>> Becky's patches of last week also added __weak annotations to 
> >>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
> >>>> parameter to swiotlb_bus_to_phys; and added a weak 
> >>>> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
> >>>> powerpc needs non-trivial implementations for those functions.
> >>>>     
> >>>>         
> >>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
> >>> can remove these and directly use virt_to_bus and bus_to_virt.
> >>>   
> >>>       
> >> In general those interfaces are deprecated.  Are we un-deprecating 
> >> them?  Or do you mean adding virt<->bus to dma_ops?
> >>     
> >
> > Hmm, these interfaces are wrong for drivers surely because they can't
> > handle dma mapping properly. However, they are exactly what swiotlb
> > needs (swiotlb doesn't need to care about dma mapping).
> 
> It needs to care about the mapping from phys to bus.  On x86 they're 
> identical, but on powerpc there can be at least an offset between them.
> 
> >  Until 2.6.28,
> > swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think.
> >   
> 
> Well, Becky's patches also added the hwdev argument to them, so 
> presumably the powerpc implementation needs that (different 
> devices/buses have differing views of physical memory, I guess).

Until I see the ppc specific swiotlb patchset, I'm not sure but I
think that we can remove phys_to_bus in swiotlb.

Even if we need phys_to_bus, we can remove the rest of __weak tricks
for only dom0. And we can make phys_to_bus arch-specific. Then we
don't need any __weak tricks in swiotlb (and x86's swiotlb). dom0
support adds many hacks to swiotlb.


> > I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc.
> >   
> 
> No, its something we need for Xen.  I was thinking that swiotlb could 
> allocate its memory with dma_alloc_coherent(NULL, size, ...).  That 
> would allocate via x86_fallback_device, which would not have the right 
> behaviour (it would set GFP_DMA, for a start), and would end up hitting 
> the uninitialized dma_ops.  So the idea doesn't really work; it would 
> need swiotlb to define another placeholder  device who's alloc_coherent 
> operation could be overridden, and it all gets pretty ugly.

It doesn't work. And swiotlb's alloc_coherent needs to be
arch-specific because the page allocator can't handle dma mask.


> As an aside, I'm also wondering why there's a distinction between 
> swiotlb_alloc() and swiotlb_alloc_boot().  The latter allocates from 
> bootmem, but I don't see what's wrong with allocating from slab a little 
> bit later, once it has been initialized.  The comment mentions something 
> about allocating ISA DMA memory, but the code doesn't make any attempt 
> to allocate the buffer below 16MB (its generally much larger than 16MB 
> anyway).

Yeah, ISA DMA comment is misleading. swiotlb can't handle it. And it
doesn't need to handle it because the block layer can thanks to
the bouncing (the network layer does the similar, I think).

As you said, we could remove the latter though I'm not sure.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

On Apr 8, 2009, at 6:01 PM, FUJITA Tomonori wrote:

>> Becky's patches of last week also added __weak annotations to
>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev
>> parameter to swiotlb_bus_to_phys; and added a weak
>> swiotlb_arch_address_needs_mapping.  I assume that was needed because
>> powerpc needs non-trivial implementations for those functions.
>
> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
> can remove these and directly use virt_to_bus and bus_to_virt.
>
> About __weak address_needs_mapping function, as I said, removing it
> and using dma_map_ops is a proper solution.

Is this something you are looking at doing in the .31 timeframe?

I'm looking at the fact that we need to switch over to using struct  
dma_map_ops on ppc.  (I'm guessing this might be the patches you  
mentioned the other day).  If so did you add set_dma_mask() to the  
generic dma_map_ops?

- k

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

On Apr 8, 2009, at 7:09 PM, Jeremy Fitzhardinge wrote:

> FUJITA Tomonori wrote:
>> On Wed, 08 Apr 2009 16:16:17 -0700
>> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>>
>>
>>> FUJITA Tomonori wrote:
>>>
>>>>> Becky's patches of last week also added __weak annotations to  
>>>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the  
>>>>> hwdev parameter to swiotlb_bus_to_phys; and added a weak  
>>>>> swiotlb_arch_address_needs_mapping.  I assume that was needed  
>>>>> because powerpc needs non-trivial implementations for those  
>>>>> functions.
>>>>>
>>>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
>>>> can remove these and directly use virt_to_bus and bus_to_virt.
>>>>
>>> In general those interfaces are deprecated.  Are we un-deprecating  
>>> them?  Or do you mean adding virt<->bus to dma_ops?
>>>
>>
>> Hmm, these interfaces are wrong for drivers surely because they can't
>> handle dma mapping properly. However, they are exactly what swiotlb
>> needs (swiotlb doesn't need to care about dma mapping).
>
> It needs to care about the mapping from phys to bus.  On x86 they're  
> identical, but on powerpc there can be at least an offset between  
> them.
>
>> Until 2.6.28,
>> swiotlb has used them. They are with IA64, X86_64 and PPC_32, I  
>> think.
>>
>
> Well, Becky's patches also added the hwdev argument to them, so  
> presumably the powerpc implementation needs that (different devices/ 
> buses have differing views of physical memory, I guess).

On powerpc we need the hwdev because things vary based on bus.  For  
our SoC chips we don't need any mapping between phys & bus.  However  
something like PCI does have a mapping (a simple offset).

- k

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
> On Wed, 08 Apr 2009 16:16:17 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>> FUJITA Tomonori wrote:
>>     
>>>> Becky's patches of last week also added __weak annotations to 
>>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
>>>> parameter to swiotlb_bus_to_phys; and added a weak 
>>>> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
>>>> powerpc needs non-trivial implementations for those functions.
>>>>     
>>>>         
>>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
>>> can remove these and directly use virt_to_bus and bus_to_virt.
>>>   
>>>       
>> In general those interfaces are deprecated.  Are we un-deprecating 
>> them?  Or do you mean adding virt<->bus to dma_ops?
>>     
>
> Hmm, these interfaces are wrong for drivers surely because they can't
> handle dma mapping properly. However, they are exactly what swiotlb
> needs (swiotlb doesn't need to care about dma mapping).

It needs to care about the mapping from phys to bus.  On x86 they're 
identical, but on powerpc there can be at least an offset between them.

>  Until 2.6.28,
> swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think.
>   

Well, Becky's patches also added the hwdev argument to them, so 
presumably the powerpc implementation needs that (different 
devices/buses have differing views of physical memory, I guess).

> I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc.
>   

No, its something we need for Xen.  I was thinking that swiotlb could 
allocate its memory with dma_alloc_coherent(NULL, size, ...).  That 
would allocate via x86_fallback_device, which would not have the right 
behaviour (it would set GFP_DMA, for a start), and would end up hitting 
the uninitialized dma_ops.  So the idea doesn't really work; it would 
need swiotlb to define another placeholder  device who's alloc_coherent 
operation could be overridden, and it all gets pretty ugly.

As an aside, I'm also wondering why there's a distinction between 
swiotlb_alloc() and swiotlb_alloc_boot().  The latter allocates from 
bootmem, but I don't see what's wrong with allocating from slab a little 
bit later, once it has been initialized.  The comment mentions something 
about allocating ISA DMA memory, but the code doesn't make any attempt 
to allocate the buffer below 16MB (its generally much larger than 16MB 
anyway).

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 08 Apr 2009 16:16:17 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> >> Becky's patches of last week also added __weak annotations to 
> >> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
> >> parameter to swiotlb_bus_to_phys; and added a weak 
> >> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
> >> powerpc needs non-trivial implementations for those functions.
> >>     
> >
> > Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
> > can remove these and directly use virt_to_bus and bus_to_virt.
> >   
> 
> In general those interfaces are deprecated.  Are we un-deprecating 
> them?  Or do you mean adding virt<->bus to dma_ops?

Hmm, these interfaces are wrong for drivers surely because they can't
handle dma mapping properly. However, they are exactly what swiotlb
needs (swiotlb doesn't need to care about dma mapping). Until 2.6.28,
swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think.


> > About __weak address_needs_mapping function, as I said, removing it
> > and using dma_map_ops is a proper solution.
> >   
> 
> Fine.  Could swiotlb_alloc() just call dma_alloc_coherent() too?

I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
>> Becky's patches of last week also added __weak annotations to 
>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
>> parameter to swiotlb_bus_to_phys; and added a weak 
>> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
>> powerpc needs non-trivial implementations for those functions.
>>     
>
> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
> can remove these and directly use virt_to_bus and bus_to_virt.
>   

In general those interfaces are deprecated.  Are we un-deprecating 
them?  Or do you mean adding virt<->bus to dma_ops?

> About __weak address_needs_mapping function, as I said, removing it
> and using dma_map_ops is a proper solution.
>   

Fine.  Could swiotlb_alloc() just call dma_alloc_coherent() too?

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 08 Apr 2009 15:36:58 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> >> Some architectures need non-trivial bus<->phys conversion routines, etc, 
> >>     
> >
> > Only Xen needs such conversion for swiotlb.
> >   
> 
> Becky's patches of last week also added __weak annotations to 
> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
> parameter to swiotlb_bus_to_phys; and added a weak 
> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
> powerpc needs non-trivial implementations for those functions.

Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
can remove these and directly use virt_to_bus and bus_to_virt.

About __weak address_needs_mapping function, as I said, removing it
and using dma_map_ops is a proper solution.


> >> so either we can require it that all architectures wishing to use 
> >> swiotlb define these functions, or have weak default functions that can 
> >> be overridden by architectures where necessary.
> >>     
> >
> > Can you give an example? I don't think IA64, X86_64 or POWER (which
> > will use swiotlb) need any __weak functions. If you say other archs
> > could use swiotlb, please tell me how they need these __weak.
> >   
> 
> As I said, Becky's patches added hooks in many of the places we added 
> them for Xen.  I assume that's because powerpc needs them; I have not 
> seen the arch/powerpc side of those changes.
> 
>     J
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
>> Some architectures need non-trivial bus<->phys conversion routines, etc, 
>>     
>
> Only Xen needs such conversion for swiotlb.
>   

Becky's patches of last week also added __weak annotations to 
swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
parameter to swiotlb_bus_to_phys; and added a weak 
swiotlb_arch_address_needs_mapping.  I assume that was needed because 
powerpc needs non-trivial implementations for those functions.

>> so either we can require it that all architectures wishing to use 
>> swiotlb define these functions, or have weak default functions that can 
>> be overridden by architectures where necessary.
>>     
>
> Can you give an example? I don't think IA64, X86_64 or POWER (which
> will use swiotlb) need any __weak functions. If you say other archs
> could use swiotlb, please tell me how they need these __weak.
>   

As I said, Becky's patches added hooks in many of the places we added 
them for Xen.  I assume that's because powerpc needs them; I have not 
seen the arch/powerpc side of those changes.

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Christoph Hellwig]

On Thu, Apr 09, 2009 at 06:15:07AM +0900, FUJITA Tomonori wrote:
> ia64 and x86_64 use swiotlb but neither need this function. And
> neither need any above __weak. They were added for dom0 support.
> Yeah, swiotlb is much cleaner and better if we don't add dom0 support.

Agreed.  All these dom0 support patches look like a cat has been barfing
over the source code.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 08 Apr 2009 14:55:55 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 8 Apr 2009 15:56:32 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >   
> >> On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote:
> >>
> >>     
> >>> On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
> >>>       
> >>>> From: Becky Bruce <beckyb@kernel.crashing.org>
> >>>>
> >>>> Some architectures require additional checking to determine
> >>>> if a device can dma to an address and need to provide their
> >>>> own address_needs_mapping..
> >>>>         
> >>> Shouldn't we just move it completely to the arch?  I think that ia64  
> >>> and
> >>> x86 currently use the same one is more of an accident.
> >>>       
> >> It seems like the swiotlb code uses __weak for a number of things:
> >>
> >> lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size,  
> >> unsigned long nslabs)
> >> lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned  
> >> long nslabs)
> >> lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device  
> >> *hwdev, phys_addr_t paddr)
> >> lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device  
> >> *hwdev, dma_addr_t baddr)
> >> lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev,  
> >> dma_addr_t address)
> >> lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct  
> >> device *hwdev,
> >> lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t  
> >> paddr, size_t size)
> >>
> >> instead of #ifndef HAVE_ARCH_<FOO>.  Not sure if there is a historical  
> >> reason for that.
> >>     
> >
> > ia64 and x86_64 use swiotlb but neither need this function. And
> > neither need any above __weak. They were added for dom0 support.
> > Yeah, swiotlb is much cleaner and better if we don't add dom0 support.
> >   
> 
> Some architectures need non-trivial bus<->phys conversion routines, etc, 

Only Xen needs such conversion for swiotlb.


> so either we can require it that all architectures wishing to use 
> swiotlb define these functions, or have weak default functions that can 
> be overridden by architectures where necessary.

Can you give an example? I don't think IA64, X86_64 or POWER (which
will use swiotlb) need any __weak functions. If you say other archs
could use swiotlb, please tell me how they need these __weak.


> This isn't a specific Xen dom0 requirement, except that enabling it in 

Yes, it is.


> the config will override these functions (but now in a Xen-only file, 
> rather than affecting the normal x86 pci-swiotlb.c).

And again, x86' pci-swiotlb is much cleaner without dom0 support.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge]

FUJITA Tomonori wrote:
> On Wed, 8 Apr 2009 15:56:32 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>   
>> On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote:
>>
>>     
>>> On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
>>>       
>>>> From: Becky Bruce <beckyb@kernel.crashing.org>
>>>>
>>>> Some architectures require additional checking to determine
>>>> if a device can dma to an address and need to provide their
>>>> own address_needs_mapping..
>>>>         
>>> Shouldn't we just move it completely to the arch?  I think that ia64  
>>> and
>>> x86 currently use the same one is more of an accident.
>>>       
>> It seems like the swiotlb code uses __weak for a number of things:
>>
>> lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size,  
>> unsigned long nslabs)
>> lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned  
>> long nslabs)
>> lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device  
>> *hwdev, phys_addr_t paddr)
>> lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device  
>> *hwdev, dma_addr_t baddr)
>> lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev,  
>> dma_addr_t address)
>> lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct  
>> device *hwdev,
>> lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t  
>> paddr, size_t size)
>>
>> instead of #ifndef HAVE_ARCH_<FOO>.  Not sure if there is a historical  
>> reason for that.
>>     
>
> ia64 and x86_64 use swiotlb but neither need this function. And
> neither need any above __weak. They were added for dom0 support.
> Yeah, swiotlb is much cleaner and better if we don't add dom0 support.
>   

Some architectures need non-trivial bus<->phys conversion routines, etc, 
so either we can require it that all architectures wishing to use 
swiotlb define these functions, or have weak default functions that can 
be overridden by architectures where necessary.

This isn't a specific Xen dom0 requirement, except that enabling it in 
the config will override these functions (but now in a Xen-only file, 
rather than affecting the normal x86 pci-swiotlb.c).

    J

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Wed, 8 Apr 2009 15:56:32 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote:
> 
> > On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
> >> From: Becky Bruce <beckyb@kernel.crashing.org>
> >>
> >> Some architectures require additional checking to determine
> >> if a device can dma to an address and need to provide their
> >> own address_needs_mapping..
> >
> > Shouldn't we just move it completely to the arch?  I think that ia64  
> > and
> > x86 currently use the same one is more of an accident.
> 
> It seems like the swiotlb code uses __weak for a number of things:
> 
> lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size,  
> unsigned long nslabs)
> lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned  
> long nslabs)
> lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device  
> *hwdev, phys_addr_t paddr)
> lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device  
> *hwdev, dma_addr_t baddr)
> lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev,  
> dma_addr_t address)
> lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct  
> device *hwdev,
> lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t  
> paddr, size_t size)
> 
> instead of #ifndef HAVE_ARCH_<FOO>.  Not sure if there is a historical  
> reason for that.

ia64 and x86_64 use swiotlb but neither need this function. And
neither need any above __weak. They were added for dom0 support.
Yeah, swiotlb is much cleaner and better if we don't add dom0 support.

About this patch, I think that we could do better. What we need to do
is allowing each architectures to have is_buffer_dma_capable().

I'm doing the dma_mapping_ops unification. I think that adding
something like is_buffer_dma_capable to dma_map_ops struct is
cleaner. Then we don't need this __weak function. But this patch is
fine by me for now.

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

On Apr 8, 2009, at 3:38 PM, Christoph Hellwig wrote:

> On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
>> From: Becky Bruce <beckyb@kernel.crashing.org>
>>
>> Some architectures require additional checking to determine
>> if a device can dma to an address and need to provide their
>> own address_needs_mapping..
>
> Shouldn't we just move it completely to the arch?  I think that ia64  
> and
> x86 currently use the same one is more of an accident.

It seems like the swiotlb code uses __weak for a number of things:

lib/swiotlb.c:void * __weak __init swiotlb_alloc_boot(size_t size,  
unsigned long nslabs)
lib/swiotlb.c:void * __weak swiotlb_alloc(unsigned order, unsigned  
long nslabs)
lib/swiotlb.c:dma_addr_t __weak swiotlb_phys_to_bus(struct device  
*hwdev, phys_addr_t paddr)
lib/swiotlb.c:phys_addr_t __weak swiotlb_bus_to_phys(struct device  
*hwdev, dma_addr_t baddr)
lib/swiotlb.c:void * __weak swiotlb_bus_to_virt(struct device *hwdev,  
dma_addr_t address)
lib/swiotlb.c:int __weak swiotlb_arch_address_needs_mapping(struct  
device *hwdev,
lib/swiotlb.c:int __weak swiotlb_arch_range_needs_mapping(phys_addr_t  
paddr, size_t size)

instead of #ifndef HAVE_ARCH_<FOO>.  Not sure if there is a historical  
reason for that.

- k

Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Christoph Hellwig]

On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
> From: Becky Bruce <beckyb@kernel.crashing.org>
> 
> Some architectures require additional checking to determine
> if a device can dma to an address and need to provide their
> own address_needs_mapping..

Shouldn't we just move it completely to the arch?  I think that ia64 and
x86 currently use the same one is more of an accident.

[PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Kumar Gala]

From: Becky Bruce <beckyb@kernel.crashing.org>

Some architectures require additional checking to determine
if a device can dma to an address and need to provide their
own address_needs_mapping..

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 lib/swiotlb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index e8a47c8..d81afab 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -145,6 +145,12 @@ static void *swiotlb_bus_to_virt(dma_addr_t address)
 	return phys_to_virt(swiotlb_bus_to_phys(address));
 }
 
+int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
+					       dma_addr_t addr, size_t size)
+{
+	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+}
+
 int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size)
 {
 	return 0;
@@ -309,10 +315,10 @@ cleanup1:
 	return -ENOMEM;
 }
 
-static int
+static inline int
 address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size)
 {
-	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+	return swiotlb_arch_address_needs_mapping(hwdev, addr, size);
 }
 
 static inline int range_needs_mapping(phys_addr_t paddr, size_t size)
-- 
1.6.0.6