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

[ard.biesheuvel@linaro.org (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
>> 
>