[RFC PATCH] Link the bootwrapper as a position-independent executable

[RFC PATCH] Link the bootwrapper as a position-independent executable

[Paul Mackerras]

This changes the way we make the boot wrapper position-independent.
Before we just added the offset (the difference between runtime
address and link address) to each entry in the .got2 section.  That
doesn't relocate pointer values in initialized variables and arrays.

Instead we now link the bootwrapper with -pie to get a position-
independent executable, and process the relocations in the dynamic
relocation section that the linker puts into the executable.  This
means that initialized variables and arrays get relocated properly.

Currently we only process R_PPC_RELATIVE relocations, which is all we
get provided C files are compiled with -fPIC (which they are already)
and assembly code is written to be position-independent.  Some of the
changes below are to make parts of crt0.S be PIC code (i.e. generate
no relocations other than R_PPC_RELATIVE).

The relocation code below does nothing if there is no dynamic section,
which means that we can link without -pie and it will work provided
that the bootwrapper executable is loaded at its linked-at address.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index f1c4dfc..a6901c3 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -6,16 +6,28 @@
  * as published by the Free Software Foundation; either version
  * 2 of the License, or (at your option) any later version.
  *
- * NOTE: this code runs in 32 bit mode and is packaged as ELF32.
+ * NOTE: this code runs in 32 bit mode, is position-independent,
+ * and is packaged as ELF32.
  */
 
 #include "ppc_asm.h"
 
 	.text
-	/* a procedure descriptor used when booting this as a COFF file */
+	/* A procedure descriptor used when booting this as a COFF file.
+	 * When making COFF, this comes first in the link and we're
+	 * linked at 0x500000.
+	 */
 	.globl	_zimage_start_opd
 _zimage_start_opd:
-	.long	_zimage_start, 0, 0, 0
+	.long	0x500000, 0, 0, 0
+
+p_start:	.long	_start
+p_etext:	.long	_etext
+p_bss_start:	.long	__bss_start
+p_end:		.long	_end
+
+	.weak	_platform_stack_top
+p_pstack:	.long	_platform_stack_top
 
 	.weak	_zimage_start
 	.globl	_zimage_start
@@ -24,37 +36,60 @@ _zimage_start:
 _zimage_start_lib:
 	/* Work out the offset between the address we were linked at
 	   and the address where we're running. */
-	bl	1f
-1:	mflr	r0
-	lis	r9,1b@ha
-	addi	r9,r9,1b@l
-	subf.	r0,r9,r0
-	beq	3f		/* if running at same address as linked */
+	bl	.+4
+p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
+	/* grab the link address of the dynamic section in r11 */
+	addis	r11,r10,(_GLOBAL_OFFSET_TABLE_-p_base)@ha
+	lwz	r11,(_GLOBAL_OFFSET_TABLE_-p_base)@l(r11)
+	cmpwi	r11,0
+	beq	3f		/* if not linked -pie */
+	/* get the runtime address of the dynamic section in r12 */
+	.weak	__dynamic_start
+	addis	r12,r10,(__dynamic_start-p_base)@ha
+	addi	r12,r12,(__dynamic_start-p_base)@l
+	subf	r11,r11,r12	/* runtime - linktime offset */
 
-	/* The .got2 section contains a list of addresses, so add
-	   the address offset onto each entry. */
-	lis	r9,__got2_start@ha
-	addi	r9,r9,__got2_start@l
-	lis	r8,__got2_end@ha
-	addi	r8,r8,__got2_end@l
-	subf.	r8,r9,r8
+	/* The dynamic section contains a series of tagged entries.
+	 * We need the RELA and RELACOUNT entries. */
+RELA = 7
+RELACOUNT = 0x6ffffff9
+	li	r9,0
+	li	r0,0
+9:	lwz	r8,0(r12)	/* get tag */
+	cmpwi	r8,0
+	beq	10f		/* end of list */
+	cmpwi	r8,RELA
+	bne	11f
+	lwz	r9,4(r12)	/* get RELA pointer in r9 */
+	b	12f
+11:	addis	r8,r8,(-RELACOUNT)@ha
+	cmpwi	r8,RELACOUNT@l
+	bne	12f
+	lwz	r0,4(r12)	/* get RELACOUNT value in r0 */
+12:	addi	r12,r12,8
+	b	9b
+
+	/* The relocation section contains a list of relocations.
+	 * We now do the R_PPC_RELATIVE ones, which point to words
+	 * which need to be initialized with addend + offset.
+	 * The R_PPC_RELATIVE ones come first and there are RELACOUNT
+	 * of them. */
+10:	or.	r8,r0,r9	/* skip relocation if we don't have both */
 	beq	3f
-	srwi.	r8,r8,2
-	mtctr	r8
-	add	r9,r0,r9
-2:	lwz	r8,0(r9)
-	add	r8,r8,r0
-	stw	r8,0(r9)
-	addi	r9,r9,4
+	mtctr	r0
+2:	lbz	r0,4+3(r9)	/* ELF32_R_INFO(reloc->r_info) */
+	cmpwi	r0,22		/* R_PPC_RELATIVE */
+	bne	3f
+	lwz	r12,0(r9)	/* reloc->r_offset */
+	lwz	r0,8(r9)	/* reloc->r_addend */
+	add	r0,r0,r11
+	stwx	r0,r11,r12
+	addi	r9,r9,12
 	bdnz	2b
 
 	/* Do a cache flush for our text, in case the loader didn't */
-3:	lis	r9,_start@ha
-	addi	r9,r9,_start@l
-	add	r9,r0,r9
-	lis	r8,_etext@ha
-	addi	r8,r8,_etext@l
-	add	r8,r0,r8
+3:	lwz	r9,p_start-p_base(r10)	/* note: these are relocated now */
+	lwz	r8,p_etext-p_base(r10)
 4:	dcbf	r0,r9
 	icbi	r0,r9
 	addi	r9,r9,0x20
@@ -64,27 +99,19 @@ _zimage_start_lib:
 	isync
 
 	/* Clear the BSS */
-	lis	r9,__bss_start@ha
-	addi	r9,r9,__bss_start@l
-	add	r9,r0,r9
-	lis	r8,_end@ha
-	addi	r8,r8,_end@l
-	add	r8,r0,r8
-	li	r10,0
-5:	stw	r10,0(r9)
+	lwz	r9,p_bss_start-p_base(r10)
+	lwz	r8,p_end-p_base(r10)
+	li	r0,0
+5:	stw	r0,0(r9)
 	addi	r9,r9,4
 	cmplw	cr0,r9,r8
 	blt	5b
 
 	/* Possibly set up a custom stack */
-.weak	_platform_stack_top
-	lis	r8,_platform_stack_top@ha
-	addi	r8,r8,_platform_stack_top@l
+	lwz	r8,p_pstack-p_base(r10)
 	cmpwi	r8,0
 	beq	6f
-	add	r8,r0,r8
 	lwz	r1,0(r8)
-	add	r1,r0,r1
 	li	r0,0
 	stwu	r0,-16(r1)	/* establish a stack frame */
 6:
diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 644bf9d..64653b3 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -39,6 +39,7 @@ dts=
 cacheit=
 binary=
 gzip=.gz
+pie=-pie
 
 # cross-compilation prefix
 CROSS=
@@ -149,9 +150,10 @@ pmac|chrp)
     platformo=$object/of.o
     ;;
 coff)
-    platformo=$object/of.o
+    platformo=$object/crt0.o $object/of.o
     lds=$object/zImage.coff.lds
     link_address='0x500000'
+    pie=
     ;;
 miboot|uboot)
     # miboot and U-boot want just the bare bits, not an ELF binary
@@ -197,6 +199,7 @@ ps3)
     ksection=.kernel:vmlinux.bin
     isection=.kernel:initrd
     link_address=''
+    pie=
     ;;
 ep88xc|ep405|ep8248e)
     platformo="$object/fixed-head.o $object/$platform.o"
@@ -288,9 +291,9 @@ fi
 
 if [ "$platform" != "miboot" ]; then
     if [ -n "$link_address" ] ; then
-        text_start="-Ttext $link_address --defsym _start=$link_address"
+        text_start="-Ttext $link_address"
     fi
-    ${CROSS}ld -m elf32ppc -T $lds $text_start -o "$ofile" \
+    ${CROSS}ld -m elf32ppc -T $lds $text_start $pie -o "$ofile" \
 	$platformo $tmp $object/wrapper.a
     rm $tmp
 fi
diff --git a/arch/powerpc/boot/zImage.coff.lds.S b/arch/powerpc/boot/zImage.coff.lds.S
index 856dc78..de4c9e3 100644
--- a/arch/powerpc/boot/zImage.coff.lds.S
+++ b/arch/powerpc/boot/zImage.coff.lds.S
@@ -3,13 +3,13 @@ ENTRY(_zimage_start_opd)
 EXTERN(_zimage_start_opd)
 SECTIONS
 {
-  _start = .;
   .text      :
   {
+    _start = .;
     *(.text)
     *(.fixup)
+    _etext = .;
   }
-  _etext = .;
   . = ALIGN(4096);
   .data    :
   {
@@ -17,9 +17,7 @@ SECTIONS
     *(.data*)
     *(__builtin_*)
     *(.sdata*)
-    __got2_start = .;
     *(.got2)
-    __got2_end = .;
 
     _dtb_start = .;
     *(.kernel:dtb)
diff --git a/arch/powerpc/boot/zImage.lds.S b/arch/powerpc/boot/zImage.lds.S
index 0962d62..05af879 100644
--- a/arch/powerpc/boot/zImage.lds.S
+++ b/arch/powerpc/boot/zImage.lds.S
@@ -3,49 +3,63 @@ ENTRY(_zimage_start)
 EXTERN(_zimage_start)
 SECTIONS
 {
-  _start = .;
   .text      :
   {
+    _start = .;
     *(.text)
     *(.fixup)
+    _etext = .;
   }
-  _etext = .;
   . = ALIGN(4096);
   .data    :
   {
     *(.rodata*)
     *(.data*)
     *(.sdata*)
-    __got2_start = .;
     *(.got2)
-    __got2_end = .;
   }
+  .dynsym : { *(.dynsym) }
+  .dynstr : { *(.dynstr) }
+  .dynamic :
+  {
+    __dynamic_start = .;
+    *(.dynamic)
+  }
+  .hash : { *(.hash) }
+  .interp : { *(.interp) }
+  .rela.dyn : { *(.rela*) }
 
   . = ALIGN(8);
-  _dtb_start = .;
-  .kernel:dtb : { *(.kernel:dtb) }
-  _dtb_end = .;
-
-  . = ALIGN(4096);
-  _vmlinux_start =  .;
-  .kernel:vmlinux.strip : { *(.kernel:vmlinux.strip) }
-  _vmlinux_end =  .;
+  .kernel:dtb :
+  {
+    _dtb_start = .;
+    *(.kernel:dtb)
+    _dtb_end = .;
+  }
 
   . = ALIGN(4096);
-  _initrd_start =  .;
-  .kernel:initrd : { *(.kernel:initrd) }
-  _initrd_end =  .;
+  .kernel:vmlinux.strip :
+  {
+    _vmlinux_start =  .;
+    *(.kernel:vmlinux.strip)
+    _vmlinux_end =  .;
+  }
 
   . = ALIGN(4096);
-  _edata  =  .;
+  .kernel:initrd :
+  {
+    _initrd_start =  .;
+    *(.kernel:initrd)
+    _initrd_end =  .;
+  }
 
   . = ALIGN(4096);
-  __bss_start = .;
   .bss       :
   {
-   *(.sbss)
-   *(.bss)
+    _edata  =  .;
+    __bss_start = .;
+    *(.sbss)
+    *(.bss)
+    _end = . ;
   }
-  . = ALIGN(4096);
-  _end = . ;
 }

Re: [RFC PATCH] Link the bootwrapper as a position-independent executable

[Segher Boessenkool]

> Instead we now link the bootwrapper with -pie to get a position-
> independent executable, and process the relocations in the dynamic
> relocation section that the linker puts into the executable.

Hurray!  Looks good, just a few nits...

> +	bl	.+4
> +p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */

bl p_base  instead?

> +10:	or.	r8,r0,r9	/* skip relocation if we don't have both */
>  	beq	3f

Either the code or the comment is wrong -- the code says "skip  
relocation
if we don't have either".

> +	cmpwi	r0,22		/* R_PPC_RELATIVE */
> +	bne	3f

It would be nice if there was some way to complain (at build time?)
if there are unhandled relocations present.  Prevents a lot of headaches
when things go wrong (and they will ;-) )

>  4:	dcbf	r0,r9
>  	icbi	r0,r9

Fix these while you're at it?  It's not r0, it's 0.

> +  .dynsym : { *(.dynsym) }
> +  .dynstr : { *(.dynstr) }
> +  .dynamic :
> +  {
> +    __dynamic_start = .;
> +    *(.dynamic)
> +  }
> +  .hash : { *(.hash) }
> +  .interp : { *(.interp) }
> +  .rela.dyn : { *(.rela*) }

Do some of these sections need alignment?


Segher

Re: [RFC PATCH] Link the bootwrapper as a position-independent executable

[Paul Mackerras]

Segher Boessenkool writes:

> Hurray!  Looks good, just a few nits...

Thanks for reviewing.

> > +	bl	.+4
> > +p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
> 
> bl p_base  instead?

I went back and forth on that.  I ended up with it that way to
emphasize that the bl does need to branch to the *next* instruction
for the idiom to work.

> > +10:	or.	r8,r0,r9	/* skip relocation if we don't have both */
> >  	beq	3f
> 
> Either the code or the comment is wrong -- the code says "skip  
> relocation
> if we don't have either".

Ah, operator precedence rules in English. :)  I (and I think most
native English speakers) would parse my comment as "don't (have both)"
whereas I think you parsed it as "(don't have) both".  Similarly what
you say the code says parses as "don't (have either)" for me rather
than "(don't have) either".   IOW, "don't have either" means "both are
missing".

Anyway I can change the comment to say "we need both to do
relocation".  OK?

> > +	cmpwi	r0,22		/* R_PPC_RELATIVE */
> > +	bne	3f
> 
> It would be nice if there was some way to complain (at build time?)
> if there are unhandled relocations present.  Prevents a lot of headaches
> when things go wrong (and they will ;-) )

Yes, that would be a good idea.  There is one extra relocation when I
build for pSeries, which was for _platform_stack_top (which is
undefined), which we're OK ignoring.

> >  4:	dcbf	r0,r9
> >  	icbi	r0,r9
> 
> Fix these while you're at it?  It's not r0, it's 0.

Yeah.

> > +  .dynsym : { *(.dynsym) }
> > +  .dynstr : { *(.dynstr) }
> > +  .dynamic :
> > +  {
> > +    __dynamic_start = .;
> > +    *(.dynamic)
> > +  }
> > +  .hash : { *(.hash) }
> > +  .interp : { *(.interp) }
> > +  .rela.dyn : { *(.rela*) }
> 
> Do some of these sections need alignment?

I suppose they should ideally be 4-byte aligned.  We don't actually
use .dynsym, .dynstr, .hash or .interp, but I couldn't find any way to
make the linker omit them.

Paul.

Re: [RFC PATCH] Link the bootwrapper as a position-independent executable

[Segher Boessenkool]

>>> +	bl	.+4
>>> +p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
>>
>> bl p_base  instead?
>
> I went back and forth on that.  I ended up with it that way to
> emphasize that the bl does need to branch to the *next* instruction
> for the idiom to work.

Right, I see.  Make it even more obvious?  Use "bcl 20,31,$+4" instead
(so people will have to look it up before changing this code, not  
because
it is a few cycles faster ;-) ), add an empty line before the label, or
even put an actual comment there?  :-)


>>> +10:	or.	r8,r0,r9	/* skip relocation if we don't have both */
>>>  	beq	3f
>>
>> Either the code or the comment is wrong -- the code says "skip
>> relocation
>> if we don't have either".
>
> Ah, operator precedence rules in English. :)

While I don't think that double (and triple) negations in English are
not overused and confusing...

> I (and I think most
> native English speakers) would parse my comment as "don't (have both)"
> whereas I think you parsed it as "(don't have) both".  Similarly what
> you say the code says parses as "don't (have either)" for me rather
> than "(don't have) either".   IOW, "don't have either" means "both are
> missing".

... that is exactly what I meant: the code skips relocation only if
both are missing.

> Anyway I can change the comment to say "we need both to do
> relocation".  OK?

Please do -- but also change the code to match :-)

>>> +  .dynsym : { *(.dynsym) }
>>> +  .dynstr : { *(.dynstr) }
>>> +  .dynamic :
>>> +  {
>>> +    __dynamic_start = .;
>>> +    *(.dynamic)
>>> +  }
>>> +  .hash : { *(.hash) }
>>> +  .interp : { *(.interp) }
>>> +  .rela.dyn : { *(.rela*) }
>>
>> Do some of these sections need alignment?
>
> I suppose they should ideally be 4-byte aligned.

Ideally, yes; I was questioning whether the ABI requires it.  It will
only cost a few bytes of object size so let's just do it?

> We don't actually
> use .dynsym, .dynstr, .hash or .interp, but I couldn't find any way to
> make the linker omit them.

Assign those input sections to the /DISCARD/ output section (and do
that early enough in the linker script so they aren't picked up by
some other wildcard).  Something like

	/DISCARD/ : { *(.dynstr .dynsym .hash .interp) }


Segher

Re: [RFC PATCH] Link the bootwrapper as a position-independent executable

[Paul Mackerras]

Segher Boessenkool writes:

> ... that is exactly what I meant: the code skips relocation only if
> both are missing.

Doh!  You're right, thanks.  Serves me right for being so clever. :)

Paul.