Re: [PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr

[Randolph Sapp <rs@ti.com>]

On Mon May 11, 2026 at 1:28 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
>> memory: reserve from start_addr_sp to initial_relocaddr
>>
>> Add a new global data struct member called initial_relocaddr. This
>> stores the original value of relocaddr, directly from setup_dest_addr.
>> This is specifically to avoid any adjustments made by other init
>> functions.
>>
>> Reserve the memory from gd->start_addr_sp - CONFIG_STACK_SIZE to
>> gd->initial_relocaddr instead of gd->ram_top. This allows platform
>> specific relocation addresses to work without unnecessarily painting
>> over a large range.
>>
>> Since PRAM comes out of this initial area up to initial_relocaddr, we no
>> longer need to account for it separately.
>>
>> Signed-off-by: Randolph Sapp <rs@ti.com>
>>
>> common/board_f.c                  |  9 ++++++++-
>>  include/asm-generic/global_data.h |  7 +++++++
>>  lib/efi_loader/efi_memory.c       |  2 +-
>>  lib/lmb.c                         | 39 ++++++---------------------------------
>>  4 files changed, 22 insertions(+), 35 deletions(-)
>
> BTW I am missing a change long in this patch, so a bit of guesswork here.

I tend to keep the changelogs for a series in the series cover letter. I assume
tracking per patch is the preference for this list based on this message.

>> diff --git a/common/board_f.c b/common/board_f.c
>> @@ -330,6 +330,8 @@ __weak int arch_setup_dest_addr(void)
>>
>>  static int setup_dest_addr(void)
>>  {
>> +     int ret;
>> +
>>       debug("Monitor len: %08x\n", gd->mon_len);
>>       /*
>>        * Ram is setup, size stored in gd !!
>> @@ -356,7 +358,12 @@ static int setup_dest_addr(void)
>>       gd->relocaddr = gd->ram_top;
>>       debug("Ram top: %08llX\n", (unsigned long long)gd->ram_top);
>>
>> -     return arch_setup_dest_addr();
>> +     ret = arch_setup_dest_addr();
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     gd->initial_relocaddr = gd->relocaddr;
>> +     return ret;
>>  }
>
> Please use 'if (ret)' to match U-Boot style. The trailing 'return
> ret;' is misleading since ret is known to be zero - make it 'return
> 0;'.
>
>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>> @@ -107,6 +107,13 @@ struct global_data {
>>        * GDB using the 'add-symbol-file u-boot <relocaddr>' command.
>>        */
>>       unsigned long relocaddr;
>> +     /**
>> +      * @initial_relocaddr: end of memory currently in use by uboot
>> +      *
>> +      * This should be the original value of relocaddr before any other
>> +      * allocations or reservations shift it.
>> +      */
>> +     unsigned long initial_relocaddr;
>
> The kerneldoc summary is misleading: 'currently in use' suggests live
> state, but this is written once at the end of setup_dest_addr() and
> never updated. Please reword to something like 'initial top of the
> U-Boot reservation' and note that it is set once in setup_dest_addr().
> Also 'uboot' should be 'U-Boot'.
>
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>>
>>  static void lmb_reserve_uboot_region(void)
>>  {
>> -     int bank;
>> -     ulong end, bank_end;
>> +     ulong size;
>>       phys_addr_t rsv_start;
>> -     ulong pram = 0;
>>
>>       rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
>> -     end = gd->ram_top;
>> +     size = gd->initial_relocaddr - rsv_start;
>>
>> -     /*
>> -      * Reserve memory from aligned address below the bottom of U-Boot stack
>> -      * until end of RAM area to prevent LMB from overwriting that memory.
>> -      */
>> -     debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
>> +     debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);
>
> Please keep a trimmed version of the explanatory comment - it
> documents the intent of this reservation and is more useful than the
> surrounding context.
>
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>> -     for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> -             if (!gd->bd->bi_dram[bank].size ||
>> -                 rsv_start < gd->bd->bi_dram[bank].start)
>> -                     continue;
>> -             /* Watch out for RAM at end of address space! */
>> -             bank_end = gd->bd->bi_dram[bank].start +
>> -                     gd->bd->bi_dram[bank].size - 1;
>> -             if (rsv_start > bank_end)
>> -                     continue;
>> -             if (bank_end > end)
>> -                     bank_end = end - 1;
>> +     lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);
>
> The old code walked CONFIG_NR_DRAM_BANKS and clamped the reservation
> to the bank containing rsv_start. The new code drops that and just
> reserves [rsv_start, initial_relocaddr). On platforms with
> non-contiguous banks (where U-Boot occupies the top bank and there is
> a hole below it), this can mark a range that is not all RAM. Is that
> intentional? If so the commit message should call it out; if not,
> please re-add a sanity check so we do not reserve across holes.

I had a few thoughts about this in the past week. Initially this bank walking
behavior seemed more than a little unusual to me. It appears here and in 2 other
places. (FDT and kernel image loading, if I recall correctly.)

I was mulling over adding it back in as-is, but this really feels like something
LMB should be aware of and should handle on it's own during the reservation of
those regions. Something akin to the EFI allocator's conventional memory overlap
check.

Seems like most of the infrastructure for that is already there. We have a
separate list tracking memory banks already. It was just bothering me that there
are quite a few instances where the allocator is blindly receiving external
information instead of taking more agency in the act of reserving that memory.

Suppose I'll just reinstate that old logic though as this series has drug out
much longer than I initially intended.

>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>> -#ifdef CFG_PRAM
>> -     pram = env_get_ulong('pram', 10, CFG_PRAM);
>> -     pram = pram << 10; /* size is in kB */
>> -#endif
>
> PRAM previously sat above the LMB reservation (the old code subtracted
> pram from the size so PRAM was left out of used_mem). With this patch,
> PRAM ends up inside the [rsv_start, initial_relocaddr) range and is
> now marked LMB_NOOVERWRITE - likely this is more correct, but it is a
> behaviour change for boards setting pram at runtime to be larger than
> CFG_PRAM - those bytes used to be available to LMB clients and now are
> not.
>
> Please spell this out in the commit message rather than the brief
> 'PRAM comes out of this initial area' line, so reviewers on PRAM-using
> boards know to look.
>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> @@ -869,7 +869,7 @@ static void add_u_boot_and_runtime(void)
>>       /* Add U-Boot */
>>       uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
>>                      uboot_stack_size) & ~EFI_PAGE_MASK;
>> -     uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
>> +     uboot_pages = ((uintptr_t)map_sysmem(gd->initial_relocaddr, 0) -
>>                      uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>
> Hmmm, the current expression uses ram_top - 1 (inclusive top byte) and
> then rounded up via EFI_PAGE_MASK. The new one passes
> initial_relocaddr without the - 1 - so when initial_relocaddr is
> page-aligned this adds an extra page. What is the intention here?
>
> Regards,
> Simon

This stemmed from the misinterpretation of board_get_usable_ram_top being
exclusive. This is to be adjusted in the next revision.