LinuxLists.cc - [PATCH V3 0/7] swiotlb: changes for powerpc/highmem

[permalink] [raw]

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

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
lib/swiotlb.c | 30 ++++++++++++------------------
1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 2bde54a..d912f06 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -731,10 +731,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
@@ -761,13 +767,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
@@ -890,15 +890,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.6.0.6

2009-04-08 14:11:49

[permalink] [raw]

Subject: [PATCH 2/7] swiotlb: fix compile warning

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
lib/swiotlb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 170cf56..4fd6a76 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.6.0.6

2009-04-08 14:12:28

[permalink] [raw]

Subject: [PATCH 5/7] swiotlb: Rename unmap_single to do_unmap_single

From: Becky Bruce <[email protected]>

Previously, swiotlb_unmap_page and swiotlb_unmap_sg were
duplicating very similar code. Refactor that code into a
new unmap_single and unmap_single use do_unmap_single.

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 <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
lib/swiotlb.c | 42 +++++++++++++++++++++++++-----------------
1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index d81afab..2bde54a 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,17 +688,29 @@ 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 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))
- unmap_single(hwdev, dma_addr, size, dir);
- else if (dir == DMA_FROM_DEVICE)
- dma_mark_clean(dma_addr, size);
+
+ if (is_swiotlb_buffer(dma_addr)) {
+ do_unmap_single(hwdev, dma_addr, size, dir);
+ 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)
+{
+ unmap_single(hwdev, dev_addr, size, dir);
}
EXPORT_SYMBOL_GPL(swiotlb_unmap_page);

@@ -850,13 +862,9 @@ 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)
+ unmap_single(hwdev, sg->dma_address, sg->dma_length, dir);
+
}
EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);

--
1.6.0.6

2009-04-08 14:13:03

[permalink] [raw]

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

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
lib/swiotlb.c | 24 +++++++++++-------------
1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 2b0b5a7..170cf56 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.6.0.6

2009-04-08 14:13:39

[permalink] [raw]

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

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
lib/swiotlb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 4fd6a76..e8a47c8 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.6.0.6

2009-04-08 14:14:17

[permalink] [raw]

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

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
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 d912f06..bffe6d7 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,
@@ -691,7 +691,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
static void 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);

@@ -728,7 +728,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.6.0.6

2009-04-08 14:14:46

[permalink] [raw]

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

From: Becky Bruce <[email protected]>

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
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

2009-04-08 14:22:04

by Ingo Molnar

[permalink] [raw]

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

* Kumar Gala <[email protected]> wrote:

> This is a repost of Becky's patches from the v2 version. The only
> change is to incorporate feedback on "Create swiotlb_unmap_single"
> an to add Ack by FUJITA Tomonori for patches that have been
> reviewed.

thanks - picked them up.

This commit:

dd6b02f: swiotlb: map_page fix for highmem systems

should perhaps go into an urgent track for .30 too? Jeremy, does Xen
already make use of the swiotlb? (it does not right now AFAICS but
.30 mergs of Xen might, so it would be good for them to have this
fix.)

Ingo

2009-04-08 15:25:40

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: comment corrections

Commit-ID: ceb5ac3264686e75e6951de6a18d4baa9bdecb92
Gitweb: http://git.kernel.org/tip/ceb5ac3264686e75e6951de6a18d4baa9bdecb92
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:15 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:35 +0200

swiotlb: comment corrections

Impact: cleanup

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
lib/swiotlb.c | 24 +++++++++++-------------
1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 2b0b5a7..170cf56 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,

2009-04-08 15:26:58

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: use swiotlb_sync_single instead of duplicating code

Commit-ID: 380d687833aee098c4a2c3b35beaefe1c1f48d01
Gitweb: http://git.kernel.org/tip/380d687833aee098c4a2c3b35beaefe1c1f48d01
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:20 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:37 +0200

swiotlb: use swiotlb_sync_single instead of duplicating code

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
lib/swiotlb.c | 30 ++++++++++++------------------
1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 2bde54a..d912f06 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -731,10 +731,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
@@ -761,13 +767,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
@@ -890,15 +890,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

2009-04-08 15:27:26

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: rename unmap_single to do_unmap_single

Commit-ID: 7fcebbd2d984eac3fdd6da2f4453e7c43d32de89
Gitweb: http://git.kernel.org/tip/7fcebbd2d984eac3fdd6da2f4453e7c43d32de89
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:19 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:37 +0200

swiotlb: rename unmap_single to do_unmap_single

Previously, swiotlb_unmap_page and swiotlb_unmap_sg were
duplicating very similar code. Refactor that code into a
new unmap_single and unmap_single use do_unmap_single.

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 <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
lib/swiotlb.c | 42 +++++++++++++++++++++++++-----------------
1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index d81afab..2bde54a 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,17 +688,29 @@ 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 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))
- unmap_single(hwdev, dma_addr, size, dir);
- else if (dir == DMA_FROM_DEVICE)
- dma_mark_clean(dma_addr, size);
+
+ if (is_swiotlb_buffer(dma_addr)) {
+ do_unmap_single(hwdev, dma_addr, size, dir);
+ 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)
+{
+ unmap_single(hwdev, dev_addr, size, dir);
}
EXPORT_SYMBOL_GPL(swiotlb_unmap_page);

@@ -850,13 +862,9 @@ 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)
+ unmap_single(hwdev, sg->dma_address, sg->dma_length, dir);
+
}
EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);

2009-04-08 15:27:44

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: map_page fix for highmem systems

Commit-ID: dd6b02fe427f30520d0adc94aa52352367227873
Gitweb: http://git.kernel.org/tip/dd6b02fe427f30520d0adc94aa52352367227873
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:17 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:36 +0200

swiotlb: map_page fix for highmem systems

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
lib/swiotlb.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 4fd6a76..e8a47c8 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;

/*

2009-04-08 15:28:05

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: fix compile warning

Commit-ID: 67131ad0514d7105b55003a0506209cf1bba3f00
Gitweb: http://git.kernel.org/tip/67131ad0514d7105b55003a0506209cf1bba3f00
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:16 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:35 +0200

swiotlb: fix compile warning

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
lib/swiotlb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 170cf56..4fd6a76 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),

2009-04-08 15:28:28

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: allow arch override of address_needs_mapping

Commit-ID: ef5722f698bde01cfec2b98fff733a48663ebf55
Gitweb: http://git.kernel.org/tip/ef5722f698bde01cfec2b98fff733a48663ebf55
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:18 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:36 +0200

swiotlb: allow arch override of address_needs_mapping

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

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

2009-04-08 15:29:29

[permalink] [raw]

Subject: [tip:core/iommu] swiotlb: change swiotlb_bus_to[phys,virt] prototypes

Commit-ID: 42d7c5e353cef9062129b0de3ec9ddf10567b9ca
Gitweb: http://git.kernel.org/tip/42d7c5e353cef9062129b0de3ec9ddf10567b9ca
Author: Becky Bruce <[email protected]>
AuthorDate: Wed, 8 Apr 2009 09:09:21 -0500
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 8 Apr 2009 16:18:38 +0200

swiotlb: change swiotlb_bus_to[phys,virt] prototypes

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 <[email protected]>
Acked-by: FUJITA Tomonori <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>

---
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 d912f06..bffe6d7 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,
@@ -691,7 +691,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
static void 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);

@@ -728,7 +728,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);

2009-04-08 20:38:42

by Christoph Hellwig

[permalink] [raw]

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

On Wed, Apr 08, 2009 at 09:09:18AM -0500, Kumar Gala wrote:
> From: Becky Bruce <[email protected]>
>
> 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.

2009-04-08 20:58:21

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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

2009-04-08 21:15:49

[permalink] [raw]

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

On Wed, 8 Apr 2009 15:56:32 -0500
Kumar Gala <[email protected]> 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 <[email protected]>
> >>
> >> 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.

2009-04-08 21:56:21

[permalink] [raw]

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

FUJITA Tomonori wrote:
> On Wed, 8 Apr 2009 15:56:32 -0500
> Kumar Gala <[email protected]> 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 <[email protected]>
>>>>
>>>> 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

2009-04-08 22:11:22

[permalink] [raw]

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

On Wed, 08 Apr 2009 14:55:55 -0700
Jeremy Fitzhardinge <[email protected]> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 8 Apr 2009 15:56:32 -0500
> > Kumar Gala <[email protected]> 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 <[email protected]>
> >>>>
> >>>> 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.

2009-04-08 22:24:47

by Christoph Hellwig

[permalink] [raw]

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

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.

2009-04-08 22:37:20

[permalink] [raw]

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

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

2009-04-08 23:02:20

[permalink] [raw]

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

On Wed, 08 Apr 2009 15:36:58 -0700
Jeremy Fitzhardinge <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2009-04-08 23:18:15

[permalink] [raw]

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

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

2009-04-08 23:38:18

[permalink] [raw]

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

On Wed, 08 Apr 2009 16:16:17 -0700
Jeremy Fitzhardinge <[email protected]> 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.

2009-04-09 00:09:42

[permalink] [raw]

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

FUJITA Tomonori wrote:
> On Wed, 08 Apr 2009 16:16:17 -0700
> Jeremy Fitzhardinge <[email protected]> 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

2009-04-09 04:45:09

[permalink] [raw]

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

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 <[email protected]> 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

2009-04-09 05:01:48

[permalink] [raw]

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

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

2009-04-09 18:36:27

[permalink] [raw]

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

On Wed, 08 Apr 2009 17:09:00 -0700
Jeremy Fitzhardinge <[email protected]> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 08 Apr 2009 16:16:17 -0700
> > Jeremy Fitzhardinge <[email protected]> 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.

2009-04-09 18:51:20

[permalink] [raw]

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

On Wed, 8 Apr 2009 23:59:18 -0500
Kumar Gala <[email protected]> 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?

2009-04-09 19:19:33

[permalink] [raw]

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

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

2009-04-09 19:44:55

[permalink] [raw]

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

On Thu, 09 Apr 2009 12:19:19 -0700
Jeremy Fitzhardinge <[email protected]> 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.

2009-04-09 19:51:19

[permalink] [raw]

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

2009-04-09 19:54:27

[permalink] [raw]

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

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

2009-04-09 20:12:18

[permalink] [raw]

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

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

> On Wed, 8 Apr 2009 23:59:18 -0500
> Kumar Gala <[email protected]> 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

2009-04-09 20:26:54