[PATCH 1/4] efi_loader: aarch64: align runtime section to 64kb
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu May 28 19:11:07 CEST 2020
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 at 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
More information about the U-Boot
mailing list