Hi Heinrich,
On Mon, 23 Sept 2024 at 14:15, Heinrich Schuchardt <xypron.g...@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.g...@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 <s...@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
>
