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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4149 bytes --]

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@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@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]