[PATCHv3 6/6] memory: reserve from start_addr_sp to end_addr_sp

[Ilias Apalodimas]

[....]

> >>>          */
> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> >>> index b3b292ebf56..bd0d875a8cd 100644
> >>> --- a/lib/efi_loader/efi_memory.c
> >>> +++ b/lib/efi_loader/efi_memory.c
> >>> @@ -870,8 +870,8 @@ 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_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> >>> +       uboot_pages = ((gd->end_addr_sp - uboot_start) + EFI_PAGE_MASK) >>
> >>> +                     EFI_PAGE_SHIFT;
> >>
> >> The map_sysmem dance is still needed here for sandbox.
> >>
> >
> > Why? We're not using that address directly. We're just trying to figure out how
> > large the carveout region is.
> >
> > There's not anything funky going on with the global data structure for sandbox
> > functionality is there? LMB definitely didn't do anything to account for that.
>
> Ah. Symmetry, to answer my own question. We *may* get a virtual address.

Yes.

>
> That does bring up another issue though. What good does reserving a virtual
> region that simply maps to a physical region do? Shouldn't we reserve the
> physical region outright? Isn't that what LMB is tracking?
>
> Suppose I need to read up on this sandbox and architecture dependent virtual
> addressing scheme.

Yea and arguably we havent done a great job documenting that :(.
The tl;dr is that you are unaffected in the majority of the cases. I
wanted to fix the map_sysmem parts for a while.

Thanks
/Ilias
>
> >>>         efi_update_memory_map(uboot_start, uboot_pages, EFI_BOOT_SERVICES_CODE,
> >>>                               false, false);
> >>>  #if defined(__aarch64__)
> >>> diff --git a/lib/lmb.c b/lib/lmb.c
> >>> index 7ecc548d831..da61b005154 100644
> >>> --- 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->end_addr_sp - 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);
> >>> -
> >>> -#ifdef CFG_PRAM
> >>> -       pram = env_get_ulong("pram", 10, CFG_PRAM);
> >>> -       pram = pram << 10; /* size is in kB */
> >>> -#endif
> >>
> >> This functionality is removed, but we still have boards defining CFG_PRAM
> >
> > The PRAM allocation occurs before this, and it is carved out of the region
> > covered by start_addr_sp through end_addr_sp. It's actually part of the reason I
> > needed to add end_addr_sp, as it moves the relocation address pointer. We don't
> > need to account for it separately now.
> >
> >>> +       debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);
> >>>
> >>> -       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, bank_end - rsv_start - pram + 1,
> >>> +       if (gd->flags & GD_FLG_SKIP_RELOC)
> >>> +               lmb_reserve((phys_addr_t)(uintptr_t)_start, gd->mon_len,
> >>>                             LMB_NOOVERWRITE);
> >>> -
> >>> -               if (gd->flags & GD_FLG_SKIP_RELOC)
> >>> -                       lmb_reserve((phys_addr_t)(uintptr_t)_start,
> >>> -                                   gd->mon_len, LMB_NOOVERWRITE);
> >>> -
> >>> -               break;
> >>> -       }
> >>> +       else
> >>> +               lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);
> >>>  }
> >>>
> >>>  static void lmb_reserve_common(void *fdt_blob)
> >>> --
> >>> 2.53.0
> >>>
> >>
> >> Thanks
> >> /Ilias
>