[U-Boot] [PATCH v3 3/7] efi_loader: Move efi_allocate_pool implementation to efi_memory.c

[Alexander Graf]

On 01.10.16 23:32, Stefan Brüns wrote:
> Implementation essentially unchanged, but use EFI_PAGE_MASK/SHIFT
> instead of numeric constants.

Sorry for being nitpicky about the commit message again. Imagine you're
a distribution maintainer for U-Boot and you need to read the commit
messages of all commits in the tree to figure out whether you need to
cherry-pick a patch or not. Or you git bisect down to this commit and
need to figure out what the original intention of this patch was.

How about something like

---

We currently handle efi_allocate_pool() inside of our boot time service
file. In the following patch, pool allocation will receive additional
internal semantics that we should preserve inside efi_memory.c instead.

As foundation for those changes, split the function into an externally
facing helper efi_allocate_pool_ext() for use by payloads and an
internal function efi_allocate_pool() in efi_memory.c that handles the
actual allocation.

While at it, change the magic 0xfff / 12 constants to the more obvious
EFI_PAGE_MASK/SHIFT defines.

---


Alex

> 
> Signed-off-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> ---
>  include/efi_loader.h          |  3 +++
>  lib/efi_loader/efi_boottime.c | 11 +++++------
>  lib/efi_loader/efi_memory.c   | 11 +++++++++++
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9738835..40e7beb 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -119,6 +119,9 @@ efi_status_t efi_allocate_pages(int type, int memory_type, unsigned long pages,
>  				uint64_t *memory);
>  /* EFI memory free function. Not implemented today */
>  efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
> +/* EFI memory allocator for small allocations, called by EFI payloads */

The function that gets called by EFI payloads is actually the _ext one,
as that's the one that swizzles the gd pointer as well. Just remove the
last bit of the sentence.

> +efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
> +			       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 784891b..eb74cb0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -130,15 +130,14 @@ efi_status_t EFIAPI efi_get_memory_map_ext(unsigned long *memory_map_size,
>  	return EFI_EXIT(r);
>  }
>  
> -static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
> -					     void **buffer)
> +static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
> +						 unsigned long size,
> +						 void **buffer)
>  {
>  	efi_status_t r;
> -	efi_physical_addr_t t;
>  
>  	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;
> +	r = efi_allocate_pool(pool_type, size, buffer);
>  	return EFI_EXIT(r);
>  }
>  
> @@ -736,7 +735,7 @@ static const struct efi_boot_services efi_boot_services = {
>  	.allocate_pages = efi_allocate_pages_ext,
>  	.free_pages = efi_free_pages_ext,
>  	.get_memory_map = efi_get_memory_map_ext,
> -	.allocate_pool = efi_allocate_pool,
> +	.allocate_pool = efi_allocate_pool_ext,
>  	.free_pool = efi_free_pool,
>  	.create_event = efi_create_event,
>  	.set_timer = efi_set_timer,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 5d71fdf..045558d 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -327,6 +327,17 @@ efi_status_t efi_free_pages(uint64_t memory, unsigned long pages)
>  	return EFI_SUCCESS;
>  }
>  
> +efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
> +			       void **buffer)
> +{
> +	efi_status_t r;
> +	efi_physical_addr_t t;
> +	u64 num_pages = (size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> +
> +	r = efi_allocate_pages(0, pool_type, num_pages, &t);

This is missing the *buffer update, no?

> +	return EFI_EXIT(r);

Double EFI_EXIT is fatal. Please just return r here.


Alex

> +}
> +
>  efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  			       struct efi_mem_desc *memory_map,
>  			       unsigned long *map_key,
>