Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35029/1//COMMIT_MSG@10 PS1, Line 10: alignment requirments.
All the algorithms in question already try to fit the requested address and size to meet mtrr natural alignment requirements. I'm suggesting that if there is an address which isn't aligned to high enough granularity we should just have cbmem_top() enforce a better alignment to optimize for MTRR usage at the expense of stranded DRAM. That said, it's easier said than done when it comes to FSP deciding memory maps and needing to deal w/ the reserved space.
I believe from KBL time, we have enforce memory map alignment in CB space as well so its not true that FSP is deciding everything on reserving some random memory that CB is not aware of. design of memmap.c is taking care of transparent memory layout between FSP and CB. I'm quite sure we will get aligned memory in cbmem_top but the question was raised here that set_var_mtrr is wokring because cbmem_top is aligned, what if cbmem_top() is not align and we are still using set_var_mtrr hence i have ensured alignment again in this file. But now as we have lost romstage.c and its been using from common location, i believe we don't have any mean to apply these changes.
I wasn't aware we were talking about using set_var_mtrr() directly and only once. That's a completely different problem. Once can just as easily use the same looping flow to call set_var_mtrr() more often. I think it's important to be specific in what one is trying to optimize for. The terse commit description didn't indicate what assumptions in the goal it was trying to achieve: one call to set_var_mtrr() which I don't think is a great goal on its own, fwiw.