[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 5/14/20 2:38 PM, Michael Walle wrote:
> Commit 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> already aligned the memory region to 64kb, but it does not align the
> actual efi runtime code. Thus it is likely, that efi_add_memory_map()
> actually adds a larger memory region than the efi runtime code really
> is, which is no error I guess. But what actually leads to an error is
> that there might be other efi_add_memory_map() calls with regions
> overlapping with the already registered efi runtime code section.

Do you relate to this sentence:

"If a 64KiB physical page contains any 4KiB page with any of the
following types listed below, then all 4KiB pages in the 64KiB page must
use identical ARM Memory Page Attributes"?


>
> Align the actual runtime code to 64kb instead.
>
> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  arch/arm/cpu/armv8/u-boot.lds |  9 ++++++++-
>  lib/efi_loader/efi_memory.c   | 15 ++-------------
>  2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index 2554980595..3bc4675586 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -27,7 +27,14 @@ SECTIONS
>  		CPUDIR/start.o (.text*)
>  	}
>
> -	/* This needs to come before *(.text*) */
> +	/*
> +	 * Runtime Services must be 64KiB aligned according to the
> +	 * "AArch64 Platforms" section in the UEFI spec (2.7+).

This is not the exact requirement of the spec. Please, use a description
that matches the spec.

The requirement that 64KiB areas should have the same attributes was
already presen in UEFI spec 2.4. Please, drop the version reference.

> +	 *
> +	 * This needs to come before *(.text*)
> +	 */
> +
> +	. = ALIGN(65536);

Isn't this an alignment before relocation that is irrelevant with
regards to the UEFI spec?

>  	.efi_runtime : {
>                  __efi_runtime_start = .;
>  		*(.text.efi_runtime*)
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 97d90f069a..fd79178da9 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -12,7 +12,6 @@
>  #include <mapmem.h>
>  #include <watchdog.h>
>  #include <linux/list_sort.h>
> -#include <linux/sizes.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -734,7 +733,6 @@ __weak void efi_add_known_memory(void)
>  static void add_u_boot_and_runtime(void)
>  {
>  	unsigned long runtime_start, runtime_end, runtime_pages;
> -	unsigned long runtime_mask = EFI_PAGE_MASK;
>  	unsigned long uboot_start, uboot_pages;
>  	unsigned long uboot_stack_size = 16 * 1024 * 1024;
>
> @@ -745,22 +743,13 @@ static void add_u_boot_and_runtime(void)
>  		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>  	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
>
> -#if defined(__aarch64__)
> -	/*
> -	 * Runtime Services must be 64KiB aligned according to the
> -	 * "AArch64 Platforms" section in the UEFI spec (2.7+).> -	 */
> -
> -	runtime_mask = SZ_64K - 1;
> -#endif
> -
>  	/*
>  	 * Add Runtime Services. We mark surrounding boottime code as runtime as
>  	 * well to fulfill the runtime alignment constraints but avoid padding.
>  	 */
> -	runtime_start = (ulong)&__efi_runtime_start & ~runtime_mask;
> +	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
>  	runtime_end = (ulong)&__efi_runtime_stop;
> -	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
> +	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;

I cannot see that after these changes you match the requirements of the
UEFI spec.

Best regards

Heinrich

>  	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
>  	efi_add_memory_map(runtime_start, runtime_pages,
>  			   EFI_RUNTIME_SERVICES_CODE, false);
>