[U-Boot] [PATCH v2 3/6] efi_loader: Track size of pool allocations to allow freeing

[Alexander Graf]

On 01.10.16 19:31, 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.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> ---
>  lib/efi_loader/efi_boottime.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 784891b..c413ecb 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -135,19 +135,37 @@ static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
>  {
>  	efi_status_t r;
>  	efi_physical_addr_t t;
> +	u64 num_pages = (size + 8 + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;

Would it make things more readable if you used sizeof(u64) here?

Also, it's great to have a nice patch description which described why we
have the 8 byte padding here, but we definitely need it as comment in
the code as well. Otherwise people 2 years from now will have no idea
why we allocate 8 bytes more ;). At least not without reverse
engineering the source.

>  
>  	EFI_ENTRY("%d, %ld, %p", pool_type, size, buffer);
> -	r = efi_allocate_pages(0, pool_type, (size + 0xfff) >> 12, &t);
> -	*buffer = (void *)(uintptr_t)t;
> +
> +	if (size == 0) {
> +		*buffer = NULL;
> +		return EFI_EXIT(EFI_SUCCESS);
> +	}
> +
> +	r = efi_allocate_pages(0, pool_type, num_pages, &t);
> +
> +	if (r == EFI_SUCCESS) {
> +		*(u64 *)(uintptr_t)t = num_pages;

I would prefer if we could make that cast a bit more expressive. How about

  u64 *num_pages_hint = (void *)(uintptr_t)t;

  *num_pages_hint = num_pages;
  *buffer = &num_pages_hint[1];

or maybe even better would be a trivial struct. Something like

  struct efi_pool_allocation {
    u64 num_pages;
    char data[];
  };

Then you could basically write mostly self-documenting code:

  struct efi_pool_alloction *alloc = (void *)(uintptr_t)t;

  alloc->num_pages = num_pages;
  *buffer = alloc->data;

I would still prefer if you could write a comment about what's going on,
but with this it's much more obvious IMHO.

> +		*buffer = (void *)(uintptr_t)(t + 8);
> +	}
> +
>  	return EFI_EXIT(r);
>  }
>  
>  static efi_status_t EFIAPI efi_free_pool(void *buffer)
>  {
>  	efi_status_t r;
> +	u64 num_pages;
>  
>  	EFI_ENTRY("%p", buffer);
> -	r = efi_free_pages((ulong)buffer, 0);
> +
> +	buffer = (char *)(buffer) - 8;
> +	assert(((ulong)buffer & EFI_PAGE_MASK) == 0);
> +	num_pages = *(u64 *)buffer;

With the struct, this could look a lot cleaner too:

  struct efi_pool_alloction *alloc = container_of(buffer, struct
efi_pool_alloction, data);

  /* Pool allocations always happen on page boundaries */
  assert(!((uintptr_t)alloc & EFI_PAGE_MASK));
  r = efi_free_pages((uintptr_t)alloc->data, alloc->num_pages);


Alex

> +
> +	r = efi_free_pages((ulong)buffer, num_pages);
>  	return EFI_EXIT(r);
>  }
>  
>