[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

Using u-boot-2017.05 on i.MX6UL we ran into following problem:
Initially U-Boot could be started normally.
If we added one random command in configuration, the newly generated
image hung at startup (last output was DRAM:  256 MiB).

We tracked this down to a data abort within relocation (relocated_code).

relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
iteration until the source pointer is equal to __image_copy_end.
In a good case __image_copy_end was aligned to 8 bytes, so the loop
stopped as suggested, but in an errornous case __image_copy_end was
not aligned to 8 bytes, so the loop ran out of bounds and caused a
data abort exception.

This patches solves the issue by aligning __image_copy_end to 8 byte
using the linker script related to arm.

I don't know if it's the correct way to solve this, so some review would
be very appreciated.
---
 arch/arm/cpu/u-boot.lds | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 37d4c60..70bee1a 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -165,7 +165,7 @@ SECTIONS
 		*(.__efi_runtime_rel_stop)
 	}
 
-	. = ALIGN(4);
+	. = ALIGN(8);
 
 	.image_copy_end :
 	{
-- 
2.1.4

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Alexander Graf]

On 10.05.17 15:41, Manfred Schlaegl wrote:
> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> Initially U-Boot could be started normally.
> If we added one random command in configuration, the newly generated
> image hung at startup (last output was DRAM:  256 MiB).
>
> We tracked this down to a data abort within relocation (relocated_code).
>
> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
> iteration until the source pointer is equal to __image_copy_end.
> In a good case __image_copy_end was aligned to 8 bytes, so the loop
> stopped as suggested, but in an errornous case __image_copy_end was
> not aligned to 8 bytes, so the loop ran out of bounds and caused a
> data abort exception.
>
> This patches solves the issue by aligning __image_copy_end to 8 byte
> using the linker script related to arm.
>
> I don't know if it's the correct way to solve this, so some review would
> be very appreciated.

I think it makes sense, but needs a comment next to the ALIGN() command. 
Also please make sure you update all the other lds files as well, so 
that people don't run into it by accident.


Alex

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Lokesh Vutla]

On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> Initially U-Boot could be started normally.
> If we added one random command in configuration, the newly generated
> image hung at startup (last output was DRAM:  256 MiB).
> 
> We tracked this down to a data abort within relocation (relocated_code).
> 
> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
> iteration until the source pointer is equal to __image_copy_end.
> In a good case __image_copy_end was aligned to 8 bytes, so the loop
> stopped as suggested, but in an errornous case __image_copy_end was
> not aligned to 8 bytes, so the loop ran out of bounds and caused a
> data abort exception.

Well I agree with this patch but I have a small query. Looking at the
relocation code:

copy_loop:
	ldmia	r1!, {r10-r11}		/* copy from source address [r1]    */
	stmia	r0!, {r10-r11}		/* copy to   target address [r0]    */
	cmp  	r1, r2			/* until source end address [r2]    */
	blo		copy_loop

IIUC, this loops exits as soon as r1 >= r2

In your case
r1 is __image_copy_start
r0 is relocation address
r2 is __image_copy_end (which is 4 byte aligned)

In the corner case you mentioned there will be extra memcpy of 4 bytes
from address in r1 to address in r0. I assume you are running from DDR
and relocation offset is calculated such that (aligned to previous 4K
page) it should be able to accommodate extra 4 bytes(I assume). I am
wondering why should this give a data abort.

Thanks and regards,
Lokesh



> 
> This patches solves the issue by aligning __image_copy_end to 8 byte
> using the linker script related to arm.
> 
> I don't know if it's the correct way to solve this, so some review would
> be very appreciated.
> ---
>  arch/arm/cpu/u-boot.lds | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 37d4c60..70bee1a 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -165,7 +165,7 @@ SECTIONS
>  		*(.__efi_runtime_rel_stop)
>  	}
>  
> -	. = ALIGN(4);
> +	. = ALIGN(8);
>  
>  	.image_copy_end :
>  	{
> 

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Jagan Teki]

On Wed, May 10, 2017 at 7:11 PM, Manfred Schlaegl
<manfred.schlaegl@ginzinger.com> wrote:
> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> Initially U-Boot could be started normally.
> If we added one random command in configuration, the newly generated
> image hung at startup (last output was DRAM:  256 MiB).

I've observed the similar issue where the startup hang before
relocating [1], couldn't get any abort or panic message on the
console. Do you have any suggestion on 'how to debug' I've tried gdb
but couldn't succeed.

[1] https://patchwork.ozlabs.org/patch/760018/

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-05-11 08:53, Lokesh Vutla wrote:
> 
> 
> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>> Initially U-Boot could be started normally.
>> If we added one random command in configuration, the newly generated
>> image hung at startup (last output was DRAM:  256 MiB).
>>
>> We tracked this down to a data abort within relocation (relocated_code).
>>
>> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
>> iteration until the source pointer is equal to __image_copy_end.
>> In a good case __image_copy_end was aligned to 8 bytes, so the loop
>> stopped as suggested, but in an errornous case __image_copy_end was
>> not aligned to 8 bytes, so the loop ran out of bounds and caused a
>> data abort exception.
> 
> Well I agree with this patch but I have a small query. Looking at the
> relocation code:
> 
> copy_loop:
> 	ldmia	r1!, {r10-r11}		/* copy from source address [r1]    */
> 	stmia	r0!, {r10-r11}		/* copy to   target address [r0]    */
> 	cmp  	r1, r2			/* until source end address [r2]    */
> 	blo		copy_loop
> 
> IIUC, this loops exits as soon as r1 >= r2
> 
> In your case
> r1 is __image_copy_start
> r0 is relocation address
> r2 is __image_copy_end (which is 4 byte aligned)
> 
> In the corner case you mentioned there will be extra memcpy of 4 bytes
> from address in r1 to address in r0. I assume you are running from DDR
> and relocation offset is calculated such that (aligned to previous 4K
> page) it should be able to accommodate extra 4 bytes(I assume). I am
> wondering why should this give a data abort.
> 
> Thanks and regards,
> Lokesh
> 

Thanks a lot for your input!
The patch solved my problem randomly.

The loop terminates only one word beyond __image_copy_end.
This is not the problem in my case because both, the source and destination
pointers point to valid addresses in dram.
So the problem must be later in relocate_code.

I spent some time using a debugger and found that the data abort happens here

	/*
	 * fix .rel.dyn relocations
	 */
	ldr	r2, =__rel_dyn_start	/* r2 <- SRC &__rel_dyn_start */
	ldr	r3, =__rel_dyn_end	/* r3 <- SRC &__rel_dyn_end */
fixloop:
	ldmia	r2!, {r0-r1}		/* (r0,r1) <- (SRC location,fixup) */
	and	r1, r1, #0xff
	cmp	r1, #R_ARM_RELATIVE
	bne	fixnext

	/* relative fix: increase location by offset */
	add	r0, r0, r4
	ldr	r1, [r0]
	add	r1, r1, r4
	str	r1, [r0]
fixnext:
	cmp	r2, r3
	blo	fixloop


Also I found out, that it's not the alignment of image_copy_end, but of rel_dyn_start
(which was also implicitly changed by my patch).

In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
rel_dyn_start is 0x8785FC28
rel_dyn_end is 0x87857BD0
A dump of this memory area shows no abnormality

In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
rel_dyn_start is 0x8785FC24
rel_dyn_end is 0x87857BCC
So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
very interesting:

At offset 0x610 (relative to rel_dyn_start) we have following difference
-00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
+00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|

Here is a pseudo-trace of fixloop in bad case starting at offset 0x618 (relative to rel_dyn_start)

fixloop:				// R2 = 0x878581E4, R4 = 0x08786000
	ldmia	r2!, {r0-r1}		// R0 = 0x00000000, R1 = 0x00000017, R2 = 0x878581EC
	and	r1, r1, #0xff
	cmp	r1, #R_ARM_RELATIVE
	bne	fixnext			// R1 == 0x17 -> no branch
	
	/* relative fix: increase location by offset */
	add	r0, r0, r4		// R0 = 0x08786000
	ldr	r1, [r0]		// ABORT while accessing 0x08786000 -> not a valid address in dram


It seems, that the entry 00 00 00 00 17 00 00 00 @ offset 0x618 is some how incorrect, or at least
produces some incorrect behavior. There are no similar entries in the table.

I'm not really experienced with these tables.
Has anyone suggestions, starting points, ideas/hints which could help me to track this down?

Thanks a lot!

Best regards,
Manfred

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-05-11 14:12, Jagan Teki wrote:
> On Wed, May 10, 2017 at 7:11 PM, Manfred Schlaegl
> <manfred.schlaegl@ginzinger.com> wrote:
>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>> Initially U-Boot could be started normally.
>> If we added one random command in configuration, the newly generated
>> image hung at startup (last output was DRAM:  256 MiB).
> 
> I've observed the similar issue where the startup hang before
> relocating [1], couldn't get any abort or panic message on the
> console. Do you have any suggestion on 'how to debug' I've tried gdb
> but couldn't succeed.
> 
> [1] https://patchwork.ozlabs.org/patch/760018/
> 
> thanks!
> 

Hi!

I'm sorry for the late answer but, as you can see by my last message, I'd also had problems to identify my problem correctly...

Generally my experience from the last 10 years in embedded systems is, that with using u-boot and linux a physical
debugger (like BDI or even some FTDI things with OpenOCD) is very rarely needed.
BUT: finding problems at u-boot low-level is definitively a thing were such a debugger is necessary ;-).

I used a Segger JLink and gdb.
You can try to use http://www.openocd.de/

Hope it helped .. a bit.

Best regards,
Manfred

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Lokesh Vutla]

On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>
>>
>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>> Initially U-Boot could be started normally.
>>> If we added one random command in configuration, the newly generated
>>> image hung at startup (last output was DRAM:  256 MiB).
>>>
>>> We tracked this down to a data abort within relocation (relocated_code).
>>>
>>> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
>>> iteration until the source pointer is equal to __image_copy_end.
>>> In a good case __image_copy_end was aligned to 8 bytes, so the loop
>>> stopped as suggested, but in an errornous case __image_copy_end was
>>> not aligned to 8 bytes, so the loop ran out of bounds and caused a
>>> data abort exception.
>>
>> Well I agree with this patch but I have a small query. Looking at the
>> relocation code:
>>
>> copy_loop:
>> 	ldmia	r1!, {r10-r11}		/* copy from source address [r1]    */
>> 	stmia	r0!, {r10-r11}		/* copy to   target address [r0]    */
>> 	cmp  	r1, r2			/* until source end address [r2]    */
>> 	blo		copy_loop
>>
>> IIUC, this loops exits as soon as r1 >= r2
>>
>> In your case
>> r1 is __image_copy_start
>> r0 is relocation address
>> r2 is __image_copy_end (which is 4 byte aligned)
>>
>> In the corner case you mentioned there will be extra memcpy of 4 bytes
>> from address in r1 to address in r0. I assume you are running from DDR
>> and relocation offset is calculated such that (aligned to previous 4K
>> page) it should be able to accommodate extra 4 bytes(I assume). I am
>> wondering why should this give a data abort.
>>
>> Thanks and regards,
>> Lokesh
>>
> 
> Thanks a lot for your input!
> The patch solved my problem randomly.
> 
> The loop terminates only one word beyond __image_copy_end.
> This is not the problem in my case because both, the source and destination
> pointers point to valid addresses in dram.
> So the problem must be later in relocate_code.

That's right.

> 
> I spent some time using a debugger and found that the data abort happens here
> 
> 	/*
> 	 * fix .rel.dyn relocations
> 	 */
> 	ldr	r2, =__rel_dyn_start	/* r2 <- SRC &__rel_dyn_start */
> 	ldr	r3, =__rel_dyn_end	/* r3 <- SRC &__rel_dyn_end */
> fixloop:
> 	ldmia	r2!, {r0-r1}		/* (r0,r1) <- (SRC location,fixup) */
> 	and	r1, r1, #0xff
> 	cmp	r1, #R_ARM_RELATIVE
> 	bne	fixnext
> 
> 	/* relative fix: increase location by offset */
> 	add	r0, r0, r4
> 	ldr	r1, [r0]
> 	add	r1, r1, r4
> 	str	r1, [r0]
> fixnext:
> 	cmp	r2, r3
> 	blo	fixloop
> 
> 
> Also I found out, that it's not the alignment of image_copy_end, but of rel_dyn_start
> (which was also implicitly changed by my patch).
> 
> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
> rel_dyn_start is 0x8785FC28
> rel_dyn_end is 0x87857BD0
> A dump of this memory area shows no abnormality
> 
> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
> rel_dyn_start is 0x8785FC24
> rel_dyn_end is 0x87857BCC
> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
> very interesting:
> 
> At offset 0x610 (relative to rel_dyn_start) we have following difference
> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|

Looks like someone is corrupting the data(assuming). Is it all 0's just
at this location or continuously after this?

 If possible can you try the following experiments with the failing code:

- Dump rel_dyn_* data right after spl copies U-boot to ddr.
- Similarly dump rel_dyn_* data before calling relocate_code() and
compare both?

If the comparison fails, add a write breakpoint at the corrupted address
before starting u-boot and see when it hits.

BTW, where is your BSS located?

Thanks and regards,
Lokesh

> 
> Here is a pseudo-trace of fixloop in bad case starting at offset 0x618 (relative to rel_dyn_start)
> 
> fixloop:				// R2 = 0x878581E4, R4 = 0x08786000
> 	ldmia	r2!, {r0-r1}		// R0 = 0x00000000, R1 = 0x00000017, R2 = 0x878581EC
> 	and	r1, r1, #0xff
> 	cmp	r1, #R_ARM_RELATIVE
> 	bne	fixnext			// R1 == 0x17 -> no branch
> 	
> 	/* relative fix: increase location by offset */
> 	add	r0, r0, r4		// R0 = 0x08786000
> 	ldr	r1, [r0]		// ABORT while accessing 0x08786000 -> not a valid address in dram
> 
> 
> It seems, that the entry 00 00 00 00 17 00 00 00 @ offset 0x618 is some how incorrect, or at least
> produces some incorrect behavior. There are no similar entries in the table.
> 
> I'm not really experienced with these tables.
> Has anyone suggestions, starting points, ideas/hints which could help me to track this down?
> 
> Thanks a lot!
> 
> Best regards,
> Manfred
> 
> 
> 
> 
> 

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-05-17 06:13, Lokesh Vutla wrote:
> 
> 
> On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
>> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>>
>>>
>>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>>> Initially U-Boot could be started normally.
>>>> If we added one random command in configuration, the newly generated
>>>> image hung at startup (last output was DRAM:  256 MiB).
>>>>
>>>> We tracked this down to a data abort within relocation (relocated_code).
>>>>
>>>> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
>>>> iteration until the source pointer is equal to __image_copy_end.
>>>> In a good case __image_copy_end was aligned to 8 bytes, so the loop
>>>> stopped as suggested, but in an errornous case __image_copy_end was
>>>> not aligned to 8 bytes, so the loop ran out of bounds and caused a
>>>> data abort exception.
>>>
>>> Well I agree with this patch but I have a small query. Looking at the
>>> relocation code:
>>>
>>> copy_loop:
>>> 	ldmia	r1!, {r10-r11}		/* copy from source address [r1]    */
>>> 	stmia	r0!, {r10-r11}		/* copy to   target address [r0]    */
>>> 	cmp  	r1, r2			/* until source end address [r2]    */
>>> 	blo		copy_loop
>>>
>>> IIUC, this loops exits as soon as r1 >= r2
>>>
>>> In your case
>>> r1 is __image_copy_start
>>> r0 is relocation address
>>> r2 is __image_copy_end (which is 4 byte aligned)
>>>
>>> In the corner case you mentioned there will be extra memcpy of 4 bytes
>>> from address in r1 to address in r0. I assume you are running from DDR
>>> and relocation offset is calculated such that (aligned to previous 4K
>>> page) it should be able to accommodate extra 4 bytes(I assume). I am
>>> wondering why should this give a data abort.
>>>
>>> Thanks and regards,
>>> Lokesh
>>>
>>
>> Thanks a lot for your input!
>> The patch solved my problem randomly.
>>
>> The loop terminates only one word beyond __image_copy_end.
>> This is not the problem in my case because both, the source and destination
>> pointers point to valid addresses in dram.
>> So the problem must be later in relocate_code.
> 
> That's right.
> 
>>
>> I spent some time using a debugger and found that the data abort happens here
>>
>> 	/*
>> 	 * fix .rel.dyn relocations
>> 	 */
>> 	ldr	r2, =__rel_dyn_start	/* r2 <- SRC &__rel_dyn_start */
>> 	ldr	r3, =__rel_dyn_end	/* r3 <- SRC &__rel_dyn_end */
>> fixloop:
>> 	ldmia	r2!, {r0-r1}		/* (r0,r1) <- (SRC location,fixup) */
>> 	and	r1, r1, #0xff
>> 	cmp	r1, #R_ARM_RELATIVE
>> 	bne	fixnext
>>
>> 	/* relative fix: increase location by offset */
>> 	add	r0, r0, r4
>> 	ldr	r1, [r0]
>> 	add	r1, r1, r4
>> 	str	r1, [r0]
>> fixnext:
>> 	cmp	r2, r3
>> 	blo	fixloop
>>
>>
>> Also I found out, that it's not the alignment of image_copy_end, but of rel_dyn_start
>> (which was also implicitly changed by my patch).
>>
>> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
>> rel_dyn_start is 0x8785FC28
>> rel_dyn_end is 0x87857BD0
>> A dump of this memory area shows no abnormality
>>
>> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
>> rel_dyn_start is 0x8785FC24
>> rel_dyn_end is 0x87857BCC
>> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
>> very interesting:
>>
>> At offset 0x610 (relative to rel_dyn_start) we have following difference
>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
> 
> Looks like someone is corrupting the data(assuming). Is it all 0's just
> at this location or continuously after this?

No. Above diff is the only difference of the good and bad case in memory located between
rel_dyn_start and rel_dyn_end.

To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
found the same difference
-00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
+00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump

So it must be some kind of corruption.

> 
>  If possible can you try the following experiments with the failing code:
> 
> - Dump rel_dyn_* data right after spl copies U-boot to ddr.
> - Similarly dump rel_dyn_* data before calling relocate_code() and
> compare both?

We don't use SPL bootloader. On boot from NAND, the ROM Bootloader does the whole job.
While testing I use JTAG to place the image in RAM and start it from there.
The Problem happens in both cases, so I can eliminate the ROM Bootloader.
I understand what you mean. I will try to check the memory location on some points while
bootloader startup to localize the corruption.

> 
> If the comparison fails, add a write breakpoint at the corrupted address
> before starting u-boot and see when it hits.
> 
> BTW, where is your BSS located?

__bss_base says 87857bcc (in bad case). This is also exactly where rel_dyn_start starts
extracted from System.map
{{{
87857bcc B __bss_base
87857bcc B __bss_start
87857bcc D __image_copy_end
87857bcc D __rel_dyn_start
}}}

Thanks a lot!

Best regards,
Manfred

> 
> Thanks and regards,
> Lokesh
> 
>>
>> Here is a pseudo-trace of fixloop in bad case starting at offset 0x618 (relative to rel_dyn_start)
>>
>> fixloop:				// R2 = 0x878581E4, R4 = 0x08786000
>> 	ldmia	r2!, {r0-r1}		// R0 = 0x00000000, R1 = 0x00000017, R2 = 0x878581EC
>> 	and	r1, r1, #0xff
>> 	cmp	r1, #R_ARM_RELATIVE
>> 	bne	fixnext			// R1 == 0x17 -> no branch
>> 	
>> 	/* relative fix: increase location by offset */
>> 	add	r0, r0, r4		// R0 = 0x08786000
>> 	ldr	r1, [r0]		// ABORT while accessing 0x08786000 -> not a valid address in dram
>>
>>
>> It seems, that the entry 00 00 00 00 17 00 00 00 @ offset 0x618 is some how incorrect, or at least
>> produces some incorrect behavior. There are no similar entries in the table.
>>
>> I'm not really experienced with these tables.
>> Has anyone suggestions, starting points, ideas/hints which could help me to track this down?
>>
>> Thanks a lot!
>>
>> Best regards,
>> Manfred
>>
>>
>>
>>
>>

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Lothar Waßmann]

Manfred Schlaegl <manfred.schlaegl@ginzinger.com> wrote:

> On 2017-05-17 06:13, Lokesh Vutla wrote:
> > 
> > 
> > On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
> >> On 2017-05-11 08:53, Lokesh Vutla wrote:
> >>>
> >>>
> >>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
> >>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> >>>> Initially U-Boot could be started normally.
> >>>> If we added one random command in configuration, the newly generated
> >>>> image hung at startup (last output was DRAM:  256 MiB).
> >>>>
> >>>> We tracked this down to a data abort within relocation (relocated_code).
> >>>>
[...]
> >> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
> >> rel_dyn_start is 0x8785FC28
> >> rel_dyn_end is 0x87857BD0
> >> A dump of this memory area shows no abnormality
> >>
> >> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
> >> rel_dyn_start is 0x8785FC24
> >> rel_dyn_end is 0x87857BCC
> >> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
> >> very interesting:
> >>
> >> At offset 0x610 (relative to rel_dyn_start) we have following difference
> >> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
> >> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
> > 
> > Looks like someone is corrupting the data(assuming). Is it all 0's just
> > at this location or continuously after this?
> 
> No. Above diff is the only difference of the good and bad case in memory located between
> rel_dyn_start and rel_dyn_end.
> 
> To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
> found the same difference
> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump
> 
> So it must be some kind of corruption.
> 
This can be caused by a static variable, that is written to prior to
relocation. Since the .rel section overlays the .bss section, the write
to a variable in the BSS will corrupt the relocation data.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-05-18 16:59, Lothar Waßmann wrote:
> Manfred Schlaegl <manfred.schlaegl@ginzinger.com> wrote:
> 
>> On 2017-05-17 06:13, Lokesh Vutla wrote:
>>>
>>>
>>> On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
>>>> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>>>>
>>>>>
>>>>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>>>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>>>>> Initially U-Boot could be started normally.
>>>>>> If we added one random command in configuration, the newly generated
>>>>>> image hung at startup (last output was DRAM:  256 MiB).
>>>>>>
>>>>>> We tracked this down to a data abort within relocation (relocated_code).
>>>>>>
> [...]
>>>> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
>>>> rel_dyn_start is 0x8785FC28
>>>> rel_dyn_end is 0x87857BD0
>>>> A dump of this memory area shows no abnormality
>>>>
>>>> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
>>>> rel_dyn_start is 0x8785FC24
>>>> rel_dyn_end is 0x87857BCC
>>>> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
>>>> very interesting:
>>>>
>>>> At offset 0x610 (relative to rel_dyn_start) we have following difference
>>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
>>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
>>>
>>> Looks like someone is corrupting the data(assuming). Is it all 0's just
>>> at this location or continuously after this?
>>
>> No. Above diff is the only difference of the good and bad case in memory located between
>> rel_dyn_start and rel_dyn_end.
>>
>> To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
>> found the same difference
>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump
>>
>> So it must be some kind of corruption.
>>
> This can be caused by a static variable, that is written to prior to
> relocation. Since the .rel section overlays the .bss section, the write
> to a variable in the BSS will corrupt the relocation data.
> 

Yes! That's it!

Using a watchpoint I tracked the corruption down to an early write to a static variable in our custom code.

So finally: 
The whole thing was a problem in a custom modification and was solved there. It has no implication on u-boot itself.

Thanks a lot for your help and time!

Best regards
Manfred

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Jagan Teki]

On Thu, May 18, 2017 at 9:04 PM, Manfred Schlaegl
<manfred.schlaegl@ginzinger.com> wrote:
> On 2017-05-18 16:59, Lothar Waßmann wrote:
>> Manfred Schlaegl <manfred.schlaegl@ginzinger.com> wrote:
>>
>>> On 2017-05-17 06:13, Lokesh Vutla wrote:
>>>>
>>>>
>>>> On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
>>>>> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>>>>>
>>>>>>
>>>>>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>>>>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>>>>>> Initially U-Boot could be started normally.
>>>>>>> If we added one random command in configuration, the newly generated
>>>>>>> image hung at startup (last output was DRAM:  256 MiB).
>>>>>>>
>>>>>>> We tracked this down to a data abort within relocation (relocated_code).
>>>>>>>
>> [...]
>>>>> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
>>>>> rel_dyn_start is 0x8785FC28
>>>>> rel_dyn_end is 0x87857BD0
>>>>> A dump of this memory area shows no abnormality
>>>>>
>>>>> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
>>>>> rel_dyn_start is 0x8785FC24
>>>>> rel_dyn_end is 0x87857BCC
>>>>> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
>>>>> very interesting:
>>>>>
>>>>> At offset 0x610 (relative to rel_dyn_start) we have following difference
>>>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
>>>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
>>>>
>>>> Looks like someone is corrupting the data(assuming). Is it all 0's just
>>>> at this location or continuously after this?
>>>
>>> No. Above diff is the only difference of the good and bad case in memory located between
>>> rel_dyn_start and rel_dyn_end.
>>>
>>> To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
>>> found the same difference
>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump
>>>
>>> So it must be some kind of corruption.
>>>
>> This can be caused by a static variable, that is written to prior to
>> relocation. Since the .rel section overlays the .bss section, the write
>> to a variable in the BSS will corrupt the relocation data.
>>
>
> Yes! That's it!
>
> Using a watchpoint I tracked the corruption down to an early write to a static variable in our custom code.
>
> So finally:
> The whole thing was a problem in a custom modification and was solved there. It has no implication on u-boot itself.

Any pointers on which kind of custom modification, because I could see
the similar issue with upstream u-boot.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-05-18 17:37, Jagan Teki wrote:
> On Thu, May 18, 2017 at 9:04 PM, Manfred Schlaegl
> <manfred.schlaegl@ginzinger.com> wrote:
>> On 2017-05-18 16:59, Lothar Waßmann wrote:
>>> Manfred Schlaegl <manfred.schlaegl@ginzinger.com> wrote:
>>>
>>>> On 2017-05-17 06:13, Lokesh Vutla wrote:
>>>>>
>>>>>
>>>>> On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
>>>>>> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>>>>>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>>>>>>> Initially U-Boot could be started normally.
>>>>>>>> If we added one random command in configuration, the newly generated
>>>>>>>> image hung at startup (last output was DRAM:  256 MiB).
>>>>>>>>
>>>>>>>> We tracked this down to a data abort within relocation (relocated_code).
>>>>>>>>
>>> [...]
>>>>>> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
>>>>>> rel_dyn_start is 0x8785FC28
>>>>>> rel_dyn_end is 0x87857BD0
>>>>>> A dump of this memory area shows no abnormality
>>>>>>
>>>>>> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
>>>>>> rel_dyn_start is 0x8785FC24
>>>>>> rel_dyn_end is 0x87857BCC
>>>>>> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
>>>>>> very interesting:
>>>>>>
>>>>>> At offset 0x610 (relative to rel_dyn_start) we have following difference
>>>>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
>>>>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
>>>>>
>>>>> Looks like someone is corrupting the data(assuming). Is it all 0's just
>>>>> at this location or continuously after this?
>>>>
>>>> No. Above diff is the only difference of the good and bad case in memory located between
>>>> rel_dyn_start and rel_dyn_end.
>>>>
>>>> To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
>>>> found the same difference
>>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
>>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump
>>>>
>>>> So it must be some kind of corruption.
>>>>
>>> This can be caused by a static variable, that is written to prior to
>>> relocation. Since the .rel section overlays the .bss section, the write
>>> to a variable in the BSS will corrupt the relocation data.
>>>
>>
>> Yes! That's it!
>>
>> Using a watchpoint I tracked the corruption down to an early write to a static variable in our custom code.
>>
>> So finally:
>> The whole thing was a problem in a custom modification and was solved there. It has no implication on u-boot itself.
> 
> Any pointers on which kind of custom modification, because I could see
> the similar issue with upstream u-boot.
> 
> thanks!
> 

For compatibility reasons we use a custom environment implementation. In this implementation we had an write access
to a static variable in env_init. env_init is called before relocation.
As Lothar stated out .bss overlaps .rel at this stage, so we corrupted .rel.

Hope that helped!

Best regards,
Manfred

[U-Boot] [PATCH] ARM: fixed relocation using proper alignment

[Lokesh Vutla]

On Thursday 18 May 2017 09:04 PM, Manfred Schlaegl wrote:
> On 2017-05-18 16:59, Lothar Waßmann wrote:
>> Manfred Schlaegl <manfred.schlaegl@ginzinger.com> wrote:
>>
>>> On 2017-05-17 06:13, Lokesh Vutla wrote:
>>>>
>>>>
>>>> On Tuesday 16 May 2017 07:59 PM, Manfred Schlaegl wrote:
>>>>> On 2017-05-11 08:53, Lokesh Vutla wrote:
>>>>>>
>>>>>>
>>>>>> On Wednesday 10 May 2017 07:11 PM, Manfred Schlaegl wrote:
>>>>>>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>>>>>>> Initially U-Boot could be started normally.
>>>>>>> If we added one random command in configuration, the newly generated
>>>>>>> image hung at startup (last output was DRAM:  256 MiB).
>>>>>>>
>>>>>>> We tracked this down to a data abort within relocation (relocated_code).
>>>>>>>
>> [...]
>>>>> In a good case (rel_dyn_start aligned to 8 byte), u-boot is starting up normally
>>>>> rel_dyn_start is 0x8785FC28
>>>>> rel_dyn_end is 0x87857BD0
>>>>> A dump of this memory area shows no abnormality
>>>>>
>>>>> In a bad case (same source, but rel_dyn_start aligned to 4 byte), the data abort happens
>>>>> rel_dyn_start is 0x8785FC24
>>>>> rel_dyn_end is 0x87857BCC
>>>>> So we have the same size of 32856 bytes but a memory dump showed exactly one difference, which is
>>>>> very interesting:
>>>>>
>>>>> At offset 0x610 (relative to rel_dyn_start) we have following difference
>>>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 00 00 00 00  |0>......4>......|
>>>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|
>>>>
>>>> Looks like someone is corrupting the data(assuming). Is it all 0's just
>>>> at this location or continuously after this?
>>>
>>> No. Above diff is the only difference of the good and bad case in memory located between
>>> rel_dyn_start and rel_dyn_end.
>>>
>>> To see if it might be a corruption I compared the the rel_dyn with the created u-boot.img and
>>> found the same difference
>>> -00000610  30 3e 80 87 17 00 00 00  34 3e 80 87 17 00 00 00  |0>......4>......|   <--- generated image
>>> +00000610  30 3e 80 87 17 00 00 00  00 00 00 00 17 00 00 00  |0>..............|   <--- memory dump
>>>
>>> So it must be some kind of corruption.
>>>
>> This can be caused by a static variable, that is written to prior to
>> relocation. Since the .rel section overlays the .bss section, the write
>> to a variable in the BSS will corrupt the relocation data.
>>
> 
> Yes! That's it!
> 
> Using a watchpoint I tracked the corruption down to an early write to a static variable in our custom code.
> 
> So finally: 
> The whole thing was a problem in a custom modification and was solved there. It has no implication on u-boot itself.

Awesome. Good to hear that it is resolved :)

Thanks and regards,
Lokesh

[U-Boot] ARM: fixed relocation using proper alignment

[Tom Rini]

On Wed, May 10, 2017 at 03:41:32PM +0200, Manfred Schlaegl wrote:

> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> Initially U-Boot could be started normally.
> If we added one random command in configuration, the newly generated
> image hung at startup (last output was DRAM:  256 MiB).
> 
> We tracked this down to a data abort within relocation (relocated_code).
> 
> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
> iteration until the source pointer is equal to __image_copy_end.
> In a good case __image_copy_end was aligned to 8 bytes, so the loop
> stopped as suggested, but in an errornous case __image_copy_end was
> not aligned to 8 bytes, so the loop ran out of bounds and caused a
> data abort exception.
> 
> This patches solves the issue by aligning __image_copy_end to 8 byte
> using the linker script related to arm.
> 
> I don't know if it's the correct way to solve this, so some review would
> be very appreciated.

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170612/af724c73/attachment.sig>

[U-Boot] ARM: fixed relocation using proper alignment

[Manfred Schlaegl]

On 2017-06-13 00:42, Tom Rini wrote:
> On Wed, May 10, 2017 at 03:41:32PM +0200, Manfred Schlaegl wrote:
>
>> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
>> Initially U-Boot could be started normally.
>> If we added one random command in configuration, the newly generated
>> image hung at startup (last output was DRAM:  256 MiB).
>>
>> We tracked this down to a data abort within relocation (relocated_code).
>>
>> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
>> iteration until the source pointer is equal to __image_copy_end.
>> In a good case __image_copy_end was aligned to 8 bytes, so the loop
>> stopped as suggested, but in an errornous case __image_copy_end was
>> not aligned to 8 bytes, so the loop ran out of bounds and caused a
>> data abort exception.
>>
>> This patches solves the issue by aligning __image_copy_end to 8 byte
>> using the linker script related to arm.
>>
>> I don't know if it's the correct way to solve this, so some review would
>> be very appreciated.
>
> Applied to u-boot/master, thanks!
>

Hi!

Please do not apply this patch!
u-boot/master was not the cause of the problem.
The patch had hidden the symptom of a bug which was located in custom modifications.

For see the other mails in the conversation for details.

Thanks and best regards
Manfred

________________________________

Ginzinger electronic systems GmbH
Gewerbegebiet Pirath 16
4952 Weng im Innkreis
www.ginzinger.com

Firmenbuchnummer: FN 364958d
Firmenbuchgericht: Ried im Innkreis
UID-Nr.: ATU66521089

[U-Boot] ARM: fixed relocation using proper alignment

[Tom Rini]

On Wed, Jun 14, 2017 at 02:58:31PM +0200, Manfred Schlaegl wrote:
> On 2017-06-13 00:42, Tom Rini wrote:
> > On Wed, May 10, 2017 at 03:41:32PM +0200, Manfred Schlaegl wrote:
> >
> >> Using u-boot-2017.05 on i.MX6UL we ran into following problem:
> >> Initially U-Boot could be started normally.
> >> If we added one random command in configuration, the newly generated
> >> image hung at startup (last output was DRAM:  256 MiB).
> >>
> >> We tracked this down to a data abort within relocation (relocated_code).
> >>
> >> relocated_code in arch/arm/lib/relocate.S copies 8 bytes per loop
> >> iteration until the source pointer is equal to __image_copy_end.
> >> In a good case __image_copy_end was aligned to 8 bytes, so the loop
> >> stopped as suggested, but in an errornous case __image_copy_end was
> >> not aligned to 8 bytes, so the loop ran out of bounds and caused a
> >> data abort exception.
> >>
> >> This patches solves the issue by aligning __image_copy_end to 8 byte
> >> using the linker script related to arm.
> >>
> >> I don't know if it's the correct way to solve this, so some review would
> >> be very appreciated.
> >
> > Applied to u-boot/master, thanks!
> 
> Hi!
> 
> Please do not apply this patch!
> u-boot/master was not the cause of the problem.
> The patch had hidden the symptom of a bug which was located in custom modifications.
> 
> For see the other mails in the conversation for details.

Oh? Hmm, sorry, it read like this was still a useful / correct change
when I was looking over it.  I'll undo it, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170614/a9affb5f/attachment.sig>