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. -- To view, visit https://review.coreboot.org/c/coreboot/+/59132 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3ba01be7da1f09a8cac287751497c18cda97d293 Gerrit-Change-Number: 59132 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Sean Rhodes <sean@starlabs.systems> Gerrit-Reviewer: Yu-Ping Wu <yupingso@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-Attention: Martin Roth <martinroth@google.com> Gerrit-Attention: Julius Werner <jwerner@chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso@google.com> Gerrit-Comment-Date: Tue, 05 Apr 2022 18:18:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth <martinroth@google.com> Comment-In-Reply-To: Julius Werner <jwerner@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment