[RFC PATCH 15/31] efi_memory: add an event handler to update memory map
Sughosh Ganu
sughosh.ganu at linaro.org
Mon Jun 10 16:52:19 CEST 2024
hi Ilias,
On Mon, 10 Jun 2024 at 19:48, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Sughosh
>
> On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Fri, 7 Jun 2024 at 21:54, Sughosh Ganu <sughosh.ganu at linaro.org> 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
> > > > 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.
> > >
> > > So the question here, is why do we need a notifier chain overall?
> > > Can't we change the EFI subsystem and allocate memory with lmb now?
> > > And any special functions we have in EFI (e.g allocate aligned memory)
> > > can migrate to lmb()
> >
> > Like we discussed offline, that was my initial attempt -- to use the
> > LMB allocation API's from inside the EFI allocate pages module. But I
> > was facing a lot of corner case issues, primarily in keeping the two
> > memory maps the same.
>
> I think it would worth discussing this a bit more. I like the idea of
> having a single allocator more than having events to update memory
> reservations
>
> > Which is why I moved to the current
> > implementation of notifying other modules, and that too only for the
> > addresses in the RAM region.
>
> The notification to 'other modules' i still done by updating the
> static memory map though no?
> So what corner cases we couldn't solve by having a single allocator?
I can re-check what were the issues that I faced when trying with a
single allocator. But please note that we are not making any
significant gains by having a single allocator. Even with a common
allocator(say LMB), it would still be required to have the same
notification mechanism to update the EFI memory map. Else the EFI
memory map would show regions of memory as conventional memory, while
they are being used by the other subsystem. So, all in all, I think
that the notification mechanism is not that inefficient. Thanks.
-sughosh
>
> Thanks
> /Ilias
> >
> > -sughosh
> >
> > >
> > > Cheers
> > > /Ilias
> > > >
> > > > 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 ?
> > > > --
> > > > 2.34.1
> > > >
More information about the U-Boot
mailing list