[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[Zhi-zhou Zhang]

If bal is 8 bytes aligned, the _gp will not be 8 bytes aligned.
then the following ld insntrustion generates a Adel exception.
So here make _gp be always aligned in 8 bytes.

Signed-off-by: Zhi-zhou Zhang <zhizhou.zh@gmail.com>
---
 arch/mips/cpu/mips64/start.S |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/mips/cpu/mips64/start.S b/arch/mips/cpu/mips64/start.S
index 4112de7..8e8cc33 100644
--- a/arch/mips/cpu/mips64/start.S
+++ b/arch/mips/cpu/mips64/start.S
@@ -108,7 +108,10 @@ reset:
 	mtc0	t0, CP0_CONFIG
 #endif
 
-	/* Initialize $gp */
+	/* Initialize $gp, _gp must be 8 bytes algined. */
+	.align 3
+	nop
+	nop
 	bal	1f
 	 nop
 	.dword	_gp
-- 
1.7.9.5

[U-Boot] [Patch 2/2] MIPS: do not modify variable before relocate_code

[Zhi-zhou Zhang]

Because timestamp is declared as `static', we needn't initialize
it by writing it a zero. If we do it before relocate_code, we
will write into a flash address(0xffffffffbfc0xxxx).

Signed-off-by: Zhi-zhou Zhang <zhizhou.zh@gmail.com>
---
 arch/mips/cpu/mips32/time.c |    1 -
 arch/mips/cpu/mips64/time.c |    1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/mips/cpu/mips32/time.c b/arch/mips/cpu/mips32/time.c
index 350896a..09fc842 100644
--- a/arch/mips/cpu/mips32/time.c
+++ b/arch/mips/cpu/mips32/time.c
@@ -36,7 +36,6 @@ static unsigned long timestamp;
 int timer_init(void)
 {
 	/* Set up the timer for the first expiration. */
-	timestamp = 0;
 	write_c0_compare(read_c0_count() + CYCLES_PER_JIFFY);
 
 	return 0;
diff --git a/arch/mips/cpu/mips64/time.c b/arch/mips/cpu/mips64/time.c
index 5154280..720f7b7 100644
--- a/arch/mips/cpu/mips64/time.c
+++ b/arch/mips/cpu/mips64/time.c
@@ -37,7 +37,6 @@ static unsigned long timestamp;
 int timer_init(void)
 {
 	/* Set up the timer for the first expiration. */
-	timestamp = 0;
 	write_c0_compare(read_c0_count() + CYCLES_PER_JIFFY);
 
 	return 0;
-- 
1.7.9.5

[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[Daniel Schwierzeck]

2012/11/24 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> If bal is 8 bytes aligned, the _gp will not be 8 bytes aligned.
> then the following ld insntrustion generates a Adel exception.
> So here make _gp be always aligned in 8 bytes.

which toolchain do you use? Actually _gp is aligned to 16 bytes in the
linker script.
Thus the instruction "ld gp, 0(ra)" and the address loaded into gp is
always aligned to 8 bytes.
This works at least with ELDK-5.2.1 and a self-built gcc-4.7.2.

>
> Signed-off-by: Zhi-zhou Zhang <zhizhou.zh@gmail.com>
> ---
>  arch/mips/cpu/mips64/start.S |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/cpu/mips64/start.S b/arch/mips/cpu/mips64/start.S
> index 4112de7..8e8cc33 100644
> --- a/arch/mips/cpu/mips64/start.S
> +++ b/arch/mips/cpu/mips64/start.S
> @@ -108,7 +108,10 @@ reset:
>         mtc0    t0, CP0_CONFIG
>  #endif
>
> -       /* Initialize $gp */
> +       /* Initialize $gp, _gp must be 8 bytes algined. */
> +       .align 3
> +       nop
> +       nop
>         bal     1f
>          nop
>         .dword  _gp
> --
> 1.7.9.5
>

-- 
Best regards,
Daniel

[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[Zhi-zhou Zhang]

On Sun, Nov 25, 2012 at 09:30:54PM +0100, Daniel Schwierzeck wrote:
> 2012/11/24 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> > If bal is 8 bytes aligned, the _gp will not be 8 bytes aligned.
> > then the following ld insntrustion generates a Adel exception.
> > So here make _gp be always aligned in 8 bytes.
> 
> which toolchain do you use? Actually _gp is aligned to 16 bytes in the
> linker script.
> Thus the instruction "ld gp, 0(ra)" and the address loaded into gp is
> always aligned to 8 bytes.
> This works at least with ELDK-5.2.1 and a self-built gcc-4.7.2.
Thanks for review.
I have do a simple test by modify this snip as following:
.align 3
  //nop
  nop
  bal     1f
  nop
.dword  _gp
1:
  ld      gp, 0(ra)
Qemu give me a Adel exception:
(qemu) info registers 
pc=0xffffffffbfc00688 HI=0x0000000000000000 LO=0x0000000000000000 ds
0098 0000000000000000 0
GPR00: r0 0000000000000000 at 000000001000009f v0 0000000000000000 v1
0000000000000000
GPR04: a0 0000000000000000 a1 0000000000000000 a2 0000000000000000 a3
0000000000000000
GPR08: t0 0000000000000000 t1 0000000000000000 t2 0000000000000000 t3
0000000000000000
GPR12: t4 0000000000000002 t5 0000000000000000 t6 0000000000000000 t7
0000000000000000
GPR16: s0 0000000000000000 s1 0000000000000000 s2 0000000000000000 s3
0000000000000000
GPR20: s4 0000000000000000 s5 0000000000000000 s6 0000000000000000 s7
0000000000000000
GPR24: t8 0000000000000000 t9 0000000000000000 k0 0000000000000000 k1
0000000000000000
GPR28: gp 0000000000000000 sp 0000000000000000 s8 0000000000000000 ra
ffffffffbfc00544
CP0 Status  0x10400082 Cause   0x00000410 EPC    0xffffffffbfc00550
    Config0 0x80004482 Config1 0xfea3519b LLAddr 0x0000000000000000

We can also see the arrange with objdump:
ffffffffbfc00538:       00000000        nop                                         
ffffffffbfc0053c:       04110004        bal     ffffffffbfc00550
ffffffffbfc00540:       00000000        nop                                         
ffffffffbfc00544:       00000000        nop                                         
ffffffffbfc00548:       bfc36ed0        cache   0x3,28368(s8)                       
ffffffffbfc0054c:       ffffffff        sd      ra,-1(ra)                           
ffffffffbfc00550:       dffc0000        ld      gp,0(ra)

It shows that although _gp is aligned to 8 bytes. but it's not in the
address of 0(ra).
> 
> >
> > Signed-off-by: Zhi-zhou Zhang <zhizhou.zh@gmail.com>
> > ---
> >  arch/mips/cpu/mips64/start.S |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/mips/cpu/mips64/start.S b/arch/mips/cpu/mips64/start.S
> > index 4112de7..8e8cc33 100644
> > --- a/arch/mips/cpu/mips64/start.S
> > +++ b/arch/mips/cpu/mips64/start.S
> > @@ -108,7 +108,10 @@ reset:
> >         mtc0    t0, CP0_CONFIG
> >  #endif
> >
> > -       /* Initialize $gp */
> > +       /* Initialize $gp, _gp must be 8 bytes algined. */
> > +       .align 3
> > +       nop
> > +       nop
> >         bal     1f
> >          nop
> >         .dword  _gp
> > --
> > 1.7.9.5
> >
> 
> -- 
> Best regards,
> Daniel

[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[Daniel Schwierzeck]

2012/11/26 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> On Sun, Nov 25, 2012 at 09:30:54PM +0100, Daniel Schwierzeck wrote:
>> 2012/11/24 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
>> > If bal is 8 bytes aligned, the _gp will not be 8 bytes aligned.
>> > then the following ld insntrustion generates a Adel exception.
>> > So here make _gp be always aligned in 8 bytes.
>>
>> which toolchain do you use? Actually _gp is aligned to 16 bytes in the
>> linker script.
>> Thus the instruction "ld gp, 0(ra)" and the address loaded into gp is
>> always aligned to 8 bytes.
>> This works at least with ELDK-5.2.1 and a self-built gcc-4.7.2.
> Thanks for review.
> I have do a simple test by modify this snip as following:
> .align 3
>   //nop
>   nop
>   bal     1f
>   nop
> .dword  _gp
> 1:
>   ld      gp, 0(ra)
> Qemu give me a Adel exception:
> (qemu) info registers
> pc=0xffffffffbfc00688 HI=0x0000000000000000 LO=0x0000000000000000 ds
> 0098 0000000000000000 0
> GPR00: r0 0000000000000000 at 000000001000009f v0 0000000000000000 v1
> 0000000000000000
> GPR04: a0 0000000000000000 a1 0000000000000000 a2 0000000000000000 a3
> 0000000000000000
> GPR08: t0 0000000000000000 t1 0000000000000000 t2 0000000000000000 t3
> 0000000000000000
> GPR12: t4 0000000000000002 t5 0000000000000000 t6 0000000000000000 t7
> 0000000000000000
> GPR16: s0 0000000000000000 s1 0000000000000000 s2 0000000000000000 s3
> 0000000000000000
> GPR20: s4 0000000000000000 s5 0000000000000000 s6 0000000000000000 s7
> 0000000000000000
> GPR24: t8 0000000000000000 t9 0000000000000000 k0 0000000000000000 k1
> 0000000000000000
> GPR28: gp 0000000000000000 sp 0000000000000000 s8 0000000000000000 ra
> ffffffffbfc00544
> CP0 Status  0x10400082 Cause   0x00000410 EPC    0xffffffffbfc00550
>     Config0 0x80004482 Config1 0xfea3519b LLAddr 0x0000000000000000
>
> We can also see the arrange with objdump:
> ffffffffbfc00538:       00000000        nop
> ffffffffbfc0053c:       04110004        bal     ffffffffbfc00550
> ffffffffbfc00540:       00000000        nop
> ffffffffbfc00544:       00000000        nop
> ffffffffbfc00548:       bfc36ed0        cache   0x3,28368(s8)
> ffffffffbfc0054c:       ffffffff        sd      ra,-1(ra)
> ffffffffbfc00550:       dffc0000        ld      gp,0(ra)
>
> It shows that although _gp is aligned to 8 bytes. but it's not in the
> address of 0(ra).

I did some experiments too and now I understand your problem. If you put
additional instructions before "bal 1f", the compiler puts a nop between bal
and _gp to fill the gap in order to keep _gp aligned. But then the link address
of bal that will be stored in the gp register points to the additional
nop and not
to _gp as it was intended. I can't reproduce this with MIPS32.
Maybe this is a compiler bug.

I pushed a modified version of your patch to
git://git.denx.de/u-boot-mips.git [1].
Could you check if this is okay for you?

[1] http://git.denx.de/?p=u-boot/u-boot-mips.git;a=commitdiff;h=4bb639edce852c8ed2ae3219e436845495270453

-- 
Best regards,
Daniel

[U-Boot] [Patch 1/2] MIPS: fix a latent bug on initialize $gp

[Zhi-zhou Zhang]

On Wed, Nov 28, 2012 at 02:33:10PM +0100, Daniel Schwierzeck wrote:
> 2012/11/26 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> > On Sun, Nov 25, 2012 at 09:30:54PM +0100, Daniel Schwierzeck wrote:
> >> 2012/11/24 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> >> > If bal is 8 bytes aligned, the _gp will not be 8 bytes aligned.
> >> > then the following ld insntrustion generates a Adel exception.
> >> > So here make _gp be always aligned in 8 bytes.
> >>
> >> which toolchain do you use? Actually _gp is aligned to 16 bytes in the
> >> linker script.
> >> Thus the instruction "ld gp, 0(ra)" and the address loaded into gp is
> >> always aligned to 8 bytes.
> >> This works at least with ELDK-5.2.1 and a self-built gcc-4.7.2.
> > Thanks for review.
> > I have do a simple test by modify this snip as following:
> > .align 3
> >   //nop
> >   nop
> >   bal     1f
> >   nop
> > .dword  _gp
> > 1:
> >   ld      gp, 0(ra)
> > Qemu give me a Adel exception:
> > (qemu) info registers
> > pc=0xffffffffbfc00688 HI=0x0000000000000000 LO=0x0000000000000000 ds
> > 0098 0000000000000000 0
> > GPR00: r0 0000000000000000 at 000000001000009f v0 0000000000000000 v1
> > 0000000000000000
> > GPR04: a0 0000000000000000 a1 0000000000000000 a2 0000000000000000 a3
> > 0000000000000000
> > GPR08: t0 0000000000000000 t1 0000000000000000 t2 0000000000000000 t3
> > 0000000000000000
> > GPR12: t4 0000000000000002 t5 0000000000000000 t6 0000000000000000 t7
> > 0000000000000000
> > GPR16: s0 0000000000000000 s1 0000000000000000 s2 0000000000000000 s3
> > 0000000000000000
> > GPR20: s4 0000000000000000 s5 0000000000000000 s6 0000000000000000 s7
> > 0000000000000000
> > GPR24: t8 0000000000000000 t9 0000000000000000 k0 0000000000000000 k1
> > 0000000000000000
> > GPR28: gp 0000000000000000 sp 0000000000000000 s8 0000000000000000 ra
> > ffffffffbfc00544
> > CP0 Status  0x10400082 Cause   0x00000410 EPC    0xffffffffbfc00550
> >     Config0 0x80004482 Config1 0xfea3519b LLAddr 0x0000000000000000
> >
> > We can also see the arrange with objdump:
> > ffffffffbfc00538:       00000000        nop
> > ffffffffbfc0053c:       04110004        bal     ffffffffbfc00550
> > ffffffffbfc00540:       00000000        nop
> > ffffffffbfc00544:       00000000        nop
> > ffffffffbfc00548:       bfc36ed0        cache   0x3,28368(s8)
> > ffffffffbfc0054c:       ffffffff        sd      ra,-1(ra)
> > ffffffffbfc00550:       dffc0000        ld      gp,0(ra)
> >
> > It shows that although _gp is aligned to 8 bytes. but it's not in the
> > address of 0(ra).
> 
> I did some experiments too and now I understand your problem. If you put
> additional instructions before "bal 1f", the compiler puts a nop between bal
> and _gp to fill the gap in order to keep _gp aligned. But then the link address
> of bal that will be stored in the gp register points to the additional
> nop and not
> to _gp as it was intended. I can't reproduce this with MIPS32.
> Maybe this is a compiler bug.
Dear Daniel, 
I'm sorry for my poor Engilsh. I didn't describe the problem exactly.
I think tlink script only set the value of _gp. and .dword do decide
where _gp be linked. Because the compiler always align dword to 8
bytes, the gap comes and was filled with zero.
The MIPS's ISA set 4 bytes per instruction. so MIPS's link address
is always aligned to 4 bytes. MIPS32 won't occur this problem. But
if the link address is not aligned to 8 bytes, MIPS64 goes wrong.
We have half chance encountering it.
Thanks.
> 
> I pushed a modified version of your patch to
> git://git.denx.de/u-boot-mips.git [1].
> Could you check if this is okay for you?
> 
> [1] http://git.denx.de/?p=u-boot/u-boot-mips.git;a=commitdiff;h=4bb639edce852c8ed2ae3219e436845495270453
> 
> -- 
> Best regards,
> Daniel

[U-Boot] [Patch 2/2] MIPS: do not modify variable before relocate_code

[Daniel Schwierzeck]

2012/11/24 Zhi-zhou Zhang <zhizhou.zh@gmail.com>:
> Because timestamp is declared as `static', we needn't initialize
> it by writing it a zero. If we do it before relocate_code, we
> will write into a flash address(0xffffffffbfc0xxxx).
>
> Signed-off-by: Zhi-zhou Zhang <zhizhou.zh@gmail.com>
> ---
>  arch/mips/cpu/mips32/time.c |    1 -
>  arch/mips/cpu/mips64/time.c |    1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/arch/mips/cpu/mips32/time.c b/arch/mips/cpu/mips32/time.c
> index 350896a..09fc842 100644
> --- a/arch/mips/cpu/mips32/time.c
> +++ b/arch/mips/cpu/mips32/time.c
> @@ -36,7 +36,6 @@ static unsigned long timestamp;
>  int timer_init(void)
>  {
>         /* Set up the timer for the first expiration. */
> -       timestamp = 0;
>         write_c0_compare(read_c0_count() + CYCLES_PER_JIFFY);
>
>         return 0;
> diff --git a/arch/mips/cpu/mips64/time.c b/arch/mips/cpu/mips64/time.c
> index 5154280..720f7b7 100644
> --- a/arch/mips/cpu/mips64/time.c
> +++ b/arch/mips/cpu/mips64/time.c
> @@ -37,7 +37,6 @@ static unsigned long timestamp;
>  int timer_init(void)
>  {
>         /* Set up the timer for the first expiration. */
> -       timestamp = 0;
>         write_c0_compare(read_c0_count() + CYCLES_PER_JIFFY);
>
>         return 0;
> --
> 1.7.9.5
>

applied to u-boot-mips/master with a tiny fixup of the commit summary, thanks

-- 
Best regards,
Daniel