[PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr
Simon Glass
sjg at chromium.org
Wed May 13 18:39:17 CEST 2026
Hi Randolph,
On Mon, 11 May 2026 at 13:41, Randolph Sapp <rs at ti.com> wrote:
>
> On Mon May 11, 2026 at 1:28 PM CDT, Simon Glass wrote:
> > 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.
>
> 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.
I'm not sure about others, but for me I struggle with a change log in
another place. It's OK to have it in both places (like patman does),
but trying to read a different email and figure out what change
relates to what patch is hard, particularly when it might have been a
day or a week since I last saw the patch.
>
> >> 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.
I think Ilias would be the right person to sort out the LMB stuff...I
don't really have much insight on that.
>
> >> 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.
OK ta.
REgards,
Simon
More information about the U-Boot
mailing list