armv7: Fix infinite loop for the spl boot

PATCH New Zhong Hongbo 3 participants 8 messages Jul 2, 2012 – Jul 6, 2012 Arm (32-bit)

lore

Share & tools

lore

Zhong Hongbo

author

2012-07-02 23:46 UTC #0 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

From: Zhong Hongbo <[email protected]>

In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.

Signed-off-by Hongbo Zhong <[email protected]>

---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

arch/arm/cpu/armv7/start.S +3 -0

index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs

@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/* clear loop...		    */
 	add	r0, r0, #4
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:
 
 /*
  * We are done. Do not return, instead branch to second part of board

Signature · 1 line

1.7.5.4

Zhong Hongbo

author

2012-07-05 11:53 UTC #1 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Hi Albert,

Could you applied the patch to the arm tree?

Thanks,
hongbo

Quoted 33 lines · Zhong Hongbo

From: Zhong Hongbo <[email protected]>
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by Hongbo Zhong <[email protected]>
---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/cpu/armv7/start.S +3 -0
index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/* clear loop...		    */
 	add	r0, r0, #4
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:
 
 /*
  * We are done. Do not return, instead branch to second part of board

Albert ARIBAUD

2012-07-05 12:19 UTC #2 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Hi Zhong Hongbo,

Quoted 28 lines · Zhong Hongbo

Hi Albert,
Could you applied the patch to the arm tree?
Thanks,
hongbo
Quoted 21 lines · Zhong Hongbo
From: Zhong Hongbo <[email protected]>
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by Hongbo Zhong <[email protected]>
---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/cpu/armv7/start.S +2 -0
index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss

Quoted 8 lines

arch/arm/cpu/armv7/start.S clbss_l:str r2, [r0] /* clear loop... */ · line 276
Quoted 8 lines
arch/arm/cpu/armv7/start.S +0 -0
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/*
clear loop... */ add r0, r0, #4
arch/arm/cpu/armv7/start.S +1 -0
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:

Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".

I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:

Quoted 4 lines

clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l

Into something like

Quoted 6 lines

clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l

clbss_d:

Thanks in advance.

Amicalement,

Signature · 1 line

Albert.

Zhong Hongbo

author

2012-07-05 12:34 UTC #3 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Hi Albert,

Quoted 36 lines · Albert ARIBAUD

Hi Zhong Hongbo,
Quoted 32 lines · Zhong Hongbo
Hi Albert,
Could you applied the patch to the arm tree?
Thanks,
hongbo
Quoted 25 lines · Zhong Hongbo
From: Zhong Hongbo <[email protected]>
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by Hongbo Zhong <[email protected]>
---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/cpu/armv7/start.S +2 -0
index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/*

Quoted 8 lines

Quoted 4 lines
arch/arm/cpu/armv7/start.S clbss_l:str r2, [r0] /* clear loop... */ · line 276
Quoted 4 lines
clear loop... */ add r0, r0, #4
arch/arm/cpu/armv7/start.S +1 -0
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".

Ok, Good to known, I will send V2

Quoted 3 lines

I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:

Ok,

Thanks,
hongbo

Quoted 19 lines

Quoted 5 lines

clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l

Into something like

Quoted 6 lines

clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l

clbss_d:

Thanks in advance.

Amicalement,

Albert ARIBAUD (by way of Albert ARIBAUD <[email protected]>)

2012-07-05 17:35 UTC #4 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

In reply to Zhong Hongbo

Hi Zhong Hongbo,

Quoted 28 lines · Zhong Hongbo

Hi Albert,
Could you applied the patch to the arm tree?
Thanks,
hongbo
Quoted 21 lines · Zhong Hongbo
From: Zhong Hongbo <[email protected]>
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by Hongbo Zhong <[email protected]>
---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/cpu/armv7/start.S +2 -0
index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss

Quoted 8 lines

arch/arm/cpu/armv7/start.S clbss_l:str r2, [r0] /* clear loop... */ · line 276
Quoted 8 lines
arch/arm/cpu/armv7/start.S +0 -0
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/*
clear loop... */ add r0, r0, #4
arch/arm/cpu/armv7/start.S +1 -0
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:

Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".

I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:

Quoted 4 lines

clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l

Into something like

Quoted 6 lines

clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l

clbss_d:

Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.

Thanks in advance.

Amicalement,

Signature · 1 line

Albert.

Zhong Hongbo

author

2012-07-06 11:42 UTC #5 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Quoted 36 lines · On 07/06/2012 01:35 AM, Albert ARIBAUD <[email protected]> (by way of…

Hi Zhong Hongbo,
Quoted 32 lines · Zhong Hongbo
Hi Albert,
Could you applied the patch to the arm tree?
Thanks,
hongbo
Quoted 25 lines · Zhong Hongbo
From: Zhong Hongbo <[email protected]>
In the spl booting step, When __bss_start is equal to __bss_end__, The loop will clear all the things in CPU space. If there are have the same address for this symbol, To skip the clear bss section.
Signed-off-by Hongbo Zhong <[email protected]>
---
 arch/arm/cpu/armv7/start.S |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/arm/cpu/armv7/start.S +2 -0
index 76ccef1..c72f337 100644
@@ -258,6 +258,8 @@ clear_bss:
 	/* No relocation for SPL */
 	ldr	r0, =__bss_start
 	ldr	r1, =__bss_end__
+	cmp	r0, r1
+	beq	skip_clbss
 #else
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
@@ -271,6 +273,7 @@ clbss_l:str	r2, [r0]		/*

Quoted 9 lines

Quoted 4 lines
arch/arm/cpu/armv7/start.S
Quoted 4 lines
clear loop... */ add r0, r0, #4
arch/arm/cpu/armv7/start.S +1 -0
 	cmp	r0, r1
 	bne	clbss_l
+skip_clbss:
Clearly the loop was wrong in that it should implement a "for (r0 = start; r0 < end; r0++)" but actually implements a "for (r0 = start; r0 != end; r0++)".

Yes? My new patch have do this.

Quoted 3 lines

I'd rather the loop be fixed to match the intended implementation rather than worked around. Please rewrite your patch to turn:

Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:

commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d

Author: Christian Riesch <[email protected]>
Date:   Wed Nov 30 22:27:37 2011 +0000

arm, arm926ejs: Fix clear bss loop for zero length bss

This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__.

Signed-off-by Christian Riesch <[email protected]>

Cc Albert Aribaud <[email protected]>

arch/arm/cpu/arm926ejs/start.S +5 -3

index 339c5ed..bb4d00b 100644
@@ -301,10 +301,12 @@ clear_bss:
 #endif
        mov     r2, #0x00000000         /* clear
    */
 
-clbss_l:str    r2, [r0]                /* clear loop...
    */
+clbss_l:cmp    r0, r1                  /* clear loop... */
+       bhs     clbss_e                 /* if reached end of bss, exit */
+       str     r2, [r0]
        add     r0, r0, #4
-       cmp     r0, r1
-       bne     clbss_l
+       b       clbss_l
+clbss_e:

Quoted 7 lines

Quoted 4 lines

clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l

Into something like

Quoted 8 lines

Quoted 6 lines
clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l
clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.

Ok?

Thanks,
hongbo

Quoted 5 lines

Thanks in advance.
Amicalement,

Andreas Bießmann

2012-07-06 11:49 UTC #6 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Dear Zhong Hongbo,

Quoted 6 lines · Zhong Hongbo

Quoted 4 lines · On 07/06/2012 01:35 AM, Albert ARIBAUD <[email protected]> (by way of…
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo <[email protected]> wrote:

<snip>

Quoted 49 lines

Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:
commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d
Author: Christian Riesch <[email protected]>
Date:   Wed Nov 30 22:27:37 2011 +0000
arm, arm926ejs: Fix clear bss loop for zero length bss
This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__.
Signed-off-by Christian Riesch <[email protected]>
Cc Albert Aribaud <[email protected]>
arch/arm/cpu/arm926ejs/start.S +5 -3
index 339c5ed..bb4d00b 100644
@@ -301,10 +301,12 @@ clear_bss:
 #endif
        mov     r2, #0x00000000         /* clear
    */
 
-clbss_l:str    r2, [r0]                /* clear loop...
    */
+clbss_l:cmp    r0, r1                  /* clear loop... */
+       bhs     clbss_e                 /* if reached end of bss, exit */
+       str     r2, [r0]
        add     r0, r0, #4
-       cmp     r0, r1
-       bne     clbss_l
+       b       clbss_l
+clbss_e:
Quoted 14 lines
Quoted 4 lines
clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l
Into something like
Quoted 6 lines
clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l
clbss_d:

Quoted 3 lines

Quoted 1 line
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.
Ok?

I think this is the thread we will proceed.
So as I understand we had missed in 2011 to update the other start.S with a fix found by Christian Riesch for arm926ejs. So please do update _all_ start.S (of arm;) now. I think they do have all the same error cause we copied it over from one to the other when implementing the 'new relocation' stuff.

Best regards

Andreas Bie?mann

Zhong Hongbo

author

2012-07-06 11:53 UTC #7 lore raw

PATCH 1/1 armv7: Fix infinite loop for the spl boot

Quoted 63 lines · Andreas Bie?mann

Dear Zhong Hongbo,
Quoted 6 lines · Zhong Hongbo
Quoted 4 lines · On 07/06/2012 01:35 AM, Albert ARIBAUD <[email protected]> (by way of…
Hi Zhong Hongbo,
On Thu, 05 Jul 2012 19:53:46 +0800, Zhong Hongbo <[email protected]> wrote:
<snip>
Quoted 51 lines
Ok, I just found the issue have found in other arm platfor 2011 yeas, the detail information as following:
commit 8f1da53508c78789ebeea98a92a3f55c3f84dc5d
Author: Christian Riesch <[email protected]>
Date:   Wed Nov 30 22:27:37 2011 +0000
arm, arm926ejs: Fix clear bss loop for zero length bss
This patch fixes the clear bss loop for bss sections that have zero length, i.e., where __bss_start == __bss_end__.
Signed-off-by Christian Riesch <[email protected]>
Cc Albert Aribaud <[email protected]>
arch/arm/cpu/arm926ejs/start.S +5 -3
index 339c5ed..bb4d00b 100644
@@ -301,10 +301,12 @@ clear_bss:
 #endif
        mov     r2, #0x00000000         /* clear
    */
 
-clbss_l:str    r2, [r0]                /* clear loop...
    */
+clbss_l:cmp    r0, r1                  /* clear loop... */
+       bhs     clbss_e                 /* if reached end of bss, exit */
+       str     r2, [r0]
        add     r0, r0, #4
-       cmp     r0, r1
-       bne     clbss_l
+       b       clbss_l
+clbss_e:
Quoted 15 lines
Quoted 4 lines
clbss_l:str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        cmp     r0, r1
        bne     clbss_l
Into something like
Quoted 6 lines
clbss_l:cmp     r0, r1
        blo     clbss_d
        str     r2, [r0]  /* clear loop...*/
        add     r0, r0, #4
        b       clbss_l
clbss_d:
Also, as Andreas points out, make sure the same fix is applied to all ARM start.S files which need it.

Quoted 8 lines

Quoted 1 line
Ok?
I think this is the thread we will proceed.
So as I understand we had missed in 2011 to update the other start.S with a fix found by Christian Riesch for arm926ejs. So please do update _all_ start.S (of arm;) now. I think they do have all the same error cause we copied it over from one to the other when implementing the 'new relocation' stuff.

Agree, I will do it now.

Thanks,
hongbo

Quoted 6 lines

Best regards
Andreas Bie?mann