Patchwork [U-Boot,v2,RFC] armv7: fixloop: don't fixup if location is invalid on RAM

login
register
mail settings
Submitter Minkyu Kang
Date Jan. 4, 2011, 8:52 a.m.
Message ID <4D22DFBE.8090503@samsung.com>
Download mbox | patch
Permalink /patch/77439/
State Changes Requested
Headers show

Comments

Minkyu Kang - Jan. 4, 2011, 8:52 a.m.
Need to check that location is valid on RAM before the fixup.

Signed-off-by: Minkyu Kang <mk7.kang@samsung.com>
---
 arch/arm/cpu/armv7/start.S |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Joakim Tjernlund - Jan. 4, 2011, 9:49 a.m.
>
> Need to check that location is valid on RAM before the fixup.

Why this change? The [PATCH RFC] armv7: fixloop: don't fixup if location is NULL
should be what you need.
You could mention what causes these NULL fixups, here is commit
entry for powerpc:

powerpc: do not fixup NULL ptrs

    The fixup routine must not fixup NULL pointers.
    Problem can be seen by
     char *testfun(void) __attribute__((weak));
     char *(*myfun)(void) = testfun;

    Then add
      printf("myfun:%p, &myfun:%p\n", myfun, &myfun);
    before relocation and after relocation.
    myfun should be NULL in both cases but it is not.
Minkyu Kang - Jan. 4, 2011, 10:04 a.m.
Dear Joakim Tjernlund,

On 4 January 2011 18:49, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>>
>> Need to check that location is valid on RAM before the fixup.
>
> Why this change? The [PATCH RFC] armv7: fixloop: don't fixup if location is NULL
> should be what you need.

Some bss variables are initialized to 0xffffffff in my test case.
In this case we should skip the fixup too.
So, I make v2 patch for check it.

Thanks
Minkyu Kang
Joakim Tjernlund - Jan. 4, 2011, 10:31 a.m.
Minkyu Kang <promsoft@gmail.com> wrote on 2011/01/04 11:04:57:
>
> Dear Joakim Tjernlund,
>
> On 4 January 2011 18:49, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >>
> >> Need to check that location is valid on RAM before the fixup.
> >
> > Why this change? The [PATCH RFC] armv7: fixloop: don't fixup if location is NULL
> > should be what you need.
>
> Some bss variables are initialized to 0xffffffff in my test case.
> In this case we should skip the fixup too.
> So, I make v2 patch for check it.

0xffffffff? What variables? Feels like a some other bug to me.
Minkyu Kang - Jan. 4, 2011, 11:02 a.m.
Dear Joakim Tjernlund,

On 4 January 2011 19:31, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> Minkyu Kang <promsoft@gmail.com> wrote on 2011/01/04 11:04:57:
>>
>> Dear Joakim Tjernlund,
>>
>> On 4 January 2011 18:49, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>> >>
>> >> Need to check that location is valid on RAM before the fixup.
>> >
>> > Why this change? The [PATCH RFC] armv7: fixloop: don't fixup if location is NULL
>> > should be what you need.
>>
>> Some bss variables are initialized to 0xffffffff in my test case.
>> In this case we should skip the fixup too.
>> So, I make v2 patch for check it.
>
> 0xffffffff? What variables? Feels like a some other bug to me.

It's DRAM initial variable.

Thanks
Minkyu Kang
Joakim Tjernlund - Jan. 4, 2011, 4:23 p.m.
Minkyu Kang <promsoft@gmail.com> wrote on 2011/01/04 12:02:29:
>
> Dear Joakim Tjernlund,
>
> On 4 January 2011 19:31, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > Minkyu Kang <promsoft@gmail.com> wrote on 2011/01/04 11:04:57:
> >>
> >> Dear Joakim Tjernlund,
> >>
> >> On 4 January 2011 18:49, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> >> >>
> >> >> Need to check that location is valid on RAM before the fixup.
> >> >
> >> > Why this change? The [PATCH RFC] armv7: fixloop: don't fixup if location is NULL
> >> > should be what you need.
> >>
> >> Some bss variables are initialized to 0xffffffff in my test case.
> >> In this case we should skip the fixup too.
> >> So, I make v2 patch for check it.
> >
> > 0xffffffff? What variables? Feels like a some other bug to me.
>
> It's DRAM initial variable.

what does that mean? initial value of the whole DRAM?
With such low level of detail one cannot do much and now
I am travelling a few days so no help from me.
In either case it feels like another bug and playing games with fixup
isn't the right solution.

 Jocke
Albert ARIBAUD - Jan. 4, 2011, 5:02 p.m.
(sending again, this time to the list)

Le 04/01/2011 12:02, Minkyu Kang a écrit :

>>> Some bss variables are initialized to 0xffffffff in my test case.
>>> In this case we should skip the fixup too.
>>> So, I make v2 patch for check it.
>>
>> 0xffffffff? What variables? Feels like a some other bug to me.
>
> It's DRAM initial variable.

Yes, seems like an unrelated issue to me, or at least an unexplained one 
so far.

For instance, there is no fixup to BSS variables (they are to be 
initialized to 0 whatever the load address is, and this initialization 
is not in the binary, it is done in the startup code). So which fixup 
should be skipped exactly and how is is related to the BSS variable?

> Thanks
> Minkyu Kang

Amicalement,
Minkyu Kang - Jan. 5, 2011, 5:27 a.m.
On 5 January 2011 02:02, Albert ARIBAUD <albert.aribaud@free.fr> wrote:
> (sending again, this time to the list)
>
> Le 04/01/2011 12:02, Minkyu Kang a écrit :
>
>>>> Some bss variables are initialized to 0xffffffff in my test case.
>>>> In this case we should skip the fixup too.
>>>> So, I make v2 patch for check it.
>>>
>>> 0xffffffff? What variables? Feels like a some other bug to me.
>>
>> It's DRAM initial variable.
>
> Yes, seems like an unrelated issue to me, or at least an unexplained one
> so far.
>
> For instance, there is no fixup to BSS variables (they are to be
> initialized to 0 whatever the load address is, and this initialization
> is not in the binary, it is done in the startup code). So which fixup
> should be skipped exactly and how is is related to the BSS variable?
>

Yes, you right.
Seems to another bug.
It sometimes happens at arrays on BSS area.
OK, I'll investigate this problem.

How about v1 patch for NULL pointer.
Do you agreed with this patch?
http://patchwork.ozlabs.org/patch/76784/

If so, I'm going to make a patch for all of ARM CPUs.

Thanks
Minkyu Kang
Albert ARIBAUD - Jan. 8, 2011, 7:43 a.m.
Hi,

Le 05/01/2011 06:27, Minkyu Kang a écrit :

> How about v1 patch for NULL pointer.
> Do you agreed with this patch?
> http://patchwork.ozlabs.org/patch/76784/

This one I'm okay with; please repost as a non-RFC, simple, [PATCH].

> If so, I'm going to make a patch for all of ARM CPUs.

That would be great. :)

> Thanks
> Minkyu Kang

Amicalement,

Patch

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 684f2d2..a052378 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -195,6 +195,11 @@  copy_loop:
 	add	r3, r3, r0		/* r3 <- rel dyn end in FLASH */
 fixloop:
 	ldr	r0, [r2]		/* r0 <- location to fix up, IN FLASH! */
+	ldr	r7, _TEXT_BASE
+	cmp	r0, r7
+	blo	fixskip
+	cmp	r0, r6
+	bhs	fixskip
 	add	r0, r0, r9		/* r0 <- location to fix up in RAM */
 	ldr	r1, [r2, #4]
 	and	r7, r1, #0xff
@@ -217,6 +222,7 @@  fixrel:
 	add	r1, r1, r9
 fixnext:
 	str	r1, [r0]
+fixskip:
 	add	r2, r2, #8		/* each rel.dyn entry is 8 bytes */
 	cmp	r2, r3
 	blo	fixloop