Re: [PATCH 1/4] lmb: check if a region can be reserved by lmb_reserve()

[Heinrich Schuchardt]

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.

+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?

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);