On Thu, 13 Feb 2025 at 19:45, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:
>
> On 13.02.25 14:11, Sughosh Ganu wrote:
> > The logic used in lmb_alloc() takes into consideration the existing
> > reserved regions, and ensures that the allocated region does not
> > overlap with any existing allocated regions. The lmb_reserve()
> > function is not doing any such checks -- the requested region might
> > overlap with an existing region. This also shows up with
> > lmb_alloc_addr() as this function ends up calling lmb_reserve().
> >
> > Add a function which checks if the region requested is overlapping
> > with an existing reserved region, and allow for the reservation to
> > happen only if both the regions have LMB_NONE flag, which allows
> > re-requesting of the region. In any other scenario of an overlap, have
> > lmb_reserve() return -EEXIST, implying that the requested region is
> > already reserved.
> >
> > Also add corresponding test cases which checks for overlapping
> > reservation requests made through lmb_reserve() and lmb_alloc_addr().
> >
> > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org>
> > ---
> > lib/lmb.c | 23 ++++++++++++
> > test/lib/lmb.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 122 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 7ca44591e1d..aeaf120f57d 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -595,6 +595,26 @@ static __maybe_unused void lmb_reserve_common_spl(void)
> > }
> > }
> >
>
> Every new function should get a Sphinx style description.
Okay. Will add.
>
> > +static bool lmb_can_reserve_region(phys_addr_t base, phys_size_t size,
> > + u32 flags)
> > +{
> > + uint i;
> > + struct lmb_region *lmb_reserved = lmb.used_mem.data;
> > +
> > + for (i = 0; i < lmb.used_mem.count; i++) {
> > + u32 rgnflags = lmb_reserved[i].flags;
> > + phys_addr_t rgnbase = lmb_reserved[i].base;
> > + phys_size_t rgnsize = lmb_reserved[i].size;
> > +
> > + if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) {
> > + if (flags != LMB_NONE || flags != rgnflags)
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
>
> This seems to duplicate code in lmb_resize_regions(). Can we reduce the
> code size by using a common function?
Okay. Let me check if the common parts can be refactored into a single
function for reuse. Thanks.
-sughosh
>
> Best regards
>
> Heinrich
>
> > void lmb_add_memory(void)
> > {
> > int i;
> > @@ -667,6 +687,9 @@ long lmb_reserve(phys_addr_t base, phys_size_t size,
> > u32 flags)
> > long ret = 0;
> > struct alist *lmb_rgn_lst = &lmb.used_mem;
> >
> > + if (!lmb_can_reserve_region(base, size, flags))
> > + return -EEXIST;
> > +
> > ret = lmb_add_region_flags(lmb_rgn_lst, base, size, flags);
> > if (ret)
> > return ret;
> > diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> > index fcb5f1af532..6b995c976dd 100644
> > --- a/test/lib/lmb.c
> > +++ b/test/lib/lmb.c
> > @@ -499,6 +499,32 @@ static int lib_test_lmb_overlapping_reserve(struct
> > unit_test_state *uts)
> > ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40000000, 0x40000,
> > 0, 0, 0, 0);
> >
> > + /* try to allocate overlapping region with a different flag, should
> > fail */
> > + ret = lmb_reserve(0x40008000, 0x1000, LMB_NOOVERWRITE);
> > + ut_asserteq(ret, -EEXIST);
> > +
> > + /* allocate another region at 0x40050000 with a different flag */
> > + ret = lmb_reserve(0x40050000, 0x10000, LMB_NOOVERWRITE);
> > + ut_asserteq(ret, 0);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x40000,
> > + 0x40050000, 0x10000, 0, 0);
> > +
> > + /*
> > + * try to allocate a region adjacent to region 1 overlapping the 2nd
> > region,
> > + * should fail
> > + */
> > + ret = lmb_reserve(0x40040000, 0x20000, LMB_NONE);
> > + ut_asserteq(ret, -EEXIST);
> > +
> > + /*
> > + * try to allocate a region between the two regions, but without an
> > overlap,
> > + * should succeed. this added region coalesces with the region 1
> > + */
> > + ret = lmb_reserve(0x40040000, 0x10000, LMB_NONE);
> > + ut_asserteq(ret, 0);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x50000,
> > + 0x40050000, 0x10000, 0, 0);
> > +
> > lmb_pop(&store);
> >
> > return 0;
> > @@ -549,6 +575,78 @@ static int test_alloc_addr(struct unit_test_state
> > *uts, const phys_addr_t ram)
> > ret = lmb_free(alloc_addr_a, 0x1000);
> > ut_asserteq(ret, 0);
> >
> > + /*
> > + * Add two regions with different flags, region1 and region2 with
> > + * a gap between them.
> > + * Try adding another region, adjacent to region 1 and overlapping
> > + * region 2. Should fail.
> > + */
> > + a = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NONE);
> > + ut_asserteq(a, alloc_addr_a);
> > +
> > + b = lmb_alloc_addr(alloc_addr_a + 0x4000, 0x1000, LMB_NOOVERWRITE);
> > + ut_asserteq(b, alloc_addr_a + 0x4000);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, a, 0x1000,
> > + b, 0x1000, 0, 0);
> > +
> > + c = lmb_alloc_addr(alloc_addr_a + 0x1000, 0x5000, LMB_NONE);
> > + ut_asserteq(c, 0);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, a, 0x1000,
> > + b, 0x1000, 0, 0);
> > +
> > + ret = lmb_free(a, 0x1000);
> > + ut_asserteq(ret, 0);
> > + ret = lmb_free(b, 0x1000);
> > + ut_asserteq(ret, 0);
> > +
> > +
> > + /*
> > + * Add two regions with same flags(LMB_NONE), region1 and region2
> > + * with a gap between them.
> > + * Try adding another region, adjacent to region 1 and overlapping
> > + * region 2. Should succeed. All regions should coalesce into a
> > + * single region.
> > + */
> > + a = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NONE);
> > + ut_asserteq(a, alloc_addr_a);
> > +
> > + b = lmb_alloc_addr(alloc_addr_a + 0x4000, 0x1000, LMB_NONE);
> > + ut_asserteq(b, alloc_addr_a + 0x4000);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, a, 0x1000,
> > + b, 0x1000, 0, 0);
> > +
> > + c = lmb_alloc_addr(alloc_addr_a + 0x1000, 0x5000, LMB_NONE);
> > + ut_asserteq(c, alloc_addr_a + 0x1000);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, a, 0x6000,
> > + 0, 0, 0, 0);
> > +
> > + ret = lmb_free(a, 0x6000);
> > + ut_asserteq(ret, 0);
> > +
> > + /*
> > + * Add two regions with same flags(LMB_NOOVERWRITE), region1 and
> > + * region2 with a gap between them.
> > + * Try adding another region, adjacent to region 1 and overlapping
> > + * region 2. Should fail.
> > + */
> > + a = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NOOVERWRITE);
> > + ut_asserteq(a, alloc_addr_a);
> > +
> > + b = lmb_alloc_addr(alloc_addr_a + 0x4000, 0x1000, LMB_NOOVERWRITE);
> > + ut_asserteq(b, alloc_addr_a + 0x4000);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, a, 0x1000,
> > + b, 0x1000, 0, 0);
> > +
> > + c = lmb_alloc_addr(alloc_addr_a + 0x1000, 0x5000, LMB_NOOVERWRITE);
> > + ut_asserteq(c, 0);
> > + ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, a, 0x1000,
> > + b, 0x1000, 0, 0);
> > +
> > + ret = lmb_free(a, 0x1000);
> > + ut_asserteq(ret, 0);
> > + ret = lmb_free(b, 0x1000);
> > + ut_asserteq(ret, 0);
> > +
> > /* reserve 3 blocks */
> > ret = lmb_reserve(alloc_addr_a, 0x10000, LMB_NONE);
> > ut_asserteq(ret, 0);
> > @@ -760,7 +858,7 @@ static int lib_test_lmb_flags(struct unit_test_state
> > *uts)
> >
> > /* reserve again, new flag */
> > ret = lmb_reserve(0x40010000, 0x10000, LMB_NONE);
> > - ut_asserteq(ret, -1);
> > + ut_asserteq(ret, -EEXIST);
> > ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
> > 0, 0, 0, 0);
> >
>
