ARM: compressed: clean up section layout and enable EFI debugging

Instead of relying on unspecified linker behavior, move the SharpSL startup code into a subroutine and call it from the location we expect the linker to put the code.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/boot/compressed/head-sharpsl.S | 24 ++++++++++----------
 arch/arm/boot/compressed/head.S         |  3 +++
 2 files changed, 15 insertions(+), 12 deletions(-)

arch/arm/boot/compressed/head-sharpsl.S +12 -12

index 992e784500fa..f4e6ad318376 100644
@@ -20,9 +20,10 @@
 #error What am I doing here...
 #endif
 
-		.section        ".start", "ax"
+	.text
 
-__SharpSL_start:
+ENTRY(__SharpSL_start)
+	mov	ip, lr			@ preserve lr
 
 /* Check for TC6393 - if found we have a Tosa */
 	ldr	r7, .TOSAID

@@ -30,7 +31,7 @@ __SharpSL_start:
 	mov 	r6, #0x03
 	ldrh	r3, [r1, #8]		@ Load TC6393XB Revison: This is 0x0003
 	cmp	r6, r3
-	beq	.SHARPEND		@ Success -> tosa
+	moveq	pc, ip			@ Success -> tosa
 
 /* Check for pxa270 - if found, branch */
 	mrc p15, 0, r4, c0, c0		@ Get Processor ID

@@ -55,30 +56,30 @@ __SharpSL_start:
 	ldr	r3, .W100ID
 	ldr	r7, .POODLEID
 	cmp	r6, r3
-	bne	.SHARPEND			@ We have no w100 - Poodle
+	movne	pc, ip				@ We have no w100 - Poodle
 
 /* Check for pxa250 - if found we have a Corgi */
 	ldr	r7, .CORGIID
 	ldr	r3, .PXA255ID
 	cmp	r4, r3
-	blo	.SHARPEND			@ We have a PXA250 - Corgi
+	movlo	pc, ip				@ We have a PXA250 - Corgi
 
 /* Check for 64MiB flash - if found we have a Shepherd */
 	bl	get_flash_ids
 	ldr	r7, .SHEPHERDID
 	cmp	r3, #0x76			@ 64MiB flash
-	beq	.SHARPEND			@ We have Shepherd
+	moveq	pc, ip				@ We have Shepherd
 
 /* Must be a Husky */
 	ldr	r7, .HUSKYID		@ Must be Husky
-	b .SHARPEND
+	mov	pc, ip			@ return
 
 .PXA270:
 /* Check for 16MiB flash - if found we have Spitz */
 	bl	get_flash_ids
 	ldr	r7, .SPITZID
 	cmp	r3, #0x73			@ 16MiB flash
-	beq	.SHARPEND			@ We have Spitz
+	moveq	pc, ip				@ We have Spitz
 
 /* Check for a second SCOOP chip - if found we have Borzoi */
 	ldr	r1, .SCOOP2ADDR

@@ -87,11 +88,12 @@ __SharpSL_start:
 	strh	r6, [r1]
 	ldrh	r6, [r1]
 	cmp	r6, #0x0140
-	beq	.SHARPEND			@ We have Borzoi
+	moveq	pc, ip				@ We have Borzoi
 
 /* Must be Akita */
 	ldr	r7, .AKITAID
-	b	.SHARPEND			@ We have Borzoi
+	mov	pc, ip				@ We have Borzoi
+ENDPROC(__SharpSL_start)
 
 .PXA255ID:
 	.word	0x69052d00		@ PXA255 Processor ID

@@ -147,5 +149,3 @@ get_flash_ids:
 	ldrb	r2, [r1, #20]		@ NAND Manufacturer ID
 	ldrb	r3, [r1, #20]		@ NAND Chip ID
 	mov	pc, lr
-
-.SHARPEND:

arch/arm/boot/compressed/head.S +3 -0

index 6c7ccb428c07..5067f287fa5a 100644
@@ -213,6 +213,9 @@ not_angel:
 		 */
 
 		.text
+#ifdef CONFIG_PXA_SHARPSL_DETECT_MACH_ID
+		bl	__SharpSL_start
+#endif
 
 #ifdef CONFIG_AUTO_ZRELADDR
 		/*

Signature · 1 line

2.19.1

Quoted 3 lines · Ard Biesheuvel

Instead of relying on unspecified linker behavior, move the SharpSL startup code into a subroutine and call it from the location we expect the linker to put the code.

Why is it "unspecified linker behaviour" ? Lots of libraries and programs rely on the linker building tables, whether it be data or code from sections.

For example, programs build a section called .init which starts with the code in crti.o, code from other files, and finishes with code from crtn.o.

crti.o:

Disassembly of section .init:

00000000 <_init>:

   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <_init>

4: R_ARM_CALL call_weak_fn

crtn.o:

Disassembly of section .init:

00000000 <.init>:

   0:   e8bd8008        pop     {r3, pc}

So, I don't think there's anything "unspecified" here.

We also rely heavily on this in the kernel to build exception tables, symbol tables, and so forth.

Quoted 105 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/boot/compressed/head-sharpsl.S | 24 ++++++++++----------
 arch/arm/boot/compressed/head.S         |  3 +++
 2 files changed, 15 insertions(+), 12 deletions(-)

arch/arm/boot/compressed/head-sharpsl.S +12 -12

index 992e784500fa..f4e6ad318376 100644
@@ -20,9 +20,10 @@
 #error What am I doing here...
 #endif
 
-             .section        ".start", "ax"
+     .text
 
-__SharpSL_start:
+ENTRY(__SharpSL_start)
+     mov     ip, lr                  @ preserve lr
 
 /* Check for TC6393 - if found we have a Tosa */
      ldr     r7, .TOSAID

@@ -30,7 +31,7 @@ __SharpSL_start:
      mov     r6, #0x03
      ldrh    r3, [r1, #8]            @ Load TC6393XB Revison: This is 0x0003
      cmp     r6, r3
-     beq     .SHARPEND               @ Success -> tosa
+     moveq   pc, ip                  @ Success -> tosa
 
 /* Check for pxa270 - if found, branch */
      mrc p15, 0, r4, c0, c0          @ Get Processor ID

@@ -55,30 +56,30 @@ __SharpSL_start:
      ldr     r3, .W100ID
      ldr     r7, .POODLEID
      cmp     r6, r3
-     bne     .SHARPEND                       @ We have no w100 - Poodle
+     movne   pc, ip                          @ We have no w100 - Poodle
 
 /* Check for pxa250 - if found we have a Corgi */
      ldr     r7, .CORGIID
      ldr     r3, .PXA255ID
      cmp     r4, r3
-     blo     .SHARPEND                       @ We have a PXA250 - Corgi
+     movlo   pc, ip                          @ We have a PXA250 - Corgi
 
 /* Check for 64MiB flash - if found we have a Shepherd */
      bl      get_flash_ids
      ldr     r7, .SHEPHERDID
      cmp     r3, #0x76                       @ 64MiB flash
-     beq     .SHARPEND                       @ We have Shepherd
+     moveq   pc, ip                          @ We have Shepherd
 
 /* Must be a Husky */
      ldr     r7, .HUSKYID            @ Must be Husky
-     b .SHARPEND
+     mov     pc, ip                  @ return
 
 .PXA270:
 /* Check for 16MiB flash - if found we have Spitz */
      bl      get_flash_ids
      ldr     r7, .SPITZID
      cmp     r3, #0x73                       @ 16MiB flash
-     beq     .SHARPEND                       @ We have Spitz
+     moveq   pc, ip                          @ We have Spitz
 
 /* Check for a second SCOOP chip - if found we have Borzoi */
      ldr     r1, .SCOOP2ADDR

@@ -87,11 +88,12 @@ __SharpSL_start:
      strh    r6, [r1]
      ldrh    r6, [r1]
      cmp     r6, #0x0140
-     beq     .SHARPEND                       @ We have Borzoi
+     moveq   pc, ip                          @ We have Borzoi
 
 /* Must be Akita */
      ldr     r7, .AKITAID
-     b       .SHARPEND                       @ We have Borzoi
+     mov     pc, ip                          @ We have Borzoi
+ENDPROC(__SharpSL_start)
 
 .PXA255ID:
      .word   0x69052d00              @ PXA255 Processor ID

@@ -147,5 +149,3 @@ get_flash_ids:
      ldrb    r2, [r1, #20]           @ NAND Manufacturer ID
      ldrb    r3, [r1, #20]           @ NAND Chip ID
      mov     pc, lr
-
-.SHARPEND:

arch/arm/boot/compressed/head.S +3 -1

index 6c7ccb428c07..5067f287fa5a 100644
@@ -213,6 +213,9 @@ not_angel:
               */
 
              .text
+#ifdef CONFIG_PXA_SHARPSL_DETECT_MACH_ID
+             bl      __SharpSL_start
+#endif
 
 #ifdef CONFIG_AUTO_ZRELADDR
              /*
--

Quoted 2 lines

2.19.1

--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up

Quoted 22 lines · Russell King - ARM Linux

Quoted 3 lines · Ard Biesheuvel

Instead of relying on unspecified linker behavior, move the SharpSL startup code into a subroutine and call it from the location we expect the linker to put the code.

Why is it "unspecified linker behaviour" ? Lots of libraries and programs rely on the linker building tables, whether it be data or code from sections.

For example, programs build a section called .init which starts with the code in crti.o, code from other files, and finishes with code from crtn.o.

crti.o:

Disassembly of section .init:

00000000 <_init>:

   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <_init>

4: R_ARM_CALL call_weak_fn

Quoted 9 lines

crtn.o:

Disassembly of section .init:

00000000 <.init>:

   0:   e8bd8008        pop     {r3, pc}

So, I don't think there's anything "unspecified" here.

In that case, we should do what GCC's builtin linker script uses:

  .init           :
  {
    KEEP (*(SORT_NONE(.init)))
  } =0

Quoted 3 lines

We also rely heavily on this in the kernel to build exception tables, symbol tables, and so forth.

No, not really. In that case, they are arrays of data structures, not sequences of instructions, and if they need to appear in a particular order, they are sorted explicitly.

Instead of relying on unspecified linker behavior, move the SharpSL startup code into a subroutine and call it from the location we expect the linker to put the code.

Signed-off-by Ard Biesheuvel <[email protected]>

---

Quoted 6 lines · Ard Biesheuvel

Instead of relying on unspecified linker behavior, move the SharpSL startup code into a subroutine and call it from the location we expect the linker to put the code.

Signed-off-by Ard Biesheuvel <[email protected]>

---

[...]

arch/arm/boot/compressed/head.S not_angel: · line 218

Quoted 9 lines

arch/arm/boot/compressed/head.S +3 -0

@@ -213,6 +213,9 @@ not_angel:
               */
 
              .text
+#ifdef CONFIG_PXA_SHARPSL_DETECT_MACH_ID
+             bl      __SharpSL_start
+#endif

Independently from Russell's points, I don't like the above. that will turn into an ugly list of #ifdefs and machine specific calls. I think you should use:

bl __early_arch_specific_start

and somewhere else in head.S:

WEAK(__early_arch_specific_start)
mov pc, lr
ENDPROC(__early_arch_specific_start)

That would make this change more palatable.

Instead of relying on unspecified linker behavior, move the SA1100 startup code into a subroutine and call it from the location we expect the linker to put the code.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/boot/compressed/head-sa1100.S | 9 +++++----
 arch/arm/boot/compressed/head.S        | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

arch/arm/boot/compressed/head-sa1100.S +5 -4

index 95abdd850fe3..89a7e9dfd5c0 100644
@@ -4,18 +4,17 @@
  * 
  * Copyright (C) 1999 Nicolas Pitre <[email protected]>
  * 
- * SA1100 specific tweaks.  This is merged into head.S by the linker.
+ * SA1100 specific tweaks.
  *
  */
 
 #include <linux/linkage.h>
 #include <asm/mach-types.h>
 
-		.section        ".start", "ax"
+		.text
 		.arch	armv4
 
-__SA1100_start:
-
+ENTRY(__SA1100_start)
 		@ Preserve r8/r7 i.e. kernel entry values
 #ifdef CONFIG_SA1100_COLLIE
 		mov	r7, #MACH_TYPE_COLLIE

@@ -47,3 +46,5 @@ __SA1100_start:
 		bic	r0, r0, #0x1000		@ clear Icache
 		mcr	p15, 0, r0, c1, c0, 0
 99:
+		mov	pc, lr
+ENDPROC(__SA1100_start)

arch/arm/boot/compressed/head.S +3 -0

index 5067f287fa5a..c5355f929cee 100644
@@ -216,6 +216,9 @@ not_angel:
 #ifdef CONFIG_PXA_SHARPSL_DETECT_MACH_ID
 		bl	__SharpSL_start
 #endif
+#ifdef CONFIG_ARCH_SA1100
+		bl	__SA1100_start
+#endif
 
 #ifdef CONFIG_AUTO_ZRELADDR
 		/*

Signature · 1 line

2.19.1

Instead of relying on unspecified linker behavior, move the XScale startup code into a subroutine and call it from the location we expect the linker to put the code.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/boot/compressed/head-xscale.S | 7 ++++---
 arch/arm/boot/compressed/head.S        | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

arch/arm/boot/compressed/head-xscale.S +4 -3

index 20fa44d59f82..89e14a55ec3d 100644
@@ -8,10 +8,9 @@
 
 #include <linux/linkage.h>
 
-		.section        ".start", "ax"
-
-__XScale_start:
+		.text
 
+ENTRY(__XScale_start)
 		@ Preserve r8/r7 i.e. kernel entry values
 
 		@ Data cache might be active.

@@ -33,3 +32,5 @@ __XScale_start:
 		bic	r0, r0, #0x1000		@ clear Icache
 		mcr	p15, 0, r0, c1, c0, 0
 
+		mov	pc, lr
+ENDPROC(__XScale_start)

arch/arm/boot/compressed/head.S +3 -0

index c5355f929cee..da93f419d1f4 100644
@@ -219,6 +219,9 @@ not_angel:
 #ifdef CONFIG_ARCH_SA1100
 		bl	__SA1100_start
 #endif
+#ifdef CONFIG_CPU_XSCALE
+		bl	__XScale_start
+#endif
 
 #ifdef CONFIG_AUTO_ZRELADDR
 		/*

Signature · 7 lines

2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Currently, the decompressor section layout is somewhat unusual: head.S puts code into a .start and a .text section, which are expected to be emitted back to back, unless other objects are included that define a .start section as well, in which case execution is expected to proceed linearly from head.S's section .start section into the other .start section and then onward into head.S's .text section.

This relies on the input files to be consumed by the linker in the same order they were specified on the command line, but this is not actually guaranteed by the linker, so it is better to make the inclusion of this code specific by using function calls.

Quoted 11 lines · Ard Biesheuvel

Currently, the decompressor section layout is somewhat unusual: head.S puts code into a .start and a .text section, which are expected to be emitted back to back, unless other objects are included that define a .start section as well, in which case execution is expected to proceed linearly from head.S's section .start section into the other .start section and then onward into head.S's .text section.

This relies on the input files to be consumed by the linker in the same order they were specified on the command line, but this is not actually guaranteed by the linker, so it is better to make the inclusion of this code specific by using function calls.

I think you're wrong about that - as I've pointed out, this behaviour is relied upon quite heavily when linking programs using GCC, so while it may not be explicitly specified, the fact that GCC relies upon it when creating shared libraries (using crti.o before any object files, and crtn.o at the end) means that it's pretty much guaranteed.

What is less guaranteed is proceeding between two sections, but these separate sections in object files are combined into one .text section in the output file, and so we know that code in .start will be present _before_ any code in .text.

Put the two things together, and the code from head.S will be listed first, and therefore will be output first in the .start and .text sections. Other object files will have code placed into the output .text section accordingly.

So really I see no point to your patch 1 to 5, other than change for change's sake.

Can you _prove_ that there is a problem here - if you can, it's likely that such a problem also affects gcc -shared too.

Quoted 32 lines · Russell King - ARM Linux

Quoted 11 lines · Ard Biesheuvel

Currently, the decompressor section layout is somewhat unusual: head.S puts code into a .start and a .text section, which are expected to be emitted back to back, unless other objects are included that define a .start section as well, in which case execution is expected to proceed linearly from head.S's section .start section into the other .start section and then onward into head.S's .text section.

This relies on the input files to be consumed by the linker in the same order they were specified on the command line, but this is not actually guaranteed by the linker, so it is better to make the inclusion of this code specific by using function calls.

I think you're wrong about that - as I've pointed out, this behaviour is relied upon quite heavily when linking programs using GCC, so while it may not be explicitly specified, the fact that GCC relies upon it when creating shared libraries (using crti.o before any object files, and crtn.o at the end) means that it's pretty much guaranteed.

What is less guaranteed is proceeding between two sections, but these separate sections in object files are combined into one .text section in the output file, and so we know that code in .start will be present _before_ any code in .text.

Put the two things together, and the code from head.S will be listed first, and therefore will be output first in the .start and .text sections. Other object files will have code placed into the output .text section accordingly.

So really I see no point to your patch 1 to 5, other than change for change's sake.

Quoted 3 lines

Can you _prove_ that there is a problem here - if you can, it's likely that such a problem also affects gcc -shared too.

As I replied in the other patch, GCC's builtin linker script uses SORT_NONE(), and probably does so for a reason.

Quoted 38 lines · Ard Biesheuvel

Quoted 35 lines · Russell King - ARM Linux

Quoted 11 lines · Ard Biesheuvel

Currently, the decompressor section layout is somewhat unusual: head.S puts code into a .start and a .text section, which are expected to be emitted back to back, unless other objects are included that define a .start section as well, in which case execution is expected to proceed linearly from head.S's section .start section into the other .start section and then onward into head.S's .text section.

This relies on the input files to be consumed by the linker in the same order they were specified on the command line, but this is not actually guaranteed by the linker, so it is better to make the inclusion of this code specific by using function calls.

I think you're wrong about that - as I've pointed out, this behaviour is relied upon quite heavily when linking programs using GCC, so while it may not be explicitly specified, the fact that GCC relies upon it when creating shared libraries (using crti.o before any object files, and crtn.o at the end) means that it's pretty much guaranteed.

What is less guaranteed is proceeding between two sections, but these separate sections in object files are combined into one .text section in the output file, and so we know that code in .start will be present _before_ any code in .text.

Put the two things together, and the code from head.S will be listed first, and therefore will be output first in the .start and .text sections. Other object files will have code placed into the output .text section accordingly.

So really I see no point to your patch 1 to 5, other than change for change's sake.

Can you _prove_ that there is a problem here - if you can, it's likely that such a problem also affects gcc -shared too.

Quoted 2 lines

As I replied in the other patch, GCC's builtin linker script uses SORT_NONE(), and probably does so for a reason.

To disregard any command line section sorting option that may be supplied by the user. That is not the case here. Please read the LD manual, specifically the "Input Section Wildcards" section.