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

[Ilias Apalodimas]

Hi Simon

On Sat, 19 Oct 2024 at 21:07, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 16 Oct 2024 at 08:02, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > [...]
> >
> > > > > +
> > > > > +/**
> > > > > + * 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?
>
> Just that. U-Boot should not use memory below its own region, that's all.

That's not a problem though. Before the subsystem bringup the only
thing that gets initialized is the memory map

>
> >
> >
> > >
> > > >
> > > > > + *
> > > > > + * 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
>
> Yes that sounds like a good idea...but there is a catch. There are few
> allocations that happen when U-Boot is running - e.g. partitions. I
> think we need to work towards that bit by bit.

There's also the early capsule updates that might need this -- hence
the memory init really early. I'll have a look at this instead of
splitting the memory inits

Thanks
/Ilias

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