| Message ID | 20240901222259.456932-3-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: Start tidying up memory management | expand |
On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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 >
Hi Simon, Apologies for the late reply, I was attending a conference. On Mon, 2 Sept 2024 at 01:23, Simon Glass <sjg@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 <sjg@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), > +}; Why do we need to handle cases differently? IOW can't all EFI allocations that need a pool gi via malloc? [...] > @@ -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); I don't think the explanation is enough here. On top of that adding random values to fix the problem doesn't sound right. Can we figure out why? > + if (!ptr) > + return EFI_OUT_OF_RESOURCES; > + > + *buffer = ptr; > + r = EFI_SUCCESS; > + log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr); So as I commented above, I think this is papering over whatever problem you are seeing. If you want to move the pool to use malloc() that's fine, but *all* of the pool allocations should use it. Not just boot services because its easier to retrofit it on the current code. > + } 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; > [...] Thanks /Ilias
Hi Sughosh, On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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. I don't believe so. Once we have booted into the app, U-Boot loses control of its memory layout, in the sense that the efi_allocate_pages() has likely been called and placed things all over the place in the memory. People should expect this. We can potentially deal with this if we find a specific problem, but I can't think of one at the moment. > > -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 > > Regards, Simon
Hi Ilias, On Fri, 6 Sept 2024 at 01:13, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > Apologies for the late reply, I was attending a conference. > > On Mon, 2 Sept 2024 at 01:23, Simon Glass <sjg@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 <sjg@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), > > +}; > > Why do we need to handle cases differently? IOW can't all EFI > allocations that need a pool gi via malloc? Once the app boots, as Heinrich pointed out, it expects to be able to malloc() very large amount of memory, but the malloc() pool is small. > > [...] > > > @@ -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); > > I don't think the explanation is enough here. On top of that adding > random values to fix the problem doesn't sound right. Can we figure > out why? My guess is that an allocated pointer is going beyond its limits. The newer upstream dlmalloc() has a checker which might help. I fiddled around a bit but could not work one where the problem was. > > > + if (!ptr) > > + return EFI_OUT_OF_RESOURCES; > > + > > + *buffer = ptr; > > + r = EFI_SUCCESS; > > + log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr); > > So as I commented above, I think this is papering over whatever > problem you are seeing. If you want to move the pool to use malloc() > that's fine, but *all* of the pool allocations should use it. Not just > boot services because its easier to retrofit it on the current code. Please see above. Also, please see the commit message. This change actually solves the problems I am seeing, quite well. > > > + } 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; > > > [...] Regards, Simon
On Fri, 6 Sept 2024 at 18:31, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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. > > I don't believe so. Once we have booted into the app, U-Boot loses > control of its memory layout, in the sense that the > efi_allocate_pages() has likely been called and placed things all over > the place in the memory. People should expect this. I am referring to a scenario where the app exits and control returns back to U-Boot, which I believe is a valid scenario. In such a case, should control not switch back to the malloc based allocations. Otherwise we do not have consistent behaviour with the allocations -- any subsequent calls to efi_allocate_pool on return from an EFI app would continue using the other (efi_allocate_pages() based) path. This is of course with the assumption that the EFI maintainers are fine with using this hybrid approach on the allocations. -sughosh > > We can potentially deal with this if we find a specific problem, but I > can't think of one at the moment. > > > > > -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 > > > > > Regards, > Simon
Hi Sughosh, On Mon, 9 Sept 2024 at 01:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 6 Sept 2024 at 18:31, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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. > > > > I don't believe so. Once we have booted into the app, U-Boot loses > > control of its memory layout, in the sense that the > > efi_allocate_pages() has likely been called and placed things all over > > the place in the memory. People should expect this. > > I am referring to a scenario where the app exits and control returns > back to U-Boot, which I believe is a valid scenario. In such a case, > should control not switch back to the malloc based allocations. > Otherwise we do not have consistent behaviour with the allocations -- > any subsequent calls to efi_allocate_pool on return from an EFI app > would continue using the other (efi_allocate_pages() based) path. Thanks for reviewing. I completely understand your scenario, but I think I explained it above. The important thing is to keep memory 'consistent' from a U-Boot point of view until we actually boot something. U-Boot expects memory from 0 to the bottom of the stack to be available for loading images. That is why it relocates itself. > > This is of course with the assumption that the EFI maintainers are > fine with using this hybrid approach on the allocations. Yes, I certainly hope so. This whole problem has caused an enormous amount of confusion and I very much want to clean it up. Regards, Simon > > -sughosh > > > > > We can potentially deal with this if we find a specific problem, but I > > can't think of one at the moment. > > > > > > > > -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 > > > > > > > > Regards, > > Simon
On Wed, 11 Sept 2024 at 00:14, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 9 Sept 2024 at 01:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 6 Sept 2024 at 18:31, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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. > > > > > > I don't believe so. Once we have booted into the app, U-Boot loses > > > control of its memory layout, in the sense that the > > > efi_allocate_pages() has likely been called and placed things all over > > > the place in the memory. People should expect this. > > > > I am referring to a scenario where the app exits and control returns > > back to U-Boot, which I believe is a valid scenario. In such a case, > > should control not switch back to the malloc based allocations. > > Otherwise we do not have consistent behaviour with the allocations -- > > any subsequent calls to efi_allocate_pool on return from an EFI app > > would continue using the other (efi_allocate_pages() based) path. > > Thanks for reviewing. > > I completely understand your scenario, but I think I explained it > above. The important thing is to keep memory 'consistent' from a > U-Boot point of view until we actually boot something. How does reverting back to using the malloc heap for efi_allocate_pool() after returning back from the EFI app affect memory consistency? We now have the LMB memory map which is global and persistent. So any allocations that were done (and not freed) from the app, would be using the memory that you mention -- from 0 to bottom of the stack. In fact, I would argue that not reverting back to malloc based allocations on return from the app is inconsistent behaviour. > > U-Boot expects memory from 0 to the bottom of the stack to be > available for loading images. That is why it relocates itself. The EFI app is supposed to use the memory allocation API's (efi_allocate_{pages,pool}) for getting memory. And those requests are going to come from the LMB allocations (after the latest LMB rework series). So the heap memory is not supposed to be trampled over with. -sughosh > > > > > This is of course with the assumption that the EFI maintainers are > > fine with using this hybrid approach on the allocations. > > Yes, I certainly hope so. This whole problem has caused an enormous > amount of confusion and I very much want to clean it up. > > Regards, > Simon > > > > > > -sughosh > > > > > > > > We can potentially deal with this if we find a specific problem, but I > > > can't think of one at the moment. > > > > > > > > > > > -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 > > > > > > > > > > > Regards, > > > Simon
Hi Sughosh, On Wed, 11 Sept 2024 at 00:50, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 11 Sept 2024 at 00:14, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Mon, 9 Sept 2024 at 01:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 6 Sept 2024 at 18:31, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg@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 <sjg@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. > > > > > > > > I don't believe so. Once we have booted into the app, U-Boot loses > > > > control of its memory layout, in the sense that the > > > > efi_allocate_pages() has likely been called and placed things all over > > > > the place in the memory. People should expect this. > > > > > > I am referring to a scenario where the app exits and control returns > > > back to U-Boot, which I believe is a valid scenario. In such a case, > > > should control not switch back to the malloc based allocations. > > > Otherwise we do not have consistent behaviour with the allocations -- > > > any subsequent calls to efi_allocate_pool on return from an EFI app > > > would continue using the other (efi_allocate_pages() based) path. > > > > Thanks for reviewing. > > > > I completely understand your scenario, but I think I explained it > > above. The important thing is to keep memory 'consistent' from a > > U-Boot point of view until we actually boot something. > > How does reverting back to using the malloc heap for > efi_allocate_pool() after returning back from the EFI app affect > memory consistency? We now have the LMB memory map which is global and > persistent. So any allocations that were done (and not freed) from the > app, would be using the memory that you mention -- from 0 to bottom of > the stack. In fact, I would argue that not reverting back to malloc > based allocations on return from the app is inconsistent behaviour. No, I mean we should *not* use memory from 0 to the bottom of the stack. That is supposed to be reserved for image loading. Scripts may assume they can do anything in this space and most boards have fixed addresses for kernel, etc. > > > > > U-Boot expects memory from 0 to the bottom of the stack to be > > available for loading images. That is why it relocates itself. > > The EFI app is supposed to use the memory allocation API's > (efi_allocate_{pages,pool}) for getting memory. And those requests are > going to come from the LMB allocations (after the latest LMB rework > series). So the heap memory is not supposed to be trampled over with. I'm not quite sure what you are getting at here? My goal is to tidy up the allocation of memory *before* the app boots. > > > This is of course with the assumption that the EFI maintainers are > > > fine with using this hybrid approach on the allocations. > > > > Yes, I certainly hope so. This whole problem has caused an enormous > > amount of confusion and I very much want to clean it up. > > > > > > > > > > > We can potentially deal with this if we find a specific problem, but I > > > > can't think of one at the moment. > > > > [..] Regards, Simon
Hi Simon, On Fri, 6 Sept 2024 at 16:01, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Fri, 6 Sept 2024 at 01:13, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > Apologies for the late reply, I was attending a conference. > > > > On Mon, 2 Sept 2024 at 01:23, Simon Glass <sjg@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. This won't happen with LMB merged no? > > > > > > 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. Where? We explained in the past that malloc is only used to handle internal EFI stuff which don't need the efi allocations and that's perfectly fine. > > > > > > 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 <sjg@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), > > > +}; > > > > Why do we need to handle cases differently? IOW can't all EFI > > allocations that need a pool gi via malloc? > > Once the app boots, as Heinrich pointed out, it expects to be able to > malloc() very large amount of memory, but the malloc() pool is small. > Yes, it does. My problem is that right now we have everything in the same pool, handled by LMB, but you are arguing it's 'cleaner' to split the allocations. I can't really understand what the problem with the current allocations is. > > > > [...] > > > > > @@ -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); > > > > I don't think the explanation is enough here. On top of that adding > > random values to fix the problem doesn't sound right. Can we figure > > out why? > > My guess is that an allocated pointer is going beyond its limits. The > newer upstream dlmalloc() has a checker which might help. I fiddled > around a bit but could not work one where the problem was. Ok, but I don't want us to pull code with random values that happened to work during testing. We need to understand why > > > > > > + if (!ptr) > > > + return EFI_OUT_OF_RESOURCES; > > > + > > > + *buffer = ptr; > > > + r = EFI_SUCCESS; > > > + log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr); > > > > So as I commented above, I think this is papering over whatever > > problem you are seeing. If you want to move the pool to use malloc() > > that's fine, but *all* of the pool allocations should use it. Not just > > boot services because its easier to retrofit it on the current code. > > Please see above. Also, please see the commit message. This change > actually solves the problems I am seeing, quite well. I did and the only 'problem' that is mentioned is polluting and overwriting memory of scripts etc. But that goes away with LMB AFAICT. So the only advantage is that you save a few kbs of space when requesting pool allocations? Thanks /Ilias > > > > > > + } 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; > > > > > [...] > > Regards, > Simon
Hi Ilias, On Thu, 12 Sept 2024 at 08:37, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Fri, 6 Sept 2024 at 16:01, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Fri, 6 Sept 2024 at 01:13, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > Apologies for the late reply, I was attending a conference. > > > > > > On Mon, 2 Sept 2024 at 01:23, Simon Glass <sjg@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. > > This won't happen with LMB merged no? It still happens with LMB merged. As I covered in the cover letter, this is orthogonal to all of that. In fact, I think a lot of the effort there is actually missing the point, unfortunately. > > > > > > > > > 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. > > Where? We explained in the past that malloc is only used to handle > internal EFI stuff which don't need the efi allocations and that's > perfectly fine. I see only about 5 allocations affected by this change, when booting from EFI. > > > > > > > > > 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 <sjg@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), > > > > +}; > > > > > > Why do we need to handle cases differently? IOW can't all EFI > > > allocations that need a pool gi via malloc? > > > > Once the app boots, as Heinrich pointed out, it expects to be able to > > malloc() very large amount of memory, but the malloc() pool is small. > > > > Yes, it does. My problem is that right now we have everything in the > same pool, handled by LMB, but you are arguing it's 'cleaner' to split > the allocations. I can't really understand what the problem with the > current allocations is. The problem is that they happen in space, between the bottom of memory and the bottom of the stack. That is the area which is supposed to not be used by U-Boot. > > > > > > > [...] > > > > > > > @@ -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); > > > > > > I don't think the explanation is enough here. On top of that adding > > > random values to fix the problem doesn't sound right. Can we figure > > > out why? > > > > My guess is that an allocated pointer is going beyond its limits. The > > newer upstream dlmalloc() has a checker which might help. I fiddled > > around a bit but could not work one where the problem was. > > Ok, but I don't want us to pull code with random values that happened > to work during testing. We need to understand why It is presumably because of bugs in the EFI code. The current code adds about 4KB to the size of each allocation, so adding 100 bytes is at least an improvement. I did do some digging but couldn't immediately locate any bugs. To be honest I am not sure what is going on. But once I have these two EFI series in, I will spend some time digging into it and help fix these (presumed) bugs. > > > > > > > > > > + if (!ptr) > > > > + return EFI_OUT_OF_RESOURCES; > > > > + > > > > + *buffer = ptr; > > > > + r = EFI_SUCCESS; > > > > + log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr); > > > > > > So as I commented above, I think this is papering over whatever > > > problem you are seeing. If you want to move the pool to use malloc() > > > that's fine, but *all* of the pool allocations should use it. Not just > > > boot services because its easier to retrofit it on the current code. > > > > Please see above. Also, please see the commit message. This change > > actually solves the problems I am seeing, quite well. > > I did and the only 'problem' that is mentioned is polluting and > overwriting memory of scripts etc. But that goes away with LMB AFAICT. > So the only advantage is that you save a few kbs of space when > requesting pool allocations? No, you misunderstand. I am happy to arrange a call to go through this if it is still unclear from my comment above. When I say this is orthogonal to the lmb series, I really do mean that. Regards, Simon > > Thanks > /Ilias > > > > > > > > > + } 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; > > > > > > > [...] > > > > Regards, > > Simon
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); + /* 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();
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 <sjg@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(-)