diff mbox

ARM: Fix relocation if image end past uncompressed kernel end

Message ID alpine.LFD.2.00.1104212233170.24613@xanadu.home (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolas Pitre April 22, 2011, 3:23 a.m. UTC 
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>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Shawn Guo April 22, 2011, 5:19 a.m. UTC | #1 
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
Shawn Guo April 22, 2011, 5:36 a.m. UTC | #2 
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.
Tony Lindgren April 22, 2011, 6:28 a.m. UTC | #3 
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Pitre April 22, 2011, 2:12 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 26, 2011, 8:57 a.m. UTC | #5 
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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