Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56120 )
Change subject: Makefile.inc: Drop the cbfs master header from non-X86 ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
There was a rewrite on gerrit: https://review.coreboot.org/c/coreboot/+/15774 but it was blocked due to licensing issues...
Yeah, the licensing issue with libpayload is always a pain. Unfortunately I think the rdev API has had too many patches by different people now to make relicensing feasible. At least the new CBFS core code I wrote is all in commonlib/bsd so it should be possible to reuse there, and it's written to be able to tie into different storage backends.
Anyway, I think that patch didn't touch the parts at issue here anyway, it's more about all the stuff that happens in ram_media.c.
SeaBIOS has a patch pending to not use it. I need to write one for grub, it's not much work. I think it's worth getting rid of the header as updating a hardcoded pointer at the top of the flash is not so nice + most options in the header are pretty meaningless, like cbfs file alignment. boot_media_params has been around for a long time now too, so updating payloads to not rely on it and dropping the header is my current plan.
Hmmm... okay. I'm a bit worried about other payloads we don't know about out there, but if nobody else complains I guess we can do it. FWIW the pointer was never at the top of flash anyway, just at the top of the CBFS region (for the cases where those are not the same). So it's really just an extra file at the end.
It's ok. I'll see if it's easy to fix libpayload.
It's a big mess made by most of the libpayload code being really old (from before there were FMAP sections), and then some glue code to hooking into that in a way that can be made towork for more cases without touching the core libpayload code but doesn't make a lot of sense in itself. I think for depthcharge our cbfs_media is written so that accesses with a positive offset are treated as absolute flash offsets, but accesses with a negative (i.e. higher than 0x80000000) offset are treated as if counting back from the end of the CBFS section represented by that (which may not touch either the start or the end of the flash), so that when libpayload was trying to follow the master header pointer we would make sure it found the master header in the right section. I think this was forced by the cbfs_media structure not being very clear on whether it represented a region or a whole flash, and because the offset in the master header is always counted absolute from the start of the flash instead of the start of the CBFS region.
So I don't think there's really a way to keep letting this work the way it did while taking the master header out of the equation. You'll probably have to change how cbfs_media is treated and then we'll have to change depthcharge to match that (and whoever else has payloads relying on this). You could make one cbfs_media only represent one region, but that's weird again because for the default media from the coreboot tables is also expects that whatever libpayload_init_default_cbfs_media() returns represents the whole flash. I think the long term good way to resolve this is to make the payload only provide low-level access to the whole flash and move FMAP integration into libpayload, similar to how coreboot does it (with explicit APIs to access the default CBFS, RO CBFS, or other named section).