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 <mich...@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);
>
