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