| Message ID | 20241011212126.747741-2-sjg@chromium.org |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Adjust initial EFI memory-allocation to be in the U-Boot region | expand |
On Fri, Oct 11, 2024 at 03:21:22PM -0600, 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. > > 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. > > Comments were made in v3 that another project uses memset() when > allocating memory, but that is not required by the spec. In any case, as > above, from inspection, none of the users need the memory to be zeroed, > as they fill the entire region with their own data. The other comments made in v3 about another project (EDKII) is that we want to remain implementation compatible. This is a case where (a) if needed, we can reach out and get the spec clarified to say zeroed memory and (b) even if it's not, this is a case where real world interoperability is more important than strict spec compliance. If there's an app out there relying on the zeroed behavior and it's fine on EDKII and not fine on us, it'll be a problem for us because we stopped matching behavior. So unless in the end Heinrich and/or Ilias drop their previous objection, I don't think this is a good idea.
Hi Tom, On Fri, 11 Oct 2024 at 19:20, Tom Rini <trini@konsulko.com> wrote: > > On Fri, Oct 11, 2024 at 03:21:22PM -0600, 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. > > > > 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. > > > > Comments were made in v3 that another project uses memset() when > > allocating memory, but that is not required by the spec. In any case, as > > above, from inspection, none of the users need the memory to be zeroed, > > as they fill the entire region with their own data. > > The other comments made in v3 about another project (EDKII) is that we > want to remain implementation compatible. This is a case where (a) if > needed, we can reach out and get the spec clarified to say zeroed memory > and (b) even if it's not, this is a case where real world > interoperability is more important than strict spec compliance. If > there's an app out there relying on the zeroed behavior and it's fine on > EDKII and not fine on us, it'll be a problem for us because we > stopped matching behavior. So unless in the end Heinrich and/or Ilias > drop their previous objection, I don't think this is a good idea. This is a minor point, so I don't mind, so long as we get the memory-reservation tidied up. But I'll note that the effect of this patch is zero, since every allocated byte is written anyway, by current users. I certainly don't want to be implementation-compatible with EDKII. Where did that come from? U-Boot is an open-source bootloader with lots of boards supported in mainline. U-Boot has very different goals and users. Regards, Simon
On Mon, Oct 14, 2024 at 02:20:42PM -0600, Simon Glass wrote: [snip] > I certainly don't want to be implementation-compatible with EDKII. > Where did that come from? It came from Ilias pointing out that EDKII does this too, and so we shouldn't deviate from that, and then you re-posted the patch with a reworded commit message, rather than dropping it. So I'm just emphasizing goals for the public record.
diff --git a/include/efi_loader.h b/include/efi_loader.h index 511281e150e..08e27e61b06 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. Comments were made in v3 that another project uses memset() when allocating memory, but that is not required by the spec. In any case, as above, from inspection, none of the users need the memory to be zeroed, as they fill the entire region with their own data. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v4: - Expand the commit message 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(-)