[PATCH 0/2] Make badr macro compatible with newer GAS versions

[PATCH 0/2] Make badr macro compatible with newer GAS versions

[Ard Biesheuvel]

Work around the mindless and backward incompatible change in GAS that
prevents us from using a simple addition to set the Thumb bit in local
symbol references taken using 'adr' instructions (#2)

As a preparatory step, remove badr occurrences in ARM code sequences
contained in Thumb2 kernels.

Ard Biesheuvel (2):
  ARM: avoid badr macro for switching to Thumb-2 mode
  ARM: assembler: prevent ADR from setting the Thumb bit twice

 arch/arm/common/mcpm_head.S      |  5 ++---
 arch/arm/include/asm/assembler.h | 22 +++++++++++++++++++-
 arch/arm/kernel/head-nommu.S     |  7 +++----
 arch/arm/kernel/head.S           | 15 +++++++------
 arch/arm/kernel/sleep.S          |  7 +++----
 5 files changed, 36 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH 1/2] ARM: avoid badr macro for switching to Thumb-2 mode

[Ard Biesheuvel]

Switching to Thumb-2 mode can be done using a single 'sub' instruction
so use that instead of the badr macro in various places in the code.
This allows us to reimplement the macro in a way that does not allow
it to be used in ARM code sequences when building a Thumb2 kernel.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/common/mcpm_head.S  |  5 ++---
 arch/arm/kernel/head-nommu.S |  7 +++----
 arch/arm/kernel/head.S       | 15 +++++++--------
 arch/arm/kernel/sleep.S      |  7 +++----
 4 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/arch/arm/common/mcpm_head.S b/arch/arm/common/mcpm_head.S
index 08b3bb9bc6a2..4c72314e87a3 100644
--- a/arch/arm/common/mcpm_head.S
+++ b/arch/arm/common/mcpm_head.S
@@ -49,10 +49,9 @@
 ENTRY(mcpm_entry_point)
 
  ARM_BE8(setend        be)
- THUMB(	badr	r12, 1f		)
- THUMB(	bx	r12		)
+ THUMB(	sub	pc, pc, #3	)
  THUMB(	.thumb			)
-1:
+
 	mrc	p15, 0, r0, c0, c0, 5		@ MPIDR
 	ubfx	r9, r0, #0, #8			@ r9 = cpu
 	ubfx	r10, r0, #8, #8			@ r10 = cluster
diff --git a/arch/arm/kernel/head-nommu.S b/arch/arm/kernel/head-nommu.S
index dae8fa2f72c5..406dab0b773c 100644
--- a/arch/arm/kernel/head-nommu.S
+++ b/arch/arm/kernel/head-nommu.S
@@ -47,10 +47,9 @@ ENTRY(stext)
 	.arm
 ENTRY(stext)
 
- THUMB(	badr	r9, 1f		)	@ Kernel is always entered in ARM.
- THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
- THUMB(	.thumb			)	@ switch to Thumb now.
- THUMB(1:			)
+ THUMB(	sub	pc, pc, #3	)	@ Kernel is always entered in ARM.
+ THUMB(	.thumb			)	@ If this is a Thumb-2 kernel,
+					@ switch to Thumb now.
 #endif
 
 	setmode	PSR_F_BIT | PSR_I_BIT | SVC_MODE, r9 @ ensure svc mode
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 4b815821ec02..1e44ee9b2074 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -80,10 +80,9 @@
 ENTRY(stext)
  ARM_BE8(setend	be )			@ ensure we are in BE8 mode
 
- THUMB(	badr	r9, 1f		)	@ Kernel is always entered in ARM.
- THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
- THUMB(	.thumb			)	@ switch to Thumb now.
- THUMB(1:			)
+ THUMB(	sub	pc, pc, #3	)	@ Kernel is always entered in ARM.
+ THUMB(	.thumb			)	@ If this is a Thumb-2 kernel,
+					@ switch to Thumb now.
 
 #ifdef CONFIG_ARM_VIRT_EXT
 	bl	__hyp_stub_install
@@ -363,10 +362,10 @@ __turn_mmu_on_loc:
 	.text
 	.arm
 ENTRY(secondary_startup_arm)
- THUMB(	badr	r9, 1f		)	@ Kernel is entered in ARM.
- THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
- THUMB(	.thumb			)	@ switch to Thumb now.
- THUMB(1:			)
+ THUMB(	sub	pc, pc, #3	)	@ Kernel is entered in ARM.
+ THUMB(	.thumb			)	@ If this is a Thumb-2 kernel,
+					@ switch to Thumb now.
+
 ENTRY(secondary_startup)
 	/*
 	 * Common entry point for secondary CPUs.
diff --git a/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S
index a8257fc9cf2a..76b3d7c1c8d0 100644
--- a/arch/arm/kernel/sleep.S
+++ b/arch/arm/kernel/sleep.S
@@ -123,10 +123,9 @@ ENDPROC(cpu_resume_after_mmu)
 #ifdef CONFIG_MMU
 	.arm
 ENTRY(cpu_resume_arm)
- THUMB(	badr	r9, 1f		)	@ Kernel is entered in ARM.
- THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
- THUMB(	.thumb			)	@ switch to Thumb now.
- THUMB(1:			)
+ THUMB(	sub	pc, pc, #3	)	@ Kernel is entered in ARM.
+ THUMB(	.thumb			)	@ If this is a Thumb-2 kernel,
+					@ switch to Thumb now.
 #endif
 
 ENTRY(cpu_resume)
-- 
2.17.1

[PATCH 2/2] ARM: assembler: prevent ADR from setting the Thumb bit twice

[Ard Biesheuvel]

To work around recent issues where ADR references to Thumb function
symbols may or may not have the Thumb bit set already when they are
resolved by GAS, reference the symbol indirectly via a local symbol
typed as 'function', and emit the reference in a way that prevents
the assembler from resolving it directly. Instead, it will be fixed
up by the linker, which behaves consistently and according to spec
when it comes to relocations of symbols with function annotation
targetting wide ADR instructions.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/assembler.h | 22 +++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 6ae42ad29518..ee2d4eb26318 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -195,13 +195,33 @@
 	.irp	c,,eq,ne,cs,cc,mi,pl,vs,vc,hi,ls,ge,lt,gt,le,hs,lo
 	.macro	badr\c, rd, sym
 #ifdef CONFIG_THUMB2_KERNEL
-	adr\c	\rd, \sym + 1
+	__badr	\c, \rd, \sym
 #else
 	adr\c	\rd, \sym
 #endif
 	.endm
 	.endr
 
+	/*
+	 * GAS's behavior with respect to setting the Thumb bit on addresses
+	 * of locally defined symbols taken using adr instructions is
+	 * inconsistent, and so we are better off letting the linker handle
+	 * it instead. So emit the reference as a relocation, and force a
+	 * wide encoding so that we can support both forward and backward
+	 * references, and avoid the R_ARM_THM_PC8 relocation that operates
+	 * on the narrow encoding, which is documented as not taking the
+	 * Thumb bit into account. (IHI 0044E ELF for the ARM Architecture)
+	 *
+	 * Note that this needs to be a separate macro or \@ does not work
+	 * correctly.
+	 */
+	.macro		__badr, c, rd, sym
+	.set		.Lsym\@, \sym
+	.type		.Lsym\@, %function
+	.reloc		., R_ARM_THM_ALU_PREL_11_0, .Lsym\@
+	adr\c\().w	\rd, .
+	.endm
+
 /*
  * Get current thread_info.
  */
-- 
2.17.1

Re: [PATCH 0/2] Make badr macro compatible with newer GAS versions

[Guenter Roeck]

On Tue, Jun 19, 2018 at 09:26:31PM +0200, Ard Biesheuvel wrote:
> Work around the mindless and backward incompatible change in GAS that
> prevents us from using a simple addition to set the Thumb bit in local
> symbol references taken using 'adr' instructions (#2)
> 
> As a preparatory step, remove badr occurrences in ARM code sequences
> contained in Thumb2 kernels.
> 
> Ard Biesheuvel (2):
>   ARM: avoid badr macro for switching to Thumb-2 mode
>   ARM: assembler: prevent ADR from setting the Thumb bit twice
> 

This doesn't work for images built with a toolchain based on gcc 7.3.0
and binutils 2.28.1. It _does_ work for images built with gcc 7.3.0/
binutils 2.30.

Guenter

Re: [PATCH 0/2] Make badr macro compatible with newer GAS versions

[Ard Biesheuvel]

On 19 June 2018 at 22:32, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Jun 19, 2018 at 09:26:31PM +0200, Ard Biesheuvel wrote:
>> Work around the mindless and backward incompatible change in GAS that
>> prevents us from using a simple addition to set the Thumb bit in local
>> symbol references taken using 'adr' instructions (#2)
>>
>> As a preparatory step, remove badr occurrences in ARM code sequences
>> contained in Thumb2 kernels.
>>
>> Ard Biesheuvel (2):
>>   ARM: avoid badr macro for switching to Thumb-2 mode
>>   ARM: assembler: prevent ADR from setting the Thumb bit twice
>>
>
> This doesn't work for images built with a toolchain based on gcc 7.3.0
> and binutils 2.28.1. It _does_ work for images built with gcc 7.3.0/
> binutils 2.30.
>

Sigh.

So does it fail? Or is the resulting binary broken?

Re: [PATCH 0/2] Make badr macro compatible with newer GAS versions

[Guenter Roeck]

On Tue, Jun 19, 2018 at 10:34:38PM +0200, Ard Biesheuvel wrote:
> On 19 June 2018 at 22:32, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Tue, Jun 19, 2018 at 09:26:31PM +0200, Ard Biesheuvel wrote:
> >> Work around the mindless and backward incompatible change in GAS that
> >> prevents us from using a simple addition to set the Thumb bit in local
> >> symbol references taken using 'adr' instructions (#2)
> >>
> >> As a preparatory step, remove badr occurrences in ARM code sequences
> >> contained in Thumb2 kernels.
> >>
> >> Ard Biesheuvel (2):
> >>   ARM: avoid badr macro for switching to Thumb-2 mode
> >>   ARM: assembler: prevent ADR from setting the Thumb bit twice
> >>
> >
> > This doesn't work for images built with a toolchain based on gcc 7.3.0
> > and binutils 2.28.1. It _does_ work for images built with gcc 7.3.0/
> > binutils 2.30.
> >
> 
> Sigh.
> 
> So does it fail? Or is the resulting binary broken?

Hard to say. It crashes early in boot, even before earlycon can say
anything.

Qemu exec trace, broken:

race 0: 0x7fffcf92c0c0 [00000000/00000008/0x11080001]
Trace 0: 0x7fffcf92c300 [00000000/21008000/0x11080001] stext
Trace 0: 0x7fffcf92c480 [00000000/210099ce/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c480 [210099ce] index 1 -> 0x7fffcf92c780 [210099ea]
Trace 0: 0x7fffcf92c780 [00000000/210099ea/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c780 [210099ea] index 0 -> 0x7fffcf92c8c0 [210099dc]
Trace 0: 0x7fffcf92c8c0 [00000000/210099dc/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c8c0 [210099dc] index 1 -> 0x7fffcf92c780 [210099ea]
Trace 0: 0x7fffcf92c780 [00000000/210099ea/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c8c0 [210099dc] index 0 -> 0x7fffcf92ca80 [210099f6]
Trace 0: 0x7fffcf92ca80 [00000000/210099f6/0x11080001] __lookup_processor_type
Trace 0: 0x7fffcf92cb80 [00000000/2100800c/0x11080001] stext
Linking TBs 0x7fffcf92cb80 [2100800c] index 1 -> 0x7fffcf92cc80 [21008014]
Trace 0: 0x7fffcf92cc80 [00000000/21008014/0x11080001] stext
Trace 0: 0x7fffcf92cdc0 [00000000/2120477e/0x11080001] __v7m_setup
Trace 0: 0x7fffcf92d400 [00000000/212047bc/0x11080001] __v7m_setup
Trace 0: 0x7fffcf92d500 [00000000/212047be/0x11280000] __v7m_setup
                                           ^^^^^^^^^^
Trace 0: 0x7fffcf92d600 [00000000/2100b830/0x11280001] __invalid_entry
Trace 0: 0x7fffcf92d880 [00000000/2100b842/0x11280001] __invalid_entry
Trace 0: 0x7fffcf92e100 [00000000/21025780/0x11280001] printk
Trace 0: 0x7fffcf92e640 [00000000/21025a7c/0x11280001] vprintk_func

Qemu exec trace, ok:

Trace 0: 0x7fffcf92c0c0 [00000000/00000008/0x11080001]
Trace 0: 0x7fffcf92c300 [00000000/21008000/0x11080001] stext
Trace 0: 0x7fffcf92c480 [00000000/210099ce/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c480 [210099ce] index 1 -> 0x7fffcf92c780 [210099ea]
Trace 0: 0x7fffcf92c780 [00000000/210099ea/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c780 [210099ea] index 0 -> 0x7fffcf92c8c0 [210099dc]
Trace 0: 0x7fffcf92c8c0 [00000000/210099dc/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c8c0 [210099dc] index 1 -> 0x7fffcf92c780 [210099ea]
Trace 0: 0x7fffcf92c780 [00000000/210099ea/0x11080001] __lookup_processor_type
Linking TBs 0x7fffcf92c8c0 [210099dc] index 0 -> 0x7fffcf92ca80 [210099f6]
Trace 0: 0x7fffcf92ca80 [00000000/210099f6/0x11080001] __lookup_processor_type
Trace 0: 0x7fffcf92cb80 [00000000/2100800c/0x11080001] stext
Linking TBs 0x7fffcf92cb80 [2100800c] index 1 -> 0x7fffcf92cc80 [21008014]
Trace 0: 0x7fffcf92cc80 [00000000/21008014/0x11080001] stext
Trace 0: 0x7fffcf92cdc0 [00000000/2120477e/0x11080001] __v7m_setup
Trace 0: 0x7fffcf92d400 [00000000/212047bc/0x11080001] __v7m_setup
Trace 0: 0x7fffcf92d500 [00000000/212047be/0x11280001] __v7m_setup
Trace 0: 0x7fffcf92d600 [00000000/212047c0/0x11280001] __v7m_setup
Trace 0: 0x7fffcf92d900 [00000000/212047d2/0x11280001] __v7m_setup
Linking TBs 0x7fffcf92d900 [212047d2] index 1 -> 0x7fffcf92dcc0 [212047e0]
Trace 0: 0x7fffcf92dcc0 [00000000/212047e0/0x11280001] __v7m_setup
Trace 0: 0x7fffcf92e0c0 [00000000/21008020/0x11280001] stext
Trace 0: 0x7fffcf92e200 [00000000/210099c8/0x11280001] __after_proc_init

Guenter

Re: [PATCH 0/2] Make badr macro compatible with newer GAS versions

[Ard Biesheuvel]

On 19 June 2018 at 22:45, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Jun 19, 2018 at 10:34:38PM +0200, Ard Biesheuvel wrote:
>> On 19 June 2018 at 22:32, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Tue, Jun 19, 2018 at 09:26:31PM +0200, Ard Biesheuvel wrote:
>> >> Work around the mindless and backward incompatible change in GAS that
>> >> prevents us from using a simple addition to set the Thumb bit in local
>> >> symbol references taken using 'adr' instructions (#2)
>> >>
>> >> As a preparatory step, remove badr occurrences in ARM code sequences
>> >> contained in Thumb2 kernels.
>> >>
>> >> Ard Biesheuvel (2):
>> >>   ARM: avoid badr macro for switching to Thumb-2 mode
>> >>   ARM: assembler: prevent ADR from setting the Thumb bit twice
>> >>
>> >
>> > This doesn't work for images built with a toolchain based on gcc 7.3.0
>> > and binutils 2.28.1. It _does_ work for images built with gcc 7.3.0/
>> > binutils 2.30.
>> >
>>
>> Sigh.
>>
>> So does it fail? Or is the resulting binary broken?
>
> Hard to say. It crashes early in boot, even before earlycon can say
> anything.
>

OK, so even the linker handling is inconsistent.

Working (binutils 2.30)

c0301164 <local_restart>:
c0301164:       f8d9 a000       ldr.w   sl, [r9]
c0301168:       e92d 0030       stmdb   sp!, {r4, r5}
c030116c:       f01a 0ff0       tst.w   sl, #240        ; 0xf0
c0301170:       d117            bne.n   c03011a2 <__sys_trace>
c0301172:       46ba            mov     sl, r7
c0301174:       f5ba 7fc8       cmp.w   sl, #400        ; 0x190
c0301178:       bf28            it      cs
c030117a:       f04f 0a00       movcs.w sl, #0
c030117e:       f3af 8014       csdb
c0301182:       f2af 1e83       subw    lr, pc, #387    ; 0x183
                        c0301182: R_ARM_THM_ALU_PREL_11_0       .Lsym28


Broken (binutils 2.26)

c0301184 <local_restart>:
c0301184:       f8d9 a000       ldr.w   sl, [r9]
c0301188:       e92d 0030       stmdb   sp!, {r4, r5}
c030118c:       f01a 0ff0       tst.w   sl, #240        ; 0xf0
c0301190:       d117            bne.n   c03011c2 <__sys_trace>
c0301192:       46ba            mov     sl, r7
c0301194:       f5ba 7fc8       cmp.w   sl, #400        ; 0x190
c0301198:       bf28            it      cs
c030119a:       f04f 0a00       movcs.w sl, #0
c030119e:       f3af 8014       csdb
c03011a2:       f2af 1ea2       subw    lr, pc, #418    ; 0x1a2
                        c03011a2: R_ARM_THM_ALU_PREL_11_0       .Lsym30


Note the even immediate in the subw instruction. So this is another
dead end, unfortunately.

Thanks for testing.

Re: [PATCH 0/2] Make badr macro compatible with newer GAS versions

[Guenter Roeck]

On Wed, Jun 20, 2018 at 12:23:56AM +0200, Ard Biesheuvel wrote:
> 
> OK, so even the linker handling is inconsistent.
> 
> Working (binutils 2.30)
> 
> c0301164 <local_restart>:
> c0301164:       f8d9 a000       ldr.w   sl, [r9]
> c0301168:       e92d 0030       stmdb   sp!, {r4, r5}
> c030116c:       f01a 0ff0       tst.w   sl, #240        ; 0xf0
> c0301170:       d117            bne.n   c03011a2 <__sys_trace>
> c0301172:       46ba            mov     sl, r7
> c0301174:       f5ba 7fc8       cmp.w   sl, #400        ; 0x190
> c0301178:       bf28            it      cs
> c030117a:       f04f 0a00       movcs.w sl, #0
> c030117e:       f3af 8014       csdb
> c0301182:       f2af 1e83       subw    lr, pc, #387    ; 0x183
>                         c0301182: R_ARM_THM_ALU_PREL_11_0       .Lsym28
> 
> 
> Broken (binutils 2.26)
> 
> c0301184 <local_restart>:
> c0301184:       f8d9 a000       ldr.w   sl, [r9]
> c0301188:       e92d 0030       stmdb   sp!, {r4, r5}
> c030118c:       f01a 0ff0       tst.w   sl, #240        ; 0xf0
> c0301190:       d117            bne.n   c03011c2 <__sys_trace>
> c0301192:       46ba            mov     sl, r7
> c0301194:       f5ba 7fc8       cmp.w   sl, #400        ; 0x190
> c0301198:       bf28            it      cs
> c030119a:       f04f 0a00       movcs.w sl, #0
> c030119e:       f3af 8014       csdb
> c03011a2:       f2af 1ea2       subw    lr, pc, #418    ; 0x1a2
>                         c03011a2: R_ARM_THM_ALU_PREL_11_0       .Lsym30
> 
> 
> Note the even immediate in the subw instruction. So this is another
> dead end, unfortunately.
> 
Looks like someone is trying to make things really difficunt :-(.
I think I'll just stick with binutils 2.28.1. Not optimal, but
at least it works.

Something else: I can boot Cortex-M under qemu (-M mps2-an385). The only problem
I have is this:

/ # kill -1 1
[    3.806568] 
[    3.806568] Unhandled exception: IPSR = 00000006 LR = fffffffd
[    3.807221] CPU: 0 PID: 1 Comm: init Not tainted 4.18.0-rc1-00043-gba4dbdedd3ed #42
[    3.807590] Hardware name: MPS2 (Device Tree Support)
[    3.808162] PC is at   (null)
[    3.808374] LR is at 0x2170fc37
[    3.808549] pc : [<00000000>]    lr : [<2170fc37>]    psr: 60000000
[    3.808841] sp : 21761b90  ip : 21761f00  fp : 21758c04
[    3.809118] r10: 00000000  r9 : 00000000  r8 : 00000000
[    3.809329] r7 : 00000000  r6 : 00000001  r5 : 00000000  r4 : 2175452c
[    3.809565] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 00000001
[    3.809791] xPSR: 60000000
[    3.809926] CPU: 0 PID: 1 Comm: init Not tainted 4.18.0-rc1-00043-gba4dbdedd3ed #42
[    3.810179] Hardware name: MPS2 (Device Tree Support)
[    3.811246] [<2100bd8d>] (unwind_backtrace) from [<2100b13b>] (show_stack+0xb/0xc)
[    3.811656] [<2100b13b>] (show_stack) from [<2100b87b>] (__invalid_entry+0x4b/0x4c)

Everything else seems to work, just sending a signal to init causes it
to blow up. Any idea what might cause this ?

Thanks,
Guenter