Change in coreboot[master]: Makefile.inc: Generate master header and pointer as C structs
Attention is currently required from: Arthur Heymans, Martin Roth, Julius Werner, Yu-Ping Wu. Arthur Heymans 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:
(4 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?
Cool. I did send some patches for SeaBIOS some time ago. I guess I can add a Kconfig variable with a deprecated warning to optionally leave it out.
File src/arch/x86/header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/b9f3e245_99c4a8d2 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)?
good idea.
File src/lib/cbfs_master_header.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/c36923b8_d2a73a58 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?
No it's often the base if the whole flash was memory mapped. Maybe it's worth dropping that usecase and force it to be 0 accross the tree as memory mapping is handled better now that there is no need for this.
File src/lib/master_header_pointer.c:
https://review.coreboot.org/c/coreboot/+/59132/comment/173a25cc_136ed574 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?)
right.
-
Arthur Heymans (Code Review)