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