[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Ard Biesheuvel]

As reported by Patrice, the header layout of the decompressor is
incorrect when building for v7-M. In this case, the __nop macro
resolves to 'mov r0, r0', which is emitted as a narrow encoding,
resulting in the header data fields to end up at lower offsets than
required.

Given the variety of targets we need to support with the same code,
the startup sequence is a bit of a jumble, and uses instructions
and macros whose encoding width cannot be specified (badr), or only
exists in a narrow encoding (bx)

So force the use of a wide encoding in __nop, and replace the start
sequence with a simple jump to the label marking the start of code,
preceded by a Thumb2 mode switch if required (using explicit wide
encodings where appropriate). The label itself can be moved to the
start of code [where it belongs] due to the larger range of branch
instructions as compared to adr instructions.

Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/efi-header.S |  4 +---
 arch/arm/boot/compressed/head.S       | 14 +++++++-------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
index 9d5dc4fda3c1..3f7d1b74c5e0 100644
--- a/arch/arm/boot/compressed/efi-header.S
+++ b/arch/arm/boot/compressed/efi-header.S
@@ -17,14 +17,12 @@
 		@ there.
 		.inst	'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
 #else
-		mov	r0, r0
+		W(mov)	r0, r0
 #endif
 		.endm
 
 		.macro	__EFI_HEADER
 #ifdef CONFIG_EFI_STUB
-		b	__efi_start
-
 		.set	start_offset, __efi_start - start
 		.org	start + 0x3c
 		@
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 7c711ba61417..8c336a6da210 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -130,19 +130,19 @@ start:
 		.rept	7
 		__nop
 		.endr
-   ARM(		mov	r0, r0		)
-   ARM(		b	1f		)
- THUMB(		badr	r12, 1f		)
- THUMB(		bx	r12		)
+      ARM(	b	1f		)
+ AR_CLASS(	sub	pc, pc, #3	)	@ A/R: switch to Thumb2 mode
+  M_CLASS(	nop.w			)	@ M: already in Thumb2 mode
+    THUMB(	.thumb			)
+    THUMB(	b.w	1f		)
 
 		.word	_magic_sig	@ Magic numbers to help the loader
 		.word	_magic_start	@ absolute load/run zImage address
 		.word	_magic_end	@ zImage end address
 		.word	0x04030201	@ endianness flag
 
- THUMB(		.thumb			)
-1:		__EFI_HEADER
-
+		__EFI_HEADER
+1:
  ARM_BE8(	setend	be		)	@ go BE8 if compiled for BE8
  AR_CLASS(	mrs	r9, cpsr	)
 #ifdef CONFIG_ARM_VIRT_EXT
-- 
2.9.3

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Nicolas Pitre]

On Fri, 19 May 2017, Ard Biesheuvel wrote:

> As reported by Patrice, the header layout of the decompressor is
> incorrect when building for v7-M. In this case, the __nop macro
> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
> resulting in the header data fields to end up at lower offsets than
> required.
> 
> Given the variety of targets we need to support with the same code,
> the startup sequence is a bit of a jumble, and uses instructions
> and macros whose encoding width cannot be specified (badr), or only
> exists in a narrow encoding (bx)
> 
> So force the use of a wide encoding in __nop, and replace the start
> sequence with a simple jump to the label marking the start of code,
> preceded by a Thumb2 mode switch if required (using explicit wide
> encodings where appropriate). The label itself can be moved to the
> start of code [where it belongs] due to the larger range of branch
> instructions as compared to adr instructions.
> 
> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Acked-by: Nicolas Pitre <nico@linaro.org>

> ---
>  arch/arm/boot/compressed/efi-header.S |  4 +---
>  arch/arm/boot/compressed/head.S       | 14 +++++++-------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -17,14 +17,12 @@
>  		@ there.
>  		.inst	'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>  #else
> -		mov	r0, r0
> +		W(mov)	r0, r0
>  #endif
>  		.endm
>  
>  		.macro	__EFI_HEADER
>  #ifdef CONFIG_EFI_STUB
> -		b	__efi_start
> -
>  		.set	start_offset, __efi_start - start
>  		.org	start + 0x3c
>  		@
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 7c711ba61417..8c336a6da210 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -130,19 +130,19 @@ start:
>  		.rept	7
>  		__nop
>  		.endr
> -   ARM(		mov	r0, r0		)
> -   ARM(		b	1f		)
> - THUMB(		badr	r12, 1f		)
> - THUMB(		bx	r12		)
> +      ARM(	b	1f		)
> + AR_CLASS(	sub	pc, pc, #3	)	@ A/R: switch to Thumb2 mode
> +  M_CLASS(	nop.w			)	@ M: already in Thumb2 mode
> +    THUMB(	.thumb			)
> +    THUMB(	b.w	1f		)
>  
>  		.word	_magic_sig	@ Magic numbers to help the loader
>  		.word	_magic_start	@ absolute load/run zImage address
>  		.word	_magic_end	@ zImage end address
>  		.word	0x04030201	@ endianness flag
>  
> - THUMB(		.thumb			)
> -1:		__EFI_HEADER
> -
> +		__EFI_HEADER
> +1:
>   ARM_BE8(	setend	be		)	@ go BE8 if compiled for BE8
>   AR_CLASS(	mrs	r9, cpsr	)
>  #ifdef CONFIG_ARM_VIRT_EXT
> -- 
> 2.9.3
> 
> 

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Dave Martin]

On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
> As reported by Patrice, the header layout of the decompressor is
> incorrect when building for v7-M. In this case, the __nop macro
> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
> resulting in the header data fields to end up at lower offsets than
> required.
> 
> Given the variety of targets we need to support with the same code,
> the startup sequence is a bit of a jumble, and uses instructions
> and macros whose encoding width cannot be specified (badr), or only
> exists in a narrow encoding (bx)
> 
> So force the use of a wide encoding in __nop, and replace the start
> sequence with a simple jump to the label marking the start of code,
> preceded by a Thumb2 mode switch if required (using explicit wide
> encodings where appropriate). The label itself can be moved to the
> start of code [where it belongs] due to the larger range of branch
> instructions as compared to adr instructions.
> 
> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/boot/compressed/efi-header.S |  4 +---
>  arch/arm/boot/compressed/head.S       | 14 +++++++-------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
> --- a/arch/arm/boot/compressed/efi-header.S
> +++ b/arch/arm/boot/compressed/efi-header.S
> @@ -17,14 +17,12 @@
>  		@ there.
>  		.inst	'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>  #else
> -		mov	r0, r0
> +		W(mov)	r0, r0
>  #endif
>  		.endm
>  
>  		.macro	__EFI_HEADER
>  #ifdef CONFIG_EFI_STUB
> -		b	__efi_start
> -
>  		.set	start_offset, __efi_start - start
>  		.org	start + 0x3c
>  		@
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 7c711ba61417..8c336a6da210 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -130,19 +130,19 @@ start:
>  		.rept	7
>  		__nop
>  		.endr
> -   ARM(		mov	r0, r0		)
> -   ARM(		b	1f		)
> - THUMB(		badr	r12, 1f		)
> - THUMB(		bx	r12		)
> +      ARM(	b	1f		)
> + AR_CLASS(	sub	pc, pc, #3	)	@ A/R: switch to Thumb2 mode
> +  M_CLASS(	nop.w			)	@ M: already in Thumb2 mode
> +    THUMB(	.thumb			)
> +    THUMB(	b.w	1f		)

It's not so easy to see that this always assembles to two words, or that
the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.

Spelling it out might be more readable:

   ARM(			mov	r0, r0		)
   ARM(			mov	r0, r0		)

 THUMB(	AR_CLASS(	sub	pc, pc, #3	))
 THUMB(	M_CLASS(	nop.w			))
 THUMB(			.thumb			)
 THUMB(			b.w	1f		)

But I guess it works either way.

Cheers
---Dave

>  
>  		.word	_magic_sig	@ Magic numbers to help the loader
>  		.word	_magic_start	@ absolute load/run zImage address
>  		.word	_magic_end	@ zImage end address
>  		.word	0x04030201	@ endianness flag
>  
> - THUMB(		.thumb			)
> -1:		__EFI_HEADER
> -
> +		__EFI_HEADER
> +1:

[...]

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Ard Biesheuvel]

On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
> On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
>> As reported by Patrice, the header layout of the decompressor is
>> incorrect when building for v7-M. In this case, the __nop macro
>> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
>> resulting in the header data fields to end up at lower offsets than
>> required.
>>
>> Given the variety of targets we need to support with the same code,
>> the startup sequence is a bit of a jumble, and uses instructions
>> and macros whose encoding width cannot be specified (badr), or only
>> exists in a narrow encoding (bx)
>>
>> So force the use of a wide encoding in __nop, and replace the start
>> sequence with a simple jump to the label marking the start of code,
>> preceded by a Thumb2 mode switch if required (using explicit wide
>> encodings where appropriate). The label itself can be moved to the
>> start of code [where it belongs] due to the larger range of branch
>> instructions as compared to adr instructions.
>>
>> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm/boot/compressed/efi-header.S |  4 +---
>>  arch/arm/boot/compressed/head.S       | 14 +++++++-------
>>  2 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
>> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
>> --- a/arch/arm/boot/compressed/efi-header.S
>> +++ b/arch/arm/boot/compressed/efi-header.S
>> @@ -17,14 +17,12 @@
>>               @ there.
>>               .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>>  #else
>> -             mov     r0, r0
>> +             W(mov)  r0, r0
>>  #endif
>>               .endm
>>
>>               .macro  __EFI_HEADER
>>  #ifdef CONFIG_EFI_STUB
>> -             b       __efi_start
>> -
>>               .set    start_offset, __efi_start - start
>>               .org    start + 0x3c
>>               @
>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>> index 7c711ba61417..8c336a6da210 100644
>> --- a/arch/arm/boot/compressed/head.S
>> +++ b/arch/arm/boot/compressed/head.S
>> @@ -130,19 +130,19 @@ start:
>>               .rept   7
>>               __nop
>>               .endr
>> -   ARM(              mov     r0, r0          )
>> -   ARM(              b       1f              )
>> - THUMB(              badr    r12, 1f         )
>> - THUMB(              bx      r12             )
>> +      ARM(   b       1f              )
>> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
>> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
>> +    THUMB(   .thumb                  )
>> +    THUMB(   b.w     1f              )
>
> It's not so easy to see that this always assembles to two words, or that
> the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
>
> Spelling it out might be more readable:
>
>    ARM(                 mov     r0, r0          )
>    ARM(                 mov     r0, r0          )
>
>  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
>  THUMB( M_CLASS(        nop.w                   ))
>  THUMB(                 .thumb                  )
>  THUMB(                 b.w     1f              )
>
> But I guess it works either way.
>

Indeed. Apart from the error in the second line, this sequence should
be equivalent, but the nested macro invocations don't make it clearer
imo.

I could add some more comments as well if that helps?

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Russell King - ARM Linux]

On Fri, May 19, 2017 at 05:59:47PM +0100, Ard Biesheuvel wrote:
> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
> >> As reported by Patrice, the header layout of the decompressor is
> >> incorrect when building for v7-M. In this case, the __nop macro
> >> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
> >> resulting in the header data fields to end up at lower offsets than
> >> required.
> >>
> >> Given the variety of targets we need to support with the same code,
> >> the startup sequence is a bit of a jumble, and uses instructions
> >> and macros whose encoding width cannot be specified (badr), or only
> >> exists in a narrow encoding (bx)
> >>
> >> So force the use of a wide encoding in __nop, and replace the start
> >> sequence with a simple jump to the label marking the start of code,
> >> preceded by a Thumb2 mode switch if required (using explicit wide
> >> encodings where appropriate). The label itself can be moved to the
> >> start of code [where it belongs] due to the larger range of branch
> >> instructions as compared to adr instructions.
> >>
> >> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm/boot/compressed/efi-header.S |  4 +---
> >>  arch/arm/boot/compressed/head.S       | 14 +++++++-------
> >>  2 files changed, 8 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
> >> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
> >> --- a/arch/arm/boot/compressed/efi-header.S
> >> +++ b/arch/arm/boot/compressed/efi-header.S
> >> @@ -17,14 +17,12 @@
> >>               @ there.
> >>               .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
> >>  #else
> >> -             mov     r0, r0
> >> +             W(mov)  r0, r0
> >>  #endif
> >>               .endm
> >>
> >>               .macro  __EFI_HEADER
> >>  #ifdef CONFIG_EFI_STUB
> >> -             b       __efi_start
> >> -
> >>               .set    start_offset, __efi_start - start
> >>               .org    start + 0x3c
> >>               @
> >> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> >> index 7c711ba61417..8c336a6da210 100644
> >> --- a/arch/arm/boot/compressed/head.S
> >> +++ b/arch/arm/boot/compressed/head.S
> >> @@ -130,19 +130,19 @@ start:
> >>               .rept   7
> >>               __nop
> >>               .endr
> >> -   ARM(              mov     r0, r0          )
> >> -   ARM(              b       1f              )
> >> - THUMB(              badr    r12, 1f         )
> >> - THUMB(              bx      r12             )
> >> +      ARM(   b       1f              )
> >> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
> >> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
> >> +    THUMB(   .thumb                  )
> >> +    THUMB(   b.w     1f              )
> >
> > It's not so easy to see that this always assembles to two words, or that
> > the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
> >
> > Spelling it out might be more readable:
> >
> >    ARM(                 mov     r0, r0          )
> >    ARM(                 mov     r0, r0          )
> >
> >  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
> >  THUMB( M_CLASS(        nop.w                   ))
> >  THUMB(                 .thumb                  )
> >  THUMB(                 b.w     1f              )
> >
> > But I guess it works either way.
> >
> 
> Indeed. Apart from the error in the second line, this sequence should
> be equivalent, but the nested macro invocations don't make it clearer
> imo.

If we're getting to this level of complexity, then maybe going back to
ifdefs will help rather than nesting the magic macros:

#ifndef CONFIG_THUMB2_KERNEL
		mov	r0, r0
		b	1f
#else
 AR_CLASS(	sub.w	pc, pc, #3	)
  M_CLASS(	nop.w			)
		.thumb
		b.w	1f
#endif

This looks pretty clear to me.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Ard Biesheuvel]

> On 19 May 2017, at 18:08, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> 
>> On Fri, May 19, 2017 at 05:59:47PM +0100, Ard Biesheuvel wrote:
>>> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
>>>> On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
>>>> As reported by Patrice, the header layout of the decompressor is
>>>> incorrect when building for v7-M. In this case, the __nop macro
>>>> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
>>>> resulting in the header data fields to end up at lower offsets than
>>>> required.
>>>> 
>>>> Given the variety of targets we need to support with the same code,
>>>> the startup sequence is a bit of a jumble, and uses instructions
>>>> and macros whose encoding width cannot be specified (badr), or only
>>>> exists in a narrow encoding (bx)
>>>> 
>>>> So force the use of a wide encoding in __nop, and replace the start
>>>> sequence with a simple jump to the label marking the start of code,
>>>> preceded by a Thumb2 mode switch if required (using explicit wide
>>>> encodings where appropriate). The label itself can be moved to the
>>>> start of code [where it belongs] due to the larger range of branch
>>>> instructions as compared to adr instructions.
>>>> 
>>>> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> arch/arm/boot/compressed/efi-header.S |  4 +---
>>>> arch/arm/boot/compressed/head.S       | 14 +++++++-------
>>>> 2 files changed, 8 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
>>>> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>> @@ -17,14 +17,12 @@
>>>>              @ there.
>>>>              .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>>>> #else
>>>> -             mov     r0, r0
>>>> +             W(mov)  r0, r0
>>>> #endif
>>>>              .endm
>>>> 
>>>>              .macro  __EFI_HEADER
>>>> #ifdef CONFIG_EFI_STUB
>>>> -             b       __efi_start
>>>> -
>>>>              .set    start_offset, __efi_start - start
>>>>              .org    start + 0x3c
>>>>              @
>>>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>>>> index 7c711ba61417..8c336a6da210 100644
>>>> --- a/arch/arm/boot/compressed/head.S
>>>> +++ b/arch/arm/boot/compressed/head.S
>>>> @@ -130,19 +130,19 @@ start:
>>>>              .rept   7
>>>>              __nop
>>>>              .endr
>>>> -   ARM(              mov     r0, r0          )
>>>> -   ARM(              b       1f              )
>>>> - THUMB(              badr    r12, 1f         )
>>>> - THUMB(              bx      r12             )
>>>> +      ARM(   b       1f              )
>>>> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
>>>> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
>>>> +    THUMB(   .thumb                  )
>>>> +    THUMB(   b.w     1f              )
>>> 
>>> It's not so easy to see that this always assembles to two words, or that
>>> the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
>>> 
>>> Spelling it out might be more readable:
>>> 
>>>   ARM(                 mov     r0, r0          )
>>>   ARM(                 mov     r0, r0          )
>>> 
>>> THUMB( AR_CLASS(       sub     pc, pc, #3      ))
>>> THUMB( M_CLASS(        nop.w                   ))
>>> THUMB(                 .thumb                  )
>>> THUMB(                 b.w     1f              )
>>> 
>>> But I guess it works either way.
>>> 
>> 
>> Indeed. Apart from the error in the second line, this sequence should
>> be equivalent, but the nested macro invocations don't make it clearer
>> imo.
> 
> If we're getting to this level of complexity, then maybe going back to
> ifdefs will help rather than nesting the magic macros:
> 
> #ifndef CONFIG_THUMB2_KERNEL
>        mov    r0, r0
>        b    1f
> #else
> AR_CLASS(    sub.w    pc, pc, #3    )
>  M_CLASS(    nop.w            )
>        .thumb
>        b.w    1f
> #endif
> 
> This looks pretty clear to me.

Good point -- let me use that for v2

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Robin Murphy]

On 19/05/17 17:59, Ard Biesheuvel wrote:
> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
>> On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
>>> As reported by Patrice, the header layout of the decompressor is
>>> incorrect when building for v7-M. In this case, the __nop macro
>>> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
>>> resulting in the header data fields to end up at lower offsets than
>>> required.
>>>
>>> Given the variety of targets we need to support with the same code,
>>> the startup sequence is a bit of a jumble, and uses instructions
>>> and macros whose encoding width cannot be specified (badr), or only
>>> exists in a narrow encoding (bx)
>>>
>>> So force the use of a wide encoding in __nop, and replace the start
>>> sequence with a simple jump to the label marking the start of code,
>>> preceded by a Thumb2 mode switch if required (using explicit wide
>>> encodings where appropriate). The label itself can be moved to the
>>> start of code [where it belongs] due to the larger range of branch
>>> instructions as compared to adr instructions.
>>>
>>> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/arm/boot/compressed/efi-header.S |  4 +---
>>>  arch/arm/boot/compressed/head.S       | 14 +++++++-------
>>>  2 files changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
>>> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
>>> --- a/arch/arm/boot/compressed/efi-header.S
>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>> @@ -17,14 +17,12 @@
>>>               @ there.
>>>               .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>>>  #else
>>> -             mov     r0, r0
>>> +             W(mov)  r0, r0
>>>  #endif
>>>               .endm
>>>
>>>               .macro  __EFI_HEADER
>>>  #ifdef CONFIG_EFI_STUB
>>> -             b       __efi_start
>>> -
>>>               .set    start_offset, __efi_start - start
>>>               .org    start + 0x3c
>>>               @
>>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>>> index 7c711ba61417..8c336a6da210 100644
>>> --- a/arch/arm/boot/compressed/head.S
>>> +++ b/arch/arm/boot/compressed/head.S
>>> @@ -130,19 +130,19 @@ start:
>>>               .rept   7
>>>               __nop
>>>               .endr
>>> -   ARM(              mov     r0, r0          )
>>> -   ARM(              b       1f              )
>>> - THUMB(              badr    r12, 1f         )
>>> - THUMB(              bx      r12             )
>>> +      ARM(   b       1f              )
>>> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
>>> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
>>> +    THUMB(   .thumb                  )
>>> +    THUMB(   b.w     1f              )
>>
>> It's not so easy to see that this always assembles to two words, or that
>> the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
>>
>> Spelling it out might be more readable:
>>
>>    ARM(                 mov     r0, r0          )
>>    ARM(                 mov     r0, r0          )
>>
>>  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
>>  THUMB( M_CLASS(        nop.w                   ))
>>  THUMB(                 .thumb                  )
>>  THUMB(                 b.w     1f              )
>>
>> But I guess it works either way.
>>
> 
> Indeed. Apart from the error in the second line, this sequence should
> be equivalent, but the nested macro invocations don't make it clearer
> imo.
> 
> I could add some more comments as well if that helps?

Hmm, couldn't it be ISA-agnostic?

	ldr pc, 1f
	.ltorg
THUMB(	.thumb	)
	...

given that that should be interworking on anything where THUMB() is
valid in the first place.

Robin.

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

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Dave Martin]

On Fri, May 19, 2017 at 05:59:47PM +0100, Ard Biesheuvel wrote:
> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:

[...]

> > Spelling it out might be more readable:
> >
> >    ARM(                 mov     r0, r0          )
> >    ARM(                 mov     r0, r0          )
> >
> >  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
> >  THUMB( M_CLASS(        nop.w                   ))
> >  THUMB(                 .thumb                  )
> >  THUMB(                 b.w     1f              )
> >
> > But I guess it works either way.
> >
> 
> Indeed. Apart from the error in the second line, this sequence should
> be equivalent, but the nested macro invocations don't make it clearer
> imo.

Err, yes.  Actually I means to write

	b	1f
	mov	r0, r0

(since executing that nop before branching is presumably pointless).

Cheers
---Dave

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Ard Biesheuvel]

> On 19 May 2017, at 18:16, Robin Murphy <robin.murphy@arm.com> wrote:
> 
>> On 19/05/17 17:59, Ard Biesheuvel wrote:
>>> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
>>>> On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
>>>> As reported by Patrice, the header layout of the decompressor is
>>>> incorrect when building for v7-M. In this case, the __nop macro
>>>> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
>>>> resulting in the header data fields to end up at lower offsets than
>>>> required.
>>>> 
>>>> Given the variety of targets we need to support with the same code,
>>>> the startup sequence is a bit of a jumble, and uses instructions
>>>> and macros whose encoding width cannot be specified (badr), or only
>>>> exists in a narrow encoding (bx)
>>>> 
>>>> So force the use of a wide encoding in __nop, and replace the start
>>>> sequence with a simple jump to the label marking the start of code,
>>>> preceded by a Thumb2 mode switch if required (using explicit wide
>>>> encodings where appropriate). The label itself can be moved to the
>>>> start of code [where it belongs] due to the larger range of branch
>>>> instructions as compared to adr instructions.
>>>> 
>>>> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> arch/arm/boot/compressed/efi-header.S |  4 +---
>>>> arch/arm/boot/compressed/head.S       | 14 +++++++-------
>>>> 2 files changed, 8 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
>>>> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>> @@ -17,14 +17,12 @@
>>>>              @ there.
>>>>              .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>>>> #else
>>>> -             mov     r0, r0
>>>> +             W(mov)  r0, r0
>>>> #endif
>>>>              .endm
>>>> 
>>>>              .macro  __EFI_HEADER
>>>> #ifdef CONFIG_EFI_STUB
>>>> -             b       __efi_start
>>>> -
>>>>              .set    start_offset, __efi_start - start
>>>>              .org    start + 0x3c
>>>>              @
>>>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>>>> index 7c711ba61417..8c336a6da210 100644
>>>> --- a/arch/arm/boot/compressed/head.S
>>>> +++ b/arch/arm/boot/compressed/head.S
>>>> @@ -130,19 +130,19 @@ start:
>>>>              .rept   7
>>>>              __nop
>>>>              .endr
>>>> -   ARM(              mov     r0, r0          )
>>>> -   ARM(              b       1f              )
>>>> - THUMB(              badr    r12, 1f         )
>>>> - THUMB(              bx      r12             )
>>>> +      ARM(   b       1f              )
>>>> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
>>>> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
>>>> +    THUMB(   .thumb                  )
>>>> +    THUMB(   b.w     1f              )
>>> 
>>> It's not so easy to see that this always assembles to two words, or that
>>> the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
>>> 
>>> Spelling it out might be more readable:
>>> 
>>>   ARM(                 mov     r0, r0          )
>>>   ARM(                 mov     r0, r0          )
>>> 
>>> THUMB( AR_CLASS(       sub     pc, pc, #3      ))
>>> THUMB( M_CLASS(        nop.w                   ))
>>> THUMB(                 .thumb                  )
>>> THUMB(                 b.w     1f              )
>>> 
>>> But I guess it works either way.
>>> 
>> 
>> Indeed. Apart from the error in the second line, this sequence should
>> be equivalent, but the nested macro invocations don't make it clearer
>> imo.
>> 
>> I could add some more comments as well if that helps?
> 
> Hmm, couldn't it be ISA-agnostic?
> 
>    ldr pc, 1f
>    .ltorg
> THUMB(    .thumb    )
>    ...
> 
> given that that should be interworking on anything where THUMB() is
> valid in the first place.
> 

The decompressor runs at a a priori unknown offset so literals containing absolute values are not helpful here.

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

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Dave Martin]

On Fri, May 19, 2017 at 06:16:36PM +0100, Robin Murphy wrote:
> On 19/05/17 17:59, Ard Biesheuvel wrote:
> > On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:

[...]

> >> Spelling it out might be more readable:
> >>
> >>    ARM(                 mov     r0, r0          )
> >>    ARM(                 mov     r0, r0          )
> >>
> >>  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
> >>  THUMB( M_CLASS(        nop.w                   ))
> >>  THUMB(                 .thumb                  )
> >>  THUMB(                 b.w     1f              )
> >>
> >> But I guess it works either way.
> >>
> > 
> > Indeed. Apart from the error in the second line, this sequence should
> > be equivalent, but the nested macro invocations don't make it clearer
> > imo.
> > 
> > I could add some more comments as well if that helps?
> 
> Hmm, couldn't it be ISA-agnostic?
> 
> 	ldr pc, 1f
> 	.ltorg
> THUMB(	.thumb	)
> 	...
> 
> given that that should be interworking on anything where THUMB() is
> valid in the first place.

Did you mean

	ldr	pc, =BSYM(1f)
?

Otherwise this is quite neat: providing there are no pending literals
when the assembler reaches the ldr pc, and providing that the ldr pc is
word-aligned (it certainly should be) then that sequence should always
expand to two words.

Cheers
---Dave

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Russell King - ARM Linux]

On Fri, May 19, 2017 at 06:29:34PM +0100, Dave Martin wrote:
> On Fri, May 19, 2017 at 05:59:47PM +0100, Ard Biesheuvel wrote:
> > On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
> 
> [...]
> 
> > > Spelling it out might be more readable:
> > >
> > >    ARM(                 mov     r0, r0          )
> > >    ARM(                 mov     r0, r0          )
> > >
> > >  THUMB( AR_CLASS(       sub     pc, pc, #3      ))
> > >  THUMB( M_CLASS(        nop.w                   ))
> > >  THUMB(                 .thumb                  )
> > >  THUMB(                 b.w     1f              )
> > >
> > > But I guess it works either way.
> > >
> > 
> > Indeed. Apart from the error in the second line, this sequence should
> > be equivalent, but the nested macro invocations don't make it clearer
> > imo.
> 
> Err, yes.  Actually I means to write
> 
> 	b	1f
> 	mov	r0, r0
> 
> (since executing that nop before branching is presumably pointless).

That "nop" is part of the 8 nops at the start of the image, since the
entry point (due to historical reasons) is at 0x20 into the image.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Russell King - ARM Linux]

On Fri, May 19, 2017 at 06:16:36PM +0100, Robin Murphy wrote:
> On 19/05/17 17:59, Ard Biesheuvel wrote:
> > Indeed. Apart from the error in the second line, this sequence should
> > be equivalent, but the nested macro invocations don't make it clearer
> > imo.
> > 
> > I could add some more comments as well if that helps?
> 
> Hmm, couldn't it be ISA-agnostic?
> 
> 	ldr pc, 1f
> 	.ltorg
> THUMB(	.thumb	)
> 	...
> 
> given that that should be interworking on anything where THUMB() is
> valid in the first place.

Unfortunately not, this is PIC code, so we must not have anything that
contains an absolute address.  LDR to branch is out of the question.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Dave Martin]

On Fri, May 19, 2017 at 06:35:03PM +0100, Russell King - ARM Linux wrote:
> On Fri, May 19, 2017 at 06:29:34PM +0100, Dave Martin wrote:

[...]

> > Err, yes.  Actually I means to write
> > 
> > 	b	1f
> > 	mov	r0, r0
> > 
> > (since executing that nop before branching is presumably pointless).
> 
> That "nop" is part of the 8 nops at the start of the image, since the
> entry point (due to historical reasons) is at 0x20 into the image.

Ah yes.

Cheers
---Dave

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Dave Martin]

On Fri, May 19, 2017 at 06:34:01PM +0100, Dave Martin wrote:
> On Fri, May 19, 2017 at 06:16:36PM +0100, Robin Murphy wrote:

[...]

> > Hmm, couldn't it be ISA-agnostic?
> > 
> > 	ldr pc, 1f
> > 	.ltorg
> > THUMB(	.thumb	)
> > 	...
> > 
> > given that that should be interworking on anything where THUMB() is
> > valid in the first place.
> 
> Did you mean
> 
> 	ldr	pc, =BSYM(1f)
> ?
> 
> Otherwise this is quite neat: providing there are no pending literals
> when the assembler reaches the ldr pc, and providing that the ldr pc is
> word-aligned (it certainly should be) then that sequence should always
> expand to two words.

Modulo Ard's comment about position-independence...  Curses.

Cheers
---Dave

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Robin Murphy]

On 19/05/17 18:33, Ard Biesheuvel wrote:
> 
> 
>> On 19 May 2017, at 18:16, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>>> On 19/05/17 17:59, Ard Biesheuvel wrote:
>>>> On 19 May 2017 at 17:46, Dave Martin <Dave.Martin@arm.com> wrote:
>>>>> On Fri, May 19, 2017 at 03:59:59PM +0100, Ard Biesheuvel wrote:
>>>>> As reported by Patrice, the header layout of the decompressor is
>>>>> incorrect when building for v7-M. In this case, the __nop macro
>>>>> resolves to 'mov r0, r0', which is emitted as a narrow encoding,
>>>>> resulting in the header data fields to end up at lower offsets than
>>>>> required.
>>>>>
>>>>> Given the variety of targets we need to support with the same code,
>>>>> the startup sequence is a bit of a jumble, and uses instructions
>>>>> and macros whose encoding width cannot be specified (badr), or only
>>>>> exists in a narrow encoding (bx)
>>>>>
>>>>> So force the use of a wide encoding in __nop, and replace the start
>>>>> sequence with a simple jump to the label marking the start of code,
>>>>> preceded by a Thumb2 mode switch if required (using explicit wide
>>>>> encodings where appropriate). The label itself can be moved to the
>>>>> start of code [where it belongs] due to the larger range of branch
>>>>> instructions as compared to adr instructions.
>>>>>
>>>>> Reported-by: Patrice CHOTARD <patrice.chotard@st.com>
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>> arch/arm/boot/compressed/efi-header.S |  4 +---
>>>>> arch/arm/boot/compressed/head.S       | 14 +++++++-------
>>>>> 2 files changed, 8 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S b/arch/arm/boot/compressed/efi-header.S
>>>>> index 9d5dc4fda3c1..3f7d1b74c5e0 100644
>>>>> --- a/arch/arm/boot/compressed/efi-header.S
>>>>> +++ b/arch/arm/boot/compressed/efi-header.S
>>>>> @@ -17,14 +17,12 @@
>>>>>              @ there.
>>>>>              .inst   'M' | ('Z' << 8) | (0x1310 << 16)   @ tstne r0, #0x4d000
>>>>> #else
>>>>> -             mov     r0, r0
>>>>> +             W(mov)  r0, r0
>>>>> #endif
>>>>>              .endm
>>>>>
>>>>>              .macro  __EFI_HEADER
>>>>> #ifdef CONFIG_EFI_STUB
>>>>> -             b       __efi_start
>>>>> -
>>>>>              .set    start_offset, __efi_start - start
>>>>>              .org    start + 0x3c
>>>>>              @
>>>>> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>>>>> index 7c711ba61417..8c336a6da210 100644
>>>>> --- a/arch/arm/boot/compressed/head.S
>>>>> +++ b/arch/arm/boot/compressed/head.S
>>>>> @@ -130,19 +130,19 @@ start:
>>>>>              .rept   7
>>>>>              __nop
>>>>>              .endr
>>>>> -   ARM(              mov     r0, r0          )
>>>>> -   ARM(              b       1f              )
>>>>> - THUMB(              badr    r12, 1f         )
>>>>> - THUMB(              bx      r12             )
>>>>> +      ARM(   b       1f              )
>>>>> + AR_CLASS(   sub     pc, pc, #3      )       @ A/R: switch to Thumb2 mode
>>>>> +  M_CLASS(   nop.w                   )       @ M: already in Thumb2 mode
>>>>> +    THUMB(   .thumb                  )
>>>>> +    THUMB(   b.w     1f              )
>>>>
>>>> It's not so easy to see that this always assembles to two words, or that
>>>> the "sub pc, pc, #3" is assembled but not executed in an ARM kernel.
>>>>
>>>> Spelling it out might be more readable:
>>>>
>>>>   ARM(                 mov     r0, r0          )
>>>>   ARM(                 mov     r0, r0          )
>>>>
>>>> THUMB( AR_CLASS(       sub     pc, pc, #3      ))
>>>> THUMB( M_CLASS(        nop.w                   ))
>>>> THUMB(                 .thumb                  )
>>>> THUMB(                 b.w     1f              )
>>>>
>>>> But I guess it works either way.
>>>>
>>>
>>> Indeed. Apart from the error in the second line, this sequence should
>>> be equivalent, but the nested macro invocations don't make it clearer
>>> imo.
>>>
>>> I could add some more comments as well if that helps?
>>
>> Hmm, couldn't it be ISA-agnostic?
>>
>>    ldr pc, 1f
>>    .ltorg
>> THUMB(    .thumb    )
>>    ...
>>
>> given that that should be interworking on anything where THUMB() is
>> valid in the first place.
>>
> 
> The decompressor runs at a a priori unknown offset so literals containing absolute values are not helpful here.

Ah, I knew I'd have overlooked something - that makes sense.

FWIW, my final thought is this:

	add	r12, pc, #5
	mov	r0, r0
	mov	pc, r12

Which I think should leave you in Thumb regardless of which ISA it's
assembled in, thanks to the wackiness of ALUWritePC().

Robin.

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

[PATCH] arm: boot/compressed: fix decompressor header layout for v7-M

[Russell King - ARM Linux]

On Fri, May 19, 2017 at 06:58:16PM +0100, Robin Murphy wrote:
> Ah, I knew I'd have overlooked something - that makes sense.
> 
> FWIW, my final thought is this:
> 
> 	add	r12, pc, #5
> 	mov	r0, r0
> 	mov	pc, r12
> 
> Which I think should leave you in Thumb regardless of which ISA it's
> assembled in, thanks to the wackiness of ALUWritePC().

You only have four bytes to fit whatever code in here.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.