| Message ID | 20240901222259.456932-2-sjg@chromium.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: Start tidying up memory management | expand |
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; > }
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
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
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 >
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; }
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(-)