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

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?

arch/powerpc/boot/main.c +0 -0

 			fatal("Insufficient memory for kernel at address 0!"
 			       " (_start=%p)\n\r", _start);
 	}

arch/powerpc/boot/wrapper +1 -1

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

Quoted 5 lines

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?

arch/powerpc/boot/main.c static struct addr_range prep_kernel(void) · line 59

Quoted 8 lines

arch/powerpc/boot/main.c +1 -1

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

Quoted 5 lines · 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.

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

Quoted 7 lines · * Milton Miller | 2008-09-23 20:46:18 [-0500]:

Quoted 5 lines · 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.

Quoted 3 lines

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 <[email protected]>

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 <[email protected]>

Quoted 10 lines · 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 <[email protected]>

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 <[email protected]>

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.

Quoted 4 lines · * 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 <[email protected]>

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

arch/powerpc/boot/libfdt-wrapper.c +1 -1

index 1daa73f..0085930 100644
@@ -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");

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?