[PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()
Ilias Apalodimas
ilias.apalodimas at linaro.org
Mon Mar 20 12:04:41 CET 2023
On Mon, Mar 20, 2023 at 10:15:21AM +0100, Heinrich Schuchardt wrote:
> On 3/20/23 08:38, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote:
> > > The incumbent function efi_alloc() is unused.
> > >
> > > Replace dp_alloc() by a new function efi_alloc() that we can use more
> > > widely.
> >
> > [...]
> >
> > > #include <efi_loader.h>
> > > #include <init.h>
> > > +#include <log.h>
> > > #include <malloc.h>
> > > #include <mapmem.h>
> > > #include <watchdog.h>
> > > @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > > return EFI_SUCCESS;
> > > }
> > >
> > > -/**
> > > - * efi_alloc() - allocate memory pages
> > > - *
> > > - * @len: size of the memory to be allocated
> > > - * @memory_type: usage type of the allocated memory
> > > - * Return: pointer to the allocated memory area or NULL
> > > - */
> > > -void *efi_alloc(uint64_t len, int memory_type)
> > > -{
> > > - uint64_t ret = 0;
> > > - uint64_t pages = efi_size_in_pages(len);
> > > - efi_status_t r;
> > > -
> > > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
> > > - &ret);
> > > - if (r == EFI_SUCCESS)
> > > - return (void*)(uintptr_t)ret;
> > > -
> > > - return NULL;
> > > -}
> > > -
> > > /**
> > > * efi_free_pages() - free memory pages
> > > *
> > > @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> > > return r;
> > > }
> > >
> > > +/**
> > > + * efi_alloc() - allocate boot services data pool memory
> > > + *
> > > + * Allocate memory from pool and zero it out.
> > > + *
> > > + * @size: number of bytes to allocate
> > > + * Return: pointer to allocated memory or NULL
> > > + */
> > > +void *efi_alloc(size_t size)
> >
> > All our allocation related functions require the memory type to be passed.
> > If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to
> > change the name a bit to indicate that.
>
> We have 13 instances of allocating EFI_BOOT_SERVICES_DATA from the pool and
> only 2 instances (see below) of other memory types. So this is the only case
> worth factoring out. I would prefer to keep identifiers short.
Ok, then at least please add the EFI_BOOT_SERVICES_DATA part in the
function documentation, or rename it to efi_alloc_bsd()
>
> >
> > > +{
> > > + void *buf;
> > > +
> > > + if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=
> >
> > Is there a reason we are using efi_allocate_pool instead of
> > efi_allocate_pages?
>
> The UEFI specification explicitly requires pool memory in most places where
> the caller has to free memory. Freeing pages requires to know the size of
> the memory area while freeing from pool does not. Furthermore when freeing
> from pool we can check if the memory was ever allocated.
>
> In future we may want to make small allocations from pool more efficient by
> not consuming a minimum of one page.
>
> We could rethink why we are allocating configuration tables from pool in the
> following instances:
>
> lib/efi_loader/efi_tcg2.c:1703
> create_final_event()
>
> lib/efi_loader/efi_runtime.c:116
> efi_init_runtime_supported()
I think they can both change to efi_alloc() now. As far as the TCG is
concerned there no particular reason of using pool allocations.
In any case you can add my
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
Regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> > > + EFI_SUCCESS) {
> > > + log_err("out of memory");
> > > + return NULL;
> > > + }
> > > + memset(buf, 0, size);
> > > +
> > > + return buf;
> > > +}
> > > +
> > > /**
> > > * efi_free_pool() - free memory from pool
> > > *
> > > --
> > > 2.39.2
> > >
> >
> > Thanks
> > /Ilias
>
More information about the U-Boot
mailing list