[PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[FUJITA Tomonori]

This patch fixes swiotlb to use dev->coherent_dma_mask in
alloc_coherent. Currently, swiotlb uses dev->dma_mask in
alloc_coherent but alloc_coherent is supposed to use
coherent_dma_mask. It could break drivers that uses smaller
coherent_dma_mask than dma_mask (though the current code works for the
majority that use the same mask for coherent_dma_mask and dma_mask).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 lib/swiotlb.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 78330c3..5f6c629 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -467,9 +467,13 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	dma_addr_t dev_addr;
 	void *ret;
 	int order = get_order(size);
+	u64 dma_mask = DMA_32BIT_MASK;
+
+	if (hwdev && hwdev->coherent_dma_mask)
+		dma_mask = hwdev->coherent_dma_mask;
 
 	ret = (void *)__get_free_pages(flags, order);
-	if (ret && address_needs_mapping(hwdev, virt_to_bus(ret), size)) {
+	if (ret && !is_buffer_dma_capable(dma_mask, virt_to_bus(ret), size)) {
 		/*
 		 * The allocated memory isn't reachable by the device.
 		 * Fall back on swiotlb_map_single().
@@ -493,9 +497,9 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	dev_addr = virt_to_bus(ret);
 
 	/* Confirm address can be DMA'd by device */
-	if (address_needs_mapping(hwdev, dev_addr, size)) {
+	if (!is_buffer_dma_capable(dma_mask, dev_addr, size)) {
 		printk("hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016Lx\n",
-		       (unsigned long long)*hwdev->dma_mask,
+		       (unsigned long long)dma_mask,
 		       (unsigned long long)dev_addr);
 
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
-- 
1.5.5.GIT

Re: [PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[Ingo Molnar]

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

> This patch fixes swiotlb to use dev->coherent_dma_mask in 
> alloc_coherent. Currently, swiotlb uses dev->dma_mask in 
> alloc_coherent but alloc_coherent is supposed to use 
> coherent_dma_mask. It could break drivers that uses smaller 
> coherent_dma_mask than dma_mask (though the current code works for 
> the majority that use the same mask for coherent_dma_mask and 
> dma_mask).
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  lib/swiotlb.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)

Applied it with the changelog below to tip/core/urgent, thanks!

I also flagged it for v2.6.28 inclusion. This bug was caused by the 
removal of the GFP_DMA hack in swiotlb_alloc_coherent() in this cycle. 
I havent seen it actually reported anywhere - have you perhaps?Or have 
you found this via code review?

Do we know roughly the range of devices/systems where there's a real 
address range that cannot be DMA-ed to coherently, and an estimation 
about how frequently they would be affected by this bug?

	Ingo

--------------->
From 1e74f3000b86969de421ca0da08f42e7d21cbd99 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Mon, 17 Nov 2008 16:24:34 +0900
Subject: [PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

Impact: fix DMA buffer allocation coherency bug in certain configs

This patch fixes swiotlb to use dev->coherent_dma_mask in
swiotlb_alloc_coherent().

coherent_dma_mask is a subset of dma_mask (equal to it most of
the time), enumerating the address range that a given device
is able to DMA to/from in a cache-coherent way.

But currently, swiotlb uses dev->dma_mask in alloc_coherent()
implicitly via address_needs_mapping(), but alloc_coherent is really
supposed to use coherent_dma_mask.

This bug could break drivers that uses smaller coherent_dma_mask than
dma_mask (though the current code works for the majority that use the
same mask for coherent_dma_mask and dma_mask).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: tony.luck@intel.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 lib/swiotlb.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 78330c3..5f6c629 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -467,9 +467,13 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	dma_addr_t dev_addr;
 	void *ret;
 	int order = get_order(size);
+	u64 dma_mask = DMA_32BIT_MASK;
+
+	if (hwdev && hwdev->coherent_dma_mask)
+		dma_mask = hwdev->coherent_dma_mask;
 
 	ret = (void *)__get_free_pages(flags, order);
-	if (ret && address_needs_mapping(hwdev, virt_to_bus(ret), size)) {
+	if (ret && !is_buffer_dma_capable(dma_mask, virt_to_bus(ret), size)) {
 		/*
 		 * The allocated memory isn't reachable by the device.
 		 * Fall back on swiotlb_map_single().
@@ -493,9 +497,9 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	dev_addr = virt_to_bus(ret);
 
 	/* Confirm address can be DMA'd by device */
-	if (address_needs_mapping(hwdev, dev_addr, size)) {
+	if (!is_buffer_dma_capable(dma_mask, dev_addr, size)) {
 		printk("hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016Lx\n",
-		       (unsigned long long)*hwdev->dma_mask,
+		       (unsigned long long)dma_mask,
 		       (unsigned long long)dev_addr);
 
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */

Re: [PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[FUJITA Tomonori]

On Mon, 17 Nov 2008 09:15:26 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > This patch fixes swiotlb to use dev->coherent_dma_mask in 
> > alloc_coherent. Currently, swiotlb uses dev->dma_mask in 
> > alloc_coherent but alloc_coherent is supposed to use 
> > coherent_dma_mask. It could break drivers that uses smaller 
> > coherent_dma_mask than dma_mask (though the current code works for 
> > the majority that use the same mask for coherent_dma_mask and 
> > dma_mask).
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  lib/swiotlb.c |   10 +++++++---
> >  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> Applied it with the changelog below to tip/core/urgent, thanks!
> 
> I also flagged it for v2.6.28 inclusion. This bug was caused by the 
> removal of the GFP_DMA hack in swiotlb_alloc_coherent() in this cycle. 
> I havent seen it actually reported anywhere - have you perhaps?Or have 
> you found this via code review?

This wasn't introduced by the removal of the GFP_DMA hack. It has been
for ages, I think.

I knew this issue but I thought that it's harmless and let it
alone. But Grant Grundler said that there are some devices are
troubled by this:

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

I fixed VT-d about this (bb9e6d65078da2f38cfe1067cfd31a896ca867c0) but
somehow I forgot about swiotlb.

I think that it would be fine to push this to 2.6.29 since seems that
nobody hits this. But it's also fine to push it for 2.6.28 since it's
theoretically a bug fix and pretty trivial.


> Do we know roughly the range of devices/systems where there's a real 
> address range that cannot be DMA-ed to coherently, and an estimation 
> about how frequently they would be affected by this bug?

I think that if a driver hits this bug, it's likely that an user sees
kinda data corruption right after loading the driver.

Re: [PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[Ingo Molnar]

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

> On Mon, 17 Nov 2008 09:15:26 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > This patch fixes swiotlb to use dev->coherent_dma_mask in 
> > > alloc_coherent. Currently, swiotlb uses dev->dma_mask in 
> > > alloc_coherent but alloc_coherent is supposed to use 
> > > coherent_dma_mask. It could break drivers that uses smaller 
> > > coherent_dma_mask than dma_mask (though the current code works for 
> > > the majority that use the same mask for coherent_dma_mask and 
> > > dma_mask).
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > ---
> > >  lib/swiotlb.c |   10 +++++++---
> > >  1 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > Applied it with the changelog below to tip/core/urgent, thanks!
> > 
> > I also flagged it for v2.6.28 inclusion. This bug was caused by the 
> > removal of the GFP_DMA hack in swiotlb_alloc_coherent() in this cycle. 
> > I havent seen it actually reported anywhere - have you perhaps?Or have 
> > you found this via code review?
> 
> This wasn't introduced by the removal of the GFP_DMA hack. It has 
> been for ages, I think.

Yeah, what i mean is that our GFP_DMA hack (which we indeed had for 
years) definitely _hid_ the problem: on x86 for example it limits 
coherent DMA buffers into the DMA zone: the first 16 MB of RAM.

( Other platforms are pretty narrow about GFP_DMA too - it implies at 
  least DMA32 which is in practice often the real limit for 
  cache-coherent DMA addresses. )

So the removal of GFP_DMA flag from coherent allocations exposed us to 
this long-standing (but hidden) problem.

( And it doesnt matter that the underlying problem has been there for
  years - what matters to regression engineering is how users are
  affected by changes. )

It's nice that you noticed and fixed it, and please be on the watchout 
for such patterns in the future too and try to move fixes to the 
urgent track in such cases. Had we missed the scope of this we could 
have released v2.6.28 with a data corruptor bug on certain 
devices/systems.

> I knew this issue but I thought that it's harmless and let it alone. 
> But Grant Grundler said that there are some devices are troubled by 
> this:
> 
> http://marc.info/?l=linux-kernel&m\x122379585203173&w=2

ok, so it can affect real devices, as suspected.

> I fixed VT-d about this (bb9e6d65078da2f38cfe1067cfd31a896ca867c0) 
> but somehow I forgot about swiotlb.
> 
> I think that it would be fine to push this to 2.6.29 since seems 
> that nobody hits this. But it's also fine to push it for 2.6.28 
> since it's theoretically a bug fix and pretty trivial.
>
> > Do we know roughly the range of devices/systems where there's a 
> > real address range that cannot be DMA-ed to coherently, and an 
> > estimation about how frequently they would be affected by this 
> > bug?
> 
> I think that if a driver hits this bug, it's likely that an user 
> sees kinda data corruption right after loading the driver.

Correct - hence definitely .28 material. We'd try to fix such a bug in 
.28 even if it was a much more complex fix - or we'd have reverted the 
original change that exposed the problem.

	Ingo

Re: [PATCH] swiotlb: use coherent_dma_mask in alloc_coherent

[FUJITA Tomonori]

On Mon, 17 Nov 2008 10:02:59 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > On Mon, 17 Nov 2008 09:15:26 +0100
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > 
> > > * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > This patch fixes swiotlb to use dev->coherent_dma_mask in 
> > > > alloc_coherent. Currently, swiotlb uses dev->dma_mask in 
> > > > alloc_coherent but alloc_coherent is supposed to use 
> > > > coherent_dma_mask. It could break drivers that uses smaller 
> > > > coherent_dma_mask than dma_mask (though the current code works for 
> > > > the majority that use the same mask for coherent_dma_mask and 
> > > > dma_mask).
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > ---
> > > >  lib/swiotlb.c |   10 +++++++---
> > > >  1 files changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > Applied it with the changelog below to tip/core/urgent, thanks!
> > > 
> > > I also flagged it for v2.6.28 inclusion. This bug was caused by the 
> > > removal of the GFP_DMA hack in swiotlb_alloc_coherent() in this cycle. 
> > > I havent seen it actually reported anywhere - have you perhaps?Or have 
> > > you found this via code review?
> > 
> > This wasn't introduced by the removal of the GFP_DMA hack. It has 
> > been for ages, I think.
> 
> Yeah, what i mean is that our GFP_DMA hack (which we indeed had for 
> years) definitely _hid_ the problem: on x86 for example it limits 
> coherent DMA buffers into the DMA zone: the first 16 MB of RAM.

Ah, I see. I misunderstood what you meant.


> ( Other platforms are pretty narrow about GFP_DMA too - it implies at 
>   least DMA32 which is in practice often the real limit for 
>   cache-coherent DMA addresses. )
> 
> So the removal of GFP_DMA flag from coherent allocations exposed us to 
> this long-standing (but hidden) problem.

Yes, you are right. But it's pretty hard to hit this bug since x86_64
rarely uses swiotlb_alloc_coherent. pci-swiotlb_64.c tries
dma_generic_alloc_coherent first and then, only if it fails, it uses
swiotlb_alloc_coherent. dma_generic_alloc_coherent usually succeeds.

Before removing the GFP_DMA hack (the dma_alloc_coherent rewrite),
x86_64 rarely used swiotlb_alloc_coherent too because
swiotlb_alloc_coherent was used only if dma_alloc_coherent fails.
This issue was hidden by dma_alloc_coherent and GFP_DMA hack.