[PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[Andrey Smirnov]

Since get_runtime_offset() is executed as a part of reloaction logic,
it cannot have code dependend on any kind of
relocation. Unfortunately, current codebase violates this rule and

linkadr:
.quad get_runtime_offset

ends up producing R_AARCH64_RELATIVE relocation that has to be
resolved at runtime. From tiral and error experimentation it seems
that the simplest way to do this is to drop "a" (allocatable)
attribute fom the section directive in runtime-offset.S

With "a" (see first entry):

aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl

images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64

DYNAMIC RELOCATION RECORDS
OFFSET           TYPE              VALUE
00000000000000b0 R_AARCH64_RELATIVE  *ABS*+0x00000000000000a0
0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028128
00000000000042e0 R_AARCH64_RELATIVE  *ABS*
00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8

Without "a":

 aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl

images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64

DYNAMIC RELOCATION RECORDS
OFFSET           TYPE              VALUE
0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028110
00000000000042e0 R_AARCH64_RELATIVE  *ABS*
00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8

Note that on recent toolchains (tested on 8.1.1), this problem is
masked by the fact that

.quad get_runtime_offset

will be initialized with link-time value of "get_runtime_offset" in
addition to having a R_AARCH64_RELATIVE relocation.

00000000000000a0 <get_runtime_offset>:
      a0:	10000000 	adr	x0, a0 <get_runtime_offset>
      a4:	58000061 	ldr	x1, b0 <linkadr>
      a8:	eb010000 	subs	x0, x0, x1
      ac:	d65f03c0 	ret

00000000000000b0 <linkadr>:
      b0:	000000a0 	.word	0x000000a0
      b4:	00000000 	.word	0x00000000

_However_, older toolchains (tested on 5.5.0), will only issue a
R_AARCH64_RELATIVE, so memory location will contain only zeroes:

00000000000000a0 <get_runtime_offset>:
      a0:	10000000 	adr	x0, a0 <get_runtime_offset>
      a4:	58000061 	ldr	x1, b0 <linkadr>
      a8:	eb010000 	subs	x0, x0, x1
      ac:	d65f03c0 	ret

00000000000000b0 <linkadr>:
	...

This leads to an very early crash and complete boot failure in the
latter case.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Sascha:

Assuming that proposed fix is OK, this should probably got to 'master'
as well as 'next'.

Thanks,
Andrey Smirnov

 arch/arm/lib64/runtime-offset.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/lib64/runtime-offset.S b/arch/arm/lib64/runtime-offset.S
index 177ca6478..c90469e66 100644
--- a/arch/arm/lib64/runtime-offset.S
+++ b/arch/arm/lib64/runtime-offset.S
@@ -1,7 +1,7 @@
 #include <linux/linkage.h>
 #include <asm/assembler.h>
 
-.section ".text_bare_init","ax"
+.section ".text_bare_init","x"
 
 /*
  * Get the offset between the link address and the address
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[Sascha Hauer]

Hi Andrey,

On Wed, Jan 23, 2019 at 07:15:43PM -0800, Andrey Smirnov wrote:
> Since get_runtime_offset() is executed as a part of reloaction logic,
> it cannot have code dependend on any kind of
> relocation. Unfortunately, current codebase violates this rule and
> 
> linkadr:
> .quad get_runtime_offset
> 
> ends up producing R_AARCH64_RELATIVE relocation that has to be
> resolved at runtime. From tiral and error experimentation it seems
> that the simplest way to do this is to drop "a" (allocatable)
> attribute fom the section directive in runtime-offset.S
> 
> With "a" (see first entry):
> 
> aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl
> 
> images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64
> 
> DYNAMIC RELOCATION RECORDS
> OFFSET           TYPE              VALUE
> 00000000000000b0 R_AARCH64_RELATIVE  *ABS*+0x00000000000000a0
> 0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
> 0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028128
> 00000000000042e0 R_AARCH64_RELATIVE  *ABS*
> 00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
> 00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8
> 
> Without "a":
> 
>  aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl
> 
> images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64
> 
> DYNAMIC RELOCATION RECORDS
> OFFSET           TYPE              VALUE
> 0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
> 0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028110
> 00000000000042e0 R_AARCH64_RELATIVE  *ABS*
> 00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
> 00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8
> 
> Note that on recent toolchains (tested on 8.1.1), this problem is
> masked by the fact that
> 
> .quad get_runtime_offset
> 
> will be initialized with link-time value of "get_runtime_offset" in
> addition to having a R_AARCH64_RELATIVE relocation.
> 
> 00000000000000a0 <get_runtime_offset>:
>       a0:	10000000 	adr	x0, a0 <get_runtime_offset>
>       a4:	58000061 	ldr	x1, b0 <linkadr>
>       a8:	eb010000 	subs	x0, x0, x1
>       ac:	d65f03c0 	ret
> 
> 00000000000000b0 <linkadr>:
>       b0:	000000a0 	.word	0x000000a0
>       b4:	00000000 	.word	0x00000000
> 
> _However_, older toolchains (tested on 5.5.0), will only issue a
> R_AARCH64_RELATIVE, so memory location will contain only zeroes:
> 
> 00000000000000a0 <get_runtime_offset>:
>       a0:	10000000 	adr	x0, a0 <get_runtime_offset>
>       a4:	58000061 	ldr	x1, b0 <linkadr>
>       a8:	eb010000 	subs	x0, x0, x1
>       ac:	d65f03c0 	ret
> 
> 00000000000000b0 <linkadr>:
> 	...
> 
> This leads to an very early crash and complete boot failure in the
> latter case.

I can reproduce this issue here. As you can imagine I do not really like
this "fix". I have no idea what the proper solution is (other than
deprecating gcc5), so I am fine removing the "a" flag as you suggested.
I think though that we should add a big comment above this function
*why* this lacks the "a" flag and that we can add it back once gcc5
is retired.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[Andrey Smirnov]

On Mon, Jan 28, 2019 at 12:56 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Andrey,
>
> On Wed, Jan 23, 2019 at 07:15:43PM -0800, Andrey Smirnov wrote:
> > Since get_runtime_offset() is executed as a part of reloaction logic,
> > it cannot have code dependend on any kind of
> > relocation. Unfortunately, current codebase violates this rule and
> >
> > linkadr:
> > .quad get_runtime_offset
> >
> > ends up producing R_AARCH64_RELATIVE relocation that has to be
> > resolved at runtime. From tiral and error experimentation it seems
> > that the simplest way to do this is to drop "a" (allocatable)
> > attribute fom the section directive in runtime-offset.S
> >
> > With "a" (see first entry):
> >
> > aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl
> >
> > images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64
> >
> > DYNAMIC RELOCATION RECORDS
> > OFFSET           TYPE              VALUE
> > 00000000000000b0 R_AARCH64_RELATIVE  *ABS*+0x00000000000000a0
> > 0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
> > 0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028128
> > 00000000000042e0 R_AARCH64_RELATIVE  *ABS*
> > 00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028118
> > 00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8
> >
> > Without "a":
> >
> >  aarch64-linux-gnu-objdump -R images/start_zii_imx8mq_dev.pbl
> >
> > images/start_zii_imx8mq_dev.pbl:     file format elf64-littleaarch64
> >
> > DYNAMIC RELOCATION RECORDS
> > OFFSET           TYPE              VALUE
> > 0000000000004258 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
> > 0000000000004260 R_AARCH64_RELATIVE  *ABS*+0x0000000000028110
> > 00000000000042e0 R_AARCH64_RELATIVE  *ABS*
> > 00000000000042e8 R_AARCH64_RELATIVE  *ABS*+0x0000000000028100
> > 00000000000042f0 R_AARCH64_RELATIVE  *ABS*+0x00000000000042c8
> >
> > Note that on recent toolchains (tested on 8.1.1), this problem is
> > masked by the fact that
> >
> > .quad get_runtime_offset
> >
> > will be initialized with link-time value of "get_runtime_offset" in
> > addition to having a R_AARCH64_RELATIVE relocation.
> >
> > 00000000000000a0 <get_runtime_offset>:
> >       a0:     10000000        adr     x0, a0 <get_runtime_offset>
> >       a4:     58000061        ldr     x1, b0 <linkadr>
> >       a8:     eb010000        subs    x0, x0, x1
> >       ac:     d65f03c0        ret
> >
> > 00000000000000b0 <linkadr>:
> >       b0:     000000a0        .word   0x000000a0
> >       b4:     00000000        .word   0x00000000
> >
> > _However_, older toolchains (tested on 5.5.0), will only issue a
> > R_AARCH64_RELATIVE, so memory location will contain only zeroes:
> >
> > 00000000000000a0 <get_runtime_offset>:
> >       a0:     10000000        adr     x0, a0 <get_runtime_offset>
> >       a4:     58000061        ldr     x1, b0 <linkadr>
> >       a8:     eb010000        subs    x0, x0, x1
> >       ac:     d65f03c0        ret
> >
> > 00000000000000b0 <linkadr>:
> >       ...
> >
> > This leads to an very early crash and complete boot failure in the
> > latter case.
>
> I can reproduce this issue here. As you can imagine I do not really like
> this "fix". I have no idea what the proper solution is (other than
> deprecating gcc5), so I am fine removing the "a" flag as you suggested.
> I think though that we should add a big comment above this function

Sure, will add the comment in v2.

> *why* this lacks the "a" flag and that we can add it back once gcc5
> is retired.
>

AFAICT, we don't want a relocation there even if GCC5 is deprecated
and it will always be conveniently initialized for us. To turn the
tables a bit, why do we need that "a" there? What's its purpose?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[Sascha Hauer]

On Mon, Jan 28, 2019 at 11:12:29AM -0800, Andrey Smirnov wrote:
> > > _However_, older toolchains (tested on 5.5.0), will only issue a
> > > R_AARCH64_RELATIVE, so memory location will contain only zeroes:
> > >
> > > 00000000000000a0 <get_runtime_offset>:
> > >       a0:     10000000        adr     x0, a0 <get_runtime_offset>
> > >       a4:     58000061        ldr     x1, b0 <linkadr>
> > >       a8:     eb010000        subs    x0, x0, x1
> > >       ac:     d65f03c0        ret
> > >
> > > 00000000000000b0 <linkadr>:
> > >       ...
> > >
> > > This leads to an very early crash and complete boot failure in the
> > > latter case.
> >
> > I can reproduce this issue here. As you can imagine I do not really like
> > this "fix". I have no idea what the proper solution is (other than
> > deprecating gcc5), so I am fine removing the "a" flag as you suggested.
> > I think though that we should add a big comment above this function
> 
> Sure, will add the comment in v2.
> 
> > *why* this lacks the "a" flag and that we can add it back once gcc5
> > is retired.
> >
> 
> AFAICT, we don't want a relocation there even if GCC5 is deprecated
> and it will always be conveniently initialized for us. To turn the
> tables a bit, why do we need that "a" there? What's its purpose?

The "a" is for "allocatable" meaning that space should be allocated in
the output binary. If you put get_runtime_offset into its own section
(outside .text) without the "a" flag then the linker linker bails out
moaning about overlapping sections. I think the .text segment is
inherently allocatable somehow, but then I wonder why the "a" flag makes
a difference at all. It may just be a bug in the early aarc64
toolchains, who knows...

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: aarch64: Avoid relocations in runtime-offset.S

[Andrey Smirnov]

On Tue, Jan 29, 2019 at 1:44 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Mon, Jan 28, 2019 at 11:12:29AM -0800, Andrey Smirnov wrote:
> > > > _However_, older toolchains (tested on 5.5.0), will only issue a
> > > > R_AARCH64_RELATIVE, so memory location will contain only zeroes:
> > > >
> > > > 00000000000000a0 <get_runtime_offset>:
> > > >       a0:     10000000        adr     x0, a0 <get_runtime_offset>
> > > >       a4:     58000061        ldr     x1, b0 <linkadr>
> > > >       a8:     eb010000        subs    x0, x0, x1
> > > >       ac:     d65f03c0        ret
> > > >
> > > > 00000000000000b0 <linkadr>:
> > > >       ...
> > > >
> > > > This leads to an very early crash and complete boot failure in the
> > > > latter case.
> > >
> > > I can reproduce this issue here. As you can imagine I do not really like
> > > this "fix". I have no idea what the proper solution is (other than
> > > deprecating gcc5), so I am fine removing the "a" flag as you suggested.
> > > I think though that we should add a big comment above this function
> >
> > Sure, will add the comment in v2.
> >
> > > *why* this lacks the "a" flag and that we can add it back once gcc5
> > > is retired.
> > >
> >
> > AFAICT, we don't want a relocation there even if GCC5 is deprecated
> > and it will always be conveniently initialized for us. To turn the
> > tables a bit, why do we need that "a" there? What's its purpose?
>
> The "a" is for "allocatable"

Yeah, this part of the desciprion in LD manual makes some sense

> meaning that space should be allocated in the output binary.

but this is the part I have trouble reasoning through or reconciling
with result I see in binary files. I can see how "a" would be
important in case when we have a full blown OS loading a proper ELF
file from disk to memory. However, I am not sure how to apply the
concept of allocatabilty to the case where we have flat binary file
created ahead of time using a linker script. It seems to me that what
should and shouldn't go into binary file should already be captured by
the script file.

> If you put get_runtime_offset into its own section
> (outside .text) without the "a" flag then the linker linker bails out
> moaning about overlapping sections.

Hmm, maybe I am not replicating your experiment exactly, but I just
tired compiling get_runtime_offset() with .section
".__image_start","x" directive on both ARM and ARM64 and it seemed to
work as you'd expect.

> I think the .text segment is
> inherently allocatable somehow, but then I wonder why the "a" flag makes
> a difference at all. It may just be a bug in the early aarc64
> toolchains, who knows...
>

Yeah, I guess what I was trying to say that this is definitely a "fix"
that's not 100% reasoned out, but that might be because the original
code that's being fixed is not quite reasoned out either.

Anyway, adding a comment with explanation is definitely a good idea.
I'll re-spin v2 with it shortly.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox