| Message ID | 20250213131104.186663-2-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lmb: miscellaneous fixes and improvements | expand |
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.ganu@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); >
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) } } +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; +} + 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);
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.ganu@linaro.org> --- lib/lmb.c | 23 ++++++++++++ test/lib/lmb.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 122 insertions(+), 1 deletion(-)