Attention is currently required from: Benjamin Doron.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79160?usp=email )
Change subject: Documentation: Improve x86_64 ......................................................................
Patch Set 1: Code-Review+1
(9 comments)
Patchset:
PS1: Thanks for doing this.
If you don't feel like addressing all of the thoughts I've added, that's fine. What you have here is a definite improvement, so I'd rather see it merged and address any issues later if you don't feel like addressing my comments.
File Documentation/arch/x86/x86_64.md:
https://review.coreboot.org/c/coreboot/+/79160/comment/79b0f00f_fa396e74 : PS1, Line 3: experimental Do you have any thoughts about how long this is going to remain experimental? What's preventing it from not being experimental? If you're not sure, that's fine - we can bring it up to a wider audience to see about creating a plan.
https://review.coreboot.org/c/coreboot/+/79160/comment/0260ce68_e96c0b59 : PS1, Line 10: a *"hack"* which places page tables in ROM Add: "This will be explained further below in the section on page tables"?
https://review.coreboot.org/c/coreboot/+/79160/comment/8b141c0f_144aaace : PS1, Line 24: : The large memory model causes the compiler to emit 64bit addressing : instructions, which are not only slower, but also increase code size. Should this be moved to a section comparing advantages and disadvantages between 32 & 64-bit modes?
A question I have after reading all of this is "Why would anyone use this? What's the advantage?". Currently it seems to only list problems.
https://review.coreboot.org/c/coreboot/+/79160/comment/5eafffdd_24b89a4b : PS1, Line 28: must generate page tables at build time Is this really a toolchain requirement, or just an implementation detail?
https://review.coreboot.org/c/coreboot/+/79160/comment/d5a763ad_bc3ade38 : PS1, Line 59: . This config option is enabled if the SOC/CPU selects HAVE_EXP_X86_64_SUPPORT.
Here's the list of boards currently selecting that feature.
src/cpu/qemu-x86/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/cpu/intel/model_206ax/Kconfig: select HAVE_EXP_X86_64_SUPPORT if USE_NATIVE_RAMINIT src/cpu/intel/model_2065x/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/amd/genoa/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/amd/picasso/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/soc/intel/cannonlake/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/northbridge/intel/x4x/Kconfig: select HAVE_EXP_X86_64_SUPPORT src/northbridge/intel/gm45/Kconfig: select HAVE_EXP_X86_64_SUPPORT
I'm not sure I'd list the boards, as done below, just because that list is going to expand pretty rapidly at this point and the documentation will be outdated.
https://review.coreboot.org/c/coreboot/+/79160/comment/f59255c6_838a8af3 : PS1, Line 133: qemu I think QEMU should be uppercase unless referring to the directory name in coreboot.
https://review.coreboot.org/c/coreboot/+/79160/comment/6e8fdcd9_29a0da76 : PS1, Line 140: Here's a list of known issues: Should this be another subsection, or is this still referring to QEMU?
https://review.coreboot.org/c/coreboot/+/79160/comment/2f58018e_2d830b1e : PS1, Line 155: * Entering long mode crashes on AMD host platforms. I don't think this is true anymore. There's was an issue with 32-bit FSPs when using 64-bit mode, but I believe Felix has added code to switch back to 32-bit until we have 64-bit FSPs.
Of course with openSIL-based platforms, like the GenoaPOC, the openSIL code is built as part of coreboot, thus in the same mode.
-
Martin L Roth (Code Review)