Attention is currently required from: Tim Wawrzynczak, Paul Menzel, Angel Pons, Patrick Rudolph. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56017 )
Change subject: Documentation: Improve x86_64 ......................................................................
Patch Set 8:
(29 comments)
File Documentation/arch/x86/x86_64.md:
https://review.coreboot.org/c/coreboot/+/56017/comment/ddd43ccc_838fff0a PS7, Line 4: In coreboot every stage (when enabled) is built for x86_64,
When enabled, every coreboot stage is built for x86_64,
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/4a054940_b73589a0 PS7, Line 11: it
What does `it` refer to?
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/5f29093e_99c2ffd4 PS7, Line 15: supports
must support
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/3f2d09ff_2e81800a 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. […]
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/df2fff81_84a17f61 PS7, Line 17: is
must be
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/f159f44f_4e9832a6 PS7, Line 23: supports
must support
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/ff58a070_8e042e1e PS7, Line 29: generates
must generate
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/e0ad1388_2ac14c2c PS7, Line 34: have
must have
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/f11bda01_6b830177 PS7, Line 34: are
must be
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/8ad55438_eeaab0ff PS7, Line 36: those
nit: these
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/0d88c108_e83c92db PS7, Line 36: compability
Still not fixed: compa*ti*bility
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/4d2db10f_349f1f36 PS7, Line 40: phase
plural `phases`?
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/bb063712_fda1ad5d PS7, Line 41: runs
If the above is changed to plural, this also needs to be changed to `run`
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/ea9ada35_962a12e1 PS7, Line 45: when
nit: Capitalize `When` for consistency with the other bullet points
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/22519020_3536ad93 PS7, Line 45: the compiler
Which one? `the compiler` sounds like there's only one compiler, which is not the case.
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/cbfa50c2_f4e2d61f PS7, Line 49: todays
today's
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/e5f65249_aa2199b6 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 imperat […]
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/e45689b6_8bedf1ff PS7, Line 70: those
nit: these
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/24155078_648649eb PS7, Line 70: had
have
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/73fc19a8_9165b57a PS7, Line 71: those
nit: `these`, or just `the`
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/3951f69a_2df9d6cc PS7, Line 71: Once : finished running those blobs it must switch back to long mode.
How about: […]
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/4a7691e6_ce426afa PS7, Line 77: will be reported back
`is relayed back` or `is propagated back`
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/20afd2d7_1d039ac4 PS7, Line 84:
drop one blank line?
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/6519cf1b_50239555 PS7, Line 91: pagetable
page tables
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/58c92ee8_1ecead85 PS7, Line 103: it
the CPU
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/25f862c4_fd32c89f PS7, Line 104: those
nit: these
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/653f5e20_1b214312 PS7, Line 109: Options
`Option` (`Option ROMs` is a compound name, the plural `s` is in `ROMs`)
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/3dc552f2_d541c285 PS7, Line 115: Must allow only that TSEG pages can be marked executable.
How about: […]
Done
https://review.coreboot.org/c/coreboot/+/56017/comment/3f1177ac_d0c228b6 PS7, Line 125: Until now it could be verified by FOSS developers on various Intel platforms : and no issues have been found.
I'd prefer to avoid using the word `verify` here, as there's no way to ensure x86_64 in CAR is fully […]
Done
-
Patrick Rudolph (Code Review)