[RFC PATCH 15/31] efi_memory: add an event handler to update memory map
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Jun 12 08:45:24 CEST 2024
On Mon, 10 Jun 2024 at 18:54, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On Mon, 10 Jun 2024 at 09:42, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Mon, 10 Jun 2024 at 20:47, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > >
> > > On 07.06.24 20:52, Sughosh Ganu wrote:
> > > > There are events that would be used to notify other interested modules
> > > > of any changes in available and occupied memory. This would happen
> > > > when a module allocates or reserves memory, or frees up memory. These
> > >
> > > I am worried about the "frees up memory" case.
> > >
> > > When LMB frees memory we cannot add it back to EFI conventional memory
> > > as there might still be a file image lingering around that EFI should
> > > not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData.
> >
> > So here is my basic doubt. Why would LMB free up memory if it still
> > has a valid image. If that is the case, the lmb_free API should not be
> > called?
> >
> > -sughosh
> >
> >
> > >
> > > How do you ensure that if a region reserved by LMB notification as
> > > EfiLoaderData is coalesced with some other allocation LMB is not
> > > requested to mark the coalesced region as reserved?
> > >
> > > @Tom
> > >
> > > Clinging to the existing logic that you can do anything when loading
> > > files is obviously leading us into coding hell.
> > >
> > > If somebody wants to load two images into the same location, he should
> > > be forced to unload the first image. This will allow us to have a single
> > > memory management system.
>
> It seems we really shouldn't use the words 'allocate' and 'free' when
> talking about LMB. They are simply reservations.
Correct and while at it can we please make the code less confusing to
read. What we today mark as reserved isnt even trully reserved as it
can be overwritten.
struct lmb_region memory -> available memory we added on LMB. That's fine
struct lmb_region reserved -> can we rename this to 'used' and rename
LMB_NOOVERWRITE to LMB_RESERVED?
Thanks
/Ilias
> I believe we have got
> into this situation due to an assumption that these two things are the
> same, but in U-Boot they certainly are not. LMB is a very lighweight
> and temporary reservation system to be used for a single boot process.
>
> Regards,
> Simon
>
>
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > > changes in memory map should be notified to other interested modules
> > > > so that the allocated memory does not get overwritten. Add an event
> > > > handler in the EFI memory module to update the EFI memory map
> > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > memory request would honour the updated memory map and only available
> > > > memory would be allocated from.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > ---
> > > > lib/efi_loader/efi_memory.c | 70 ++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 58 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > index 435e580fb3..93244161b0 100644
> > > > --- a/lib/efi_loader/efi_memory.c
> > > > +++ b/lib/efi_loader/efi_memory.c
> > > > @@ -73,6 +73,10 @@ struct efi_pool_allocation {
> > > > #if CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)
> > > > extern bool is_addr_in_ram(uintptr_t addr);
> > > >
> > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages,
> > > > + int memory_type,
> > > > + bool overlap_only_ram);
> > > > +
> > > > static void efi_map_update_notify(u64 addr, u64 size, u8 op)
> > > > {
> > > > struct event_efi_mem_map_update efi_map = {0};
> > > > @@ -84,6 +88,34 @@ static void efi_map_update_notify(u64 addr, u64 size, u8 op)
> > > > if (is_addr_in_ram((uintptr_t)addr))
> > > > event_notify(EVT_EFI_MEM_MAP_UPDATE, &efi_map, sizeof(efi_map));
> > > > }
> > > > +
> > > > +static int lmb_mem_map_update_sync(void *ctx, struct event *event)
> > > > +{
> > > > + u8 op;
> > > > + u64 addr;
> > > > + u64 pages;
> > > > + efi_status_t status;
> > > > + struct event_lmb_map_update *lmb_map = &event->data.lmb_map;
> > > > +
> > > > + addr = (uintptr_t)map_sysmem(lmb_map->base, 0);
> > > > + pages = efi_size_in_pages(lmb_map->size + (addr & EFI_PAGE_MASK));
> > > > + op = lmb_map->op;
> > > > + addr &= ~EFI_PAGE_MASK;
> > > > +
> > > > + if (op != MAP_OP_RESERVE && op != MAP_OP_FREE) {
> > > > + log_debug("Invalid map update op received (%d)\n", op);
> > > > + return -1;
> > > > + }
> > > > +
> > > > + status = __efi_add_memory_map_pg(addr, pages,
> > > > + op == MAP_OP_FREE ?
> > > > + EFI_CONVENTIONAL_MEMORY :
> > > > + EFI_BOOT_SERVICES_DATA,
> > > > + true);
> > > > +
> > > > + return status == EFI_SUCCESS ? 0 : -1;
> > > > +}
> > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync);
> > > > #endif /* MEM_MAP_UPDATE_NOTIFY */
> > > >
> > > > /**
> > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
> > > > return EFI_CARVE_LOOP_AGAIN;
> > > > }
> > > >
> > > > -/**
> > > > - * efi_add_memory_map_pg() - add pages to the memory map
> > > > - *
> > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE
> > > > - * @pages: number of pages to add
> > > > - * @memory_type: type of memory added
> > > > - * @overlap_only_ram: region may only overlap RAM
> > > > - * Return: status code
> > > > - */
> > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > > > - int memory_type,
> > > > - bool overlap_only_ram)
> > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages,
> > > > + int memory_type,
> > > > + bool overlap_only_ram)
> > > > {
> > > > struct list_head *lhandle;
> > > > struct efi_mem_list *newlist;
> > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > > > }
> > > > }
> > > >
> > > > + return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_add_memory_map_pg() - add pages to the memory map
> > > > + *
> > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE
> > > > + * @pages: number of pages to add
> > > > + * @memory_type: type of memory added
> > > > + * @overlap_only_ram: region may only overlap RAM
> > > > + * Return: status code
> > > > + */
> > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> > > > + int memory_type,
> > > > + bool overlap_only_ram)
> > > > +{
> > > > + efi_status_t status;
> > > > +
> > > > + status = __efi_add_memory_map_pg(start, pages, memory_type,
> > > > + overlap_only_ram);
> > > > + if (status != EFI_SUCCESS)
> > > > + return status;
> > > > +
> > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY))
> > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT,
> > > > memory_type == EFI_CONVENTIONAL_MEMORY ?
> > >
More information about the U-Boot
mailing list