Re: [PATCH] add iommu_num_pages helper function

Re: [PATCH] add iommu_num_pages helper function

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

Hi, 

On Tue, 29 Jul 2008 10:46:31 +0900 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>
> > So, I assume a patch created on July 25 is not destined for 2.6.27,
> 
> Hmm, seems that they will go into 2.6.27:
> 
> http://marc.info/?l=linux-kernel&m=121729275722287&w=2
> 
> 
> > right?  Especially since it breaks another architecture build and has
> > only made it to linux-next on July 29 ... and needs more work given the
> > comments already on this patch.
> 
> It doesn't break another architecture build since only x86 IOMMUs use
> the helper function. But as I pointed out, it doesn't work for some
> architectures.

Um, did you see the linux-next build report I posted (possibly not):

arch/powerpc/kernel/iommu.c:56: error: static declaration of 'iommu_num_pages' follows non-static declaration
include/linux/iommu-helper.h:11: error: previous declaration of 'iommu_num_pages' was here

This is now a build of 2.6.27-rc1 for powerpc64 ppc64_defconfig ...

So we have a patch first posted to lkml on July 25 first hits linux-next
on July 29 and is no in Linus' tree on July 29.  A grep would have found
the problem.

More care required ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] add iommu_num_pages helper function

[FUJITA Tomonori]

On Tue, 29 Jul 2008 13:56:52 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi, 
> 
> On Tue, 29 Jul 2008 10:46:31 +0900 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >
> > > So, I assume a patch created on July 25 is not destined for 2.6.27,
> > 
> > Hmm, seems that they will go into 2.6.27:
> > 
> > http://marc.info/?l=linux-kernel&m=121729275722287&w=2
> > 
> > 
> > > right?  Especially since it breaks another architecture build and has
> > > only made it to linux-next on July 29 ... and needs more work given the
> > > comments already on this patch.
> > 
> > It doesn't break another architecture build since only x86 IOMMUs use
> > the helper function. But as I pointed out, it doesn't work for some
> > architectures.
> 
> Um, did you see the linux-next build report I posted (possibly not):
> 
> arch/powerpc/kernel/iommu.c:56: error: static declaration of 'iommu_num_pages' follows non-static declaration
> include/linux/iommu-helper.h:11: error: previous declaration of 'iommu_num_pages' was here
> 
> This is now a build of 2.6.27-rc1 for powerpc64 ppc64_defconfig ...

Ah, really sorry. Yeah, it breaks POWER and SPARC builds.


> So we have a patch first posted to lkml on July 25 first hits linux-next
> on July 29 and is no in Linus' tree on July 29.  A grep would have found
> the problem.
> 
> More care required ...

Agreed... As I said before, this is not urgent at all so we didn't
need to rush.


A fix has already been posted?

==
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] move iommu_num_pages helper to x86

This IOMMU helper function doesn't work for some architectures:

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

It also breaks POWER and SPARC builds:

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

Currently, only x86 IOMMUs use this so let's move it to x86 for
now.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 arch/x86/kernel/pci-dma.c    |    8 ++++++++
 include/asm-x86/iommu.h      |    2 ++
 include/linux/iommu-helper.h |    1 -
 lib/iommu-helper.c           |    8 --------
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 8dbffb8..87d4d69 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -123,6 +123,14 @@ void __init pci_iommu_alloc(void)
 
 	pci_swiotlb_init();
 }
+
+unsigned long iommu_num_pages(unsigned long addr, unsigned long len)
+{
+	unsigned long size = roundup((addr & ~PAGE_MASK) + len, PAGE_SIZE);
+
+	return size >> PAGE_SHIFT;
+}
+EXPORT_SYMBOL(iommu_num_pages);
 #endif
 
 /*
diff --git a/include/asm-x86/iommu.h b/include/asm-x86/iommu.h
index ecc8061..5f888cc 100644
--- a/include/asm-x86/iommu.h
+++ b/include/asm-x86/iommu.h
@@ -7,6 +7,8 @@ extern struct dma_mapping_ops nommu_dma_ops;
 extern int force_iommu, no_iommu;
 extern int iommu_detected;
 
+extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
+
 #ifdef CONFIG_GART_IOMMU
 extern int gart_iommu_aperture;
 extern int gart_iommu_aperture_allowed;
diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
index f8598f5..c975caf 100644
--- a/include/linux/iommu-helper.h
+++ b/include/linux/iommu-helper.h
@@ -8,4 +8,3 @@ extern unsigned long iommu_area_alloc(unsigned long *map, unsigned long size,
 				      unsigned long align_mask);
 extern void iommu_area_free(unsigned long *map, unsigned long start,
 			    unsigned int nr);
-extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
diff --git a/lib/iommu-helper.c b/lib/iommu-helper.c
index 889ddce..a3b8d4c 100644
--- a/lib/iommu-helper.c
+++ b/lib/iommu-helper.c
@@ -80,11 +80,3 @@ void iommu_area_free(unsigned long *map, unsigned long start, unsigned int nr)
 	}
 }
 EXPORT_SYMBOL(iommu_area_free);
-
-unsigned long iommu_num_pages(unsigned long addr, unsigned long len)
-{
-	unsigned long size = roundup((addr & ~PAGE_MASK) + len, PAGE_SIZE);
-
-	return size >> PAGE_SHIFT;
-}
-EXPORT_SYMBOL(iommu_num_pages);
-- 
1.5.5.GIT

Re: [PATCH] add iommu_num_pages helper function

[David Miller]

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 29 Jul 2008 13:56:52 +1000

> Um, did you see the linux-next build report I posted (possibly not):
> 
> arch/powerpc/kernel/iommu.c:56: error: static declaration of 'iommu_num_pages' follows non-static declaration
> include/linux/iommu-helper.h:11: error: previous declaration of 'iommu_num_pages' was here
> 
> This is now a build of 2.6.27-rc1 for powerpc64 ppc64_defconfig ...

It breaks sparc64 the same exact way. :-/

sparc64: Fix build after addition of iommu_num_pages() helper.

It would have been much more pleasant if that new interface:

1) Took an "io_page_size" argument so powerpc and sparc64
   could make use of it.

2) Was build tested on something other than x86.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/arch/sparc64/kernel/iommu.c b/arch/sparc64/kernel/iommu.c
index 2a37a6c..600f712 100644
--- a/arch/sparc64/kernel/iommu.c
+++ b/arch/sparc64/kernel/iommu.c
@@ -575,7 +575,7 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
 		}
 		/* Allocate iommu entries for that segment */
 		paddr = (unsigned long) SG_ENT_PHYS_ADDRESS(s);
-		npages = iommu_num_pages(paddr, slen);
+		npages = sparc64_iommu_num_pages(paddr, slen);
 		entry = iommu_range_alloc(dev, iommu, npages, &handle);
 
 		/* Handle failure */
@@ -647,7 +647,7 @@ iommu_map_failed:
 			iopte_t *base;
 
 			vaddr = s->dma_address & IO_PAGE_MASK;
-			npages = iommu_num_pages(s->dma_address, s->dma_length);
+			npages = sparc64_iommu_num_pages(s->dma_address, s->dma_length);
 			iommu_range_free(iommu, vaddr, npages);
 
 			entry = (vaddr - iommu->page_table_map_base)
@@ -715,7 +715,7 @@ static void dma_4u_unmap_sg(struct device *dev, struct scatterlist *sglist,
 
 		if (!len)
 			break;
-		npages = iommu_num_pages(dma_handle, len);
+		npages = sparc64_iommu_num_pages(dma_handle, len);
 		iommu_range_free(iommu, dma_handle, npages);
 
 		entry = ((dma_handle - iommu->page_table_map_base)
diff --git a/arch/sparc64/kernel/iommu_common.h b/arch/sparc64/kernel/iommu_common.h
index 53b19c8..e64152b 100644
--- a/arch/sparc64/kernel/iommu_common.h
+++ b/arch/sparc64/kernel/iommu_common.h
@@ -35,8 +35,8 @@
 
 #define SG_ENT_PHYS_ADDRESS(SG)	(__pa(sg_virt((SG))))
 
-static inline unsigned long iommu_num_pages(unsigned long vaddr,
-					    unsigned long slen)
+static inline unsigned long sparc64_iommu_num_pages(unsigned long vaddr,
+						    unsigned long slen)
 {
 	unsigned long npages;
 
@@ -53,7 +53,7 @@ static inline int is_span_boundary(unsigned long entry,
 				   struct scatterlist *sg)
 {
 	unsigned long paddr = SG_ENT_PHYS_ADDRESS(outs);
-	int nr = iommu_num_pages(paddr, outs->dma_length + sg->length);
+	int nr = sparc64_iommu_num_pages(paddr, outs->dma_length + sg->length);
 
 	return iommu_is_span_boundary(entry, nr, shift, boundary_size);
 }
diff --git a/arch/sparc64/kernel/pci_sun4v.c b/arch/sparc64/kernel/pci_sun4v.c
index a104c80..08dcdbd 100644
--- a/arch/sparc64/kernel/pci_sun4v.c
+++ b/arch/sparc64/kernel/pci_sun4v.c
@@ -382,7 +382,7 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
 		}
 		/* Allocate iommu entries for that segment */
 		paddr = (unsigned long) SG_ENT_PHYS_ADDRESS(s);
-		npages = iommu_num_pages(paddr, slen);
+		npages = sparc64_iommu_num_pages(paddr, slen);
 		entry = iommu_range_alloc(dev, iommu, npages, &handle);
 
 		/* Handle failure */
@@ -459,7 +459,7 @@ iommu_map_failed:
 			unsigned long vaddr, npages;
 
 			vaddr = s->dma_address & IO_PAGE_MASK;
-			npages = iommu_num_pages(s->dma_address, s->dma_length);
+			npages = sparc64_iommu_num_pages(s->dma_address, s->dma_length);
 			iommu_range_free(iommu, vaddr, npages);
 			/* XXX demap? XXX */
 			s->dma_address = DMA_ERROR_CODE;
@@ -498,7 +498,7 @@ static void dma_4v_unmap_sg(struct device *dev, struct scatterlist *sglist,
 
 		if (!len)
 			break;
-		npages = iommu_num_pages(dma_handle, len);
+		npages = sparc64_iommu_num_pages(dma_handle, len);
 		iommu_range_free(iommu, dma_handle, npages);
 
 		entry = ((dma_handle - iommu->page_table_map_base) >> IO_PAGE_SHIFT);

Re: [PATCH] add iommu_num_pages helper function

[David Miller]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Tue, 29 Jul 2008 13:38:53 +0900

> Agreed... As I said before, this is not urgent at all so we didn't
> need to rush.

Please, if you are going to add such interface, add
"const unsigned long io_page_size" argument so that
powerpc and sparc can use it too.

Re: [PATCH] add iommu_num_pages helper function

[FUJITA Tomonori]

On Mon, 28 Jul 2008 22:26:18 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Date: Tue, 29 Jul 2008 13:38:53 +0900
> 
> > Agreed... As I said before, this is not urgent at all so we didn't
> > need to rush.
> 
> Please, if you are going to add such interface, add
> "const unsigned long io_page_size" argument so that
> powerpc and sparc can use it too.

I didn't add this function:

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

I pointed out that it doesn't work for some architectures:

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


But sorry about not noticing that it breaks SPARC and POWER.

Re: [PATCH] add iommu_num_pages helper function

[FUJITA Tomonori]

On Mon, 28 Jul 2008 22:24:39 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Tue, 29 Jul 2008 13:56:52 +1000
> 
> > Um, did you see the linux-next build report I posted (possibly not):
> > 
> > arch/powerpc/kernel/iommu.c:56: error: static declaration of 'iommu_num_pages' follows non-static declaration
> > include/linux/iommu-helper.h:11: error: previous declaration of 'iommu_num_pages' was here
> > 
> > This is now a build of 2.6.27-rc1 for powerpc64 ppc64_defconfig ...
> 
> It breaks sparc64 the same exact way. :-/

Yeah, it breaks both. It's not a proper helper function that all the
architectures can use so how about making it x86 stuff?

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

Re: [PATCH] add iommu_num_pages helper function

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

Hi Dave,

On Mon, 28 Jul 2008 22:24:39 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> sparc64: Fix build after addition of iommu_num_pages() helper.

I added this to the end of linux-next for today.  Hopefully we will have
a fix in Linus' tree tomorrow.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] add iommu_num_pages helper function

[Joerg Roedel]

On Tue, Jul 29, 2008 at 04:45:23PM +1000, Stephen Rothwell wrote:
> Hi Dave,
> 
> On Mon, 28 Jul 2008 22:24:39 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> >
> > sparc64: Fix build after addition of iommu_num_pages() helper.
> 
> I added this to the end of linux-next for today.  Hopefully we will have
> a fix in Linus' tree tomorrow.

Yes, my fault, sorry for that breakage :-/
I think moving that function to x86-only code is the best solution for
now. Before moving it back to generic code we need an architecture
independent way to find out the IOMMU page size.

FUJITA,

do you want to work on that or should I try to do this and send it to
you for review?

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] add iommu_num_pages helper function

[FUJITA Tomonori]

On Tue, 29 Jul 2008 11:50:27 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Tue, Jul 29, 2008 at 04:45:23PM +1000, Stephen Rothwell wrote:
> > Hi Dave,
> > 
> > On Mon, 28 Jul 2008 22:24:39 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> > >
> > > sparc64: Fix build after addition of iommu_num_pages() helper.
> > 
> > I added this to the end of linux-next for today.  Hopefully we will have
> > a fix in Linus' tree tomorrow.
> 
> Yes, my fault, sorry for that breakage :-/
> I think moving that function to x86-only code is the best solution for
> now. Before moving it back to generic code we need an architecture
> independent way to find out the IOMMU page size.

Agreed, it fixes both SPARC and POWER breakages.


> FUJITA,
> 
> do you want to work on that or should I try to do this and send it to
> you for review?

I've already sent a patch that fixes the breakages in such way:

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

Re: [PATCH] add iommu_num_pages helper function

[Joerg Roedel]

On Tue, Jul 29, 2008 at 06:58:08PM +0900, FUJITA Tomonori wrote:
> On Tue, 29 Jul 2008 11:50:27 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> > do you want to work on that or should I try to do this and send it to
> > you for review?
> 
> I've already sent a patch that fixes the breakages in such way:
> 
> http://marc.info/?l=linux-kernel&m=121730644704448&w=2

Yes I have seen this patch. I meant the patches that introduce something
like IOMMU_PAGE_SIZE for all architectures so that we can make this
function generic and move it back to lib/.

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] add iommu_num_pages helper function

[David Miller]

From: Joerg Roedel <joerg.roedel@amd.com>
Date: Tue, 29 Jul 2008 11:50:27 +0200

> I think moving that function to x86-only code is the best solution for
> now. Before moving it back to generic code we need an architecture
> independent way to find out the IOMMU page size.

My suggestion (again) is to pass it in as a "const unsigned long
io_page_size" argument and update the callers.

Re: [PATCH] add iommu_num_pages helper function

[Joerg Roedel]

On Tue, Jul 29, 2008 at 03:10:05AM -0700, David Miller wrote:
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Tue, 29 Jul 2008 11:50:27 +0200
> 
> > I think moving that function to x86-only code is the best solution for
> > now. Before moving it back to generic code we need an architecture
> > independent way to find out the IOMMU page size.
> 
> My suggestion (again) is to pass it in as a "const unsigned long
> io_page_size" argument and update the callers.

Ok, this has the advantage that it supports more than one IOMMU page
size per architecture and its also less intrusive.

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] add iommu_num_pages helper function

[FUJITA Tomonori]

On Tue, 29 Jul 2008 03:10:05 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Tue, 29 Jul 2008 11:50:27 +0200
> 
> > I think moving that function to x86-only code is the best solution for
> > now. Before moving it back to generic code we need an architecture
> > independent way to find out the IOMMU page size.
> 
> My suggestion (again) is to pass it in as a "const unsigned long
> io_page_size" argument and update the callers.

Either is fine by me.

Joerg, if you like, please finish this (for all the architectures,
please). As I said before, I planed to work on this cleanup after rc1
(and this should have been 2.6.28 stuff). But I'd appreciate if you
would do.

Thanks,

Re: [PATCH] add iommu_num_pages helper function

[Ingo Molnar]

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

> On Tue, 29 Jul 2008 03:10:05 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Joerg Roedel <joerg.roedel@amd.com>
> > Date: Tue, 29 Jul 2008 11:50:27 +0200
> > 
> > > I think moving that function to x86-only code is the best solution for
> > > now. Before moving it back to generic code we need an architecture
> > > independent way to find out the IOMMU page size.
> > 
> > My suggestion (again) is to pass it in as a "const unsigned long
> > io_page_size" argument and update the callers.
> 
> Either is fine by me.
> 
> Joerg, if you like, please finish this (for all the architectures, 
> please). As I said before, I planed to work on this cleanup after rc1 
> (and this should have been 2.6.28 stuff). But I'd appreciate if you 
> would do.

i've queued up your quick fix in tip/x86/urgent to resolve the immediate 
-git build breakage. The wider patch can then be done on top of that, 
without being rushed. Any objections to this approach?

	Ingo

Re: [PATCH] add iommu_num_pages helper function

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Tue, 29 Jul 2008 12:31:46 +0200

> i've queued up your quick fix in tip/x86/urgent to resolve the immediate 
> -git build breakage. The wider patch can then be done on top of that, 
> without being rushed. Any objections to this approach?

I do not object :-)

Re: [PATCH] add iommu_num_pages helper function

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

On Tue, 29 Jul 2008 03:33:42 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> From: Ingo Molnar <mingo@elte.hu>
> Date: Tue, 29 Jul 2008 12:31:46 +0200
> 
> > i've queued up your quick fix in tip/x86/urgent to resolve the immediate 
> > -git build breakage. The wider patch can then be done on top of that, 
> > without being rushed. Any objections to this approach?
> 
> I do not object :-)

Neither do I. :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] add iommu_num_pages helper function

[Joerg Roedel]

On Tue, Jul 29, 2008 at 03:10:05AM -0700, David Miller wrote:
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Tue, 29 Jul 2008 11:50:27 +0200
> 
> > I think moving that function to x86-only code is the best solution for
> > now. Before moving it back to generic code we need an architecture
> > independent way to find out the IOMMU page size.
> 
> My suggestion (again) is to pass it in as a "const unsigned long
> io_page_size" argument and update the callers.

Ok, after setting up cross compile environments for sparc64 and
powerpc64 I wrote a patch which adds the io_page_size argument to the
iommu_num pages function and moves the function back to
lib/iommu-helper.c. The patch touches all 3 architectures and can not be
split in a bisectable way.

Who is the best person to send this patch to?

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] add iommu_num_pages helper function

[FUJITA Tomonori]

On Wed, 30 Jul 2008 14:18:05 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Tue, Jul 29, 2008 at 03:10:05AM -0700, David Miller wrote:
> > From: Joerg Roedel <joerg.roedel@amd.com>
> > Date: Tue, 29 Jul 2008 11:50:27 +0200
> > 
> > > I think moving that function to x86-only code is the best solution for
> > > now. Before moving it back to generic code we need an architecture
> > > independent way to find out the IOMMU page size.
> > 
> > My suggestion (again) is to pass it in as a "const unsigned long
> > io_page_size" argument and update the callers.
> 
> Ok, after setting up cross compile environments for sparc64 and
> powerpc64 I wrote a patch which adds the io_page_size argument to the
> iommu_num pages function and moves the function back to
> lib/iommu-helper.c.

You need ia64, parisc, and alpha too. :) 

I have cross compile environments for them so I can take care of them
if you like.


> The patch touches all 3 architectures and can not be
> split in a bisectable way.

How about naming a new helper function iommu_nr_pages (or something)?

Then you can do the conversion independently. If we like, we use
iommu_num_pages name after the conversion.


> Who is the best person to send this patch to?

The -mm is an appropriate tree, I think.

Re: [PATCH] add iommu_num_pages helper function

[Joerg Roedel]

On Wed, Jul 30, 2008 at 10:16:05PM +0900, FUJITA Tomonori wrote:
> On Wed, 30 Jul 2008 14:18:05 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:

> > Ok, after setting up cross compile environments for sparc64 and
> > powerpc64 I wrote a patch which adds the io_page_size argument to the
> > iommu_num pages function and moves the function back to
> > lib/iommu-helper.c.
> 
> You need ia64, parisc, and alpha too. :) 
> 
> I have cross compile environments for them so I can take care of them
> if you like.

Ok, so you suggest to create a patchset to convert the other IOMMU
implementations too. Thats fine for me, I will try to do it :)
Do you have hardware to do runtime tests of the code? I have hardware
only for x86, sparc64 and alpha.

> 
> > The patch touches all 3 architectures and can not be
> > split in a bisectable way.
> 
> How about naming a new helper function iommu_nr_pages (or something)?
> 
> Then you can do the conversion independently. If we like, we use
> iommu_num_pages name after the conversion.

Ok, this would mean only 3 renaming patches to keep the patch series
bisectable. This should be acceptable.

> 
> > Who is the best person to send this patch to?
> 
> The -mm is an appropriate tree, I think.

Ok, then I submit it to Andrew once its tested.

Thanks,

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] add iommu_num_pages helper function

[Joerg Roedel]

On Wed, Jul 30, 2008 at 10:16:05PM +0900, FUJITA Tomonori wrote:
> On Wed, 30 Jul 2008 14:18:05 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > Ok, after setting up cross compile environments for sparc64 and
> > powerpc64 I wrote a patch which adds the io_page_size argument to the
> > iommu_num pages function and moves the function back to
> > lib/iommu-helper.c.
> 
> You need ia64, parisc, and alpha too. :) 

Ok, I looked into the different IOMMU drivers for pa-risc and ia64. The
changes needed there are non-trival and I can't test them on real
hardware (I only have an pa-risc 1.1). So I think I skip these 2
architectures for now. They can be changed by somebody who has the
hardware :)
(Alpha instead was trival. I changed this driver)

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] add iommu_num_pages helper function

[Benjamin Herrenschmidt]

On Tue, 2008-07-29 at 12:02 +0200, Joerg Roedel wrote:
> On Tue, Jul 29, 2008 at 06:58:08PM +0900, FUJITA Tomonori wrote:
> > On Tue, 29 Jul 2008 11:50:27 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > do you want to work on that or should I try to do this and send it to
> > > you for review?
> > 
> > I've already sent a patch that fixes the breakages in such way:
> > 
> > http://marc.info/?l=linux-kernel&m=121730644704448&w=2
> 
> Yes I have seen this patch. I meant the patches that introduce something
> like IOMMU_PAGE_SIZE for all architectures so that we can make this
> function generic and move it back to lib/.

That will not work in the light of some new stuff coming on powerpc in
the future. The iommu page size may/will be dynamic per instance of
iommu. IE, it will be mostly decided at runtime, I don't expect machines
to have multiple iommus of different page sizes off hand but it -can-
happen.

Ben.

Re: [PATCH] add iommu_num_pages helper function

[Benjamin Herrenschmidt]

On Tue, 2008-07-29 at 03:10 -0700, David Miller wrote:
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Tue, 29 Jul 2008 11:50:27 +0200
> 
> > I think moving that function to x86-only code is the best solution for
> > now. Before moving it back to generic code we need an architecture
> > independent way to find out the IOMMU page size.
> 
> My suggestion (again) is to pass it in as a "const unsigned long
> io_page_size" argument and update the callers.

Yeah. That's better

Ben.

Re: [PATCH] add iommu_num_pages helper function

[FUJITA Tomonori]

On Thu, 31 Jul 2008 08:44:14 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Tue, 2008-07-29 at 12:02 +0200, Joerg Roedel wrote:
> > On Tue, Jul 29, 2008 at 06:58:08PM +0900, FUJITA Tomonori wrote:
> > > On Tue, 29 Jul 2008 11:50:27 +0200
> > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > > do you want to work on that or should I try to do this and send it to
> > > > you for review?
> > > 
> > > I've already sent a patch that fixes the breakages in such way:
> > > 
> > > http://marc.info/?l=linux-kernel&m=121730644704448&w=2
> > 
> > Yes I have seen this patch. I meant the patches that introduce something
> > like IOMMU_PAGE_SIZE for all architectures so that we can make this
> > function generic and move it back to lib/.
> 
> That will not work in the light of some new stuff coming on powerpc in
> the future. The iommu page size may/will be dynamic per instance of
> iommu. IE, it will be mostly decided at runtime, I don't expect machines
> to have multiple iommus of different page sizes off hand but it -can-
> happen.

You could do something like this:

#define IOMMU_PAGE_SIZE	power_iommu_page_size

But I prefer the current proposal to have the size as an argument.