[PATCH v3 0/3] efi: Start tidying up memory management

[PATCH v3 0/3] efi: Start tidying up memory management

[Simon Glass]

We have been discussing the state of EFI memory management for some
years so I thought it might be best to send a short series showing some
of the issues we have talked about.

This one just deals with memory allocation. It provides a way to detect
EFI 'page allocations' when U-Boot' malloc() should be used. It should
help us to clean up this area of U-Boot.

It also updates EFI to use U-Boot memory allocation for the pool. Most
functions use that anyway and it is much more efficient. It also avoids
allocating things 'in space' in conflict with the loaded images.

This series also includes a little patch to show more information in
the index for the EFI pages.

Note that this series is independent from Sugosh's lmb series[1],
although I believe it will point the way to simplifying some of the
later patches in that series.

Overall, some issues which should be resolved are:
- allocation inconsistency, e.g. efi_add_protocol() uses malloc() but
  efi_dp_dup() does not (this series makes a start)
- change efi_bl_init() to register events later, when the devices are
  actually used
- rather than efi_set_bootdev(), provide a bootstd way to take note of
  the device images came from and allow EFI to query that when needed
- EFI_LOADER_BOUNCE_BUFFER - can use U-Boot bounce buffers?

Minor questions on my mind:
- is unaligned access useful? Is there a performance penalty?
- API confusion about whether something is an address or a pointer

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=416441

Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()

Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address

Simon Glass (3):
  efi: Drop the memset() from efi_alloc()
  efi: Allow use of malloc() for the EFI pool
  efi: Show the location of the bounce buffer

 common/dlmalloc.c            |   7 +++
 include/efi_loader.h         |  29 ++++++++-
 include/malloc.h             |   7 +++
 lib/efi_loader/efi_bootbin.c |  11 ++++
 lib/efi_loader/efi_memory.c  | 119 ++++++++++++++++++++++++-----------
 5 files changed, 136 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] efi: Drop the memset() from efi_alloc()

[Simon Glass]

From my inspection none of the users need the memory to be zeroed. It
is somewhat unexpected that it does so, since the name gives no clue to
this.

Drop the memset() so that it effectively becomes a wrapper around the
normal EFI-pool allocator.

Another option would be to drop this function and call
efi_allocate_pool() directly, but that increase code size a little.

Move the function comment to the header file like most other exported
functions in U-Boot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()

 include/efi_loader.h        | 11 ++++++++++-
 lib/efi_loader/efi_memory.c |  9 ---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index f84852e384f..38971d01442 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
  * Return:	size in pages
  */
 #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
-/* Allocate boot service data pool memory */
+
+/**
+ * efi_alloc() - allocate boot services data pool memory
+ *
+ * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA
+ *
+ * @size:	number of bytes to allocate
+ * Return:	pointer to allocated memory, or NULL if out of memory
+ */
 void *efi_alloc(size_t len);
+
 /* Allocate pages on the specified alignment */
 void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
 /* More specific EFI memory allocator, called by EFI payloads */
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index c6f1dd09456..50cb2f3898b 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
 	return r;
 }
 
-/**
- * efi_alloc() - allocate boot services data pool memory
- *
- * Allocate memory from pool and zero it out.
- *
- * @size:	number of bytes to allocate
- * Return:	pointer to allocated memory or NULL
- */
 void *efi_alloc(size_t size)
 {
 	void *buf;
@@ -681,7 +673,6 @@ void *efi_alloc(size_t size)
 		log_err("out of memory");
 		return NULL;
 	}
-	memset(buf, 0, size);
 
 	return buf;
 }
-- 
2.34.1

Re: [PATCH v3 1/3] efi: Drop the memset() from efi_alloc()

[Heinrich Schuchardt]

On 02.09.24 00:22, Simon Glass wrote:
>  From my inspection none of the users need the memory to be zeroed. It
> is somewhat unexpected that it does so, since the name gives no clue to
> this.

Though the UEFI specification does not require it, EDK II uses
AllocateZeroPool() to implement
EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode(). We should not
deviate from it.

>
> Drop the memset() so that it effectively becomes a wrapper around the
> normal EFI-pool allocator.
>
> Another option would be to drop this function and call
> efi_allocate_pool() directly, but that increase code size a little.
>
> Move the function comment to the header file like most other exported
> functions in U-Boot.

Yes, we should move all non-static EFI function descriptions to headers.
But it makes no sense to move the comments of a single function and
leave the other functions unchanged. Have a look at doc/api/efi.rst.

Best regards

Heinrich

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Add new patch to drop the memset() from efi_alloc()
> - Drop patch to convert device_path allocation to use malloc()
>
>   include/efi_loader.h        | 11 ++++++++++-
>   lib/efi_loader/efi_memory.c |  9 ---------
>   2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index f84852e384f..38971d01442 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>    * Return:	size in pages
>    */
>   #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
> -/* Allocate boot service data pool memory */
> +
> +/**
> + * efi_alloc() - allocate boot services data pool memory
> + *
> + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA
> + *
> + * @size:	number of bytes to allocate
> + * Return:	pointer to allocated memory, or NULL if out of memory
> + */
>   void *efi_alloc(size_t len);
> +
>   /* Allocate pages on the specified alignment */
>   void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
>   /* More specific EFI memory allocator, called by EFI payloads */
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index c6f1dd09456..50cb2f3898b 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>   	return r;
>   }
>
> -/**
> - * efi_alloc() - allocate boot services data pool memory
> - *
> - * Allocate memory from pool and zero it out.
> - *
> - * @size:	number of bytes to allocate
> - * Return:	pointer to allocated memory or NULL
> - */
>   void *efi_alloc(size_t size)
>   {
>   	void *buf;
> @@ -681,7 +673,6 @@ void *efi_alloc(size_t size)
>   		log_err("out of memory");
>   		return NULL;
>   	}
> -	memset(buf, 0, size);
>
>   	return buf;
>   }

Re: [PATCH v3 1/3] efi: Drop the memset() from efi_alloc()

[Simon Glass]

Hi Heinrich,

On Sat, 14 Sept 2024 at 09:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 02.09.24 00:22, Simon Glass wrote:
> >  From my inspection none of the users need the memory to be zeroed. It
> > is somewhat unexpected that it does so, since the name gives no clue to
> > this.
>
> Though the UEFI specification does not require it, EDK II uses
> AllocateZeroPool() to implement
> EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode(). We should not
> deviate from it.

Are you saying that there is an unwritten convention in the spec? My
inspection of the code leads me to believe that all of the bytes which
are allocated are written to, within that code, so that zeroing the
bytes serves no purpose.

My goal here is to allow use of malloc(), instead of calloc(), for example.

>
> >
> > Drop the memset() so that it effectively becomes a wrapper around the
> > normal EFI-pool allocator.
> >
> > Another option would be to drop this function and call
> > efi_allocate_pool() directly, but that increase code size a little.
> >
> > Move the function comment to the header file like most other exported
> > functions in U-Boot.
>
> Yes, we should move all non-static EFI function descriptions to headers.
> But it makes no sense to move the comments of a single function and
> leave the other functions unchanged. Have a look at doc/api/efi.rst.

I normally like to do these things one at a time, when changes are
needed, to avoid massive wholesale changes with no other purpose. That
is what has happened with image.h over time. But OK I can drop that
part of the patch, once we sort out the zeroring.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Add new patch to drop the memset() from efi_alloc()
> > - Drop patch to convert device_path allocation to use malloc()
> >
> >   include/efi_loader.h        | 11 ++++++++++-
> >   lib/efi_loader/efi_memory.c |  9 ---------
> >   2 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index f84852e384f..38971d01442 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >    * Return:  size in pages
> >    */
> >   #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
> > -/* Allocate boot service data pool memory */
> > +
> > +/**
> > + * efi_alloc() - allocate boot services data pool memory
> > + *
> > + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA
> > + *
> > + * @size:    number of bytes to allocate
> > + * Return:   pointer to allocated memory, or NULL if out of memory
> > + */
> >   void *efi_alloc(size_t len);
> > +
> >   /* Allocate pages on the specified alignment */
> >   void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
> >   /* More specific EFI memory allocator, called by EFI payloads */
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index c6f1dd09456..50cb2f3898b 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> >       return r;
> >   }
> >
> > -/**
> > - * efi_alloc() - allocate boot services data pool memory
> > - *
> > - * Allocate memory from pool and zero it out.
> > - *
> > - * @size:    number of bytes to allocate
> > - * Return:   pointer to allocated memory or NULL
> > - */
> >   void *efi_alloc(size_t size)
> >   {
> >       void *buf;
> > @@ -681,7 +673,6 @@ void *efi_alloc(size_t size)
> >               log_err("out of memory");
> >               return NULL;
> >       }
> > -     memset(buf, 0, size);
> >
> >       return buf;
> >   }
>


Regards,
Simon

Re: [PATCH v3 1/3] efi: Drop the memset() from efi_alloc()

[Heinrich Schuchardt]

On 19.09.24 16:13, Simon Glass wrote:
> Hi Heinrich,
>
> On Sat, 14 Sept 2024 at 09:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 02.09.24 00:22, Simon Glass wrote:
>>>   From my inspection none of the users need the memory to be zeroed. It
>>> is somewhat unexpected that it does so, since the name gives no clue to
>>> this.
>>
>> Though the UEFI specification does not require it, EDK II uses
>> AllocateZeroPool() to implement
>> EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode(). We should not
>> deviate from it.
>
> Are you saying that there is an unwritten convention in the spec? My
> inspection of the code leads me to believe that all of the bytes which
> are allocated are written to, within that code, so that zeroing the
> bytes serves no purpose.

EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode() is an API:

You have to consider the current and future usage outside U-Boot.

For saving a few microseconds I would not want to give up compatibility
with EDK II.

>
> My goal here is to allow use of malloc(), instead of calloc(), for example.

The caller of the API has to release with the memory with FreePool(). So
we should allocate it with AllocatePool().

See chapter 10.5.8
"EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode()" in the UEFI
specification.

Best regards

Heinrich

>
>>
>>>
>>> Drop the memset() so that it effectively becomes a wrapper around the
>>> normal EFI-pool allocator.
>>>
>>> Another option would be to drop this function and call
>>> efi_allocate_pool() directly, but that increase code size a little.
>>>
>>> Move the function comment to the header file like most other exported
>>> functions in U-Boot.
>>
>> Yes, we should move all non-static EFI function descriptions to headers.
>> But it makes no sense to move the comments of a single function and
>> leave the other functions unchanged. Have a look at doc/api/efi.rst.
>
> I normally like to do these things one at a time, when changes are
> needed, to avoid massive wholesale changes with no other purpose. That
> is what has happened with image.h over time. But OK I can drop that
> part of the patch, once we sort out the zeroring.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v3:
>>> - Add new patch to drop the memset() from efi_alloc()
>>> - Drop patch to convert device_path allocation to use malloc()
>>>
>>>    include/efi_loader.h        | 11 ++++++++++-
>>>    lib/efi_loader/efi_memory.c |  9 ---------
>>>    2 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index f84852e384f..38971d01442 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>>>     * Return:  size in pages
>>>     */
>>>    #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
>>> -/* Allocate boot service data pool memory */
>>> +
>>> +/**
>>> + * efi_alloc() - allocate boot services data pool memory
>>> + *
>>> + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA
>>> + *
>>> + * @size:    number of bytes to allocate
>>> + * Return:   pointer to allocated memory, or NULL if out of memory
>>> + */
>>>    void *efi_alloc(size_t len);
>>> +
>>>    /* Allocate pages on the specified alignment */
>>>    void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
>>>    /* More specific EFI memory allocator, called by EFI payloads */
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index c6f1dd09456..50cb2f3898b 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>>>        return r;
>>>    }
>>>
>>> -/**
>>> - * efi_alloc() - allocate boot services data pool memory
>>> - *
>>> - * Allocate memory from pool and zero it out.
>>> - *
>>> - * @size:    number of bytes to allocate
>>> - * Return:   pointer to allocated memory or NULL
>>> - */
>>>    void *efi_alloc(size_t size)
>>>    {
>>>        void *buf;
>>> @@ -681,7 +673,6 @@ void *efi_alloc(size_t size)
>>>                log_err("out of memory");
>>>                return NULL;
>>>        }
>>> -     memset(buf, 0, size);
>>>
>>>        return buf;
>>>    }
>>
>
>
> Regards,
> Simon

Re: [PATCH v3 1/3] efi: Drop the memset() from efi_alloc()

[Simon Glass]

Hi Heinrich,

On Mon, 23 Sept 2024 at 14:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 19.09.24 16:13, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sat, 14 Sept 2024 at 09:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 02.09.24 00:22, Simon Glass wrote:
> >>>   From my inspection none of the users need the memory to be zeroed. It
> >>> is somewhat unexpected that it does so, since the name gives no clue to
> >>> this.
> >>
> >> Though the UEFI specification does not require it, EDK II uses
> >> AllocateZeroPool() to implement
> >> EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode(). We should not
> >> deviate from it.
> >
> > Are you saying that there is an unwritten convention in the spec? My
> > inspection of the code leads me to believe that all of the bytes which
> > are allocated are written to, within that code, so that zeroing the
> > bytes serves no purpose.
>
> EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode() is an API:
>
> You have to consider the current and future usage outside U-Boot.
>
> For saving a few microseconds I would not want to give up compatibility
> with EDK II.

What sort of compatibility are you referring to? Are we implementing a
spec or copying Tianocore? Also, did you miss the second sentence in
my response?

>
> >
> > My goal here is to allow use of malloc(), instead of calloc(), for example.
>
> The caller of the API has to release with the memory with FreePool(). So
> we should allocate it with AllocatePool().
>
> See chapter 10.5.8
> "EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode()" in the UEFI
> specification.

Yes, my patch does not change that.

Regards,
Simon


>
> Best regards
>
> Heinrich
>
> >
> >>
> >>>
> >>> Drop the memset() so that it effectively becomes a wrapper around the
> >>> normal EFI-pool allocator.
> >>>
> >>> Another option would be to drop this function and call
> >>> efi_allocate_pool() directly, but that increase code size a little.
> >>>
> >>> Move the function comment to the header file like most other exported
> >>> functions in U-Boot.
> >>
> >> Yes, we should move all non-static EFI function descriptions to headers.
> >> But it makes no sense to move the comments of a single function and
> >> leave the other functions unchanged. Have a look at doc/api/efi.rst.
> >
> > I normally like to do these things one at a time, when changes are
> > needed, to avoid massive wholesale changes with no other purpose. That
> > is what has happened with image.h over time. But OK I can drop that
> > part of the patch, once we sort out the zeroring.
> >>>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> ---
> >>>
> >>> Changes in v3:
> >>> - Add new patch to drop the memset() from efi_alloc()
> >>> - Drop patch to convert device_path allocation to use malloc()
> >>>
> >>>    include/efi_loader.h        | 11 ++++++++++-
> >>>    lib/efi_loader/efi_memory.c |  9 ---------
> >>>    2 files changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index f84852e384f..38971d01442 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >>>     * Return:  size in pages
> >>>     */
> >>>    #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
> >>> -/* Allocate boot service data pool memory */
> >>> +
> >>> +/**
> >>> + * efi_alloc() - allocate boot services data pool memory
> >>> + *
> >>> + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA
> >>> + *
> >>> + * @size:    number of bytes to allocate
> >>> + * Return:   pointer to allocated memory, or NULL if out of memory
> >>> + */
> >>>    void *efi_alloc(size_t len);
> >>> +
> >>>    /* Allocate pages on the specified alignment */
> >>>    void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
> >>>    /* More specific EFI memory allocator, called by EFI payloads */
> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> >>> index c6f1dd09456..50cb2f3898b 100644
> >>> --- a/lib/efi_loader/efi_memory.c
> >>> +++ b/lib/efi_loader/efi_memory.c
> >>> @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> >>>        return r;
> >>>    }
> >>>
> >>> -/**
> >>> - * efi_alloc() - allocate boot services data pool memory
> >>> - *
> >>> - * Allocate memory from pool and zero it out.
> >>> - *
> >>> - * @size:    number of bytes to allocate
> >>> - * Return:   pointer to allocated memory or NULL
> >>> - */
> >>>    void *efi_alloc(size_t size)
> >>>    {
> >>>        void *buf;
> >>> @@ -681,7 +673,6 @@ void *efi_alloc(size_t size)
> >>>                log_err("out of memory");
> >>>                return NULL;
> >>>        }
> >>> -     memset(buf, 0, size);
> >>>
> >>>        return buf;
> >>>    }
> >>
> >
> >
> > Regards,
> > Simon
>

[PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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);
+
 	/* 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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Sughosh Ganu]

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
>

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Sughosh Ganu]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Sughosh Ganu]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Heinrich Schuchardt]

On 12.09.24 02:59, Simon Glass wrote:
> 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.


AllocatePool() can be used to allocate multiple GiB of memory. The
malloc pool is too small for this purpose. You would first have to
change the malloc implementation to acquire arbitrary amounts of memory
from LMB.

Please, don't complicate the code by treating different memory types
differently in AllocatePool().

The way forward is to change LMB to store EFI memory types. This will
allow us to remove most of the code in lib/efi_loader/efi_memory.c.

Once that integration work is done we can start thinking about
generalizing malloc() to support multiple memory pools of arbitrary size
to encompass AllocatePool().

Best regards

Heinrich

>
>>>> 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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

Hi Heinrich,

On Mon, 23 Sept 2024 at 14:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12.09.24 02:59, Simon Glass wrote:
> > 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.
>
>
> AllocatePool() can be used to allocate multiple GiB of memory. The
> malloc pool is too small for this purpose. You would first have to
> change the malloc implementation to acquire arbitrary amounts of memory
> from LMB.
>
> Please, don't complicate the code by treating different memory types
> differently in AllocatePool().
>
> The way forward is to change LMB to store EFI memory types. This will
> allow us to remove most of the code in lib/efi_loader/efi_memory.c.
>
> Once that integration work is done we can start thinking about
> generalizing malloc() to support multiple memory pools of arbitrary size
> to encompass AllocatePool().

Unfortunately that is not the correct approach at all. I fully
understand where you are coming from, but the complexity involved is
wrong for U-Boot.

I do not have to have to change the malloc() implementation at all. It
is perfectly fine as it is. It is only EFI that needs to change
slightly, using malloc() for the pool until the app starts.

We also need to consider what happens when EFI_LOADER is disabled,
which I am intent on preserving.

Regards,
Simon


>
> Best regards
>
> Heinrich
>
> >
> >>>> 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
>

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Ilias Apalodimas]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Ilias Apalodimas]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

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

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Ilias Apalodimas]

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
> <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,

Well it's not only for small memory. Look below.

> > > > > 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.

> > > > >
> > > > > 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 ?

>
> >
> > > > >
> > > > > 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().

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 <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

Thanks
/Ilias

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

Hi Ilias,

On Fri, 20 Sept 2024 at 14:10, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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
> > <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,
>
> 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.

>
> > > > > >
> > > > > > 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.

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 <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
>
> Thanks
> /Ilias

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Ilias Apalodimas]

On Fri, 20 Sept 2024 at 19:03, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 20 Sept 2024 at 14:10, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> 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
> > > <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,
> >
> > 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?

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.
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.

>
> >
> > > > > > >
> > > > > > > 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.


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 <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
> >
> > Thanks
> > /Ilias

Re: [PATCH v3 2/3] efi: Allow use of malloc() for the EFI pool

[Simon Glass]

Hi Ilias,

On Mon, 23 Sept 2024 at 12:03, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 20 Sept 2024 at 19:03, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Fri, 20 Sept 2024 at 14:10, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> 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
> > > > <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,
> > >
> > > 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 <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
> > >
> > > Thanks
> > > /Ilias

[PATCH v3 3/3] efi: Show the location of the bounce buffer

[Simon Glass]

The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is not
clear whether it is still needed, but 24 boards (lx2160ardb_tfa_stmm,
lx2162aqds_tfa_SECURE_BOOT and the like) use it.

This feature uses EFI page allocation to create a 64MB buffer 'in space'
without any knowledge of where boards intend to load their images. This
may result in image corruption or other problems.

For example, if the feature is enabled on qemu_arm64 it puts the EFI
bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at
1088MB. The kernel is probably smaller than 27MB but the buffer does
overlap the ramdisk.

The solution is probably to use BOUNCE_BUFFER instead, with the EFI
version being dropped. For now, show the address of the EFI bounce
buffer so people have a better chance to detect the problem.

Note: I avoided converting this #ifdef to use IS_ENABLED() since I hope
that the feature may be removed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v2)

Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address

 lib/efi_loader/efi_bootbin.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
index 5bb0fdcf75d..9779dc09b5e 100644
--- a/lib/efi_loader/efi_bootbin.c
+++ b/lib/efi_loader/efi_bootbin.c
@@ -211,6 +211,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
 		return -1;
 	}
 
+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
+	/*
+	 * Add a warning about this buffer, since it may conflict with other
+	 * things
+	 */
+	log_warning("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,
+		    efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);
+#endif
+
 	ret = efi_install_fdt(fdt);
 	if (ret != EFI_SUCCESS)
 		return ret;
-- 
2.34.1

Re: [PATCH v3 3/3] efi: Show the location of the bounce buffer

[Heinrich Schuchardt]

Am 2. September 2024 00:22:59 MESZ schrieb Simon Glass <sjg@chromium.org>:
>The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is not
>clear whether it is still needed, but 24 boards (lx2160ardb_tfa_stmm,
>lx2162aqds_tfa_SECURE_BOOT and the like) use it.
>
>This feature uses EFI page allocation to create a 64MB buffer 'in space'
>without any knowledge of where boards intend to load their images. This
>may result in image corruption or other problems.
>
>For example, if the feature is enabled on qemu_arm64 it puts the EFI
>bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at
>1088MB. The kernel is probably smaller than 27MB but the buffer does
>overlap the ramdisk.
>
>The solution is probably to use BOUNCE_BUFFER instead, with the EFI
>version being dropped. For now, show the address of the EFI bounce
>buffer so people have a better chance to detect the problem.
>
>Note: I avoided converting this #ifdef to use IS_ENABLED() since I hope
>that the feature may be removed.

The functionality is still needed. A warning is of little use: what should the user do about it? The region should be reserved in LMB.

Best regards

Heinrich


>
>Signed-off-by: Simon Glass <sjg@chromium.org>
>---
>
>(no changes since v2)
>
>Changes in v2:
>- Drop patch 'Show more information in efi index'
>- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
>- Add the word 'warning', use log_warning() and show the end address
>
> lib/efi_loader/efi_bootbin.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
>index 5bb0fdcf75d..9779dc09b5e 100644
>--- a/lib/efi_loader/efi_bootbin.c
>+++ b/lib/efi_loader/efi_bootbin.c
>@@ -211,6 +211,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> 		return -1;
> 	}
> 
>+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
>+	/*
>+	 * Add a warning about this buffer, since it may conflict with other
>+	 * things
>+	 */
>+	log_warning("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,
>+		    efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);
>+#endif
>+
> 	ret = efi_install_fdt(fdt);
> 	if (ret != EFI_SUCCESS)
> 		return ret;

Re: [PATCH v3 3/3] efi: Show the location of the bounce buffer

[Simon Glass]

Hi Heinrich,

On Mon, 2 Sept 2024 at 05:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 2. September 2024 00:22:59 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is not
> >clear whether it is still needed, but 24 boards (lx2160ardb_tfa_stmm,
> >lx2162aqds_tfa_SECURE_BOOT and the like) use it.
> >
> >This feature uses EFI page allocation to create a 64MB buffer 'in space'
> >without any knowledge of where boards intend to load their images. This
> >may result in image corruption or other problems.
> >
> >For example, if the feature is enabled on qemu_arm64 it puts the EFI
> >bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at
> >1088MB. The kernel is probably smaller than 27MB but the buffer does
> >overlap the ramdisk.
> >
> >The solution is probably to use BOUNCE_BUFFER instead, with the EFI
> >version being dropped. For now, show the address of the EFI bounce
> >buffer so people have a better chance to detect the problem.
> >
> >Note: I avoided converting this #ifdef to use IS_ENABLED() since I hope
> >that the feature may be removed.
>
> The functionality is still needed. A warning is of little use: what should the user do about it? The region should be reserved in LMB.

I don't mind dropping this patch if we can get the other two applied.
I suppose my main purpose was to highlight this sort of issue and get
something in the commit log about it. For me, the message helped me
understand why memory regions were overlapping.

Regards,
Simon



>
> Best regards
>
> Heinrich
>
>
> >
> >Signed-off-by: Simon Glass <sjg@chromium.org>
> >---
> >
> >(no changes since v2)
> >
> >Changes in v2:
> >- Drop patch 'Show more information in efi index'
> >- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
> >- Add the word 'warning', use log_warning() and show the end address
> >
> > lib/efi_loader/efi_bootbin.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> >diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> >index 5bb0fdcf75d..9779dc09b5e 100644
> >--- a/lib/efi_loader/efi_bootbin.c
> >+++ b/lib/efi_loader/efi_bootbin.c
> >@@ -211,6 +211,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> >               return -1;
> >       }
> >
> >+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
> >+      /*
> >+       * Add a warning about this buffer, since it may conflict with other
> >+       * things
> >+       */
> >+      log_warning("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,
> >+                  efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);
> >+#endif
> >+
> >       ret = efi_install_fdt(fdt);
> >       if (ret != EFI_SUCCESS)
> >               return ret;

Re: [PATCH v3 0/3] efi: Start tidying up memory management

[Vincent Stehlé]

On Sun, Sep 01, 2024 at 04:22:56PM -0600, Simon Glass wrote:
> We have been discussing the state of EFI memory management for some
> years so I thought it might be best to send a short series showing some
> of the issues we have talked about.
> 
> This one just deals with memory allocation. It provides a way to detect
> EFI 'page allocations' when U-Boot' malloc() should be used. It should
> help us to clean up this area of U-Boot.
> 
> It also updates EFI to use U-Boot memory allocation for the pool. Most
> functions use that anyway and it is much more efficient. It also avoids
> allocating things 'in space' in conflict with the loaded images.

Hi Simon,

Thank you for this series.

I did test it on top of v2024.10-rc4 with AArch64 simulators (FVP & Qemu), with
OS boots and ACS-IR, and I see no regression. Same with sandbox on x86 and the
python tests.

Best regards,
Vincent.

> 
> This series also includes a little patch to show more information in
> the index for the EFI pages.
> 
> Note that this series is independent from Sugosh's lmb series[1],
> although I believe it will point the way to simplifying some of the
> later patches in that series.
> 
> Overall, some issues which should be resolved are:
> - allocation inconsistency, e.g. efi_add_protocol() uses malloc() but
>   efi_dp_dup() does not (this series makes a start)
> - change efi_bl_init() to register events later, when the devices are
>   actually used
> - rather than efi_set_bootdev(), provide a bootstd way to take note of
>   the device images came from and allow EFI to query that when needed
> - EFI_LOADER_BOUNCE_BUFFER - can use U-Boot bounce buffers?
> 
> Minor questions on my mind:
> - is unaligned access useful? Is there a performance penalty?
> - API confusion about whether something is an address or a pointer
> 
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=416441
> 
> Changes in v3:
> - Add new patch to drop the memset() from efi_alloc()
> - Drop patch to convert device_path allocation to use malloc()
> 
> Changes in v2:
> - Drop patch 'Show more information in efi index'
> - Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
> - Add the word 'warning', use log_warning() and show the end address
> 
> Simon Glass (3):
>   efi: Drop the memset() from efi_alloc()
>   efi: Allow use of malloc() for the EFI pool
>   efi: Show the location of the bounce buffer
> 
>  common/dlmalloc.c            |   7 +++
>  include/efi_loader.h         |  29 ++++++++-
>  include/malloc.h             |   7 +++
>  lib/efi_loader/efi_bootbin.c |  11 ++++
>  lib/efi_loader/efi_memory.c  | 119 ++++++++++++++++++++++++-----------
>  5 files changed, 136 insertions(+), 37 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH v3 0/3] efi: Start tidying up memory management

[Simon Glass]

Hi Vincent,

On Thu, 5 Sept 2024 at 01:47, Vincent Stehlé <vincent.stehle@arm.com> wrote:
>
> On Sun, Sep 01, 2024 at 04:22:56PM -0600, Simon Glass wrote:
> > We have been discussing the state of EFI memory management for some
> > years so I thought it might be best to send a short series showing some
> > of the issues we have talked about.
> >
> > This one just deals with memory allocation. It provides a way to detect
> > EFI 'page allocations' when U-Boot' malloc() should be used. It should
> > help us to clean up this area of U-Boot.
> >
> > It also updates EFI to use U-Boot memory allocation for the pool. Most
> > functions use that anyway and it is much more efficient. It also avoids
> > allocating things 'in space' in conflict with the loaded images.
>
> Hi Simon,
>
> Thank you for this series.
>
> I did test it on top of v2024.10-rc4 with AArch64 simulators (FVP & Qemu), with
> OS boots and ACS-IR, and I see no regression. Same with sandbox on x86 and the
> python tests.
>

Thank you for testing.

Ilias, Heinrich, can this series (and the EFI-test one) be applied?
Sughosh has already sent a follow up where we start to diverge on how
to fix the memory-allocation problems, so I would like to make sure
these are in there first.

Regards,
Simon


> Best regards,
> Vincent.
>
> >
> > This series also includes a little patch to show more information in
> > the index for the EFI pages.
> >
> > Note that this series is independent from Sugosh's lmb series[1],
> > although I believe it will point the way to simplifying some of the
> > later patches in that series.
> >
> > Overall, some issues which should be resolved are:
> > - allocation inconsistency, e.g. efi_add_protocol() uses malloc() but
> >   efi_dp_dup() does not (this series makes a start)
> > - change efi_bl_init() to register events later, when the devices are
> >   actually used
> > - rather than efi_set_bootdev(), provide a bootstd way to take note of
> >   the device images came from and allow EFI to query that when needed
> > - EFI_LOADER_BOUNCE_BUFFER - can use U-Boot bounce buffers?
> >
> > Minor questions on my mind:
> > - is unaligned access useful? Is there a performance penalty?
> > - API confusion about whether something is an address or a pointer
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=416441
> >
> > Changes in v3:
> > - Add new patch to drop the memset() from efi_alloc()
> > - Drop patch to convert device_path allocation to use malloc()
> >
> > Changes in v2:
> > - Drop patch 'Show more information in efi index'
> > - Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
> > - Add the word 'warning', use log_warning() and show the end address
> >
> > Simon Glass (3):
> >   efi: Drop the memset() from efi_alloc()
> >   efi: Allow use of malloc() for the EFI pool
> >   efi: Show the location of the bounce buffer
> >
> >  common/dlmalloc.c            |   7 +++
> >  include/efi_loader.h         |  29 ++++++++-
> >  include/malloc.h             |   7 +++
> >  lib/efi_loader/efi_bootbin.c |  11 ++++
> >  lib/efi_loader/efi_memory.c  | 119 ++++++++++++++++++++++++-----------
> >  5 files changed, 136 insertions(+), 37 deletions(-)
> >
> > --
> > 2.34.1
> >