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

Mon Sep 23 14:30:40 CEST 2024

On 12.09.24 02:59, Simon Glass wrote:
> Hi Sughosh,
>
> On Wed, 11 Sept 2024 at 00:50, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>>
>> On Wed, 11 Sept 2024 at 00:14, Simon Glass <sjg at chromium.org> wrote:
>>>
>>> Hi Sughosh,
>>>
>>> On Mon, 9 Sept 2024 at 01:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>>>>
>>>> On Fri, 6 Sept 2024 at 18:31, Simon Glass <sjg at chromium.org> wrote:
>>>>>
>>>>> Hi Sughosh,
>>>>>
>>>>> On Fri, 6 Sept 2024 at 00:23, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>>>>>>
>>>>>> On Mon, 2 Sept 2024 at 03:53, Simon Glass <sjg at 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 at 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