Hi Heinrich,

On Fri, 9 Aug 2024 at 10:38, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 01.08.24 19:36, Simon Glass 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 when running some of the 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@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 62e8557daa7..0bc77395035 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 f84852e384f..31899bef99e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -796,6 +796,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);
  

• /* Initialize EFI drivers */
  ret = efi_init_obj_list();
  if (ret != EFI_SUCCESS) {
  

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6f1dd09456..3c3d05eef94 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -24,6 +24,14 @@ DECLARE_GLOBAL_DATA_PTR; /* Magic number identifying memory allocated from pool */ #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2

+/* Flags controlling EFI memory-allocation - see enum efi_alloc_flags */ +static int alloc_flags;

+void efi_set_alloc(int flags) +{

• alloc_flags = flags;
  

+}

• efi_uintn_t efi_memory_map_key;

  struct efi_mem_list {

@@ -57,8 +65,12 @@ void *efi_bounce_buffer;

• The checksum calculated in function checksum() is used in FreePool() to avoid
• freeing memory not allocated by AllocatePool() and duplicate freeing.

• • EFI requires 8 byte alignment for pool allocations, so we can
• • prepend each allocation with these header fields.

• • EFI requires 8-byte alignment for pool allocations, so we can prepend each
• • allocation with these header fields.
• • Note that before the EFI app is booted, EFI_BOOT_SERVICES_DATA allocations
• • are served using malloc(), bypassing this struct. This helps to avoid memory
• • fragmentation, since efi_allocate_pages() uses any pages it likes.
  */ struct efi_pool_allocation { u64 num_pages;

@@ -631,18 +643,19 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) /**

• efi_allocate_pool - allocate memory from pool

• • This uses malloc() for EFI_BOOT_SERVICES_DATA allocations if EFIAF_USE_MALLOC
• • is enabled
• • @pool_type: type of the pool from which memory is to be allocated
  • @size: number of bytes to be allocated
  • @buffer: allocated memory
  • Return: status code
  */

-efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,

•                        void **buffer)
  

  { efi_status_t r; u64 addr;

• struct efi_pool_allocation *alloc;
  

• u64 num_pages = efi_size_in_pages(size +
  

•                                   sizeof(struct efi_pool_allocation));
  
  if (!buffer)
          return EFI_INVALID_PARAMETER;
  

@@ -652,13 +665,43 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, return EFI_SUCCESS; }

• r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
  

•                        &addr);
  

• if (r == EFI_SUCCESS) {
  

•         alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
  

•         alloc->num_pages = num_pages;
  

•         alloc->checksum = checksum(alloc);
  

•         *buffer = alloc->data;
  

• if ((alloc_flags & EFIAF_USE_MALLOC) &&
  

•     pool_type == EFI_BOOT_SERVICES_DATA) {
  

•         void *ptr;
  

•         /*
  

•          * Some tests crash on qemu_arm etc. if the correct size is
  

•          * allocated.
  

•          * Adding 0x10 seems to fix test_efi_selftest_device_tree
  

•          * Increasing it to 0x20 seems to fix test_efi_selftest_base
  

•          * except * for riscv64 (in CI only). But 0x100 fixes CI too.
  

•          *
  

•          * This workaround can be dropped once these problems are
  

•          * resolved
  

•          */
  

•         ptr = memalign(8, size + 0x100);
  

We don't want voodoo code. Please, analyze what is going on.

The allocations were cache line size aligned (0x40) because our file-system drivers rely on it.

Where is that documented and which allocation call is doing this?

...

•         if (!ptr)
  

•                 return EFI_OUT_OF_RESOURCES;
  

This will not work for allocating more memory than is in the malloc pool as pointed out in a previous review comment. The malloc pool is 2 MiB or less.

Before the app starts, only a very small amount of memory is allocated. Please read the patch thoroughly.

An EFI program can reasonable expect that the largest chunk of unallocated conventional memory reported in the memory map can be allocated with AllocatePool().

How will this code allocate 6 GiB on a 16 GiB board?

Again, this is supported by the patch. I can add a test to the testapp if you like.

We should not have different code paths depending on the EFI memory type.

Why is that?

A reasonable solution would be a single memory allocator replacing malloc(), LMB(), and the EFI memory management which is aware of EFI memory types.

Perhaps, although I doubt it very much. We never actually resolved that discussion though. So for now I am doing this patch-up series.

Anyway that point has almost nothing to do with this patch, which is aimed at avoiding strange allocations 'in space' which no one expects.

Regards, Simon

Best regards

Heinrich

...

•         *buffer = ptr;
  

•         r = EFI_SUCCESS;
  

•         log_debug("EFI pool: malloc(%zx) = %p\n", size, ptr);
  

• } else {
  

•         u64 num_pages = efi_size_in_pages(size +
  

•                                 sizeof(struct efi_pool_allocation));
  

•         r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type,
  

•                                num_pages, &addr);
  

•         if (r == EFI_SUCCESS) {
  

•                 struct efi_pool_allocation *alloc;
  

•                 alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
  

•                 alloc->num_pages = num_pages;
  

•                 alloc->checksum = checksum(alloc);
  

•                 *buffer = alloc->data;
  

•                 log_debug("EFI pool: pages alloc(%zx) type %d = %p\n",
  

•                           size, pool_type, *buffer);
  

•         }
  }
  
  return r;
  

@@ -695,27 +738,37 @@ void *efi_alloc(size_t size) efi_status_t efi_free_pool(void *buffer) { efi_status_t ret;

• struct efi_pool_allocation *alloc;
  
  if (!buffer)
          return EFI_INVALID_PARAMETER;
  
  

• ret = efi_check_allocated((uintptr_t)buffer, true);
  

• if (ret != EFI_SUCCESS)
  

•         return ret;
  

• if (malloc_check_in_range(buffer)) {
  

•         log_debug("EFI pool: free(%p)\n", buffer);
  

•         free(buffer);
  

•         ret = EFI_SUCCESS;
  

• } else {
  

•         struct efi_pool_allocation *alloc;
  
  

• alloc = container_of(buffer, struct efi_pool_allocation, data);
  

•         ret = efi_check_allocated((uintptr_t)buffer, true);
  

•         if (ret != EFI_SUCCESS)
  

•                 return ret;
  
  

• /* Check that this memory was allocated by efi_allocate_pool() */
  

• if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
  

•     alloc->checksum != checksum(alloc)) {
  

•         printf("%s: illegal free 0x%p\n", __func__, buffer);
  

•         return EFI_INVALID_PARAMETER;
  

• }
  

• /* Avoid double free */
  

• alloc->checksum = 0;
  

•         alloc = container_of(buffer, struct efi_pool_allocation, data);
  
  

• ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
  

•         /*
  

•          * Check that this memory was allocated by efi_allocate_pool()
  

•          */
  

•         if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
  

•             alloc->checksum != checksum(alloc)) {
  

•                 printf("%s: illegal free 0x%p\n", __func__, buffer);
  

•                 return EFI_INVALID_PARAMETER;
  

•         }
  

•         /* Avoid double free */
  

•         alloc->checksum = 0;
  

•         ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
  

•         log_debug("EFI pool: pages free(%p)\n", buffer);
  

• }
  
  return ret;
  

  }

@@ -935,6 +988,9 @@ static void add_u_boot_and_runtime(void)

int efi_memory_init(void) {

• /* use malloc() pool where possible */
  

• efi_set_alloc(EFIAF_USE_MALLOC);
  

• efi_add_known_memory();
  
  add_u_boot_and_runtime();
  

Hi Simon,

On Thu, 1 Aug 2024 at 20:36, Simon Glass sjg@chromium.org wrote:

Currently this calls efi_alloc() which reserves a page for each allocation and this can overwrite memory that will be used for reading images.

Switch the code to use malloc(), as with other parts of EFI, such as efi_add_protocol().

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v1)

What about https://lore.kernel.org/u-boot/CAFLszTjLAKaYK_jLXJ7Z571L-QMtoiqE-oxHCRs186dZ... ?

Thanks /Ilias

lib/efi_loader/efi_device_path.c | 43 ++++++++++++------------ lib/efi_loader/efi_device_path_to_text.c | 2 +- 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 0f684590f22..47038e8e585 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -262,7 +262,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) if (!dp) return NULL;

•   ndp = efi_alloc(sz);
  

•   ndp = malloc(sz);
    if (!ndp)
            return NULL;
    memcpy(ndp, dp, sz);
  

@@ -315,7 +315,7 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, end_size = 2 * sizeof(END); else end_size = sizeof(END);

•           p = efi_alloc(sz1 + sz2 + end_size);
  

•           p = malloc(sz1 + sz2 + end_size);
            if (!p)
                    return NULL;
            ret = p;
  

@@ -347,7 +347,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp, ret = efi_dp_dup(dp); } else if (!dp) { size_t sz = node->length;

•           void *p = efi_alloc(sz + sizeof(END));
  

•           void *p = malloc(sz + sizeof(END));
            if (!p)
                    return NULL;
            memcpy(p, node, sz);
  

@@ -356,7 +356,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp, } else { /* both dp and node are non-null */ size_t sz = efi_dp_size(dp);

•           void *p = efi_alloc(sz + node->length + sizeof(END));
  

•           void *p = malloc(sz + node->length + sizeof(END));
            if (!p)
                    return NULL;
            memcpy(p, dp, sz);
  

@@ -377,7 +377,7 @@ struct efi_device_path *efi_dp_create_device_node(const u8 type, if (length < sizeof(struct efi_device_path)) return NULL;

•   ret = efi_alloc(length);
  

•   ret = malloc(length);
    if (!ret)
            return ret;
    ret->type = type;
  

@@ -399,7 +399,7 @@ struct efi_device_path *efi_dp_append_instance( return efi_dp_dup(dpi); sz = efi_dp_size(dp); szi = efi_dp_instance_size(dpi);

•   p = efi_alloc(sz + szi + 2 * sizeof(END));
  

•   p = malloc(sz + szi + 2 * sizeof(END));
    if (!p)
            return NULL;
    ret = p;
  

@@ -424,7 +424,7 @@ struct efi_device_path *efi_dp_get_next_instance(struct efi_device_path **dp, if (!dp || !*dp) return NULL; sz = efi_dp_instance_size(*dp);

•   p = efi_alloc(sz + sizeof(END));
  

•   p = malloc(sz + sizeof(END));
    if (!p)
            return NULL;
    memcpy(p, *dp, sz + sizeof(END));
  

@@ -825,11 +825,11 @@ struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part) { void *buf, *start;

•   start = buf = efi_alloc(dp_part_size(desc, part) + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(dp_part_size(desc, part) + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   buf = dp_part_fill(buf, desc, part);
  

•   buf = dp_part_fill(start, desc, part);
  
    *((struct efi_device_path *)buf) = END;
  
  

@@ -852,7 +852,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part) dpsize = sizeof(struct efi_device_path_cdrom_path); else dpsize = sizeof(struct efi_device_path_hard_drive_path);

•   buf = efi_alloc(dpsize);
  

•   buf = malloc(dpsize);
  
    if (buf)
            dp_part_node(buf, desc, part);
  

@@ -912,7 +912,7 @@ struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp, if (fpsize > U16_MAX) return NULL;

•   buf = efi_alloc(dpsize + fpsize + sizeof(END));
  

•   buf = malloc(dpsize + fpsize + sizeof(END));
    if (!buf)
            return NULL;
  
  

@@ -940,7 +940,7 @@ struct efi_device_path *efi_dp_from_uart(void) struct efi_device_path_uart *uart; size_t dpsize = dp_size(dm_root()) + sizeof(*uart) + sizeof(END);

•   buf = efi_alloc(dpsize);
  

•   buf = malloc(dpsize);
    if (!buf)
            return NULL;
    pos = dp_fill(buf, dm_root());
  

@@ -963,11 +963,11 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)

    dpsize += dp_size(eth_get_dev());

•   start = buf = efi_alloc(dpsize + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(dpsize + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   buf = dp_fill(buf, eth_get_dev());
  

•   buf = dp_fill(start, eth_get_dev());
  
    *((struct efi_device_path *)buf) = END;
  
  

@@ -980,22 +980,21 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, uint64_t end_address) { struct efi_device_path_memory *mdp;

•   void *buf, *start;
  

•   void *start;
  
  

•   start = buf = efi_alloc(sizeof(*mdp) + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(sizeof(*mdp) + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   mdp = buf;
  

•   mdp = start;
    mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
    mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
    mdp->dp.length = sizeof(*mdp);
    mdp->memory_type = memory_type;
    mdp->start_address = start_address;
    mdp->end_address = end_address;
  

•   buf = &mdp[1];
  
  

•   *((struct efi_device_path *)buf) = END;
  

•   *((struct efi_device_path *)&mdp[1]) = END;
  
    return start;
  

} diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c index 0c7b30a26e7..4192581ac45 100644 --- a/lib/efi_loader/efi_device_path_to_text.c +++ b/lib/efi_loader/efi_device_path_to_text.c @@ -33,7 +33,7 @@ static u16 *efi_str_to_u16(char *str) u16 *out, *dst;

    len = sizeof(u16) * (utf8_utf16_strlen(str) + 1);

•   out = efi_alloc(len);
  

•   out = malloc(len);
    if (!out)
            return NULL;
    dst = out;
  

-- 2.34.1

Hi Simon

On Wed, 14 Aug 2024 at 15:40, Simon Glass sjg@chromium.org wrote:

Hi Ilias,

On Wed, 14 Aug 2024 at 05:03, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Simon,

On Thu, 1 Aug 2024 at 20:36, Simon Glass sjg@chromium.org wrote:

...

Currently this calls efi_alloc() which reserves a page for each allocation and this can overwrite memory that will be used for reading images.

Switch the code to use malloc(), as with other parts of EFI, such as efi_add_protocol().

Signed-off-by: Simon Glass sjg@chromium.org

(no changes since v1)

What about https://lore.kernel.org/u-boot/CAFLszTjLAKaYK_jLXJ7Z571L-QMtoiqE-oxHCRs186dZ... ?

Yes, I reordered the patches in this series.

You don't need to reorder them. As Heinrich already pointed out some of these functions are used in EFI protocols. e.g duplicate_device_path() requires the memory to be allocated by EFI memory services, you can't just replace it with malloc.

Thanks /Ilias

Regards, Simon

...

Thanks /Ilias

...

lib/efi_loader/efi_device_path.c | 43 ++++++++++++------------ lib/efi_loader/efi_device_path_to_text.c | 2 +- 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 0f684590f22..47038e8e585 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -262,7 +262,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) if (!dp) return NULL;

•   ndp = efi_alloc(sz);
  

•   ndp = malloc(sz);
    if (!ndp)
            return NULL;
    memcpy(ndp, dp, sz);
  

@@ -315,7 +315,7 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, end_size = 2 * sizeof(END); else end_size = sizeof(END);

•           p = efi_alloc(sz1 + sz2 + end_size);
  

•           p = malloc(sz1 + sz2 + end_size);
            if (!p)
                    return NULL;
            ret = p;
  

@@ -347,7 +347,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp, ret = efi_dp_dup(dp); } else if (!dp) { size_t sz = node->length;

•           void *p = efi_alloc(sz + sizeof(END));
  

•           void *p = malloc(sz + sizeof(END));
            if (!p)
                    return NULL;
            memcpy(p, node, sz);
  

@@ -356,7 +356,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp, } else { /* both dp and node are non-null */ size_t sz = efi_dp_size(dp);

•           void *p = efi_alloc(sz + node->length + sizeof(END));
  

•           void *p = malloc(sz + node->length + sizeof(END));
            if (!p)
                    return NULL;
            memcpy(p, dp, sz);
  

@@ -377,7 +377,7 @@ struct efi_device_path *efi_dp_create_device_node(const u8 type, if (length < sizeof(struct efi_device_path)) return NULL;

•   ret = efi_alloc(length);
  

•   ret = malloc(length);
    if (!ret)
            return ret;
    ret->type = type;
  

@@ -399,7 +399,7 @@ struct efi_device_path *efi_dp_append_instance( return efi_dp_dup(dpi); sz = efi_dp_size(dp); szi = efi_dp_instance_size(dpi);

•   p = efi_alloc(sz + szi + 2 * sizeof(END));
  

•   p = malloc(sz + szi + 2 * sizeof(END));
    if (!p)
            return NULL;
    ret = p;
  

@@ -424,7 +424,7 @@ struct efi_device_path *efi_dp_get_next_instance(struct efi_device_path **dp, if (!dp || !*dp) return NULL; sz = efi_dp_instance_size(*dp);

•   p = efi_alloc(sz + sizeof(END));
  

•   p = malloc(sz + sizeof(END));
    if (!p)
            return NULL;
    memcpy(p, *dp, sz + sizeof(END));
  

@@ -825,11 +825,11 @@ struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part) { void *buf, *start;

•   start = buf = efi_alloc(dp_part_size(desc, part) + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(dp_part_size(desc, part) + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   buf = dp_part_fill(buf, desc, part);
  

•   buf = dp_part_fill(start, desc, part);
  
    *((struct efi_device_path *)buf) = END;
  
  

@@ -852,7 +852,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part) dpsize = sizeof(struct efi_device_path_cdrom_path); else dpsize = sizeof(struct efi_device_path_hard_drive_path);

•   buf = efi_alloc(dpsize);
  

•   buf = malloc(dpsize);
  
    if (buf)
            dp_part_node(buf, desc, part);
  

@@ -912,7 +912,7 @@ struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp, if (fpsize > U16_MAX) return NULL;

•   buf = efi_alloc(dpsize + fpsize + sizeof(END));
  

•   buf = malloc(dpsize + fpsize + sizeof(END));
    if (!buf)
            return NULL;
  
  

@@ -940,7 +940,7 @@ struct efi_device_path *efi_dp_from_uart(void) struct efi_device_path_uart *uart; size_t dpsize = dp_size(dm_root()) + sizeof(*uart) + sizeof(END);

•   buf = efi_alloc(dpsize);
  

•   buf = malloc(dpsize);
    if (!buf)
            return NULL;
    pos = dp_fill(buf, dm_root());
  

@@ -963,11 +963,11 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)

    dpsize += dp_size(eth_get_dev());

•   start = buf = efi_alloc(dpsize + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(dpsize + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   buf = dp_fill(buf, eth_get_dev());
  

•   buf = dp_fill(start, eth_get_dev());
  
    *((struct efi_device_path *)buf) = END;
  
  

@@ -980,22 +980,21 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, uint64_t end_address) { struct efi_device_path_memory *mdp;

•   void *buf, *start;
  

•   void *start;
  
  

•   start = buf = efi_alloc(sizeof(*mdp) + sizeof(END));
  

•   if (!buf)
  

•   start = malloc(sizeof(*mdp) + sizeof(END));
  

•   if (!start)
            return NULL;
  
  

•   mdp = buf;
  

•   mdp = start;
    mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
    mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
    mdp->dp.length = sizeof(*mdp);
    mdp->memory_type = memory_type;
    mdp->start_address = start_address;
    mdp->end_address = end_address;
  

•   buf = &mdp[1];
  
  

•   *((struct efi_device_path *)buf) = END;
  

•   *((struct efi_device_path *)&mdp[1]) = END;
  
    return start;
  

} diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c index 0c7b30a26e7..4192581ac45 100644 --- a/lib/efi_loader/efi_device_path_to_text.c +++ b/lib/efi_loader/efi_device_path_to_text.c @@ -33,7 +33,7 @@ static u16 *efi_str_to_u16(char *str) u16 *out, *dst;

    len = sizeof(u16) * (utf8_utf16_strlen(str) + 1);

•   out = efi_alloc(len);
  

•   out = malloc(len);
    if (!out)
            return NULL;
    dst = out;
  

-- 2.34.1

Hi Heinrich,

On Fri, 9 Aug 2024 at 11:32, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 01.08.24 19:36, Simon Glass wrote:

...

The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is not clear whether it is still needed, but 24 boards (lx2160ardb_tfa_stmm, lx2162aqds_tfa_SECURE_BOOT and the like) use it.

This feature uses EFI page allocation to create a 64MB buffer 'in space' without any knowledge of where boards intend to load their images. This may result in image corruption or other problems.

For example, if the feature is enabled on qemu_arm64 it puts the EFI bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at 1088MB. The kernel is probably smaller than 27MB but the buffer does overlap the ramdisk.

The solution is probably to use BOUNCE_BUFFER instead, with the EFI version being dropped. For now, show the address of the EFI bounce buffer so people have a better chance to detect the problem.

Note: I avoided converting this #ifdef to use IS_ENABLED() since I hope that the feature may be removed.

EFI_BLOCK_IO_PROTOCOL.ReadBlocks() and EFI_BLOCK_IO_PROTOCOL.WriteBlocks() are expected to block until all bytes are transferred.

The complete file transfer is chunked according to the size of the EFI bounce buffer.

Taking care of alignment issues would best handled by the block uclass.

When CONFIG_BOUNCE_BUFFER=y, bounce_buffer_start_extalign() uses memalign() which limits the bounce buffer to available malloc() memory (typically < 2 MiB).

I cannot see how blk_read() with CONFIG_BOUNCE_BUFFER=y will work if blkcnt * desc->blksz is greater than the available malloc memory.

Yes, it uses malloc(). The maximum read length on MMC, for example, seems to default to 32MB, although it is only 128KB on a few platforms.

I am thinking we should be allocating space in the bloblist to hold all these sorts of things...it can be as large as needed and we can allocate space for it during relocation.

Shouldn't the block uclass support chunking?

It only supports readling whole blocks, if that is what you mean. Are you suggesting changing the blk API, or something else?

Regards, Simon

Best regards

Heinrich

...

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Drop patch 'Show more information in efi index'

• Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'

• Add the word 'warning', use log_warning() and show the end address

• Reorder patches a little

  lib/efi_loader/efi_bootbin.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 5bb0fdcf75d..9779dc09b5e 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -211,6 +211,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) return -1; }

+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER

• /*
  

•  * Add a warning about this buffer, since it may conflict with other
  

•  * things
  

•  */
  

• log_warning("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,
  

•             efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);
  

+#endif

• ret = efi_install_fdt(fdt);
  if (ret != EFI_SUCCESS)
          return ret;