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 <[email protected]>
---
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
* FUJITA Tomonori <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Cc: [email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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 */
On Mon, 17 Nov 2008 09:15:26 +0100
Ingo Molnar <[email protected]> wrote:
>
> * FUJITA Tomonori <[email protected]> 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 <[email protected]>
> > ---
> > 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=122379585203173&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.
* FUJITA Tomonori <[email protected]> wrote:
> On Mon, 17 Nov 2008 09:15:26 +0100
> Ingo Molnar <[email protected]> wrote:
>
> >
> > * FUJITA Tomonori <[email protected]> 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 <[email protected]>
> > > ---
> > > 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=122379585203173&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
On Mon, 17 Nov 2008 10:02:59 +0100
Ingo Molnar <[email protected]> wrote:
>
> * FUJITA Tomonori <[email protected]> wrote:
>
> > On Mon, 17 Nov 2008 09:15:26 +0100
> > Ingo Molnar <[email protected]> wrote:
> >
> > >
> > > * FUJITA Tomonori <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.