arm: use common instructions applicable to armv7m & other arm archs

PATCH New Vikas Manocha 3 participants 6 messages Jan 19, 2016 – Feb 1, 2016 Arm (32-bit)

Share & tools

Vikas Manocha

author

2016-01-19 02:52 UTC #0 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.

Signed-off-by Vikas Manocha <[email protected]>

---
 arch/arm/lib/crt0.S | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

arch/arm/lib/crt0.S +7 ~~-18~~

index 2f4c14e..3731d0c 100644
@@ -71,18 +71,12 @@ ENTRY(_main)
  */
 
 #if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
-	ldr	sp, =(CONFIG_SPL_STACK)
+	ldr	r0, =(CONFIG_SPL_STACK)
 #else
-	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
+	ldr	r0, =(CONFIG_SYS_INIT_SP_ADDR)
 #endif
-#if defined(CONFIG_CPU_V7M)	/* v7M forbids using SP as BIC destination */
-	mov	r3, sp
-	bic	r3, r3, #7
-	mov	sp, r3
-#else
-	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */
-#endif
-	mov	r0, sp
+	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
+	mov	sp, r0
 	bl	board_init_f_alloc_reserve
 	mov	sp, r0
 	/* set up gd here, outside any C code */

@@ -100,14 +94,9 @@ ENTRY(_main)
  * 'here' but relocated.
  */
 
-	ldr	sp, [r9, #GD_START_ADDR_SP]	/* sp = gd->start_addr_sp */
-#if defined(CONFIG_CPU_V7M)	/* v7M forbids using SP as BIC destination */
-	mov	r3, sp
-	bic	r3, r3, #7
-	mov	sp, r3
-#else
-	bic	sp, sp, #7	/* 8-byte alignment for ABI compliance */
-#endif
+	ldr	r0, [r9, #GD_START_ADDR_SP]	/* sp = gd->start_addr_sp */
+	bic	r0, r0, #7
+	mov	sp, r0		/* 8-byte alignment for ABI compliance */
 	ldr	r9, [r9, #GD_BD]		/* r9 = gd->bd */
 	sub	r9, r9, #GD_SIZE		/* new GD is below bd */

Signature · 1 line

1.9.1

Simon Glass

2016-01-19 03:11 UTC #1 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

Quoted 8 lines · Vikas Manocha

BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.
Signed-off-by Vikas Manocha <[email protected]>
---
 arch/arm/lib/crt0.S | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

Reviewed-by Simon Glass <[email protected]>

(not tested :-)

Albert ARIBAUD

2016-01-29 17:15 UTC #2 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

In reply to Vikas Manocha

Hello Vikas,

Quoted 3 lines · Vikas Manocha

BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.

I am not against this patch, which has merits on its own; but the commit message above raises a couple of questions.

1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is incorrectly being used on sp. However, it is *not*; it is used on r3, precisely because it cannot be used directly on sp, as the comment before the bic instruction clearly states. Why does the commit message raise this non-issue?

2. I could not find any information on BIC being deprecated in Architecture Reference Manuals of either ArmV7 (section "Deprecated Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete Features"). Where does this information come from?

Amicalement,

Signature · 1 line

Albert.

Vikas MANOCHA

author

2016-01-29 23:36 UTC #3 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

Hi Albert,

Quoted 17 lines

-----Original Message-----
From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: Friday, January 29, 2016 9:16 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw Marczak
Subject: Re: [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas,
Quoted 3 lines · Vikas Manocha
BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.

Quoted 8 lines

I am not against this patch, which has merits on its own; but the commit message above raises a couple of questions.
1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is incorrectly being used on sp. However, it is *not*; it is used on r3, precisely because it cannot be used directly on sp, as the comment before the bic instruction clearly states. Why does the commit message raise this non-issue?

For sure, BIC is used correctly. Currently BIC instruction is being used on r3 for armv7m & on SP for others.
The patch is to use same instruction for all ISAs & to make code cleaner.
The intention of message " BIC instruction to clear the SP is not allowed in armv7m" is to justify the usage of register (r0) instead of SP for BIC. Sorry if it created some confusion, how about following commit message for v2:

"This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs. Just for information, BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above"

Quoted 5 lines

2. I could not find any information on BIC being deprecated in Architecture Reference Manuals of either ArmV7 (section "Deprecated Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete Features"). Where does this information come from?

I got it from some of the web locations, one of them is:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/dom1361289864906.html

Cheers,
Vikas

Quoted 4 lines

Amicalement,
--
Albert.

Albert ARIBAUD

2016-01-31 15:00 UTC #4 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

Hello Vikas,

Quoted 28 lines · Vikas MANOCHA

Hi Albert,
Quoted 17 lines
-----Original Message-----
From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: Friday, January 29, 2016 9:16 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw Marczak
Subject: Re: [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas,
Quoted 3 lines · Vikas Manocha
BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.
Quoted 8 lines
I am not against this patch, which has merits on its own; but the commit message above raises a couple of questions.
1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is incorrectly being used on sp. However, it is *not*; it is used on r3, precisely because it cannot be used directly on sp, as the comment before the bic instruction clearly states. Why does the commit message raise this non-issue?

Quoted 7 lines

For sure, BIC is used correctly. Currently BIC instruction is being used on r3 for armv7m & on SP for others.
The patch is to use same instruction for all ISAs & to make code cleaner.
The intention of message " BIC instruction to clear the SP is not allowed in armv7m" is to justify the usage of register (r0) instead of SP for BIC. Sorry if it created some confusion, how about following commit message for v2:
"This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs. Just for information, BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above"

Almost good, but... the current code /already/ uses instructions allowed for ARMv7-M. It even has a conditional on CONFIG_CPU_V7M under which all instructions are ARMv7-compatible. Proof is, ARMv7 actually builds (and runs fine).

Granted, if someone built for ARMv7-M but with CONFIG_CPU_V7M unset, then the assembler would fail on the "BIC sp" instruction, which is indeed forbidden in ARMv7-M; but such a build cannot happen unless that same someone actively *circumvented* the U-Boot build process.

What your patch does is therefore definitely not "Fix bad use of BIC which breaks ARMv7", and not even "fix use of BIC that might cause ARMv7 to break". Your patch is, however, "Streamline code and remove ARMv7-M conditional by using only ARMv7-M-allowed instructions" -- and this it does pretty well.

I would therefore completely drop the part about BIC from the commit message as it is not actually a problem in the current code -- or more to the point, as its limitations were already acknowledged and accounted for in the current code.

However, if the bit about BIC really had to stay (with will require a very strong reason), it should be corrected, because of the following:

Quoted 8 lines

Quoted 5 lines
2. I could not find any information on BIC being deprecated in Architecture Reference Manuals of either ArmV7 (section "Deprecated Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete Features"). Where does this information come from?
I got it from some of the web locations, one of them is:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/dom1361289864906.html

Firstly, the link above is *not* to the ARMv6-M Architecture Reference Manual, but from the documentation of an ARM tool, namely the ARM DS-5 compiler (version 5.04). The only authoritative source to determine if a feature is deprecated in a given ARM architecture is its Architecture Reference Manual.

But even of the sentence above was authoritative, what it purports as deprecated is the use of PC and SP in BIC, not the use of BIC itself, whereas the proposed commit message might be read by a casual (or non-casual but not quite attentive enough) reader might as "the BIC instruction to clear the SP is not allowed in armv7m & the BIC instruction is deprecated in ARMv6T2 & above".

At the very least, that part of the commit message should be reworded as "Using BIC on SP (as well as PC) is deprecated in ARMv6T2 and above, and forbidden in ARMv7-M".

However, my opinion is that the commit is not about fixing how BIC is used but about getting rid of the ARMv7-M conditional, and therefore, I think that its message should not mention BIC at all.

I would therefore limit the commit message to just this:

This patch cleans the code by using instructions allowed for ARMv7-M as well as other Arm architectures.

Quoted 2 lines

Cheers,
Vikas

Amicalement,

Signature · 1 line

Albert.

Vikas MANOCHA

author

2016-02-01 23:48 UTC #5 lore raw

PATCH 1/1 arm: use common instructions applicable to armv7m & other arm archs

Thanks Albert,

Quoted 4 lines

I would therefore limit the commit message to just this:
This patch cleans the code by using instructions allowed for ARMv7-M as well as other Arm architectures.

I will update the commit message accordingly & send v2.

Rgds,
Vikas

Quoted 121 lines

-----Original Message-----
From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: Sunday, January 31, 2016 7:01 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw Marczak
Subject: Re: [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas,
Quoted 22 lines · Vikas MANOCHA
Hi Albert,
Quoted 20 lines
-----Original Message-----
From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: Friday, January 29, 2016 9:16 AM
To: Vikas MANOCHA
Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw Marczak
Subject: Re: [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas,
Quoted 3 lines · Vikas Manocha
BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above. This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.
I am not against this patch, which has merits on its own; but the commit message above raises a couple of questions.
Quoted 7 lines
Quoted 5 lines
1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is incorrectly being used on sp. However, it is *not*; it is used on r3, precisely because it cannot be used directly on sp, as the comment before the bic instruction clearly states. Why does the commit message raise this non-issue?
For sure, BIC is used correctly. Currently BIC instruction is being used on r3
for armv7m & on SP for others.
Quoted 2 lines
The patch is to use same instruction for all ISAs & to make code cleaner. The intention of message " BIC instruction to clear the SP is not allowed in
armv7m" is to justify the usage of register (r0) instead of SP for BIC.
Quoted 1 line
Sorry if it created some confusion, how about following commit message
for v2:
Quoted 2 lines
"This patch cleans the code by using instructions allowed for armv7m as
well as other Arm archs.
Quoted 1 line
Just for information, BIC instruction to clear the SP is not allowed in armv7m
& is deprecated in ARMv6T2 & above"
Almost good, but... the current code /already/ uses instructions allowed for ARMv7-M. It even has a conditional on CONFIG_CPU_V7M under which all instructions are ARMv7-compatible. Proof is, ARMv7 actually builds (and runs fine).
Granted, if someone built for ARMv7-M but with CONFIG_CPU_V7M unset, then the assembler would fail on the "BIC sp" instruction, which is indeed forbidden in ARMv7-M; but such a build cannot happen unless that same someone actively *circumvented* the U-Boot build process.
What your patch does is therefore definitely not "Fix bad use of BIC which breaks ARMv7", and not even "fix use of BIC that might cause ARMv7 to break". Your patch is, however, "Streamline code and remove ARMv7-M conditional by using only ARMv7-M-allowed instructions" -- and this it does pretty well.
I would therefore completely drop the part about BIC from the commit message as it is not actually a problem in the current code -- or more to the point, as its limitations were already acknowledged and accounted for in the current code.
However, if the bit about BIC really had to stay (with will require a very strong reason), it should be corrected, because of the following:
Quoted 4 lines
Quoted 4 lines
2. I could not find any information on BIC being deprecated in Architecture Reference Manuals of either ArmV7 (section "Deprecated Features in ARMv7-M", or ARMv6-M (section "Deprecated and
Obsolete
Quoted 5 lines
Quoted 1 line
Features"). Where does this information come from?
I got it from some of the web locations, one of them is:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/d
om1361289864906.html
Firstly, the link above is *not* to the ARMv6-M Architecture Reference Manual, but from the documentation of an ARM tool, namely the ARM DS-5 compiler (version 5.04). The only authoritative source to determine if a feature is deprecated in a given ARM architecture is its Architecture Reference Manual.
But even of the sentence above was authoritative, what it purports as deprecated is the use of PC and SP in BIC, not the use of BIC itself, whereas the proposed commit message might be read by a casual (or non-casual but not quite attentive enough) reader might as "the BIC instruction to clear the SP is not allowed in armv7m & the BIC instruction is deprecated in ARMv6T2 & above".
At the very least, that part of the commit message should be reworded as "Using BIC on SP (as well as PC) is deprecated in ARMv6T2 and above, and forbidden in ARMv7-M".
However, my opinion is that the commit is not about fixing how BIC is used but about getting rid of the ARMv7-M conditional, and therefore, I think that its message should not mention BIC at all.
I would therefore limit the commit message to just this:
This patch cleans the code by using instructions allowed for ARMv7-M as well as other Arm architectures.
Quoted 2 lines
Cheers,
Vikas
Amicalement,
--
Albert.