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

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

On 5/14/20 8:27 PM, Heinrich Schuchardt wrote:
> 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);
>>
>

Hello Michael,

your described that on ARMv8 the UEFI runtime must be in a 64k page that
is has to be separate from the 4k page reserved for spin-tables for
certain boards. A change that would achieve this is shown as diff below.
But it has a major impact on image size:

qemu_arm64_defconfig before:
6276656 May 28 18:53 u-boot
 673904 May 28 18:53 u-boot.bin

after:
6338240 May 28 18:51 u-boot
 735368 May 28 18:51 u-boot.bin

Image size is critical on many boards therefore this seems not to be a
good way to go forward.

U-Boot's PSCI has a similar problem to solve. Here the code has been put
into section _secure.text and the data in_secure.data. Function
relocate_secure_section() is used to move the code to the address
specified by CONFIG_ARMV8_SECURE_BASE. I think the same should work for
the spin-tables.

Could you, please, evaluate this idea.

Here is the diff for the solution I would not like to implement:

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index 2554980595..e1ba450937 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -15,6 +15,7 @@ OUTPUT_ARCH(aarch64)
 ENTRY(_start)
 SECTIONS
 {
+       . = ALIGN(65536);
 #ifdef CONFIG_ARMV8_SECURE_BASE
        /DISCARD/ : { *(.rela._secure*) }
 #endif
@@ -36,6 +37,7 @@ SECTIONS
                 __efi_runtime_stop = .;
        }

+       . = ALIGN(65536);
        .text_rest :
        {
                *(.text*)
diff --git a/common/board_f.c b/common/board_f.c
index 01194eaa0e..42d7fdff12 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -438,7 +438,8 @@ static int reserve_uboot(void)
                 */
                gd->relocaddr -= gd->mon_len;
                gd->relocaddr &= ~(4096 - 1);
-       #if defined(CONFIG_E500) || defined(CONFIG_MIPS)
+       #if defined(CONFIG_E500) || defined(CONFIG_MIPS) || \
+           (defined(CONFIG_ARM64) && defined(CONFIG_EFI_LOADER))
                /* round down to next 64 kB limit so that IVPR stays
aligned */
                gd->relocaddr &= ~(65536 - 1);
        #endif

Best regards

Heinrich