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

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

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

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

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

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

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

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

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

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

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>

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

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

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

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

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

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