Change in coreboot[master]: Makefile.inc: Generate master header and pointer as C structs
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 6:
(8 comments)
Patchset:
PS4:
but Libpayload needs to support FMAP for that.
It does now, for the new CBFS APIs (e.g. cbfs_map()). We still left the old APIs untouched so that payloads have a grace period to migrate (and I guess we should probably start announcing that on the mailing list), but we did migrate depthcharge, and I doubt any other payload actually cared about reading non-primary CBFSes (which IIRC was the only place where the master header still mattered?). So I think anything using libpayload should be good now. But that still leaves the GRUB and SeaBIOS concerns?
Patchset:
PS6: Not sure if you have anything that's blocked by this work, but considering that we seem to be so close to just drop the master header completely, wouldn't it be easier to wait for that?
File src/arch/x86/header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/d848b0b3_7fa7e91c PS6, Line 7: __attribute__((used, __section__(".header_pointer"))) const uint32_t header_pointer = Wouldn't it be simpler to just merge this with the src/lib version and wrap the section attribute into an #if CONFIG(ARCH_X86)?
File src/lib/cbfs_master_header.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/bcb35be1_02b046cb PS6, Line 10: FMAP_SECTION_FLASH_SIZE I believe this needs to be
(FMAP_SECTION_COREBOOT_START + FMAP_SECTION_COREBOOT_SIZE)
(which is not guaranteed to be the same, e.g. on Arm systems where the CBFS is not at the end of flash).
https://review.coreboot.org/c/coreboot/+/59132/comment/99901f38_3afd98b3 PS6, Line 11: .bootblocksize = cpu_to_be32(4), This should probably have a comment about why this is 4... maybe copy the one from cbfstool (although it's also not very clear on this... I think the missing piece of information is that this is _not_ in fact the actual size of the bootblock anymore, but is only used to tell the legacy CBFS reader where the CBFS ends, and we don't want to let that go up right to the 4GB boundary for the reasons mentioned in cbfstool).
https://review.coreboot.org/c/coreboot/+/59132/comment/8923626c_ddab965e PS6, Line 12: .align = cpu_to_be32(64), You should be able to use the CBFS_ALIGNMENT constant here.
https://review.coreboot.org/c/coreboot/+/59132/comment/9d65b653_35aa5bf7 PS6, Line 13: FMAP_SECTION_FLASH_START Isn't this just 0? Rather than assuming that all FMAPs are named "FLASH", I think it's easier to just leave this out?
File src/lib/master_header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/c66388fe_84427964 PS6, Line 7: __attribute((used)) I don't think you need this? We've never needed it for other structs...
(Also, I don't think __attribute(()) compiles without the __ on the other end?)
-
Julius Werner (Code Review)