diff mbox series

[v2,1/6] lmb: check if a region can be reserved by lmb_reserve()

Message ID 20250220095654.121634-2-sughosh.ganu@linaro.org
State New
Headers show
Series lmb: miscellaneous fixes and improvements | expand

Commit Message

Sughosh Ganu Feb. 20, 2025, 9:56 a.m. UTC
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.

Add corresponding test cases which check for overlapping reservation
requests made through lmb_reserve() and lmb_alloc_addr(). And while
here, fix some of the comments in the test function being touched.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V1:
* Add documentation for the lmb_can_reserve_region() function.
* Add an additional test case where a new region is being added, and
  the new region overlaps two different existing regions, with
  mismatching flags.
* Some cleanup of comments in the lib_test_lmb_overlapping_reserve()
  function.

 lib/lmb.c      |  36 ++++++++++++++++
 test/lib/lmb.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 146 insertions(+), 4 deletions(-)

Comments

Heinrich Schuchardt Feb. 20, 2025, 10:28 a.m. UTC | #1
On 20.02.25 10:56, 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.
> 
> Add corresponding test cases which check for overlapping reservation
> requests made through lmb_reserve() and lmb_alloc_addr(). And while
> here, fix some of the comments in the test function being touched.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Looks good to me.

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> ---
> Changes since V1:
> * Add documentation for the lmb_can_reserve_region() function.
> * Add an additional test case where a new region is being added, and
>    the new region overlaps two different existing regions, with
>    mismatching flags.
> * Some cleanup of comments in the lib_test_lmb_overlapping_reserve()
>    function.
> 
>   lib/lmb.c      |  36 ++++++++++++++++
>   test/lib/lmb.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 146 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7ca44591e1d..d7d2c8c6dfd 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -595,6 +595,39 @@ static __maybe_unused void lmb_reserve_common_spl(void)
>   	}
>   }
>   
> +/**
> + * lmb_can_reserve_region() - check if the region can be reserved
> + * @base: base address of region to be reserved
> + * @size: size of region to be reserved
> + * @flags: flag of the region to be reserved
> + *
> + * Go through all the reserved regions and ensure that the requested
> + * region does not overlap with any existing regions. An overlap is
> + * allowed only when the flag of the request region and the existing
> + * region is LMB_NONE.
> + *
> + * Return: true if region can be reserved, false otherwise
> + */
> +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 +700,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..24416e83491 100644
> --- a/test/lib/lmb.c
> +++ b/test/lib/lmb.c
> @@ -471,17 +471,17 @@ static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)
>   	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
>   		   0, 0, 0, 0);
>   
> -	/* allocate overlapping region should return the coalesced count */
> +	/* allocate overlapping region */
>   	ret = lmb_reserve(0x40011000, 0x10000, LMB_NONE);
>   	ut_asserteq(ret, 0);
>   	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x11000,
>   		   0, 0, 0, 0);
> -	/* allocate 3nd region */
> +	/* allocate 2nd region */
>   	ret = lmb_reserve(0x40030000, 0x10000, LMB_NONE);
>   	ut_asserteq(ret, 0);
>   	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40010000, 0x11000,
>   		   0x40030000, 0x10000, 0, 0);
> -	/* allocate 2nd region , This should coalesced all region into one */
> +	/* allocate 3rd region , This should coalesce all regions into one */
>   	ret = lmb_reserve(0x40020000, 0x10000, LMB_NONE);
>   	ut_assert(ret >= 0);
>   	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x30000,
> @@ -499,6 +499,41 @@ 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 reserve 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 reserve 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);
> +
> +	/*
> +	 * try to reserve a region which overlaps with both the regions,
> +	 * should fail as the flags do not match
> +	 */
> +	ret = lmb_reserve(0x40020000, 0x80000, LMB_NONE);
> +	ut_asserteq(ret, -EEXIST);
> +	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x50000,
> +		   0x40050000, 0x10000, 0, 0);
> +
>   	lmb_pop(&store);
>   
>   	return 0;
> @@ -549,6 +584,77 @@ 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 +866,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 mbox series

Patch

diff --git a/lib/lmb.c b/lib/lmb.c
index 7ca44591e1d..d7d2c8c6dfd 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -595,6 +595,39 @@  static __maybe_unused void lmb_reserve_common_spl(void)
 	}
 }
 
+/**
+ * lmb_can_reserve_region() - check if the region can be reserved
+ * @base: base address of region to be reserved
+ * @size: size of region to be reserved
+ * @flags: flag of the region to be reserved
+ *
+ * Go through all the reserved regions and ensure that the requested
+ * region does not overlap with any existing regions. An overlap is
+ * allowed only when the flag of the request region and the existing
+ * region is LMB_NONE.
+ *
+ * Return: true if region can be reserved, false otherwise
+ */
+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 +700,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..24416e83491 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -471,17 +471,17 @@  static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts)
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000,
 		   0, 0, 0, 0);
 
-	/* allocate overlapping region should return the coalesced count */
+	/* allocate overlapping region */
 	ret = lmb_reserve(0x40011000, 0x10000, LMB_NONE);
 	ut_asserteq(ret, 0);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x11000,
 		   0, 0, 0, 0);
-	/* allocate 3nd region */
+	/* allocate 2nd region */
 	ret = lmb_reserve(0x40030000, 0x10000, LMB_NONE);
 	ut_asserteq(ret, 0);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40010000, 0x11000,
 		   0x40030000, 0x10000, 0, 0);
-	/* allocate 2nd region , This should coalesced all region into one */
+	/* allocate 3rd region , This should coalesce all regions into one */
 	ret = lmb_reserve(0x40020000, 0x10000, LMB_NONE);
 	ut_assert(ret >= 0);
 	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x30000,
@@ -499,6 +499,41 @@  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 reserve 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 reserve 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);
+
+	/*
+	 * try to reserve a region which overlaps with both the regions,
+	 * should fail as the flags do not match
+	 */
+	ret = lmb_reserve(0x40020000, 0x80000, LMB_NONE);
+	ut_asserteq(ret, -EEXIST);
+	ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x50000,
+		   0x40050000, 0x10000, 0, 0);
+
 	lmb_pop(&store);
 
 	return 0;
@@ -549,6 +584,77 @@  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 +866,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);