[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 at 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 at 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 at st.com>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at 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