| Message ID | 20240704073544.670249-13-sughosh.ganu@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Make U-Boot memory reservations coherent | expand |
Hi Sughosh, On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Allow for resizing of LMB regions if the region attributes match. The > current code returns a failure status on detecting an overlapping > address. This worked up until now since the LMB calls were not > persistent and global -- the LMB memory map was specific and private > to a given caller of the LMB API's. > > With the change in the LMB code to make the LMB reservations > persistent, there needs to be a check on whether the memory region can > be resized, and then do it if so. To distinguish between memory that > cannot be resized, add a new flag, LMB_NOOVERWRITE. Reserving a region > of memory with this attribute would indicate that the region cannot be > resized. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since V1: > * Do the check for the region flags in lmb_resize_regions() and > lmb_merge_overlap_regions() to decide on merging the overlapping > regions. > > include/lmb.h | 1 + > lib/lmb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 103 insertions(+), 14 deletions(-) > > diff --git a/include/lmb.h b/include/lmb.h > index 069c6af2a3..99fcf5781f 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -20,6 +20,7 @@ > enum lmb_flags { > LMB_NONE = 0x0, > LMB_NOMAP = 0x4, > + LMB_NOOVERWRITE = 0x8, How about LMB_PERSIST ? These could be adjusted to use BIT() > }; > > /** > diff --git a/lib/lmb.c b/lib/lmb.c > index 0d01e58a46..80945e3cae 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -230,12 +230,88 @@ void lmb_init_and_reserve_range(phys_addr_t base, phys_size_t size, > lmb_reserve_common(fdt_blob); > } > > +static bool lmb_region_flags_match(struct lmb_region *rgn, unsigned long r1, At some point the index should change to an int (or uint), rather than unsigned long. I had a series that did that, but it died. > + enum lmb_flags flags) > +{ > + return rgn->region[r1].flags == flags; > +} > + > +static long lmb_merge_overlap_regions(struct lmb_region *rgn, unsigned long i, > + phys_addr_t base, phys_size_t size, > + enum lmb_flags flags) > +{ > + phys_size_t rgnsize; > + unsigned long rgn_cnt, idx; > + phys_addr_t rgnbase, rgnend; > + phys_addr_t mergebase, mergeend; > + > + rgn_cnt = 0; > + idx = i; > + /* > + * First thing to do is to identify how many regions does > + * the requested region overlap. how many regions the requested region overlaps > + * If the flags match, combine all these overlapping > + * regions into a single region, and remove the merged > + * regions. > + */ > + while (idx < rgn->cnt - 1) { > + rgnbase = rgn->region[idx].base; > + rgnsize = rgn->region[idx].size; > + > + if (lmb_addrs_overlap(base, size, rgnbase, > + rgnsize)) { > + if (!lmb_region_flags_match(rgn, idx, flags)) > + return -1; > + rgn_cnt++; > + idx++; > + } > + } > + > + /* The merged region's base and size */ > + rgnbase = rgn->region[i].base; > + mergebase = min(base, rgnbase); > + rgnend = rgn->region[idx].base + rgn->region[idx].size; > + mergeend = max(rgnend, (base + size)); > + > + rgn->region[i].base = mergebase; > + rgn->region[i].size = mergeend - mergebase; > + > + /* Now remove the merged regions */ > + while (--rgn_cnt) > + lmb_remove_region(rgn, i + 1); > + > + return 0; > +} > + > +static long lmb_resize_regions(struct lmb_region *rgn, unsigned long i, > + phys_addr_t base, phys_size_t size, > + enum lmb_flags flags) > +{ > + long ret = 0; > + phys_addr_t rgnend; > + > + if (i == rgn->cnt - 1 || > + base + size < rgn->region[i + 1].base) { > + if (!lmb_region_flags_match(rgn, i, flags)) > + return -1; > + > + rgnend = rgn->region[i].base + rgn->region[i].size; > + rgn->region[i].base = min(base, rgn->region[i].base); > + rgnend = max(base + size, rgnend); > + rgn->region[i].size = rgnend - rgn->region[i].base; > + } else { > + ret = lmb_merge_overlap_regions(rgn, i, base, size, flags); > + } > + > + return ret; > +} > + > /* This routine called with relocation disabled. */ > static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > phys_size_t size, enum lmb_flags flags) This could really use a function comment > { > unsigned long coalesced = 0; > - long adjacent, i; > + long ret, i; > > if (rgn->cnt == 0) { > rgn->region[0].base = base; > @@ -260,23 +336,28 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > return -1; /* regions with new flags */ > } > > - adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > - if (adjacent > 0) { > + ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > + if (ret > 0) { > if (flags != rgnflags) > break; > rgn->region[i].base -= size; > rgn->region[i].size += size; > coalesced++; > break; > - } else if (adjacent < 0) { > + } else if (ret < 0) { > if (flags != rgnflags) > break; > rgn->region[i].size += size; > coalesced++; > break; > } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { > - /* regions overlap */ > - return -1; > + ret = lmb_resize_regions(rgn, i, base, size, > + flags); > + if (ret < 0) > + return -1; > + > + coalesced++; > + break; > } > } > > @@ -418,7 +499,7 @@ static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size) > } > Regards, Simon
hi Simon, On Sat, 13 Jul 2024 at 20:45, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > Allow for resizing of LMB regions if the region attributes match. The > > current code returns a failure status on detecting an overlapping > > address. This worked up until now since the LMB calls were not > > persistent and global -- the LMB memory map was specific and private > > to a given caller of the LMB API's. > > > > With the change in the LMB code to make the LMB reservations > > persistent, there needs to be a check on whether the memory region can > > be resized, and then do it if so. To distinguish between memory that > > cannot be resized, add a new flag, LMB_NOOVERWRITE. Reserving a region > > of memory with this attribute would indicate that the region cannot be > > resized. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > Changes since V1: > > * Do the check for the region flags in lmb_resize_regions() and > > lmb_merge_overlap_regions() to decide on merging the overlapping > > regions. > > > > include/lmb.h | 1 + > > lib/lmb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ > > 2 files changed, 103 insertions(+), 14 deletions(-) > > > > diff --git a/include/lmb.h b/include/lmb.h > > index 069c6af2a3..99fcf5781f 100644 > > --- a/include/lmb.h > > +++ b/include/lmb.h > > @@ -20,6 +20,7 @@ > > enum lmb_flags { > > LMB_NONE = 0x0, > > LMB_NOMAP = 0x4, > > + LMB_NOOVERWRITE = 0x8, > > How about LMB_PERSIST ? Isn't LMB_NOOVERWRITE more suitable here ? I mean this is indicating that the said region of memory is not to be re-used/re-requested. > > These could be adjusted to use BIT() I am changing these to use the BIT macro in a subsequent commit. > > > }; > > > > /** > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 0d01e58a46..80945e3cae 100644 > > --- a/lib/lmb.c > > +++ b/lib/lmb.c > > @@ -230,12 +230,88 @@ void lmb_init_and_reserve_range(phys_addr_t base, phys_size_t size, > > lmb_reserve_common(fdt_blob); > > } > > > > +static bool lmb_region_flags_match(struct lmb_region *rgn, unsigned long r1, > > At some point the index should change to an int (or uint), rather than > unsigned long. I had a series that did that, but it died. > > > + enum lmb_flags flags) > > +{ > > + return rgn->region[r1].flags == flags; > > +} > > + > > +static long lmb_merge_overlap_regions(struct lmb_region *rgn, unsigned long i, > > + phys_addr_t base, phys_size_t size, > > + enum lmb_flags flags) > > +{ > > + phys_size_t rgnsize; > > + unsigned long rgn_cnt, idx; > > + phys_addr_t rgnbase, rgnend; > > + phys_addr_t mergebase, mergeend; > > + > > + rgn_cnt = 0; > > + idx = i; > > + /* > > + * First thing to do is to identify how many regions does > > + * the requested region overlap. > > how many regions the requested region overlaps Okay > > > + * If the flags match, combine all these overlapping > > + * regions into a single region, and remove the merged > > + * regions. > > + */ > > + while (idx < rgn->cnt - 1) { > > + rgnbase = rgn->region[idx].base; > > + rgnsize = rgn->region[idx].size; > > + > > + if (lmb_addrs_overlap(base, size, rgnbase, > > + rgnsize)) { > > + if (!lmb_region_flags_match(rgn, idx, flags)) > > + return -1; > > + rgn_cnt++; > > + idx++; > > + } > > + } > > + > > + /* The merged region's base and size */ > > + rgnbase = rgn->region[i].base; > > + mergebase = min(base, rgnbase); > > + rgnend = rgn->region[idx].base + rgn->region[idx].size; > > + mergeend = max(rgnend, (base + size)); > > + > > + rgn->region[i].base = mergebase; > > + rgn->region[i].size = mergeend - mergebase; > > + > > + /* Now remove the merged regions */ > > + while (--rgn_cnt) > > + lmb_remove_region(rgn, i + 1); > > + > > + return 0; > > +} > > + > > +static long lmb_resize_regions(struct lmb_region *rgn, unsigned long i, > > + phys_addr_t base, phys_size_t size, > > + enum lmb_flags flags) > > +{ > > + long ret = 0; > > + phys_addr_t rgnend; > > + > > + if (i == rgn->cnt - 1 || > > + base + size < rgn->region[i + 1].base) { > > + if (!lmb_region_flags_match(rgn, i, flags)) > > + return -1; > > + > > + rgnend = rgn->region[i].base + rgn->region[i].size; > > + rgn->region[i].base = min(base, rgn->region[i].base); > > + rgnend = max(base + size, rgnend); > > + rgn->region[i].size = rgnend - rgn->region[i].base; > > + } else { > > + ret = lmb_merge_overlap_regions(rgn, i, base, size, flags); > > + } > > + > > + return ret; > > +} > > + > > /* This routine called with relocation disabled. */ > > static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > > phys_size_t size, enum lmb_flags flags) > > This could really use a function comment Okay -sughosh > > > { > > unsigned long coalesced = 0; > > - long adjacent, i; > > + long ret, i; > > > > if (rgn->cnt == 0) { > > rgn->region[0].base = base; > > @@ -260,23 +336,28 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, > > return -1; /* regions with new flags */ > > } > > > > - adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > > - if (adjacent > 0) { > > + ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); > > + if (ret > 0) { > > if (flags != rgnflags) > > break; > > rgn->region[i].base -= size; > > rgn->region[i].size += size; > > coalesced++; > > break; > > - } else if (adjacent < 0) { > > + } else if (ret < 0) { > > if (flags != rgnflags) > > break; > > rgn->region[i].size += size; > > coalesced++; > > break; > > } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { > > - /* regions overlap */ > > - return -1; > > + ret = lmb_resize_regions(rgn, i, base, size, > > + flags); > > + if (ret < 0) > > + return -1; > > + > > + coalesced++; > > + break; > > } > > } > > > > @@ -418,7 +499,7 @@ static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size) > > } > > > > Regards, > Simon
Hi Sughosh, On Mon, 15 Jul 2024 at 10:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Sat, 13 Jul 2024 at 20:45, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > Allow for resizing of LMB regions if the region attributes match. The > > > current code returns a failure status on detecting an overlapping > > > address. This worked up until now since the LMB calls were not > > > persistent and global -- the LMB memory map was specific and private > > > to a given caller of the LMB API's. > > > > > > With the change in the LMB code to make the LMB reservations > > > persistent, there needs to be a check on whether the memory region can > > > be resized, and then do it if so. To distinguish between memory that > > > cannot be resized, add a new flag, LMB_NOOVERWRITE. Reserving a region > > > of memory with this attribute would indicate that the region cannot be > > > resized. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > Changes since V1: > > > * Do the check for the region flags in lmb_resize_regions() and > > > lmb_merge_overlap_regions() to decide on merging the overlapping > > > regions. > > > > > > include/lmb.h | 1 + > > > lib/lmb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ > > > 2 files changed, 103 insertions(+), 14 deletions(-) > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > index 069c6af2a3..99fcf5781f 100644 > > > --- a/include/lmb.h > > > +++ b/include/lmb.h > > > @@ -20,6 +20,7 @@ > > > enum lmb_flags { > > > LMB_NONE = 0x0, > > > LMB_NOMAP = 0x4, > > > + LMB_NOOVERWRITE = 0x8, > > > > How about LMB_PERSIST ? > > Isn't LMB_NOOVERWRITE more suitable here ? I mean this is indicating > that the said region of memory is not to be re-used/re-requested. > > > > > These could be adjusted to use BIT() > > I am changing these to use the BIT macro in a subsequent commit. Yes, I saw that later and forgot to remove this. Normally we make refactoring changes before adding new code, though. [..] Regards, Simon
hi Simon, On Mon, 15 Jul 2024 at 17:09, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 15 Jul 2024 at 10:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Sat, 13 Jul 2024 at 20:45, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Thu, 4 Jul 2024 at 08:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > Allow for resizing of LMB regions if the region attributes match. The > > > > current code returns a failure status on detecting an overlapping > > > > address. This worked up until now since the LMB calls were not > > > > persistent and global -- the LMB memory map was specific and private > > > > to a given caller of the LMB API's. > > > > > > > > With the change in the LMB code to make the LMB reservations > > > > persistent, there needs to be a check on whether the memory region can > > > > be resized, and then do it if so. To distinguish between memory that > > > > cannot be resized, add a new flag, LMB_NOOVERWRITE. Reserving a region > > > > of memory with this attribute would indicate that the region cannot be > > > > resized. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > Changes since V1: > > > > * Do the check for the region flags in lmb_resize_regions() and > > > > lmb_merge_overlap_regions() to decide on merging the overlapping > > > > regions. > > > > > > > > include/lmb.h | 1 + > > > > lib/lmb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ > > > > 2 files changed, 103 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > > index 069c6af2a3..99fcf5781f 100644 > > > > --- a/include/lmb.h > > > > +++ b/include/lmb.h > > > > @@ -20,6 +20,7 @@ > > > > enum lmb_flags { > > > > LMB_NONE = 0x0, > > > > LMB_NOMAP = 0x4, > > > > + LMB_NOOVERWRITE = 0x8, > > > > > > How about LMB_PERSIST ? > > > > Isn't LMB_NOOVERWRITE more suitable here ? I mean this is indicating > > that the said region of memory is not to be re-used/re-requested. > > > > > > > > These could be adjusted to use BIT() > > > > I am changing these to use the BIT macro in a subsequent commit. > > Yes, I saw that later and forgot to remove this. Normally we make > refactoring changes before adding new code, though. Okay, I will bring in that patch earlier in the series. -sughosh > [..] > > Regards, > Simon
diff --git a/include/lmb.h b/include/lmb.h index 069c6af2a3..99fcf5781f 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -20,6 +20,7 @@ enum lmb_flags { LMB_NONE = 0x0, LMB_NOMAP = 0x4, + LMB_NOOVERWRITE = 0x8, }; /** diff --git a/lib/lmb.c b/lib/lmb.c index 0d01e58a46..80945e3cae 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -230,12 +230,88 @@ void lmb_init_and_reserve_range(phys_addr_t base, phys_size_t size, lmb_reserve_common(fdt_blob); } +static bool lmb_region_flags_match(struct lmb_region *rgn, unsigned long r1, + enum lmb_flags flags) +{ + return rgn->region[r1].flags == flags; +} + +static long lmb_merge_overlap_regions(struct lmb_region *rgn, unsigned long i, + phys_addr_t base, phys_size_t size, + enum lmb_flags flags) +{ + phys_size_t rgnsize; + unsigned long rgn_cnt, idx; + phys_addr_t rgnbase, rgnend; + phys_addr_t mergebase, mergeend; + + rgn_cnt = 0; + idx = i; + /* + * First thing to do is to identify how many regions does + * the requested region overlap. + * If the flags match, combine all these overlapping + * regions into a single region, and remove the merged + * regions. + */ + while (idx < rgn->cnt - 1) { + rgnbase = rgn->region[idx].base; + rgnsize = rgn->region[idx].size; + + if (lmb_addrs_overlap(base, size, rgnbase, + rgnsize)) { + if (!lmb_region_flags_match(rgn, idx, flags)) + return -1; + rgn_cnt++; + idx++; + } + } + + /* The merged region's base and size */ + rgnbase = rgn->region[i].base; + mergebase = min(base, rgnbase); + rgnend = rgn->region[idx].base + rgn->region[idx].size; + mergeend = max(rgnend, (base + size)); + + rgn->region[i].base = mergebase; + rgn->region[i].size = mergeend - mergebase; + + /* Now remove the merged regions */ + while (--rgn_cnt) + lmb_remove_region(rgn, i + 1); + + return 0; +} + +static long lmb_resize_regions(struct lmb_region *rgn, unsigned long i, + phys_addr_t base, phys_size_t size, + enum lmb_flags flags) +{ + long ret = 0; + phys_addr_t rgnend; + + if (i == rgn->cnt - 1 || + base + size < rgn->region[i + 1].base) { + if (!lmb_region_flags_match(rgn, i, flags)) + return -1; + + rgnend = rgn->region[i].base + rgn->region[i].size; + rgn->region[i].base = min(base, rgn->region[i].base); + rgnend = max(base + size, rgnend); + rgn->region[i].size = rgnend - rgn->region[i].base; + } else { + ret = lmb_merge_overlap_regions(rgn, i, base, size, flags); + } + + return ret; +} + /* This routine called with relocation disabled. */ 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 ret, i; if (rgn->cnt == 0) { rgn->region[0].base = base; @@ -260,23 +336,28 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, return -1; /* regions with new flags */ } - adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); - if (adjacent > 0) { + ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); + if (ret > 0) { if (flags != rgnflags) break; rgn->region[i].base -= size; rgn->region[i].size += size; coalesced++; break; - } else if (adjacent < 0) { + } else if (ret < 0) { if (flags != rgnflags) break; rgn->region[i].size += size; coalesced++; break; } else if (lmb_addrs_overlap(base, size, rgnbase, rgnsize)) { - /* regions overlap */ - return -1; + ret = lmb_resize_regions(rgn, i, base, size, + flags); + if (ret < 0) + return -1; + + coalesced++; + break; } } @@ -418,7 +499,7 @@ static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size) } static phys_addr_t __lmb_alloc_base(phys_size_t size, ulong align, - phys_addr_t max_addr) + phys_addr_t max_addr, enum lmb_flags flags) { long i, rgn; phys_addr_t base = 0; @@ -468,7 +549,7 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) { phys_addr_t alloc; - alloc = __lmb_alloc_base(size, align, max_addr); + alloc = __lmb_alloc_base(size, align, max_addr, LMB_NONE); if (alloc == 0) printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", @@ -477,11 +558,8 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) return alloc; } -/* - * Try to allocate a specific address range: must be in defined memory but not - * reserved - */ -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size) +static phys_addr_t __lmb_alloc_addr(phys_addr_t base, phys_size_t size, + enum lmb_flags flags) { long rgn; @@ -496,13 +574,23 @@ phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size) lmb->memory.region[rgn].size, base + size - 1, 1)) { /* ok, reserve the memory */ - if (lmb_reserve(base, size) >= 0) + if (lmb_reserve_flags(base, size, flags) >= 0) return base; } } + return 0; } +/* + * Try to allocate a specific address range: must be in defined memory but not + * reserved + */ +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size) +{ + return __lmb_alloc_addr(base, size, LMB_NONE); +} + /* Return number of bytes from a given address that are free */ phys_size_t lmb_get_free_size(phys_addr_t addr) {
Allow for resizing of LMB regions if the region attributes match. The current code returns a failure status on detecting an overlapping address. This worked up until now since the LMB calls were not persistent and global -- the LMB memory map was specific and private to a given caller of the LMB API's. With the change in the LMB code to make the LMB reservations persistent, there needs to be a check on whether the memory region can be resized, and then do it if so. To distinguish between memory that cannot be resized, add a new flag, LMB_NOOVERWRITE. Reserving a region of memory with this attribute would indicate that the region cannot be resized. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V1: * Do the check for the region flags in lmb_resize_regions() and lmb_merge_overlap_regions() to decide on merging the overlapping regions. include/lmb.h | 1 + lib/lmb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 103 insertions(+), 14 deletions(-)