[PATCH v4 0/5] Adjust initial EFI memory-allocation to be in the U-Boot region

[Simon Glass]

Hi Ilias,

On Tue, 15 Oct 2024 at 07:32, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Tue, 15 Oct 2024 at 16:26, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, 12 Oct 2024 at 00:23, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > U-Boot relocates itself to the top of memory and is supposed to avoid
> > > > using any memory below that region. This allows the maximum amount of
> > > > memory to be available for loading images.
> > > >
> > > > The lmb subsystem automatically protects U-Boot's code and data from
> > > > being overwritten. It relies on this region being a contiguous area of
> > > > memory. This setup has worked well for at least 15 years and has been
> > > > documented in the README for as long.
> > > >
> > > > The EFI_LOADER subsystem currently uses memory outside the U-Boot region
> > > > when starting up.
> > > >
> > > > Some of the downstream problems this causes are:
> > > >
> > > > 1. It can overwrite images that have been loaded
> > >
> > > So can the load command
> >
> > Can you point to where? If lmb is enabled, it should be using that, so
> > not overwrite anything.
> >
>
> If you load the initrd in memory X and make the kernel ovelap you will
> overwrite parts of the initrd.

But lmb stops that. It is actually what it is for.

> Let's try once again, the ''''problem''' that was discussed in IRC, is
> that the initramfs overwrites memory that was allocated by the EFI
> subsystem *because* we haven't merged the LMB followup patches yet.

OK, my problem was booting Ubuntu where the bootflow was unbound by
exit-boot-services, resulting in freeing memory (and re-using it)
which was still in use. I don't actually remember what was in that
memory, though. This was over a year ago, when I sent my first version
of the test[1]

>
> > >
> > > > 2. Requires lmb reservations to avoid (1) even if EFI_LOADER is not
> > > >    being used
> > > > 3. Uses regions which scripts expect to be available, e.g. the existing
> > > >    kernel_addr_r variables
> > > > 4. Introduces confusion as to what memory U-Boot can use for itself
> > > >
> > > > This series sets up a very small region of memory which EFI can used at
> > > > start-up, thus avoiding touching memory outside the U-Boot region.
> > >
> > > EFI *is* U-Boot memory region. It lies outside the U-Boot binary,
> > > because some services are instantiated on boot. It also *is* U-Boot
> > > executable code. We should start treating it as such
> >
> > This seems like semantics to me. What do you think of this patch?
>
> I don't see any reason for doing this. I've asked for a reproducer of
> the problem you are trying to solve quite a few times and I still
> don't have an email with one

Yes, I have explained this many times and I still don't have an
acknowledgement of my concerns.

EFI_LOADER should only use memory in the U-Boot area, until (at least)
an EFI feature is used. Even after that, I would prefer that it keeps
to the U-Boot region, if possible, e.g. for small allocations. I fully
understand that you have no interest in this, if you think it is
possible at all.

Regards,
Simon


>
> Thanks
> /Ilias
> >
> > Regards,
> > Simon
> >
> >
> > >
> > > Thanks
> > > /Ilias
> > > >
> > > > There are still a few tables which are not in the bloblist, but that
> > > > will be resolved separately.
> > > >
> > > > Other than tables, the amount of API-visible memory allocated by EFI
> > > > before booting is actually very small: 3 allocations, of a total of
> > > > about 250 bytes. However, there are some areas where EFI allocations are
> > > > done unnecessarily. For example efi_disk_add_dev() uses page-allocation
> > > > to allocate an efi_device_path, then immediately frees it again. In this
> > > > case it would be better to use malloc(). Since each allocation consumes
> > > > a page (4KB), 256KB has been chosen for the size here, enough for 64
> > > > allocations.
> > > >
> > > > As soon as efi_init_obj_list() is called, we know that the EFI subsystem
> > > > is to be used, so we can relax the restrictions and allow EFI to use any
> > > > memory it likes. This is handled using a simple boolean flag in BSS, since
> > > > it cannot happen before relocation.
> > > >
> > > > Changes in v4:
> > > > - Expand the commit message
> > > > - Reword the commit message since this feature is needed
> > > > - Use a different technique to keep the memory-usage in place
> > > > - Drop changes to pool allocation
> > > > - Reorder the patches
> > > > - Rewrite the cover letter
> > > > - Make it a debug message for now
> > > >
> > > > Changes in v3:
> > > > - Add new patch to drop the memset() from efi_alloc()
> > > > - Drop patch to convert device_path allocation to use malloc()
> > > >
> > > > Changes in v2:
> > > > - Drop patch 'Show more information in efi index'
> > > > - Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
> > > > - Add the word 'warning', use log_warning() and show the end address
> > > >
> > > > Simon Glass (5):
> > > >   efi: Drop the memset() from efi_alloc()
> > > >   efi: Show the location of the bounce buffer when debugging
> > > >   event: Add a generic way to reserve memory
> > > >   efi: Reserve some memory for initial use
> > > >   efi: Keep early allocations to the U-Boot region
> > > >
> > > >  common/board_f.c                  |  1 +
> > > >  common/event.c                    |  1 +
> > > >  include/asm-generic/global_data.h |  6 ++++
> > > >  include/efi_loader.h              | 24 +++++++++++++-
> > > >  include/event.h                   | 11 +++++++
> > > >  lib/efi_loader/efi_bootbin.c      |  9 ++++++
> > > >  lib/efi_loader/efi_memory.c       | 54 ++++++++++++++++++++++++-------
> > > >  lib/efi_loader/efi_setup.c        |  5 +++
> > > >  test/py/tests/test_event_dump.py  |  1 +
> > > >  9 files changed, 100 insertions(+), 12 deletions(-)
> > > >
> > > > --
> > > > 2.34.1
> > > >

[1]  https://patchwork.ozlabs.org/project/uboot/list/?series=418212&state=*