Attention is currently required from: Arthur Heymans, Martin Roth, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59132 )
Change subject: Makefile.inc: Generate master header and pointer as C structs ......................................................................
Patch Set 9:
(6 comments)
File src/arch/x86/bootblock.ld:
https://review.coreboot.org/c/coreboot/+/59132/comment/eea6e6e2_8fc93454 PS7, Line 77: . = 0xfffffffc; : .header_pointer . : { : KEEP(*(.header_pointer)); : }
This needs to be smarter. It looks like the reset vector is wrong when header_pointer is missing.
The file is probably 4 bytes too short in that case? I think maybe this could do what you want:
. = 0xfffffffc; .header_pointer . : { KEEP(*(.header_pointer)); FILL(0xffffffff); . = 4; }
Then again, why would header_pointer be missing? Don't we always have it? Then this shouldn't be a problem.
edit: Okay, I guess that works too.
File src/lib/cbfs_master_header.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/42d2b82a_992623f2 PS9, Line 7: __attribute((used)) Same problem here as the other one.
https://review.coreboot.org/c/coreboot/+/59132/comment/927f5f73_c9c1bedf PS9, Line 10: /* The offset and romsize fields within the master header are absolute nit: https://doc.coreboot.org/contributing/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/59132/comment/6775785f_1eeeb8af PS9, Line 11: relfect typo
https://review.coreboot.org/c/coreboot/+/59132/comment/3154cfba_cd6cae21 PS9, Line 16: FMAP_SECTION_COREBOOT_START + FMAP_SECTION_COREBOOT_SIZE Okay, I figured out how that memory-mapped offset thing in these headers works now (and it's honestly pretty horrible, we should try getting rid of that. It seems completely arbitrary which .fmd files define a FLASH@xxx and which don't. Most of our APIs that take a flash offset (e.g. boot_device_ro()) would not know how to deal with a memory-mapped offset like that. I think we only get away with that because we barely use these header constants anymore anyway). But then this line is a problem, because if it's using the memory-mapped offsets this would be a huge number. So you also need to subtract FMAP_SECTION_FLASH_START here again.
File src/lib/master_header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/07d89e25_5f819cfd PS7, Line 13: ATTRIBUTES uint32_t header_pointer = I think an easier way to write this is
#if ENV_X86 __attribute__((__section__(".header_pointer"))) #endif uint32_t header_pointer = ...;