[PATCH 0/7] x86 dma_*_coherent rework patchset

[PATCH 0/7] x86 dma_*_coherent rework patchset

[Joerg Roedel]

Hi,

this patchset reworks the dma_*_coherent functions in the DMA layer for the x86
architecture. The patch series extends the existing DMA backends with missing
*coherent callbacks and simplifies the generic function to basically only call
the registered backend. This allows future optimizations in hardware specific
IOMMU implementations.
The code ist tested on AMD64 with AMD IOMMU and GART as well as on my old 486
box. It is not yet tested on a Calgary IOMMU system.

Joerg

git diff --stat tip/master:

 arch/x86/kernel/amd_iommu.c      |    2 -
 arch/x86/kernel/pci-calgary_64.c |   14 +++++
 arch/x86/kernel/pci-dma.c        |  116 +++----------------------------------
 arch/x86/kernel/pci-gart_64.c    |   31 ++++++++++
 arch/x86/kernel/pci-nommu.c      |   45 +++++++++++++++
 5 files changed, 100 insertions(+), 108 deletions(-)

[PATCH 1/7] x86: add alloc_coherent dma_ops callback to GART driver

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-gart_64.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index cdab678..55cc388 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -499,6 +499,26 @@ error:
 	return 0;
 }
 
+/* allocate and map a coherent mapping */
+static void *
+gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
+		    gfp_t flag)
+{
+	void *vaddr;
+
+	vaddr = (void *)__get_free_pages(flag, get_order(size));
+	if (!vaddr)
+		return NULL;
+
+	*dma_addr = gart_map_single(dev, __pa(vaddr), size, DMA_BIDIRECTIONAL);
+	if (*dma_addr != bad_dma_address)
+		return vaddr;
+
+	free_pages((unsigned long)vaddr, get_order(size));
+
+	return NULL;
+}
+
 static int no_agp;
 
 static __init unsigned long check_iommu_size(unsigned long aper, u64 aper_size)
@@ -701,6 +721,7 @@ static struct dma_mapping_ops gart_dma_ops = {
 	.sync_sg_for_device		= NULL,
 	.map_sg				= gart_map_sg,
 	.unmap_sg			= gart_unmap_sg,
+	.alloc_coherent			= gart_alloc_coherent,
 };
 
 void gart_iommu_shutdown(void)
-- 
1.5.3.7

Re: [PATCH 1/7] x86: add alloc_coherent dma_ops callback to GART driver

[FUJITA Tomonori]

On Tue, 12 Aug 2008 17:24:11 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kernel/pci-gart_64.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> index cdab678..55cc388 100644
> --- a/arch/x86/kernel/pci-gart_64.c
> +++ b/arch/x86/kernel/pci-gart_64.c
> @@ -499,6 +499,26 @@ error:
>  	return 0;
>  }
>  
> +/* allocate and map a coherent mapping */
> +static void *
> +gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
> +		    gfp_t flag)
> +{
> +	void *vaddr;
> +
> +	vaddr = (void *)__get_free_pages(flag, get_order(size));
> +	if (!vaddr)
> +		return NULL;
> +
> +	*dma_addr = gart_map_single(dev, __pa(vaddr), size, DMA_BIDIRECTIONAL);
> +	if (*dma_addr != bad_dma_address)
> +		return vaddr;
> +
> +	free_pages((unsigned long)vaddr, get_order(size));
> +
> +	return NULL;
> +}
> +
>  static int no_agp;

It would be better to return a size-aligned memory as DMA-mapping.txt
says (though I don't think that it doesn't matter much):

http://lkml.org/lkml/2008/8/8/555


I also think that x86 IOMMUs need to handle DMA_*BIT_MASK properly,
don't we?

Re: [PATCH 1/7] x86: add alloc_coherent dma_ops callback to GART driver

[Joerg Roedel]

On Wed, Aug 13, 2008 at 09:45:54AM +0900, FUJITA Tomonori wrote:
> On Tue, 12 Aug 2008 17:24:11 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kernel/pci-gart_64.c |   21 +++++++++++++++++++++
> >  1 files changed, 21 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> > index cdab678..55cc388 100644
> > --- a/arch/x86/kernel/pci-gart_64.c
> > +++ b/arch/x86/kernel/pci-gart_64.c
> > @@ -499,6 +499,26 @@ error:
> >  	return 0;
> >  }
> >  
> > +/* allocate and map a coherent mapping */
> > +static void *
> > +gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
> > +		    gfp_t flag)
> > +{
> > +	void *vaddr;
> > +
> > +	vaddr = (void *)__get_free_pages(flag, get_order(size));
> > +	if (!vaddr)
> > +		return NULL;
> > +
> > +	*dma_addr = gart_map_single(dev, __pa(vaddr), size, DMA_BIDIRECTIONAL);
> > +	if (*dma_addr != bad_dma_address)
> > +		return vaddr;
> > +
> > +	free_pages((unsigned long)vaddr, get_order(size));
> > +
> > +	return NULL;
> > +}
> > +
> >  static int no_agp;
> 
> It would be better to return a size-aligned memory as DMA-mapping.txt
> says (though I don't think that it doesn't matter much):
> 
> http://lkml.org/lkml/2008/8/8/555

Agreed. I try to change the patchset so it returns size aligned dma
addresses.
 
> I also think that x86 IOMMUs need to handle DMA_*BIT_MASK properly,
> don't we?

Shouldn't this be done by the IOMMUs using your iommu_area_alloc()
function? Or do I misunderstand something?

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

Re: [PATCH 1/7] x86: add alloc_coherent dma_ops callback to GART driver

[FUJITA Tomonori]

On Wed, 13 Aug 2008 14:46:36 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Wed, Aug 13, 2008 at 09:45:54AM +0900, FUJITA Tomonori wrote:
> > On Tue, 12 Aug 2008 17:24:11 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > > ---
> > >  arch/x86/kernel/pci-gart_64.c |   21 +++++++++++++++++++++
> > >  1 files changed, 21 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> > > index cdab678..55cc388 100644
> > > --- a/arch/x86/kernel/pci-gart_64.c
> > > +++ b/arch/x86/kernel/pci-gart_64.c
> > > @@ -499,6 +499,26 @@ error:
> > >  	return 0;
> > >  }
> > >  
> > > +/* allocate and map a coherent mapping */
> > > +static void *
> > > +gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
> > > +		    gfp_t flag)
> > > +{
> > > +	void *vaddr;
> > > +
> > > +	vaddr = (void *)__get_free_pages(flag, get_order(size));
> > > +	if (!vaddr)
> > > +		return NULL;
> > > +
> > > +	*dma_addr = gart_map_single(dev, __pa(vaddr), size, DMA_BIDIRECTIONAL);
> > > +	if (*dma_addr != bad_dma_address)
> > > +		return vaddr;
> > > +
> > > +	free_pages((unsigned long)vaddr, get_order(size));
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > >  static int no_agp;
> > 
> > It would be better to return a size-aligned memory as DMA-mapping.txt
> > says (though I don't think that it doesn't matter much):
> > 
> > http://lkml.org/lkml/2008/8/8/555
> 
> Agreed. I try to change the patchset so it returns size aligned dma
> addresses.
>  
> > I also think that x86 IOMMUs need to handle DMA_*BIT_MASK properly,
> > don't we?
> 
> Shouldn't this be done by the IOMMUs using your iommu_area_alloc()
> function? Or do I misunderstand something?

iommu_area_alloc works but IOMMUs need to use it
properly. arch/powerpc/kernel/iommu.c is a good example.

[PATCH 2/7] x86: add free_coherent dma_ops callback to GART driver

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-gart_64.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 55cc388..18db09b 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -519,6 +519,15 @@ gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
 	return NULL;
 }
 
+/* free a coherent mapping */
+static void
+gart_free_coherent(struct device *dev, size_t size, void *vaddr,
+		   dma_addr_t dma_addr)
+{
+	gart_unmap_single(dev, dma_addr, size, DMA_BIDIRECTIONAL);
+	free_pages((unsigned long)vaddr, get_order(size));
+}
+
 static int no_agp;
 
 static __init unsigned long check_iommu_size(unsigned long aper, u64 aper_size)
@@ -722,6 +731,7 @@ static struct dma_mapping_ops gart_dma_ops = {
 	.map_sg				= gart_map_sg,
 	.unmap_sg			= gart_unmap_sg,
 	.alloc_coherent			= gart_alloc_coherent,
+	.free_coherent			= gart_free_coherent,
 };
 
 void gart_iommu_shutdown(void)
-- 
1.5.3.7

Re: [PATCH 2/7] x86: add free_coherent dma_ops callback to GART driver

[FUJITA Tomonori]

On Tue, 12 Aug 2008 17:24:12 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kernel/pci-gart_64.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> index 55cc388..18db09b 100644
> --- a/arch/x86/kernel/pci-gart_64.c
> +++ b/arch/x86/kernel/pci-gart_64.c

It would be better to foil this to the first patch, I think. Any
reasonable reason to add alloc_coherent and free_coherent with two
separate patches?

I think that you can remove map_simple in gart (and please don't
forget to remove map_simple in struct dma_mapping_ops. I think only
GART uses that hook).

Re: [PATCH 2/7] x86: add free_coherent dma_ops callback to GART driver

[Joerg Roedel]

On Wed, Aug 13, 2008 at 09:45:53AM +0900, FUJITA Tomonori wrote:
> On Tue, 12 Aug 2008 17:24:12 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kernel/pci-gart_64.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
> > index 55cc388..18db09b 100644
> > --- a/arch/x86/kernel/pci-gart_64.c
> > +++ b/arch/x86/kernel/pci-gart_64.c
> 
> It would be better to foil this to the first patch, I think. Any
> reasonable reason to add alloc_coherent and free_coherent with two
> separate patches?

Yes possible. Its always a bit hard to split the patches correctly. Some
maintainers prefer small patches and for others its split up too much
then. If I am in doubt I often chose to split a patch.

> I think that you can remove map_simple in gart (and please don't
> forget to remove map_simple in struct dma_mapping_ops. I think only
> GART uses that hook).

Ok, I will check that and send a sperate 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

[PATCH 3/7] x86: add free_coherent dma_ops callback to Calgary IOMMU driver

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-calgary_64.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
index b24f1e8..6b67446 100644
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -510,8 +510,22 @@ error:
 	return ret;
 }
 
+static void calgary_free_coherent(struct device *dev, size_t size,
+				  void *vaddr, dma_addr_t dma_handle)
+{
+	unsigned int npages;
+	struct iommu_table *tbl = find_iommu_table(dev);
+
+	size = PAGE_ALIGN(size);
+	npages = size >> PAGE_SHIFT;
+
+	iommu_free(tbl, dma_handle, npages);
+	free_pages((unsigned long)vaddr, get_order(size));
+}
+
 static struct dma_mapping_ops calgary_dma_ops = {
 	.alloc_coherent = calgary_alloc_coherent,
+	.free_coherent = calgary_free_coherent,
 	.map_single = calgary_map_single,
 	.unmap_single = calgary_unmap_single,
 	.map_sg = calgary_map_sg,
-- 
1.5.3.7

Re: [PATCH 3/7] x86: add free_coherent dma_ops callback to Calgary IOMMU driver

[Muli Ben-Yehuda]

On Tue, Aug 12, 2008 at 05:24:13PM +0200, Joerg Roedel wrote:

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

Acked-by: Muli Ben-Yehuda <muli@il.ibm.com>

Cheers,
Muli
-- 
Workshop on I/O Virtualization (WIOV '08)
Co-located with OSDI '08, Dec 2008, San Diego, CA
http://www.usenix.org/wiov08

[PATCH 4/7] x86: add alloc_coherent dma_ops callback to NOMMU driver

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-nommu.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c
index 3f91f71..4d8cde3 100644
--- a/arch/x86/kernel/pci-nommu.c
+++ b/arch/x86/kernel/pci-nommu.c
@@ -72,7 +72,45 @@ 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;
+
+	if (hwdev->dma_mask == NULL)
+		return NULL;
+
+	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
+	gfp |= __GFP_ZERO;
+
+	dma_mask = hwdev->coherent_dma_mask;
+	if (!dma_mask)
+		dma_mask = *(hwdev->dma_mask);
+
+	if (dma_mask <= DMA_24BIT_MASK)
+		gfp |= GFP_DMA;
+	else if (dma_mask <= DMA_32BIT_MASK)
+		gfp |= GFP_DMA32;
+
+	node = dev_to_node(hwdev);
+	page = alloc_pages_node(node, gfp, get_order(size));
+	if (!page)
+		return NULL;
+
+	*dma_addr = page_to_phys(page);
+	if (check_addr("alloc_coherent", hwdev, *dma_addr, size))
+		return page_address(page);
+
+	free_pages((unsigned long)page_address(page), get_order(size));
+
+	return NULL;
+}
+
 struct dma_mapping_ops nommu_dma_ops = {
+	.alloc_coherent = nommu_alloc_coherent,
 	.map_single = nommu_map_single,
 	.map_sg = nommu_map_sg,
 	.is_phys = 1,
-- 
1.5.3.7

Re: [PATCH 4/7] x86: add alloc_coherent dma_ops callback to NOMMU driver

[Joerg Roedel]

On Tue, Aug 12, 2008 at 05:24:14PM +0200, Joerg Roedel wrote:
> +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;
> +
> +	if (hwdev->dma_mask == NULL)
> +		return NULL;
> +
> +	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
> +	gfp |= __GFP_ZERO;
> +
> +	dma_mask = hwdev->coherent_dma_mask;
> +	if (!dma_mask)
> +		dma_mask = *(hwdev->dma_mask);
> +
> +	if (dma_mask <= DMA_24BIT_MASK)
> +		gfp |= GFP_DMA;
> +	else if (dma_mask <= DMA_32BIT_MASK)
> +		gfp |= GFP_DMA32;

Argh. But there is a bug in this logic :-(
I fix it and resend.

Joerg

[PATCH 5/7] x86: add free_coherent dma_ops callback to NOMMU driver

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-nommu.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c
index 4d8cde3..f4ad3e7 100644
--- a/arch/x86/kernel/pci-nommu.c
+++ b/arch/x86/kernel/pci-nommu.c
@@ -109,8 +109,15 @@ nommu_alloc_coherent(struct device *hwdev, size_t size,
 	return NULL;
 }
 
+static void nommu_free_coherent(struct device *dev, size_t size, void *vaddr,
+				dma_addr_t dma_addr)
+{
+	free_pages((unsigned long)vaddr, get_order(size));
+}
+
 struct dma_mapping_ops nommu_dma_ops = {
 	.alloc_coherent = nommu_alloc_coherent,
+	.free_coherent = nommu_free_coherent,
 	.map_single = nommu_map_single,
 	.map_sg = nommu_map_sg,
 	.is_phys = 1,
-- 
1.5.3.7

[PATCH 6/7] x86: cleanup dma_*_coherent functions

[Joerg Roedel]

All dma_ops implementations support the alloc_coherent and free_coherent
callbacks now. This allows a big simplification of the dma_alloc_coherent
function which is done with this patch. The dma_free_coherent functions is also
cleaned up and calls now the free_coherent callback of the dma_ops
implementation.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/pci-dma.c |  116 ++++-----------------------------------------
 1 files changed, 10 insertions(+), 106 deletions(-)

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index f704cb5..60fa80d 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -241,17 +241,6 @@ int dma_supported(struct device *dev, u64 mask)
 }
 EXPORT_SYMBOL(dma_supported);
 
-/* Allocate DMA memory on node near device */
-static noinline struct page *
-dma_alloc_pages(struct device *dev, gfp_t gfp, unsigned order)
-{
-	int node;
-
-	node = dev_to_node(dev);
-
-	return alloc_pages_node(node, gfp, order);
-}
-
 /*
  * Allocate memory for a coherent mapping.
  */
@@ -260,14 +249,7 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
 		   gfp_t gfp)
 {
 	struct dma_mapping_ops *ops = get_dma_ops(dev);
-	void *memory = NULL;
-	struct page *page;
-	unsigned long dma_mask = 0;
-	dma_addr_t bus;
-	int noretry = 0;
-
-	/* ignore region specifiers */
-	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
+	void *memory;
 
 	if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
 		return memory;
@@ -276,90 +258,12 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
 		dev = &fallback_dev;
 		gfp |= GFP_DMA;
 	}
-	dma_mask = dev->coherent_dma_mask;
-	if (dma_mask == 0)
-		dma_mask = (gfp & GFP_DMA) ? DMA_24BIT_MASK : DMA_32BIT_MASK;
-
-	/* Device not DMA able */
-	if (dev->dma_mask == NULL)
-		return NULL;
-
-	/* Don't invoke OOM killer or retry in lower 16MB DMA zone */
-	if (gfp & __GFP_DMA)
-		noretry = 1;
-
-#ifdef CONFIG_X86_64
-	/* Why <=? Even when the mask is smaller than 4GB it is often
-	   larger than 16MB and in this case we have a chance of
-	   finding fitting memory in the next higher zone first. If
-	   not retry with true GFP_DMA. -AK */
-	if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA)) {
-		gfp |= GFP_DMA32;
-		if (dma_mask < DMA_32BIT_MASK)
-			noretry = 1;
-	}
-#endif
-
- again:
-	page = dma_alloc_pages(dev,
-		noretry ? gfp | __GFP_NORETRY : gfp, get_order(size));
-	if (page == NULL)
-		return NULL;
-
-	{
-		int high, mmu;
-		bus = page_to_phys(page);
-		memory = page_address(page);
-		high = (bus + size) >= dma_mask;
-		mmu = high;
-		if (force_iommu && !(gfp & GFP_DMA))
-			mmu = 1;
-		else if (high) {
-			free_pages((unsigned long)memory,
-				   get_order(size));
-
-			/* Don't use the 16MB ZONE_DMA unless absolutely
-			   needed. It's better to use remapping first. */
-			if (dma_mask < DMA_32BIT_MASK && !(gfp & GFP_DMA)) {
-				gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
-				goto again;
-			}
-
-			/* Let low level make its own zone decisions */
-			gfp &= ~(GFP_DMA32|GFP_DMA);
-
-			if (ops->alloc_coherent)
-				return ops->alloc_coherent(dev, size,
-							   dma_handle, gfp);
-			return NULL;
-		}
-
-		memset(memory, 0, size);
-		if (!mmu) {
-			*dma_handle = bus;
-			return memory;
-		}
-	}
-
-	if (ops->alloc_coherent) {
-		free_pages((unsigned long)memory, get_order(size));
-		gfp &= ~(GFP_DMA|GFP_DMA32);
-		return ops->alloc_coherent(dev, size, dma_handle, gfp);
-	}
-
-	if (ops->map_simple) {
-		*dma_handle = ops->map_simple(dev, virt_to_phys(memory),
-					      size,
-					      PCI_DMA_BIDIRECTIONAL);
-		if (*dma_handle != bad_dma_address)
-			return memory;
-	}
 
-	if (panic_on_overflow)
-		panic("dma_alloc_coherent: IOMMU overflow by %lu bytes\n",
-		      (unsigned long)size);
-	free_pages((unsigned long)memory, get_order(size));
+	if (ops->alloc_coherent)
+		return ops->alloc_coherent(dev, size,
+				dma_handle, gfp);
 	return NULL;
+
 }
 EXPORT_SYMBOL(dma_alloc_coherent);
 
@@ -372,13 +276,13 @@ void dma_free_coherent(struct device *dev, size_t size,
 {
 	struct dma_mapping_ops *ops = get_dma_ops(dev);
 
-	int order = get_order(size);
 	WARN_ON(irqs_disabled());	/* for portability */
-	if (dma_release_from_coherent(dev, order, vaddr))
+
+	if (dma_release_from_coherent(dev, get_order(size), vaddr))
 		return;
-	if (ops->unmap_single)
-		ops->unmap_single(dev, bus, size, 0);
-	free_pages((unsigned long)vaddr, order);
+
+	if (ops->free_coherent)
+		ops->free_coherent(dev, size, vaddr, bus);
 }
 EXPORT_SYMBOL(dma_free_coherent);
 
-- 
1.5.3.7

Re: [PATCH 6/7] x86: cleanup dma_*_coherent functions

[FUJITA Tomonori]

On Tue, 12 Aug 2008 17:24:16 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> All dma_ops implementations support the alloc_coherent and free_coherent
> callbacks now. This allows a big simplification of the dma_alloc_coherent
> function which is done with this patch. The dma_free_coherent functions is also
> cleaned up and calls now the free_coherent callback of the dma_ops
> implementation.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/kernel/pci-dma.c |  116 ++++-----------------------------------------
>  1 files changed, 10 insertions(+), 106 deletions(-)
> 
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index f704cb5..60fa80d 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c

How about moving dma_free_coherent and dma_alloc_coherent to
asm-x86/dma-mapping.h? It would be nice to have all the dma operations
in one place.

Re: [PATCH 6/7] x86: cleanup dma_*_coherent functions

[Joerg Roedel]

On Wed, Aug 13, 2008 at 09:45:51AM +0900, FUJITA Tomonori wrote:
> On Tue, 12 Aug 2008 17:24:16 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > All dma_ops implementations support the alloc_coherent and free_coherent
> > callbacks now. This allows a big simplification of the dma_alloc_coherent
> > function which is done with this patch. The dma_free_coherent functions is also
> > cleaned up and calls now the free_coherent callback of the dma_ops
> > implementation.
> > 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  arch/x86/kernel/pci-dma.c |  116 ++++-----------------------------------------
> >  1 files changed, 10 insertions(+), 106 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> > index f704cb5..60fa80d 100644
> > --- a/arch/x86/kernel/pci-dma.c
> > +++ b/arch/x86/kernel/pci-dma.c
> 
> How about moving dma_free_coherent and dma_alloc_coherent to
> asm-x86/dma-mapping.h? It would be nice to have all the dma operations
> in one place.

Actually I thought about the other direction. Having these functions not
inlined should not add a big overhead but reduces kernel code size.

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

[PATCH 7/7] x86, AMD IOMMU: remove obsolete FIXME comment

[Joerg Roedel]

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 22d7d05..61d2d59 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1035,8 +1035,6 @@ out:
 
 /*
  * The exported free_coherent function for dma_ops.
- * FIXME: fix the generic x86 DMA layer so that it actually calls that
- *        function.
  */
 static void free_coherent(struct device *dev, size_t size,
 			  void *virt_addr, dma_addr_t dma_addr)
-- 
1.5.3.7

Re: [PATCH 0/7] x86 dma_*_coherent rework patchset

[Muli Ben-Yehuda]

[added Andi to CC]

On Tue, Aug 12, 2008 at 05:24:10PM +0200, Joerg Roedel wrote:
> Hi,
> 
> this patchset reworks the dma_*_coherent functions in the DMA layer
> for the x86 architecture. The patch series extends the existing DMA
> backends with missing *coherent callbacks and simplifies the generic
> function to basically only call the registered backend. This allows
> future optimizations in hardware specific IOMMU implementations.
> The code ist tested on AMD64 with AMD IOMMU and GART as well as on
> my old 486 box. It is not yet tested on a Calgary IOMMU system.

Now it is---appears to work fine on a Calgary system.

In general the patchset looks good and is definitely a step in the
right direction. I am a bit concerned about the contortions that the
generic dma_alloc_coherent went through before calling the ops
version---have you verified they are no longer needed?

Cheers,
Muli
-- 
Workshop on I/O Virtualization (WIOV '08)
Co-located with OSDI '08, Dec 2008, San Diego, CA
http://www.usenix.org/wiov08

Re: [PATCH 0/7] x86 dma_*_coherent rework patchset

[Joerg Roedel]

On Tue, Aug 12, 2008 at 07:06:58PM +0300, Muli Ben-Yehuda wrote:
> [added Andi to CC]
> 
> On Tue, Aug 12, 2008 at 05:24:10PM +0200, Joerg Roedel wrote:
> > Hi,
> > 
> > this patchset reworks the dma_*_coherent functions in the DMA layer
> > for the x86 architecture. The patch series extends the existing DMA
> > backends with missing *coherent callbacks and simplifies the generic
> > function to basically only call the registered backend. This allows
> > future optimizations in hardware specific IOMMU implementations.
> > The code ist tested on AMD64 with AMD IOMMU and GART as well as on
> > my old 486 box. It is not yet tested on a Calgary IOMMU system.
> 
> Now it is---appears to work fine on a Calgary system.
> 
> In general the patchset looks good and is definitely a step in the
> right direction. I am a bit concerned about the contortions that the
> generic dma_alloc_coherent went through before calling the ops
> version---have you verified they are no longer needed?

Most of the logic in the old dma_alloc_coherent function is moved to the
specific IOMMU implementations now. The old function tried to handle all
cases, hardware IOMMU, GART and NOMMU in one function which made it a
bit hard to read. This logic is split up and moved to the specific DMA
backends now.

Joerg

Re: [PATCH 0/7] x86 dma_*_coherent rework patchset

[FUJITA Tomonori]

On Tue, 12 Aug 2008 17:24:10 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> Hi,
> 
> this patchset reworks the dma_*_coherent functions in the DMA layer for the x86
> architecture. The patch series extends the existing DMA backends with missing
> *coherent callbacks and simplifies the generic function to basically only call
> the registered backend. This allows future optimizations in hardware specific
> IOMMU implementations.
> The code ist tested on AMD64 with AMD IOMMU and GART as well as on my old 486
> box. It is not yet tested on a Calgary IOMMU system.
> 
> Joerg
> 
> git diff --stat tip/master:
> 
>  arch/x86/kernel/amd_iommu.c      |    2 -
>  arch/x86/kernel/pci-calgary_64.c |   14 +++++
>  arch/x86/kernel/pci-dma.c        |  116 +++----------------------------------
>  arch/x86/kernel/pci-gart_64.c    |   31 ++++++++++
>  arch/x86/kernel/pci-nommu.c      |   45 +++++++++++++++
>  5 files changed, 100 insertions(+), 108 deletions(-)

Really nice. I think that this is how other architectures that support
multiple IOMMUs handle dma operations, and this is the right thing for
x86 too.