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

[Simon Glass]

Hi Ilias,

On Tue, 11 Jun 2024 at 23:49, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> [...]
>
> > > > > > > > > +   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,
>
> It does in some cases. E.g The efi variables allocate some pages when
> the subsystem starts, the TCG protocol allocates the EventLog once
> it's installed and I am pretty sure we have more.

Both of those seem wrong to me, though.

- EFI variables should use malloc() like other small amounts of data
- TCG should probably use a bloblist, but in any case I suspect it is
needed beyond EFI

>
> > 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.
>
> Heinrich already pointed out a potential danger in the current design.
> If an EFI allocation happens *before* LMB comes up, we might end up
> updating the efi memory map with the wrong attributes. That would lead
> to the OS discarding memory areas that should be preserved and we
> won't figure that out until the OS boots and blows up.

Yes, I believe the EFI memory sort-out should happen when booting and
not before. It is a bit like creating a plate of spaghetti if we do
all this stuff randomly while trying to boot different things,
retrying, etc....

Regards,
Simon