[PATCH] lmb: Fix the allocation of overlapping memory areas with !LMB_NONE

[Ilias Apalodimas]

On Mon, 2 Dec 2024 at 16:11, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Mon, 2 Dec 2024 at 15:37, Heinrich Schuchardt <xypron.glpk at 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 at 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);
> > >
> >