hi Ilias,
On Wed, 12 Jun 2024 at 11:19, Ilias Apalodimas
<ilias.apalodi...@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.
>
> > 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.
Like I mentioned in one of the earlier replies [1], this is actually
not an issue, as long as modules are freeing memory that was actually
allocated/reserved by them. In case of the EFI subsystem marking some
memory region as EfiReservedMemory, this will send a notification to
the LMB module, and the LMB module will mark this region as reserved.
So unless some LMB user decides on freeing up memory that was not
reserved by it, this should be just fine.
Making use of the LMB API's as the common backend for allocating
memory is a separate thing, and I will look into that. But the above
highlighted issue is not one, with correctly working code.
-sughosh
[1] - https://lists.denx.de/pipermail/u-boot/2024-June/555883.html
