On Mon, 2 Dec 2024 at 16:11, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Mon, 2 Dec 2024 at 15:37, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 28.11.24 12:49, Ilias Apalodimas wrote:

...

At the moment the LMB allocator will return 'success' immediately on two consecutive allocations if the second one is smaller and the flags match without resizing the reserved area.

This is problematic for two reasons, first of all the new updated allocation won't update the size and we end up holding more memory than needed, but most importantly it breaks the EFI SCT tests since EFI now allocates via LMB.

More specifically when EFI requests a specific address twice with the EFI_ALLOCATE_ADDRESS flag set, the first allocation will succeed and update the EFI memory map. Due to the LMB behavior the second allocation will also succeed but the address ranges are already in the EFI memory map due the first allocation. EFI will then fail to update the memory map, returning EFI_OUT_OF_RESOURCES instead of EFI_NOT_FOUND which break EFI conformance.

So let's remove the fast check with is problematic anyway and leave LMB resize and calculate address properly. LMB will now

• try to resize the reservations for LMB_NONE
• return -1 if the memory is not LMB_NONE and already reserved

The LMB code needs some cleanup in that part, but since we are close to 2025.01 do the easy fix and plan to refactor it later. Also update the dm tests with the new behavior.

Fixes: commit 22f2c9ed9f53 ("efi: memory: use the LMB API's for allocating and freeing memory") Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

lib/lmb.c | 9 --------- test/lib/lmb.c | 22 +++++++++++++++++++++- 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c index 14b9b8466ff2..3a765c11bee6 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -201,15 +201,6 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, phys_addr_t rgnbase = rgn[i].base; phys_size_t rgnsize = rgn[i].size; phys_size_t rgnflags = rgn[i].flags;

•         phys_addr_t end = base + size - 1;
  

•         phys_addr_t rgnend = rgnbase + rgnsize - 1;
  

•         if (rgnbase <= base && end <= rgnend) {
  

•                 if (flags == rgnflags)
  

•                         /* Already have this region, so we're done */
  

•                         return 0;
  

•                 else
  

•                         return -1; /* regions with new flags */
  

•         }
  
          ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
          if (ret > 0) {
  

diff --git a/test/lib/lmb.c b/test/lib/lmb.c index c917115b7b66..bc44dd21192f 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -529,6 +529,26 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ret = lmb_add(ram, ram_size); ut_asserteq(ret, 0);

• /* Try to allocate a page twice */
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE);
  

• ut_asserteq(b, alloc_addr_a);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE);
  

• ut_asserteq(b, 0);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE);
  

• ut_asserteq(b, alloc_addr_a);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x2000, LMB_NONE);
  

• ut_asserteq(b, alloc_addr_a);
  

• ret = lmb_free(alloc_addr_a, 0x2000);
  

• ut_asserteq(ret, 0);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE);
  

• ut_asserteq(b, alloc_addr_a);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE);
  

• ut_asserteq(b, 0);
  

• b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE);
  

• ut_asserteq(b, 0);
  

• ret = lmb_free(alloc_addr_a, 0x1000);
  

• ut_asserteq(ret, 0);
  

• /*  reserve 3 blocks */
  ret = lmb_reserve(alloc_addr_a, 0x10000);
  ut_asserteq(ret, 0);
  

@@ -734,7 +754,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts)

  /* reserve again, same flag */
  ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP);

• ut_asserteq(ret, 0);
  

• ut_asserteq(ret, 0xffffffff);
  

lmb_reserve_flags() should return a long value < 0 on failure. Why do you expect 0xffffffff?

lmb_alloc_addr_flags has a phys_addr_t (unsigned long), I can rewrite this as ut_asserteq(ret, -1); which is probably correct for 32/64bit

Apologies was looking into lmb_reserve_flags(). lmb_reserve_flags() is ineed returning a long, so I'll change that to -1L

Thanks /Ilias

Cheers /Ilias

...

Best regards

Heinrich

...

  ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
             0, 0, 0, 0);