On Sat, Nov 30, 2024 at 08:13:24AM +0200, Ilias Apalodimas wrote:
> Hi Tom
>
> On Sat, 30 Nov 2024 at 05:42, Tom Rini <tr...@konsulko.com> wrote:
> >
> > On Fri, Nov 29, 2024 at 04:27:40PM -0700, Simon Glass wrote:
> > > (Sorry, that was supposed to be sent to Ilias)
> > >
> > > - Simon
> > >
> > > On Fri, 29 Nov 2024 at 15:26, Simon Glass <s...@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.g...@linaro.org>
> > > > wrote:
> > > > >
> > > > > On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> > > > > <ilias.apalodi...@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-...@chromium.org/
> > > > > > Fixes: commit a68c9ac5d8af ("efi_memory: do not add U-Boot memory
> > > > > > to the memory map")
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > > > > > ---
> > > > >
> > > > > Acked-by: Sughosh Ganu <sughosh.g...@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
> > > > > >
> > > >
> > > > Thank you for digging into this. It is helpful to add more things to
> > > > the commit message, but please redo your patch with a 'git revert' so
> > > > we get the correct subject and body, as my patch did.
> >
> > I don't have a preference about the subject, but the body above is
> > correct. It says "This reverts commit ..." and then explains _why_ we
> > need to revert it.
>
> Yes, we agree and although we are in bikeshedding territory now, I
> don't mind sending a new one with a changed subject. Just let me know.
> In any case feel free to merge this as
> Revert "efi_memory: do not add U-Boot memory to the memory map"Talking with Ilias and Simon off-list and I'm doing a few tweaks to the commit message and pushing this shortly, thanks all! -- Tom
signature.asc
Description: PGP signature
