[PATCH v1 0/8] Cleanup the LMB subsystem
Hi all,
This is v1 of the rfc [0]
The LMB subsystem was used opportunistically for a number of years. A while back Sughosh merged it with the EFI subsystem in order to have a common allocator and avoid subsystems overwriting memory they shouldn't.
This is an initial cleanup of all the crud we gathered over the years. There's no functional change expected from the patches as they just cleanup some abstraction functions and rename a few variables to make more sense.
I plan to make even bigger changes -- e.g I don't see the point of having *_alloc() and *_reserve() versions of the functions since they mostly do the same thing and just cause confusion. lmb_alloc_addr_flags() returning the base address on success makes little sense since we already *request* the address on the function arguments, etc. Since this patchset grew enough already, I'd like to get it in before more refactoring happens.
It's worth noting that although some patches slightly increase the code size due to an extra flags argument being carried around, the final result is eventually smaller.
# qemu_arm64_lwip_defconfig (version string adds another 20b) add/remove: 0/5 grow/shrink: 15/1 up/down: 568/-628 (-60) Function old new delta lmb_alloc_base 80 324 +244 lmb_alloc_addr 8 144 +136 lmb_reserve 8 96 +88 version_string 50 70 +20 boot_relocate_fdt 488 508 +20 boot_ramdisk_high 268 284 +16 lmb_add_region_flags 696 704 +8 boot_fdt_reserve_region 100 108 +8 load_serial 548 552 +4 lmb_alloc 8 12 +4 image_setup_libfdt 368 372 +4 do_load 728 732 +4 do_bootz 332 336 +4 do_booti 520 524 +4 bootm_run_states 2176 2180 +4 lmb_alloc_addr_flags 4 - -4 boot_fdt_add_mem_rsv_regions 284 280 -4 lmb_alloc_base_flags 76 - -76 lmb_reserve_flags 96 - -96 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=1020102, After=1020042, chg -0.01%
# sandbox_defconfig (version string adds another 20b) add/remove: 0/3 grow/shrink: 24/3 up/down: 523/-501 (22) Function old new delta lmb_alloc_base 48 299 +251 lmb_alloc_addr 4 92 +88 lmb_reserve 4 58 +54 test_alloc_addr 2933 2963 +30 version_string 50 70 +20 lib_test_lmb_overlapping_reserve 1018 1030 +12 lmb_add_region_flags 600 610 +10 test_multi_alloc.constprop 3034 3042 +8 test_get_unreserved_size 1032 1038 +6 boot_relocate_fdt 599 605 +6 boot_fdt_reserve_region 67 73 +6 lmb_alloc 4 9 +5 lmb_free_flags 190 194 +4 wget_handler 1530 1533 +3 tftp_handler 1190 1192 +2 test_noreserved 1207 1209 +2 test_bigblock 911 913 +2 load_serial 946 948 +2 lib_test_lmb_flags 2101 2103 +2 do_spi_flash 3150 3152 +2 do_bootz 526 528 +2 do_bootm_linux 2067 2069 +2 bootm_run_states 5275 5277 +2 _fs_read.lto_priv 331 333 +2 lmb_dump_region.lto_priv 356 353 -3 lmb_add 59 52 -7 efi_allocate_pages.part 303 249 -54 lmb_reserve_flags 65 - -65 _lmb_alloc_addr.lto_priv 92 - -92 _lmb_alloc_base.lto_priv 280 - -280 Total: Before=2492722, After=2492744, chg +0.00%
[0] https://lore.kernel.org/u-boot/20241208105223.2821049-1-ilias.apalodimas@lin...
Changes since v1: - Rebased on top of https://lore.kernel.org/u-boot/20241211021703.2333-1-semen.protsenko@linaro.... https://lore.kernel.org/u-boot/20241211022550.2995-1-semen.protsenko@linaro.... - Converted enum lmb_flags to a u32 - Removed the _flags from all the functions - Added a patch that removes lmb_align_down() - Picked up r-b tags. Tom I kept your on patch #3 since I only changed the name of the function and assumed you are ok with it.
Ilias Apalodimas (8): lmb: Remove lmb_align_down() lmb: Move enum lmb_flags to a u32 lmb: Remove lmb_reserve_flags() lmb: Rename free_mem to available_mem lmb: Remove lmb_add_region() lmb: Remove lmb_alloc_addr_flags() lmb: Remove lmb_alloc_base_flags() lmb: Rename _lmb_alloc_addr() to lmb_alloc_addr_flags()
arch/powerpc/cpu/mpc85xx/mp.c | 2 +- arch/powerpc/lib/misc.c | 2 +- boot/bootm.c | 3 +- boot/image-board.c | 20 +++-- boot/image-fdt.c | 15 ++-- cmd/booti.c | 2 +- cmd/bootz.c | 2 +- cmd/load.c | 2 +- fs/fs.c | 2 +- include/lmb.h | 58 ++++++--------- lib/efi_loader/efi_memory.c | 6 +- lib/lmb.c | 136 +++++++++++----------------------- test/cmd/bdinfo.c | 4 +- test/lib/lmb.c | 92 +++++++++++------------ 14 files changed, 145 insertions(+), 201 deletions(-)
-- 2.45.2
We already have a macro for this. Use it instead of adding yet another variant for alignment.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/lmb.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index f9880a8dc62b..b9c26cb02e10 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -342,11 +342,6 @@ static long lmb_overlaps_region(struct alist *lmb_rgn_lst, phys_addr_t base, return (i < lmb_rgn_lst->count) ? i : -1; }
-static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size) -{ - return addr & ~(size - 1); -} - /* * IOVA LMB memory maps using lmb pointers instead of the global LMB memory map. */ @@ -400,7 +395,7 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align)
if (lmbsize < size) continue; - base = lmb_align_down(lmbbase + lmbsize - size, align); + base = ALIGN_DOWN(lmbbase + lmbsize - size, align);
while (base && lmbbase <= base) { rgn = lmb_overlaps_region(&io_lmb->used_mem, base, size); @@ -416,7 +411,7 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align) res_base = lmb_used[rgn].base; if (res_base < size) break; - base = lmb_align_down(res_base - size, align); + base = ALIGN_DOWN(res_base - size, align); } } return 0; @@ -709,13 +704,13 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, continue;
if (max_addr == LMB_ALLOC_ANYWHERE) { - base = lmb_align_down(lmbbase + lmbsize - size, align); + base = ALIGN_DOWN(lmbbase + lmbsize - size, align); } else if (lmbbase < max_addr) { base = lmbbase + lmbsize; if (base < lmbbase) base = -1; base = min(base, max_addr); - base = lmb_align_down(base - size, align); + base = ALIGN_DOWN(base - size, align); } else { continue; } @@ -740,7 +735,7 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, res_base = lmb_used[rgn].base; if (res_base < size) break; - base = lmb_align_down(res_base - size, align); + base = ALIGN_DOWN(res_base - size, align); } } return 0;
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
We already have a macro for this. Use it instead of adding yet another variant for alignment.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
lib/lmb.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index f9880a8dc62b..b9c26cb02e10 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -342,11 +342,6 @@ static long lmb_overlaps_region(struct alist *lmb_rgn_lst, phys_addr_t base, return (i < lmb_rgn_lst->count) ? i : -1; }
-static phys_addr_t lmb_align_down(phys_addr_t addr, phys_size_t size) -{
return addr & ~(size - 1);-}
/*
- IOVA LMB memory maps using lmb pointers instead of the global LMB memory map.
*/ @@ -400,7 +395,7 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align)
if (lmbsize < size) continue;
base = lmb_align_down(lmbbase + lmbsize - size, align);
base = ALIGN_DOWN(lmbbase + lmbsize - size, align); while (base && lmbbase <= base) { rgn = lmb_overlaps_region(&io_lmb->used_mem, base, size);@@ -416,7 +411,7 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align) res_base = lmb_used[rgn].base; if (res_base < size) break;
base = lmb_align_down(res_base - size, align);
base = ALIGN_DOWN(res_base - size, align); } } return 0;@@ -709,13 +704,13 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, continue;
if (max_addr == LMB_ALLOC_ANYWHERE) {
base = lmb_align_down(lmbbase + lmbsize - size, align);
base = ALIGN_DOWN(lmbbase + lmbsize - size, align); } else if (lmbbase < max_addr) { base = lmbbase + lmbsize; if (base < lmbbase) base = -1; base = min(base, max_addr);
base = lmb_align_down(base - size, align);
base = ALIGN_DOWN(base - size, align); } else { continue; }@@ -740,7 +735,7 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, res_base = lmb_used[rgn].base; if (res_base < size) break;
base = lmb_align_down(res_base - size, align);
base = ALIGN_DOWN(res_base - size, align); } } return 0;-- 2.45.2
LMB flags is not an enum anymore. It's currently used as a bitmask in various places of our code. So make it a u32 which is more appropriate when dealing with masks.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- boot/image-fdt.c | 4 ++-- include/lmb.h | 25 +++++++++++-------------- lib/lmb.c | 18 +++++++++--------- test/cmd/bdinfo.c | 2 +- 4 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 73c43c30684f..cda7c3aa9e38 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -68,7 +68,7 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr) } #endif
-static void boot_fdt_reserve_region(u64 addr, u64 size, enum lmb_flags flags) +static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags) { long ret;
@@ -100,7 +100,7 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob) int i, total, ret; int nodeoffset, subnode; struct fdt_resource res; - enum lmb_flags flags; + u32 flags;
if (fdt_check_header(fdt_blob) != 0) return; diff --git a/include/lmb.h b/include/lmb.h index 03d5fac6aa79..7db9e0b62840 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -19,18 +19,15 @@ #define LMB_ALIST_INITIAL_SIZE 4
/** - * enum lmb_flags - Definition of memory region attributes - * @LMB_NONE: No special request - * @LMB_NOMAP: Don't add to MMU configuration - * @LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved - * @LMB_NONOTIFY: Do not notify other modules of changes to this memory region - */ -enum lmb_flags { - LMB_NONE = 0, - LMB_NOMAP = BIT(1), - LMB_NOOVERWRITE = BIT(2), - LMB_NONOTIFY = BIT(3), -}; + * LMB_NONE: No special request + * LMB_NOMAP: Don't add to MMU configuration + * LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved + * LMB_NONOTIFY: Do not notify other modules of changes to this memory region + */ +#define LMB_NONE 0 +#define LMB_NOMAP BIT(0) +#define LMB_NOOVERWRITE BIT(1) +#define LMB_NONOTIFY BIT(2)
/** * struct lmb_region - Description of one region @@ -41,7 +38,7 @@ enum lmb_flags { struct lmb_region { phys_addr_t base; phys_size_t size; - enum lmb_flags flags; + u32 flags; };
/** @@ -101,7 +98,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size); * * %-1 - Failure */ long lmb_reserve_flags(phys_addr_t base, phys_size_t size, - enum lmb_flags flags); + u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); diff --git a/lib/lmb.c b/lib/lmb.c index b9c26cb02e10..edecdb8e9cbf 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -188,7 +188,7 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst, * * %-1 - Failure */ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, - phys_size_t size, enum lmb_flags flags) + phys_size_t size, u32 flags) { unsigned long coalesced = 0; long ret, i; @@ -201,7 +201,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, for (i = 0; i < lmb_rgn_lst->count; i++) { phys_addr_t rgnbase = rgn[i].base; phys_size_t rgnsize = rgn[i].size; - enum lmb_flags rgnflags = rgn[i].flags; + u32 rgnflags = rgn[i].flags;
ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (ret > 0) { @@ -430,14 +430,14 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
static struct lmb lmb;
-static bool lmb_should_notify(enum lmb_flags flags) +static bool lmb_should_notify(u32 flags) { return !lmb.test && !(flags & LMB_NONOTIFY) && CONFIG_IS_ENABLED(EFI_LOADER); }
static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, - enum lmb_flags flags) + u32 flags) { u64 efi_addr; u64 pages; @@ -470,7 +470,7 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, return 0; }
-static void lmb_print_region_flags(enum lmb_flags flags) +static void lmb_print_region_flags(u32 flags) { const char * const flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; @@ -495,7 +495,7 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end; - enum lmb_flags flags; + u32 flags; int i;
printf(" %s.count = %#x\n", name, lmb_rgn_lst->count); @@ -669,7 +669,7 @@ long lmb_free(phys_addr_t base, phys_size_t size) return lmb_free_flags(base, size, LMB_NONE); }
-long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags) +long lmb_reserve_flags(phys_addr_t base, phys_size_t size, u32 flags) { long ret = 0; struct alist *lmb_rgn_lst = &lmb.used_mem; @@ -687,7 +687,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size) }
static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, - phys_addr_t max_addr, enum lmb_flags flags) + phys_addr_t max_addr, u32 flags) { int ret; long i, rgn; @@ -774,7 +774,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, }
static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, - enum lmb_flags flags) + u32 flags) { long rgn; struct lmb_region *lmb_memory = lmb.free_mem.data; diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c index bb419ab2394e..014391b38ac0 100644 --- a/test/cmd/bdinfo.c +++ b/test/cmd/bdinfo.c @@ -104,7 +104,7 @@ static int lmb_test_dump_region(struct unit_test_state *uts, { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end; - enum lmb_flags flags; + u32 flags; int i;
ut_assert_nextline(" %s.count = %#x", name, lmb_rgn_lst->count); -- 2.45.2
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
LMB flags is not an enum anymore. It's currently used as a bitmask in various places of our code. So make it a u32 which is more appropriate when dealing with masks.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
boot/image-fdt.c | 4 ++-- include/lmb.h | 25 +++++++++++-------------- lib/lmb.c | 18 +++++++++--------- test/cmd/bdinfo.c | 2 +- 4 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 73c43c30684f..cda7c3aa9e38 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -68,7 +68,7 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr) } #endif
-static void boot_fdt_reserve_region(u64 addr, u64 size, enum lmb_flags flags) +static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags) { long ret;
@@ -100,7 +100,7 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob) int i, total, ret; int nodeoffset, subnode; struct fdt_resource res;
enum lmb_flags flags;
u32 flags; if (fdt_check_header(fdt_blob) != 0) return;diff --git a/include/lmb.h b/include/lmb.h index 03d5fac6aa79..7db9e0b62840 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -19,18 +19,15 @@ #define LMB_ALIST_INITIAL_SIZE 4
/**
- enum lmb_flags - Definition of memory region attributes
- @LMB_NONE: No special request
- @LMB_NOMAP: Don't add to MMU configuration
- @LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
- @LMB_NONOTIFY: Do not notify other modules of changes to this memory region
- */
-enum lmb_flags {
LMB_NONE = 0,LMB_NOMAP = BIT(1),LMB_NOOVERWRITE = BIT(2),LMB_NONOTIFY = BIT(3),-};
- LMB_NONE: No special request
- LMB_NOMAP: Don't add to MMU configuration
- LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
- LMB_NONOTIFY: Do not notify other modules of changes to this memory region
- */
This causes a kernel-doc warning:
$ scripts/kernel-doc -v -none include/lmb.h
include/lmb.h:22: warning: missing initial short description on line: * LMB_NONE: No special request
Suggest using DOC stanza in this kernel-doc comment, as described at [1], like this:
/** * DOC: Memory region attribute flags. * * %LMB_NONE: No special request * %LMB_NOMAP: Don't add to MMU configuration * %LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved * %LMB_NONOTIFY: Do not notify other modules of changes to this memory region */
That's helpful if you want to keep Sphinx generated doc [2] for LMB subsystem tidy.
Other than that, LGTM:
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
[1] https://docs.kernel.org/doc-guide/kernel-doc.html [2] https://docs.u-boot.org/en/latest/api/lmb.html
*/
+#define LMB_NONE 0 +#define LMB_NOMAP BIT(0) +#define LMB_NOOVERWRITE BIT(1) +#define LMB_NONOTIFY BIT(2)
/**
- struct lmb_region - Description of one region
@@ -41,7 +38,7 @@ enum lmb_flags { struct lmb_region { phys_addr_t base; phys_size_t size;
enum lmb_flags flags;
u32 flags;};
/** @@ -101,7 +98,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size);
- %-1 - Failure
*/ long lmb_reserve_flags(phys_addr_t base, phys_size_t size,
enum lmb_flags flags);
u32 flags);phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); diff --git a/lib/lmb.c b/lib/lmb.c index b9c26cb02e10..edecdb8e9cbf 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -188,7 +188,7 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst,
- %-1 - Failure
*/ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
phys_size_t size, enum lmb_flags flags)
phys_size_t size, u32 flags){ unsigned long coalesced = 0; long ret, i; @@ -201,7 +201,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, for (i = 0; i < lmb_rgn_lst->count; i++) { phys_addr_t rgnbase = rgn[i].base; phys_size_t rgnsize = rgn[i].size;
enum lmb_flags rgnflags = rgn[i].flags;
u32 rgnflags = rgn[i].flags; ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (ret > 0) {@@ -430,14 +430,14 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
static struct lmb lmb;
-static bool lmb_should_notify(enum lmb_flags flags) +static bool lmb_should_notify(u32 flags) { return !lmb.test && !(flags & LMB_NONOTIFY) && CONFIG_IS_ENABLED(EFI_LOADER); }
static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
enum lmb_flags flags)
u32 flags){ u64 efi_addr; u64 pages; @@ -470,7 +470,7 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, return 0; }
-static void lmb_print_region_flags(enum lmb_flags flags) +static void lmb_print_region_flags(u32 flags) { const char * const flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; @@ -495,7 +495,7 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end;
enum lmb_flags flags;
u32 flags; int i; printf(" %s.count = %#x\n", name, lmb_rgn_lst->count);@@ -669,7 +669,7 @@ long lmb_free(phys_addr_t base, phys_size_t size) return lmb_free_flags(base, size, LMB_NONE); }
-long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags) +long lmb_reserve_flags(phys_addr_t base, phys_size_t size, u32 flags) { long ret = 0; struct alist *lmb_rgn_lst = &lmb.used_mem; @@ -687,7 +687,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size) }
static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
phys_addr_t max_addr, enum lmb_flags flags)
phys_addr_t max_addr, u32 flags){ int ret; long i, rgn; @@ -774,7 +774,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, }
static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
enum lmb_flags flags)
u32 flags){ long rgn; struct lmb_region *lmb_memory = lmb.free_mem.data; diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c index bb419ab2394e..014391b38ac0 100644 --- a/test/cmd/bdinfo.c +++ b/test/cmd/bdinfo.c @@ -104,7 +104,7 @@ static int lmb_test_dump_region(struct unit_test_state *uts, { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end;
enum lmb_flags flags;
u32 flags; int i; ut_assert_nextline(" %s.count = %#x", name, lmb_rgn_lst->count);-- 2.45.2
Hi Sam,
On Thu, 12 Dec 2024 at 23:58, Sam Protsenko semen.protsenko@linaro.org wrote:
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
LMB flags is not an enum anymore. It's currently used as a bitmask in various places of our code. So make it a u32 which is more appropriate when dealing with masks.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
boot/image-fdt.c | 4 ++-- include/lmb.h | 25 +++++++++++-------------- lib/lmb.c | 18 +++++++++--------- test/cmd/bdinfo.c | 2 +- 4 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 73c43c30684f..cda7c3aa9e38 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -68,7 +68,7 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr) } #endif
-static void boot_fdt_reserve_region(u64 addr, u64 size, enum lmb_flags flags) +static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags) { long ret;
@@ -100,7 +100,7 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob) int i, total, ret; int nodeoffset, subnode; struct fdt_resource res;
enum lmb_flags flags;
u32 flags; if (fdt_check_header(fdt_blob) != 0) return;diff --git a/include/lmb.h b/include/lmb.h index 03d5fac6aa79..7db9e0b62840 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -19,18 +19,15 @@ #define LMB_ALIST_INITIAL_SIZE 4
/**
- enum lmb_flags - Definition of memory region attributes
- @LMB_NONE: No special request
- @LMB_NOMAP: Don't add to MMU configuration
- @LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
- @LMB_NONOTIFY: Do not notify other modules of changes to this memory region
- */
-enum lmb_flags {
LMB_NONE = 0,LMB_NOMAP = BIT(1),LMB_NOOVERWRITE = BIT(2),LMB_NONOTIFY = BIT(3),-};
- LMB_NONE: No special request
- LMB_NOMAP: Don't add to MMU configuration
- LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
- LMB_NONOTIFY: Do not notify other modules of changes to this memory region
- */
This causes a kernel-doc warning:
$ scripts/kernel-doc -v -none include/lmb.h
include/lmb.h:22: warning: missing initial short description on line: * LMB_NONE: No special requestSuggest using DOC stanza in this kernel-doc comment, as described at [1], like this:
/**
- DOC: Memory region attribute flags.
- %LMB_NONE: No special request
- %LMB_NOMAP: Don't add to MMU configuration
- %LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
- %LMB_NONOTIFY: Do not notify other modules of changes to this memory region
*/
That's helpful if you want to keep Sphinx generated doc [2] for LMB subsystem tidy.
Other than that, LGTM:
Sure, the CI didn't complain so I assumed it was fine. I'll change this in v2
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
Thanks /Ilias
[1] https://docs.kernel.org/doc-guide/kernel-doc.html [2] https://docs.u-boot.org/en/latest/api/lmb.html
*/
+#define LMB_NONE 0 +#define LMB_NOMAP BIT(0) +#define LMB_NOOVERWRITE BIT(1) +#define LMB_NONOTIFY BIT(2)
/**
- struct lmb_region - Description of one region
@@ -41,7 +38,7 @@ enum lmb_flags { struct lmb_region { phys_addr_t base; phys_size_t size;
enum lmb_flags flags;
u32 flags;};
/** @@ -101,7 +98,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size);
- %-1 - Failure
*/ long lmb_reserve_flags(phys_addr_t base, phys_size_t size,
enum lmb_flags flags);
u32 flags);phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); diff --git a/lib/lmb.c b/lib/lmb.c index b9c26cb02e10..edecdb8e9cbf 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -188,7 +188,7 @@ static long lmb_resize_regions(struct alist *lmb_rgn_lst,
- %-1 - Failure
*/ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
phys_size_t size, enum lmb_flags flags)
phys_size_t size, u32 flags){ unsigned long coalesced = 0; long ret, i; @@ -201,7 +201,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base, for (i = 0; i < lmb_rgn_lst->count; i++) { phys_addr_t rgnbase = rgn[i].base; phys_size_t rgnsize = rgn[i].size;
enum lmb_flags rgnflags = rgn[i].flags;
u32 rgnflags = rgn[i].flags; ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (ret > 0) {@@ -430,14 +430,14 @@ long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size)
static struct lmb lmb;
-static bool lmb_should_notify(enum lmb_flags flags) +static bool lmb_should_notify(u32 flags) { return !lmb.test && !(flags & LMB_NONOTIFY) && CONFIG_IS_ENABLED(EFI_LOADER); }
static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
enum lmb_flags flags)
u32 flags){ u64 efi_addr; u64 pages; @@ -470,7 +470,7 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, return 0; }
-static void lmb_print_region_flags(enum lmb_flags flags) +static void lmb_print_region_flags(u32 flags) { const char * const flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; @@ -495,7 +495,7 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end;
enum lmb_flags flags;
u32 flags; int i; printf(" %s.count = %#x\n", name, lmb_rgn_lst->count);@@ -669,7 +669,7 @@ long lmb_free(phys_addr_t base, phys_size_t size) return lmb_free_flags(base, size, LMB_NONE); }
-long lmb_reserve_flags(phys_addr_t base, phys_size_t size, enum lmb_flags flags) +long lmb_reserve_flags(phys_addr_t base, phys_size_t size, u32 flags) { long ret = 0; struct alist *lmb_rgn_lst = &lmb.used_mem; @@ -687,7 +687,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size) }
static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
phys_addr_t max_addr, enum lmb_flags flags)
phys_addr_t max_addr, u32 flags){ int ret; long i, rgn; @@ -774,7 +774,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, }
static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
enum lmb_flags flags)
u32 flags){ long rgn; struct lmb_region *lmb_memory = lmb.free_mem.data; diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c index bb419ab2394e..014391b38ac0 100644 --- a/test/cmd/bdinfo.c +++ b/test/cmd/bdinfo.c @@ -104,7 +104,7 @@ static int lmb_test_dump_region(struct unit_test_state *uts, { struct lmb_region *rgn = lmb_rgn_lst->data; unsigned long long base, size, end;
enum lmb_flags flags;
u32 flags; int i; ut_assert_nextline(" %s.count = %#x", name, lmb_rgn_lst->count);-- 2.45.2
lmb_reserve() is just calling lmb_reserve_flags() with LMB_NONE. There's not much we gain from this abstraction. So let's remove the latter, add the flags argument to lmb_reserve() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Tom Rini trini@konsulko.com --- arch/powerpc/cpu/mpc85xx/mp.c | 2 +- arch/powerpc/lib/misc.c | 2 +- boot/bootm.c | 3 ++- boot/image-board.c | 2 +- boot/image-fdt.c | 6 ++--- cmd/booti.c | 2 +- cmd/bootz.c | 2 +- cmd/load.c | 2 +- include/lmb.h | 14 ++-------- lib/lmb.c | 28 ++++++++------------ test/lib/lmb.c | 48 +++++++++++++++++------------------ 11 files changed, 48 insertions(+), 63 deletions(-)
diff --git a/arch/powerpc/cpu/mpc85xx/mp.c b/arch/powerpc/cpu/mpc85xx/mp.c index bed465cb2cba..8918a401fac3 100644 --- a/arch/powerpc/cpu/mpc85xx/mp.c +++ b/arch/powerpc/cpu/mpc85xx/mp.c @@ -412,7 +412,7 @@ void cpu_mp_lmb_reserve(void) { u32 bootpg = determine_mp_bootpg(NULL);
- lmb_reserve(bootpg, 4096); + lmb_reserve(bootpg, 4096, LMB_NONE); }
void setup_mp(void) diff --git a/arch/powerpc/lib/misc.c b/arch/powerpc/lib/misc.c index 4cd23b3406d1..7e303419624a 100644 --- a/arch/powerpc/lib/misc.c +++ b/arch/powerpc/lib/misc.c @@ -40,7 +40,7 @@ int arch_misc_init(void)
printf("WARNING: adjusting available memory from 0x%lx to 0x%llx\n", size, (unsigned long long)bootm_size); - lmb_reserve(base, bootm_size - size); + lmb_reserve(base, bootm_size - size, LMB_NONE); }
#ifdef CONFIG_MP diff --git a/boot/bootm.c b/boot/bootm.c index 16a43d519a8a..854ac7ec7388 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -696,7 +696,8 @@ static int bootm_load_os(struct bootm_headers *images, int boot_progress) }
if (CONFIG_IS_ENABLED(LMB)) - lmb_reserve(images->os.load, (load_end - images->os.load)); + lmb_reserve(images->os.load, (load_end - images->os.load), + LMB_NONE);
return 0; } diff --git a/boot/image-board.c b/boot/image-board.c index b726bd6b3035..070ada007185 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -562,7 +562,7 @@ int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start, debug(" in-place initrd\n"); *initrd_start = rd_data; *initrd_end = rd_data + rd_len; - lmb_reserve(rd_data, rd_len); + lmb_reserve(rd_data, rd_len, LMB_NONE); } else { if (initrd_high) *initrd_start = (ulong)lmb_alloc_base(rd_len, diff --git a/boot/image-fdt.c b/boot/image-fdt.c index cda7c3aa9e38..d717f6690729 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -72,7 +72,7 @@ static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags) { long ret;
- ret = lmb_reserve_flags(addr, size, flags); + ret = lmb_reserve(addr, size, flags); if (!ret) { debug(" reserving fdt memory region: addr=%llx size=%llx flags=%x\n", (unsigned long long)addr, @@ -184,7 +184,7 @@ int boot_relocate_fdt(char **of_flat_tree, ulong *of_size) if (desired_addr == ~0UL) { /* All ones means use fdt in place */ of_start = fdt_blob; - lmb_reserve(map_to_sysmem(of_start), of_len); + lmb_reserve(map_to_sysmem(of_start), of_len, LMB_NONE); disable_relocation = 1; } else if (desired_addr) { addr = lmb_alloc_base(of_len, 0x1000, desired_addr); @@ -675,7 +675,7 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
/* Create a new LMB reservation */ if (CONFIG_IS_ENABLED(LMB) && lmb) - lmb_reserve(map_to_sysmem(blob), of_size); + lmb_reserve(map_to_sysmem(blob), of_size, LMB_NONE);
#if defined(CONFIG_ARCH_KEYSTONE) if (IS_ENABLED(CONFIG_OF_BOARD_SETUP)) diff --git a/cmd/booti.c b/cmd/booti.c index 43e79e87201b..1a57fe913977 100644 --- a/cmd/booti.c +++ b/cmd/booti.c @@ -87,7 +87,7 @@ static int booti_start(struct bootm_info *bmi) images->os.start = relocated_addr; images->os.end = relocated_addr + image_size;
- lmb_reserve(images->ep, le32_to_cpu(image_size)); + lmb_reserve(images->ep, le32_to_cpu(image_size), LMB_NONE);
/* * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not diff --git a/cmd/bootz.c b/cmd/bootz.c index 787203f5bd70..99318ff213f4 100644 --- a/cmd/bootz.c +++ b/cmd/bootz.c @@ -56,7 +56,7 @@ static int bootz_start(struct cmd_tbl *cmdtp, int flag, int argc, if (ret != 0) return 1;
- lmb_reserve(images->ep, zi_end - zi_start); + lmb_reserve(images->ep, zi_end - zi_start, LMB_NONE);
/* * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not diff --git a/cmd/load.c b/cmd/load.c index 20d802502ae6..899bb4f598e2 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -179,7 +179,7 @@ static ulong load_serial(long offset) { void *dst;
- ret = lmb_reserve(store_addr, binlen); + ret = lmb_reserve(store_addr, binlen, LMB_NONE); if (ret) { printf("\nCannot overwrite reserved area (%08lx..%08lx)\n", store_addr, store_addr + binlen); diff --git a/include/lmb.h b/include/lmb.h index 7db9e0b62840..9539cb67c3d7 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -78,16 +78,7 @@ void lmb_add_memory(void); long lmb_add(phys_addr_t base, phys_size_t size);
/** - * lmb_reserve() - Reserve a memory region (with no special flags) - * @base: Base address of the memory region - * @size: Size of the memory region - * - * Return: 0 on success, negative error code on failure. - */ -long lmb_reserve(phys_addr_t base, phys_size_t size); - -/** - * lmb_reserve_flags() - Reserve one region with a specific flags bitfield + * lmb_reserve() - Reserve one region with a specific flags bitfield * @base: Base address of the memory region * @size: Size of the memory region * @flags: Flags for the memory region @@ -97,8 +88,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size); * * %-EEXIST - The region is already added, and flags != LMB_NONE * * %-1 - Failure */ -long lmb_reserve_flags(phys_addr_t base, phys_size_t size, - u32 flags); +long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); diff --git a/lib/lmb.c b/lib/lmb.c index edecdb8e9cbf..fd0e91981ad4 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -553,12 +553,11 @@ static void lmb_reserve_uboot_region(void) if (bank_end > end) bank_end = end - 1;
- lmb_reserve_flags(rsv_start, bank_end - rsv_start + 1, - LMB_NOOVERWRITE); + lmb_reserve(rsv_start, bank_end - rsv_start + 1, LMB_NOOVERWRITE);
if (gd->flags & GD_FLG_SKIP_RELOC) - lmb_reserve_flags((phys_addr_t)(uintptr_t)_start, - gd->mon_len, LMB_NOOVERWRITE); + lmb_reserve((phys_addr_t)(uintptr_t)_start, + gd->mon_len, LMB_NOOVERWRITE);
break; } @@ -584,7 +583,7 @@ static __maybe_unused void lmb_reserve_common_spl(void) if (IS_ENABLED(CONFIG_SPL_STACK_R_ADDR)) { rsv_start = gd->start_addr_sp - 16384; rsv_size = 16384; - lmb_reserve_flags(rsv_start, rsv_size, LMB_NOOVERWRITE); + lmb_reserve(rsv_start, rsv_size, LMB_NOOVERWRITE); }
if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS)) { @@ -592,7 +591,7 @@ static __maybe_unused void lmb_reserve_common_spl(void) rsv_start = (phys_addr_t)(uintptr_t)__bss_start; rsv_size = (phys_addr_t)(uintptr_t)__bss_end - (phys_addr_t)(uintptr_t)__bss_start; - lmb_reserve_flags(rsv_start, rsv_size, LMB_NOOVERWRITE); + lmb_reserve(rsv_start, rsv_size, LMB_NOOVERWRITE); } }
@@ -624,11 +623,11 @@ void lmb_add_memory(void) * allocated */ if (bd->bi_dram[i].start >= ram_top) - lmb_reserve_flags(bd->bi_dram[i].start, size, - LMB_NOOVERWRITE); + lmb_reserve(bd->bi_dram[i].start, size, + LMB_NOOVERWRITE); else if (bank_end > ram_top) - lmb_reserve_flags(ram_top, bank_end - ram_top, - LMB_NOOVERWRITE); + lmb_reserve(ram_top, bank_end - ram_top, + LMB_NOOVERWRITE); } } } @@ -669,7 +668,7 @@ long lmb_free(phys_addr_t base, phys_size_t size) return lmb_free_flags(base, size, LMB_NONE); }
-long lmb_reserve_flags(phys_addr_t base, phys_size_t size, u32 flags) +long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags) { long ret = 0; struct alist *lmb_rgn_lst = &lmb.used_mem; @@ -681,11 +680,6 @@ long lmb_reserve_flags(phys_addr_t base, phys_size_t size, u32 flags) return lmb_map_update_notify(base, size, MAP_OP_RESERVE, flags); }
-long lmb_reserve(phys_addr_t base, phys_size_t size) -{ - return lmb_reserve_flags(base, size, LMB_NONE); -} - static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, u32 flags) { @@ -790,7 +784,7 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, lmb_memory[rgn].size, base + size - 1, 1)) { /* ok, reserve the memory */ - if (!lmb_reserve_flags(base, size, flags)) + if (!lmb_reserve(base, size, flags)) return base; } } diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 48c3c966f8f2..3c3e862ffa0e 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -117,7 +117,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, }
/* reserve 64KiB somewhere */ - ret = lmb_reserve(alloc_64k_addr, 0x10000); + ret = lmb_reserve(alloc_64k_addr, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, 0, 0, 1, alloc_64k_addr, 0x10000, 0, 0, 0, 0); @@ -264,7 +264,7 @@ static int test_bigblock(struct unit_test_state *uts, const phys_addr_t ram) ut_asserteq(ret, 0);
/* reserve 64KiB in the middle of RAM */ - ret = lmb_reserve(alloc_64k_addr, 0x10000); + ret = lmb_reserve(alloc_64k_addr, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, alloc_64k_addr, 0x10000, 0, 0, 0, 0); @@ -466,35 +466,35 @@ static int lib_test_lmb_overlapping_reserve(struct unit_test_state *uts) ret = lmb_add(ram, ram_size); ut_asserteq(ret, 0);
- ret = lmb_reserve(0x40010000, 0x10000); + ret = lmb_reserve(0x40010000, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000, 0, 0, 0, 0);
/* allocate overlapping region should return the coalesced count */ - ret = lmb_reserve(0x40011000, 0x10000); + ret = lmb_reserve(0x40011000, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x11000, 0, 0, 0, 0); /* allocate 3nd region */ - ret = lmb_reserve(0x40030000, 0x10000); + ret = lmb_reserve(0x40030000, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40010000, 0x11000, 0x40030000, 0x10000, 0, 0); /* allocate 2nd region , This should coalesced all region into one */ - ret = lmb_reserve(0x40020000, 0x10000); + ret = lmb_reserve(0x40020000, 0x10000, LMB_NONE); ut_assert(ret >= 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x30000, 0, 0, 0, 0);
/* allocate 2nd region, which should be added as first region */ - ret = lmb_reserve(0x40000000, 0x8000); + ret = lmb_reserve(0x40000000, 0x8000, LMB_NONE); ut_assert(ret >= 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x8000, 0x40010000, 0x30000, 0, 0);
/* allocate 3rd region, coalesce with first and overlap with second */ - ret = lmb_reserve(0x40008000, 0x10000); + ret = lmb_reserve(0x40008000, 0x10000, LMB_NONE); ut_assert(ret >= 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40000000, 0x40000, 0, 0, 0, 0); @@ -550,11 +550,11 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ut_asserteq(ret, 0);
/* reserve 3 blocks */ - ret = lmb_reserve(alloc_addr_a, 0x10000); + ret = lmb_reserve(alloc_addr_a, 0x10000, LMB_NONE); ut_asserteq(ret, 0); - ret = lmb_reserve(alloc_addr_b, 0x10000); + ret = lmb_reserve(alloc_addr_b, 0x10000, LMB_NONE); ut_asserteq(ret, 0); - ret = lmb_reserve(alloc_addr_c, 0x10000); + ret = lmb_reserve(alloc_addr_c, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, alloc_addr_a, 0x10000, alloc_addr_b, 0x10000, alloc_addr_c, 0x10000); @@ -680,11 +680,11 @@ static int test_get_unreserved_size(struct unit_test_state *uts, ut_asserteq(ret, 0);
/* reserve 3 blocks */ - ret = lmb_reserve(alloc_addr_a, 0x10000); + ret = lmb_reserve(alloc_addr_a, 0x10000, LMB_NONE); ut_asserteq(ret, 0); - ret = lmb_reserve(alloc_addr_b, 0x10000); + ret = lmb_reserve(alloc_addr_b, 0x10000, LMB_NONE); ut_asserteq(ret, 0); - ret = lmb_reserve(alloc_addr_c, 0x10000); + ret = lmb_reserve(alloc_addr_c, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, alloc_addr_a, 0x10000, alloc_addr_b, 0x10000, alloc_addr_c, 0x10000); @@ -747,19 +747,19 @@ static int lib_test_lmb_flags(struct unit_test_state *uts) ut_asserteq(ret, 0);
/* reserve, same flag */ - ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40010000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000, 0, 0, 0, 0);
/* reserve again, same flag */ - ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40010000, 0x10000, LMB_NOMAP); ut_asserteq(ret, -EEXIST); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000, 0, 0, 0, 0);
/* reserve again, new flag */ - ret = lmb_reserve_flags(0x40010000, 0x10000, LMB_NONE); + ret = lmb_reserve(0x40010000, 0x10000, LMB_NONE); ut_asserteq(ret, -1); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x10000, 0, 0, 0, 0); @@ -767,20 +767,20 @@ static int lib_test_lmb_flags(struct unit_test_state *uts) ut_asserteq(lmb_is_nomap(&used[0]), 1);
/* merge after */ - ret = lmb_reserve_flags(0x40020000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40020000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40010000, 0x20000, 0, 0, 0, 0);
/* merge before */ - ret = lmb_reserve_flags(0x40000000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40000000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, 0x40000000, 0x30000, 0, 0, 0, 0);
ut_asserteq(lmb_is_nomap(&used[0]), 1);
- ret = lmb_reserve_flags(0x40030000, 0x10000, LMB_NONE); + ret = lmb_reserve(0x40030000, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x30000, 0x40030000, 0x10000, 0, 0); @@ -789,7 +789,7 @@ static int lib_test_lmb_flags(struct unit_test_state *uts) ut_asserteq(lmb_is_nomap(&used[1]), 0);
/* test that old API use LMB_NONE */ - ret = lmb_reserve(0x40040000, 0x10000); + ret = lmb_reserve(0x40040000, 0x10000, LMB_NONE); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, 0x40000000, 0x30000, 0x40030000, 0x20000, 0, 0); @@ -797,18 +797,18 @@ static int lib_test_lmb_flags(struct unit_test_state *uts) ut_asserteq(lmb_is_nomap(&used[0]), 1); ut_asserteq(lmb_is_nomap(&used[1]), 0);
- ret = lmb_reserve_flags(0x40070000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40070000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, 0x40000000, 0x30000, 0x40030000, 0x20000, 0x40070000, 0x10000);
- ret = lmb_reserve_flags(0x40050000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40050000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 4, 0x40000000, 0x30000, 0x40030000, 0x20000, 0x40050000, 0x10000);
/* merge with 2 adjacent regions */ - ret = lmb_reserve_flags(0x40060000, 0x10000, LMB_NOMAP); + ret = lmb_reserve(0x40060000, 0x10000, LMB_NOMAP); ut_asserteq(ret, 0); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, 0x40000000, 0x30000, 0x40030000, 0x20000, 0x40050000, 0x30000);
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_reserve() is just calling lmb_reserve_flags() with LMB_NONE. There's not much we gain from this abstraction. So let's remove the latter, add the flags argument to lmb_reserve() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Tom Rini trini@konsulko.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
[snip]
free_mem is a misnomer. We never update it with the free memory for LMB. Instead, it describes all available memory and is checked against used_mem to decide whether an area is free or not.
So let's rename this field to better match its usage.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- include/lmb.h | 4 ++-- lib/lmb.c | 34 +++++++++++++++++----------------- test/cmd/bdinfo.c | 2 +- test/lib/lmb.c | 2 +- 4 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 9539cb67c3d7..3db35992df8d 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -43,12 +43,12 @@ struct lmb_region {
/** * struct lmb - The LMB structure - * @free_mem: List of free memory regions + * @available_mem: List of memory available to LMB * @used_mem: List of used/reserved memory regions * @test: Is structure being used for LMB tests */ struct lmb { - struct alist free_mem; + struct alist available_mem; struct alist used_mem; bool test; }; diff --git a/lib/lmb.c b/lib/lmb.c index fd0e91981ad4..da960e422ada 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -350,7 +350,7 @@ int io_lmb_setup(struct lmb *io_lmb) { int ret;
- ret = alist_init(&io_lmb->free_mem, sizeof(struct lmb_region), + ret = alist_init(&io_lmb->available_mem, sizeof(struct lmb_region), (uint)LMB_ALIST_INITIAL_SIZE); if (!ret) { log_debug("Unable to initialise the list for LMB free IOVA\n"); @@ -371,13 +371,13 @@ int io_lmb_setup(struct lmb *io_lmb)
void io_lmb_teardown(struct lmb *io_lmb) { - alist_uninit(&io_lmb->free_mem); + alist_uninit(&io_lmb->available_mem); alist_uninit(&io_lmb->used_mem); }
long io_lmb_add(struct lmb *io_lmb, phys_addr_t base, phys_size_t size) { - return lmb_add_region_flags(&io_lmb->free_mem, base, size, LMB_NONE); + return lmb_add_region_flags(&io_lmb->available_mem, base, size, LMB_NONE); }
/* derived and simplified from _lmb_alloc_base() */ @@ -387,9 +387,9 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align) phys_addr_t base = 0; phys_addr_t res_base; struct lmb_region *lmb_used = io_lmb->used_mem.data; - struct lmb_region *lmb_memory = io_lmb->free_mem.data; + struct lmb_region *lmb_memory = io_lmb->available_mem.data;
- for (i = io_lmb->free_mem.count - 1; i >= 0; i--) { + for (i = io_lmb->available_mem.count - 1; i >= 0; i--) { phys_addr_t lmbbase = lmb_memory[i].base; phys_size_t lmbsize = lmb_memory[i].size;
@@ -515,7 +515,7 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name) void lmb_dump_all_force(void) { printf("lmb_dump_all:\n"); - lmb_dump_region(&lmb.free_mem, "memory"); + lmb_dump_region(&lmb.available_mem, "memory"); lmb_dump_region(&lmb.used_mem, "reserved"); }
@@ -642,7 +642,7 @@ static long lmb_add_region(struct alist *lmb_rgn_lst, phys_addr_t base, long lmb_add(phys_addr_t base, phys_size_t size) { long ret; - struct alist *lmb_rgn_lst = &lmb.free_mem; + struct alist *lmb_rgn_lst = &lmb.available_mem;
ret = lmb_add_region(lmb_rgn_lst, base, size); if (ret) @@ -688,9 +688,9 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t base = 0; phys_addr_t res_base; struct lmb_region *lmb_used = lmb.used_mem.data; - struct lmb_region *lmb_memory = lmb.free_mem.data; + struct lmb_region *lmb_memory = lmb.available_mem.data;
- for (i = lmb.free_mem.count - 1; i >= 0; i--) { + for (i = lmb.available_mem.count - 1; i >= 0; i--) { phys_addr_t lmbbase = lmb_memory[i].base; phys_size_t lmbsize = lmb_memory[i].size;
@@ -771,10 +771,10 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) { long rgn; - struct lmb_region *lmb_memory = lmb.free_mem.data; + struct lmb_region *lmb_memory = lmb.available_mem.data;
/* Check if the requested address is in one of the memory regions */ - rgn = lmb_overlaps_region(&lmb.free_mem, base, size); + rgn = lmb_overlaps_region(&lmb.available_mem, base, size); if (rgn >= 0) { /* * Check if the requested end address is in the same memory @@ -813,10 +813,10 @@ phys_size_t lmb_get_free_size(phys_addr_t addr) int i; long rgn; struct lmb_region *lmb_used = lmb.used_mem.data; - struct lmb_region *lmb_memory = lmb.free_mem.data; + struct lmb_region *lmb_memory = lmb.available_mem.data;
/* check if the requested address is in the memory regions */ - rgn = lmb_overlaps_region(&lmb.free_mem, addr, 1); + rgn = lmb_overlaps_region(&lmb.available_mem, addr, 1); if (rgn >= 0) { for (i = 0; i < lmb.used_mem.count; i++) { if (addr < lmb_used[i].base) { @@ -830,8 +830,8 @@ phys_size_t lmb_get_free_size(phys_addr_t addr) } } /* if we come here: no reserved ranges above requested addr */ - return lmb_memory[lmb.free_mem.count - 1].base + - lmb_memory[lmb.free_mem.count - 1].size - addr; + return lmb_memory[lmb.available_mem.count - 1].base + + lmb_memory[lmb.available_mem.count - 1].size - addr; } return 0; } @@ -854,7 +854,7 @@ static int lmb_setup(bool test) { bool ret;
- ret = alist_init(&lmb.free_mem, sizeof(struct lmb_region), + ret = alist_init(&lmb.available_mem, sizeof(struct lmb_region), (uint)LMB_ALIST_INITIAL_SIZE); if (!ret) { log_debug("Unable to initialise the list for LMB free memory\n"); @@ -914,7 +914,7 @@ int lmb_push(struct lmb *store)
void lmb_pop(struct lmb *store) { - alist_uninit(&lmb.free_mem); + alist_uninit(&lmb.available_mem); alist_uninit(&lmb.used_mem); lmb = *store; } diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c index 014391b38ac0..764294857082 100644 --- a/test/cmd/bdinfo.c +++ b/test/cmd/bdinfo.c @@ -131,7 +131,7 @@ static int lmb_test_dump_all(struct unit_test_state *uts) struct lmb *lmb = lmb_get();
ut_assert_nextline("lmb_dump_all:"); - ut_assertok(lmb_test_dump_region(uts, &lmb->free_mem, "memory")); + ut_assertok(lmb_test_dump_region(uts, &lmb->available_mem, "memory")); ut_assertok(lmb_test_dump_region(uts, &lmb->used_mem, "reserved"));
return 0; diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 3c3e862ffa0e..6e870274fedb 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -65,7 +65,7 @@ static int setup_lmb_test(struct unit_test_state *uts, struct lmb *store,
ut_assertok(lmb_push(store)); lmb = lmb_get(); - *mem_lstp = &lmb->free_mem; + *mem_lstp = &lmb->available_mem; *used_lstp = &lmb->used_mem;
return 0;
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
free_mem is a misnomer. We never update it with the free memory for LMB. Instead, it describes all available memory and is checked against used_mem to decide whether an area is free or not.
So let's rename this field to better match its usage.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
[snip]
There's no point defining a function that's called only once just to avoid passing the flags. Remove the wrapper and just call lmb_add_region_flags().
Acked-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/lmb.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index da960e422ada..659581f13f20 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -632,19 +632,13 @@ void lmb_add_memory(void) } }
-static long lmb_add_region(struct alist *lmb_rgn_lst, phys_addr_t base, - phys_size_t size) -{ - return lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE); -} - /* This routine may be called with relocation disabled. */ long lmb_add(phys_addr_t base, phys_size_t size) { long ret; struct alist *lmb_rgn_lst = &lmb.available_mem;
- ret = lmb_add_region(lmb_rgn_lst, base, size); + ret = lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE); if (ret) return ret;
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
There's no point defining a function that's called only once just to avoid passing the flags. Remove the wrapper and just call lmb_add_region_flags().
Acked-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
lib/lmb.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index da960e422ada..659581f13f20 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -632,19 +632,13 @@ void lmb_add_memory(void) } }
-static long lmb_add_region(struct alist *lmb_rgn_lst, phys_addr_t base,
phys_size_t size)-{
return lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE);-}
/* This routine may be called with relocation disabled. */ long lmb_add(phys_addr_t base, phys_size_t size) { long ret; struct alist *lmb_rgn_lst = &lmb.available_mem;
ret = lmb_add_region(lmb_rgn_lst, base, size);
ret = lmb_add_region_flags(lmb_rgn_lst, base, size, LMB_NONE); if (ret) return ret;-- 2.45.2
lmb_alloc_addr() is just calling lmb_alloc_addr_flags() with LMB_NONE There's not much we gain from this abstraction, so let's remove the latter, add a flags argument to lmb_alloc_addr() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- fs/fs.c | 2 +- include/lmb.h | 10 ++++------ lib/efi_loader/efi_memory.c | 2 +- lib/lmb.c | 15 ++------------- test/lib/lmb.c | 34 +++++++++++++++++----------------- 5 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/fs/fs.c b/fs/fs.c index 21a23efd932d..99ddcc5e37be 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -554,7 +554,7 @@ static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
lmb_dump_all();
- if (lmb_alloc_addr(addr, read_len) == addr) + if (lmb_alloc_addr(addr, read_len, LMB_NONE) == addr) return 0;
log_err("** Reading file would overwrite reserved memory **\n"); diff --git a/include/lmb.h b/include/lmb.h index 3db35992df8d..5e59915340b7 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -92,7 +92,6 @@ long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size); phys_size_t lmb_get_free_size(phys_addr_t addr);
/** @@ -113,8 +112,8 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, phys_addr_t max_addr, uint flags);
/** - * lmb_alloc_addr_flags() - Allocate specified memory address with specified - * attributes + * lmb_alloc_addr() - Allocate specified memory address with specified attributes + * * @base: Base Address requested * @size: Size of the region requested * @flags: Memory region attributes to be set @@ -125,8 +124,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, * * Return: Base address on success, 0 on error. */ -phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size, - uint flags); +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags);
/** * lmb_is_reserved_flags() - Test if address is in reserved region with flag @@ -164,7 +162,7 @@ void lmb_pop(struct lmb *store);
static inline int lmb_read_check(phys_addr_t addr, phys_size_t len) { - return lmb_alloc_addr(addr, len) == addr ? 0 : -1; + return lmb_alloc_addr(addr, len, LMB_NONE) == addr ? 0 : -1; }
/** diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index edd7da7d8c6e..34e2b9e18ef0 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -490,7 +490,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_NOT_FOUND;
addr = map_to_sysmem((void *)(uintptr_t)*memory); - addr = (u64)lmb_alloc_addr_flags(addr, len, flags); + addr = (u64)lmb_alloc_addr(addr, len, flags); if (!addr) return EFI_NOT_FOUND; break; diff --git a/lib/lmb.c b/lib/lmb.c index 659581f13f20..ffad7ec12eb5 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -761,8 +761,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, return alloc; }
-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, - u32 flags) +static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) { long rgn; struct lmb_region *lmb_memory = lmb.available_mem.data; @@ -786,17 +785,7 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, return 0; }
-/* - * Try to allocate a specific address range: must be in defined memory but not - * reserved - */ -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size) -{ - return _lmb_alloc_addr(base, size, LMB_NONE); -} - -phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size, - uint flags) +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags) { return _lmb_alloc_addr(base, size, flags); } diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 6e870274fedb..971614fd8314 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -530,21 +530,21 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ut_asserteq(ret, 0);
/* Try to allocate a page twice */ - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NONE); ut_asserteq(b, alloc_addr_a); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); ut_asserteq(b, 0); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NONE); ut_asserteq(b, alloc_addr_a); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x2000, LMB_NONE); + b = lmb_alloc_addr(alloc_addr_a, 0x2000, LMB_NONE); ut_asserteq(b, alloc_addr_a); ret = lmb_free(alloc_addr_a, 0x2000); ut_asserteq(ret, 0); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); ut_asserteq(b, alloc_addr_a); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NONE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NONE); ut_asserteq(b, 0); - b = lmb_alloc_addr_flags(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); + b = lmb_alloc_addr(alloc_addr_a, 0x1000, LMB_NOOVERWRITE); ut_asserteq(b, 0); ret = lmb_free(alloc_addr_a, 0x1000); ut_asserteq(ret, 0); @@ -560,22 +560,22 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) alloc_addr_b, 0x10000, alloc_addr_c, 0x10000);
/* allocate blocks */ - a = lmb_alloc_addr(ram, alloc_addr_a - ram); + a = lmb_alloc_addr(ram, alloc_addr_a - ram, LMB_NONE); ut_asserteq(a, ram); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 3, ram, 0x8010000, alloc_addr_b, 0x10000, alloc_addr_c, 0x10000); b = lmb_alloc_addr(alloc_addr_a + 0x10000, - alloc_addr_b - alloc_addr_a - 0x10000); + alloc_addr_b - alloc_addr_a - 0x10000, LMB_NONE); ut_asserteq(b, alloc_addr_a + 0x10000); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, ram, 0x10010000, alloc_addr_c, 0x10000, 0, 0); c = lmb_alloc_addr(alloc_addr_b + 0x10000, - alloc_addr_c - alloc_addr_b - 0x10000); + alloc_addr_c - alloc_addr_b - 0x10000, LMB_NONE); ut_asserteq(c, alloc_addr_b + 0x10000); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram, 0x18010000, 0, 0, 0, 0); d = lmb_alloc_addr(alloc_addr_c + 0x10000, - ram_end - alloc_addr_c - 0x10000); + ram_end - alloc_addr_c - 0x10000, LMB_NONE); ut_asserteq(d, alloc_addr_c + 0x10000); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram, ram_size, 0, 0, 0, 0); @@ -591,7 +591,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram)
/* allocate at 3 points in free range */
- d = lmb_alloc_addr(ram_end - 4, 4); + d = lmb_alloc_addr(ram_end - 4, 4, LMB_NONE); ut_asserteq(d, ram_end - 4); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, ram, 0x18010000, d, 4, 0, 0); @@ -600,7 +600,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram, 0x18010000, 0, 0, 0, 0);
- d = lmb_alloc_addr(ram_end - 128, 4); + d = lmb_alloc_addr(ram_end - 128, 4, LMB_NONE); ut_asserteq(d, ram_end - 128); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, ram, 0x18010000, d, 4, 0, 0); @@ -609,7 +609,7 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram, 0x18010000, 0, 0, 0, 0);
- d = lmb_alloc_addr(alloc_addr_c + 0x10000, 4); + d = lmb_alloc_addr(alloc_addr_c + 0x10000, 4, LMB_NONE); ut_asserteq(d, alloc_addr_c + 0x10000); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram, 0x18010004, 0, 0, 0, 0); @@ -624,18 +624,18 @@ static int test_alloc_addr(struct unit_test_state *uts, const phys_addr_t ram) ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram + 0x8000000, 0x10010000, 0, 0, 0, 0);
- d = lmb_alloc_addr(ram, 4); + d = lmb_alloc_addr(ram, 4, LMB_NONE); ut_asserteq(d, ram); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 2, d, 4, ram + 0x8000000, 0x10010000, 0, 0);
/* check that allocating outside memory fails */ if (ram_end != 0) { - ret = lmb_alloc_addr(ram_end, 1); + ret = lmb_alloc_addr(ram_end, 1, LMB_NONE); ut_asserteq(ret, 0); } if (ram != 0) { - ret = lmb_alloc_addr(ram - 1, 1); + ret = lmb_alloc_addr(ram - 1, 1, LMB_NONE); ut_asserteq(ret, 0); }
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_alloc_addr() is just calling lmb_alloc_addr_flags() with LMB_NONE There's not much we gain from this abstraction, so let's remove the latter, add a flags argument to lmb_alloc_addr() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
fs/fs.c | 2 +- include/lmb.h | 10 ++++------ lib/efi_loader/efi_memory.c | 2 +- lib/lmb.c | 15 ++------------- test/lib/lmb.c | 34 +++++++++++++++++----------------- 5 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/fs/fs.c b/fs/fs.c index 21a23efd932d..99ddcc5e37be 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -554,7 +554,7 @@ static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
lmb_dump_all();
if (lmb_alloc_addr(addr, read_len) == addr)
if (lmb_alloc_addr(addr, read_len, LMB_NONE) == addr) return 0; log_err("** Reading file would overwrite reserved memory **\n");diff --git a/include/lmb.h b/include/lmb.h index 3db35992df8d..5e59915340b7 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -92,7 +92,6 @@ long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size); phys_size_t lmb_get_free_size(phys_addr_t addr);
/** @@ -113,8 +112,8 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, phys_addr_t max_addr, uint flags);
/**
- lmb_alloc_addr_flags() - Allocate specified memory address with specified
attributes
- lmb_alloc_addr() - Allocate specified memory address with specified attributes
One character over 80 limit trips my OCD :/ Any chance you can move "attributes" to the next line? It's ok to break the line in kernel-doc function briefs.
- @base: Base Address requested
- @size: Size of the region requested
- @flags: Memory region attributes to be set
@@ -125,8 +124,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
- Return: Base address on success, 0 on error.
*/ -phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
uint flags);+phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags);
Can we keep u32 for flags everywhere, for consistency?
With above nitpicks addressed, feel free to add:
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
[snip]
On Fri, 13 Dec 2024 at 00:34, Sam Protsenko semen.protsenko@linaro.org wrote:
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_alloc_addr() is just calling lmb_alloc_addr_flags() with LMB_NONE There's not much we gain from this abstraction, so let's remove the latter, add a flags argument to lmb_alloc_addr() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
fs/fs.c | 2 +- include/lmb.h | 10 ++++------ lib/efi_loader/efi_memory.c | 2 +- lib/lmb.c | 15 ++------------- test/lib/lmb.c | 34 +++++++++++++++++----------------- 5 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/fs/fs.c b/fs/fs.c index 21a23efd932d..99ddcc5e37be 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -554,7 +554,7 @@ static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset,
lmb_dump_all();
if (lmb_alloc_addr(addr, read_len) == addr)
if (lmb_alloc_addr(addr, read_len, LMB_NONE) == addr) return 0; log_err("** Reading file would overwrite reserved memory **\n");diff --git a/include/lmb.h b/include/lmb.h index 3db35992df8d..5e59915340b7 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -92,7 +92,6 @@ long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size); phys_size_t lmb_get_free_size(phys_addr_t addr);
/** @@ -113,8 +112,8 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, phys_addr_t max_addr, uint flags);
/**
- lmb_alloc_addr_flags() - Allocate specified memory address with specified
attributes
- lmb_alloc_addr() - Allocate specified memory address with specified attributes
One character over 80 limit trips my OCD :/ Any chance you can move "attributes" to the next line? It's ok to break the line in kernel-doc function briefs.
- @base: Base Address requested
- @size: Size of the region requested
- @flags: Memory region attributes to be set
@@ -125,8 +124,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
- Return: Base address on success, 0 on error.
*/ -phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
uint flags);+phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags);
Can we keep u32 for flags everywhere, for consistency?
We are, I thought it would make the patches easier to review if I only changed the enum here. This turns to a u32 in the last patch
With above nitpicks addressed, feel free to add:
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
Thanks! /Ilias
[snip]
lmb_alloc_base() is just calling lmb_alloc_base_flags() with LMB_NONE. There's not much we gain from this abstraction, so let's remove the former add the flags argument to lmb_alloc_base() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- boot/image-board.c | 18 +++++++++++------- boot/image-fdt.c | 5 +++-- include/lmb.h | 7 +++---- lib/efi_loader/efi_memory.c | 4 ++-- lib/lmb.c | 19 +++---------------- test/lib/lmb.c | 8 ++++---- 6 files changed, 26 insertions(+), 35 deletions(-)
diff --git a/boot/image-board.c b/boot/image-board.c index 070ada007185..4e86a9a22719 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -565,9 +565,11 @@ int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start, lmb_reserve(rd_data, rd_len, LMB_NONE); } else { if (initrd_high) - *initrd_start = (ulong)lmb_alloc_base(rd_len, - 0x1000, - initrd_high); + *initrd_start = + (ulong)lmb_alloc_base(rd_len, + 0x1000, + initrd_high, + LMB_NONE); else *initrd_start = (ulong)lmb_alloc(rd_len, 0x1000); @@ -839,7 +841,8 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end)
barg = IF_ENABLED_INT(CONFIG_SYS_BOOT_GET_CMDLINE, CONFIG_SYS_BARGSIZE); cmdline = (char *)(ulong)lmb_alloc_base(barg, 0xf, - env_get_bootm_mapsize() + env_get_bootm_low()); + env_get_bootm_mapsize() + env_get_bootm_low(), + LMB_NONE); if (!cmdline) return -1;
@@ -872,9 +875,10 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end) int boot_get_kbd(struct bd_info **kbd) { *kbd = (struct bd_info *)(ulong)lmb_alloc_base(sizeof(struct bd_info), - 0xf, - env_get_bootm_mapsize() + - env_get_bootm_low()); + 0xf, + env_get_bootm_mapsize() + + env_get_bootm_low(), + LMB_NONE); if (!*kbd) return -1;
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index d717f6690729..9d1598b1a93c 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -187,7 +187,8 @@ int boot_relocate_fdt(char **of_flat_tree, ulong *of_size) lmb_reserve(map_to_sysmem(of_start), of_len, LMB_NONE); disable_relocation = 1; } else if (desired_addr) { - addr = lmb_alloc_base(of_len, 0x1000, desired_addr); + addr = lmb_alloc_base(of_len, 0x1000, desired_addr, + LMB_NONE); of_start = map_sysmem(addr, of_len); if (of_start == NULL) { puts("Failed using fdt_high value for Device Tree"); @@ -216,7 +217,7 @@ int boot_relocate_fdt(char **of_flat_tree, ulong *of_size) * for LMB allocation. */ usable = min(start + size, low + mapsize); - addr = lmb_alloc_base(of_len, 0x1000, usable); + addr = lmb_alloc_base(of_len, 0x1000, usable, LMB_NONE); of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) diff --git a/include/lmb.h b/include/lmb.h index 5e59915340b7..d481972a6686 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -91,11 +91,10 @@ long lmb_add(phys_addr_t base, phys_size_t size); long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); -phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); phys_size_t lmb_get_free_size(phys_addr_t addr);
/** - * lmb_alloc_base_flags() - Allocate specified memory region with specified + * lmb_alloc_base() - Allocate specified memory region with specified * attributes * @size: Size of the region requested * @align: Alignment of the memory region requested @@ -108,8 +107,8 @@ phys_size_t lmb_get_free_size(phys_addr_t addr); * * Return: Base address on success, 0 on error. */ -phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, - phys_addr_t max_addr, uint flags); +phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, + uint flags);
/** * lmb_alloc_addr() - Allocate specified memory address with specified attributes diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 34e2b9e18ef0..1212772471e3 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -472,7 +472,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, switch (type) { case EFI_ALLOCATE_ANY_PAGES: /* Any page */ - addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, + addr = (u64)lmb_alloc_base(len, EFI_PAGE_SIZE, LMB_ALLOC_ANYWHERE, flags); if (!addr) return EFI_OUT_OF_RESOURCES; @@ -480,7 +480,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, case EFI_ALLOCATE_MAX_ADDRESS: /* Max address */ addr = map_to_sysmem((void *)(uintptr_t)*memory); - addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr, + addr = (u64)lmb_alloc_base(len, EFI_PAGE_SIZE, addr, flags); if (!addr) return EFI_OUT_OF_RESOURCES; diff --git a/lib/lmb.c b/lib/lmb.c index ffad7ec12eb5..bdaaa634abd1 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -731,24 +731,11 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
phys_addr_t lmb_alloc(phys_size_t size, ulong align) { - return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE); + return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE, LMB_NONE); }
-phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) -{ - phys_addr_t alloc; - - alloc = _lmb_alloc_base(size, align, max_addr, LMB_NONE); - - if (alloc == 0) - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", - (ulong)size, (ulong)max_addr); - - return alloc; -} - -phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, - phys_addr_t max_addr, uint flags) +phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, + uint flags) { phys_addr_t alloc;
diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 971614fd8314..fcb5f1af532a 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -128,7 +128,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr, 0x10000, ram_end - 4, 4, 0, 0); /* alloc below end of reserved region -> below reserved region */ - b = lmb_alloc_base(4, 1, alloc_64k_end); + b = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(b, alloc_64k_addr - 4); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 4, 0x10000 + 4, ram_end - 4, 4, 0, 0); @@ -138,7 +138,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, ut_asserteq(c, ram_end - 8); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 4, 0x10000 + 4, ram_end - 8, 8, 0, 0); - d = lmb_alloc_base(4, 1, alloc_64k_end); + d = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(d, alloc_64k_addr - 8); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 8, 0, 0); @@ -163,7 +163,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, alloc_64k_addr - 8, 4, alloc_64k_addr, 0x10000, ram_end - 8, 4); /* allocate again to ensure we get the same address */ - b2 = lmb_alloc_base(4, 1, alloc_64k_end); + b2 = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(b, b2); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 4, 0, 0); @@ -363,7 +363,7 @@ static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram, ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
/* allocate a block with base*/ - b = lmb_alloc_base(alloc_size, align, ram_end); + b = lmb_alloc_base(alloc_size, align, ram_end, LMB_NONE); ut_assert(a == b); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram + ram_size - alloc_size_aligned,
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_alloc_base() is just calling lmb_alloc_base_flags() with LMB_NONE. There's not much we gain from this abstraction, so let's remove the former add the flags argument to lmb_alloc_base() and make the code a bit easier to follow.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
boot/image-board.c | 18 +++++++++++------- boot/image-fdt.c | 5 +++-- include/lmb.h | 7 +++---- lib/efi_loader/efi_memory.c | 4 ++-- lib/lmb.c | 19 +++---------------- test/lib/lmb.c | 8 ++++---- 6 files changed, 26 insertions(+), 35 deletions(-)
diff --git a/boot/image-board.c b/boot/image-board.c index 070ada007185..4e86a9a22719 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -565,9 +565,11 @@ int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start, lmb_reserve(rd_data, rd_len, LMB_NONE); } else { if (initrd_high)
*initrd_start = (ulong)lmb_alloc_base(rd_len,0x1000,initrd_high);
*initrd_start =(ulong)lmb_alloc_base(rd_len,0x1000,initrd_high,LMB_NONE); else *initrd_start = (ulong)lmb_alloc(rd_len, 0x1000);@@ -839,7 +841,8 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end)
barg = IF_ENABLED_INT(CONFIG_SYS_BOOT_GET_CMDLINE, CONFIG_SYS_BARGSIZE); cmdline = (char *)(ulong)lmb_alloc_base(barg, 0xf,
env_get_bootm_mapsize() + env_get_bootm_low());
env_get_bootm_mapsize() + env_get_bootm_low(),LMB_NONE); if (!cmdline) return -1;@@ -872,9 +875,10 @@ int boot_get_cmdline(ulong *cmd_start, ulong *cmd_end) int boot_get_kbd(struct bd_info **kbd) { *kbd = (struct bd_info *)(ulong)lmb_alloc_base(sizeof(struct bd_info),
0xf,env_get_bootm_mapsize() +env_get_bootm_low());
0xf,env_get_bootm_mapsize() +env_get_bootm_low(),LMB_NONE); if (!*kbd) return -1;diff --git a/boot/image-fdt.c b/boot/image-fdt.c index d717f6690729..9d1598b1a93c 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -187,7 +187,8 @@ int boot_relocate_fdt(char **of_flat_tree, ulong *of_size) lmb_reserve(map_to_sysmem(of_start), of_len, LMB_NONE); disable_relocation = 1; } else if (desired_addr) {
addr = lmb_alloc_base(of_len, 0x1000, desired_addr);
addr = lmb_alloc_base(of_len, 0x1000, desired_addr,LMB_NONE); of_start = map_sysmem(addr, of_len); if (of_start == NULL) { puts("Failed using fdt_high value for Device Tree");@@ -216,7 +217,7 @@ int boot_relocate_fdt(char **of_flat_tree, ulong *of_size) * for LMB allocation. */ usable = min(start + size, low + mapsize);
addr = lmb_alloc_base(of_len, 0x1000, usable);
addr = lmb_alloc_base(of_len, 0x1000, usable, LMB_NONE); of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL)diff --git a/include/lmb.h b/include/lmb.h index 5e59915340b7..d481972a6686 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -91,11 +91,10 @@ long lmb_add(phys_addr_t base, phys_size_t size); long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags);
phys_addr_t lmb_alloc(phys_size_t size, ulong align); -phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr); phys_size_t lmb_get_free_size(phys_addr_t addr);
/**
- lmb_alloc_base_flags() - Allocate specified memory region with specified
- lmb_alloc_base() - Allocate specified memory region with specified
attributes- @size: Size of the region requested
- @align: Alignment of the memory region requested
@@ -108,8 +107,8 @@ phys_size_t lmb_get_free_size(phys_addr_t addr);
- Return: Base address on success, 0 on error.
*/ -phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
phys_addr_t max_addr, uint flags);+phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr,
uint flags);/**
- lmb_alloc_addr() - Allocate specified memory address with specified attributes
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 34e2b9e18ef0..1212772471e3 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -472,7 +472,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, switch (type) { case EFI_ALLOCATE_ANY_PAGES: /* Any page */
addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE,
addr = (u64)lmb_alloc_base(len, EFI_PAGE_SIZE, LMB_ALLOC_ANYWHERE, flags); if (!addr) return EFI_OUT_OF_RESOURCES;@@ -480,7 +480,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, case EFI_ALLOCATE_MAX_ADDRESS: /* Max address */ addr = map_to_sysmem((void *)(uintptr_t)*memory);
addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr,
addr = (u64)lmb_alloc_base(len, EFI_PAGE_SIZE, addr, flags); if (!addr) return EFI_OUT_OF_RESOURCES;diff --git a/lib/lmb.c b/lib/lmb.c index ffad7ec12eb5..bdaaa634abd1 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -731,24 +731,11 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
phys_addr_t lmb_alloc(phys_size_t size, ulong align) {
return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE);
return lmb_alloc_base(size, align, LMB_ALLOC_ANYWHERE, LMB_NONE);}
-phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr) -{
phys_addr_t alloc;alloc = _lmb_alloc_base(size, align, max_addr, LMB_NONE);if (alloc == 0)printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",(ulong)size, (ulong)max_addr);return alloc;-}
-phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
phys_addr_t max_addr, uint flags)+phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr,
uint flags){ phys_addr_t alloc;
diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 971614fd8314..fcb5f1af532a 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -128,7 +128,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr, 0x10000, ram_end - 4, 4, 0, 0); /* alloc below end of reserved region -> below reserved region */
b = lmb_alloc_base(4, 1, alloc_64k_end);
b = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(b, alloc_64k_addr - 4); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 4, 0x10000 + 4, ram_end - 4, 4, 0, 0);@@ -138,7 +138,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, ut_asserteq(c, ram_end - 8); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 4, 0x10000 + 4, ram_end - 8, 8, 0, 0);
d = lmb_alloc_base(4, 1, alloc_64k_end);
d = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(d, alloc_64k_addr - 8); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 8, 0, 0);@@ -163,7 +163,7 @@ static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram, alloc_64k_addr - 8, 4, alloc_64k_addr, 0x10000, ram_end - 8, 4); /* allocate again to ensure we get the same address */
b2 = lmb_alloc_base(4, 1, alloc_64k_end);
b2 = lmb_alloc_base(4, 1, alloc_64k_end, LMB_NONE); ut_asserteq(b, b2); ASSERT_LMB(mem_lst, used_lst, 0, 0, 2, alloc_64k_addr - 8, 0x10000 + 8, ram_end - 8, 4, 0, 0);@@ -363,7 +363,7 @@ static int test_noreserved(struct unit_test_state *uts, const phys_addr_t ram, ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 0, 0, 0, 0, 0, 0, 0);
/* allocate a block with base*/
b = lmb_alloc_base(alloc_size, align, ram_end);
b = lmb_alloc_base(alloc_size, align, ram_end, LMB_NONE); ut_assert(a == b); ASSERT_LMB(mem_lst, used_lst, ram, ram_size, 1, ram + ram_size - alloc_size_aligned,-- 2.45.2
lmb_alloc_addr_flags() is a wrapper for _lmb_alloc_addr() and it's the only function using it. Rename _lmb_alloc_addr() to lmb_alloc_addr_flags() and remove the wrapper.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- include/lmb.h | 2 +- lib/lmb.c | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index d481972a6686..76fb82b197a9 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -123,7 +123,7 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, * * Return: Base address on success, 0 on error. */ -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags); +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags);
/** * lmb_is_reserved_flags() - Test if address is in reserved region with flag diff --git a/lib/lmb.c b/lib/lmb.c index bdaaa634abd1..7ca44591e1d7 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -748,7 +748,7 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, return alloc; }
-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) { long rgn; struct lmb_region *lmb_memory = lmb.available_mem.data; @@ -772,11 +772,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags return 0; }
-phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags) -{ - return _lmb_alloc_addr(base, size, flags); -} - /* Return number of bytes from a given address that are free */ phys_size_t lmb_get_free_size(phys_addr_t addr) {
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
lmb_alloc_addr_flags() is a wrapper for _lmb_alloc_addr() and it's the only function using it. Rename _lmb_alloc_addr() to lmb_alloc_addr_flags() and remove the wrapper.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
include/lmb.h | 2 +- lib/lmb.c | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index d481972a6686..76fb82b197a9 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -123,7 +123,7 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr,
- Return: Base address on success, 0 on error.
*/ -phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags); +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags);
/**
- lmb_is_reserved_flags() - Test if address is in reserved region with flag
diff --git a/lib/lmb.c b/lib/lmb.c index bdaaa634abd1..7ca44591e1d7 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -748,7 +748,7 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr, return alloc; }
-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) +phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags) { long rgn; struct lmb_region *lmb_memory = lmb.available_mem.data; @@ -772,11 +772,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, u32 flags return 0; }
-phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size, uint flags) -{
return _lmb_alloc_addr(base, size, flags);-}
/* Return number of bytes from a given address that are free */ phys_size_t lmb_get_free_size(phys_addr_t addr) { -- 2.45.2
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi all,
This is v1 of the rfc [0]
The LMB subsystem was used opportunistically for a number of years. A while back Sughosh merged it with the EFI subsystem in order to have a common allocator and avoid subsystems overwriting memory they shouldn't.
This is an initial cleanup of all the crud we gathered over the years. There's no functional change expected from the patches as they just cleanup some abstraction functions and rename a few variables to make more sense.
I plan to make even bigger changes -- e.g I don't see the point of having *_alloc() and *_reserve() versions of the functions since they mostly do the same thing and just cause confusion. lmb_alloc_addr_flags() returning the base address on success makes little sense since we already *request* the address on the function arguments, etc. Since this patchset grew enough already, I'd like to get it in before more refactoring happens.
It's worth noting that although some patches slightly increase the code size due to an extra flags argument being carried around, the final result is eventually smaller.
# qemu_arm64_lwip_defconfig (version string adds another 20b) add/remove: 0/5 grow/shrink: 15/1 up/down: 568/-628 (-60) Function old new delta lmb_alloc_base 80 324 +244 lmb_alloc_addr 8 144 +136 lmb_reserve 8 96 +88 version_string 50 70 +20 boot_relocate_fdt 488 508 +20 boot_ramdisk_high 268 284 +16 lmb_add_region_flags 696 704 +8 boot_fdt_reserve_region 100 108 +8 load_serial 548 552 +4 lmb_alloc 8 12 +4 image_setup_libfdt 368 372 +4 do_load 728 732 +4 do_bootz 332 336 +4 do_booti 520 524 +4 bootm_run_states 2176 2180 +4 lmb_alloc_addr_flags 4 - -4 boot_fdt_add_mem_rsv_regions 284 280 -4 lmb_alloc_base_flags 76 - -76 lmb_reserve_flags 96 - -96 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=1020102, After=1020042, chg -0.01%
# sandbox_defconfig (version string adds another 20b) add/remove: 0/3 grow/shrink: 24/3 up/down: 523/-501 (22) Function old new delta lmb_alloc_base 48 299 +251 lmb_alloc_addr 4 92 +88 lmb_reserve 4 58 +54 test_alloc_addr 2933 2963 +30 version_string 50 70 +20 lib_test_lmb_overlapping_reserve 1018 1030 +12 lmb_add_region_flags 600 610 +10 test_multi_alloc.constprop 3034 3042 +8 test_get_unreserved_size 1032 1038 +6 boot_relocate_fdt 599 605 +6 boot_fdt_reserve_region 67 73 +6 lmb_alloc 4 9 +5 lmb_free_flags 190 194 +4 wget_handler 1530 1533 +3 tftp_handler 1190 1192 +2 test_noreserved 1207 1209 +2 test_bigblock 911 913 +2 load_serial 946 948 +2 lib_test_lmb_flags 2101 2103 +2 do_spi_flash 3150 3152 +2 do_bootz 526 528 +2 do_bootm_linux 2067 2069 +2 bootm_run_states 5275 5277 +2 _fs_read.lto_priv 331 333 +2 lmb_dump_region.lto_priv 356 353 -3 lmb_add 59 52 -7 efi_allocate_pages.part 303 249 -54 lmb_reserve_flags 65 - -65 _lmb_alloc_addr.lto_priv 92 - -92 _lmb_alloc_base.lto_priv 280 - -280 Total: Before=2492722, After=2492744, chg +0.00%
[0] https://lore.kernel.org/u-boot/20241208105223.2821049-1-ilias.apalodimas@lin...
Changes since v1:
- Rebased on top of https://lore.kernel.org/u-boot/20241211021703.2333-1-semen.protsenko@linaro.... https://lore.kernel.org/u-boot/20241211022550.2995-1-semen.protsenko@linaro....
- Converted enum lmb_flags to a u32
- Removed the _flags from all the functions
- Added a patch that removes lmb_align_down()
- Picked up r-b tags. Tom I kept your on patch #3 since I only changed the name of the function and assumed you are ok with it.
Ilias Apalodimas (8): lmb: Remove lmb_align_down() lmb: Move enum lmb_flags to a u32 lmb: Remove lmb_reserve_flags() lmb: Rename free_mem to available_mem lmb: Remove lmb_add_region() lmb: Remove lmb_alloc_addr_flags() lmb: Remove lmb_alloc_base_flags() lmb: Rename _lmb_alloc_addr() to lmb_alloc_addr_flags()
arch/powerpc/cpu/mpc85xx/mp.c | 2 +- arch/powerpc/lib/misc.c | 2 +- boot/bootm.c | 3 +- boot/image-board.c | 20 +++-- boot/image-fdt.c | 15 ++-- cmd/booti.c | 2 +- cmd/bootz.c | 2 +- cmd/load.c | 2 +- fs/fs.c | 2 +- include/lmb.h | 58 ++++++--------- lib/efi_loader/efi_memory.c | 6 +- lib/lmb.c | 136 +++++++++++----------------------- test/cmd/bdinfo.c | 4 +- test/lib/lmb.c | 92 +++++++++++------------ 14 files changed, 145 insertions(+), 201 deletions(-)
--
Hi Ilias,
This series doesn't apply as is on current master (tried MBOX file from lore). I'll still try to review it by merging conflicts locally, but please double check it on your side, and re-submit in case of actual conflicts.
2.45.2
On Thu, Dec 12, 2024 at 3:30 PM Sam Protsenko semen.protsenko@linaro.org wrote:
Hi Ilias,
This series doesn't apply as is on current master (tried MBOX file from lore). I'll still try to review it by merging conflicts locally, but please double check it on your side, and re-submit in case of actual conflicts.
Never mind, I missed the bit about this series being based on mine [1].
[1] https://lore.kernel.org/u-boot/20241211022550.2995-1-semen.protsenko@linaro....
[snip]
On Wed, Dec 11, 2024 at 4:55 AM Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi all,
This is v1 of the rfc [0]
The LMB subsystem was used opportunistically for a number of years. A while back Sughosh merged it with the EFI subsystem in order to have a common allocator and avoid subsystems overwriting memory they shouldn't.
This is an initial cleanup of all the crud we gathered over the years. There's no functional change expected from the patches as they just cleanup some abstraction functions and rename a few variables to make more sense.
I plan to make even bigger changes -- e.g I don't see the point of having *_alloc() and *_reserve() versions of the functions since they mostly do the same thing and just cause confusion. lmb_alloc_addr_flags() returning the base address on success makes little sense since we already *request* the address on the function arguments, etc. Since this patchset grew enough already, I'd like to get it in before more refactoring happens.
It's worth noting that although some patches slightly increase the code size due to an extra flags argument being carried around, the final result is eventually smaller.
# qemu_arm64_lwip_defconfig (version string adds another 20b) add/remove: 0/5 grow/shrink: 15/1 up/down: 568/-628 (-60) Function old new delta lmb_alloc_base 80 324 +244 lmb_alloc_addr 8 144 +136 lmb_reserve 8 96 +88 version_string 50 70 +20 boot_relocate_fdt 488 508 +20 boot_ramdisk_high 268 284 +16 lmb_add_region_flags 696 704 +8 boot_fdt_reserve_region 100 108 +8 load_serial 548 552 +4 lmb_alloc 8 12 +4 image_setup_libfdt 368 372 +4 do_load 728 732 +4 do_bootz 332 336 +4 do_booti 520 524 +4 bootm_run_states 2176 2180 +4 lmb_alloc_addr_flags 4 - -4 boot_fdt_add_mem_rsv_regions 284 280 -4 lmb_alloc_base_flags 76 - -76 lmb_reserve_flags 96 - -96 _lmb_alloc_addr 144 - -144 _lmb_alloc_base 304 - -304 Total: Before=1020102, After=1020042, chg -0.01%
# sandbox_defconfig (version string adds another 20b) add/remove: 0/3 grow/shrink: 24/3 up/down: 523/-501 (22) Function old new delta lmb_alloc_base 48 299 +251 lmb_alloc_addr 4 92 +88 lmb_reserve 4 58 +54 test_alloc_addr 2933 2963 +30 version_string 50 70 +20 lib_test_lmb_overlapping_reserve 1018 1030 +12 lmb_add_region_flags 600 610 +10 test_multi_alloc.constprop 3034 3042 +8 test_get_unreserved_size 1032 1038 +6 boot_relocate_fdt 599 605 +6 boot_fdt_reserve_region 67 73 +6 lmb_alloc 4 9 +5 lmb_free_flags 190 194 +4 wget_handler 1530 1533 +3 tftp_handler 1190 1192 +2 test_noreserved 1207 1209 +2 test_bigblock 911 913 +2 load_serial 946 948 +2 lib_test_lmb_flags 2101 2103 +2 do_spi_flash 3150 3152 +2 do_bootz 526 528 +2 do_bootm_linux 2067 2069 +2 bootm_run_states 5275 5277 +2 _fs_read.lto_priv 331 333 +2 lmb_dump_region.lto_priv 356 353 -3 lmb_add 59 52 -7 efi_allocate_pages.part 303 249 -54 lmb_reserve_flags 65 - -65 _lmb_alloc_addr.lto_priv 92 - -92 _lmb_alloc_base.lto_priv 280 - -280 Total: Before=2492722, After=2492744, chg +0.00%
[0] https://lore.kernel.org/u-boot/20241208105223.2821049-1-ilias.apalodimas@lin...
Changes since v1:
- Rebased on top of https://lore.kernel.org/u-boot/20241211021703.2333-1-semen.protsenko@linaro.... https://lore.kernel.org/u-boot/20241211022550.2995-1-semen.protsenko@linaro....
- Converted enum lmb_flags to a u32
- Removed the _flags from all the functions
- Added a patch that removes lmb_align_down()
- Picked up r-b tags. Tom I kept your on patch #3 since I only changed the name of the function and assumed you are ok with it.
Ilias Apalodimas (8): lmb: Remove lmb_align_down() lmb: Move enum lmb_flags to a u32 lmb: Remove lmb_reserve_flags() lmb: Rename free_mem to available_mem lmb: Remove lmb_add_region() lmb: Remove lmb_alloc_addr_flags() lmb: Remove lmb_alloc_base_flags() lmb: Rename _lmb_alloc_addr() to lmb_alloc_addr_flags()
arch/powerpc/cpu/mpc85xx/mp.c | 2 +- arch/powerpc/lib/misc.c | 2 +- boot/bootm.c | 3 +- boot/image-board.c | 20 +++-- boot/image-fdt.c | 15 ++-- cmd/booti.c | 2 +- cmd/bootz.c | 2 +- cmd/load.c | 2 +- fs/fs.c | 2 +- include/lmb.h | 58 ++++++--------- lib/efi_loader/efi_memory.c | 6 +- lib/lmb.c | 136 +++++++++++----------------------- test/cmd/bdinfo.c | 4 +- test/lib/lmb.c | 92 +++++++++++------------ 14 files changed, 145 insertions(+), 201 deletions(-)
--
For all patches in the series:
Tested-by: Sam Protsenko semen.protsenko@linaro.org
Tested on the E850-96 board. Didn't notice any changes. 'bdinfo' output is good, Debian is booting just fine, as before this series. The U-Boot output is good too (nothing changed). No regressions found.
2.45.2
participants (2)
-
Ilias Apalodimas -
Sam Protsenko