[U-Boot] [PATCH v3 4/7] efi_loader: Track size of pool allocations to allow freeing

[Alexander Graf]

On 01.10.16 23:32, Stefan Brüns wrote:
> allocate_pool has to return a buffer which is 8-byte aligned. Shift the
> region returned by allocate_pages by 8 byte and store the size in the
> headroom. The 8 byte overhead is neglegible, but provides the required
> size when freeing the allocation later.

The description doesn't say why you're doing what you're doing :).
Basically you want to say

  * We want to free
  * to free we need the size
  * to get the size we need to include it with the allocation
  * move allocations into their own struct for that
  * it's legal because allocations have to be 64bit aligned
  * fixes free

> 
> Signed-off-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> ---
>  include/efi_loader.h          |  2 ++
>  lib/efi_loader/efi_boottime.c |  6 +++---
>  lib/efi_loader/efi_memory.c   | 40 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 40e7beb..341d4a4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -122,6 +122,8 @@ efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
>  /* EFI memory allocator for small allocations, called by EFI payloads */
>  efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
>  			       void **buffer);
> +/* EFI pool memory free function. */
> +efi_status_t efi_free_pool(void *buffer);
>  /* Returns the EFI memory map */
>  efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  				struct efi_mem_desc *memory_map,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index eb74cb0..8274d8e 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -141,12 +141,12 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
>  	return EFI_EXIT(r);
>  }
>  
> -static efi_status_t EFIAPI efi_free_pool(void *buffer)
> +static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
>  {
>  	efi_status_t r;
>  
>  	EFI_ENTRY("%p", buffer);
> -	r = efi_free_pages((ulong)buffer, 0);
> +	r = efi_free_pool(buffer);
>  	return EFI_EXIT(r);
>  }
>  
> @@ -736,7 +736,7 @@ static const struct efi_boot_services efi_boot_services = {
>  	.free_pages = efi_free_pages_ext,
>  	.get_memory_map = efi_get_memory_map_ext,
>  	.allocate_pool = efi_allocate_pool_ext,
> -	.free_pool = efi_free_pool,
> +	.free_pool = efi_free_pool_ext,
>  	.create_event = efi_create_event,
>  	.set_timer = efi_set_timer,
>  	.wait_for_event = efi_wait_for_event,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 045558d..fa5c639 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -34,6 +34,18 @@ void *efi_bounce_buffer;
>  #endif
>  
>  /*
> + * U-Boot services each EFI AllocatePool request as a separate
> + * (multiple) page allocation.  We have to track the number of pages
> + * to be able to free the correct amount later.
> + * EFI requires 8 byte alignement for pool allocations, so it is

alignment

> + * possible to reserve some headroom and serve the remainder.

Any way you could write this in a way that makes it more obvious for
future readers what's going on? Maybe something like

EFI requires 8 byte alignment for pool allocations, so we can extend
every allocation by an internal 64bit variable that holds the size of
the allocation. That way we know how much to free later on.

> + */
> +struct efi_pool_allocation {
> +	u64 num_pages;
> +	char data[];
> +};
> +
> +/*
>   * Sorts the memory list from highest address to lowest address
>   *
>   * When allocating memory we should always start from the highest
> @@ -332,9 +344,35 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
>  {
>  	efi_status_t r;
>  	efi_physical_addr_t t;
> -	u64 num_pages = (size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +	u64 num_pages = (size + sizeof(u64) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +
> +	if (size == 0) {
> +		*buffer = NULL;
> +		return EFI_EXIT(EFI_SUCCESS);

Please don't call EFI_EXIT() inside functions that are not also calling
EFI_ENTRY.

> +	}
>  
>  	r = efi_allocate_pages(0, pool_type, num_pages, &t);
> +
> +	if (r == EFI_SUCCESS) {
> +		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
> +		alloc->num_pages = num_pages;
> +		*buffer = &(alloc->data);

This should work without the &, as you're referring to an array (which
is a pointer).

> +		assert(((uintptr_t)(*buffer) & 0x7) == 0);

This should be a given if t is 64bit aligned, as the compiler takes care
of the alignment for you here.

> +	}
> +
> +	return EFI_EXIT(r);

See above

> +}
> +
> +efi_status_t efi_free_pool(void *buffer)
> +{
> +	efi_status_t r;
> +	struct efi_pool_allocation *alloc;
> +
> +	alloc = container_of(buffer, struct efi_pool_allocation, data);

The assert below wants a comment here :)

> +	assert(((uintptr_t)alloc & EFI_PAGE_MASK) == 0);
> +
> +	r = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
> +
>  	return EFI_EXIT(r);

See above.


Alex

>  }
>  
>