[PATCH 0/3] fix some issues with the kernel decompressor

[PATCH 0/3] fix some issues with the kernel decompressor

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

[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

[PATCH 1/3] ARM: zImage: no need to get the decompressed size from the filesystem

[Nicolas Pitre]

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 <nicolas.pitre@linaro.org>
---
 arch/arm/boot/compressed/Makefile |    2 --
 arch/arm/boot/compressed/head.S   |   18 ++++++++++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 8ebbb51..58ac434 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -98,8 +98,6 @@ endif
 ccflags-y := -fpic -fno-builtin
 asflags-y := -Wa,-march=all
 
-# Provide size of uncompressed kernel to the decompressor via a linker symbol.
-LDFLAGS_vmlinux = --defsym _image_size=$(shell stat -c "%s" $(obj)/../Image)
 # Supply ZRELADDR to the decompressor via a linker symbol.
 ifneq ($(CONFIG_AUTO_ZRELADDR),y)
 LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index adf583c..c74f048 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -179,7 +179,7 @@ not_angel:
 		bl	cache_on
 
 restart:	adr	r0, LC0
-		ldmia	r0, {r1, r2, r3, r5, r6, r9, r11, r12}
+		ldmia	r0, {r1, r2, r3, r5, r6, r10, r11, r12}
 		ldr	sp, [r0, #32]
 
 		/*
@@ -189,7 +189,21 @@ restart:	adr	r0, LC0
 		sub	r0, r0, r1		@ calculate the delta offset
 		add	r5, r5, r0		@ _start
 		add	r6, r6, r0		@ _edata
+		add	r10, r10, r0		@ inflated kernel size location
 
+		/*
+		 * The kernel build system appends the size of the
+		 * decompressed kernel at the end of the compressed data
+		 * in little-endian form.
+		 */
+		ldrb	r9, [r10, #0]
+		ldrb	lr, [r10, #1]
+		orr	r9, r9, lr, lsl #8
+		ldrb	lr, [r10, #2]
+		ldrb	r10, [r10, #3]
+		orr	r9, r9, lr, lsl #16
+		orr	r9, r9, r10, lsl #24
+		
 #ifndef CONFIG_ZBOOT_ROM
 		/* malloc space is above the relocated stack (64k max) */
 		add	sp, sp, r0
@@ -335,7 +349,7 @@ LC0:		.word	LC0			@ r1
 		.word	_end			@ r3
 		.word	_start			@ r5
 		.word	_edata			@ r6
-		.word	_image_size		@ r9
+		.word	input_data_end - 4	@ r9 (inflated size location)
 		.word	_got_start		@ r11
 		.word	_got_end		@ ip
 		.word	user_stack_end		@ sp
-- 
1.7.4

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[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 <nicolas.pitre@linaro.org>
---

BTW, I've yet to understand why, but this patch makes CONFIG_KERNEL_LZMA
work for me.
 
 arch/arm/boot/compressed/misc.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
index 2df3826..a565853 100644
--- a/arch/arm/boot/compressed/misc.c
+++ b/arch/arm/boot/compressed/misc.c
@@ -26,8 +26,6 @@ unsigned int __machine_arch_type;
 #include <linux/linkage.h>
 #include <asm/string.h>
 
-#include <asm/unaligned.h>
-
 
 static void putstr(const char *ptr);
 extern void error(char *x);
@@ -139,13 +137,12 @@ void *memcpy(void *__dest, __const void *__src, size_t __n)
 }
 
 /*
- * gzip delarations
+ * gzip declarations
  */
 extern char input_data[];
 extern char input_data_end[];
 
 unsigned char *output_data;
-unsigned long output_ptr;
 
 unsigned long free_mem_ptr;
 unsigned long free_mem_end_ptr;
@@ -173,13 +170,11 @@ asmlinkage void __div0(void)
 extern void do_decompress(u8 *input, int len, u8 *output, void (*error)(char *x));
 
 
-unsigned long
+void
 decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p,
 		unsigned long free_mem_ptr_end_p,
 		int arch_id)
 {
-	unsigned char *tmp;
-
 	output_data		= (unsigned char *)output_start;
 	free_mem_ptr		= free_mem_ptr_p;
 	free_mem_end_ptr	= free_mem_ptr_end_p;
@@ -187,12 +182,8 @@ decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p,
 
 	arch_decomp_setup();
 
-	tmp = (unsigned char *) (((unsigned long)input_data_end) - 4);
-	output_ptr = get_unaligned_le32(tmp);
-
 	putstr("Uncompressing Linux...");
 	do_decompress(input_data, input_data_end - input_data,
 			output_data, error);
 	putstr(" done, booting the kernel.\n");
-	return output_ptr;
 }
-- 
1.7.4

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[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 <nicolas.pitre@linaro.org>
---

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

diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
index a565853..0125dae 100644
--- a/arch/arm/boot/compressed/misc.c
+++ b/arch/arm/boot/compressed/misc.c
@@ -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
+ * 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
 
 #ifdef CONFIG_DEBUG_ICEDCC
 
-- 
1.7.4

[PATCH 0/3] fix some issues with the kernel decompressor

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110419 21:12]:
> 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?

Tony

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -307,6 +307,7 @@ dtb_check_done:
 		/* Round up to next 256-byte boundary. */
 		add	r10, r10, #256
 		bic	r10, r10, #255
+		orr	r10, r10, #0x00100000
 
 		sub	r9, r6, r5		@ size to copy
 		add	r9, r9, #31		@ rounded up to a multiple

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Uwe Kleine-König]

On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> 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 <nicolas.pitre@linaro.org>
> ---
> 
> 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(-)
> 
> diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> index a565853..0125dae 100644
> --- a/arch/arm/boot/compressed/misc.c
> +++ b/arch/arm/boot/compressed/misc.c
> @@ -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
> + * 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Shawn Guo]

On Wed, Apr 20, 2011 at 10:42:34AM +0200, Uwe Kleine-K?nig wrote:
[...]
> > +/*
> > + * 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
> > + * 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.
> 
Also, the static attribute of functions gets lost, though it may not
matter.

-- 
Regards,
Shawn

[PATCH 0/3] fix some issues with the kernel decompressor

[Shawn Guo]

On Wed, Apr 20, 2011 at 12:15:01AM -0400, Nicolas Pitre wrote:
> 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
> 
Tested-by: Shawn Guo <shawn.guo@linaro.org>

On mx51 babbage, tested both LZMA decompressor and DTB append patch.

-- 
Regards,
Shawn

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

> On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > 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 <nicolas.pitre@linaro.org>
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > index a565853..0125dae 100644
> > --- a/arch/arm/boot/compressed/misc.c
> > +++ b/arch/arm/boot/compressed/misc.c
> > @@ -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
> > + * 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.

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


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Uwe Kleine-König]

Hello,

On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:
> > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > +#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.
> 
> 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:

> Hello,
> 
> On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> > On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:
> > > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > > +#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.
> > 
> > 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, and as a side effect the optimization that 
can be performed on that access.  Here the goal is to prevent gcc from 
applying such optimizations.


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Shawn Guo]

On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:
> 
> > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > 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 <nicolas.pitre@linaro.org>
> > > ---
> > > 
> > > 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(-)
> > > 
> > > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > > index a565853..0125dae 100644
> > > --- a/arch/arm/boot/compressed/misc.c
> > > +++ b/arch/arm/boot/compressed/misc.c
> > > @@ -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
> > > + * 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.
> 
> 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?

-- 
Regards,
Shawn

[PATCH 0/3] fix some issues with the kernel decompressor

[Nicolas Pitre]

On Wed, 20 Apr 2011, Tony Lindgren wrote:

> * Nicolas Pitre <nicolas.pitre@linaro.org> [110419 21:12]:
> > 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.


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 20 Apr 2011, Shawn Guo wrote:

> On Wed, Apr 20, 2011 at 08:28:20AM -0400, Nicolas Pitre wrote:
> > On Wed, 20 Apr 2011, Uwe Kleine-K?nig wrote:
> > 
> > > On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > > > 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 <nicolas.pitre@linaro.org>
> > > > ---
> > > > 
> > > > 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(-)
> > > > 
> > > > diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
> > > > index a565853..0125dae 100644
> > > > --- a/arch/arm/boot/compressed/misc.c
> > > > +++ b/arch/arm/boot/compressed/misc.c
> > > > @@ -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
> > > > + * 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.
> > 
> > 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?

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.


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 20 Apr 2011, Nicolas Pitre wrote:

> On Wed, 20 Apr 2011, Shawn Guo wrote:
> 
> > 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.

Sorry, more clarification is needed here.

The patch changing all occurrences of "static" into nothing would break 
code using static variables within functions.  I just hope that no one 
is using local static variables in their uncompress.h file.

And local static variables are always using GOTOFF relocations which is 
what we want to avoid in the first place.

The conclusion is that no one should be using local static variables in 
code linked by the kernel decompressor, unless those variables are also 
marked const.


Nicolas

[PATCH 0/3] fix some issues with the kernel decompressor

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 05:57]:
> On Wed, 20 Apr 2011, Tony Lindgren wrote:
> 
> > * Nicolas Pitre <nicolas.pitre@linaro.org> [110419 21:12]:
> > > 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.

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

Tony

[PATCH 0/3] fix some issues with the kernel decompressor

[Nicolas Pitre]

On Wed, 20 Apr 2011, Tony Lindgren wrote:

> * Nicolas Pitre <nicolas.pitre@linaro.org> [110420 05:57]:
> > On Wed, 20 Apr 2011, Tony Lindgren wrote:
> > 
> > > * Nicolas Pitre <nicolas.pitre@linaro.org> [110419 21:12]:
> > > > 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.
> 
> 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?


Nicolas

[PATCH 0/3] fix some issues with the kernel decompressor

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 10:16]:
> On Wed, 20 Apr 2011, Tony Lindgren wrote:
> 
> > * Nicolas Pitre <nicolas.pitre@linaro.org> [110420 05:57]:
> > > On Wed, 20 Apr 2011, Tony Lindgren wrote:
> > > 
> > > > * Nicolas Pitre <nicolas.pitre@linaro.org> [110419 21:12]:
> > > > > 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.
> > 
> > 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?

This is omap2plus_defconfig with DEBUG_LL and EARLY_PRINTK added on
v2.6.39-rc4 with no extra patches.

This same kernel binary boots everything except n900. It also boots
n8x0 just fine, and that too is using nolo bootloader with zImage.

Out of your three patches only the last one seems to make any difference
where it now prints an error about DT data not found if the DT append
patches are applied.

Tony

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[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 <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -282,6 +282,7 @@ dtb_check_done:
 
 /*
  * Check to see if we will overwrite ourselves.
+ *   r1  = corrupted, temporary uncompressed kernel end 
  *   r4  = final kernel address
  *   r5  = start of this image
  *   r9  = size of decompressed image
@@ -292,15 +293,24 @@ dtb_check_done:
  */
 		cmp	r4, r10
 		bhs	wont_overwrite
-		add	r10, r4, r9
-		cmp	r10, r5
+		add	r1, r4, r9
+		cmp	r1, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the compressed image end is past the uncompressed
+		 * kernel end. In that case, relocate ourselves to the end
+		 * of the compressed image instead of the uncompressed kernel
+		 * end to avoid overwriting ourselves.
+		 */
+		cmp	r10, r1
+		movls	r10, r1
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of this image if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[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 <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -282,6 +282,7 @@ dtb_check_done:
 
 /*
  * Check to see if we will overwrite ourselves.
+ *   r1  = corrupted, temporary uncompressed kernel end 
  *   r4  = final kernel address
  *   r5  = start of this image
  *   r9  = size of decompressed image
@@ -292,15 +293,24 @@ dtb_check_done:
  */
 		cmp	r4, r10
 		bhs	wont_overwrite
-		add	r10, r4, r9
-		cmp	r10, r5
+		add	r1, r4, r9
+		cmp	r1, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the compressed image end is past the uncompressed
+		 * kernel end. In that case, relocate ourselves to the end
+		 * of the compressed image instead of the uncompressed kernel
+		 * end to avoid overwriting ourselves.
+		 */
+		cmp	r10, r1
+		movls	r10, r1
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of this image if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH 1/3] ARM: zImage: no need to get the decompressed size from the filesystem

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 07:12]:
> 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 <nicolas.pitre@linaro.org>

Works fine on the boards I'm using:

Tested-by: Tony Lindgren <tony@atomide.com> 

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 07:12]:
> 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 <nicolas.pitre@linaro.org>

Works for me:

Tested-by: Tony Lindgren <tony@atomide.com>

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

Maybe the compressed image size changes slightly so the compressed
image end no longer goes past the uncompressed kernel end and relocate
works properly? Maybe see if the patch I posted solves your LZMA
issue even without this patch?

Tony

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110420 07:12]:
> 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 <nicolas.pitre@linaro.org>
> ---
> 
> This should fix remaining issues some people have with the DT append patch.

This fixes the issue where the beginning of the DT data got overwritten
by the uncompressed kernel:

Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Tony Lindgren wrote:

> 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 <tony@atomide.com>

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


Nicolas

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Tony Lindgren wrote:

> 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 <tony@atomide.com>

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


Nicolas

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Nicolas Pitre wrote:

> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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.


Nicolas

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Nicolas Pitre wrote:

> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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.


Nicolas

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Nicolas Pitre wrote:

> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > 
> > > 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 <tony@atomide.com>
> > 
> > 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.

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

----- >8

From: Nicolas Pitre <nicolas.pitre@linaro.org>

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 <nicolas.pitre@linaro.org>

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 58ac434..79b5c62 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -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)/
diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
index 5309909..ea80abe 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.in
+++ b/arch/arm/boot/compressed/vmlinux.lds.in
@@ -54,6 +54,7 @@ SECTIONS
   .bss			: { *(.bss) }
   _end = .;
 
+  . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
 
   .stab 0		: { *(.stab) }

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Nicolas Pitre wrote:

> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > 
> > > 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 <tony@atomide.com>
> > 
> > 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.

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

----- >8

From: Nicolas Pitre <nicolas.pitre@linaro.org>

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 <nicolas.pitre@linaro.org>

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 58ac434..79b5c62 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -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)/
diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
index 5309909..ea80abe 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.in
+++ b/arch/arm/boot/compressed/vmlinux.lds.in
@@ -54,6 +54,7 @@ SECTIONS
   .bss			: { *(.bss) }
   _end = .;
 
+  . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
 
   .stab 0		: { *(.stab) }

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Shawn Guo]

On Thu, Apr 21, 2011 at 11:23:22PM -0400, Nicolas Pitre wrote:
> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> > 
> > > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > > 
> > > > 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 <tony@atomide.com>
> > > 
> > > 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.
> 
> I found the bugger.  The problem was a bad stack alignment.
> 
> ----- >8
> 
> From: Nicolas Pitre <nicolas.pitre@linaro.org>
> 
> 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 <nicolas.pitre@linaro.org>
> 
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index 58ac434..79b5c62 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -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)/
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
> index 5309909..ea80abe 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.in
> +++ b/arch/arm/boot/compressed/vmlinux.lds.in
> @@ -54,6 +54,7 @@ SECTIONS
>    .bss			: { *(.bss) }
>    _end = .;
>  
> +  . = ALIGN(8);		/* the stack must be 64-bit aligned */
>    .stack		: { *(.stack) }
>  
>    .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

-- 
Regards,
Shawn

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Shawn Guo]

On Thu, Apr 21, 2011 at 11:23:22PM -0400, Nicolas Pitre wrote:
> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> > 
> > > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > > 
> > > > 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 <tony@atomide.com>
> > > 
> > > 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.
> 
> I found the bugger.  The problem was a bad stack alignment.
> 
> ----- >8
> 
> From: Nicolas Pitre <nicolas.pitre@linaro.org>
> 
> 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 <nicolas.pitre@linaro.org>
> 
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index 58ac434..79b5c62 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -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)/
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
> index 5309909..ea80abe 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.in
> +++ b/arch/arm/boot/compressed/vmlinux.lds.in
> @@ -54,6 +54,7 @@ SECTIONS
>    .bss			: { *(.bss) }
>    _end = .;
>  
> +  . = ALIGN(8);		/* the stack must be 64-bit aligned */
>    .stack		: { *(.stack) }
>  
>    .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

-- 
Regards,
Shawn

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Shawn Guo]

On Fri, Apr 22, 2011 at 01:19:14PM +0800, Shawn Guo wrote:
> > ----- >8
> > 
> > From: Nicolas Pitre <nicolas.pitre@linaro.org>
> > 
> > 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 <nicolas.pitre@linaro.org>
> > 
> > diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> > index 58ac434..79b5c62 100644
> > --- a/arch/arm/boot/compressed/Makefile
> > +++ b/arch/arm/boot/compressed/Makefile
> > @@ -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)/
> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
> > index 5309909..ea80abe 100644
> > --- a/arch/arm/boot/compressed/vmlinux.lds.in
> > +++ b/arch/arm/boot/compressed/vmlinux.lds.in
> > @@ -54,6 +54,7 @@ SECTIONS
> >    .bss			: { *(.bss) }
> >    _end = .;
> >  
> > +  . = ALIGN(8);		/* the stack must be 64-bit aligned */
> >    .stack		: { *(.stack) }
> >  
> >    .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
> 
On mx51 babbage,

Tested-by: Shawn Guo <shawn.guo@linaro.org>

which is only a regression test.

-- 
Regards,
Shawn

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Shawn Guo]

On Fri, Apr 22, 2011 at 01:19:14PM +0800, Shawn Guo wrote:
> > ----- >8
> > 
> > From: Nicolas Pitre <nicolas.pitre@linaro.org>
> > 
> > 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 <nicolas.pitre@linaro.org>
> > 
> > diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> > index 58ac434..79b5c62 100644
> > --- a/arch/arm/boot/compressed/Makefile
> > +++ b/arch/arm/boot/compressed/Makefile
> > @@ -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)/
> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.in b/arch/arm/boot/compressed/vmlinux.lds.in
> > index 5309909..ea80abe 100644
> > --- a/arch/arm/boot/compressed/vmlinux.lds.in
> > +++ b/arch/arm/boot/compressed/vmlinux.lds.in
> > @@ -54,6 +54,7 @@ SECTIONS
> >    .bss			: { *(.bss) }
> >    _end = .;
> >  
> > +  . = ALIGN(8);		/* the stack must be 64-bit aligned */
> >    .stack		: { *(.stack) }
> >  
> >    .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
> 
On mx51 babbage,

Tested-by: Shawn Guo <shawn.guo@linaro.org>

which is only a regression test.

-- 
Regards,
Shawn

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 16:18]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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...

Yeah that's what I'm wondering too.. This is probably not the
right fix.. I'm also wondering that it should be possible to
make uImage also not work by setting loadaddr just before the
uncompressed kernel end.

You would assume that only the running code would not survive
relocation if some of it gets overwritten. But that should be
only the beginning, no idea why the need to relocate all the
way after the whole image?

If stack was overlapping the zImage, I could see it corrupt
the zImage but there not much happening between relocating
and restarting of the bootloader.

Tony

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 16:18]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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...

Yeah that's what I'm wondering too.. This is probably not the
right fix.. I'm also wondering that it should be possible to
make uImage also not work by setting loadaddr just before the
uncompressed kernel end.

You would assume that only the running code would not survive
relocation if some of it gets overwritten. But that should be
only the beginning, no idea why the need to relocate all the
way after the whole image?

If stack was overlapping the zImage, I could see it corrupt
the zImage but there not much happening between relocating
and restarting of the bootloader.

Tony

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> > 
> > > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > > 
> > > > 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 <tony@atomide.com>
> > > 
> > > 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.

Yes so it seems, but it also seems that there is still something else wrong..

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

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

Regards,

Tony

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> 
> > On Thu, 21 Apr 2011, Nicolas Pitre wrote:
> > 
> > > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > > 
> > > > 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 <tony@atomide.com>
> > > 
> > > 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.

Yes so it seems, but it also seems that there is still something else wrong..

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

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

Regards,

Tony

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Tony Lindgren wrote:

> * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> > 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".

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.


Nicolas

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Thu, 21 Apr 2011, Tony Lindgren wrote:

> * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> > 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".

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.


Nicolas

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110422 17:08]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> > > 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".
> 
> Hmmm......
> 
> Is it possible you have bad RAM?  In compressed/head.S, locate this 
> code:

This is happening on all n900 boards AFAIK.
 
> #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.

OK thanks, I'll take a look. I guess it could also be a cache flush
issue or borderline memory timings set in the bootloader.

Regards,

Tony

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110422 17:08]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 20:20]:
> > > 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".
> 
> Hmmm......
> 
> Is it possible you have bad RAM?  In compressed/head.S, locate this 
> code:

This is happening on all n900 boards AFAIK.
 
> #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.

OK thanks, I'll take a look. I guess it could also be a cache flush
issue or borderline memory timings set in the bootloader.

Regards,

Tony

[PATCH] ARM: Fix bad SP address after relocating kernel

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

Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
the address of the relocated area that gets corrupted, and to
Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
related fixes.

Signed-off-by: Tony Lindgren <tony@atomide.com>

---

I think this is the right fix.. And we don't want to mess with the
ZBOOT_ROM sp address, right?

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -253,6 +253,15 @@ restart:	adr	r0, LC0
 		/* Preserve offset to relocated code. */
 		sub	r6, r9, r6
 
+#ifndef CONFIG_ZBOOT_ROM
+		/*
+		 * Fix sp to use the relocated address in case old sp is
+		 * within the relocated area. Otherwise cache_clean_flush
+		 * will trash some of the relocated area.
+		 */
+		add	sp, r6
+#endif
+
 		bl	cache_clean_flush
 
 		adr	r0, BSYM(restart)

[PATCH] ARM: Fix bad SP address after relocating kernel

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

Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
the address of the relocated area that gets corrupted, and to
Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
related fixes.

Signed-off-by: Tony Lindgren <tony@atomide.com>

---

I think this is the right fix.. And we don't want to mess with the
ZBOOT_ROM sp address, right?

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -253,6 +253,15 @@ restart:	adr	r0, LC0
 		/* Preserve offset to relocated code. */
 		sub	r6, r9, r6
 
+#ifndef CONFIG_ZBOOT_ROM
+		/*
+		 * Fix sp to use the relocated address in case old sp is
+		 * within the relocated area. Otherwise cache_clean_flush
+		 * will trash some of the relocated area.
+		 */
+		add	sp, r6
+#endif
+
 		bl	cache_clean_flush
 
 		adr	r0, BSYM(restart)

Re: [PATCH] ARM: Fix bad SP address after relocating kernel

[Nicolas Pitre]

On Tue, 26 Apr 2011, Tony Lindgren wrote:

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

> Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
> the address of the relocated area that gets corrupted, and to
> Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
> related fixes.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

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

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index adf583c..8e3c54b 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -838,9 +838,11 @@ __armv3_mmu_cache_off:
  * Clean and flush the cache to maintain consistency.
  *
  * On exit,
- *  r1, r2, r3, r9, r10, r11, r12 corrupted
+ *  r0, r1, r2, r3, r5, r9, r10, r11, r12, sp corrupted
  * This routine must preserve:
  *  r4, r6, r7, r8
+ *
+ * Yes, sp is destroyed by this call in the armv7 hierarchical case.
  */
 		.align	5
 cache_clean_flush:
@@ -888,7 +890,6 @@ __armv7_mmu_cache_flush:
 		b	iflush
 hierarchical:
 		mcr	p15, 0, r10, c7, c10, 5	@ DMB
-		stmfd	sp!, {r0-r7, r9-r11}
 		mrc	p15, 1, r0, c0, c0, 1	@ read clidr
 		ands	r3, r0, #0x7000000	@ extract loc from clidr
 		mov	r3, r3, lsr #23		@ left align loc bit field
@@ -905,31 +906,31 @@ loop1:
 		mrc	p15, 1, r1, c0, c0, 0	@ read the new csidr
 		and	r2, r1, #7		@ extract the length of the cache lines
 		add	r2, r2, #4		@ add 4 (line length offset)
-		ldr	r4, =0x3ff
-		ands	r4, r4, r1, lsr #3	@ find maximum number on the way size
-		clz	r5, r4			@ find bit position of way size increment
-		ldr	r7, =0x7fff
-		ands	r7, r7, r1, lsr #13	@ extract max number of the index size
+		ldr	r9, =0x3ff
+		ands	r9, r9, r1, lsr #3	@ find maximum number on the way size
+		clz	r5, r9			@ find bit position of way size increment
+		mov	sp, r9
+		ldr	r9, =0x7fff
+		ands	r1, r9, r1, lsr #13	@ extract max number of the index size
 loop2:
-		mov	r9, r4			@ create working copy of max way size
+		mov	r9, sp			@ create working copy of max way size
 loop3:
  ARM(		orr	r11, r10, r9, lsl r5	) @ factor way and cache number into r11
- ARM(		orr	r11, r11, r7, lsl r2	) @ factor index number into r11
- THUMB(		lsl	r6, r9, r5		)
- THUMB(		orr	r11, r10, r6		) @ factor way and cache number into r11
- THUMB(		lsl	r6, r7, r2		)
- THUMB(		orr	r11, r11, r6		) @ factor index number into r11
+ ARM(		orr	r11, r11, r1, lsl r2	) @ factor index number into r11
+ THUMB(		lsl	r12, r9, r5		)
+ THUMB(		orr	r11, r10, r12		) @ factor way and cache number into r11
+ THUMB(		lsl	r12, r1, r2		)
+ THUMB(		orr	r11, r11, r12		) @ factor index number into r11
 		mcr	p15, 0, r11, c7, c14, 2	@ clean & invalidate by set/way
 		subs	r9, r9, #1		@ decrement the way
 		bge	loop3
-		subs	r7, r7, #1		@ decrement the index
+		subs	r1, r1, #1		@ decrement the index
 		bge	loop2
 skip:
 		add	r10, r10, #2		@ increment cache number
 		cmp	r3, r10
 		bgt	loop1
 finished:
-		ldmfd	sp!, {r0-r7, r9-r11}
 		mov	r10, #0			@ swith back to cache level 0
 		mcr	p15, 2, r10, c0, c0, 0	@ select current cache level in cssr
 iflush:

[PATCH] ARM: Fix bad SP address after relocating kernel

[Nicolas Pitre]

On Tue, 26 Apr 2011, Tony Lindgren wrote:

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

> Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
> the address of the relocated area that gets corrupted, and to
> Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
> related fixes.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

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

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index adf583c..8e3c54b 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -838,9 +838,11 @@ __armv3_mmu_cache_off:
  * Clean and flush the cache to maintain consistency.
  *
  * On exit,
- *  r1, r2, r3, r9, r10, r11, r12 corrupted
+ *  r0, r1, r2, r3, r5, r9, r10, r11, r12, sp corrupted
  * This routine must preserve:
  *  r4, r6, r7, r8
+ *
+ * Yes, sp is destroyed by this call in the armv7 hierarchical case.
  */
 		.align	5
 cache_clean_flush:
@@ -888,7 +890,6 @@ __armv7_mmu_cache_flush:
 		b	iflush
 hierarchical:
 		mcr	p15, 0, r10, c7, c10, 5	@ DMB
-		stmfd	sp!, {r0-r7, r9-r11}
 		mrc	p15, 1, r0, c0, c0, 1	@ read clidr
 		ands	r3, r0, #0x7000000	@ extract loc from clidr
 		mov	r3, r3, lsr #23		@ left align loc bit field
@@ -905,31 +906,31 @@ loop1:
 		mrc	p15, 1, r1, c0, c0, 0	@ read the new csidr
 		and	r2, r1, #7		@ extract the length of the cache lines
 		add	r2, r2, #4		@ add 4 (line length offset)
-		ldr	r4, =0x3ff
-		ands	r4, r4, r1, lsr #3	@ find maximum number on the way size
-		clz	r5, r4			@ find bit position of way size increment
-		ldr	r7, =0x7fff
-		ands	r7, r7, r1, lsr #13	@ extract max number of the index size
+		ldr	r9, =0x3ff
+		ands	r9, r9, r1, lsr #3	@ find maximum number on the way size
+		clz	r5, r9			@ find bit position of way size increment
+		mov	sp, r9
+		ldr	r9, =0x7fff
+		ands	r1, r9, r1, lsr #13	@ extract max number of the index size
 loop2:
-		mov	r9, r4			@ create working copy of max way size
+		mov	r9, sp			@ create working copy of max way size
 loop3:
  ARM(		orr	r11, r10, r9, lsl r5	) @ factor way and cache number into r11
- ARM(		orr	r11, r11, r7, lsl r2	) @ factor index number into r11
- THUMB(		lsl	r6, r9, r5		)
- THUMB(		orr	r11, r10, r6		) @ factor way and cache number into r11
- THUMB(		lsl	r6, r7, r2		)
- THUMB(		orr	r11, r11, r6		) @ factor index number into r11
+ ARM(		orr	r11, r11, r1, lsl r2	) @ factor index number into r11
+ THUMB(		lsl	r12, r9, r5		)
+ THUMB(		orr	r11, r10, r12		) @ factor way and cache number into r11
+ THUMB(		lsl	r12, r1, r2		)
+ THUMB(		orr	r11, r11, r12		) @ factor index number into r11
 		mcr	p15, 0, r11, c7, c14, 2	@ clean & invalidate by set/way
 		subs	r9, r9, #1		@ decrement the way
 		bge	loop3
-		subs	r7, r7, #1		@ decrement the index
+		subs	r1, r1, #1		@ decrement the index
 		bge	loop2
 skip:
 		add	r10, r10, #2		@ increment cache number
 		cmp	r3, r10
 		bgt	loop1
 finished:
-		ldmfd	sp!, {r0-r7, r9-r11}
 		mov	r10, #0			@ swith back to cache level 0
 		mcr	p15, 2, r10, c0, c0, 0	@ select current cache level in cssr
 iflush:

Re: [PATCH] ARM: Fix bad SP address after relocating kernel

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110426 14:28]:
> On Tue, 26 Apr 2011, Tony Lindgren wrote:
> 
> > 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.
> 
> > Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
> > the address of the relocated area that gets corrupted, and to
> > Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
> > related fixes.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> I think there could be a better fix yet.  Could you test this patch:

Cool that works too and avoids using the stack, so that's a better fix:

Tested-by: Tony Lindgren <tony@atomide.com>

[PATCH] ARM: Fix bad SP address after relocating kernel

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110426 14:28]:
> On Tue, 26 Apr 2011, Tony Lindgren wrote:
> 
> > 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.
> 
> > Thanks to Aaro Koskinen <aaro.koskinen@nokia.com> for debugging
> > the address of the relocated area that gets corrupted, and to
> > Nicolas Pitre <nicolas.pitre@linaro.org> for the other uncompress
> > related fixes.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> I think there could be a better fix yet.  Could you test this patch:

Cool that works too and avoids using the stack, so that's a better fix:

Tested-by: Tony Lindgren <tony@atomide.com>

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Russell King - ARM Linux]

On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote:
> 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 <nicolas.pitre@linaro.org>
> ---
> 
> 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?

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Russell King - ARM Linux]

On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> 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.

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Russell King - ARM Linux]

On Wed, Apr 20, 2011 at 08:47:17AM -0400, Nicolas Pitre wrote:
> 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.

Moreover, if we have:

int foo(void)
{
	static int table[] = { ... };
}

then with static defined away, this turns into a runtime memcpy from
read-only data to read-write data each time the function is called.

Defining away static causes this kind of undesirable (and often hidden)
effect - and in these cases it most certainly is not just about variable
visibility.

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 06:18]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -286,6 +286,7 @@ dtb_check_done:
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, corrupted if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +298,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r5		@ use GOT end for upper limit
+		cmp	r10, r12		@ relocating less than GOT end?
+		mov	r10, r12		@ if so, relocate past GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110421 06:18]:
> On Thu, 21 Apr 2011, Tony Lindgren wrote:
> 
> > 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 <tony@atomide.com>
> 
> 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.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -286,6 +286,7 @@ dtb_check_done:
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, corrupted if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +298,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r5		@ use GOT end for upper limit
+		cmp	r10, r12		@ relocating less than GOT end?
+		mov	r10, r12		@ if so, relocate past GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [110427 05:44]:
> * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 06:18]:
> > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > 
> > > 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 <tony@atomide.com>
> > 
> > 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.

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

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -286,6 +286,7 @@ dtb_check_done:
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, corrupted if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +298,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r5		@ use GOT end for upper limit
+		cmp	r10, r12		@ relocating less than GOT end?
+		movle	r10, r12		@ if so, relocate past GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [110427 05:44]:
> * Nicolas Pitre <nicolas.pitre@linaro.org> [110421 06:18]:
> > On Thu, 21 Apr 2011, Tony Lindgren wrote:
> > 
> > > 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 <tony@atomide.com>
> > 
> > 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.

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

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -286,6 +286,7 @@ dtb_check_done:
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, corrupted if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +298,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r5		@ use GOT end for upper limit
+		cmp	r10, r12		@ relocating less than GOT end?
+		movle	r10, r12		@ if so, relocate past GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Nicolas Pitre]

On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:

> On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote:
> > 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 <nicolas.pitre@linaro.org>
> > ---
> > 
> > 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?

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.


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:

> On Wed, Apr 20, 2011 at 12:15:04AM -0400, Nicolas Pitre wrote:
> > 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.

Yes, but in such case those variables would be allocated to the .data 
section, and we're already discarding .data up front to prevent its 
usage.  So there might not be static variables within functions used 
now, unless they're also const.

However I've discovered that static variables within functions with an 
initial value of zero are instead allocated in the .bss section, and 
always using a GOTOFF relocation.  So those can't be used either and 
right now there is nothing preventing them.

Anyway I've discarded this patch as it also makes problems with 
architecture code including standard kernel header files containing 
static inline 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.

Exact.  And I now have a patch failing the build if the wrong usage is 
made ending up in GOTOFF relocs that we can't support which is even 
better than manual auditing.


Nicolas

[PATCH 3/3] ARM: zImage: fix issues with missing GOT entries for some global variables

[Nicolas Pitre]

On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:

> On Wed, Apr 20, 2011 at 08:47:17AM -0400, Nicolas Pitre wrote:
> > 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++;
> }

I was talking about global variables.  Variables within a function are 
always visible to the local scope, static or not.

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

Exact.  And I've dropped that patch.  That doesn't mean that static 
variables within functions are always OK in the context of zImage 
though, as they are always turned into GOTOFF relocs.


Nicolas

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Wed, 27 Apr 2011, Tony Lindgren wrote:

> * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > 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.

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:

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 8dab5e3..71fc1d9 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -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.
  */
-		/* Round up to next 256-byte boundary. */
-		add	r10, r10, #256
+		/*
+		 * Round to a 256-byte boundary on the next page. This
+		 * avoids overwriting ourself if the offset is small.
+		 */
+		add	r10, r10, #4096
 		bic	r10, r10, #255
 
 		sub	r9, r6, r5		@ size to copy

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Nicolas Pitre]

On Wed, 27 Apr 2011, Tony Lindgren wrote:

> * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > 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.

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:

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 8dab5e3..71fc1d9 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -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.
  */
-		/* Round up to next 256-byte boundary. */
-		add	r10, r10, #256
+		/*
+		 * Round to a 256-byte boundary on the next page. This
+		 * avoids overwriting ourself if the offset is small.
+		 */
+		add	r10, r10, #4096
 		bic	r10, r10, #255
 
 		sub	r9, r6, r5		@ size to copy

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110428 01:12]:
> On Wed, 27 Apr 2011, Tony Lindgren wrote:
> 
> > * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > > 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.
> 
> 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.
 
> And this could simply be fixed with a big enough constant like this:
> 
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 8dab5e3..71fc1d9 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -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.
>   */
> -		/* Round up to next 256-byte boundary. */
> -		add	r10, r10, #256
> +		/*
> +		 * Round to a 256-byte boundary on the next page. This
> +		 * avoids overwriting ourself if the offset is small.
> +		 */
> +		add	r10, r10, #4096
>  		bic	r10, r10, #255
>  
>  		sub	r9, r6, r5		@ size to copy

Yeah that's what I had originally, but then we'll be potentially
hitting the same bug again once more cache flushing code etc gets
added.

Regards,

Tony

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110428 01:12]:
> On Wed, 27 Apr 2011, Tony Lindgren wrote:
> 
> > * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > > 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.
> 
> 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.
 
> And this could simply be fixed with a big enough constant like this:
> 
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 8dab5e3..71fc1d9 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -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.
>   */
> -		/* Round up to next 256-byte boundary. */
> -		add	r10, r10, #256
> +		/*
> +		 * Round to a 256-byte boundary on the next page. This
> +		 * avoids overwriting ourself if the offset is small.
> +		 */
> +		add	r10, r10, #4096
>  		bic	r10, r10, #255
>  
>  		sub	r9, r6, r5		@ size to copy

Yeah that's what I had originally, but then we'll be potentially
hitting the same bug again once more cache flushing code etc gets
added.

Regards,

Tony

Re: [PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [110427 23:35]:
> * Nicolas Pitre <nicolas.pitre@linaro.org> [110428 01:12]:
> > On Wed, 27 Apr 2011, Tony Lindgren wrote:
> > 
> > > * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > > > 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.
> > 
> > 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's this one with r12 calculation fixed using r0 delta. Also updated
it to use movlt instead of movle as that should be sufficient.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -282,10 +282,12 @@ dtb_check_done:
 
 /*
  * Check to see if we will overwrite ourselves.
+ *   r0  = delta
  *   r4  = final kernel address
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, fixed up with delta in r0 if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +299,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r0		@ fixup GOT end with delta
+		cmp	r10, r12		@ relocating less than GOT end?
+		movlt	r10, r12		@ if so, relocate to GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH] ARM: Fix relocation if image end past uncompressed kernel end

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [110427 23:35]:
> * Nicolas Pitre <nicolas.pitre@linaro.org> [110428 01:12]:
> > On Wed, 27 Apr 2011, Tony Lindgren wrote:
> > 
> > > * Tony Lindgren <tony@atomide.com> [110427 05:44]:
> > > > 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.
> > 
> > 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's this one with r12 calculation fixed using r0 delta. Also updated
it to use movlt instead of movle as that should be sufficient.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Wed, 27 Apr 2011 02:06:13 -0700
Subject: [PATCH] ARM: Fix relocation to move past the running code

Otherwise we end up overwriting ourselves partially when relocating
less than size of the running code in head.S.

Without this patch, a system will not boot if the compressed image
load address is slightly less than where the compressed image gets
relocated.

For example, using mkimage to set the load address to something like
zreladdr + uncompressed image size - 0x100 will make the system hang
without this patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -282,10 +282,12 @@ dtb_check_done:
 
 /*
  * Check to see if we will overwrite ourselves.
+ *   r0  = delta
  *   r4  = final kernel address
  *   r5  = start of this image
  *   r9  = size of decompressed image
  *   r10 = end of this image, including  bss/stack/malloc space if non XIP
+ *   r12 = GOT end, fixed up with delta in r0 if relocating
  * We basically want:
  *   r4 - 16k page directory >= r10 -> OK
  *   r4 + image length <= r5 -> OK
@@ -297,11 +299,20 @@ dtb_check_done:
 		cmp	r10, r5
 		bls	wont_overwrite
 
+		/*
+		 * Check if the relocate address overlaps the running code in
+		 * head.S. In that case we need to relocate past the code
+		 * to avoid overwriting some of the running code.
+		 */
+		add	r12, r12, r0		@ fixup GOT end with delta
+		cmp	r10, r12		@ relocating less than GOT end?
+		movlt	r10, r12		@ if so, relocate to GOT end
+
 /*
  * Relocate ourselves past the end of the decompressed kernel.
  *   r5  = start of this image
  *   r6  = _edata
- *   r10 = end of the decompressed kernel
+ *   r10 = end of the decompressed kernel or end of GOT end if larger
  * Because we always copy ahead, we need to do it from the end and go
  * backward in case the source and destination overlap.
  */

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Russell King - ARM Linux]

On Wed, Apr 27, 2011 at 10:48:07AM -0400, Nicolas Pitre wrote:
> On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:
> 
> > On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote:
> > > 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 <nicolas.pitre@linaro.org>
> > > ---
> > > 
> > > 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?
> 
> 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.

Please first resend the entire set of patches for review - I've no idea
what the set of patches contain having read through most of the emails
on the decompressor which were sent over Easter.

I really don't like the idea of getting rid of the 'sp' for the cache
flush and stamping over a load of registers instead.  I think that's
storing up problems for the future.

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110427 07:44]:
> On Wed, 27 Apr 2011, Russell King - ARM Linux wrote:
> 
> > On Wed, Apr 20, 2011 at 12:15:03AM -0400, Nicolas Pitre wrote:
> > > 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 <nicolas.pitre@linaro.org>
> > > ---
> > > 
> > > 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?
> 
> 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.

Regards,

Tony

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Nicolas Pitre]

On Thu, 28 Apr 2011, Tony Lindgren wrote:

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

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

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

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


Nicolas

[PATCH 2/3] ARM: compressed/misc.c: simplify decompress_kernel()

[Tony Lindgren]

* Nicolas Pitre <nicolas.pitre@linaro.org> [110428 11:17]:
> On Thu, 28 Apr 2011, Tony Lindgren wrote:
> 
> > 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.

Right, not a regression, but a nasty bug.
 
> > 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).

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

Cool saw that, yeah that's a nice way of fixing it.
 
> > 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.

Great & thanks for tracking down these issues.

Tony