Re: gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t

Re: gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t

[Stefan Lippers-Hollmann]

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

Hi

On Samstag, 28. Februar 2009, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4ab0d47d0ab311eb181532c1ecb6d02905685071
> Commit:     4ab0d47d0ab311eb181532c1ecb6d02905685071
> Parent:     6644107d57a8fa82b47e4c55da4d9d91a612f29c
> Author:     Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> AuthorDate: Tue Feb 24 17:35:12 2009 -0800
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Wed Feb 25 13:09:51 2009 +0100
> 
>     gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t
>     
>     io_mapping_create_wc should take a resource_size_t parameter in place of
>     unsigned long. With unsigned long, there will be no way to map greater than 4GB
>     address in i386/32 bit.
>     
>     On x86, greater than 4GB addresses cannot be mapped on i386 without PAE. Return
>     error for such a case.
>     
>     Patch also adds a structure for io_mapping, that saves the base, size and
>     type on HAVE_ATOMIC_IOMAP archs, that can be used to verify the offset on
>     io_mapping_map calls.
>     
>     Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>     Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
>     Cc: Dave Airlie <airlied@redhat.com>
>     Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>     Cc: Eric Anholt <eric@anholt.net>
>     Cc: Keith Packard <keithp@keithp.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>

This patch, as part of 2.6.29-rc6-git5, fails to build for me on i386 (it 
builds on amd64) with the attached (slimmed down) config (CONFIG_X86_PAT=y)
and buildlog:

  Building modules, stage 2.
  MODPOST 809 modules
ERROR: "pgprot_writecombine" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "is_io_mapping_possible" [drivers/gpu/drm/i915/i915.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

This is a build regression in comparison to 2.6.29-rc6.

Regards
	Stefan Lippers-Hollmann
-- 

>  arch/x86/include/asm/iomap.h |    3 ++
>  arch/x86/mm/iomap_32.c       |   18 ++++++++++++++++
>  include/linux/io-mapping.h   |   46 +++++++++++++++++++++++++++++++----------
>  3 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/iomap.h b/arch/x86/include/asm/iomap.h
> index c1f0628..86af260 100644
> --- a/arch/x86/include/asm/iomap.h
> +++ b/arch/x86/include/asm/iomap.h
> @@ -23,6 +23,9 @@
>  #include <asm/pgtable.h>
>  #include <asm/tlbflush.h>
>  
> +int
> +is_io_mapping_possible(resource_size_t base, unsigned long size);
> +
>  void *
>  iomap_atomic_prot_pfn(unsigned long pfn, enum km_type type, pgprot_t prot);
>  
> diff --git a/arch/x86/mm/iomap_32.c b/arch/x86/mm/iomap_32.c
> index ca53224..6c2b1af 100644
> --- a/arch/x86/mm/iomap_32.c
> +++ b/arch/x86/mm/iomap_32.c
> @@ -20,6 +20,24 @@
>  #include <asm/pat.h>
>  #include <linux/module.h>
>  
> +#ifdef CONFIG_X86_PAE
> +int
> +is_io_mapping_possible(resource_size_t base, unsigned long size)
> +{
> +	return 1;
> +}
> +#else
> +int
> +is_io_mapping_possible(resource_size_t base, unsigned long size)
> +{
> +	/* There is no way to map greater than 1 << 32 address without PAE */
> +	if (base + size > 0x100000000ULL)
> +		return 0;
> +
> +	return 1;
> +}
> +#endif
> +
>  /* Map 'pfn' using fixed map 'type' and protections 'prot'
>   */
>  void *
> diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
> index 82df317..cbc2f0c 100644
> --- a/include/linux/io-mapping.h
> +++ b/include/linux/io-mapping.h
> @@ -30,11 +30,14 @@
>   * See Documentation/io_mapping.txt
>   */
>  
> -/* this struct isn't actually defined anywhere */
> -struct io_mapping;
> -
>  #ifdef CONFIG_HAVE_ATOMIC_IOMAP
>  
> +struct io_mapping {
> +	resource_size_t base;
> +	unsigned long size;
> +	pgprot_t prot;
> +};
> +
>  /*
>   * For small address space machines, mapping large objects
>   * into the kernel virtual space isn't practical. Where
> @@ -43,23 +46,40 @@ struct io_mapping;
>   */
>  
>  static inline struct io_mapping *
> -io_mapping_create_wc(unsigned long base, unsigned long size)
> +io_mapping_create_wc(resource_size_t base, unsigned long size)
>  {
> -	return (struct io_mapping *) base;
> +	struct io_mapping *iomap;
> +
> +	if (!is_io_mapping_possible(base, size))
> +		return NULL;
> +
> +	iomap = kmalloc(sizeof(*iomap), GFP_KERNEL);
> +	if (!iomap)
> +		return NULL;
> +
> +	iomap->base = base;
> +	iomap->size = size;
> +	iomap->prot = pgprot_writecombine(__pgprot(__PAGE_KERNEL));
> +	return iomap;
>  }
>  
>  static inline void
>  io_mapping_free(struct io_mapping *mapping)
>  {
> +	kfree(mapping);
>  }
>  
>  /* Atomic map/unmap */
>  static inline void *
>  io_mapping_map_atomic_wc(struct io_mapping *mapping, unsigned long offset)
>  {
> -	offset += (unsigned long) mapping;
> -	return iomap_atomic_prot_pfn(offset >> PAGE_SHIFT, KM_USER0,
> -				     __pgprot(__PAGE_KERNEL_WC));
> +	resource_size_t phys_addr;
> +	unsigned long pfn;
> +
> +	BUG_ON(offset >= mapping->size);
> +	phys_addr = mapping->base + offset;
> +	pfn = (unsigned long) (phys_addr >> PAGE_SHIFT);
> +	return iomap_atomic_prot_pfn(pfn, KM_USER0, mapping->prot);
>  }
>  
>  static inline void
> @@ -71,8 +91,9 @@ io_mapping_unmap_atomic(void *vaddr)
>  static inline void *
>  io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
>  {
> -	offset += (unsigned long) mapping;
> -	return ioremap_wc(offset, PAGE_SIZE);
> +	BUG_ON(offset >= mapping->size);
> +	resource_size_t phys_addr = mapping->base + offset;
> +	return ioremap_wc(phys_addr, PAGE_SIZE);
>  }
>  
>  static inline void
> @@ -83,9 +104,12 @@ io_mapping_unmap(void *vaddr)
>  
>  #else
>  
> +/* this struct isn't actually defined anywhere */
> +struct io_mapping;
> +
>  /* Create the io_mapping object*/
>  static inline struct io_mapping *
> -io_mapping_create_wc(unsigned long base, unsigned long size)
> +io_mapping_create_wc(resource_size_t base, unsigned long size)
>  {
>  	return (struct io_mapping *) ioremap_wc(base, size);
>  }

[-- Attachment #2: config-2.6.29-rc6-git5.gz --]
[-- Type: application/x-gzip, Size: 11567 bytes --]

[-- Attachment #3: linux-2.6.29-rc6-git5.build.log.gz --]
[-- Type: application/x-gzip, Size: 18282 bytes --]

Re: gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t

[Pallipadi, Venkatesh]

On Sun, Mar 01, 2009 at 05:54:53AM -0800, Stefan Lippers-Hollmann wrote:
> Hi
> 
> On Samstag, 28. Februar 2009, Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4ab0d47d0ab311eb181532c1ecb6d02905685071
> > Commit:     4ab0d47d0ab311eb181532c1ecb6d02905685071
> > Parent:     6644107d57a8fa82b47e4c55da4d9d91a612f29c
> > Author:     Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> > AuthorDate: Tue Feb 24 17:35:12 2009 -0800
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Wed Feb 25 13:09:51 2009 +0100
> >
> >     gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t
> >
> >     io_mapping_create_wc should take a resource_size_t parameter in place of
> >     unsigned long. With unsigned long, there will be no way to map greater than 4GB
> >     address in i386/32 bit.
> >
> >     On x86, greater than 4GB addresses cannot be mapped on i386 without PAE. Return
> >     error for such a case.
> >
> >     Patch also adds a structure for io_mapping, that saves the base, size and
> >     type on HAVE_ATOMIC_IOMAP archs, that can be used to verify the offset on
> >     io_mapping_map calls.
> >
> >     Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> >     Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> >     Cc: Dave Airlie <airlied@redhat.com>
> >     Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >     Cc: Eric Anholt <eric@anholt.net>
> >     Cc: Keith Packard <keithp@keithp.com>
> >     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> This patch, as part of 2.6.29-rc6-git5, fails to build for me on i386 (it
> builds on amd64) with the attached (slimmed down) config (CONFIG_X86_PAT=y)
> and buildlog:
> 
>   Building modules, stage 2.
>   MODPOST 809 modules
> ERROR: "pgprot_writecombine" [drivers/gpu/drm/i915/i915.ko] undefined!
> ERROR: "is_io_mapping_possible" [drivers/gpu/drm/i915/i915.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> This is a build regression in comparison to 2.6.29-rc6.
> 

My bad. I had missed drm as module compilation. Below patch should fix it.
Can you please verify.

Thanks,
Venki


Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 arch/x86/mm/iomap_32.c     |    1 +
 arch/x86/mm/pat.c          |    2 ++
 include/linux/io-mapping.h |    3 ++-
 3 files changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/iomap_32.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/iomap_32.c	2009-03-01 08:31:12.000000000 -0800
+++ linux-2.6/arch/x86/mm/iomap_32.c	2009-03-01 08:47:20.000000000 -0800
@@ -37,6 +37,7 @@ is_io_mapping_possible(resource_size_t b
 	return 1;
 }
 #endif
+EXPORT_SYMBOL_GPL(is_io_mapping_possible);
 
 /* Map 'pfn' using fixed map 'type' and protections 'prot'
  */
Index: linux-2.6/arch/x86/mm/pat.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/pat.c	2009-03-01 08:31:00.000000000 -0800
+++ linux-2.6/arch/x86/mm/pat.c	2009-03-01 08:45:48.000000000 -0800
@@ -11,6 +11,7 @@
 #include <linux/bootmem.h>
 #include <linux/debugfs.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/gfp.h>
 #include <linux/mm.h>
 #include <linux/fs.h>
@@ -868,6 +869,7 @@ pgprot_t pgprot_writecombine(pgprot_t pr
 	else
 		return pgprot_noncached(prot);
 }
+EXPORT_SYMBOL(pgprot_writecombine);
 
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
 
Index: linux-2.6/include/linux/io-mapping.h
===================================================================
--- linux-2.6.orig/include/linux/io-mapping.h	2009-03-01 08:31:12.000000000 -0800
+++ linux-2.6/include/linux/io-mapping.h	2009-03-01 08:49:53.000000000 -0800
@@ -91,8 +91,9 @@ io_mapping_unmap_atomic(void *vaddr)
 static inline void *
 io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
 {
+	resource_size_t phys_addr;
 	BUG_ON(offset >= mapping->size);
-	resource_size_t phys_addr = mapping->base + offset;
+	phys_addr = mapping->base + offset;
 	return ioremap_wc(phys_addr, PAGE_SIZE);
 }
 

Re: gpu/drm, x86, PAT: io_mapping_create_wc and resource_size_t

[Stefan Lippers-Hollmann]

Hi

On Sonntag, 1. März 2009, Pallipadi, Venkatesh wrote:
> On Sun, Mar 01, 2009 at 05:54:53AM -0800, Stefan Lippers-Hollmann wrote:
> > Hi
> > 
> > On Samstag, 28. Februar 2009, Linux Kernel Mailing List wrote:
> > > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4ab0d47d0ab311eb181532c1ecb6d02905685071
> > > Commit:     4ab0d47d0ab311eb181532c1ecb6d02905685071
[...]
> >   Building modules, stage 2.
> >   MODPOST 809 modules
> > ERROR: "pgprot_writecombine" [drivers/gpu/drm/i915/i915.ko] undefined!
> > ERROR: "is_io_mapping_possible" [drivers/gpu/drm/i915/i915.ko] undefined!
> > make[1]: *** [__modpost] Error 1
> > make: *** [modules] Error 2
> > 
> > This is a build regression in comparison to 2.6.29-rc6.
> > 
> 
> My bad. I had missed drm as module compilation. Below patch should fix it.
> Can you please verify.

Thanks, this builds fine for i386 and amd64 and seems to work on i945GME.

Regards
	Stefan Lippers-Hollmann
-- 
> Thanks,
> Venki
> 
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> 
> ---
>  arch/x86/mm/iomap_32.c     |    1 +
>  arch/x86/mm/pat.c          |    2 ++
>  include/linux/io-mapping.h |    3 ++-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/arch/x86/mm/iomap_32.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/iomap_32.c	2009-03-01 08:31:12.000000000 -0800
> +++ linux-2.6/arch/x86/mm/iomap_32.c	2009-03-01 08:47:20.000000000 -0800
> @@ -37,6 +37,7 @@ is_io_mapping_possible(resource_size_t b
>  	return 1;
>  }
>  #endif
> +EXPORT_SYMBOL_GPL(is_io_mapping_possible);
>  
>  /* Map 'pfn' using fixed map 'type' and protections 'prot'
>   */
> Index: linux-2.6/arch/x86/mm/pat.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/pat.c	2009-03-01 08:31:00.000000000 -0800
> +++ linux-2.6/arch/x86/mm/pat.c	2009-03-01 08:45:48.000000000 -0800
> @@ -11,6 +11,7 @@
>  #include <linux/bootmem.h>
>  #include <linux/debugfs.h>
>  #include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/gfp.h>
>  #include <linux/mm.h>
>  #include <linux/fs.h>
> @@ -868,6 +869,7 @@ pgprot_t pgprot_writecombine(pgprot_t pr
>  	else
>  		return pgprot_noncached(prot);
>  }
> +EXPORT_SYMBOL(pgprot_writecombine);
>  
>  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_X86_PAT)
>  
> Index: linux-2.6/include/linux/io-mapping.h
> ===================================================================
> --- linux-2.6.orig/include/linux/io-mapping.h	2009-03-01 08:31:12.000000000 -0800
> +++ linux-2.6/include/linux/io-mapping.h	2009-03-01 08:49:53.000000000 -0800
> @@ -91,8 +91,9 @@ io_mapping_unmap_atomic(void *vaddr)
>  static inline void *
>  io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
>  {
> +	resource_size_t phys_addr;
>  	BUG_ON(offset >= mapping->size);
> -	resource_size_t phys_addr = mapping->base + offset;
> +	phys_addr = mapping->base + offset;
>  	return ioremap_wc(phys_addr, PAGE_SIZE);
>  }
>  
>