Attention is currently required from: Julius Werner, Jérémy Compostella, Kapil Porwal, Nico Huber, Subrata Banik.
Arthur Heymans 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 1:
(1 comment)
File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/81960/comment/96c7b3d9_d3f3a525 : PS1, Line 25: if (CONFIG(PAYLOAD_X86_64_SUPPORT)) {
AIUI, the payload handover and the coreboot tables are the most important ABI pieces of coreboot. Making this a compile-time option would mean that the resulting coreboot is suddenly incompatible to all prior (x86) payload builds. So, shouldn't this be decided at runtime, maybe based on information from CBFS?
Is it not a fact that using a 64-bit toolchain to compile the libpayload is also a static piece of information? If so, then why are we not allowing a configuration to also enter LB in the desired 64-bit mode instead of limiting it to 32-bit?
Currently, the default/non-configurable decision about LB entry is always in protected mode.
This CL allows for one more option where 64-bit direct entry is also possible, hence all of the following options are now valid:
- 64-bit CB / 32-bit LB (using protected mode)
- 64-bit CB / 64-bit LB (using long mode)
- 32-bit CB / 32-bit LB
If we are relying on the runtime information about coreboot operational mode and using this information to decide whether to jump into LB, then wouldn't that limit #1 (where a protected mode entry into the libpayload is not possible while using 64-bit coreboot)? Perhaps not everyone wishes to use a 64-bit payload, so it would be better to keep those selections static based on the selection of payload mode.
If you are thinking that coreboot in 64-bit mode would break the compatibility with other 32-bit mode payload if we are selecting this Kconfig explicitly then we can choose two below means
- choose this Kconfig from mainboard rather than SOC directly
- allow this kconfig only select for chromeos booting (hence, only applies to depthcharge payload)
I agree with Nico on this. You don't want a buildtime configurable ABI. In the past you could take any coreboot image and slap any payload on there. Also the ability to chainload payloads is an important feature. Now sometimes things break a little and a 10y old payload might not work with a modern coreboot or vise versa. I think a lot of us just build coreboot and payload together and never look back. However I still think there is value in that model, so lets not throw that away. Also the whole universal payload stuff is exactly about defining an ABI + payload info standard :-).
It's ok to break the ABI in the sense that there is a new way to jump to a 64bit payload directly for 64bit coreboot, but that should be decided at runtime: i.e. the payload should have a new cbfs attribute to inform coreboot or the chainloading payload about the ABI. coreboot then needs multiple codepaths to deal with the different options and that's fine IMO. That way also chainloading 32bit payloads or 32bit coreboot could in principle load a 64bit payload.
noted! sounds like a mixmatch requirement for post processing the binary for in-field device which may/may not be the motivation for device manufacture to support. I value your opinion here.
Can you please let me know what is the way to add new cbfs attribute into the payload/libpayload here to tell that coreboot should hand-off to the libpayload in 64-bit mode rather trunking?
So there would be 2 ways: Add the information in a new 'cbfs_payload_segment_type'. cbfstool would parse the elf and add that information in there. A different way would be in cbfs_file_attr_tag. To remain backwards compatible the lack of the cbfs attribute would mean the payload is x86 32bit.