[RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Sebastian Siewior]

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

[RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Milton Miller]

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)
> -- 

Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Sebastian Siewior]

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

Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Sebastian Siewior]

* 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

Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Benjamin Herrenschmidt]

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.

[PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Sebastian Andrzej Siewior]

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

Re: [PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[David Gibson]

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

Re: [PATCH] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[David Gibson]

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

[PATCH] powerpc/boot: allocate more memory for dtb

[Sebastian Andrzej Siewior]

* 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

Re: [PATCH] powerpc/boot: allocate more memory for dtb

[David Gibson]

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

Re: [RFC] powerpc/boot: compare _start against ei.loadsize instead ei.memsize

[Scott Wood]

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