[patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

__alloc_bootmem_low() and __alloc_bootmem_low_node() should use
MAX_DMA_ADDRESS as limit which is per architecture instead of a global
LOW32LIMIT. Otherwise the bootmem allocator may return addresses
to memory regions which cannot be used for DMA access.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 mm/bootmem.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index 50353e0..541bbe9 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -436,8 +436,6 @@ void * __init __alloc_bootmem_node(pg_da
 	return __alloc_bootmem(size, align, goal);
 }
 
-#define LOW32LIMIT 0xffffffff
-
 void * __init __alloc_bootmem_low(unsigned long size, unsigned long align, unsigned long goal)
 {
 	bootmem_data_t *bdata;
@@ -445,7 +443,7 @@ void * __init __alloc_bootmem_low(unsign
 
 	list_for_each_entry(bdata, &bdata_list, list)
 		if ((ptr = __alloc_bootmem_core(bdata, size,
-						 align, goal, LOW32LIMIT)))
+						 align, goal, MAX_DMA_ADDRESS)))
 			return(ptr);
 
 	/*
@@ -459,5 +457,6 @@ void * __init __alloc_bootmem_low(unsign
 void * __init __alloc_bootmem_low_node(pg_data_t *pgdat, unsigned long size,
 				       unsigned long align, unsigned long goal)
 {
-	return __alloc_bootmem_core(pgdat->bdata, size, align, goal, LOW32LIMIT);
+	return __alloc_bootmem_core(pgdat->bdata, size, align, goal,
+				    MAX_DMA_ADDRESS);
 }

Re: [patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Ingo Molnar]

* Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> __alloc_bootmem_low() and __alloc_bootmem_low_node() should use 
> MAX_DMA_ADDRESS as limit which is per architecture instead of a global 
> LOW32LIMIT. Otherwise the bootmem allocator may return addresses to 
> memory regions which cannot be used for DMA access.

> -#define LOW32LIMIT 0xffffffff

>  		if ((ptr = __alloc_bootmem_core(bdata, size,
> -						 align, goal, LOW32LIMIT)))
> +						 align, goal, MAX_DMA_ADDRESS)))

but this limits things to 16MB on i686. Are you sure this wont break 
anything?

	Ingo

Re: [patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Martin Schwidefsky]

On Fri, 2006-07-28 at 15:13 +0200, Ingo Molnar wrote:
> > -#define LOW32LIMIT 0xffffffff
> 
> >  		if ((ptr = __alloc_bootmem_core(bdata, size,
> > -						 align, goal, LOW32LIMIT)))
> > +						 align, goal, MAX_DMA_ADDRESS)))
> 
> but this limits things to 16MB on i686. Are you sure this wont break 
> anything?

That is something we should not do. MAX_DMA_ADDRESS is not the correct
value, it says something about the DMA limitations. LOW32LIMIT says
something about the cpu addressing limitations which is a completly
different thing. I think it would be best to introduce an architecture
overridable define like LOW_ADDRESS_LIMIT. The default is 4GB-1, for
s390 it is 2GB-1. The current name is misleading LOW32LIMIT indicates
that the address for alloc_bootmem_low objects has 32 bits, which isn't
true for s390.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Heiko Carstens]

On Fri, Jul 28, 2006 at 04:58:45PM +0200, Martin Schwidefsky wrote:
> On Fri, 2006-07-28 at 15:13 +0200, Ingo Molnar wrote:
> > > -#define LOW32LIMIT 0xffffffff
> > 
> > >  		if ((ptr = __alloc_bootmem_core(bdata, size,
> > > -						 align, goal, LOW32LIMIT)))
> > > +						 align, goal, MAX_DMA_ADDRESS)))
> > 
> > but this limits things to 16MB on i686. Are you sure this wont break 
> > anything?
> 
> That is something we should not do. MAX_DMA_ADDRESS is not the correct
> value, it says something about the DMA limitations. LOW32LIMIT says
> something about the cpu addressing limitations which is a completly
> different thing. I think it would be best to introduce an architecture
> overridable define like LOW_ADDRESS_LIMIT. The default is 4GB-1, for
> s390 it is 2GB-1. The current name is misleading LOW32LIMIT indicates
> that the address for alloc_bootmem_low objects has 32 bits, which isn't
> true for s390.

Hm... how about this one then:

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Introduce ARCH_LOW_ADDRESS_LIMIT which can be set per architecture to
override the 4GB default limit used by the bootmem allocater within
__alloc_bootmem_low() and __alloc_bootmem_low_node().
E.g. s390 needs a 2GB limit instead of 4GB.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 include/asm-s390/processor.h |    2 ++
 mm/bootmem.c                 |   11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/asm-s390/processor.h b/include/asm-s390/processor.h
index 5b71d37..a21d2c5 100644
--- a/include/asm-s390/processor.h
+++ b/include/asm-s390/processor.h
@@ -337,6 +337,8 @@ struct notifier_block;
 int register_idle_notifier(struct notifier_block *nb);
 int unregister_idle_notifier(struct notifier_block *nb);
 
+#define ARCH_LOW_ADDRESS_LIMIT	0x7fffffffUL
+
 #endif
 
 #endif                                 /* __ASM_S390_PROCESSOR_H           */
diff --git a/mm/bootmem.c b/mm/bootmem.c
index 50353e0..bf99002 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <asm/dma.h>
 #include <asm/io.h>
+#include <asm/processor.h>
 #include "internal.h"
 
 /*
@@ -436,7 +437,9 @@ void * __init __alloc_bootmem_node(pg_da
 	return __alloc_bootmem(size, align, goal);
 }
 
-#define LOW32LIMIT 0xffffffff
+#ifndef ARCH_LOW_ADDRESS_LIMIT
+#define ARCH_LOW_ADDRESS_LIMIT	0xffffffffUL
+#endif
 
 void * __init __alloc_bootmem_low(unsigned long size, unsigned long align, unsigned long goal)
 {
@@ -445,7 +448,8 @@ void * __init __alloc_bootmem_low(unsign
 
 	list_for_each_entry(bdata, &bdata_list, list)
 		if ((ptr = __alloc_bootmem_core(bdata, size,
-						 align, goal, LOW32LIMIT)))
+						align, goal,
+						ARCH_LOW_ADDRESS_LIMIT)))
 			return(ptr);
 
 	/*
@@ -459,5 +463,6 @@ void * __init __alloc_bootmem_low(unsign
 void * __init __alloc_bootmem_low_node(pg_data_t *pgdat, unsigned long size,
 				       unsigned long align, unsigned long goal)
 {
-	return __alloc_bootmem_core(pgdat->bdata, size, align, goal, LOW32LIMIT);
+	return __alloc_bootmem_core(pgdat->bdata, size, align, goal,
+				    ARCH_LOW_ADDRESS_LIMIT);
 }

Re: [patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Ingo Molnar]

* Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> Hm... how about this one then:
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Introduce ARCH_LOW_ADDRESS_LIMIT which can be set per architecture to
> override the 4GB default limit used by the bootmem allocater within
> __alloc_bootmem_low() and __alloc_bootmem_low_node().
> E.g. s390 needs a 2GB limit instead of 4GB.
> 
> Cc: Ingo Molnar <mingo@elte.hu>

Acked-by: Ingo Molnar <mingo@elte.hu>

(although you might get some flak about using an ARCH* define. I'm not 
sure what the current upstream policy is - using an #ifndef default 
value is the most compact hence sanest thing to do, still it's sometimes 
being frowned upon in favor of sprinkling the default value into every 
architecture's processor.h. Putting the value into a Kconfig and 
combining it with #ifndef might be better.)

	Ingo

Re: [patch] bootmem: use MAX_DMA_ADDRESS instead of LOW32LIMIT

[Randy.Dunlap]

On Sat, 29 Jul 2006 05:55:23 +0200 Ingo Molnar wrote:

> 
> * Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > Hm... how about this one then:
> > 
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > Introduce ARCH_LOW_ADDRESS_LIMIT which can be set per architecture to
> > override the 4GB default limit used by the bootmem allocater within
> > __alloc_bootmem_low() and __alloc_bootmem_low_node().
> > E.g. s390 needs a 2GB limit instead of 4GB.
> > 
> > Cc: Ingo Molnar <mingo@elte.hu>
> 
> Acked-by: Ingo Molnar <mingo@elte.hu>
> 
> (although you might get some flak about using an ARCH* define. I'm not 
> sure what the current upstream policy is - using an #ifndef default 
> value is the most compact hence sanest thing to do, still it's sometimes 
> being frowned upon in favor of sprinkling the default value into every 
> architecture's processor.h. Putting the value into a Kconfig and 
> combining it with #ifndef might be better.)

(sorry for the delay, too much travel/conferences)

I agree with your ordering.  Linus wrote about the current
ARCH_HAS* (and HAVE_ARCH* I suppose):
  "WE SHOULD GET RID OF ARCH_HAS_XYZZY. It's a disease."

I have patches for some of these that I will post soon (prob.
Monday), converting several ARCH_HAS* to CONFIG_ namespace.

---
~Randy