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