Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34897 )
Change subject: arch/x86: Simplified postcar WB MTRR setup ......................................................................
arch/x86: Simplified postcar WB MTRR setup
Change-Id: I8822c4dcdced872a19ac183b993e8885aadd0a2c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/skylake/memmap.c 1 file changed, 25 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/34897/1
diff --git a/src/soc/intel/skylake/memmap.c b/src/soc/intel/skylake/memmap.c index e3590fd..3d8f989 100644 --- a/src/soc/intel/skylake/memmap.c +++ b/src/soc/intel/skylake/memmap.c @@ -290,11 +290,10 @@ return (void *)(uintptr_t)ebda_cfg.tolum_base; }
-void fill_postcar_frame(struct postcar_frame *pcf) +static void postcar_wb_region(uintptr_t *start, size_t *size) { - uintptr_t top_of_ram; - uintptr_t smm_base; - size_t smm_size; + uintptr_t wb_start, wb_end; + size_t wb_size;
/* * We need to make sure ramstage will be run cached. At this @@ -302,10 +301,8 @@ * Instruct postcar to cache 16 megs under cbmem top which is * a safe bet to cover ramstage. */ - top_of_ram = (uintptr_t) cbmem_top(); - printk(BIOS_DEBUG, "top_of_ram = 0x%lx\n", top_of_ram); - top_of_ram -= 16*MiB; - postcar_frame_add_mtrr(pcf, top_of_ram, 16*MiB, MTRR_TYPE_WRBACK); + wb_start = (uintptr_t) cbmem_top(); + wb_start -= 16 * MiB;
/* * Cache the TSEG region at the top of ram. This region is @@ -314,6 +311,24 @@ * when relocating the SMM handler as well as using the TSEG * region for other purposes. */ - smm_region(&smm_base, &smm_size); - postcar_frame_add_mtrr(pcf, smm_base, smm_size, MTRR_TYPE_WRBACK); + smm_region(&wb_end, &wb_size); + wb_end += wb_size; + + /* Do some magic alignments */ + wb_start = ALIGN_DOWN(wb_start, x); + wb_end = ALIGN_UP(wb_end, x); + wb_size = wb_end - wb_start; + + /* Return one continuos region allowed for WB. */ + *start = wb_start; + *size = wb_size; +} + +void fill_postcar_frame(struct postcar_frame *pcf) +{ + uintptr_t wb_start; + size_t wb_size; + + postcar_wb_region(&wb_start, &wb_size); + postcar_frame_add_mtrr(pcf, wb_start, wb_size, MTRR_TYPE_WRBACK); }
Kyösti Mälkki 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:
Would this approach, if applied to all intel platforms, have some issues? Do we need UC holes between cbmem_top() and TSEG?
Subrata Banik 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:
Patch Set 1:
Would this approach, if applied to all intel platforms, have some issues? Do we need UC holes between cbmem_top() and TSEG?
kindly take a look and read this entire topic https://review.coreboot.org/q/topic:%22dram_cache_wb%22+(status:open%20OR%20...) why we have dropped WB and moved to WP for intermediate caching (cbmem_top -16MB) to cbmem-top
Kyösti Mälkki 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:
Patch Set 1:
Patch Set 1:
Would this approach, if applied to all intel platforms, have some issues? Do we need UC holes between cbmem_top() and TSEG?
kindly take a look and read this entire topic https://review.coreboot.org/q/topic:%22dram_cache_wb%22+(status:open%20OR%20...) why we have dropped WB and moved to WP for intermediate caching (cbmem_top -16MB) to cbmem-top
So far I have not seen any numbers of __significant__ performance boost using intermediate caching or POSTCAR_STAGE=n. Point to 'cbmem -t' output with numbers better than 4ms, which equals to about 0.5% improvement in reaching the kernel. Maybe You have someone in mind who will approve all the added complexity in source this adds, high price for 0.5%. Definetly not going to be me hitting +2 but will defer from giving -2 too.
Other type of optimizations will yield much better results, but I anticipate Intel is not willing to shrink down their FSP but continue to try force open-source project to adapt to their bloated API? But that's all the politics for today. As for coreboot proper, sure, I have many ideas what you can do to reach kernel faster. I have no numbers. I have no code. At the moment I have also no interest to share those ideas until this postcar MTRR discussion reaches a closure.
Besides, that change of WB->WP might apply to TSEG region as well, so you can equally consider this commit as providing the region for WP instead of WB. Or WC instead of WB. The type does not matter as long as one type is satisfactory for the complete region.
Subrata Banik 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:
Besides, that change of WB->WP might apply to TSEG region as well, so you can equally consider this commit as providing the region for WP instead of WB. Or WC instead of WB. The type does not matter as long as one type is satisfactory for the complete region.
You have asked if this CL will cause any issue on intel platform. Hence i thought of providing insight and asked to refer this this topic, specially https://review.coreboot.org/c/coreboot/+/34992/2 one as you will understand the implication of marking any region as WB and while tear down what happen, i would expect to see the same with this CL. I guess Aaron also said that WB might have its assumptions with LLC cache lines getting cleared due to uarch.
Kyösti Mälkki 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:
Patch Set 1:
Besides, that change of WB->WP might apply to TSEG region as well, so you can equally consider this commit as providing the region for WP instead of WB. Or WC instead of WB. The type does not matter as long as one type is satisfactory for the complete region.
You have asked if this CL will cause any issue on intel platform. Hence i thought of providing insight and asked to refer this this topic, specially https://review.coreboot.org/c/coreboot/+/34992/2 one as you will understand the implication of marking any region as WB and while tear down what happen, i would expect to see the same with this CL. I guess Aaron also said that WB might have its assumptions with LLC cache lines getting cleared due to uarch.
The code is not setting any additional MTRRs during romstage, only creating postcar_frame and having them applied after/at time of CAR teardown in the beginning of POSTCAR_STAGE. So far that has not been forbidden, right?
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 */ Why are we doing anything magic? FWIW, we can't lay down a MTRR over potential holes needed in the memory map.
Kyösti Mälkki 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 */
Why are we doing anything magic? FWIW, we can't lay down a MTRR over potential holes needed in the m […]
Can you pinpoint me to cases/platforms where such holes are required, between TSEG and cbmem_top(), before we reprogram these MTRRs again? MTRR reprogramming used to happen early in PARALLEL_MP, but I do see it may be delayed until payload entry.
Many platforms already do this and select "a suitable alignment somewhere higher than cbmem_top()". Aligning wb_end downwards relaxes us from having any alignment requirement on the implementation on cbmem_top(), which is somewhat convoluted for Intel implementations with EBDA located near TOLM.
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 */
Can you pinpoint me to cases/platforms where such holes are required, between TSEG and cbmem_top(), before we reprogram these MTRRs again? MTRR reprogramming used to happen early in PARALLEL_MP, but I do see it may be delayed until payload entry.
There are certain chipset options which are allocated there. Check out line 169 and onwards in this file which has a pictorial description.
We can argue the merits of why stolen memory actually surfaces in the host memory map... but that's an Intel decision in their implementation.
Many platforms already do this and select "a suitable alignment somewhere higher than cbmem_top()". Aligning wb_end downwards relaxes us from having any alignment requirement on the implementation on cbmem_top(), which is somewhat convoluted for Intel implementations with EBDA located near TOLM.
Yes, I understand the technical desires for this change, but it is ignoring the other selectable options which invalidates the contiguous address range. I think I noted earlier that we could modify the postcar_loader.c code to merge entries if the memory map aligns such that we can optimize the use of variable mtrrs. However, I haven't seen a situation where it's required because of exhausting variable mtrr resources.
Kyösti Mälkki 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 */
Can you pinpoint me to cases/platforms where such holes are required, between TSEG and cbmem_top() […]
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.
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.
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.
Paul Menzel 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//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34897/1//COMMIT_MSG@7 PS1, Line 7: Simplified Simplify or Add simplified
Kyösti Mälkki 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)
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.. […]
I think CB:34894 and various smm_subregion() are ready. Former is blocking the submits of latter I believe.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34897 )
Change subject: arch/x86: Simplified postcar WB MTRR setup ......................................................................
Abandoned
Too simple, would break.
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)