Re: [PATCH 0/3] x86: restore old GART alloc_coherent

Previous thread: What's in in the block git tree for 2.6.28? by Jens Axboe on Wednesday, September 24, 2008 - 4:47 am. (1 message)

Next thread: dmaengine: DMA_CTRL_ACK flag signification by Nicolas Ferre on Wednesday, September 24, 2008 - 4:59 am. (3 messages)
From: FUJITA Tomonori
Date: Wednesday, September 24, 2008 - 4:48 am

This pachset is against tip/iommu.

What this patchset does is restoring old GART alloc_coherent behavior
(before the alloc_coherent rewrite):

http://lkml.org/lkml/2008/8/12/200

Currently, GART alloc_coherent tries to allocate pages with GFP_DMA32
for a device having dma_masks > 24bit < 32bits. If GART gets an
address that a device can't access to, GART tries to map the address
to a virtual I/O address that the device can access to.

But Andi pointed out, "The GART is somewhere in the 4GB range so you
cannot use it to map anything < 4GB. Also GART is pretty small."

http://lkml.org/lkml/2008/9/12/43

So it's possible that GART doesn't have virtual I/O address space that
a device can access to. The current behavior might not work for a
device having dma_masks > 24bit < 32bits. This patchset restores old
GART alloc_coherent behavior, which doesn't use GART hardware (if an
user doesn't enable force_iommu option).


--

From: FUJITA Tomonori
Date: Wednesday, September 24, 2008 - 4:48 am

This patch exports nommu_alloc_coherent (renamed
dma_generic_alloc_coherent). GART needs this function.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-dma.c     |   31 +++++++++++++++++++++++++++++++
 arch/x86/kernel/pci-nommu.c   |   39 +--------------------------------------
 include/asm-x86/dma-mapping.h |    3 +++
 3 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 23882c4..0a3824e 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -134,6 +134,37 @@ unsigned long iommu_num_pages(unsigned long addr, unsigned long len)
 EXPORT_SYMBOL(iommu_num_pages);
 #endif
 
+void *dma_generic_alloc_coherent(struct device *dev, size_t size,
+				 dma_addr_t *dma_addr, gfp_t flag)
+{
+	unsigned long dma_mask;
+	struct page *page;
+	dma_addr_t addr;
+
+	dma_mask = dma_alloc_coherent_mask(dev, flag);
+
+	flag |= __GFP_ZERO;
+again:
+	page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
+	if (!page)
+		return NULL;
+
+	addr = page_to_phys(page);
+	if (!is_buffer_dma_capable(dma_mask, addr, size)) {
+		__free_pages(page, get_order(size));
+
+		if (dma_mask < DMA_32BIT_MASK && !(flag & GFP_DMA)) {
+			flag = (flag & ~GFP_DMA32) | GFP_DMA;
+			goto again;
+		}
+
+		return NULL;
+	}
+
+	*dma_addr = addr;
+	return page_address(page);
+}
+
 /*
  * See <Documentation/x86_64/boot-options.txt> for the iommu kernel parameter
  * documentation.
diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c
index 1c1c98a..c70ab5a 100644
--- a/arch/x86/kernel/pci-nommu.c
+++ b/arch/x86/kernel/pci-nommu.c
@@ -72,43 +72,6 @@ static int nommu_map_sg(struct device *hwdev, struct scatterlist *sg,
 	return nents;
 }
 
-static void *
-nommu_alloc_coherent(struct device *hwdev, size_t size,
-		     dma_addr_t *dma_addr, gfp_t gfp)
-{
-	unsigned long dma_mask;
-	int node;
-	struct page *page;
-	dma_addr_t ...
From: FUJITA Tomonori
Date: Wednesday, September 24, 2008 - 4:48 am

This reverts:

commit bee44f294efd8417f5e68553778a6cc957af1547
Author: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date:   Fri Sep 12 19:42:35 2008 +0900

    x86: make GART to respect device's dma_mask about virtual mappings

I wrote the above commit to fix a GART alloc_coherent regression, that
can't handle a device having dma_masks > 24bit < 32bits, introduced by
the alloc_coherent rewrite:

http://lkml.org/lkml/2008/8/12/200

After the alloc_coherent rewrite, GART alloc_coherent tried to
allocate pages with GFP_DMA32. If GART got an address that a device
can't access to, GART mapped the address to a virtual I/O address. But
GART mapping mechanism didn't take account of dma mask, so GART could
use a virtual I/O address that the device can't access to again.

The above commit modified GART mapping mechanism to take care of dma
mask. But Andi pointed out, "The GART is somewhere in the 4GB range so
you cannot use it to map anything < 4GB. Also GART is pretty small."

http://lkml.org/lkml/2008/9/12/43

That means it's possible that GART doesn't have virtual I/O address
space that a device can access to. The above commit (to modify GART
mapping mechanism to take care of dma mask) can't fix the regression
reliably so let's avoid making GART more complicated.

We need a solution that always works for dma_masks > 24bit <
32bits. That's how GART worked before the alloc_coherent rewrite.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-gart_64.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 9e390f1..7e08e46 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -83,34 +83,23 @@ static unsigned long next_bit;  /* protected by iommu_bitmap_lock */
 static int need_flush;		/* global flush state. set for each gart wrap */
 
 static unsigned long alloc_iommu(struct device ...
From: FUJITA Tomonori
Date: Wednesday, September 24, 2008 - 4:48 am

Currently, GART alloc_coherent tries to allocate pages with GFP_DMA32
for a device having dma_masks > 24bit < 32bits. If GART gets an
address that a device can't access to, GART try to map the address to
a virtual I/O address that the device can access to.

But Andi pointed out, "The GART is somewhere in the 4GB range so you
cannot use it to map anything < 4GB. Also GART is pretty small."

http://lkml.org/lkml/2008/9/12/43

That is, it's possible that GART doesn't have virtual I/O address
space that a device can access to. The above behavior doesn't work for
a device having dma_masks > 24bit < 32bits.

This patch restores old GART alloc_coherent behavior (before the
alloc_coherent rewrite).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-gart_64.c |   43 +++++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 7e08e46..25c94fb 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -487,31 +487,28 @@ static void *
 gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
 		    gfp_t flag)
 {
-	void *vaddr;
 	dma_addr_t paddr;
 	unsigned long align_mask;
-	u64 dma_mask = dma_alloc_coherent_mask(dev, flag);
-
-	vaddr = (void *)__get_free_pages(flag | __GFP_ZERO, get_order(size));
-	if (!vaddr)
-		return NULL;
-
-	paddr = virt_to_phys(vaddr);
-	if (is_buffer_dma_capable(dma_mask, paddr, size)) {
-		*dma_addr = paddr;
-		return vaddr;
-	}
-
-	align_mask = (1UL << get_order(size)) - 1;
-
-	*dma_addr = dma_map_area(dev, paddr, size, DMA_BIDIRECTIONAL,
-				 align_mask);
-	flush_gart();
-
-	if (*dma_addr != bad_dma_address)
-		return vaddr;
-
-	free_pages((unsigned long)vaddr, get_order(size));
+	struct page *page;
+
+	if (force_iommu && !(flag & GFP_DMA)) {
+		flag &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
+		page = alloc_pages(flag | __GFP_ZERO, ...
From: Joerg Roedel
Date: Wednesday, September 24, 2008 - 5:11 am

Patchset looks good in principle. But I have a small question, see

Can't we check if a mapping is required before calling dma_map_area?
Your recently introduced IOMMU helper functions should make that easy
and GART address space is a rare resource. AFAIR this is also the


-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

--

From: FUJITA Tomonori
Date: Wednesday, September 24, 2008 - 5:43 am

On Wed, 24 Sep 2008 14:11:27 +0200

I think that the behavior of the old generic dma_alloc_coherent
function and GART is different.

The old GART code does virtual mappings only with force_iommu option
enabled. The old GART code always do virtual mappings with force_iommu

Theoretically, yes, I think. But this patch restores the old GART code
behavior.
--

From: Joerg Roedel
Date: Wednesday, September 24, 2008 - 5:57 am

Hmm true, the old code called map_simple and not map_single so it mapped

Ok. The map_simple function of GART did it unconditionally too so this
is the old behavior. But its safe here to flush conditionally. I'll send
a follow-up patch.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

--

From: Alan Cox
Date: Wednesday, September 24, 2008 - 6:06 am

On Wed, 24 Sep 2008 20:48:37 +0900

Acked-by: Alan Cox <alan@redhat.com>

This is indeed a specific problem found with things like older AACRAID
where control blocks must be below 31bits and the GART is above 0x80000000

Alan
--

From: Andi Kleen
Date: Wednesday, September 24, 2008 - 3:01 pm

Thanks for changing it back.

Acked-by: Andi Kleen <ak@linux.intel.com>

-- 
ak@linux.intel.com
--

From: Joerg Roedel
Date: Wednesday, September 24, 2008 - 5:58 am

Acked-by: Joerg Roedel <joerg.roedel@amd.com>

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

--

From: Ingo Molnar
Date: Thursday, September 25, 2008 - 2:04 am

applied your fixes to tip/x86/iommu:

 1615965: x86 gart: remove unnecessary initialization
 1d99088: x86: restore old GART alloc_coherent behavior
 ecef533: revert "x86: make GART to respect device's dma_mask about virtual mappings"
 9f6ac57: x86: export pci-nommu's alloc_coherent

thanks Fujita!

	Ingo
--

Previous thread: What's in in the block git tree for 2.6.28? by Jens Axboe on Wednesday, September 24, 2008 - 4:47 am. (1 message)

Next thread: dmaengine: DMA_CTRL_ACK flag signification by Nicolas Ferre on Wednesday, September 24, 2008 - 4:59 am. (3 messages)