linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
@ 2008-09-23 20:38 Sebastian Siewior
  2008-09-24  1:46 ` Milton Miller
  2008-09-25 18:15 ` [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Scott Wood
  0 siblings, 2 replies; 12+ messages in thread
From: Sebastian Siewior @ 2008-09-23 20:38 UTC (permalink / raw)
  To: linuxppc-dev

My mylinux binary incl. bss is ~5 MiB without bss less than 4 MiB.
Therefore I though that I could replace ei.memsize with ei.loadsize. It
didn't work. I'm not sure why it did not work but I guess that the
memset() of bss in the initial kernel code overwrote the cuimage code
which is required for some reason. Maybe some device-tree callbacks.

My current (working) solution is to move cuImage from 4 MiB to 8 MiB.
Something similar has been done for pSeries in 9b09c6d "powerpc: Change
the default link address for pSeries zImage kernels". Would it be
appropriate to move initial address to 64 MiB as the default loading
address or do we have here some boards which have only 64 MiB of memory?
Does someone have another idea?

--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -56,7 +56,7 @@ static struct addr_range prep_kernel(void)
 	if (platform_ops.vmlinux_alloc) {
 		addr = platform_ops.vmlinux_alloc(ei.memsize);
 	} else {
-		if ((unsigned long)_start < ei.memsize)
+		if ((unsigned long)_start < ei.loadsize)
 			fatal("Insufficient memory for kernel at address 0!"
 			       " (_start=%p)\n\r", _start);
 	}
-- 

--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -138,7 +138,7 @@ objflags=-S
 tmp=$tmpdir/zImage.$$.o
 ksection=.kernel:vmlinux.strip
 isection=.kernel:initrd
-link_address='0x400000'
+link_address='0x800000'
 
 case "$platform" in
 pseries)
-- 
1.5.6.5

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-09-23 20:38 [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Sebastian Siewior
@ 2008-09-24  1:46 ` Milton Miller
  2008-09-25 10:21   ` Sebastian Siewior
  2008-09-25 21:43   ` Sebastian Siewior
  2008-09-25 18:15 ` [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Scott Wood
  1 sibling, 2 replies; 12+ messages in thread
From: Milton Miller @ 2008-09-24  1:46 UTC (permalink / raw)
  To: Sebastian Siewior; +Cc: linuxppc-dev, David Gibson

On Wed Sep 24 at about 06:38:57 EST in 2008, Sebastian Siewior wrote:
> My mylinux binary incl. bss is ~5 MiB without bss less than 4 MiB.
> Therefore I though that I could replace ei.memsize with ei.loadsize. It
> didn't work. I'm not sure why it did not work but I guess that the
> memset() of bss in the initial kernel code overwrote the cuimage code
> which is required for some reason. Maybe some device-tree callbacks.

probably because the bss extended beyond the cuboot _end to include
where your device tree was copied (just a malloc and we start
simple_malloc at the boot _end on most platforms).

> 
> My current (working) solution is to move cuImage from 4 MiB to 8 MiB.
> Something similar has been done for pSeries in 9b09c6d "powerpc: Change
> the default link address for pSeries zImage kernels". Would it be
> appropriate to move initial address to 64 MiB as the default loading
> address or do we have here some boards which have only 64 MiB of memory?

I think there are some boards that have even less (8xx?).  Is the
link address a wrapper option yet?

> Does someone have another idea?
> 
> --- a/arch/powerpc/boot/main.c
> +++ b/arch/powerpc/boot/main.c
> @@ -56,7 +56,7 @@ static struct addr_range prep_kernel(void)
>  	if (platform_ops.vmlinux_alloc) {
>  		addr = platform_ops.vmlinux_alloc(ei.memsize);
>  	} else {
> -		if ((unsigned long)_start < ei.memsize)
> +		if ((unsigned long)_start < ei.loadsize)


I think this needs to be a two part test (add approprate casts):  
	_start < ei.loadsize || _end < ei.memsize

.. and a comment explaining why.

We could do better if we kept allocating more memory until we got
above it, but the current code has no such provision.  (prpmc2800, or
one of those, actually decompresses the header to peek at memsize
before starting the simple_alloc code).

>  			fatal("Insufficient memory for kernel at address 0!"
>  			       " (_start=%p)\n\r", _start);
>  	}
> -- 
> 
> --- a/arch/powerpc/boot/wrapper
> +++ b/arch/powerpc/boot/wrapper
> @@ -138,7 +138,7 @@ objflags=-S
>  tmp=$tmpdir/zImage.$$.o
>  ksection=.kernel:vmlinux.strip
>  isection=.kernel:initrd
> -link_address='0x400000'
> +link_address='0x800000'
>  
>  case "$platform" in
>  pseries)
> -- 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-09-24  1:46 ` Milton Miller
@ 2008-09-25 10:21   ` Sebastian Siewior
  2008-09-25 21:43   ` Sebastian Siewior
  1 sibling, 0 replies; 12+ messages in thread
From: Sebastian Siewior @ 2008-09-25 10:21 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, David Gibson

Milton Miller wrote:
>> My current (working) solution is to move cuImage from 4 MiB to 8 MiB.
>> Something similar has been done for pSeries in 9b09c6d "powerpc: Change
>> the default link address for pSeries zImage kernels". Would it be
>> appropriate to move initial address to 64 MiB as the default loading
>> address or do we have here some boards which have only 64 MiB of memory?
> 
> I think there are some boards that have even less (8xx?).  Is the
> link address a wrapper option yet?

Nope. The wrapper script distinguishes between platforms. Default is 4MiB, 
pSeries has 64MiB, coff does 5MiB and the PS3 has other magic.

>> --- a/arch/powerpc/boot/main.c
>> +++ b/arch/powerpc/boot/main.c
>> @@ -56,7 +56,7 @@ static struct addr_range prep_kernel(void)
>>  	if (platform_ops.vmlinux_alloc) {
>>  		addr = platform_ops.vmlinux_alloc(ei.memsize);
>>  	} else {
>> -		if ((unsigned long)_start < ei.memsize)
>> +		if ((unsigned long)_start < ei.loadsize)
> 
> 
> I think this needs to be a two part test (add approprate casts):  
> 	_start < ei.loadsize || _end < ei.memsize
> 
> .. and a comment explaining why.

okay.

> We could do better if we kept allocating more memory until we got
> above it, but the current code has no such provision.  (prpmc2800, or
> one of those, actually decompresses the header to peek at memsize
> before starting the simple_alloc code).

I don't thing I can follow. Do you want to move the bootwrapper code above 
the limit and recall it?

Sebastian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-09-23 20:38 [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Sebastian Siewior
  2008-09-24  1:46 ` Milton Miller
@ 2008-09-25 18:15 ` Scott Wood
  1 sibling, 0 replies; 12+ messages in thread
From: Scott Wood @ 2008-09-25 18:15 UTC (permalink / raw)
  To: Sebastian Siewior; +Cc: linuxppc-dev

On Tue, Sep 23, 2008 at 10:38:57PM +0200, Sebastian Siewior wrote:
> My current (working) solution is to move cuImage from 4 MiB to 8 MiB.
> Something similar has been done for pSeries in 9b09c6d "powerpc: Change
> the default link address for pSeries zImage kernels". Would it be
> appropriate to move initial address to 64 MiB as the default loading
> address or do we have here some boards which have only 64 MiB of memory?
> Does someone have another idea?

We have at least one supported board with only 8 MiB.  

Some options:
1. Dynamically relocate the wrapper to the top of RAM.
2. Relink based on the uncompressed size of the image bing wrapped.
3. Try to fall back on regular malloc() rather than fail if there's no
vmlinux_alloc() and there's no room at zero.

-Scott

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-09-24  1:46 ` Milton Miller
  2008-09-25 10:21   ` Sebastian Siewior
@ 2008-09-25 21:43   ` Sebastian Siewior
  2008-10-10  3:58     ` Benjamin Herrenschmidt
  1 sibling, 1 reply; 12+ messages in thread
From: Sebastian Siewior @ 2008-09-25 21:43 UTC (permalink / raw)
  To: Milton Miller; +Cc: linuxppc-dev, David Gibson

* Milton Miller | 2008-09-23 20:46:18 [-0500]:

>On Wed Sep 24 at about 06:38:57 EST in 2008, Sebastian Siewior wrote:
>> My mylinux binary incl. bss is ~5 MiB without bss less than 4 MiB.
>> Therefore I though that I could replace ei.memsize with ei.loadsize. It
>> didn't work. I'm not sure why it did not work but I guess that the
>> memset() of bss in the initial kernel code overwrote the cuimage code
>> which is required for some reason. Maybe some device-tree callbacks.
>
>probably because the bss extended beyond the cuboot _end to include
>where your device tree was copied (just a malloc and we start
>simple_malloc at the boot _end on most platforms).

Right, the bss section went past _dtb_start, moving the device tree
helps.

Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
---
 arch/powerpc/boot/libfdt-wrapper.c |   16 ++++++++--------
 arch/powerpc/boot/main.c           |   12 +++++++++++-
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
index c541fd8..1daa73f 100644
--- a/arch/powerpc/boot/libfdt-wrapper.c
+++ b/arch/powerpc/boot/libfdt-wrapper.c
@@ -165,6 +165,7 @@ static unsigned long fdt_wrapper_finalize(void)
 void fdt_init(void *blob)
 {
 	int err;
+	int bufsize;
 
 	dt_ops.finddevice = fdt_wrapper_finddevice;
 	dt_ops.getprop = fdt_wrapper_getprop;
@@ -178,16 +179,15 @@ void fdt_init(void *blob)
 
 	/* Make sure the dt blob is the right version and so forth */
 	fdt = blob;
-	err = fdt_open_into(fdt, fdt, fdt_totalsize(blob));
-	if (err == -FDT_ERR_NOSPACE) {
-		int bufsize = fdt_totalsize(fdt) + 4;
-		buf = malloc(bufsize);
-		err = fdt_open_into(fdt, buf, bufsize);
-	}
+	bufsize = fdt_totalsize(fdt) + 4;
+	buf = malloc(bufsize);
+	if(!buf)
+		fatal("malloc failed. can't relocate the device tree\n\r");
+
+	err = fdt_open_into(fdt, buf, bufsize);
 
 	if (err != 0)
 		fatal("fdt_init(): %s\n\r", fdt_strerror(err));
 
-	if (buf)
-		fdt = buf;
+	fdt = buf;
 }
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index 45a81c3..f5fcd14 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -56,9 +56,19 @@ static struct addr_range prep_kernel(void *chosen)
 	if (platform_ops.vmlinux_alloc) {
 		addr = platform_ops.vmlinux_alloc(ei.memsize);
 	} else {
+		/*
+		 * Check if the kernel image (without bss) would overwrite the
+		 * bootwrapper. The device tree has been moved in fdt_init()
+		 * to an area allocated with malloc() (somewhere past _end).
+		 */
 		if ((unsigned long)_start < ei.loadsize)
 			fatal("Insufficient memory for kernel at address 0!"
-			       " (_start=%p)\n\r", _start);
+			       " (_start=%p, uncomressed size=%08x)\n\r",
+			       _start, ei.loadsize);
+
+		if ((unsigned long)_end < ei.memsize)
+			fatal("The final kernel image would overwrite the "
+					"device tree\n\r");
 	}
 
 	/* Finally, gunzip the kernel */
-- 
1.5.6.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-09-25 21:43   ` Sebastian Siewior
@ 2008-10-10  3:58     ` Benjamin Herrenschmidt
  2008-10-13  9:15       ` [PATCH] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 12+ messages in thread
From: Benjamin Herrenschmidt @ 2008-10-10  3:58 UTC (permalink / raw)
  To: Sebastian Siewior; +Cc: linuxppc-dev, Milton Miller, David Gibson

On Thu, 2008-09-25 at 23:43 +0200, Sebastian Siewior wrote:
> * Milton Miller | 2008-09-23 20:46:18 [-0500]:
> 
> >On Wed Sep 24 at about 06:38:57 EST in 2008, Sebastian Siewior wrote:
> >> My mylinux binary incl. bss is ~5 MiB without bss less than 4 MiB.
> >> Therefore I though that I could replace ei.memsize with ei.loadsize. It
> >> didn't work. I'm not sure why it did not work but I guess that the
> >> memset() of bss in the initial kernel code overwrote the cuimage code
> >> which is required for some reason. Maybe some device-tree callbacks.
> >
> >probably because the bss extended beyond the cuboot _end to include
> >where your device tree was copied (just a malloc and we start
> >simple_malloc at the boot _end on most platforms).
> 
> Right, the bss section went past _dtb_start, moving the device tree
> helps.
> 
> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>

Please repost in correct form, that is with the changeset comment.

Cheers,
Ben.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-10-10  3:58     ` Benjamin Herrenschmidt
@ 2008-10-13  9:15       ` Sebastian Andrzej Siewior
  2008-10-14  1:57         ` David Gibson
  0 siblings, 1 reply; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2008-10-13  9:15 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Milton Miller, David Gibson

If the vmlinux binary in memory is larger than 4 MiB than it collides
with the initial boot code which is linked at 4 MiB in case of cuBoot.
If the the uncompressed image size (on disk size) is less than 4 MiB
then it would fit. The difference between those two sizes is the bss
section. In cuBoot we have the dtb embedded right after the data
section so it is very likely that the reset of the bss section (in
kernel's start up code) will overwrite the dtb blob. Therefore we
reallocate the dtb. Something similar is allready done to the initrd.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/powerpc/boot/libfdt-wrapper.c |   16 ++++++++--------
 arch/powerpc/boot/main.c           |   14 ++++++++++++--
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
index c541fd8..1daa73f 100644
--- a/arch/powerpc/boot/libfdt-wrapper.c
+++ b/arch/powerpc/boot/libfdt-wrapper.c
@@ -165,6 +165,7 @@ static unsigned long fdt_wrapper_finalize(void)
 void fdt_init(void *blob)
 {
 	int err;
+	int bufsize;
 
 	dt_ops.finddevice = fdt_wrapper_finddevice;
 	dt_ops.getprop = fdt_wrapper_getprop;
@@ -178,16 +179,15 @@ void fdt_init(void *blob)
 
 	/* Make sure the dt blob is the right version and so forth */
 	fdt = blob;
-	err = fdt_open_into(fdt, fdt, fdt_totalsize(blob));
-	if (err == -FDT_ERR_NOSPACE) {
-		int bufsize = fdt_totalsize(fdt) + 4;
-		buf = malloc(bufsize);
-		err = fdt_open_into(fdt, buf, bufsize);
-	}
+	bufsize = fdt_totalsize(fdt) + 4;
+	buf = malloc(bufsize);
+	if(!buf)
+		fatal("malloc failed. can't relocate the device tree\n\r");
+
+	err = fdt_open_into(fdt, buf, bufsize);
 
 	if (err != 0)
 		fatal("fdt_init(): %s\n\r", fdt_strerror(err));
 
-	if (buf)
-		fdt = buf;
+	fdt = buf;
 }
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index 9e7f3dd..ae32801 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -56,9 +56,19 @@ static struct addr_range prep_kernel(void)
 	if (platform_ops.vmlinux_alloc) {
 		addr = platform_ops.vmlinux_alloc(ei.memsize);
 	} else {
-		if ((unsigned long)_start < ei.memsize)
+		/*
+		 * Check if the kernel image (without bss) would overwrite the
+		 * bootwrapper. The device tree has been moved in fdt_init()
+		 * to an area allocated with malloc() (somewhere past _end).
+		 */
+		if ((unsigned long)_start < ei.loadsize)
 			fatal("Insufficient memory for kernel at address 0!"
-			       " (_start=%p)\n\r", _start);
+			       " (_start=%p, uncomressed size=%08x)\n\r",
+			       _start, ei.loadsize);
+
+		if ((unsigned long)_end < ei.memsize)
+			fatal("The final kernel image would overwrite the "
+					"device tree\n\r");
 	}
 
 	/* Finally, gunzip the kernel */
-- 
1.5.6.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-10-13  9:15       ` [PATCH] " Sebastian Andrzej Siewior
@ 2008-10-14  1:57         ` David Gibson
  2008-10-14  2:00           ` David Gibson
  0 siblings, 1 reply; 12+ messages in thread
From: David Gibson @ 2008-10-14  1:57 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: Milton Miller, linuxppc-dev

On Mon, Oct 13, 2008 at 11:15:26AM +0200, Sebastian Andrzej Siewior wrote:
> If the vmlinux binary in memory is larger than 4 MiB than it collides
> with the initial boot code which is linked at 4 MiB in case of cuBoot.
> If the the uncompressed image size (on disk size) is less than 4 MiB
> then it would fit. The difference between those two sizes is the bss
> section. In cuBoot we have the dtb embedded right after the data
> section so it is very likely that the reset of the bss section (in
> kernel's start up code) will overwrite the dtb blob. Therefore we
> reallocate the dtb. Something similar is allready done to the initrd.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This patch looks sound to me.  It does strike me as only a stopgap
until we come up with some better way of managing the early memory,
preferably avoiding the magical fixed 4MiB (or whatever) offset.

Still, it looks safe - since it does check that the wrapper's malloc()
region can't collide with the kernel's BSS.  So, if this little slice
of extra BSS room is useful for the time being, I see no reason not to
apply the patch.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
  2008-10-14  1:57         ` David Gibson
@ 2008-10-14  2:00           ` David Gibson
  2008-11-01 20:00             ` [PATCH] powerpc/boot: allocate more memory for dtb Sebastian Andrzej Siewior
  0 siblings, 1 reply; 12+ messages in thread
From: David Gibson @ 2008-10-14  2:00 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Benjamin Herrenschmidt, linuxppc-dev,
	Milton Miller

On Tue, Oct 14, 2008 at 12:57:53PM +1100, David Gibson wrote:
> On Mon, Oct 13, 2008 at 11:15:26AM +0200, Sebastian Andrzej Siewior wrote:
> > If the vmlinux binary in memory is larger than 4 MiB than it collides
> > with the initial boot code which is linked at 4 MiB in case of cuBoot.
> > If the the uncompressed image size (on disk size) is less than 4 MiB
> > then it would fit. The difference between those two sizes is the bss
> > section. In cuBoot we have the dtb embedded right after the data
> > section so it is very likely that the reset of the bss section (in
> > kernel's start up code) will overwrite the dtb blob. Therefore we
> > reallocate the dtb. Something similar is allready done to the initrd.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> This patch looks sound to me.  It does strike me as only a stopgap
> until we come up with some better way of managing the early memory,
> preferably avoiding the magical fixed 4MiB (or whatever) offset.
> 
> Still, it looks safe - since it does check that the wrapper's malloc()
> region can't collide with the kernel's BSS.  So, if this little slice
> of extra BSS room is useful for the time being, I see no reason not to
> apply the patch.
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>

Oh, one other thing.  Since we are now unconditionally copying the dtb
into a malloc()ed buffer, possibly it would be sensible to add a
little padding to the buffer at that point, so that further device
tree manipulations won't need to reallocate it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] powerpc/boot: allocate more memory for dtb
  2008-10-14  2:00           ` David Gibson
@ 2008-11-01 20:00             ` Sebastian Andrzej Siewior
  2008-11-06  0:16               ` David Gibson
  0 siblings, 1 reply; 12+ messages in thread
From: Sebastian Andrzej Siewior @ 2008-11-01 20:00 UTC (permalink / raw)
  To: David Gibson; +Cc: Milton Miller, linuxppc-dev

* David Gibson | 2008-10-14 13:00:04 [+1100]:

>Oh, one other thing.  Since we are now unconditionally copying the dtb
>into a malloc()ed buffer, possibly it would be sensible to add a
>little padding to the buffer at that point, so that further device
>tree manipulations won't need to reallocate it.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Haven't notice earlier that the other patch went mainline.
EXPAND_GRANULARITY is 1KiB and it is used in expand_buf() if the buffer
has to be increased. I hope that is okay.

All other platforms except prpmc2800 are passing the dtb to fdt_init().
prpmc2800 is reallocating the dtb to a malloced area. I thing I can
remove that part since fdt_init() is doing this anyway.

 arch/powerpc/boot/libfdt-wrapper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
index 1daa73f..0085930 100644
--- a/arch/powerpc/boot/libfdt-wrapper.c
+++ b/arch/powerpc/boot/libfdt-wrapper.c
@@ -179,7 +179,7 @@ void fdt_init(void *blob)
 
 	/* Make sure the dt blob is the right version and so forth */
 	fdt = blob;
-	bufsize = fdt_totalsize(fdt) + 4;
+	bufsize = fdt_totalsize(fdt) + EXPAND_GRANULARITY;
 	buf = malloc(bufsize);
 	if(!buf)
 		fatal("malloc failed. can't relocate the device tree\n\r");
-- 
1.5.6.5

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] powerpc/boot: allocate more memory for dtb
  2008-11-01 20:00             ` [PATCH] powerpc/boot: allocate more memory for dtb Sebastian Andrzej Siewior
@ 2008-11-06  0:16               ` David Gibson
  0 siblings, 0 replies; 12+ messages in thread
From: David Gibson @ 2008-11-06  0:16 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linuxppc-dev, Milton Miller

On Sat, Nov 01, 2008 at 09:00:56PM +0100, Sebastian Andrzej Siewior wrote:
> * David Gibson | 2008-10-14 13:00:04 [+1100]:
> 
> >Oh, one other thing.  Since we are now unconditionally copying the dtb
> >into a malloc()ed buffer, possibly it would be sensible to add a
> >little padding to the buffer at that point, so that further device
> >tree manipulations won't need to reallocate it.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Looks fine.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> Haven't notice earlier that the other patch went mainline.
> EXPAND_GRANULARITY is 1KiB and it is used in expand_buf() if the buffer
> has to be increased. I hope that is okay.
> 
> All other platforms except prpmc2800 are passing the dtb to fdt_init().
> prpmc2800 is reallocating the dtb to a malloced area. I thing I can
> remove that part since fdt_init() is doing this anyway.
> 
>  arch/powerpc/boot/libfdt-wrapper.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
> index 1daa73f..0085930 100644
> --- a/arch/powerpc/boot/libfdt-wrapper.c
> +++ b/arch/powerpc/boot/libfdt-wrapper.c
> @@ -179,7 +179,7 @@ void fdt_init(void *blob)
>  
>  	/* Make sure the dt blob is the right version and so forth */
>  	fdt = blob;
> -	bufsize = fdt_totalsize(fdt) + 4;
> +	bufsize = fdt_totalsize(fdt) + EXPAND_GRANULARITY;
>  	buf = malloc(bufsize);
>  	if(!buf)
>  		fatal("malloc failed. can't relocate the device tree\n\r");

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize
@ 2008-09-25 19:54 Milton Miller
  0 siblings, 0 replies; 12+ messages in thread
From: Milton Miller @ 2008-09-25 19:54 UTC (permalink / raw)
  To: Sebastian Siewior, Milton Miller; +Cc: linuxppc-dev, David Gibson

----- Original Message Follows -----
From: Sebastian Siewior <bigeasy@linutronix.de>
To: Milton Miller <miltonm@bga.com>
Cc: linuxppc-dev@ozlabs.org, David Gibson
<david@gibson.dropbear.id.au>, Grant Likely
<grant.likely@secretlab.ca>
Subject: Re: [RFC] powerpc/boot: compare _start against
ei.loadsize instead ei.memsize
Date: Thu, 25 Sep 2008 12:21:41 +0200
 
> Milton Miller wrote:
>>> My current (working) solution is to move cuImage from 4
>>> MiB to 8 MiB. Something similar has been done for
>>> pSeries in 9b09c6d "powerpc: Change the default link
>>> address for pSeries zImage kernels". Would it be 
>>> appropriate to move initial address to 64 MiB as the
>>> default loading address or do we have here some boards
>> which have only 64 MiB of memory? 
>> I think there are some boards that have even less
>> (8xx?).  Is the link address a wrapper option yet?
> 
> Nope. The wrapper script distinguishes between platforms.
> Default is 4MiB,  pSeries has 64MiB, coff does 5MiB and
> the PS3 has other magic.
 
ok.  I think some of the embedded boards have less than
64M.  On the other hand, my (64 bit with drivers) kernel
static load is over 8M (close to 9).  The needed size
is obviously configuration dependent.
 
There has been talk of reading the size to determine a
suitable address, but its all talk so far.
 
There are two approaches: one is to make it self-
configuring to the minimum for the image you build, 
the other other is to make it a command line option
to the wrapper and then put platform selections in
Kconfig, hidden with embedded for most people.
 
Or we can change the default to say 16M and hope its
enough but not too much for most and not can decode
the platforms for the rest.
 
We could also add a link time check, but we need to
know which platforms, like the OF ones, have a non-
default vmlinux_alloc and don't put the kernel in
place directly (and hence need to skip the check).
 
> >> --- a/arch/powerpc/boot/main.c
> >> +++ b/arch/powerpc/boot/main.c
> >> @@ -56,7 +56,7 @@ static struct addr_range
> prep_kernel(void) >>      if (platform_ops.vmlinux_alloc)
> { >>          addr =
> platform_ops.vmlinux_alloc(ei.memsize); >>      } else {
> >> -        if ((unsigned long)_start < ei.memsize)
> >> +        if ((unsigned long)_start < ei.loadsize)
> > 
> > 
> > I think this needs to be a two part test (add
appropriate
> >     casts):   _start < ei.loadsize || _end < ei.memsize
> > 
> > .. and a comment explaining why.
> 
> okay.
> 
> > We could do better if we kept allocating more memory
> > until we got above it, but the current code has no such
> > provision.  (prpmc2800, or one of those, actually
> > decompresses the header to peek at memsize before
> starting the simple_alloc code).
> 
> I don't thing I can follow. Do you want to move the
> bootwrapper code above  the limit and recall it?
 
I'm saying the code could be changed to do better, but
am not currently requesting any change.
 
milton

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2008-11-06  0:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-09-23 20:38 [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Sebastian Siewior
2008-09-24  1:46 ` Milton Miller
2008-09-25 10:21   ` Sebastian Siewior
2008-09-25 21:43   ` Sebastian Siewior
2008-10-10  3:58     ` Benjamin Herrenschmidt
2008-10-13  9:15       ` [PATCH] " Sebastian Andrzej Siewior
2008-10-14  1:57         ` David Gibson
2008-10-14  2:00           ` David Gibson
2008-11-01 20:00             ` [PATCH] powerpc/boot: allocate more memory for dtb Sebastian Andrzej Siewior
2008-11-06  0:16               ` David Gibson
2008-09-25 18:15 ` [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize Scott Wood
2008-09-25 19:54 Milton Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).