On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas <[email protected]> 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/[email protected]/ > Fixes: commit a68c9ac5d8af ("efi_memory: do not add U-Boot memory to the > memory map") > Signed-off-by: Ilias Apalodimas <[email protected]> > ---
Acked-by: Sughosh Ganu <[email protected]> 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 >
