Hi Tom,
On Tue, 30 Jul 2024 at 14:11, Tom Rini <tr...@konsulko.com> wrote:
>
> On Tue, Jul 30, 2024 at 02:05:19PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 30 Jul 2024 at 13:49, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Tue, Jul 30, 2024 at 01:42:17PM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 30 Jul 2024 at 09:24, Tom Rini <tr...@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jul 30, 2024 at 09:18:02AM -0600, Simon Glass wrote:
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Tue, 30 Jul 2024 at 08:54, Ilias Apalodimas
> > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, 30 Jul 2024 at 17:38, Simon Glass <s...@chromium.org>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Ilias,
> > > > > > > >
> > > > > > > > On Tue, 30 Jul 2024 at 02:20, Ilias Apalodimas
> > > > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, 29 Jul 2024 at 18:28, Simon Glass <s...@chromium.org>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Ilias,
> > > > > > > > > >
> > > > > > > > > > On Mon, 29 Jul 2024 at 04:02, Ilias Apalodimas
> > > > > > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > I can put that code back, depending on what we decide below.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > To be more specific, I am suggesting:
> > > > > > > > > > - use malloc() for EFI_BOOT_SERVICES_DATA allocations (with
> > > > > > > > > > no memory
> > > > > > > > > > type since it is always that). This should cover all the
> > > > > > > > > > U-Boot
> > > > > > > > > > allocations and make sure they are safely within the
> > > > > > > > > > malloc() pool
> > > > > > > > > > - use efi_allocate_pages() for other memory types (keeping
> > > > > > > > > > the memory
> > > > > > > > > > type in metadata)
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Personally, I don't see the point and we deviate from the
> > > > > > > > > spec as
> > > > > > > > > well. Perhaps Heinrich thinks otherwise, but the EFI spec
> > > > > > > > > still says
> > > > > > > > > you need to allocate *pages* to grow the pool. What's missing
> > > > > > > > > from our
> > > > > > > > > efi_allocate_pool() is the ability to merge allocations of
> > > > > > > > > the same
> > > > > > > > > memory type to an existing pool assuming there's space,
> > > > > > > > > rather than
> > > > > > > > > requesting a new pool of 4kb.
> > > > > > > >
> > > > > > > > "This function allocates pages from EfiConventionalMemory as
> > > > > > > > needed to
> > > > > > > > grow the requested pool type". So far as EFI is concerned, the
> > > > > > > > malloc() region is in EfiConventionalMemory, so I don't see any
> > > > > > > > deviation.
> > > > > > > >
> > > > > > > > Please also see below.
> > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > You can switch some callsite of efi_allocate_pool to
> > > > > > > > > > > efi_allocate_pages() internally. Will that fix the
> > > > > > > > > > > behavior?
> > > > > > > > > >
> > > > > > > > > > It still allocates memory 'in space' and uses 4KB for each
> > > > > > > > > > allocation.
> > > > > > > > > >
> > > > > > > >
> > > > > > > > This is the key point that doesn't seem to be coming across. The
> > > > > > > > current allocator is allocating memory wherever it likes,
> > > > > > > > potentially
> > > > > > > > interfering with the kernel_addr_r addresses, etc. as on
> > > > > > > > qemu_arm.
> > > > > > >
> > > > > > > It is, but I don't think using malloc is solving it. It's papering
> > > > > > > over the problem, because if someone in the future launches an
> > > > > > > EFI app
> > > > > > > or allocates EFI memory with a different type you are back on the
> > > > > > > same
> > > > > > > problem.
> > > > > >
> > > > > > It is certainly solving this problem.
> > > > > >
> > > > > > Once the app is launched it is OK to overwrite memory...after all it
> > > > > > has been loaded and is running. The issue is that these little
> > > > > > allocations can end up anywhere in memory. Did you see the qemu_arm
> > > > > > note?
> > > > >
> > > > > Isn't this another part of why we need the LMB rework? So that
> > > > > kernel_addr_r, et al, can be marked as reserved.
> > > >
> > > > If we mark them as reserved, we won't be able to load a file into that
> > > > region, so boot scripts will fail.
> > >
> > > No? We mark it as being overwriteable but allocated. This is part of the
> > > LMB rework series.
> >
> > Yes, I see, that makes sense. I don't know any way to guess the
> > expected size of the kernel or ramdisk. I see that Apple uses 128MB
> > for the kernel (plenty) and 1GB for the ramdisk.
>
> Yes, 128MiB is a practical limit. Ramdisk is where it gets trickier.
>
> > > > Please take a look at the whole series and let me know if there is
> > > > anything missing from the descriptions I have given. I have had this
> > > > problem in the back of my mind for some time...but just a few hours of
> > > > investigation was enough to determine that it really is broken.
> > >
> > > I'm missing something, sorry. Yes, it is known that EFI can make some
> > > incorrect assumptions about what memory is/isn't available (as other
> > > implementations give EFI the world to work with), hence the LMB rework
> > > series to address some of these problems.
> >
> > My goal here is to tidy up EFI memory allocation so that it doesn't
> > result in allocating memory in strange places. Avoiding overlaps is
> > one thing, but the way this is heading, we will get overlap errors
> > randomly on platforms when someone tries to load something into RAM,
> > or we won't protect things that need to be protected. It is all a bit
> > mushy without a proper design.
> >
> > Does that make sense?
>
> Well, to me step one is to get the lmb series done so that if something
> needs a range of memory it can both check if it's available and mark it
> as unavailable to others. As yes, we have something analogous to
> cooperative multitasking when it comes to memory management today, and
> that doesn't always work out.
OK, and that is in progress.
>
> Step two is getting the new lmb series and EFI_LOADER talking,
What sort of talking? Bear in mind that EFI_LOADER currently does
allocations even if it isn't used.
> so that
> step three can be seeing what tweaks may be needed in where things
> allocate memory.
So my series is step 3?
Regards,
SImon
