diff mbox

[U-Boot,1/1] armv7: start.S: Fix relocation address caculation

Message ID 1292421479-2269-1-git-send-email-r64343@freescale.com
State Deferred
Headers show

Commit Message

Liu Hui-R64343 Dec. 15, 2010, 1:57 p.m. UTC
There will have issue if the _start not equal TEXT_BASE
when enable relocation. The reason is as the followings,

The _dynsym_start_ofs and _rel_dyn_start_ofs is the
offset from _start, so, need use _start address instead
of _TEXT_BASE to caculate the rel dyn start and sym table
address. This patch also correct the board_init_r function
address caculation in relocation area. The addr of board_init_r
after relocation is: _board_init_r_offs + relocation start address.

This patch also make code cleanup by removing some useless code

Signed-off-by: Jason Liu <r64343@freescale.com>
---
 arch/arm/cpu/armv7/start.S |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

Comments

Albert ARIBAUD Dec. 15, 2010, 5:06 p.m. UTC | #1
Hi Jason,

Le 15/12/2010 14:57, Jason Liu a écrit :
> There will have issue if the _start not equal TEXT_BASE
> when enable relocation.

In what case does this happen?

Amicalement,
Jason Liu Dec. 16, 2010, 3:04 a.m. UTC | #2
Hi, Albert,

2010/12/16 Albert ARIBAUD <albert.aribaud@free.fr>:
> Hi Jason,
>
> Le 15/12/2010 14:57, Jason Liu a écrit :
>> There will have issue if the _start not equal TEXT_BASE
>> when enable relocation.
>
> In what case does this happen?

Some ARM SOC ROM need run the plug-in code first in IRAM and the
plugin-in code need appear at the beginning of the u-boot. ROM will
check the plugin-in header to do security check and run the plug-in
code to init the DDR etc. In this case the _start will be not the same
as TEXT_BASE.

Thanks for your comments. Cheers,

>
> Amicalement,
> --
> Albert.
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Albert ARIBAUD Dec. 16, 2010, 9:04 a.m. UTC | #3
Le 16/12/2010 04:04, Jason Liu a écrit :
> Hi, Albert,
>
> 2010/12/16 Albert ARIBAUD<albert.aribaud@free.fr>:
>> Hi Jason,
>>
>> Le 15/12/2010 14:57, Jason Liu a écrit :
>>> There will have issue if the _start not equal TEXT_BASE
>>> when enable relocation.
>>
>> In what case does this happen?
>
> Some ARM SOC ROM need run the plug-in code first in IRAM and the
> plugin-in code need appear at the beginning of the u-boot. ROM will
> check the plugin-in header to do security check and run the plug-in
> code to init the DDR etc. In this case the _start will be not the same
> as TEXT_BASE.

I still don't see why u-boot would not end up where specified.

The fact that there is a "plug-in" (I assume it's what I would call an 
IPL) does not change the fact that its payload (u-boot) can and will be 
loaded where specified, i.e. at TEXT_BASE -- and if it is loaded 
elsewhere, it is at a fixed address, so TEXT_BASE can be adjusted) All 
IPLs that I know of put their payload where specified.

> Thanks for your comments. Cheers,

Amicalement,
Jason Liu Dec. 16, 2010, 9:18 a.m. UTC | #4
Hi, Albert,

2010/12/16 Albert ARIBAUD <albert.aribaud@free.fr>:
> Le 16/12/2010 04:04, Jason Liu a écrit :
>>
>> Hi, Albert,
>>
>> 2010/12/16 Albert ARIBAUD<albert.aribaud@free.fr>:
>>>
>>> Hi Jason,
>>>
>>> Le 15/12/2010 14:57, Jason Liu a écrit :
>>>>
>>>> There will have issue if the _start not equal TEXT_BASE
>>>> when enable relocation.
>>>
>>> In what case does this happen?
>>
>> Some ARM SOC ROM need run the plug-in code first in IRAM and the
>> plugin-in code need appear at the beginning of the u-boot. ROM will
>> check the plugin-in header to do security check and run the plug-in
>> code to init the DDR etc. In this case the _start will be not the same
>> as TEXT_BASE.
>
> I still don't see why u-boot would not end up where specified.
>
> The fact that there is a "plug-in" (I assume it's what I would call an IPL)
> does not change the fact that its payload (u-boot) can and will be loaded
> where specified, i.e. at TEXT_BASE -- and if it is loaded elsewhere, it is
> at a fixed address, so TEXT_BASE can be adjusted) All IPLs that I know of
> put their payload where specified.

It's not an IPL. The layout is that as the following,

---- ----- TEXT_BASE
plug-in
---------- _start

---------- _end

No matter what you adjusted the TEXT_BASE, the _star is not equal to it.

The fix doe not affect the original functionality but just make it
more flexible.

>
>> Thanks for your comments. Cheers,
>
> Amicalement,
> --
> Albert.
>
Wolfgang Denk Dec. 16, 2010, 9:53 a.m. UTC | #5
Dear Jason Liu,

In message <AANLkTimMct8TKe7k-0LA4Gob1mJZnCw0f9xbbp9VKMoz@mail.gmail.com> you wrote:
> 
> > In what case does this happen?
> 
> Some ARM SOC ROM need run the plug-in code first in IRAM and the
> plugin-in code need appear at the beginning of the u-boot. ROM will
> check the plugin-in header to do security check and run the plug-in
> code to init the DDR etc. In this case the _start will be not the same
> as TEXT_BASE.

So we are talking about aSoC that is not supported yet in U-Boot.
Let's talk about this when such support gets added to U-Boot.

Best regards,

Wolfgang Denk
Albert ARIBAUD Dec. 16, 2010, 9:58 a.m. UTC | #6
Le 16/12/2010 10:18, Jason Liu a écrit :
> Hi, Albert,
>
> 2010/12/16 Albert ARIBAUD<albert.aribaud@free.fr>:
>> Le 16/12/2010 04:04, Jason Liu a écrit :
>>>
>>> Hi, Albert,
>>>
>>> 2010/12/16 Albert ARIBAUD<albert.aribaud@free.fr>:
>>>>
>>>> Hi Jason,
>>>>
>>>> Le 15/12/2010 14:57, Jason Liu a écrit :
>>>>>
>>>>> There will have issue if the _start not equal TEXT_BASE
>>>>> when enable relocation.
>>>>
>>>> In what case does this happen?
>>>
>>> Some ARM SOC ROM need run the plug-in code first in IRAM and the
>>> plugin-in code need appear at the beginning of the u-boot. ROM will
>>> check the plugin-in header to do security check and run the plug-in
>>> code to init the DDR etc. In this case the _start will be not the same
>>> as TEXT_BASE.
>>
>> I still don't see why u-boot would not end up where specified.
>>
>> The fact that there is a "plug-in" (I assume it's what I would call an IPL)
>> does not change the fact that its payload (u-boot) can and will be loaded
>> where specified, i.e. at TEXT_BASE -- and if it is loaded elsewhere, it is
>> at a fixed address, so TEXT_BASE can be adjusted) All IPLs that I know of
>> put their payload where specified.
>
> It's not an IPL. The layout is that as the following,
>
> ---- ----- TEXT_BASE
> plug-in
> ---------- _start
>
> ---------- _end
>
> No matter what you adjusted the TEXT_BASE, the _star is not equal to it.
>
> The fix doe not affect the original functionality but just make it
> more flexible.

This layout is not that of u-boot for ARM; the fix thus corrects a fault 
not inherent to u-boot but introduced by inserting this "plug-in" where 
it should not be.

Why must you modify the original layout?

Also, what is this 'plug-in' if it is not an IPL?

Amicalement,
Wolfgang Denk Dec. 16, 2010, 10:09 a.m. UTC | #7
Dear Albert ARIBAUD,

In message <4D09E2B9.5030405@free.fr> you wrote:
>
> Why must you modify the original layout?

right!

> Also, what is this 'plug-in' if it is not an IPL?

Yes, and why must it be linked at TEXT_BASE?

Let's stop this here.

Let's wait until any code is submitted or proposed that actually
needs such a feature.  Then we can discuss the design and try to find
a solution that makes sense.

Best regards,

Wolfgang Denk
Jason Liu Dec. 16, 2010, 10:22 a.m. UTC | #8
Hi, Wolfgang,

2010/12/16 Wolfgang Denk <wd@denx.de>:
> Dear Albert ARIBAUD,
>
> In message <4D09E2B9.5030405@free.fr> you wrote:
>>
>> Why must you modify the original layout?
>
> right!
>
>> Also, what is this 'plug-in' if it is not an IPL?
>
> Yes, and why must it be linked at TEXT_BASE?
>
> Let's stop this here.
>
> Let's wait until any code is submitted or proposed that actually
> needs such a feature.  Then we can discuss the design and try to find
> a solution that makes sense.

In fact, this is indeed one bug in the code as I said in the comit log:

"the _dynsym_start_ofs and _rel_dyn_start_ofs is the
offset from _start, so, need use _start address instead
of _TEXT_BASE to caculate the rel dyn start and sym table
address. This patch also correct the board_init_r function
address caculation in relocation area. The addr of board_init_r
after relocation is: _board_init_r_offs + relocation start address."

Is there any thing wrong with my patch?
The case I met just make the bug show up.


>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> History is only a confused heap of facts.
>                                       -- Philip Earl of Chesterfield
>
Wolfgang Denk Dec. 16, 2010, 12:20 p.m. UTC | #9
Dear Jason Liu,

In message <AANLkTik=BSaBbV-NOJYg2QXx0Q-YYK4=Ox3iF8dWR5re@mail.gmail.com> you wrote:
> 
> In fact, this is indeed one bug in the code as I said in the comit log:
> 
> "the _dynsym_start_ofs and _rel_dyn_start_ofs is the
> offset from _start, so, need use _start address instead
> of _TEXT_BASE to caculate the rel dyn start and sym table
> address. This patch also correct the board_init_r function
> address caculation in relocation area. The addr of board_init_r
> after relocation is: _board_init_r_offs + relocation start address."

If there are really two different bugs, then please split your patch
into to separate commits, and explain the respective problem in the
commit messages.

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 684f2d2..08902b0 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -171,7 +171,6 @@  stack_setup:
 	beq	clear_bss		/* skip relocation */
 #endif
 	mov	r1, r6			/* r1 <- scratch for copy_loop */
-	ldr	r2, _TEXT_BASE
 	ldr	r3, _bss_start_ofs
 	add	r2, r0, r3		/* r2 <- source end address	    */
 
@@ -185,7 +184,7 @@  copy_loop:
 	/*
 	 * fix .rel.dyn relocations
 	 */
-	ldr	r0, _TEXT_BASE		/* r0 <- Text base */
+	adr     r0, _start
 	sub	r9, r6, r0		/* r9 <- relocation offset */
 	ldr	r10, _dynsym_start_ofs	/* r10 <- sym table ofs */
 	add	r10, r10, r0		/* r10 <- sym table in FLASH */
@@ -224,7 +223,6 @@  fixnext:
 clear_bss:
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
-	ldr	r3, _TEXT_BASE		/* Text base */
 	mov	r4, r6			/* reloc addr */
 	add	r0, r0, r4
 	add	r1, r1, r4
@@ -242,9 +240,7 @@  clbss_l:str	r2, [r0]		/* clear loop...		    */
  */
 jump_2_ram:
 	ldr	r0, _board_init_r_ofs
-	adr	r1, _start
-	add	lr, r0, r1
-	add	lr, lr, r9
+	add	lr, r0, r4
 	/* setup parameters for board_init_r */
 	mov	r0, r5		/* gd_t */
 	mov	r1, r6		/* dest_addr */