[PATCH 4/4] arm64: head: tidy up the Image header definition

[Robin Murphy]

On 2020-10-29 07:30, Ard Biesheuvel wrote:
> On Wed, 28 Oct 2020 at 18:56, Robin Murphy <robin.murphy at arm.com> wrote:
>>
>> On 2020-10-28 14:17, Will Deacon wrote:
>>> On Tue, Oct 27, 2020 at 08:32:09AM +0100, Ard Biesheuvel wrote:
>>>> Even though support for EFI boot remains entirely optional for arm64,
>>>> it is unlikely that we will ever be able to repurpose the image header
>>>> fields that the EFI loader relies on, i.e., the magic NOP at offset
>>>> 0x0 and the PE header address at offset 0x3c.
>>>>
>>>> So let's factor out the differences into a 'magic_nop' macro and a local
>>>> symbol representing the PE header address, and move the conditional
>>>> definitions into efi-header.S, taking into account whether CONFIG_EFI is
>>>> enabled or not.
>>>
>>> How many architectures can claim to have both a "magic nop" and a
>>> "mysterious nop", hey?
>>
>> It's fun 'n'all, but putting my serious hat on for a moment, if the name
>> still requires a comment to explain it at point of use, it's not a very
>> good name :(
>>
>> At worst magic_nop is even potentially misleading, since it's not
>> necessarily a nop; there's no mention of the implicit dependency on a
>> context where the side-effect of executing it wouldn't affect anything
>> important.
>>
>> Could we call the macro itself something clear and self-explanatory like
>> efi_signature_insn please? I'm happy for it to be *commented* as "Magic
>> NOP" if you want parity with the VDSO :D
>>
> 
> Will efi_pseudo_nop do?

Again, what's the defining significance of the instruction that this 
macro stands for - that it does nothing; that it does pseudo-nothing; or 
that it has a specific signature encoding? I know this probably sounds 
like bikeshedding to most, but I firmly believe that good, accurate 
names really do matter :)

> Also, do you think it would be better to use an opcode here that has
> no architectural side effects, such as a PRFM (literal) instruction?
> It is obviously not going to make a difference in practice, but it
> always annoyed me that the pseudo NOP is not a NOP.

Yeah, it's a shame there's no way to get a guaranteed non-taken 
conditional branch in A64, and nearly every good candidate for a 
non-destructive operation with an arbitrary immediate seems to rely on 
an rt=31 encoding... 'prfm PLIL3STRM, . + 2888' is utterly impenetrable, 
but should indeed work; 'ccmp x18, #0, #0xd, pl' is probably the least 
destructive ALU option (only a chance of changing the flags).

Robin.

>>>> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>>>> ---
>>>>    arch/arm64/kernel/efi-header.S | 43 +++++++++++++++-----
>>>>    arch/arm64/kernel/head.S       | 19 +--------
>>>>    2 files changed, 35 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-header.S
>>>> index ddaf57d825b5..7b7ac4316d95 100644
>>>> --- a/arch/arm64/kernel/efi-header.S
>>>> +++ b/arch/arm64/kernel/efi-header.S
>>>> @@ -7,7 +7,27 @@
>>>>    #include <linux/pe.h>
>>>>    #include <linux/sizes.h>
>>>>
>>>> +    .macro  magic_nop
>>>> +#ifdef CONFIG_EFI
>>>> +.L_head:
>>>> +    /*
>>>> +     * This add instruction has no meaningful effect except that
>>>> +     * its opcode forms the magic "MZ" signature required by UEFI.
>>>> +     */
>>>> +    add     x13, x18, #0x16
>>>
>>> It's probably faster too ;)
>>>
>>>> +#else
>>>> +    /*
>>>> +     * Bootloaders may inspect the opcode at the start of the kernel
>>>> +     * image to decide if the kernel is capable of booting via UEFI.
>>>> +     * So put an ordinary NOP here, not the "MZ.." pseudo-nop above.
>>>> +     */
>>>> +    nop
>>>
>>> Let's just hope nobody was decoding the branch instruction...
>>>
>>> Acked-by: Will Deacon >will at kernel.org>
>>>
>>> Will
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>