[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 at arm.com> wrote:
>>
>>> On 19/05/17 17:59, 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.
>>>
>>> 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
>>>
>>