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

[Ard Biesheuvel]

On Thu, 29 Oct 2020 at 14:07, Robin Murphy <robin.murphy at arm.com> wrote:
>
> 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 :)
>

Fair enough. But by that reasoning, putting NOP in the name is
important, given that efi_signature_insn does not explain what the
instruction does.

So, efi_signature_nop then?

> > 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).
>

I think ccmp is probably a better choice, given that PRFM PLI
instructions issued with the MMU off are more likely to trigger
something unexpected.




>
> >>>> 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
> >>>