On Fri, 2 May 2025 at 13:11, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> On Thu May 1, 2025 at 3:02 PM EEST, Sughosh Ganu wrote:
> > There currently are two API's for requesting memory from the LMB
> > module, lmb_alloc() and lmb_alloc_base(). The function which does the
> > actual allocation is the same. Use the earlier introduced API
> > lmb_allocate_mem() for both types of allocation requests.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org>
> > ---
> > arch/arm/mach-apple/board.c | 27 ++++++++++++++-----
> > arch/arm/mach-snapdragon/board.c | 13 ++++++++-
> > boot/bootm.c | 6 +++--
> > boot/image-board.c | 45 ++++++++++++++++++--------------
> > boot/image-fdt.c | 32 +++++++++++++++++------
> > include/lmb.h | 22 +++-------------
> > lib/efi_loader/efi_memory.c | 14 +++++-----
> > lib/lmb.c | 30 ++++++++++-----------
> > test/lib/lmb.c | 26 ++++++++++++++++++
> > 9 files changed, 138 insertions(+), 77 deletions(-)
> >
> > diff --git a/arch/arm/mach-apple/board.c b/arch/arm/mach-apple/board.c
> > index 2644a04a622..f0eab5df1ef 100644
> > --- a/arch/arm/mach-apple/board.c
> > +++ b/arch/arm/mach-apple/board.c
> > @@ -772,6 +772,19 @@ u64 get_page_table_size(void)
> >
> > #define KERNEL_COMP_SIZE SZ_128M
> >
> > +static phys_addr_t lmb_alloc(phys_size_t size)
> > +{
> > + int ret;
> > + phys_addr_t addr;
> > +
> > + /* All memory regions allocated with a 2MiB alignment */
> > + ret = lmb_allocate_mem(LMB_MEM_ALLOC_ANY, SZ_2M, &addr, size,
> > LMB_NONE);
> > + if (ret)
> > + return 0;
> > +
> > + return addr;
> > +}
> > +
> > int board_late_init(void)
> > {
> > u32 status = 0;
> > @@ -779,15 +792,15 @@ int board_late_init(void)
> > /* somewhat based on the Linux Kernel boot requirements:
> > * align by 2M and maximal FDT size 2M
> > */
> > - status |= env_set_hex("loadaddr", lmb_alloc(SZ_1G, SZ_2M));
> > - status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));
> > - status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_128M, SZ_2M));
> > - status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_1G, SZ_2M));
> > + status |= env_set_hex("loadaddr", lmb_alloc(SZ_1G));
>
> env_set_hex() expects a ulong, which might end up causing problems for some
> archs, but I don't think
> that's a problem of this patchset. It's something that has to be fixed in a
> the wider codebase
Yes, there seems to be a prevalence of using the ulong type instead of
a fixed address size type, or something like phys_addr_t, which might
be an issue with certain toolchains. But this issue has been around
for a long time now.
>
> > + status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M));
> > + status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_128M));
> > rd_len, LMB_NONE);
> > } else {
> > if (initrd_high)
> > - *initrd_start =
> > - (ulong)lmb_alloc_base(rd_len,
> > - 0x1000,
> > -
> > initrd_high,
> > - LMB_NONE);
> > + err = lmb_allocate_mem(LMB_MEM_ALLOC_MAX,
> > + 0x1000, &initrd_high,
> > + rd_len, LMB_NONE);
> > else
> > - *initrd_start = (ulong)lmb_alloc(rd_len,
> > - 0x1000);
> > + err = lmb_allocate_mem(LMB_MEM_ALLOC_ANY,
> > + 0x1000, &initrd_high,
> > + rd_len, LMB_NONE);
>
> You are now calling the same function, put
> LMB_MEM_ALLOC_ANY/LMB_MEM_ALLOC_MAX in a variable instead
> and make the if smaller
Will do.
>
> [...]
>
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 6585813de00..b8e1b0f35bb 100644
> > --- a/boot/image-fdt.c
> > +++ b/boot/image-fdt.c
> > @@ -198,15 +198,27 @@ int boot_relocate_fdt(char **of_flat_tree, ulong
> > *of_size)
> > of_start = (void *)(uintptr_t)addr;
> > disable_relocation = 1;
> > } else if (desired_addr) {
> > - addr = lmb_alloc_base(of_len, 0x1000, desired_addr,
> > - LMB_NONE);
> > + addr = desired_addr;
> > + err = lmb_allocate_mem(LMB_MEM_ALLOC_MAX, 0x1000,
> > &addr,
>
> Is this LMB_MEM_ALLOC_MAX or LMB_MEM_ALLOC_ADDR?
This is a case for LMB_MEM_ALLOC_MAX as the desired_addr variable that
is read as fdt_high has a valid value. So the allocated address should
be below desired_addr.
>
> > + of_len, LMB_NONE);
> > +
> > + if (err) {
> > + puts("Failed using fdt_high value for Device
> > Tree");
> > + goto error;
> > + }
> > +
> > of_start = map_sysmem(addr, of_len);
> > } else {
> > @@ -228,11 +240,15 @@ int boot_relocate_fdt(char **of_flat_tree, ulong
> > *of_size)
> >
> > switch (type) {
> > + case LMB_MEM_ALLOC_ANY:
> > + *addr = LMB_ALLOC_ANYWHERE;
> > + ret = _lmb_alloc_base(size, align, addr, flags);
> > + break;
> > + case LMB_MEM_ALLOC_MAX:
> > + ret = _lmb_alloc_base(size, align, addr, flags);
> > + break;
>
> You can make this a fallthrough
> case LMB_MEM_ALLOC_ANY:
> *addr = LMB_ALLOC_ANYWHERE;
> case LMB_MEM_ALLOC_MAX:
> ret = _lmb_alloc_base(size, align, addr, flags);
> break;
Will change. Thanks.
-sughosh
>
> > case LMB_MEM_ALLOC_ADDR:
> > ret = _lmb_alloc_addr(*addr, size, flags);
> > break;
> > diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> > index f80115570e7..8ce19efc854 100644
> > --- a/test/lib/lmb.c
> > +++ b/test/lib/lmb.c
> > @@ -82,6 +82,32 @@ static int lmb_reserve(phys_addr_t addr, phys_size_t
> > size, u32 flags)
> > return 0;
> > }
> >
> > +static phys_addr_t lmb_alloc(phys_size_t size, ulong align)
> > +{
> > + int err;
> > + phys_addr_t addr;
> > +
> > + err = lmb_allocate_mem(LMB_MEM_ALLOC_ANY, align, &addr, size,
> > LMB_NONE);
> > + if (err)
> > + return 0;
> > +
> > + return addr;
> > +}
> > +
> > +static phys_addr_t lmb_alloc_base(phys_size_t size, ulong align,
> > + phys_addr_t max_addr, u32 flags)
> > +{
> > + int err;
> > + phys_addr_t addr;
> > +
> > + addr = max_addr;
> > + err = lmb_allocate_mem(LMB_MEM_ALLOC_MAX, align, &addr, size, flags);
> > + if (err)
> > + return 0;
> > +
> > + return addr;
> > +}
> > +
> > #define lmb_alloc_addr(addr, size, flags) lmb_reserve(addr, size, flags)
> >
> > static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t
> > ram,
>
> Thanks
> /Ilias
