Hi Ilias, On Mon, 23 Sept 2024 at 12:03, Ilias Apalodimas <[email protected]> wrote: > > On Fri, 20 Sept 2024 at 19:03, Simon Glass <[email protected]> wrote: > > > > Hi Ilias, > > > > On Fri, 20 Sept 2024 at 14:10, Ilias Apalodimas > > <[email protected]> wrote: > > > > > > Hi Simon > > > > > > A few more comments after looking into this a bit more > > > > > > On Mon, Sep 16, 2024 at 05:42:59PM +0200, Simon Glass wrote: > > > > Hi Ilias, > > > > > > > > On Thu, 12 Sept 2024 at 08:37, Ilias Apalodimas > > > > <[email protected]> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, 6 Sept 2024 at 16:01, Simon Glass <[email protected]> wrote: > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > On Fri, 6 Sept 2024 at 01:13, Ilias Apalodimas > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > Apologies for the late reply, I was attending a conference. > > > > > > > > > > > > > > On Mon, 2 Sept 2024 at 01:23, Simon Glass <[email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > This API call is intended for allocating small amounts of > > > > > > > > memory, > > > > > > Well it's not only for small memory. Look below. > > > > Yes, you are right, I was imprecise. In U-Boot, it it used for > > allocations or small amounts of memory. Until the app runs, we have > > control over this, so can be sure the allocations will be small. After > > that, allocations could be very large. > > > > > > > > > > > > > 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. > > > > > > Yes and there is a reason for that. The EFI spec description is > > > > > > "The AllocatePool() function allocates a memory region of Size bytes > > > from memory of type PoolType and returns the address of the allocated > > > memory in the location referenced by Buffer. This function allocates > > > *pages* from EfiConventionalMemory as needed to grow the requested pool > > > type. All allocations are eight-byte aligned. The allocated pool memory > > > is returned to the available pool with the EFI_BOOT_SERVICES.FreePool() > > > function" > > > > > > The current implementation of efi_allocate_pool kind of does that, but is > > > indeed very inefficient, since it allocates a single page and pool every > > > time. > > > A proper implementation would be to allocate a page and return the size > > > needed. Then for every subsequent request to the same pooltype, you look > > > up > > > the existing pool, and return the requested memory. If the pool is > > > depleted > > > you allocate another *page* and so on and so forth. That's where the > > > metadata is actually useful. > > > > > > So the suggested solution here is trying to fix an existing hack, by > > > papering over the problem with *another* hack that will basically make the > > > process of fixing efi_allocate_pool() properly impossible. > > > > It actually makes it easier, doesn't it? We can decrease the 'margin' > > I have set in the patch until we see the first failure, then fix the > > bug thus exposed, decrease it again, etc. Eventually when all the bugs > > are fixed, the margin will be zero. > > Not really. The AllocatePool says "it must allocate pages" for a > reason, which we did discuss in earlier versions of the patch. How do > you plan to deal with memory attributes and permissions when > allocating memory, which you can only control per page?
How about this explantion?: 1) The pool-allocation is only used by U-Boot itself until the app starts (obviously :-) 2) Until that point all pool allocations are boot-services data 3) We can therefore simply mark the malloc() region as boot-services data 4) If what you say is true, how does that square with add_u_boot_and_runtime() which sets the whole region (code and malloc() region, etc.) to boot-services code? > > EFI is a spec. You either adhere to it or not. You can't change random > parts because it interferes with U-Boot image loading assumptions that > were in place *before* merging EFI. I have no intention of changing any parts, random or otherwise > This is a memory dump on QEMU aarch64. and > > CONVENTIONAL 0000000040000000-000000023d52e000 WB > BOOT DATA 000000023d52e000-000000023d533000 WB > RUNTIME DATA 000000023d533000-000000023d534000 WB|RT > BOOT DATA 000000023d534000-000000023d535000 WB > ACPI NVS 000000023d535000-000000023d546000 WB > BOOT DATA 000000023d546000-000000023d558000 WB > RUNTIME DATA 000000023d558000-000000023d57a000 WB|RT > BOOT DATA 000000023d57a000-000000023d585000 WB > BOOT CODE 000000023d585000-000000023e6a3000 WB > RUNTIME DATA 000000023e6a3000-000000023e6a4000 WB|RT > BOOT CODE 000000023e6a4000-000000023f6c0000 WB > RUNTIME CODE 000000023f6c0000-000000023f6d0000 WB|RT > BOOT CODE 000000023f6d0000-0000000240000000 WB > > As you can see the memory from the bottom is empty and free to load > images. efi_find_free_memory() is always trying to allocate the > highest available memory and stay out of the way. That's good, but not quite good enough. We need to use the existing malloc() pool until the app starts. > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > So why isn't this an LMB problem since we plan to use it for EFI > > > allocations? LMB is trying to fix similar issues for efi_allocate_pages(). > > > Once that's done efi_allocate_pool() which calls efi_allocate_pages() is > > > automatically sorted. So the only problem is efficiency, no ? > > > > We are still not on the same page with this. If you happen to be at > > Plumbers, let's have a chat. Otherwise we can try to have a call to go > > over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > What we can do, is go over the usage of efi_allocate_pool(). A quick grep > > > showed a few uses, but I am pretty certain those can be replaced by > > > efi_allocate_pages(). > > > > Oh dear, please no. Until the EFI app runs, we should not use that > > function. It uses memory which should not be used for simple > > allocations, but instead kept free for images. > > So this is one more point..., efi_allocate_pages is used more than EFI > allocate pool. So I really don't see any point in merging this. > AFAICT it makes it impossible to fix memory permissions, it only deals > with a very small fraction of the problem, it fragments the EFI memory > management since random parts go via malloc now etc etc. It is so odd that we are completely talking at cross-purposes after so many weeks. The fragmenting of memory is the current state. All of U-Boot's allocations go through malloc(), except for EFI which have their own, strange, confusing, unnecessary variation. We need to clean this up. EFI should use malloc() just like any other *part of U-Boot*. When and if we get to a patch to change the memory permissions, we can do so for the whole malloc() region, surely? It doesn't make a lot of sense to me to allow code execution in the malloc() region... Regards, Simon > > > Thanks > > /Ilias > > > > > > Regards, > > Simon > > > > > > > > > > Thanks > > > /Ilias > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 <[email protected]> > > > > > > > > --- > > > > > > > > > > > > > > > > (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 > > > > > > Thanks > > > /Ilias
