On Mon, 2 Sept 2024 at 03:53, Simon Glass <s...@chromium.org> wrote:
>
> This API call is intended for allocating small amounts of memory,
> similar to malloc(). The current implementation rounds up to whole pages
> which can waste large amounts of memory. It also implements its own
> malloc()-style header on each block.
>
> For certain allocations (those of type EFI_BOOT_SERVICES_DATA) we can
> use U-Boot's built-in malloc() instead, at least until the app starts.
> This avoids poluting the memory space with blocks of data which may
> interfere with boot scripts, etc.
>
> Once the app has started, there is no advantage to using malloc(), since
> it doesn't matter what memory is used: everything is under control of
> the EFI subsystem. Also, using malloc() after the app starts might
> result in running of memory, since U-Boot's malloc() space is typically
> quite small.
>
> In fact, malloc() is already used for most EFI-related allocations, so
> the impact of this change is fairly small.
>
> One side effect is that this seems to be showing up some bugs in the
> EFI code, since the malloc() pool becomes corrupted with some tests.
> This has likely crept in due to the very large gaps between allocations
> (around 4KB), which provides a lot of leeway when the allocation size is
> too small. Work around this by increasing the size for now, until these
> (presumed) bugs are located.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> (no changes since v1)
>
> common/dlmalloc.c | 7 +++
> include/efi_loader.h | 18 ++++++
> include/malloc.h | 7 +++
> lib/efi_loader/efi_bootbin.c | 2 +
> lib/efi_loader/efi_memory.c | 110 ++++++++++++++++++++++++++---------
> 5 files changed, 117 insertions(+), 27 deletions(-)
>
> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
> index 1ac7ce3f43c..48e9f3515f7 100644
> --- a/common/dlmalloc.c
> +++ b/common/dlmalloc.c
> @@ -613,6 +613,13 @@ void mem_malloc_init(ulong start, ulong size)
> #endif
> }
>
> +bool malloc_check_in_range(void *ptr)
> +{
> + ulong val = (ulong)ptr;
> +
> + return val >= mem_malloc_start && val < mem_malloc_end;
> +}
> +
> /* field-extraction macros */
>
> #define first(b) ((b)->fd)
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 38971d01442..d07bc06bad4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -805,6 +805,24 @@ int efi_disk_probe(void *ctx, struct event *event);
> int efi_disk_remove(void *ctx, struct event *event);
> /* Called by board init to initialize the EFI memory map */
> int efi_memory_init(void);
> +
> +/**
> + * enum efi_alloc_flags - controls EFI memory allocation
> + *
> + * @EFIAF_USE_MALLOC: Use malloc() pool for pool allocations of type
> + * EFI_BOOT_SERVICES_DATA, otherwise use page allocation
> + */
> +enum efi_alloc_flags {
> + EFIAF_USE_MALLOC = BIT(0),
> +};
> +
> +/**
> + * efi_set_alloc() - Set behaviour of EFI memory allocation
> + *
> + * @flags: new value for allocation flags (see enum efi_alloc_flags)
> + */
> +void efi_set_alloc(int flags);
> +
> /* Adds new or overrides configuration table entry to the system table */
> efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void
> *table);
> /* Sets up a loaded image */
> diff --git a/include/malloc.h b/include/malloc.h
> index 07d3e90a855..a64f117e2f2 100644
> --- a/include/malloc.h
> +++ b/include/malloc.h
> @@ -983,6 +983,13 @@ extern ulong mem_malloc_brk;
>
> void mem_malloc_init(ulong start, ulong size);
>
> +/**
> + * malloc_check_in_range() - Check if a pointer is within the malloc() region
> + *
> + * Return: true if within malloc() region
> + */
> +bool malloc_check_in_range(void *ptr);
> +
> #ifdef __cplusplus
> }; /* end of extern "C" */
> #endif
> diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> index a87006b3c0e..5bb0fdcf75d 100644
> --- a/lib/efi_loader/efi_bootbin.c
> +++ b/lib/efi_loader/efi_bootbin.c
> @@ -201,6 +201,8 @@ efi_status_t efi_binary_run(void *image, size_t size,
> void *fdt)
> {
> efi_status_t ret;
>
> + efi_set_alloc(0);
> +
Here we are setting the flags to use the efi_allocate_pages() route to
allocate memory, when booting into an EFI app. Do we need to set it
back to EFIAF_USE_MALLOC if the app exits and control lands back in
U-Boot? I am not sure that is being handled.
-sughosh
> /* Initialize EFI drivers */
> ret = efi_init_obj_list();
> if (ret != EFI_SUCCESS) {
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 50cb2f3898b..206d10f207a 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -24,6 +24,14 @@ DECLARE_GLOBAL_DATA_PTR;
> /* Magic number identifying memory allocated from pool */
> #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2
>
> +/* Flags controlling EFI memory-allocation - see enum efi_alloc_flags */
> +static int alloc_flags;
> +
> +void efi_set_alloc(int flags)
> +{
> + alloc_flags = flags;
> +}
> +
> efi_uintn_t efi_memory_map_key;
>
> struct efi_mem_list {
> @@ -57,8 +65,12 @@ void *efi_bounce_buffer;
> * The checksum calculated in function checksum() is used in FreePool() to
> avoid
> * freeing memory not allocated by AllocatePool() and duplicate freeing.
> *
> - * EFI requires 8 byte alignment for pool allocations, so we can
> - * prepend each allocation with these header fields.
> + * EFI requires 8-byte alignment for pool allocations, so we can prepend each
> + * allocation with these header fields.
> + *
> + * Note that before the EFI app is booted, EFI_BOOT_SERVICES_DATA allocations
> + * are served using malloc(), bypassing this struct. This helps to avoid
> memory
> + * fragmentation, since efi_allocate_pages() uses any pages it likes.
> */
> struct efi_pool_allocation {
> u64 num_pages;
> @@ -631,18 +643,19 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type,
> size_t align)
> /**
> * efi_allocate_pool - allocate memory from pool
> *
> + * This uses malloc() for EFI_BOOT_SERVICES_DATA allocations if
> EFIAF_USE_MALLOC
> + * is enabled
> + *
> * @pool_type: type of the pool from which memory is to be allocated
> * @size: number of bytes to be allocated
> * @buffer: allocated memory
> * Return: status code
> */
> -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t
> size, void **buffer)
> +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t
> size,
> + void **buffer)
> {
> efi_status_t r;
> u64 addr;
> - struct efi_pool_allocation *alloc;
> - u64 num_pages = efi_size_in_pages(size +
> - sizeof(struct efi_pool_allocation));
>
> if (!buffer)
> return EFI_INVALID_PARAMETER;
> @@ -652,13 +665,43 @@ efi_status_t efi_allocate_pool(enum efi_memory_type
> pool_type, efi_uintn_t size,
> return EFI_SUCCESS;
> }
>
> - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> - &addr);
> - if (r == EFI_SUCCESS) {
> - alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
> - alloc->num_pages = num_pages;
> - alloc->checksum = checksum(alloc);
> - *buffer = alloc->data;
> + if ((alloc_flags & EFIAF_USE_MALLOC) &&
> + pool_type == EFI_BOOT_SERVICES_DATA) {
> + void *ptr;
> +
> + /*
> + * Some tests crash on qemu_arm etc. if the correct size is
> + * allocated.
> + * Adding 0x10 seems to fix test_efi_selftest_device_tree
> + * Increasing it to 0x20 seems to fix test_efi_selftest_base
> + * except * for riscv64 (in CI only). But 0x100 fixes CI too.
> + *
> + * This workaround can be dropped once these problems are
> + * resolved
> + */
> + ptr = memalign(8, size + 0x100);
> + if (!ptr)
> + return EFI_OUT_OF_RESOURCES;
> +
> + *buffer = ptr;
> + r = EFI_SUCCESS;
> + log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr);
> + } else {
> + u64 num_pages = efi_size_in_pages(size +
> + sizeof(struct efi_pool_allocation));
> +
> + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type,
> + num_pages, &addr);
> + if (r == EFI_SUCCESS) {
> + struct efi_pool_allocation *alloc;
> +
> + alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
> + alloc->num_pages = num_pages;
> + alloc->checksum = checksum(alloc);
> + *buffer = alloc->data;
> + log_debug("EFI pool: pages alloc(%zx) type %d = %p\n",
> + size, pool_type, *buffer);
> + }
> }
>
> return r;
> @@ -686,27 +729,37 @@ void *efi_alloc(size_t size)
> efi_status_t efi_free_pool(void *buffer)
> {
> efi_status_t ret;
> - struct efi_pool_allocation *alloc;
>
> if (!buffer)
> return EFI_INVALID_PARAMETER;
>
> - ret = efi_check_allocated((uintptr_t)buffer, true);
> - if (ret != EFI_SUCCESS)
> - return ret;
> + if (malloc_check_in_range(buffer)) {
> + log_debug("EFI pool: free(%p)\n", buffer);
> + free(buffer);
> + ret = EFI_SUCCESS;
> + } else {
> + struct efi_pool_allocation *alloc;
>
> - alloc = container_of(buffer, struct efi_pool_allocation, data);
> + ret = efi_check_allocated((uintptr_t)buffer, true);
> + if (ret != EFI_SUCCESS)
> + return ret;
>
> - /* Check that this memory was allocated by efi_allocate_pool() */
> - if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
> - alloc->checksum != checksum(alloc)) {
> - printf("%s: illegal free 0x%p\n", __func__, buffer);
> - return EFI_INVALID_PARAMETER;
> - }
> - /* Avoid double free */
> - alloc->checksum = 0;
> + alloc = container_of(buffer, struct efi_pool_allocation,
> data);
>
> - ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
> + /*
> + * Check that this memory was allocated by efi_allocate_pool()
> + */
> + if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
> + alloc->checksum != checksum(alloc)) {
> + printf("%s: illegal free 0x%p\n", __func__, buffer);
> + return EFI_INVALID_PARAMETER;
> + }
> + /* Avoid double free */
> + alloc->checksum = 0;
> +
> + ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
> + log_debug("EFI pool: pages free(%p)\n", buffer);
> + }
>
> return ret;
> }
> @@ -926,6 +979,9 @@ static void add_u_boot_and_runtime(void)
>
> int efi_memory_init(void)
> {
> + /* use malloc() pool where possible */
> + efi_set_alloc(EFIAF_USE_MALLOC);
> +
> efi_add_known_memory();
>
> add_u_boot_and_runtime();
> --
> 2.34.1
>
