Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: arch/arm: Implement FIT support ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem
I guess that assumes that the payload will be loaded at _dram, which might not be the case on qemu a […]
Isn't that the point of the bootmem_walk()? The goal is to find a memory area (might as well start from the lowest possible) that isn't needed by coreboot at this point. Anything that is needed by coreboot should be reserved with a bootmem tag... for the standard stuff (e.g. ramstage image, stack) that should already be in place, but there might be some platform or arch-specific things you still need to add.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 50: /* Both of these cases (an arm32 kernel using FIT compression or being smaller than a handful of bytes) are really hard errors that should never happen (FIT compression is only meant for kernels that cannot compress themselves, like arm64). I'd maybe just error out here, or at least print an angry error message.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 107: kernel->size += DECOMP_SIZE; I still think kernel->offset = _dram + DECOMP_SIZE is better (and also makes the code intention easier to follow). The difference is that if any coreboot-reserved memory regions (e.g. the ramstage program image) are placed in that 32MB window, this version would unnecessarily push the kernel further back than it needs to be.
https://review.coreboot.org/c/coreboot/+/35410/5/src/arch/arm/fit_payload.c@... PS5, Line 121: bootmem_add_range(kernel->offset, kernel->size, BM_MEM_PAYLOAD); You're doing this twice now?