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

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs at 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 at 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.

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

> 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