Hi Simon
On Fri, 26 Jul 2024 at 17:54, Simon Glass <s...@chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 26 Jul 2024 at 02:31, Ilias Apalodimas
> <ilias.apalodi...@linaro.org> wrote:
> >
> > Hi Simon,
> >
> >
> > On Fri, 26 Jul 2024 at 02:33, Simon Glass <s...@chromium.org> wrote:
> > >
> > > Hi Heinrich,
> > >
> > > On Thu, 25 Jul 2024 at 09:54, Heinrich Schuchardt <xypron.g...@gmx.de>
> > > wrote:
> > > >
> > > > On 25.07.24 15:56, Simon Glass 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.
> > > > >
> > > > > Use U-Boot's built-in malloc() instead, to avoid these problems:
> > > > >
> > > > > - it should normally be large enough for pool allocations
> > > > > - if it isn't we can enforce a minimum size for boards which use
> > > > > EFI_LOADER
> > > > > - the existing mechanism may create an unwatned entry in the memory
> > > > > map
> > > > > - it is used for most EFI allocations already
> > > > >
> > > > > One side effect is that this seems to be showing up some bugs in the
> > > > > EFI code, since the malloc() pool becomes corrupted with some tests.
> > > > > This has likely crept in due to the very large gaps between
> > > > > allocations
> > > > > (around 4KB), which provides a lot of leeway when the allocation size
> > > > > is
> > > > > too small. Work around this by increasing the size for now, until
> > > > > these
> > > > > (presumed) bugs are located.
> > > > >
> > > > > Signed-off-by: Simon Glass <s...@chromium.org>
> > > > > ---
> > > > >
> > > > > lib/efi_loader/efi_memory.c | 93
> > > > > ++++++++-----------------------------
> > > > > 1 file changed, 19 insertions(+), 74 deletions(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > > > index 2945f5648c7..fabe9e3a87a 100644
> > > > > --- a/lib/efi_loader/efi_memory.c
> > > > > +++ b/lib/efi_loader/efi_memory.c
> > > > > @@ -80,45 +80,6 @@ static LIST_HEAD(efi_mem);
> > > > > void *efi_bounce_buffer;
> > > > > #endif
> > > > >
> > > > > -/**
> > > > > - * struct efi_pool_allocation - memory block allocated from pool
> > > > > - *
> > > > > - * @num_pages: number of pages allocated
> > > > > - * @checksum: checksum
> > > > > - * @data: allocated pool memory
> > > > > - *
> > > > > - * U-Boot services each UEFI AllocatePool() request as a separate
> > > > > - * (multiple) page allocation. We have to track the number of pages
> > > > > - * to be able to free the correct amount later.
> > > > > - *
> > > > > - * 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.
> > > > > - */
> > > > > -struct efi_pool_allocation {
> > > > > - u64 num_pages;
> > > > > - u64 checksum;
> > > > > - char data[] __aligned(ARCH_DMA_MINALIGN);
> > > > > -};
> > > > > -
> > > > > -/**
> > > > > - * checksum() - calculate checksum for memory allocated from pool
> > > > > - *
> > > > > - * @alloc: allocation header
> > > > > - * Return: checksum, always non-zero
> > > > > - */
> > > > > -static u64 checksum(struct efi_pool_allocation *alloc)
> > > > > -{
> > > > > - u64 addr = (uintptr_t)alloc;
> > > > > - u64 ret = (addr >> 32) ^ (addr << 32) ^ alloc->num_pages ^
> > > > > - EFI_ALLOC_POOL_MAGIC;
> > > > > - if (!ret)
> > > > > - ++ret;
> > > > > - return ret;
> > > > > -}
> > > > > -
> > > > > /**
> > > > > * efi_mem_cmp() - comparator function for sorting memory map
> > > > > *
> > > > > @@ -681,13 +642,10 @@ void *efi_alloc_aligned_pages(u64 len, int
> > > > > memory_type, size_t align)
> > > > > * @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));
> > > > > + void *ptr;
> > > > >
> > > > > if (!check_allowed())
> > > > > return EFI_UNSUPPORTED;
> > > > > @@ -700,16 +658,21 @@ efi_status_t efi_allocate_pool(enum
> > > > > efi_memory_type pool_type, efi_uintn_t size,
> > > > > return EFI_SUCCESS;
> > > > > }
> > > >
> > > > Unfortunately this patch does not match the UEFI specification:
> > > >
> > > > Two different memory types cannot reside in the same memory page. You
> > > > have to keep track of the memory type of each allocated memory page and
> > > > report it in GetMemoryMap().
> > >
> > > I actually hadn't thought about that. Is it implied in the spec or
> > > actually stated somewhere?
> >
> > I don't remember if it's explicitly stated, but it *really* doesn't
> > have to be. Although u-boot doesn't currently support this on modern
> > secure systems you can't have code and data mixed on the same page.
> > This would prevent you to map pages with proper permissions and take
> > advantage of CPU security features e.g RW^X.
>
> As it stands, U-Boot's data is reported as EFI_BOOT_SERVICES_CODE
> through EFI. Perhaps the difference between EFI_BOOT_SERVICES_CODE and
> EFI_BOOT_SERVICES_DATA isn't that important?
>
> But if we did want to fix that, we could make the malloc region
> EFI_BOOT_SERVICES_DATA.
>
> >
> > In any case, the AllocatePool description reads "This function
> > allocates pages from EfiConventionalMemory as needed to grow the
> > requested pool type. All allocations are eight-byte aligned". I don't
> > think using malloc in AllocatePool is appropriate. If we ever want to
> > do that and allocate from the malloc space, we need to teach malloc
> > some EFI semantics, but that's a really bad idea.
>
> Here I don't see the difference between EFI's malloc(n) and
> memalign(8, n). I do see a lot of comments like 'use the spec', 'read
> the spec'. It seems very clear to me that the bootloader can use
> whatever algorithm it likes to provide the allocated memory for the
> pool.
>
> What sort of EFI semantics are you referring to?
the memory type. Your patch removes the call to efi_allocate_pages
which tracks it.
>
> >
> > >
> > > Anyway, from what I can tell, we mostly use EFI_BOOT_SERVICES_DATA.
> > > The only exception is EFI_RUNTIME_SERVICES_DATA for the runtime stuff.
> >
> > That's not entirely correct, we use a lot more than these 2.
> > EFI_LOADER_DATA, EFI_LOADER_CODE and EFI_ACPI_RECLAIM_MEMORY from a
> > quick grep
>
> Yes I did a quick grep too, but then checked each site. The first two
> are used in an EFI app, not U-Boot itself. The ACPI one is not an
> allocation.
>
> > >
> > > So perhaps we can use malloc() for EFI_BOOT_SERVICES_DATA allocations
> > > and stick with whole pages otherwise? That will mostly fix the problem
> > > I am seeing.
>
> Any comments on this?
This violates the spec and probably breaks a few tests and the EFI
boot services API, as you are supposed to be able to define the memory
type. We might be able to control it from U-Boot, but EFI apps are
going to end up being buggy and hard to debug -- e.g an app calls
allocatePool to allocate memory that needs to be preserved at runtime.
You can switch some callsite of efi_allocate_pool to
efi_allocate_pages() internally. Will that fix the behavior?
Regards
/Ilias
>
> > >
> > > >
> > > > The specification is available https://uefi.org/specifications.
> > >
> > > The problem is that it is thousands of pages so it is hard to pick up
> > > on some of these issues.
> > >
> > > >
> > > > >
> > > > > - 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;
> > > > > - }
> > > > > + /*
> > > > > + * 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);
> > > >
> > > > We were using ARCH_DMA_MINALIGN before this patch.
> > >
> > > Where is that in the code? All I see is EFI_PAGE_SHIFT.
> > >
> > > >
> > > > If we are overwriting allocated memory, we must fix that instead of
> > > > increasing size. Do you have a git tag showing the problem?
> > >
> > > Not easy to do, because the 4KB per allocation thing has been there
> > > from the start. It might be possible to apply my patch to earlier and
> > > earlier U-Boots to try to bisect it.
>
> Would anyone like to take a look at these presumed bugs?
>
> It might be worth moving to a newer dlmalloc() [1,2] as it has a checker.
>
> > >
> [..]
>
> Regards,
> Simon
>
> [1] https://gee.cs.oswego.edu/dl/html/malloc.html
> [2] https://gee.cs.oswego.edu/pub/misc/malloc.c
