| Message ID | 20240607185240.1892031-1-sughosh.ganu@linaro.org |
|---|---|
| Headers | show |
| Series | Make U-Boot memory reservations coherent | expand |
On Sat, Jun 08, 2024 at 12:22:09AM +0530, Sughosh Ganu wrote: > The aim of this patch series is to fix the current state of > incoherence between modules when it comes to memory usage. The primary > issue that this series is trying to fix is that the EFI memory module > which is responsible for allocating and freeing memory, does not have > any visibility of the memory that is being used by the LMB > module. This is further complicated by the fact that the LMB > allocations are caller specific -- the LMB memory map is not global > nor persistent. This means that the memory "allocated" by the LMB > module might be relevant only for a given function. Hence one of the > requirements for making the memory usage visible across modules is to > make LMB allocations persistent and global, and then have means to > communicate the use of memory across modules. > > The first set of patches in this series work on making the LMB memory > map persistent and global. This is being done keeping in mind the > usage of LMB memory by platforms where the same memory region can be > used to load multiple different images. What is not allowed is to > overwrite memory that has been allocated by the other module, > currently the EFI memory module. This is being achieved by introducing > a new flag, LMB_NOOVERWRITE, which represents memory which cannot be > re-requested once allocated. > > The second set of patches are then introducing a notification > mechanism to indicate any changes to a respective module's memory > map. This way, any memory allocation/reservation by a module gets > notified to any interested listners, who then update their memory map > accordingly, thus keeping memory usage coherent. > > Todo's > ------ > I have run these patches through CI, but the LMB unit tests need an > overhaul. I have currently marked these tests for manual run, as > running these would obviously tamper the LMB memory map, thus > affecting subsequent tests. The current set of LMB tests are written > with the assumption of local LMB memory maps. This needs to be > reworked. > > Secondly, there needs to be a test written for testing the various > scenarios of memory being allocated and freed by different modules, > namely LMB and EFI. I have written a couple of commands for testing > the changes that I have made -- I have also included these temporary > commands to assist anyone who might want to test these changes. But I > will be working on adding a more comprehensive test. > > Lastly, as the series touches common code, there is obviously an > increase in the size of the image, moreover since the LMB memory is > now persistent, and the variables do not reside on the stack. I want > to check if there can be ways of decreasing the size impact of the > changes. So, I think you made some changes between your last CI run and posting? A ton of platforms (basically anything with EFI_LOADER disabled) fail to build now because the two "temp" commits at the end of the series are always enabled. I took those commits out and ran my world build size check tests and the results are at: https://gist.github.com/trini/d42bd392463c39766a5f872c190ccf27 And 64bit ARM looks very reasonable, but I wonder if we can improve the 32bit ARM results? I also did an every commit run for a 32bit ARM board without EFI_LOADER (which is going to be the case for size constrained systems) and see that the series isn't bisect'able to start with, so please fix that for the next go-round. That said: Summary of 30 commits for 1 boards (1 thread, 12 jobs per thread) 01: Added arm64 assembly for examples/api crt0 02: lmb: remove the unused lmb_is_reserved() function 03: lmb: staticize __lmb_alloc_base() arm: (for 1/1 boards) all -16.0 text -16.0 omapl138_lcdk : all -16 text -16 u-boot: add: 0/-1, grow: 1/0 bytes: 172/-186 (-14) function old new delta lmb_alloc_base 40 212 +172 __lmb_alloc_base 186 - -186 04: lmb: make the lmb reservations persistent arm: + omapl138_lcdk +(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': +(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); +(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': +(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': +(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': +(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; +(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': +(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; +(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; +(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': +(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); +(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, +(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, +(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': +(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); +(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { +(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { +(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; +(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': +(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; +(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) +(omapl138_lcdk) | ^ +(omapl138_lcdk) | -> +(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? +(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 +(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 +(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 05: lmb: remove local instances of the lmb structure variable -(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': -(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); -(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': -(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': -(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': -(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; -(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': -(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; -(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; -(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': -(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); -(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, -(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, -(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': -(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); -(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { -(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { -(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; -(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': -(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; -(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) -(omapl138_lcdk) | ^ -(omapl138_lcdk) | -> -(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? -(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 -(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 +(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': +(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) +(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); +(omapl138_lcdk) | ^~~ +(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 +(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 06: lmb: pass a flag to image_setup_libfdt() for lmb reservations arm: omapl138_lcdk -(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': -(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) -(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); -(omapl138_lcdk) | ^~~ -(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 -(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 -(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 07: lmb: reserve and add common memory regions post relocation arm: + omapl138_lcdk +(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': +(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] +(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); +(omapl138_lcdk) | ^~~~~~~~~~~~~~ +(omapl138_lcdk) cc1: all warnings being treated as errors +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 +(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 08: lmb: remove lmb_init_and_reserve_range() function 09: lmb: replcace the lmb_init_and_reserve() function arm: omapl138_lcdk -(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': -(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] -(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); -(omapl138_lcdk) | ^~~~~~~~~~~~~~ -(omapl138_lcdk) cc1: all warnings being treated as errors -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 -(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 10: lmb: allow for resizing lmb regions arm: (for 1/1 boards) all +224.0 text +224.0 omapl138_lcdk : all +224 text +224 u-boot: add: 0/0, grow: 1/0 bytes: 224/0 (224) function old new delta lmb_add_region_flags 416 640 +224 11: event: add events to notify memory map changes 12: lib: Kconfig: add a config symbol for getting memory map updates 13: add a function to check if an address is in RAM memory 14: efi_memory: notify of any changes to the EFI memory map 15: lmb: notify of any changes to the LMB memory map arm: (for 1/1 boards) all +8.0 text +8.0 omapl138_lcdk : all +8 text +8 u-boot: add: 0/0, grow: 2/0 bytes: 12/0 (12) function old new delta lmb_reserve_flags 32 40 +8 lmb_free 192 196 +4 16: efi_memory: add an event handler to update memory map 17: lmb: add an event handler to update memory map arm: (for 1/1 boards) all +349.0 rodata +61.0 text +288.0 omapl138_lcdk : all +349 rodata +61 text +288 u-boot: add: 3/0, grow: 3/0 bytes: 232/0 (232) function old new delta event_notify - 104 +104 initcall_run_list 96 144 +48 image_setup_libfdt 236 284 +48 event_notify_null - 18 +18 event_type_name - 8 +8 run_main_loop 10 16 +6 18: lmb: remove call to efi_lmb_reserve() 19: sandbox: iommu: remove lmb allocation in the driver 20: zynq: lmb: do not add to lmb map before relocation 21: test: cedit: use allocated address for reading file 22: test: event: update the expected event dump output 23: test: lmb: run the LMB tests only on sandbox 24: test: lmb: initialise the lmb structure before tests 25: test: lmb: add a test case for checking overlapping region add 26: test: lmb: adjust the test case to handle overlapping regions 27: test: lmb: run lmb tests only manually 28: test: bdinfo: dump the global LMB memory map 29: cmd: bdinfo: only dump the current LMB memory arm: (for 1/1 boards) all -8.0 text -8.0 omapl138_lcdk : all -8 text -8 u-boot: add: 0/0, grow: 0/-1 bytes: 0/-8 (-8) function old new delta do_bdinfo 472 464 -8 30: temp: mx6sabresd: bump up the size limit of the board So my first thought is, do we really need the event notifier framework in the case where it's NOT also using EFI_LOADER? Or is perhaps that Kconfig logic not quite right?
On Tue, 11 Jun 2024 at 02:35, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Jun 08, 2024 at 12:22:09AM +0530, Sughosh Ganu wrote: > > > The aim of this patch series is to fix the current state of > > incoherence between modules when it comes to memory usage. The primary > > issue that this series is trying to fix is that the EFI memory module > > which is responsible for allocating and freeing memory, does not have > > any visibility of the memory that is being used by the LMB > > module. This is further complicated by the fact that the LMB > > allocations are caller specific -- the LMB memory map is not global > > nor persistent. This means that the memory "allocated" by the LMB > > module might be relevant only for a given function. Hence one of the > > requirements for making the memory usage visible across modules is to > > make LMB allocations persistent and global, and then have means to > > communicate the use of memory across modules. > > > > The first set of patches in this series work on making the LMB memory > > map persistent and global. This is being done keeping in mind the > > usage of LMB memory by platforms where the same memory region can be > > used to load multiple different images. What is not allowed is to > > overwrite memory that has been allocated by the other module, > > currently the EFI memory module. This is being achieved by introducing > > a new flag, LMB_NOOVERWRITE, which represents memory which cannot be > > re-requested once allocated. > > > > The second set of patches are then introducing a notification > > mechanism to indicate any changes to a respective module's memory > > map. This way, any memory allocation/reservation by a module gets > > notified to any interested listners, who then update their memory map > > accordingly, thus keeping memory usage coherent. > > > > Todo's > > ------ > > I have run these patches through CI, but the LMB unit tests need an > > overhaul. I have currently marked these tests for manual run, as > > running these would obviously tamper the LMB memory map, thus > > affecting subsequent tests. The current set of LMB tests are written > > with the assumption of local LMB memory maps. This needs to be > > reworked. > > > > Secondly, there needs to be a test written for testing the various > > scenarios of memory being allocated and freed by different modules, > > namely LMB and EFI. I have written a couple of commands for testing > > the changes that I have made -- I have also included these temporary > > commands to assist anyone who might want to test these changes. But I > > will be working on adding a more comprehensive test. > > > > Lastly, as the series touches common code, there is obviously an > > increase in the size of the image, moreover since the LMB memory is > > now persistent, and the variables do not reside on the stack. I want > > to check if there can be ways of decreasing the size impact of the > > changes. > > So, I think you made some changes between your last CI run and posting? > A ton of platforms (basically anything with EFI_LOADER disabled) fail to > build now because the two "temp" commits at the end of the series are > always enabled. I took those commits out and ran my world build size > check tests and the results are at: > https://gist.github.com/trini/d42bd392463c39766a5f872c190ccf27 > > And 64bit ARM looks very reasonable, but I wonder if we can improve the > 32bit ARM results? > > I also did an every commit run for a 32bit ARM board without EFI_LOADER > (which is going to be the case for size constrained systems) and see > that the series isn't bisect'able to start with, so please fix that for > the next go-round. That said: Yes, I should have mentioned it in the cover letter about issues with bisectability, primarily because some code under boot/ uses the lmb API's unconditionally, without having a check for LMB being enabled or not. > Summary of 30 commits for 1 boards (1 thread, 12 jobs per thread) > 01: Added arm64 assembly for examples/api crt0 > 02: lmb: remove the unused lmb_is_reserved() function > 03: lmb: staticize __lmb_alloc_base() > arm: (for 1/1 boards) all -16.0 text -16.0 > omapl138_lcdk : all -16 text -16 > u-boot: add: 0/-1, grow: 1/0 bytes: 172/-186 (-14) > function old new delta > lmb_alloc_base 40 212 +172 > __lmb_alloc_base 186 - -186 > 04: lmb: make the lmb reservations persistent > arm: + omapl138_lcdk > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': > +(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': > +(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': > +(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': > +(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; > +(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': > +(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; > +(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': > +(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); > +(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, > +(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': > +(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); > +(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { > +(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { > +(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; > +(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': > +(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; > +(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) > +(omapl138_lcdk) | ^ > +(omapl138_lcdk) | -> > +(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? > +(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 > +(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 > +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 > +(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 > +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > 05: lmb: remove local instances of the lmb structure variable > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': > -(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': > -(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': > -(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': > -(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; > -(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': > -(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; > -(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': > -(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); > -(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, > -(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': > -(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); > -(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { > -(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { > -(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; > -(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': > -(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; > -(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) > -(omapl138_lcdk) | ^ > -(omapl138_lcdk) | -> > -(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? > -(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 > -(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 > -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 > +(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': > +(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) > +(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); > +(omapl138_lcdk) | ^~~ > +(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 > +(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 > +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 > 06: lmb: pass a flag to image_setup_libfdt() for lmb reservations > arm: omapl138_lcdk > -(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': > -(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) > -(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); > -(omapl138_lcdk) | ^~~ > -(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 > -(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 > -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 > -(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 > -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > 07: lmb: reserve and add common memory regions post relocation > arm: + omapl138_lcdk > +(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': > +(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] > +(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); > +(omapl138_lcdk) | ^~~~~~~~~~~~~~ > +(omapl138_lcdk) cc1: all warnings being treated as errors > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 > +(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 > +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > 08: lmb: remove lmb_init_and_reserve_range() function > 09: lmb: replcace the lmb_init_and_reserve() function > arm: omapl138_lcdk > -(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': > -(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] > -(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); > -(omapl138_lcdk) | ^~~~~~~~~~~~~~ > -(omapl138_lcdk) cc1: all warnings being treated as errors > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 > -(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 > -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > 10: lmb: allow for resizing lmb regions > arm: (for 1/1 boards) all +224.0 text +224.0 > omapl138_lcdk : all +224 text +224 > u-boot: add: 0/0, grow: 1/0 bytes: 224/0 (224) > function old new delta > lmb_add_region_flags 416 640 +224 > 11: event: add events to notify memory map changes > 12: lib: Kconfig: add a config symbol for getting memory map updates > 13: add a function to check if an address is in RAM memory > 14: efi_memory: notify of any changes to the EFI memory map > 15: lmb: notify of any changes to the LMB memory map > arm: (for 1/1 boards) all +8.0 text +8.0 > omapl138_lcdk : all +8 text +8 > u-boot: add: 0/0, grow: 2/0 bytes: 12/0 (12) > function old new delta > lmb_reserve_flags 32 40 +8 > lmb_free 192 196 +4 > 16: efi_memory: add an event handler to update memory map > 17: lmb: add an event handler to update memory map > arm: (for 1/1 boards) all +349.0 rodata +61.0 text +288.0 > omapl138_lcdk : all +349 rodata +61 text +288 > u-boot: add: 3/0, grow: 3/0 bytes: 232/0 (232) > function old new delta > event_notify - 104 +104 > initcall_run_list 96 144 +48 > image_setup_libfdt 236 284 +48 > event_notify_null - 18 +18 > event_type_name - 8 +8 > run_main_loop 10 16 +6 > 18: lmb: remove call to efi_lmb_reserve() > 19: sandbox: iommu: remove lmb allocation in the driver > 20: zynq: lmb: do not add to lmb map before relocation > 21: test: cedit: use allocated address for reading file > 22: test: event: update the expected event dump output > 23: test: lmb: run the LMB tests only on sandbox > 24: test: lmb: initialise the lmb structure before tests > 25: test: lmb: add a test case for checking overlapping region add > 26: test: lmb: adjust the test case to handle overlapping regions > 27: test: lmb: run lmb tests only manually > 28: test: bdinfo: dump the global LMB memory map > 29: cmd: bdinfo: only dump the current LMB memory > arm: (for 1/1 boards) all -8.0 text -8.0 > omapl138_lcdk : all -8 text -8 > u-boot: add: 0/0, grow: 0/-1 bytes: 0/-8 (-8) > function old new delta > do_bdinfo 472 464 -8 > 30: temp: mx6sabresd: bump up the size limit of the board > > So my first thought is, do we really need the event notifier framework > in the case where it's NOT also using EFI_LOADER? Or is perhaps that > Kconfig logic not quite right? So, I had actually tried putting all the notification code under the newly introduced config symbol, but I get build warnings on sandbox spl and sandbox vpl builds, which result in CI failures. I would have thought that having the function call under a CONFIG_IS_ENABLED check would result in the linker to discard the function call. But that is not happening. I will check if this can be handled by introducing SPL, TPL and VPL variants of this config. -sughosh > > -- > Tom
On Tue, Jun 11, 2024 at 02:31:27PM +0530, Sughosh Ganu wrote: > On Tue, 11 Jun 2024 at 02:35, Tom Rini <trini@konsulko.com> wrote: > > > > On Sat, Jun 08, 2024 at 12:22:09AM +0530, Sughosh Ganu wrote: > > > > > The aim of this patch series is to fix the current state of > > > incoherence between modules when it comes to memory usage. The primary > > > issue that this series is trying to fix is that the EFI memory module > > > which is responsible for allocating and freeing memory, does not have > > > any visibility of the memory that is being used by the LMB > > > module. This is further complicated by the fact that the LMB > > > allocations are caller specific -- the LMB memory map is not global > > > nor persistent. This means that the memory "allocated" by the LMB > > > module might be relevant only for a given function. Hence one of the > > > requirements for making the memory usage visible across modules is to > > > make LMB allocations persistent and global, and then have means to > > > communicate the use of memory across modules. > > > > > > The first set of patches in this series work on making the LMB memory > > > map persistent and global. This is being done keeping in mind the > > > usage of LMB memory by platforms where the same memory region can be > > > used to load multiple different images. What is not allowed is to > > > overwrite memory that has been allocated by the other module, > > > currently the EFI memory module. This is being achieved by introducing > > > a new flag, LMB_NOOVERWRITE, which represents memory which cannot be > > > re-requested once allocated. > > > > > > The second set of patches are then introducing a notification > > > mechanism to indicate any changes to a respective module's memory > > > map. This way, any memory allocation/reservation by a module gets > > > notified to any interested listners, who then update their memory map > > > accordingly, thus keeping memory usage coherent. > > > > > > Todo's > > > ------ > > > I have run these patches through CI, but the LMB unit tests need an > > > overhaul. I have currently marked these tests for manual run, as > > > running these would obviously tamper the LMB memory map, thus > > > affecting subsequent tests. The current set of LMB tests are written > > > with the assumption of local LMB memory maps. This needs to be > > > reworked. > > > > > > Secondly, there needs to be a test written for testing the various > > > scenarios of memory being allocated and freed by different modules, > > > namely LMB and EFI. I have written a couple of commands for testing > > > the changes that I have made -- I have also included these temporary > > > commands to assist anyone who might want to test these changes. But I > > > will be working on adding a more comprehensive test. > > > > > > Lastly, as the series touches common code, there is obviously an > > > increase in the size of the image, moreover since the LMB memory is > > > now persistent, and the variables do not reside on the stack. I want > > > to check if there can be ways of decreasing the size impact of the > > > changes. > > > > So, I think you made some changes between your last CI run and posting? > > A ton of platforms (basically anything with EFI_LOADER disabled) fail to > > build now because the two "temp" commits at the end of the series are > > always enabled. I took those commits out and ran my world build size > > check tests and the results are at: > > https://gist.github.com/trini/d42bd392463c39766a5f872c190ccf27 > > > > And 64bit ARM looks very reasonable, but I wonder if we can improve the > > 32bit ARM results? > > > > I also did an every commit run for a 32bit ARM board without EFI_LOADER > > (which is going to be the case for size constrained systems) and see > > that the series isn't bisect'able to start with, so please fix that for > > the next go-round. That said: > > Yes, I should have mentioned it in the cover letter about issues with > bisectability, primarily because some code under boot/ uses the lmb > API's unconditionally, without having a check for LMB being enabled or > not. Yeah, today at least LMB functionally should be a def_bool y instead of a choice. > > Summary of 30 commits for 1 boards (1 thread, 12 jobs per thread) > > 01: Added arm64 assembly for examples/api crt0 > > 02: lmb: remove the unused lmb_is_reserved() function > > 03: lmb: staticize __lmb_alloc_base() > > arm: (for 1/1 boards) all -16.0 text -16.0 > > omapl138_lcdk : all -16 text -16 > > u-boot: add: 0/-1, grow: 1/0 bytes: 172/-186 (-14) > > function old new delta > > lmb_alloc_base 40 212 +172 > > __lmb_alloc_base 186 - -186 > > 04: lmb: make the lmb reservations persistent > > arm: + omapl138_lcdk > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': > > +(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': > > +(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': > > +(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': > > +(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; > > +(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': > > +(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; > > +(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': > > +(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); > > +(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, > > +(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': > > +(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); > > +(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { > > +(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { > > +(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; > > +(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': > > +(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; > > +(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) > > +(omapl138_lcdk) | ^ > > +(omapl138_lcdk) | -> > > +(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? > > +(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; > > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 > > +(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 > > +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 > > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 > > +(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 > > +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > > 05: lmb: remove local instances of the lmb structure variable > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_dump_all_force': > > -(omapl138_lcdk) lib/lmb.c:64:29: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 64 | lmb_dump_region(&lmb.memory, "memory"); > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:65:29: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 65 | lmb_dump_region(&lmb.reserved, "reserved"); > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_add': > > -(omapl138_lcdk) lib/lmb.c:356:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 356 | struct lmb_region *_rgn = &lmb.memory; > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_free': > > -(omapl138_lcdk) lib/lmb.c:363:38: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 363 | struct lmb_region *rgn = &lmb.reserved; > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_reserve_flags': > > -(omapl138_lcdk) lib/lmb.c:414:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 414 | struct lmb_region *_rgn = &lmb.reserved; > > -(omapl138_lcdk) lib/lmb.c: In function '__lmb_alloc_base': > > -(omapl138_lcdk) lib/lmb.c:451:21: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 451 | for (i = lmb.memory.cnt - 1; i >= 0; i--) { > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:452:42: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 452 | phys_addr_t lmbbase = lmb.memory.region[i].base; > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:453:42: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 453 | phys_size_t lmbsize = lmb.memory.region[i].size; > > -(omapl138_lcdk) lib/lmb.c:469:55: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 469 | rgn = lmb_overlaps_region(&lmb.reserved, base, size); > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:472:56: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 472 | if (lmb_add_region(&lmb.reserved, base, > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:477:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 477 | res_base = lmb.reserved.region[rgn].base; > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_alloc_addr': > > -(omapl138_lcdk) lib/lmb.c:513:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 513 | rgn = lmb_overlaps_region(&lmb.memory, base, size); > > -(omapl138_lcdk) lib/lmb.c:519:42: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 519 | if (lmb_addrs_overlap(lmb.memory.region[rgn].base, > > -(omapl138_lcdk) lib/lmb.c:520:42: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 520 | lmb.memory.region[rgn].size, > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_get_free_size': > > -(omapl138_lcdk) lib/lmb.c:537:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 537 | rgn = lmb_overlaps_region(&lmb.memory, addr, 1); > > -(omapl138_lcdk) lib/lmb.c:539:36: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 539 | for (i = 0; i < lmb.reserved.cnt; i++) { > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:540:39: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 540 | if (addr < lmb.reserved.region[i].base) { > > -(omapl138_lcdk) lib/lmb.c:542:43: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 542 | return lmb.reserved.region[i].base - addr; > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:544:32: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 544 | if (lmb.reserved.region[i].base + > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:545:32: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 545 | lmb.reserved.region[i].size > addr) { > > -(omapl138_lcdk) lib/lmb.c:551:27: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 551 | return lmb.memory.region[lmb.memory.cnt - 1].base + > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:551:45: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:552:27: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 552 | lmb.memory.region[lmb.memory.cnt - 1].size - addr; > > -(omapl138_lcdk) lib/lmb.c:552:45: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) lib/lmb.c: In function 'lmb_is_reserved_flags': > > -(omapl138_lcdk) lib/lmb.c:561:28: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 561 | for (i = 0; i < lmb.reserved.cnt; i++) { > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:562:40: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 562 | phys_addr_t upper = lmb.reserved.region[i].base + > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:563:28: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 563 | lmb.reserved.region[i].size - 1; > > -(omapl138_lcdk) lib/lmb.c:564:33: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 564 | if ((addr >= lmb.reserved.region[i].base) && (addr <= upper)) > > -(omapl138_lcdk) | ^ > > -(omapl138_lcdk) | -> > > -(omapl138_lcdk) lib/lmb.c:565:36: error: 'lmb' is a pointer; did you mean to use '->'? > > -(omapl138_lcdk) 565 | return (lmb.reserved.region[i].flags & flags) == flags; > > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: lib/lmb.o] Error 1 > > -(omapl138_lcdk) make[1]: *** [Makefile:1892: lib] Error 2 > > -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:256: spl/lib/lmb.o] Error 1 > > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/lib] Error 2 > > +(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': > > +(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) > > +(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); > > +(omapl138_lcdk) | ^~~ > > +(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in > > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 > > +(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 > > +(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 > > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 > > 06: lmb: pass a flag to image_setup_libfdt() for lmb reservations > > arm: omapl138_lcdk > > -(omapl138_lcdk) boot/image-board.c: In function 'image_setup_linux': > > -(omapl138_lcdk) boot/image-board.c:907:65: error: 'lmb' undeclared (first use in this function) > > -(omapl138_lcdk) 907 | ret = image_setup_libfdt(images, *of_flat_tree, lmb); > > -(omapl138_lcdk) | ^~~ > > -(omapl138_lcdk) boot/image-board.c:907:65: note: each undeclared identifier is reported only once for each function it appears in > > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: boot/image-board.o] Error 1 > > -(omapl138_lcdk) make[1]: *** [Makefile:1892: boot] Error 2 > > -(omapl138_lcdk) make[3]: *** [scripts/Makefile.build:257: spl/boot/image-board.o] Error 1 > > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.spl:538: spl/boot] Error 2 > > -(omapl138_lcdk) make[1]: *** [Makefile:2092: spl/u-boot-spl] Error 2 > > -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > > 07: lmb: reserve and add common memory regions post relocation > > arm: + omapl138_lcdk > > +(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': > > +(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] > > +(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); > > +(omapl138_lcdk) | ^~~~~~~~~~~~~~ > > +(omapl138_lcdk) cc1: all warnings being treated as errors > > +(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 > > +(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 > > +(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > > 08: lmb: remove lmb_init_and_reserve_range() function > > 09: lmb: replcace the lmb_init_and_reserve() function > > arm: omapl138_lcdk > > -(omapl138_lcdk) common/board_r.c: In function 'initr_lmb': > > -(omapl138_lcdk) common/board_r.c:564:9: error: implicit declaration of function 'lmb_add_memory' [-Werror=implicit-function-declaration] > > -(omapl138_lcdk) 564 | lmb_add_memory(gd->bd); > > -(omapl138_lcdk) | ^~~~~~~~~~~~~~ > > -(omapl138_lcdk) cc1: all warnings being treated as errors > > -(omapl138_lcdk) make[2]: *** [scripts/Makefile.build:256: common/board_r.o] Error 1 > > -(omapl138_lcdk) make[1]: *** [Makefile:1892: common] Error 2 > > -(omapl138_lcdk) make: *** [Makefile:177: sub-make] Error 2 > > 10: lmb: allow for resizing lmb regions > > arm: (for 1/1 boards) all +224.0 text +224.0 > > omapl138_lcdk : all +224 text +224 > > u-boot: add: 0/0, grow: 1/0 bytes: 224/0 (224) > > function old new delta > > lmb_add_region_flags 416 640 +224 > > 11: event: add events to notify memory map changes > > 12: lib: Kconfig: add a config symbol for getting memory map updates > > 13: add a function to check if an address is in RAM memory > > 14: efi_memory: notify of any changes to the EFI memory map > > 15: lmb: notify of any changes to the LMB memory map > > arm: (for 1/1 boards) all +8.0 text +8.0 > > omapl138_lcdk : all +8 text +8 > > u-boot: add: 0/0, grow: 2/0 bytes: 12/0 (12) > > function old new delta > > lmb_reserve_flags 32 40 +8 > > lmb_free 192 196 +4 > > 16: efi_memory: add an event handler to update memory map > > 17: lmb: add an event handler to update memory map > > arm: (for 1/1 boards) all +349.0 rodata +61.0 text +288.0 > > omapl138_lcdk : all +349 rodata +61 text +288 > > u-boot: add: 3/0, grow: 3/0 bytes: 232/0 (232) > > function old new delta > > event_notify - 104 +104 > > initcall_run_list 96 144 +48 > > image_setup_libfdt 236 284 +48 > > event_notify_null - 18 +18 > > event_type_name - 8 +8 > > run_main_loop 10 16 +6 > > 18: lmb: remove call to efi_lmb_reserve() > > 19: sandbox: iommu: remove lmb allocation in the driver > > 20: zynq: lmb: do not add to lmb map before relocation > > 21: test: cedit: use allocated address for reading file > > 22: test: event: update the expected event dump output > > 23: test: lmb: run the LMB tests only on sandbox > > 24: test: lmb: initialise the lmb structure before tests > > 25: test: lmb: add a test case for checking overlapping region add > > 26: test: lmb: adjust the test case to handle overlapping regions > > 27: test: lmb: run lmb tests only manually > > 28: test: bdinfo: dump the global LMB memory map > > 29: cmd: bdinfo: only dump the current LMB memory > > arm: (for 1/1 boards) all -8.0 text -8.0 > > omapl138_lcdk : all -8 text -8 > > u-boot: add: 0/0, grow: 0/-1 bytes: 0/-8 (-8) > > function old new delta > > do_bdinfo 472 464 -8 > > 30: temp: mx6sabresd: bump up the size limit of the board > > > > So my first thought is, do we really need the event notifier framework > > in the case where it's NOT also using EFI_LOADER? Or is perhaps that > > Kconfig logic not quite right? > > So, I had actually tried putting all the notification code under the > newly introduced config symbol, but I get build warnings on sandbox > spl and sandbox vpl builds, which result in CI failures. I would have > thought that having the function call under a CONFIG_IS_ENABLED check > would result in the linker to discard the function call. But that is > not happening. I will check if this can be handled by introducing SPL, > TPL and VPL variants of this config. It's also possible that some stages aren't passing -ffunction-sections/-fdata-sections/--gc-sections ? A testing option would be to enable (and be sure it's used) LTO ?
Hi Sughosh, On Fri, 7 Jun 2024 at 12:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > The aim of this patch series is to fix the current state of > incoherence between modules when it comes to memory usage. The primary > issue that this series is trying to fix is that the EFI memory module > which is responsible for allocating and freeing memory, does not have > any visibility of the memory that is being used by the LMB > module. This is further complicated by the fact that the LMB > allocations are caller specific -- the LMB memory map is not global > nor persistent. This means that the memory "allocated" by the LMB > module might be relevant only for a given function. Hence one of the > requirements for making the memory usage visible across modules is to > make LMB allocations persistent and global, and then have means to > communicate the use of memory across modules. > > The first set of patches in this series work on making the LMB memory > map persistent and global. For the purposes of testing at least, it should be possible to create a new LMB. You could have a global_data pointer to the current (global) one, like we do with driver model. > This is being done keeping in mind the > usage of LMB memory by platforms where the same memory region can be > used to load multiple different images. What is not allowed is to > overwrite memory that has been allocated by the other module, > currently the EFI memory module. This is being achieved by introducing > a new flag, LMB_NOOVERWRITE, which represents memory which cannot be > re-requested once allocated. > > The second set of patches are then introducing a notification > mechanism to indicate any changes to a respective module's memory > map. This way, any memory allocation/reservation by a module gets > notified to any interested listners, who then update their memory map > accordingly, thus keeping memory usage coherent. What is the need for this listener? > > Todo's > ------ > I have run these patches through CI, but the LMB unit tests need an > overhaul. I have currently marked these tests for manual run, as > running these would obviously tamper the LMB memory map, thus > affecting subsequent tests. The current set of LMB tests are written > with the assumption of local LMB memory maps. This needs to be > reworked. See above > > Secondly, there needs to be a test written for testing the various > scenarios of memory being allocated and freed by different modules, > namely LMB and EFI. I have written a couple of commands for testing > the changes that I have made -- I have also included these temporary > commands to assist anyone who might want to test these changes. But I > will be working on adding a more comprehensive test. > > Lastly, as the series touches common code, there is obviously an > increase in the size of the image, moreover since the LMB memory is > now persistent, and the variables do not reside on the stack. I want > to check if there can be ways of decreasing the size impact of the > changes. Perhaps some part of the feature set could be made optional? Regards, Simon > > > Sughosh Ganu (31): > lmb: remove the unused lmb_is_reserved() function > lmb: staticize __lmb_alloc_base() > lmb: make the lmb reservations persistent > lmb: remove local instances of the lmb structure variable > lmb: pass a flag to image_setup_libfdt() for lmb reservations > lmb: reserve and add common memory regions post relocation > lmb: remove lmb_init_and_reserve_range() function > lmb: replcace the lmb_init_and_reserve() function > lmb: allow for resizing lmb regions > event: add events to notify memory map changes > lib: Kconfig: add a config symbol for getting memory map updates > add a function to check if an address is in RAM memory > efi_memory: notify of any changes to the EFI memory map > lmb: notify of any changes to the LMB memory map > efi_memory: add an event handler to update memory map > lmb: add an event handler to update memory map > lmb: remove call to efi_lmb_reserve() > sandbox: iommu: remove lmb allocation in the driver > zynq: lmb: do not add to lmb map before relocation > test: cedit: use allocated address for reading file > test: event: update the expected event dump output > test: lmb: run the LMB tests only on sandbox > test: lmb: initialise the lmb structure before tests > test: lmb: add a test case for checking overlapping region add > test: lmb: adjust the test case to handle overlapping regions > test: lmb: run lmb tests only manually > test: bdinfo: dump the global LMB memory map > cmd: bdinfo: only dump the current LMB memory > temp: mx6sabresd: bump up the size limit of the board > temp: cmd: efi_mem: add a command to test efi alloc/free > temp: cmd: efi: add a command to dump EFI memory map > > arch/arc/lib/cache.c | 4 +- > arch/arm/lib/stack.c | 4 +- > arch/arm/mach-apple/board.c | 17 +- > arch/arm/mach-snapdragon/board.c | 17 +- > arch/arm/mach-stm32mp/dram_init.c | 8 +- > arch/arm/mach-stm32mp/stm32mp1/cpu.c | 6 +- > arch/m68k/lib/bootm.c | 7 +- > arch/microblaze/lib/bootm.c | 4 +- > arch/mips/lib/bootm.c | 11 +- > arch/nios2/lib/bootm.c | 4 +- > arch/powerpc/cpu/mpc85xx/mp.c | 4 +- > arch/powerpc/include/asm/mp.h | 4 +- > arch/powerpc/lib/bootm.c | 14 +- > arch/riscv/lib/bootm.c | 4 +- > arch/sandbox/cpu/cpu.c | 5 + > arch/sh/lib/bootm.c | 4 +- > arch/x86/lib/bootm.c | 4 +- > arch/xtensa/lib/bootm.c | 4 +- > board/xilinx/common/board.c | 33 -- > boot/bootm.c | 26 +- > boot/bootm_os.c | 5 +- > boot/image-board.c | 34 +-- > boot/image-fdt.c | 36 +-- > cmd/Makefile | 2 + > cmd/bdinfo.c | 5 +- > cmd/booti.c | 2 +- > cmd/bootz.c | 2 +- > cmd/efi_map_dump.c | 28 ++ > cmd/efi_memory.c | 155 ++++++++++ > cmd/elf.c | 2 +- > cmd/load.c | 7 +- > common/board_r.c | 20 ++ > common/event.c | 4 + > configs/mx6sabresd_defconfig | 2 +- > drivers/iommu/apple_dart.c | 8 +- > drivers/iommu/sandbox_iommu.c | 17 +- > fs/fs.c | 7 +- > include/efi_loader.h | 2 + > include/event.h | 28 ++ > include/image.h | 27 +- > include/lmb.h | 96 +++--- > lib/Kconfig | 10 + > lib/efi_loader/efi_dt_fixup.c | 2 +- > lib/efi_loader/efi_helper.c | 2 +- > lib/efi_loader/efi_memory.c | 127 +++++++- > lib/lmb.c | 442 ++++++++++++++++++--------- > net/tftp.c | 5 +- > net/wget.c | 5 +- > test/boot/cedit.c | 5 +- > test/cmd/bdinfo.c | 22 +- > test/lib/Makefile | 2 +- > test/lib/lmb.c | 274 +++++++++-------- > test/py/tests/test_event_dump.py | 2 + > 53 files changed, 1001 insertions(+), 570 deletions(-) > create mode 100644 cmd/efi_map_dump.c > create mode 100644 cmd/efi_memory.c > > -- > 2.34.1 > >