Re: [PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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