[PATCH v2] efi_loader: Add U-Boot memory to the EFI memory map

[Sughosh Ganu]

On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> This reverts commit ("commit a68c9ac5d8af ("efi_memory: do not add U-Boot memory to the memory map")
> This code was removed when the EFI subsystem started using LMB calls for
> the reservations. In hindsight it unearthed two problems.
>
> The e820 code is adding u-boot memory as EfiReservedMemory while it
> should look at what LMB added and decide instead of blindly overwriting
> it. The reason this worked is that we marked that code properly late,
> when the EFI came up. But now with the LMB changes, the EFI map gets
> added first and the e820 code overwrites it.
>
> The second problem is that we never mark SetVirtualAddressMap as runtime
> code, which we should according to the spec. Until we fix this the
> current hack can't go away, at least for architectures that *need* to
> call SVAM.
>
> More specifically x86 currently requires SVAM and sets the NX bit for
> pages not marked as *_CODE. So unless we do that late, it will crash
> trying to execute from non-executable memory. It's also worth noting
> that x86 calls SVAM late in the boot, so this will work until someone
> decides to overwrite/use BootServicesData from the OS.
>
> Notably arm64 disables it explicitly if the VA space is > 48bits, so
> doesn't suffer from any of these problems.
>
> This doesn't really deserve a fixes tag, since it brings back a hack to
> remedy a situation that was wrong long before that commit, but in case
> anyone hits the same bug ...
> Simon sent the original revert in the link, but we need a proper
> justification for it.
>
> Link: https://lore.kernel.org/u-boot/20241112131830.576864-1-sjg@chromium.org/
> Fixes: commit a68c9ac5d8af ("efi_memory: do not add U-Boot memory to the memory map")
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---

Acked-by: Sughosh Ganu <sughosh.ganu at linaro.org>

Like you mention in the commit message, I don't think it warrants a Fixes tag.

-sughosh

> Apologies for sending v2 so fast but we need this in for the release
> Changes since v1:
> - reword the commit message and fix spelling
>
>  lib/efi_loader/efi_memory.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index e493934c7131..edd7da7d8c6e 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -814,7 +814,16 @@ 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 = CONFIG_STACK_SIZE;
> +
> +       /* Add U-Boot */
> +       uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
> +                      uboot_stack_size) & ~EFI_PAGE_MASK;
> +       uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
> +                      uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +       efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_CODE,
> +                             false);
>  #if defined(__aarch64__)
>         /*
>          * Runtime Services must be 64KiB aligned according to the
> --
> 2.45.2
>