Attention is currently required from: Arthur Heymans, Jérémy Compostella, Kapil Porwal, Nico Huber, Patrick Rudolph, Subrata Banik.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81960?usp=email )
Change subject: arch/x86: Enable long mode entry into payload for x86_64 support ......................................................................
Patch Set 14:
(2 comments)
Patchset:
PS14: Sorry for being late to this discussion. So is the decision here now that there's gonna be two entry points, or just a single one? Because using another payload segment type sounds like you want a whole second entry point, but your code is currently not doing that yet (it looks like it's just using the segment type as a boolean flag).
Personally, I don't think the two entry points are really worth it. I'd posit that while compatibility of newer coreboot versions to older payloads is nice and something we have preserved for a long time, compatibility of older coreboot with newer payloads gets broken regularly anyway and not something people really care about. Basically every time we add a new coreboot table entry (which happens a lot), that's usually to communicate something to the payload that it requires (and it won't work right and often hang without it), and I don't recall anybody ever complaining about that. In practice I think that while payloads may be binaries that get reused after a coreboot update, the people who do build their own payload also tend to build their own coreboot anyway (and keep their payload repo synced to whatever coreboot branch they're planning to use it with, not ToT).
If we're okay with only one entry point and all we need is a flag that says whether a payload is 32 or 64 bit, I don't think payload segments are a great way to model that. They're designed to always be associated with a memory area and not really efficient to store other stuff. Besides, the code currently in this patch looks like it expects the PAYLOAD_SEGMENT_X86_LONG_MODE_ENTRY beyond the PAYLOAD_SEGMENT_ENTRY(?), which is not correct and not safe because PAYLOAD_SEGMENT_ENTRY is defined as the segment that ends the segment list. Reading beyond it means you'd read beyond the end of the list on older payloads.
I think the best way to model this would just be with a different CBFS file type (e.g. 0x22 CBFS_TYPE_SELF64). That also means the information will automatically be available in `arch_prog_run()` and you don't need any extra effort to extract it.
File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/81960/comment/e0d3dc98_ea30b305 : PS14, Line 32: void *mapping = cbfs_type_map(prog_name(&payload), NULL, &payload.cbfs_type); Please don't find and parse the entire payload again in this code. Instead, modify selfboot.c to find the new segment type and expose the info somehow (e.g. if we're going to have two different entry points then you'd have to add an `entry64` member to `struct prog`... otherwise, just adding a bool for this should be fine.
Moving this to a CBFS type would make things easier because that's already stored in `struct prog` right now.