fix some issues with the kernel decompressor

In commit d239b1dc093d the hardcoded 4x estimate for the decompressed kernel size was replaced by the exact Image file size and passed to the linker as a symbol value. Turns out that this is unneeded as the size is already included at the end of the compressed piggy data. For those compressed formats that don't include this data, the build system already takes care of appending it using size_append in scripts/Makefile.lib. So let's use that instead.

Signed-off-by Nicolas Pitre <[email protected]>

The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it.

Signed-off-by Nicolas Pitre <[email protected]>

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me.

The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it.

Signed-off-by Nicolas Pitre <[email protected]>

---

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me.

Quoted 10 lines · Nicolas Pitre

The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it.

Signed-off-by Nicolas Pitre <[email protected]>

---

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me.

Hmm. I've switched almost entirely to LZMA since it was introduced and it works for me on all my platforms. I haven't tested kernels rigorously since your patches to the decompressor, so maybe there's a regression in there somewhere.

Can you try LZMA with your various patches over the last four months reverted?

Quoted 12 lines · Russell King - ARM Linux

Quoted 10 lines · Nicolas Pitre

The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it.

Signed-off-by Nicolas Pitre <[email protected]>

---

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me.

Quoted 7 lines

Hmm. I've switched almost entirely to LZMA since it was introduced and it works for me on all my platforms. I haven't tested kernels rigorously since your patches to the decompressor, so maybe there's a regression in there somewhere.

Can you try LZMA with your various patches over the last four months reverted?

Yes I did, and the issue turned up to be about the stack alignment wich was not enforced to a 64 bit boundary. Depending on the size of the code and compressed kernel data, the stack would end up properly aligned with a 50% probability. And this was a problem only if your gcc version emited LDRD/STRD type accesses to the stack.

I'm about to send you a pull request with all those fixes.

Quoted 12 lines · Russell King - ARM Linux

Quoted 10 lines · Nicolas Pitre

The return value for decompress_kernel() is no longer used. Furthermore, this was obtained and stored in a variable called output_ptr which is a complete misnomer for what is actually the size of the decompressed kernel image. Let's get rid of it.

Signed-off-by Nicolas Pitre <[email protected]>

---

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA work for me.

Quoted 7 lines

Hmm. I've switched almost entirely to LZMA since it was introduced and it works for me on all my platforms. I haven't tested kernels rigorously since your patches to the decompressor, so maybe there's a regression in there somewhere.

Can you try LZMA with your various patches over the last four months reverted?

Yes I did, and the issue turned up to be about the stack alignment wich was not enforced to a 64 bit boundary. Depending on the size of the code and compressed kernel data, the stack would end up properly aligned with a 50% probability. And this was a problem only if your gcc version emited LDRD/STRD type accesses to the stack.

I'm about to send you a pull request with all those fixes.

Just to summarize, looks like there are three regressions:

1. The stack alignment mentioned above

2. Stack overwriting relocated kernel with cache flush in some cases on ARMv7

3. Reloction overwriting running code when the relocate offset is small

These fixes would be nice to get merged in the -rc cycle. Then the other patches like the static usage in uncompress.h can probably wait until the merge window.

Quoted 3 lines · Tony Lindgren

Just to summarize, looks like there are three regressions:

1. The stack alignment mentioned above

This one has been there forever. I marked it for inclusion into the stable tree.

Quoted 1 line

2. Stack overwriting relocated kernel with cache flush in some cases on ARMv7

Yes, and I reverted to your original patch. I think I agree with RMK about the fact that having a stack available is more sane than trying to squeeze everything into registers (including sp as my patch did).

Quoted 1 line

3. Reloction overwriting running code when the relocate offset is small

Yep. However I agree with you that having a constant like my patch did is prone to failure if the code grows big enough (unlikely but still). However your usage of the GOT end is overshooting as only the copy loop and cache flush code has to be protected. I have an alternative patch using an explicit symbol to mark critical code.

Quoted 3 lines

These fixes would be nice to get merged in the -rc cycle. Then the other patches like the static usage in uncompress.h can probably wait until the merge window.

Right. I have sorted them so Russell can send a pointer to the first part to Linus and keep the rest for later. I'll repost the whole set soon anyway.

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that it won't come back, let's simply
define it out.

Signed-off-by Nicolas Pitre <[email protected]>

---

This should fix remaining issues some people have with the DT append patch.

 arch/arm/boot/compressed/misc.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/boot/compressed/misc.c +4 -0

index a565853..0125dae 100644
@@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
 static void putstr(const char *ptr);
 extern void error(char *x);
 
+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot

Quoted 4 lines

arch/arm/boot/compressed/misc.c +4 -0

+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 10 lines

arch/arm/boot/compressed/misc.c +9 -0

+ * be modified at run time.  The entry code in head.S relies on the ability
+ * to move writable sections around, and for that to work, we must have all
+ * references going through the GOT which works only with non static
+ * variables.  So, instead of asking for a non intuitive requirement
+ * making many files non standard according to accepted coding practices
+ * we fix the issue here by simply defining the static keyword to nothing.
+ */
+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

	static something *ptr;

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Quoted 28 lines · Nicolas Pitre

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that it won't come back, let's simply
define it out.

Signed-off-by Nicolas Pitre <[email protected]>

---

This should fix remaining issues some people have with the DT append patch.

 arch/arm/boot/compressed/misc.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/boot/compressed/misc.c +4 -0

index a565853..0125dae 100644
@@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
 static void putstr(const char *ptr);
 extern void error(char *x);
 
+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 10 lines

arch/arm/boot/compressed/misc.c +9 -0

+ * be modified at run time.  The entry code in head.S relies on the ability
+ * to move writable sections around, and for that to work, we must have all
+ * references going through the GOT which works only with non static
+ * variables.  So, instead of asking for a non intuitive requirement
+ * making many files non standard according to accepted coding practices
+ * we fix the issue here by simply defining the static keyword to nothing.
+ */
+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

	static something *ptr;

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Quoted 6 lines · Uwe Kleine-K?nig

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 3 lines · Nicolas Pitre

arch/arm/boot/compressed/misc.c +2 -0

+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

Quoted 4 lines

	static something *ptr;

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Please tell me more about your setup. This should still be initialized to NULL. If not then something else is still wrong.

Hello,

Quoted 9 lines · Nicolas Pitre

Quoted 8 lines · Uwe Kleine-K?nig

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 3 lines · Nicolas Pitre

arch/arm/boot/compressed/misc.c +2 -0

+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

	static something *ptr;

Quoted 5 lines

Quoted 2 lines

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Please tell me more about your setup. This should still be initialized to NULL. If not then something else is still wrong.

I didn't test your branch. If you say that non-static data is zero-initialised, too, I believe you without further checking.

Uninitialized variables are allocated to the .bss section, regardless if they're static or not. The static keyword only affects the global visibility of the variable,

Quoted 3 lines · Nicolas Pitre

Uninitialized variables are allocated to the .bss section, regardless if they're static or not. The static keyword only affects the global visibility of the variable,

Wrong.

int foo(void)

{
	static int bar = 1;
	return bar++;
}

With static allowed to have its normal meaning, this function will return 1 on the first call, 2 on the second, etc. With static defined away, it will always return 1. That's a behavioural change.

Quoted 41 lines · Uwe Kleine-K?nig

Quoted 28 lines · Nicolas Pitre

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that it won't come back, let's simply
define it out.

Signed-off-by Nicolas Pitre <[email protected]>

---

This should fix remaining issues some people have with the DT append patch.

 arch/arm/boot/compressed/misc.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/boot/compressed/misc.c +4 -0

index a565853..0125dae 100644
@@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
 static void putstr(const char *ptr);
 extern void error(char *x);
 
+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 10 lines

arch/arm/boot/compressed/misc.c +9 -0

+ * be modified at run time.  The entry code in head.S relies on the ability
+ * to move writable sections around, and for that to work, we must have all
+ * references going through the GOT which works only with non static
+ * variables.  So, instead of asking for a non intuitive requirement
+ * making many files non standard according to accepted coding practices
+ * we fix the issue here by simply defining the static keyword to nothing.
+ */
+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

Quoted 4 lines

	static something *ptr;

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Please tell me more about your setup. This should still be initialized to NULL. If not then something else is still wrong.

Quoted 45 lines · Nicolas Pitre

Quoted 43 lines · Uwe Kleine-K?nig

Quoted 28 lines · Nicolas Pitre

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that it won't come back, let's simply
define it out.

Signed-off-by Nicolas Pitre <[email protected]>

---

This should fix remaining issues some people have with the DT append patch.

 arch/arm/boot/compressed/misc.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/boot/compressed/misc.c +4 -0

index a565853..0125dae 100644
@@ -30,7 +30,20 @@ unsigned int __machine_arch_type;
 static void putstr(const char *ptr);
 extern void error(char *x);
 
+/*
+ * Many instances of mach/uncompress.h are including global variables.
+ * Contrary to standard usage, we should _not_ mark those variables
+ * static otherwise they get accessed via GOTOFF references which cannot

arch/arm/boot/compressed/misc.c unsigned int __machine_arch_type; · line 46

Quoted 10 lines

arch/arm/boot/compressed/misc.c +9 -0

+ * be modified at run time.  The entry code in head.S relies on the ability
+ * to move writable sections around, and for that to work, we must have all
+ * references going through the GOT which works only with non static
+ * variables.  So, instead of asking for a non intuitive requirement
+ * making many files non standard according to accepted coding practices
+ * we fix the issue here by simply defining the static keyword to nothing.
+ */
+#define static /* non-static */
 #include <mach/uncompress.h>
+#undef static

This has a strange side effect, i.e.

	static something *ptr;

Quoted 6 lines

Quoted 2 lines

isn't initialised to NULL anymore IIRC. So the maintainers of the various mach/uncompress.h still need to be aware of the issue.

Please tell me more about your setup. This should still be initialized to NULL. If not then something else is still wrong.

Though I'm unsure if it realistically exists, what if it's a static variable inside a function? It's actually changing something, right?

Quoted 2 lines · Shawn Guo

Though I'm unsure if it realistically exists, what if it's a static variable inside a function? It's actually changing something, right?

Right. In that case the variable is allocated to the .data section, but we explicitly discard .data at link time to prevent a successful build.

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Instead of removing the static keyword from all those files and hope that it won't come back, let's simply
define it out.

Signed-off-by Nicolas Pitre <[email protected]>

---

This should fix remaining issues some people have with the DT append patch.

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

Quoted 3 lines · Nicolas Pitre

Many architecture specific versions of uncompress.h make use of global variables marked static. In such case the GOT is bypassed and the code in head.S can't relocate them.

No, we're not doing this - it creates confusion with static variables declared inside functions.

It's far easier to audit for the presence of 'static' than it is for the possibility of an ignored 'static' inside a function body.

Ensure that the uncompress.h header files are right.

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

Quoted 3 lines · Nicolas Pitre

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

Weird still no luck here booting n900 zImage.. LZMA image prints corrupt error, and only works if the image is relocated further by 1MB or so. With the following hack n900 boots, so some size calculations still are wrong. No luck with DT append patch either.

Any ideas why the following helps on n900?

Quoted 10 lines · Tony Lindgren

Quoted 3 lines · Nicolas Pitre

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

Weird still no luck here booting n900 zImage.. LZMA image prints corrupt error, and only works if the image is relocated further by 1MB or so. With the following hack n900 boots, so some size calculations still are wrong. No luck with DT append patch either.

Quoted 1 line

Any ideas why the following helps on n900?

No idea. This could indicate that the lzma code is crapping into memory outside of its output buffer. The fact that my patch #2/3 makes lzma decompression suddenly work for me with no apparent explanation is not reassuring either.

Quoted 14 lines · Nicolas Pitre

Quoted 11 lines · Tony Lindgren

Quoted 3 lines · Nicolas Pitre

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

Weird still no luck here booting n900 zImage.. LZMA image prints corrupt error, and only works if the image is relocated further by 1MB or so. With the following hack n900 boots, so some size calculations still are wrong. No luck with DT append patch either.

Any ideas why the following helps on n900?

Quoted 4 lines

No idea. This could indicate that the lzma code is crapping into memory outside of its output buffer. The fact that my patch #2/3 makes lzma decompression suddenly work for me with no apparent explanation is not reassuring either.

With gzip kernel looks like I need to move the kernel further by 8MB instead of 1MB for some reason..

Quoted 20 lines · Tony Lindgren

Quoted 18 lines · Nicolas Pitre

Quoted 11 lines · Tony Lindgren

Quoted 3 lines · Nicolas Pitre

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

Weird still no luck here booting n900 zImage.. LZMA image prints corrupt error, and only works if the image is relocated further by 1MB or so. With the following hack n900 boots, so some size calculations still are wrong. No luck with DT append patch either.

Any ideas why the following helps on n900?

No idea. This could indicate that the lzma code is crapping into memory outside of its output buffer. The fact that my patch #2/3 makes lzma decompression suddenly work for me with no apparent explanation is not reassuring either.

Quoted 2 lines

With gzip kernel looks like I need to move the kernel further by 8MB instead of 1MB for some reason..

What kernel config are you using?

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

Quoted 5 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

I think your patch is 1) unneeded (see the copy loop code and the comment before it), and 2) simply hiding the real bug.

I just need to modify the code in compressed/misc.c slightly for the lzma decompressor to start or stop working randomly. It seems that this code might be sensitive to slight displacement in memory caused by modifications to totally unrelated code. I'm still trying to track this down.

Quoted 14 lines · Nicolas Pitre

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

Quoted 8 lines

I think your patch is 1) unneeded (see the copy loop code and the comment before it), and 2) simply hiding the real bug.

I just need to modify the code in compressed/misc.c slightly for the lzma decompressor to start or stop working randomly. It seems that this code might be sensitive to slight displacement in memory caused by modifications to totally unrelated code. I'm still trying to track this down.

I found the bugger. The problem was a bad stack alignment.

----- >8

From: Nicolas Pitre <[email protected]>

ARM: zImage: make sure the stack is 64-bit aligned

With ARMv5+ and EABI, the compiler expects a 64-bit aligned stack so instructions like STRD and LDRD can be used. Without this, mysterious boot failures were seen semi randomly with the LZMA decompressor.

While at it, let's align .bss as well.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/boot/compressed/Makefile +1 -1

index 58ac434..79b5c62 100644
@@ -74,7 +74,7 @@ ZTEXTADDR	:= $(CONFIG_ZBOOT_ROM_TEXT)
 ZBSSADDR	:= $(CONFIG_ZBOOT_ROM_BSS)
 else
 ZTEXTADDR	:= 0
-ZBSSADDR	:= ALIGN(4)
+ZBSSADDR	:= ALIGN(8)
 endif
 
 SEDFLAGS	= s/TEXT_START/$(ZTEXTADDR)/;s/BSS_START/$(ZBSSADDR)/

arch/arm/boot/compressed/vmlinux.lds.in +0 -0

index 5309909..ea80abe 100644

Quoted 38 lines

----- >8

From: Nicolas Pitre <[email protected]>

ARM: zImage: make sure the stack is 64-bit aligned

With ARMv5+ and EABI, the compiler expects a 64-bit aligned stack so instructions like STRD and LDRD can be used. Without this, mysterious boot failures were seen semi randomly with the LZMA decompressor.

While at it, let's align .bss as well.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/boot/compressed/Makefile +1 -1

index 58ac434..79b5c62 100644
@@ -74,7 +74,7 @@ ZTEXTADDR	:= $(CONFIG_ZBOOT_ROM_TEXT)
 ZBSSADDR	:= $(CONFIG_ZBOOT_ROM_BSS)
 else
 ZTEXTADDR	:= 0
-ZBSSADDR	:= ALIGN(4)
+ZBSSADDR	:= ALIGN(8)
 endif
 
 SEDFLAGS	= s/TEXT_START/$(ZTEXTADDR)/;s/BSS_START/$(ZBSSADDR)/

arch/arm/boot/compressed/vmlinux.lds.in +1 -0

index 5309909..ea80abe 100644
@@ -54,6 +54,7 @@ SECTIONS
   .bss			: { *(.bss) }
   _end = .;
 
+  . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
 

arch/arm/boot/compressed/vmlinux.lds.in SECTIONS · line 60

Quoted 2 lines

arch/arm/boot/compressed/vmlinux.lds.in +0 -0

   .stab 0		: { *(.stab) }

So this is the [PATCH 1/3] in the same set with following two?

[PATCH 2/3] ARM: zImage: don't ignore error returned from decompress() [PATCH 3/3] ARM: zImage: the page table memory must be considered before relocation

Quoted 8 lines · Nicolas Pitre

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

I think your patch is 1) unneeded (see the copy loop code and the comment before it), and 2) simply hiding the real bug.

Quoted 5 lines

I just need to modify the code in compressed/misc.c slightly for the lzma decompressor to start or stop working randomly. It seems that this code might be sensitive to slight displacement in memory caused by modifications to totally unrelated code. I'm still trying to track this down.

I found the bugger. The problem was a bad stack alignment.

Quoted 1 line · Nicolas Pitre

I found the bugger. The problem was a bad stack alignment.

.. as this patch won't solve the n900 booting problem with zImage. With LZMA I'm still also getting "LZMA data is corrupt".

Quoted 3 lines · Tony Lindgren

Quoted 1 line · Nicolas Pitre

I found the bugger. The problem was a bad stack alignment.

Quoted 2 lines

.. as this patch won't solve the n900 booting problem with zImage. With LZMA I'm still also getting "LZMA data is corrupt".

Hmmm......

Is it possible you have bad RAM? In compressed/head.S, locate this code:

#ifdef CONFIG_AUTO_ZRELADDR
@ determine final kernel image address
mov r4, pc
and r4, r4, #0xf8000000
add r4, r4, #TEXT_OFFSET
#else
ldr r4, =zreladdr
#endif

Right after that, simply override r4 with a physical address towards the end of the RAM, say 8MB before end of RAM (unless your decompressed kernel is larger than that). That won't make a booting system, but at least you will be able to test the decompressor when loaded at various locations in memory without involving the relocation loop.

Otherwise cache_clean_flush can overwrite some of the relocated area depending on where the kernel image gets loaded. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Thanks to Aaro Koskinen <[email protected]> for debugging the address of the relocated area that gets corrupted, and to Nicolas Pitre <[email protected]> for the other uncompress related fixes.

Signed-off-by Tony Lindgren <[email protected]>

Quoted 4 lines · Tony Lindgren

Otherwise cache_clean_flush can overwrite some of the relocated area depending on where the kernel image gets loaded. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Gaaaah. Indeed.

Quoted 6 lines

Thanks to Aaro Koskinen <[email protected]> for debugging the address of the relocated area that gets corrupted, and to Nicolas Pitre <[email protected]> for the other uncompress related fixes.

Signed-off-by Tony Lindgren <[email protected]>

I think there could be a better fix yet. Could you test this patch:

Quoted 4 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Quoted 1 line

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

Hmmm...

Quoted 5 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Tony Lindgren

Otherwise we end up overwriting ourselves. This fixes booting on n900 after commit 6d7d0ae51574943bf571d269da3243257a2d15db (ARM: 6750/1: improvements to compressed/head.S).

Signed-off-by Tony Lindgren <[email protected]>

I don't understand why this is needed. The copy loop is explicitly copying from the end going backward exactly to cope with this possibility.

This one is starting to make sense now too after the stack corrupting the image issue is out of the way :)

We can't overwrite the running code when relocating only a small amount, say 0x100 or so.

There's no need to relocate all the way past the compressed kernel, we just need to relocate past the size of the code in head.o.

Updated patch below using the GOT end instead of the compressed image end.

Quoted 8 lines · Tony Lindgren

We can't overwrite the running code when relocating only a small amount, say 0x100 or so.

There's no need to relocate all the way past the compressed kernel, we just need to relocate past the size of the code in head.o.

Updated patch below using the GOT end instead of the compressed image end.

Oops, the mov should be movle of course. Updated patch below.

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Tony Lindgren

We can't overwrite the running code when relocating only a small amount, say 0x100 or so.

There's no need to relocate all the way past the compressed kernel, we just need to relocate past the size of the code in head.o.

Quoted 4 lines

Quoted 2 lines

Updated patch below using the GOT end instead of the compressed image end.

Oops, the mov should be movle of course. Updated patch below.

This is wrong. You're using r12 before it is fixed up with the proper offset.

And this could simply be fixed with a big enough constant like this:

arch/arm/boot/compressed/head.S +0 -0

index 8dab5e3..71fc1d9 100644
@@ -250,8 +250,11 @@ restart:	adr	r0, LC0
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

Quoted 12 lines · Nicolas Pitre

Quoted 10 lines · Tony Lindgren

Quoted 8 lines · Tony Lindgren

We can't overwrite the running code when relocating only a small amount, say 0x100 or so.

There's no need to relocate all the way past the compressed kernel, we just need to relocate past the size of the code in head.o.

Updated patch below using the GOT end instead of the compressed image end.

Quoted 4 lines

Quoted 1 line

Oops, the mov should be movle of course. Updated patch below.

This is wrong. You're using r12 before it is fixed up with the proper offset.

Hmm I see. I guess I was thinking it only needs to be fixed up after the relocation.

Here some patches to fix some issues with the zImage code. The LZMA decompressor now works for me, and the DT append patch should work on all platforms now too.

[PATCH 1/3] ARM: zImage: no need to get the decompressed size from the filesystem [PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()
[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables