[PATCHv3 2/6] lmb: add LMB_FDT for fdt reserved regions
Ilias Apalodimas
ilias.apalodimas at linaro.org
Mon Apr 20 19:06:46 CEST 2026
On Mon, 20 Apr 2026 at 20:03, Randolph Sapp <rs at ti.com> wrote:
>
> On Mon Apr 20, 2026 at 11:47 AM CDT, Randolph Sapp wrote:
> > On Mon Apr 20, 2026 at 9:30 AM CDT, Ilias Apalodimas wrote:
> >> On Fri, 17 Apr 2026 at 19:53, Randolph Sapp <rs at ti.com> wrote:
> >>>
> >>> On Fri Apr 17, 2026 at 3:12 AM CDT, Ilias Apalodimas wrote:
> >>> > On Thu, 16 Apr 2026 at 23:32, Randolph Sapp <rs at ti.com> wrote:
> >>> >>
> >>> >> On Thu Apr 16, 2026 at 2:54 PM CDT, Ilias Apalodimas wrote:
> >>> >> > On Thu, 16 Apr 2026 at 22:23, Randolph Sapp <rs at ti.com> wrote:
> >>> >> >>
> >>> >> >> On Thu Apr 16, 2026 at 3:39 AM CDT, Ilias Apalodimas wrote:
> >>> >> >> > Hi Randolph.
> >>> >> >> >
> >>> >> >> > On Mon, 13 Apr 2026 at 23:36, <rs at ti.com> wrote:
> >>> >> >> >>
> >>> >> >> >> From: Randolph Sapp <rs at ti.com>
> >>> >> >> >>
> >>> >> >> >> Add an LMB_FDT bit for fdt reserved regions, so we can reclaim them when
> >>> >> >> >> parsing a new device tree and properly warn people when a reservation
> >>> >> >> >> overlaps with an existing allocation.
> >>> >> >> >
> >>> >> >> > The LMB has a set of regions that clearly describe memory in an
> >>> >> >> > abstact way, e.g reserved, no overwrite etc.
> >>> >> >> > I don't think adding we should open the door for treating reserved
> >>> >> >> > memory as special. Can't we apply the same fix without adding a new
> >>> >> >> > description?
> >>> >> >> >
> >>> >> >> > Thanks
> >>> >> >> > /Ilias
> >>> >> >>
> >>> >> >> I did have an initial implementation using a separate allocation list for fdt
> >>> >> >> regions in LMB. It works, but boy it's much more messy. All allocations and
> >>> >> >> frees had to check both the regular allocation list and the fdt allocation list.
> >>> >> >>
> >>> >> >> I thought about having the fdt helpers build their own local list separate from
> >>> >> >> LMB, but that's also a little messy in that it means we're either double
> >>> >> >> tracking regions or tying to reach into the guts of LMB to extract information
> >>> >> >> about that existing allocation region.
> >>> >> >>
> >>> >> >> Maybe that's fine considering LMB already reaches out to kick
> >>> >> >> boot_fdt_add_mem_rsv_regions. Just trying to keep things compartmentalized at
> >>> >> >> the moment.
> >>> >> >>
> >>> >> >> I'll revisit the localized reservation list for image-fdt. I'll just setup some
> >>> >> >> goofy linked list to track allocation start and size for the later free. I
> >>> >> >> assume we prefer a separate set of lmb_region structs for that list as it's a
> >>> >> >> little concerning to pass references to an active LMB region outside of the LMB
> >>> >> >> core.
> >>> >> >
> >>> >> > Can you explain a bit more when the problem happens? Is it because
> >>> >> > boot_fdt_add_mem_rsv_regions() get called twice in some boot flows? Or
> >>> >> > is there another reason?
> >>> >> >
> >>> >> > Thanks
> >>> >> > /Ilias
> >>> >>
> >>> >> The initial comment explicitly calls out that as the reason they disabled the
> >>> >> warning, but it should occur any time a device tree is loaded. Internal or
> >>> >> external.
> >>> >>
> >>> >
> >>> > Yes, but that's requesting to reserve specific address(es) which is
> >>> > always the same address ranges unless the DTB changes. In which case
> >>> > you just have to find and free the reserved-ranges. Keeping in mind
> >>> > that's rare, you can re-parse the DT with minimal overhead and you
> >>> > dont have to keep any metadat stored in LMB or anywhere else. and the
> >>> > LMB subsystem is not the place to do that. The EEXIST basically means
> >>> > "I've already done that" here so I am trying to understand what can
> >>> > trigger this problem.
> >>> >
> >>> >> That also lead me to believe if one were to load multiple external device trees
> >>> >> that you could theoretically trip an OOM with a bunch of non-overlapping
> >>> >> reserved regions. I have yet to test that, but culling old allocations seemed
> >>> >> practical enough without that PoC.
> >>> >
> >>> > Yea but that's a user error. You can also load a bigger binary that
> >>> > your entire DRAM and trigger an OOM
> >>> >
> >>> > Thanks
> >>> > /Ilias
> >>>
> >>> The explicit case were I really wish we had this warning was when the u-boot
> >>> stack was creeping into a FDT reserved region. The FDT reservation naturally
> >>> failed with EEXIST. That error is overloaded as is. We have to track the
> >>> reservations we successfully create in order to clean them up properly.
> >>
> >> Ok, I think we might have a similar issue to the EFI subsystem then,
> >> were we can unload and replace the DT based on various config options.
> >> Why don't we just add a function that read the (existing loaded) DT
> >> and free the LMB regions based on the DT info?
> >>
> >> Cheers
> >> /Ilias
> >
> > I did think about that for a bit. Issue is, we need to make sure that function
> > gets called in all of the paths this function is expected to be called prior to
> > the device tree being switched out. It's doable, but it's a little less
> > foolproof.
>
> It would also have to blindly assume the reservation worked the first time. No
> way to know if something else just so happened to reserve that region before the
> device tree. Fingers crossed freeing it won't be a problem.
Yea I think that's fine. Because if it didn't work we should report
that way earlier (and perhaps stop loading because that's a critical
error) before we reach the point of freeing it.
Cheers
/Ilias
>
> > I'll play around with that idea a little more, as it's certainly better for
> > low-mem applications. My localized tracking patch is here if anyone's curious
> > about that implementation:
> >
> > https://github.com/StaticRocket/u-boot/commit/d0d86e065c9a3588977f4bebf6fd783936de0c82
> >
> >>>
> >>> >>
> >>> >> >>
> >>> >> >> >>
> >>> >> >> >> If we don't at least warn the user of these reservation failures,
> >>> >> >> >> there's a chance that this region could be freed and reallocated for
> >>> >> >> >> something important later.
> >>> >> >> >>
> >>> >> >> >> This useful warning mechanism was broken in:
> >>> >> >> >> 5a6aa7d5913 ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")
> >>> >> >> >>
> >>> >> >> >> Signed-off-by: Randolph Sapp <rs at ti.com>
> >>> >> >> >> ---
> >>> >> >> >> boot/image-fdt.c | 5 ++++-
> >>> >> >> >> include/lmb.h | 14 ++++++++++++++
> >>> >> >> >> lib/lmb.c | 33 +++++++++++++++++++++++++++++----
> >>> >> >> >> 3 files changed, 47 insertions(+), 5 deletions(-)
> >>> >> >> >>
> >>> >> >> >> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> >>> >> >> >> index a3a4fb8b558..0f5857f24d2 100644
> >>> >> >> >> --- a/boot/image-fdt.c
> >>> >> >> >> +++ b/boot/image-fdt.c
> >>> >> >> >> @@ -73,6 +73,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags)
> >>> >> >> >> {
> >>> >> >> >> long ret;
> >>> >> >> >> phys_addr_t rsv_addr;
> >>> >> >> >> + flags |= LMB_FDT;
> >>> >> >> >>
> >>> >> >> >> rsv_addr = (phys_addr_t)addr;
> >>> >> >> >> ret = lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &rsv_addr, size, flags);
> >>> >> >> >> @@ -80,7 +81,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags)
> >>> >> >> >> debug(" reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
> >>> >> >> >> (unsigned long long)addr,
> >>> >> >> >> (unsigned long long)size, flags);
> >>> >> >> >> - } else if (ret != -EEXIST && ret != -EINVAL) {
> >>> >> >> >> + } else if (ret != -EINVAL) {
> >>> >> >> >> puts("ERROR: reserving fdt memory region failed ");
> >>> >> >> >> printf("(addr=%llx size=%llx flags=%x)\n",
> >>> >> >> >> (unsigned long long)addr,
> >>> >> >> >> @@ -108,6 +109,8 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
> >>> >> >> >> if (fdt_check_header(fdt_blob) != 0)
> >>> >> >> >> return;
> >>> >> >> >>
> >>> >> >> >> + lmb_free_fdt_regions();
> >>> >> >> >> +
> >>> >> >> >> /* process memreserve sections */
> >>> >> >> >> total = fdt_num_mem_rsv(fdt_blob);
> >>> >> >> >> for (i = 0; i < total; i++) {
> >>> >> >> >> diff --git a/include/lmb.h b/include/lmb.h
> >>> >> >> >> index 427d701bc30..c6a1fc1ca47 100644
> >>> >> >> >> --- a/include/lmb.h
> >>> >> >> >> +++ b/include/lmb.h
> >>> >> >> >> @@ -51,6 +51,15 @@
> >>> >> >> >> */
> >>> >> >> >> #define LMB_NONOTIFY BIT(3)
> >>> >> >> >>
> >>> >> >> >> +/**
> >>> >> >> >> + * define LMB_FDT - reclaim this region with lmb_free_fdt_regions()
> >>> >> >> >> + *
> >>> >> >> >> + * LMB Memory region attribute flag to indicate that the region will be
> >>> >> >> >> + * reclaimed with lmb_free_fdt_regions(). This allows device tree reservations
> >>> >> >> >> + * to be cleaned up and tracked more granularly.
> >>> >> >> >> + */
> >>> >> >> >> +#define LMB_FDT BIT(4)
> >>> >> >> >> +
> >>> >> >> >> /**
> >>> >> >> >> * enum lmb_mem_type - type of memory allocation request
> >>> >> >> >> * @LMB_MEM_ALLOC_ADDR: request for a particular region of memory
> >>> >> >> >> @@ -235,6 +244,11 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align);
> >>> >> >> >> */
> >>> >> >> >> long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size);
> >>> >> >> >>
> >>> >> >> >> +/**
> >>> >> >> >> + * lmb_free_fdt_regions() - Reclaim all %LMB_FDT tagged reserved regions
> >>> >> >> >> + */
> >>> >> >> >> +void lmb_free_fdt_regions(void);
> >>> >> >> >> +
> >>> >> >> >> #endif /* __KERNEL__ */
> >>> >> >> >>
> >>> >> >> >> #endif /* _LINUX_LMB_H */
> >>> >> >> >> diff --git a/lib/lmb.c b/lib/lmb.c
> >>> >> >> >> index 8f12c6ad8e5..7ecc548d831 100644
> >>> >> >> >> --- a/lib/lmb.c
> >>> >> >> >> +++ b/lib/lmb.c
> >>> >> >> >> @@ -463,10 +463,10 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size,
> >>> >> >> >>
> >>> >> >> >> static void lmb_print_region_flags(u32 flags)
> >>> >> >> >> {
> >>> >> >> >> - const char * const flag_str[] = { "none", "no-map", "no-overwrite",
> >>> >> >> >> - "no-notify" };
> >>> >> >> >> - unsigned int pflags = flags &
> >>> >> >> >> - (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY);
> >>> >> >> >> + const char *const flag_str[] = { "none", "no-map", "no-overwrite",
> >>> >> >> >> + "no-notify", "fdt" };
> >>> >> >> >> + unsigned int pflags =
> >>> >> >> >> + flags & (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY | LMB_FDT);
> >>> >> >> >>
> >>> >> >> >> if (flags != pflags) {
> >>> >> >> >> printf("invalid %#x\n", flags);
> >>> >> >> >> @@ -654,6 +654,31 @@ long lmb_free(phys_addr_t base, phys_size_t size, u32 flags)
> >>> >> >> >> return lmb_map_update_notify(base, size, LMB_MAP_OP_FREE, flags);
> >>> >> >> >> }
> >>> >> >> >>
> >>> >> >> >> +void lmb_free_fdt_regions(void)
> >>> >> >> >> +{
> >>> >> >> >> + struct alist *lmb_rgn_lst = &lmb.used_mem;
> >>> >> >> >> + struct lmb_region *rgn = lmb_rgn_lst->data;
> >>> >> >> >> + long ret;
> >>> >> >> >> + int i = 0;
> >>> >> >> >> +
> >>> >> >> >> + while (i < lmb_rgn_lst->count) {
> >>> >> >> >> + phys_addr_t base = rgn[i].base;
> >>> >> >> >> + phys_size_t size = rgn[i].size;
> >>> >> >> >> + u32 flags = rgn[i].flags;
> >>> >> >> >> +
> >>> >> >> >> + if (flags & LMB_FDT) {
> >>> >> >> >> + ret = lmb_free(base, size, flags);
> >>> >> >> >> + if (ret < 0) {
> >>> >> >> >> + printf("Unable to free FDT memory at 0x%08lx\n",
> >>> >> >> >> + (ulong)base);
> >>> >> >> >> + i++;
> >>> >> >> >> + }
> >>> >> >> >> + } else {
> >>> >> >> >> + i++;
> >>> >> >> >> + }
> >>> >> >> >> + }
> >>> >> >> >> +}
> >>> >> >> >> +
> >>> >> >> >> static int _lmb_alloc_base(phys_size_t size, ulong align,
> >>> >> >> >> phys_addr_t *addr, u32 flags)
> >>> >> >> >> {
> >>> >> >> >> --
> >>> >> >> >> 2.53.0
> >>> >> >> >>
> >>> >> >>
> >>> >>
> >>>
>
More information about the U-Boot
mailing list