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

[Ard Biesheuvel]

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?

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.





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