Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34897 )
Change subject: arch/x86: Simplified postcar WB MTRR setup ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34897/1/src/soc/intel/skylake/memma... File src/soc/intel/skylake/memmap.c:
https://review.coreboot.org/c/coreboot/+/34897/1/src/soc/intel/skylake/memma... PS1, Line 317: /* Do some magic alignments */
I see. Well.. do you thing all the smm_region() work in topic x86-smm-tseg is still worth doing? And CB:34894? Still, I would rather not directly reference cbmem_top() as we create postcar_frame, but let platform code separately define the upper boundary of the MTRR.
CB:34894 seems fine still; it provides consistency. As for not directly referencing cbmem_top() -- I assume you meant in common code? Sadly, one has to be intimate with the chipset behavior to make the correct choices. I don't think we can get away from that. I do like the common smm_region() approach. I do think if we want to optimize we can handle that case postcar_loader for merging entries if we think it's a problem.
To simply align cbmem_top() downwards does not look like solution either, wasting some MiB and what's this line 264 of "FSP Reserved Memory" between CBMEM root and the entries? Maybe some of this ascii-art is just incorrect.
FSP reserved memory is the first cbmem entry below the cbmem root. That is correct.