On Mon, 10 Jun 2024 at 20:22, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> hi Ilias,
>
> On Mon, 10 Jun 2024 at 19:48, Ilias Apalodimas
> <ilias.apalodi...@linaro.org> wrote:
> >
> > Hi Sughosh
> >
> > On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> > >
> > > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas
> > > <ilias.apalodi...@linaro.org> wrote:
> > > >
> > > > On Fri, 7 Jun 2024 at 21:54, Sughosh Ganu <sughosh.g...@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.
Or rather, "the notification mechanism is not inefficient".
-sughosh
>
> -sughosh
>
> >
> > Thanks
> > /Ilias
> > >
> > > -sughosh
> > >
> > > >
> > > > Cheers
> > > > /Ilias
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.g...@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
> > > > >
