Attention is currently required from: Patrick Rudolph, Tim Wawrzynczak, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56017 )
Change subject: Documentation: Improve x86_64 ......................................................................
Patch Set 7:
(24 comments)
File Documentation/arch/x86/x86_64.md:
https://review.coreboot.org/c/coreboot/+/56017/comment/0b4aeed6_57f4ab43 PS7, Line 4: In coreboot every stage (when enabled) is built for x86_64, When enabled, every coreboot stage is built for x86_64,
https://review.coreboot.org/c/coreboot/+/56017/comment/6d008f7d_bfcfcd75 PS7, Line 11: it What does `it` refer to?
https://review.coreboot.org/c/coreboot/+/56017/comment/d373751e_b2f8b89f PS7, Line 15: supports must support
https://review.coreboot.org/c/coreboot/+/56017/comment/16647115_d1a449c8 PS7, Line 15: This is the case for Pentium 4 or : compatible CPUs. Not all Pentium 4's support x86_64, only Cedar Mill and some Prescott models do. Also, this is Intel-specific. I'd just drop this sentence.
https://review.coreboot.org/c/coreboot/+/56017/comment/0af4f661_ecfc7c58 PS7, Line 17: is must be
https://review.coreboot.org/c/coreboot/+/56017/comment/e7a271bd_8b2616da PS7, Line 17: On Intel platforms What about non-Intel platforms? Does the same idea apply (except for the `BIOS region` term), or is it a completely different thing?
https://review.coreboot.org/c/coreboot/+/56017/comment/e3a90726_5245c266 PS7, Line 18: 24 KiB minimum Where does this number come from?
https://review.coreboot.org/c/coreboot/+/56017/comment/ac98e32f_a842a2d8 PS7, Line 23: supports must support
https://review.coreboot.org/c/coreboot/+/56017/comment/141dbebc_e11b8fc4 PS7, Line 29: generates must generate
https://review.coreboot.org/c/coreboot/+/56017/comment/533363db_a8a6e046 PS7, Line 34: have must have
https://review.coreboot.org/c/coreboot/+/56017/comment/21a945fa_516e0095 PS7, Line 34: are must be
https://review.coreboot.org/c/coreboot/+/56017/comment/4d022cff_00e98949 PS7, Line 36: those nit: these
https://review.coreboot.org/c/coreboot/+/56017/comment/b610e6fe_76d8768e PS7, Line 54: The high dword of pointers is always zero Is this an actual requirement, or just a side effect? If it's a requirement, then please use imperative mood:
The high dword of pointers must always be zero.
It would be great to mention why this is required (if it's required).
https://review.coreboot.org/c/coreboot/+/56017/comment/9142cafd_f5389cfd PS7, Line 70: those nit: these
https://review.coreboot.org/c/coreboot/+/56017/comment/0fb080c7_a408044c PS7, Line 70: had have
https://review.coreboot.org/c/coreboot/+/56017/comment/f7f38fc2_a11f57c6 PS7, Line 71: those nit: `these`, or just `the`
https://review.coreboot.org/c/coreboot/+/56017/comment/90f12f00_58f6fc88 PS7, Line 71: Once : finished running those blobs it must switch back to long mode. How about:
Once the invoked blobs finish running, coreboot needs to switch back to long mode.
https://review.coreboot.org/c/coreboot/+/56017/comment/4660cf05_48f81334 PS7, Line 77: will be reported back `is relayed back` or `is propagated back`
https://review.coreboot.org/c/coreboot/+/56017/comment/f465d548_aaec537b PS7, Line 84: drop one blank line?
https://review.coreboot.org/c/coreboot/+/56017/comment/971d8b84_ea442f72 PS7, Line 91: pagetable page tables
https://review.coreboot.org/c/coreboot/+/56017/comment/61a7429c_7e1281ad PS7, Line 103: it the CPU
https://review.coreboot.org/c/coreboot/+/56017/comment/8bd4d6fe_0a7b35e8 PS7, Line 104: those nit: these
https://review.coreboot.org/c/coreboot/+/56017/comment/e33e2f20_2376d25a PS7, Line 110: Sandy Bridge / Haswell Broadwell and Bay Trail as well?
https://review.coreboot.org/c/coreboot/+/56017/comment/d8287608_2ee939d7 PS7, Line 115: Must allow only that TSEG pages can be marked executable. How about:
Must only allow TSEG pages to be marked as executable.