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

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@linaro.org>
---
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(-)

--
2.45.2

On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
<ilias.apalodimas@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@linaro.org>
> ---

Acked-by: Sughosh Ganu <sughosh.ganu@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
>

Hi Sughosh,

On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> <ilias.apalodimas@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@linaro.org>
> > ---
>
> Acked-by: Sughosh Ganu <sughosh.ganu@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.

Regards,
Simon

(Sorry, that was supposed to be sent to Ilias)

- Simon

On Fri, 29 Nov 2024 at 15:26, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> > <ilias.apalodimas@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@linaro.org>
> > > ---
> >
> > Acked-by: Sughosh Ganu <sughosh.ganu@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.
>
> Regards,
> Simon

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 <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> > > <ilias.apalodimas@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@linaro.org>
> > > > ---
> > >
> > > Acked-by: Sughosh Ganu <sughosh.ganu@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.

Hi Tom

On Sat, 30 Nov 2024 at 05:42, Tom Rini <trini@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 <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> > > > <ilias.apalodimas@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@linaro.org>
> > > > > ---
> > > >
> > > > Acked-by: Sughosh Ganu <sughosh.ganu@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"

Cheers
/Ilias
> --
> Tom

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 <trini@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 <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Fri, 29 Nov 2024 at 10:26, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > On Fri, 29 Nov 2024 at 22:38, Ilias Apalodimas
> > > > > <ilias.apalodimas@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@linaro.org>
> > > > > > ---
> > > > >
> > > > > Acked-by: Sughosh Ganu <sughosh.ganu@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!

On Fri, 29 Nov 2024 19:08:13 +0200, Ilias Apalodimas 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.
> 
> [...]

Applied to u-boot/master, thanks!