[PATCH v4 5/5] efi: Keep early allocations to the U-Boot region

[Ilias Apalodimas]

[...]

> > > +
> > > +/**
> > > + * efi_memory_coop() - Allow EFI to use all available memory
> > > + *
> > > + * Up until this function is called, only a small portion of memory is available
> > > + * for use by the EFI memory-allocator. This function is called at the
> > > + * 'point of cooperation', before jumping into an EFI app, which needs to be
> > > + * able to make use of all the memory in the machine
> >
> > As above, this is called when the EFI subsystem comes up. Not when an
> > application is called. For example doing a 'printenv -e' to print EFI
> > variables would trigger this.
>
> Yes. At that point we know we are using an EFI feature. We can
> certainly extend the intent of this patch to cover that, but I am
> trying to take baby steps here.
>
> > The funtion call you are removing just adds all memory all memory as
> > EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't
> > understand what this patch is supposed to fix
>
> Basically it feeds a small amount of memory to EFI initially, within
> the U-Boot region. That is enough to deal with the few allocations
> that are done before efi_init_obj_list() is called.
>
> So if you don't use any EFI features, EFI's memory usage is limited to
> the U-Boot region.

Yes, but what does it fix?


>
> >
> > > + *
> > > + * Return: efi_status_t (EFI_SUCCESS on success)
> > > + */
> > > +int efi_memory_coop(void);
> > > +
> > >  /* Adds new or overrides configuration table entry to the system table */
> > >  efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table);
> > >  /* Sets up a loaded image */
> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > index 9cc33397371..2cd0b00e9eb 100644
> > > --- a/lib/efi_loader/efi_memory.c
> > > +++ b/lib/efi_loader/efi_memory.c
> > > @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR;
> > >   */
> > >  #define EFI_EARLY_REGION_SIZE  SZ_256K
> > >
> > > +/* Set when all memory has been added for use by EFI */
> > > +static bool efi_full_memory;
> > > +
> > >  efi_uintn_t efi_memory_map_key;
> > >
> > >  struct efi_mem_list {
> > > @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
> > >
> > >  int efi_memory_init(void)
> > >  {
> > > -       efi_add_known_memory();
> > > +       efi_status_t ret;
> > > +
> > > +       /* restrict EFI to its own region until efi_memory_coop() is called */
> > > +       ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),
> > > +                                   EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,
> > > +                                   EFI_CONVENTIONAL_MEMORY, false);
> > > +
> > > +       return ret;
> > > +}
> > >
> > > +int efi_memory_coop(void)
> > > +{
> > > +       if (efi_full_memory)
> > > +               return 0;
> > > +       log_debug("Enabling coop memory\n");
> > > +
> > > +       efi_full_memory = true;
> > > +
> > > +       efi_add_known_memory();
> > >         add_u_boot_and_runtime();
> > >
> > >  #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
> > > @@ -943,7 +963,7 @@ int efi_memory_init(void)
> > >         if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
> > >                                (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
> > >                                &efi_bounce_buffer_addr) != EFI_SUCCESS)
> > > -               return -1;
> > > +               return -ENOMEM;
> > >
> > >         efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;
> > >  #endif
> > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > index a610e032d2f..40e23ca104c 100644
> > > --- a/lib/efi_loader/efi_setup.c
> > > +++ b/lib/efi_loader/efi_setup.c
> > > @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void)
> > >         if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
> > >                 return efi_obj_list_initialized;
> > >
> > > +       /* Allow EFI to use all memory */
> > > +       ret = efi_memory_coop();
> > > +       if (ret != EFI_SUCCESS)
> > > +               goto out;
> > > +

I am not sure that will work. I'll have to check if not adding the
runtime regions from the linker scripts during bringup is safe.
But, instead of adding all that, including a small gd region for
efi_region and having to explicitly make that region bigger every time
something new gets added on efi allocations, why don't we simply move
efi_memory_init() out of init_sequence_r()?
We can protect the runtime services etc via LMB now and initialize the
memory map in efi_init_obj_list().

I haven't gone through all the code, but it sounds doable

Thanks
/Ilias

> > >         /* Set up console modes */
> > >         efi_setup_console_size();
> > >
> > > --
> > > 2.34.1
> > >
> >
> > Thanks
> > /Ilias
>
> Regards,
> SImon