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

[Simon Glass]

Hi Tom,

On Tue, 11 Jun 2024 at 16:54, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Jun 11, 2024 at 04:22:25PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 11 Jun 2024 at 15:01, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Jun 11, 2024 at 12:52:19PM -0600, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Tue, 11 Jun 2024 at 08:36, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jun 11, 2024 at 12:17:16PM +0200, Heinrich Schuchardt 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.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Yes, Sughosh is working on the single memory reservation system for
> > > > > everyone to use. This pairs with the single memory allocation system
> > > > > (malloc) that we have. Parts of the code base that aren't keeping these
> > > > > systems up to date / obeying their results need to be corrected.
> > > >
> > > > The EFI allocations don't happen until boot time...so why do we need
> > > > to do this now? We can instead have an EFI function to scan LMB and
> > > > add to its memory map.
> > >
> > > We're talking about reservations, not allocations. So yes, when someone
> > > is making their reservation, they need to make it. I don't understand
> > > your question.
> >
> > As I understand it, this is used to tell EFI about a memory reservation.
>
> This patch, or this series? This series isn't about EFI. This patch is,
> yes.
>
> > But the EFI code can scan the LMB reservations just before booting and
> > update its tables. I don't see a need to keep them in sync before the
> > boot actually happens.
>
> But that wouldn't work. If something needs to reserve a region it needs
> to do it when it starts using it. It's not about the EFI map for the OS,
> it's about making sure that U-Boot doesn't scribble over a now-reserved
> area.

I'm not convinced of that yet. EFI does not do allocations until it
starts loading images, and it uses LMB for those (or at least it does
with bootstd). I'm just trying to keep this all as simple as possible.

Regards,
Simon