[RFC PATCH 15/31] efi_memory: add an event handler to update memory map

[Sughosh Ganu]

On Tue, 11 Jun 2024 at 15: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
> > 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 :
>
> This is dangerous. LMB might turn memory that is marked as
> EfiReservedMemory which the OS must respect into EfiBootServicesData
> which the OS may discard.

If memory is being marked as EfiReservedMemory, that will also update
the LMB memory map to have it marked as reserved. So a correct
implementation should then not be freeing memory that is not allocated
by that module.

-sughosh

>
> E.g. initr_lmb() is being called after efi_memory_init().
>
> Getting all cases of synchronization properly tested seems very hard to
> me. Everything would be much easier if we had only a single memory
> management system.
>
> Best regards
>
> Heinrich
>
> > +                                      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 ?
>