Re: [PATCHv3 2/6] lmb: add LMB_FDT for fdt reserved regions

[Randolph Sapp <rs@ti.com>]

On Thu Apr 16, 2026 at 2:54 PM CDT, Ilias Apalodimas wrote:
> On Thu, 16 Apr 2026 at 22:23, Randolph Sapp <rs@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@ti.com> wrote:
>> >>
>> >> From: Randolph Sapp <rs@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.

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.

>>
>> >>
>> >> 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@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
>> >>
>>