Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35410 )
Change subject: [WIP]arch/arm: Implement FIT support ......................................................................
Patch Set 2:
(6 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 29: #define SECTION_ALIGN 64 /* TODO not mentioned in kernel DOCs */ This is not a page boundary, like your comment below says. Why not just use 4*KiB? I don't think you'd ever want to place it non-page-aligned anyway.
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 43: MAX_KERNEL_SIZE >> 20); nit: is there really a point in printing out the same constant size on every boot?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 83: "boot a armv7 kernel!\n"); nit: don't break strings (and we can use 96 chars now anyway). (Also, "an armv7"?)
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 108: kernel->offset += 32 * MiB; Doesn't this mean the size you checked in fit_place_mem() isn't necessarily enough anymore (because now you subtracted 32MB)?
Why don't you just do this above
kernel->offset = (uintptr_t)_dram + 32*MiB; ...bootmem_walk...
to achieve the same thing easier?
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 121: lower_mem Again, just use _dram. All Arm platforms know where their memory starts at compile time.
https://review.coreboot.org/c/coreboot/+/35410/2/src/arch/arm/fit_payload.c@... PS2, Line 135: fdt->size Actually, I just noticed a bug in lib/fit_payload.c with this (applies to arm64 as well): we're first determining the total FDT size with dt_flat_size(), then calling fit_payload_arch(), and then calling fit_add_ramdisk() to add the ramdisk information. This will increase the final flattened FDT size. There are no further size checks on flattening so it may overrun the struct region 'fdt' there.
We should add some constant amount of slack to the dt_flag_size() when setting fdt->size, and make pack_fdt() check for overruns after packing.