[coreboot-gerrit] [S] Change in coreboot[main]: soc/intel/xeon_sp: Reserve FSP MMIO high window

[Nico Huber (Code Review)]

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.

Nico Huber has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/83538?usp=email )

Change subject: soc/intel/xeon_sp: Reserve FSP MMIO high window
......................................................................

Patch Set 1:

(6 comments)

Patchset:

PS1:
> Some critical log diffs: […]

Just for the future, please disable CONSOLE_USE_ANSI_ESCAPES and use triple backticks ``` for quote blocks.

Also, what you pasted is not the MTRR assignment but the requirements.

Commit Message:

https://review.coreboot.org/c/coreboot/+/83538/comment/edaa428b_6c4eb88b?usp... :
PS1, Line 7: soc/intel/xeon_sp: Reserve FSP MMIO high window
Only for SPR, right? btw. why?

https://review.coreboot.org/c/coreboot/+/83538/comment/0de08812_32eaf051?usp... :
PS1, Line 9: Xeon-SP supports MMIO high window, a.k.a. MMIO window above 4G.
TBH, I don't know what this means; or what platform doesn't support it?

https://review.coreboot.org/c/coreboot/+/83538/comment/58821c4e_7aa00c89?usp... :
PS1, Line 19: is especially important
            : on systems with 2 or more sockets, where each socket has multiple
            : domains.
How so? In what scenario is this important? What part of coreboot
or your payload has such requirements?

File src/soc/intel/xeon_sp/uncore.c:

https://review.coreboot.org/c/coreboot/+/83538/comment/d10ee5ad_5e0830c5?usp... :
PS1, Line 368: 	 * Add MMIO high window.
That's what the code says. Please don't state the obvious in comments.

What a reader needs to know here is *why* we do it here in set_resources().

https://review.coreboot.org/c/coreboot/+/83538/comment/231d4481_8bfd6293?usp... :
PS1, Line 375: 				index++;
This not stable because mmapvtd_read_resources() mixes generic PCI
resources and the static ones counted from 0. The latter start at
0x10, IIRC. Does the `mmapvtd` device really have standard PCI
resources?

If so, I would choose some higher constant and document that.