| Message ID | 20240412155307.735631-2-patrice.chotard@foss.st.com |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2,1/2] lmb: Avoid to add identical region in lmb_add_region_flags() | expand |
On 4/12/24 17:53, Patrice Chotard wrote: > In case a new region is adjacent to a previous region with > similar flag, this region is merged with its predecessor, but no > check are done if this new added region is overlapping another region > present in lmb (see reserved[3] which overlaps reserved[4]). > > This occurs when the LMB [0xdaafd000-0xddb18000] is added and overlaps > the LMB [0xdbaf4380-0xddffffff]. > > Call lmb_overlaps_region() before merging the new region with the > adjacent region already present in lmb. > > In case of adjacent region found, code is 90% similar in case > adjacent region is located before/after the new region. > Factorize adjacent region management in lmb_add_region_flags(). > > Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output > shows: > > before this patch: > ... > lmb_dump_all: > memory.cnt = 0x1 / max = 0x2 > memory[0] [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0 > reserved.cnt = 0x5 / max = 0x10 > reserved[0] [0x10000000-0x10045fff], 0x00046000 bytes flags: 4 > reserved[1] [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4 > reserved[2] [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4 > reserved[3] [0xdaae1000-0xdfffffff], 0x0551f000 bytes flags: 0 > reserved[4] [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4 > ... > > after this patch: > > ... > lmb_dump_all: > memory.cnt = 0x1 / max = 0x2 > memory[0] [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0 > reserved.cnt = 0x5 / max = 0x10 > reserved[0] [0x10000000-0x10045fff], 0x00046000 bytes flags: 4 > reserved[1] [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4 > reserved[2] [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4 > reserved[3] [0xdaae1000-0xddffffff], 0x0351f000 bytes flags: 0 > reserved[4] [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4 > ... > > Fixes: 4ed6552f7159 ("[new uImage] Introduce lmb from linux kernel for memory mgmt of boot images") > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > > Changes in v2: > _ Fix lmb_add_region_flags() by updating test which leads to > extend an existing region > > lib/lmb.c | 57 +++++++++++++++++++++++++++++-------------------------- > 1 file changed, 30 insertions(+), 27 deletions(-) > > diff --git a/lib/lmb.c b/lib/lmb.c > index b6afb731440..4ed60f4a843 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -130,6 +130,22 @@ static void lmb_fix_over_lap_regions(struct lmb_region *rgn, unsigned long r1, > lmb_remove_region(rgn, r2); > } > > +static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base, > + phys_size_t size) > +{ > + unsigned long i; > + > + for (i = 0; i < rgn->cnt; i++) { > + phys_addr_t rgnbase = rgn->region[i].base; > + phys_size_t rgnsize = rgn->region[i].size; > + > + if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) > + break; > + } > + > + return (i < rgn->cnt) ? i : -1; > +} > + > void lmb_init(struct lmb *lmb) > { > #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > @@ -257,7 +273,7 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > phys_size_t size, enum lmb_flags flags) > { > unsigned long coalesced = 0; > - long adjacent, i; > + long adjacent, i, overlap; > > if (rgn->cnt == 0) { > rgn->region[0].base = base; > @@ -283,19 +299,21 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > } > > adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > - if (adjacent > 0) { > - if (flags != rgnflags) > - continue; > - rgn->region[i].base -= size; > - rgn->region[i].size += size; > - coalesced++; > - break; > - } else if (adjacent < 0) { > + if (adjacent != 0) { > if (flags != rgnflags) > continue; > - rgn->region[i].size += size; > - coalesced++; > - break; > + overlap = lmb_overlaps_region(rgn, base, size); > + if (overlap < 0 || flags == rgn->region[overlap].flags) { > + /* > + * no overlap detected or overlap with same flags detected, > + * extend region > + */ > + if (adjacent > 0) > + rgn->region[i].base -= size; > + rgn->region[i].size += size; > + coalesced++; > + break; > + } > } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { > /* regions overlap */ > return -1; > @@ -420,21 +438,6 @@ long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size) > return lmb_reserve_flags(lmb, base, size, LMB_NONE); > } > > -static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base, > - phys_size_t size) > -{ > - unsigned long i; > - > - for (i = 0; i < rgn->cnt; i++) { > - phys_addr_t rgnbase = rgn->region[i].base; > - phys_size_t rgnsize = rgn->region[i].size; > - if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) > - break; > - } > - > - return (i < rgn->cnt) ? i : -1; > -} > - > phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align) > { > return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE); I think this series (v2) is not correct even if now the CI tests are OK. After re-reading carefully the lib_test_lmb_overlapping_reserve() test it appears to me there is a contradiction. It's indicating that "check that calling lmb_reserve with overlapping regions fails" but the very last test of lib_test_lmb_overlapping_reserve() has this comment : /* allocate 3rd region, coalesce with first and overlap with second */ and this test allows this overlap case. It's not clear if LMB region can overlap each other or not ? Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range") is authorizing LMB overlapping right ? Patrice
Hello Patrice, On 4/13/2024 1:54 PM, Patrice CHOTARD wrote: > > On 4/12/24 17:53, Patrice Chotard wrote: >> In case a new region is adjacent to a previous region with >> similar flag, this region is merged with its predecessor, but no >> check are done if this new added region is overlapping another region >> present in lmb (see reserved[3] which overlaps reserved[4]). >> >> [..] >> phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align) >> { >> return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE); > > I think this series (v2) is not correct even if now the CI tests are OK. > After re-reading carefully the lib_test_lmb_overlapping_reserve() test > it appears to me there is a contradiction. > > It's indicating that "check that calling lmb_reserve with overlapping regions fails" > > but the very last test of lib_test_lmb_overlapping_reserve() has this comment : > /* allocate 3rd region, coalesce with first and overlap with second */ > and this test allows this overlap case. > > It's not clear if LMB region can overlap each other or not ? I would say partial overlap and coalescing with before one May be Below can help /* allocate 2nd region , This should coalesced all region into one you will get one region as Address --- Size 0x40010000 --- 0x30000 Next after this /* allocate 2nd region, which should be added as first region */ we will have two region like Address --- Size (0x40000000 -- 0x8000) (0x40010000 --- 0x30000) Now third request comes in /* allocate 3rd region, coalesce with first and overlap with second */ which is address of 0x40008000 and size of 0x10000, Now this region to be added is coalescing with first (0x40000000 -- 0x8000) and part of this overlap with (0x40010000 --- 0x30000). So, what this patch does , merge all these into one region as (0x40000000 -- 0x40000) > Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range") > is authorizing LMB overlapping right ? As said before this is checking overlap and coalescing and acting accordingly. > Patrice > > > > >
On 4/14/24 13:10, Kumar, Udit wrote: > Hello Patrice, > > On 4/13/2024 1:54 PM, Patrice CHOTARD wrote: >> >> On 4/12/24 17:53, Patrice Chotard wrote: >>> In case a new region is adjacent to a previous region with >>> similar flag, this region is merged with its predecessor, but no >>> check are done if this new added region is overlapping another region >>> present in lmb (see reserved[3] which overlaps reserved[4]). >>> >>> [..] >>> phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align) >>> { >>> return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE); >> >> I think this series (v2) is not correct even if now the CI tests are OK. >> After re-reading carefully the lib_test_lmb_overlapping_reserve() test >> it appears to me there is a contradiction. >> >> It's indicating that "check that calling lmb_reserve with overlapping regions fails" >> >> but the very last test of lib_test_lmb_overlapping_reserve() has this comment : >> /* allocate 3rd region, coalesce with first and overlap with second */ >> and this test allows this overlap case. >> >> It's not clear if LMB region can overlap each other or not ? > > > I would say partial overlap and coalescing with before one > > May be Below can help > > /* allocate 2nd region , This should coalesced all region into one > > you will get one region as > > Address --- Size > > 0x40010000 --- 0x30000 > > Next after this /* allocate 2nd region, which should be added as first region */ > > we will have two region like > > Address --- Size > > (0x40000000 -- 0x8000) > > (0x40010000 --- 0x30000) > > Now third request comes in > > /* allocate 3rd region, coalesce with first and overlap with second */ > > which is address of 0x40008000 and size of 0x10000, Now this region to be added > > is coalescing with first (0x40000000 -- 0x8000) and part of this overlap with (0x40010000 --- 0x30000). > > So, what this patch does , merge all these into one region > > as (0x40000000 -- 0x40000) Hi Udit Ok, but why the second test done in test/lib/lmb.c test should be considered as failed ? ret = lmb_reserve(&lmb, 0x40010000, 0x10000); ut_asserteq(ret, 0); ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000, 0, 0, 0, 0); /* allocate overlapping region should fail */ ret = lmb_reserve(&lmb, 0x40011000, 0x10000); ut_asserteq(ret, -1); The 2 area 0x40010000 -- 0x10000 and 0x40011000 -- 0x10000 area overlaps each other and should be merged in one 0x40010000 -- 11000 ? What is the criteria to merge or not 2 overlapping areas ? For me overlapping shouldn't be authorized. Patrice > >> Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range") >> is authorizing LMB overlapping right ? > > As said before this is checking overlap and coalescing and acting accordingly. > >> Patrice >> >> >> >> >>
Hello Patrice On 4/15/2024 12:33 PM, Patrice CHOTARD wrote: > > On 4/14/24 13:10, Kumar, Udit wrote: >> Hello Patrice, >> >> On 4/13/2024 1:54 PM, Patrice CHOTARD wrote: >>> On 4/12/24 17:53, Patrice Chotard wrote: >>>> In case a new region is adjacent to a previous region with >>>> similar flag, this region is merged with its predecessor, but no >>>> check are done if this new added region is overlapping another region >>>> present in lmb (see reserved[3] which overlaps reserved[4]). >>>> >>>> [..] >>>> phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align) >>>> { >>>> return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE); >>> I think this series (v2) is not correct even if now the CI tests are OK. >>> After re-reading carefully the lib_test_lmb_overlapping_reserve() test >>> it appears to me there is a contradiction. >>> >>> It's indicating that "check that calling lmb_reserve with overlapping regions fails" >>> >>> but the very last test of lib_test_lmb_overlapping_reserve() has this comment : >>> /* allocate 3rd region, coalesce with first and overlap with second */ >>> and this test allows this overlap case. >>> >>> It's not clear if LMB region can overlap each other or not ? >> >> I would say partial overlap and coalescing with before one >> >> May be Below can help >> >> /* allocate 2nd region , This should coalesced all region into one >> >> you will get one region as >> >> Address --- Size >> >> 0x40010000 --- 0x30000 >> >> Next after this /* allocate 2nd region, which should be added as first region */ >> >> we will have two region like >> >> Address --- Size >> >> (0x40000000 -- 0x8000) >> >> (0x40010000 --- 0x30000) >> >> Now third request comes in >> >> /* allocate 3rd region, coalesce with first and overlap with second */ >> >> which is address of 0x40008000 and size of 0x10000, Now this region to be added >> >> is coalescing with first (0x40000000 -- 0x8000) and part of this overlap with (0x40010000 --- 0x30000). >> >> So, what this patch does , merge all these into one region >> >> as (0x40000000 -- 0x40000) > Hi Udit > > Ok, but why the second test done in test/lib/lmb.c test should be considered as failed ? > > > ret = lmb_reserve(&lmb, 0x40010000, 0x10000); > ut_asserteq(ret, 0); > ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000, > 0, 0, 0, 0); > /* allocate overlapping region should fail */ > ret = lmb_reserve(&lmb, 0x40011000, 0x10000); > ut_asserteq(ret, -1); > > > The 2 area 0x40010000 -- 0x10000 and 0x40011000 -- 0x10000 area overlaps each other and > should be merged in one 0x40010000 -- 11000 ? > > What is the criteria to merge or not 2 overlapping areas ? > > For me overlapping shouldn't be authorized. There are two areas 1) Overlapping : Which are not authorized 2) Overlapping and coalescing , Which is authorized to merge I am ok if you say 'coalescing and overlapping' should be treated as overlapping . as long as new region is not getting created, for above. What we see on our J784S4 EVM without patch bdinfo says , reserved[1] and reserved[2] are overlapping and should not be created. reserved.cnt = 0x4 reserved[0] [0x9e800000-0xabffffff], 0x0d800000 bytes flags: 4 reserved[1] [0xfce92000-0xffffffff], 0x0316e000 bytes flags: 0 reserved[2] [0xfde91ec0-0xfffffffe], 0x0216e13f bytes flags: 0 reserved[3] [0x880000000-0xfffffefff], 0x77ffff000 bytes flags: 0 > > Patrice > >>> Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range") >>> is authorizing LMB overlapping right ? >> As said before this is checking overlap and coalescing and acting accordingly. >> >>> Patrice >>> >>> >>> >>> >>>
diff --git a/lib/lmb.c b/lib/lmb.c index b6afb731440..4ed60f4a843 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -130,6 +130,22 @@ static void lmb_fix_over_lap_regions(struct lmb_region *rgn, unsigned long r1, lmb_remove_region(rgn, r2); } +static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base, + phys_size_t size) +{ + unsigned long i; + + for (i = 0; i < rgn->cnt; i++) { + phys_addr_t rgnbase = rgn->region[i].base; + phys_size_t rgnsize = rgn->region[i].size; + + if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) + break; + } + + return (i < rgn->cnt) ? i : -1; +} + void lmb_init(struct lmb *lmb) { #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) @@ -257,7 +273,7 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, phys_size_t size, enum lmb_flags flags) { unsigned long coalesced = 0; - long adjacent, i; + long adjacent, i, overlap; if (rgn->cnt == 0) { rgn->region[0].base = base; @@ -283,19 +299,21 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, } adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); - if (adjacent > 0) { - if (flags != rgnflags) - continue; - rgn->region[i].base -= size; - rgn->region[i].size += size; - coalesced++; - break; - } else if (adjacent < 0) { + if (adjacent != 0) { if (flags != rgnflags) continue; - rgn->region[i].size += size; - coalesced++; - break; + overlap = lmb_overlaps_region(rgn, base, size); + if (overlap < 0 || flags == rgn->region[overlap].flags) { + /* + * no overlap detected or overlap with same flags detected, + * extend region + */ + if (adjacent > 0) + rgn->region[i].base -= size; + rgn->region[i].size += size; + coalesced++; + break; + } } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { /* regions overlap */ return -1; @@ -420,21 +438,6 @@ long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size) return lmb_reserve_flags(lmb, base, size, LMB_NONE); } -static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base, - phys_size_t size) -{ - unsigned long i; - - for (i = 0; i < rgn->cnt; i++) { - phys_addr_t rgnbase = rgn->region[i].base; - phys_size_t rgnsize = rgn->region[i].size; - if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) - break; - } - - return (i < rgn->cnt) ? i : -1; -} - phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align) { return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE);
In case a new region is adjacent to a previous region with similar flag, this region is merged with its predecessor, but no check are done if this new added region is overlapping another region present in lmb (see reserved[3] which overlaps reserved[4]). This occurs when the LMB [0xdaafd000-0xddb18000] is added and overlaps the LMB [0xdbaf4380-0xddffffff]. Call lmb_overlaps_region() before merging the new region with the adjacent region already present in lmb. In case of adjacent region found, code is 90% similar in case adjacent region is located before/after the new region. Factorize adjacent region management in lmb_add_region_flags(). Issue reproduced on STM32MP157-DK2 with SCMI DT, bdinfo command's output shows: before this patch: ... lmb_dump_all: memory.cnt = 0x1 / max = 0x2 memory[0] [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0 reserved.cnt = 0x5 / max = 0x10 reserved[0] [0x10000000-0x10045fff], 0x00046000 bytes flags: 4 reserved[1] [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4 reserved[2] [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4 reserved[3] [0xdaae1000-0xdfffffff], 0x0551f000 bytes flags: 0 reserved[4] [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4 ... after this patch: ... lmb_dump_all: memory.cnt = 0x1 / max = 0x2 memory[0] [0xc0000000-0xdfffffff], 0x20000000 bytes flags: 0 reserved.cnt = 0x5 / max = 0x10 reserved[0] [0x10000000-0x10045fff], 0x00046000 bytes flags: 4 reserved[1] [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4 reserved[2] [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4 reserved[3] [0xdaae1000-0xddffffff], 0x0351f000 bytes flags: 0 reserved[4] [0xde000000-0xdfffffff], 0x02000000 bytes flags: 4 ... Fixes: 4ed6552f7159 ("[new uImage] Introduce lmb from linux kernel for memory mgmt of boot images") Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> --- Changes in v2: _ Fix lmb_add_region_flags() by updating test which leads to extend an existing region lib/lmb.c | 57 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 27 deletions(-)