Attention is currently required from: Arthur Heymans, Christian Walter, Jincheng Li, Johnny Lin, Jonathan Zhang, Jérémy Compostella, 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/+/82264?usp=email )
Change subject: cpu/x86/mtrr: Add soc_fill_uc_holes_in_physical_address_space ......................................................................
Patch Set 8:
(4 comments)
Patchset:
PS8: Not sure if we have to put much effort into this? Is there actually a requirement for MTRR-based caching above for 4G?
In any case, I wouldn't want FSP hooks here.
File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/82264/comment/c0b97b96_9b7c1f84?usp... : PS3, Line 203: soc_update_physical_address_space
There seem to be no specification when it comes to MTRRs. […]
Looks like ASSIGNED is actually set? There are two more pitfalls: size = 0 is ignored and SUBTRACTIVE resources as well. Two signs that the code isn't meant to consider windows. Subtractive windows shouldn't be. But we don't have those on Xeon-SP. I guess setting the size could work (something we wouldn't have to worry about if coreboot would handle the resources).
File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/82264/comment/2639a8dc_e6b32e7b?usp... : PS8, Line 203: soc_fill_uc_holes_in_physical_address_space(addr_space); Putting MMIO high above the DRAM seems common. And I assume we'll see more of it. A general solution could look as follows: ``` max_dram = find_highest_dram_above_4g(); min_mmio = find_lowest_mmio_above_4g(); if (max_dram > min_mmio) that's odd, bail out;
mmio_base = 1 << log2_64(min_mmio); /* align to best power of 2 */ while (mmio_base <= max_dram) mmio_base |= mmio_base >> 1;
fill holes from mmio_base; ``` Well, that still assumes that there is a reasonably aligned barrier between DRAM and MMIO. But I hope the idea comes through.
*OTOH* unless somebody knows a good reason for MTRR caching above 4G, we could do a much simpler best effort: ``` min_mmio = find_lowest_mmio_above_4g(); mmio_base = 1 << log2_64(min_mmio); ``` This would kill some of the caching for DRAM if MMIO isn't well enough above it. But do we care?
Not sure if we optimize the upper end yet, do we? For the very end, it's just a matter of aligning up to a power of 2.
File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/c/coreboot/+/82264/comment/68799775_311bf54a?usp... : PS8, Line 64: include <
alphabetical order.
why? it wasn't before and one could as well argue that it looks better with the standard C headers above. but let's not argue, we probably won't need it.