Re: [PATCH v3 06/10] arm64: efi: add EFI stub

[Mark Rutland]

Hi Leif,

On Fri, Apr 04, 2014 at 07:45:09PM +0100, Leif Lindholm wrote:
> From: Mark Salter <msalter@xxxxxxxxxx>
>
> This patch adds PE/COFF header fields to the start of the Image
> so that it appears as an EFI application to EFI firmware. An EFI
> stub is included to allow direct booting of the kernel Image.
> Support in the COFF header for signed images was provided by
> Ard Biesheuvel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Signed-off-by: Mark Salter <msalter@xxxxxxxxxx>
> Signed-off-by: Leif Lindholm <leif.lindholm@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Matt Fleming <matt.fleming@xxxxxxxxx>
> ---
>  arch/arm64/Kconfig              |   14 +++
>  arch/arm64/kernel/Makefile      |    3 +
>  arch/arm64/kernel/efi-entry.S   |  100 ++++++++++++++++
>  arch/arm64/kernel/efi-stub.c    |   97 +++++++++++++++
>  arch/arm64/kernel/head.S        |  112 ++++++++++++++++++
>  drivers/firmware/efi/arm-stub.c |  248 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 574 insertions(+)
>  create mode 100644 arch/arm64/kernel/efi-entry.S
>  create mode 100644 arch/arm64/kernel/efi-stub.c
>  create mode 100644 drivers/firmware/efi/arm-stub.c
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index d9f23ad..791ec61 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -280,6 +280,20 @@ config CMDLINE_FORCE
>           This is useful if you cannot or don't want to change the
>           command-line options your boot loader passes to the kernel.
>
> +config EFI
> +       bool "UEFI firmware support"
> +       depends on OF

I note we're not depending on !CPU_BIG_ENDIAN here, and it looks like
the implementation is not endian-clean (I've pointed out a few issues
below).

We need to fix that up for CPU_BIG_ENDIAN. For the moment we could
depend on !CPU_BIG_ENDIAN which would at least to make it clear we don't
support EFI && CPU_BIG_ENDIAN yet. The commit message should be updated
to mention that.

[...]

> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> new file mode 100644
> index 0000000..d99a034e
> --- /dev/null
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -0,0 +1,100 @@
> +/*
> + * EFI entry point.
> + *
> + * Copyright (C) 2013, 2014 Red Hat, Inc.
> + * Author: Mark Salter <msalter@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/linkage.h>
> +#include <linux/init.h>
> +
> +#include <asm/assembler.h>
> +
> +#define EFI_LOAD_ERROR 0x8000000000000001
> +
> +       __INIT
> +
> +       /*
> +        * We arrive here from the EFI boot manager with:
> +        *
> +        *    * MMU on with identity-mapped RAM.
> +        *    * Icache and Dcache on

I assume we always enter with the CPU in little-endian mode? It would be
nice to note that here if so, or otherwise if not

> +        *
> +        * We will most likely be running from some place other than where
> +        * we want to be. The kernel image wants to be placed at TEXT_OFFSET
> +        * from start of RAM.
> +        */
> +ENTRY(efi_stub_entry)
> +       stp     x29, x30, [sp, #-32]!

A comment as to what the additional space is for would be nice. It seems
to be the relocated kernel address and padding for the required
alignment?

[...]

> +/*
> + * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from
> + * start of kernel and may not cross a 2MiB boundary. We set alignment to
> + * 2MiB so we know it won't cross a 2MiB boundary.
> + */

Nit: the booting documentation is poor here, but the actual requirement
is slightly different than that described. We currently need the DTB to
be within the same naturally-aligned 512 MiB region as the kernel,
though that restriction could be lifted with some rework of the initial
page tables.

[...]

>From here on in, all comments are regarding endianness issues and how we
can fix them. If you do not care about BE, stop reading here. ;)

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 1fe5d8d..2aee658 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -107,8 +107,18 @@
>         /*
>          * DO NOT MODIFY. Image header expected by Linux boot-loaders.
>          */
> +#ifdef CONFIG_EFI
> +       /*
> +        * Magic "MZ" signature for PE/COFF
> +        * Little Endian:  add x13, x18, #0x16
> +        */
> +efi_head:
> +       .long   0x91005a4d

As .long will emit the value in native endianness, this will only work
for LE kernels. In BE this will be backwards (which looks to be an
undefined instruction).

> +       b       stext
> +#else
>         b       stext                           // branch to kernel start, magic
>         .long   0                               // reserved
> +#endif
>         .quad   TEXT_OFFSET                     // Image load offset from start of RAM

This also needs to be fixed up endianness wise; I have a series for that
which I'll post shortly.

>         .quad   0                               // reserved
>         .quad   0                               // reserved
> @@ -119,7 +129,109 @@
>         .byte   0x52
>         .byte   0x4d
>         .byte   0x64
> +#ifdef CONFIG_EFI
> +       .long   pe_header - efi_head            // Offset to the PE header.

Likewise this will be backwards. Unfortunately values derived from
calculations involving symbols can't be endian-swapped here due to lack
of a suitable relocation -- we'll have to get the linker to do the
endianness swap for us.

I have a patch enabling the linker to endian-swap such values for us for
64-bit values, but it looks like we'll need other sizes too (for the
shorts below). To prevent vast swathes of symbols appearing in the main
linker script we could have a header to gather those into a
FIXED_ENDIAN_SYMBOLS macro or similar.

> +#else
>         .word   0                               // reserved
> +#endif
> +
> +#ifdef CONFIG_EFI
> +       .align 3
> +pe_header:
> +       .ascii  "PE"
> +       .short  0
> +coff_header:
> +       .short  0xaa64                          // AArch64
> +       .short  2                               // nr_sections
> +       .long   0                               // TimeDateStamp
> +       .long   0                               // PointerToSymbolTable
> +       .long   1                               // NumberOfSymbols

Everything here but the string will be the wrong way around on BE.

We could add .{short,int,long}_le macros to do the endianness swapping
of these values for us (which would also help to document the
endianness).

All other uses of {short,int,long} for non-zero values will need
endianness correction.

> +       .short  section_table - optional_header // SizeOfOptionalHeader

This might need the linker's help to swap unless the assembler resolves
the difference at assemble time.

> +       .long   efi_stub_entry - efi_head       // AddressOfEntryPoint
> +       .long   stext - efi_head                // BaseOfCode

Likewise.

> +       .long   _edata - efi_head               // SizeOfImage

This will definitely require the help of the linker for endianness
swapping.

> +       // Everything before the kernel image is considered part of the header
> +       .long   stext - efi_head                        // SizeOfHeaders

And this.

[...]

> +       .long   _edata - stext          // VirtualSize
> +       .long   stext - efi_head        // VirtualAddress
> +       .long   _edata - stext          // SizeOfRawData
> +       .long   stext - efi_head        // PointerToRawData

And these.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/