[RFC 0/1] am335x kernel not booting

[RFC 0/1] am335x kernel not booting

[Stefan Müller-Klieser]

Dear list,

this is more of a request for help, as I am not sure if my current
analysis is correct. The problem I am having is that the kernel current
master does not boot and fails with:

Error: unrecognized/unsupported machine ID (r1 = 0x00001030).

My setup is:
phyBOARD-WEGA-AM335x
kernel: master with multiv7/omap2plus defconfig
barebox: master with phycore-am335x image

Git bisecting lead to the DEBUG_RODATA change, which is now turned on
for arm and arm64. Google brought up this thread:

http://www.spinics.net/lists/arm-kernel/msg511355.html

Which brought me to the bootm.c in barebox to check if there is enough
space for decompression. I found the assumption of a compression factor
of 4 also in the barebox. What puzzles me is that this assumption still
holds true even for DEBUG_RODATA, at least for my case, though.
With DEBUG_RODATA turned on, the compression ratio went up from 2.9 to
3.5. So my guess is that the kernel is relocating and barebox does not
account for this case. Additionally I found some recommendations in
git/kernel/Documentation/arm/Booting which I think should be taken
into consideration for barebox.
With the attached patch, everything works fine (tested on one single
board 256MiB RAM only).

So, if anyone has more information, I am happy to learn.

Yours, Stefan


Stefan Müller-Klieser (1):
  ARM: bootm: recalculate decompression space

 arch/arm/lib/bootm.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[RFC 1/1] ARM: bootm: recalculate decompression space

[Stefan Müller-Klieser]

According to the kernel documentation it is recommended to place the
compressed image between 32MiB and 128MiB. We will conform to this if we
have at least 64MiB of RAM. If this is not the case, we fall back to the
old scheme but take the relocated image into account.
This is required because of the ARM default kernel config changes
regarding RODATA layout, which lead to an increased compression factor
of the kernel image.

Signed-off-by: Stefan Müller-Klieser <s.mueller-klieser@phytec.de>
---
 arch/arm/lib/bootm.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 803aa94..d4fe9a4 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -73,18 +73,24 @@ static int get_kernel_addresses(size_t image_size,
 {
 	unsigned long mem_start, mem_size;
 	int ret;
-	size_t image_decomp_size;
-	unsigned long spacing;
+	unsigned long decomp_space, spacing;
 
 	ret = sdram_start_and_size(&mem_start, &mem_size);
 	if (ret)
 		return ret;
 
 	/*
-	 * We don't know the exact decompressed size so just use a conservative
-	 * default of 4 times the size of the compressed image.
+	 * The kernel documentation "Documentation/arm/Booting" advises
+	 * to place the compressed image outside of the lowest 32MiB to
+	 * avoid relocation. We should do this if we have at least 64MiB
+	 * of ram. If we have less space, we assume a maximum
+	 * compression factor of 4 plus 1. The latter factor is the
+	 * space for the relocated image.
 	 */
-	image_decomp_size = PAGE_ALIGN(image_size * 4);
+	if (mem_size >= SZ_64M)
+		decomp_space = SZ_32M;
+	else
+		decomp_space = PAGE_ALIGN(image_size * 5);
 
 	/*
 	 * By default put oftree/initrd close behind compressed kernel image to
@@ -97,21 +103,21 @@ static int get_kernel_addresses(size_t image_size,
 		 * Place the kernel at an address where it does not need to
 		 * relocate itself before decompression.
 		 */
-		*load_address = mem_start + image_decomp_size;
+		*load_address = mem_start + decomp_space;
 		if (verbose)
 			printf("no OS load address, defaulting to 0x%08lx\n",
 				*load_address);
-	} else if (*load_address <= mem_start + image_decomp_size) {
+	} else if (*load_address <= mem_start + decomp_space) {
 		/*
 		 * If the user/image specified an address where the kernel needs
 		 * to relocate itself before decompression we need to extend the
 		 * spacing to allow this relocation to happen without
 		 * overwriting anything placed behind the kernel.
 		 */
-		spacing += image_decomp_size;
+		spacing += decomp_space;
 	}
 
-	*mem_free = PAGE_ALIGN(*load_address + image_size + spacing);
+	*mem_free = PAGE_ALIGN(*load_address + decomp_space + spacing);
 
 	return 0;
 }
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 1/1] ARM: bootm: recalculate decompression space

[Sascha Hauer]

Hi Stefan,

On Wed, Jun 22, 2016 at 04:49:04PM +0200, Stefan Müller-Klieser wrote:
> According to the kernel documentation it is recommended to place the
> compressed image between 32MiB and 128MiB. We will conform to this if we
> have at least 64MiB of RAM. If this is not the case, we fall back to the
> old scheme but take the relocated image into account.
> This is required because of the ARM default kernel config changes
> regarding RODATA layout, which lead to an increased compression factor
> of the kernel image.
> 
> Signed-off-by: Stefan Müller-Klieser <s.mueller-klieser@phytec.de>
> ---
>  arch/arm/lib/bootm.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 803aa94..d4fe9a4 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -73,18 +73,24 @@ static int get_kernel_addresses(size_t image_size,
>  {
>  	unsigned long mem_start, mem_size;
>  	int ret;
> -	size_t image_decomp_size;
> -	unsigned long spacing;
> +	unsigned long decomp_space, spacing;

I renamed back decomp_space to image_decomp_size which leads to this
easier to read patch:

> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 803aa94..8630f2c 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -81,10 +81,17 @@ static int get_kernel_addresses(size_t image_size,
>  		return ret;
>  
>  	/*
> -	 * We don't know the exact decompressed size so just use a conservative
> -	 * default of 4 times the size of the compressed image.
> +	 * The kernel documentation "Documentation/arm/Booting" advises
> +	 * to place the compressed image outside of the lowest 32MiB to
> +	 * avoid relocation. We should do this if we have at least 64MiB
> +	 * of ram. If we have less space, we assume a maximum
> +	 * compression factor of 4 plus 1. The latter factor is the
> +	 * space for the relocated image.

Why space for the relocated image? Our mission is to place the image
where it doesn't have to be relocated.

>  	 */
> -	image_decomp_size = PAGE_ALIGN(image_size * 4);
> +	if (mem_size >= SZ_64M)
> +		image_decomp_size = SZ_32M;
> +	else
> +		image_decomp_size = PAGE_ALIGN(image_size * 5);

	image_decomp_size = PAGE_ALIGN(image_size * 5);
	if (mem_size >= SZ_64M)
		image_decomp_size = max(image_decomp_size, SZ_32M);

>  
>  	/*
>  	 * By default put oftree/initrd close behind compressed kernel image to
> @@ -111,7 +118,7 @@ static int get_kernel_addresses(size_t image_size,
>  		spacing += image_decomp_size;
>  	}
>  
> -	*mem_free = PAGE_ALIGN(*load_address + image_size + spacing);
> +	*mem_free = PAGE_ALIGN(*load_address + image_decomp_size + spacing);

Now why this change? We have two cases to consider. First, when the
*load_address is unspecified, we aim for this setup:

|- uncompressed image -||- compressed image -||- spacing -||- free for initrd/oftree -|

The code should be correct for this case without this hunk:

*load_address = mem_start + image_decomp_size
*mem_free = *load_address + image_size + spacing

Second case is when the load address is specified. In this case we
adjust the spacing by image_decomp_size, this also seems to be done
correctly in the current code.

So I think this hunk is wrong.

Instead we should consider adding the following at the end:

	if (mem_size > SZ_256M)
		*mem_free = max(*mem_free, mem_start + SZ_128M);

This would make sure to follow the recommendation to put the
initrd/device tree at 128MiB.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 0/1] am335x kernel not booting

[Sascha Hauer]

On Wed, Jun 22, 2016 at 04:49:03PM +0200, Stefan Müller-Klieser wrote:
> Dear list,
> 
> this is more of a request for help, as I am not sure if my current
> analysis is correct. The problem I am having is that the kernel current
> master does not boot and fails with:
> 
> Error: unrecognized/unsupported machine ID (r1 = 0x00001030).
> 
> My setup is:
> phyBOARD-WEGA-AM335x
> kernel: master with multiv7/omap2plus defconfig
> barebox: master with phycore-am335x image
> 
> Git bisecting lead to the DEBUG_RODATA change, which is now turned on
> for arm and arm64. Google brought up this thread:
> 
> http://www.spinics.net/lists/arm-kernel/msg511355.html
> 
> Which brought me to the bootm.c in barebox to check if there is enough
> space for decompression. I found the assumption of a compression factor
> of 4 also in the barebox. What puzzles me is that this assumption still
> holds true even for DEBUG_RODATA, at least for my case, though.
> With DEBUG_RODATA turned on, the compression ratio went up from 2.9 to
> 3.5. So my guess is that the kernel is relocating and barebox does not
> account for this case.

No, the kernel does not relocate itself. The problem is that the kernel
overwrites the device tree while zeroing the bss segment.

We setup this:

|--- space for uncompressed image ---||-- compressed image --||-- oftree --|

After uncompressing the kernel clears bss and has this:

| ----- uncompressed image ----------||--------- bss -------------|

so bss overlaps the device tree.

> Additionally I found some recommendations in
> git/kernel/Documentation/arm/Booting which I think should be taken
> into consideration for barebox.

I have my problems with these recommendations. The recommendation is to
put the image 32MiB into RAM. a multi_v7_defconfig already builds a 18MiB
image, so it's forseeable that 32MiB won't last very long.

Then the recommendation for placing the device tree and initrd is 128MiB
into RAM which can become very tight when the initrd is big.

These recommendations are written with multigigabyte memories in mind
and completely ignore that there are still small systems with a total of
32MiB of RAM.


That said, I currently have no better idea than to follow these
recommendations and wait until we hit the wall.
Another way would be to supplement the kernel image with the information
how big it will be after decompression. I have a draft patch for that,
but it doesn't work properly yet.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox