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/72162b80_b14e977e : 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.