Marty E. Plummer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32392
Change subject: src/Kconfig: increase size if using flattened image tree ......................................................................
src/Kconfig: increase size if using flattened image tree
FIT support takes more heap memory than most coreboot payloads.
Change-Id: Id17f25e94d97e937b0e9a9cee3dd1a8aef1d525d Signed-off-by: Marty E. Plummer hanetzer@startmail.com --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32392/1
diff --git a/src/Kconfig b/src/Kconfig index 62b3818..90c724e 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -428,6 +428,7 @@
config HEAP_SIZE hex + default 0x100000 if FLATTENED_DEVICE_TREE default 0x4000
config STACK_SIZE
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase size if using flattened image tree ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32392/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32392/1//COMMIT_MSG@7 PS1, Line 7: nit: *heap* size is the important part
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32392
to look at the new patch set (#2).
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
src/Kconfig: increase heap size if using flattened image tree
FIT support takes more heap memory than most coreboot payloads.
Change-Id: Id17f25e94d97e937b0e9a9cee3dd1a8aef1d525d Signed-off-by: Marty E. Plummer hanetzer@startmail.com --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32392/2
Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32392/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32392/1//COMMIT_MSG@7 PS1, Line 7:
nit: *heap* size is the important part
Fixed. I'm pretty sure I typed that off the getgo but may have fat-fingered some stuff in vim.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
src/Kconfig: increase heap size if using flattened image tree
FIT support takes more heap memory than most coreboot payloads.
Change-Id: Id17f25e94d97e937b0e9a9cee3dd1a8aef1d525d Signed-off-by: Marty E. Plummer hanetzer@startmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32392 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/Kconfig b/src/Kconfig index 62b3818..90c724e 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -428,6 +428,7 @@
config HEAP_SIZE hex + default 0x100000 if FLATTENED_DEVICE_TREE default 0x4000
config STACK_SIZE
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
1M is really a lot and wouldn't fit in most ramstages. Is that much heap really needed or should the linker scripts be adapted?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
1M is really a lot and wouldn't fit in most ramstages. Is that much heap really needed or should the linker scripts be adapted?
It's lib/malloc.c, just print (uintptr_t)free_mem_end_ptr - (uinptr_t)free_mem_ptr before entry to payload to find out actual usage. Doing that in payload_run() after checkstack() would make some sense in any case.
While HEAP_SIZE is adjusted based on FLATTENED_DEVICE_TREE (and lib/device_tree.c), maybe majority of heap requirement comes from PAYLOAD_FIT_SUPPORT, unpack_fdt() in lib/fit_payload.c instead.
IMHO it is not particularly clean if payload does references back to ramstage heap.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
1M is really a lot and wouldn't fit in most ramstages. Is that much heap really needed or should the linker scripts be adapted?
I'm confused by the "wouldn't fit" comment. Can you please explain under what conditions something won't fit?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
1M is really a lot and wouldn't fit in most ramstages. Is that much heap really needed or should the linker scripts be adapted?
I'm confused by the "wouldn't fit" comment. Can you please explain under what conditions something won't fit?
If the heap is too large the ramstage can exceed the allotted size, resulting in an assertion error during the linking ("Ramstage exceeded its allotted size! (sz)" in memlayout.h). A lot of arm SOC have < 1M allotted for ramstage which prevents FDT support without increasing it or specifying a smaller heap size.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
1M is really a lot and wouldn't fit in most ramstages. Is that much heap really needed or should the linker scripts be adapted?
I'm confused by the "wouldn't fit" comment. Can you please explain under what conditions something won't fit?
If the heap is too large the ramstage can exceed the allotted size, resulting in an assertion error during the linking ("Ramstage exceeded its allotted size! (sz)" in memlayout.h). A lot of arm SOC have < 1M allotted for ramstage which prevents FDT support without increasing it or specifying a smaller heap size.
So it sounds like we need a way to tie in the Kconfig to this allocation (if possible). Have you looked into various options?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
So it sounds like we need a way to tie in the Kconfig to this allocation (if possible). Have you looked into various options?
In the old days of bounce buffers and non-relocatable ramstage on x86, it would have been fragile for payload to access back to ramstage program. If I read the source correctly, ramstage heap became part of the hand-of-buffer with FIT payloads. Do we really want to allow that?
I think lib/device_tree.c and lib/fit_payload.c should malloc() from a completely separate heap.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
So it sounds like we need a way to tie in the Kconfig to this allocation (if possible). Have you looked into various options?
In the old days of bounce buffers and non-relocatable ramstage on x86, it would have been fragile for payload to access back to ramstage program. If I read the source correctly, ramstage heap became part of the hand-of-buffer with FIT payloads. Do we really want to allow that?
I think lib/device_tree.c and lib/fit_payload.c should malloc() from a completely separate heap.
I'm not up on the details of this sharing. Is the region just referenced or is it reused for its own malloc implementation?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
So it sounds like we need a way to tie in the Kconfig to this allocation (if possible). Have you looked into various options?
In the old days of bounce buffers and non-relocatable ramstage on x86, it would have been fragile for payload to access back to ramstage program. If I read the source correctly, ramstage heap became part of the hand-of-buffer with FIT payloads. Do we really want to allow that?
I think lib/device_tree.c and lib/fit_payload.c should malloc() from a completely separate heap.
I'm not up on the details of this sharing. Is the region just referenced or is it reused for its own malloc implementation?
NVM. After a second read, it appears the flattened devicetree is the handoff buffer, and it is located outside ramstage heap. So FIT payload does not reference back to ramstage heap.
Instead of attempting to adjust ARM memlayouts, might still be easier to have (unpacked) devicetree carved from any BM_MEM_RAM, it only has lifetime of fit_payload() and memory could be freed right after pack_fdt(), even before loading kernel and initrd.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32392 )
Change subject: src/Kconfig: increase heap size if using flattened image tree ......................................................................
Patch Set 3:
For most (all?) Arm SoCs, there's no real reason the RAMSTAGE area is so small in memlayout. There's a lot of RAM available so it might well be larger. We just didn't have any need for that much space back when they were added, but then the FIT payload code came along which may need a decent chunk of scratch space (depending on how big of an FDT/initramfs you're loading). So if you have a problem on a specific SoC, just submit a change to increase the RAMSTAGE area (or manually configure a smaller HEAP_SIZE if that's enough for what you're trying to load).
So there's no real problem here, it's just that you may need to statically resize some things to make everything fit. There's more than enough space, usually, coreboot is just not good at placing stuff automatically for you so you need to tell it where what goes. We could tie the FIT scratch space to something like bootmem_allocate_buffer() instead, but then you trade the configuration hassle for the risk that something may end up overlapping later (e.g. if bootmem ends up grabbing the range that the payload wants to be loaded at later, you have a problem).
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marty E. Plummer (Code Review)