From patchwork Thu May 28 17:11:07 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heinrich Schuchardt X-Patchwork-Id: 246782 List-Id: U-Boot discussion From: xypron.glpk at gmx.de (Heinrich Schuchardt) Date: Thu, 28 May 2020 19:11:07 +0200 Subject: [PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb In-Reply-To: <568bafaf-eb68-9cf9-f3dd-21bf9c3d7a2a@gmx.de> References: <20200514123831.30157-1-michael@walle.cc> <20200514123831.30157-2-michael@walle.cc> <568bafaf-eb68-9cf9-f3dd-21bf9c3d7a2a@gmx.de> Message-ID: 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 >> --- >> 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 >> #include >> #include >> -#include >> >> 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: #endif Best regards Heinrich 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);