Hello Kyösti Mälkki,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to review the following change.
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch helps to save additional ~6ms of booting time in normal and s3 resume on CML-hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c 1 file changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index bceed6c..408d155 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -20,6 +20,7 @@ #include <cpu/cpu.h> #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> +#include <cpu/x86/smm.h> #include <program_loading.h> #include <rmodule.h> #include <romstage_handoff.h> @@ -149,6 +150,23 @@ set_var_mtrr(mtrr, base, size, MTRR_TYPE_WRPROT); }
+/* + * Cache the TSEG region at the top of ram. This region is + * not restricted to SMM mode until SMM has been relocated. + * By setting the region to cacheable it provides faster access + * when relocating the SMM handler as well as using the TSEG + * region for other purposes. + */ +static void enable_tseg_cache(struct postcar_frame *pcf) +{ + uintptr_t smm_base; + size_t smm_size; + + smm_region(&smm_base, &smm_size); + postcar_frame_add_mtrr(pcf, smm_base, smm_size, + MTRR_TYPE_WRBACK); +} + void postcar_frame_setup_top_of_dram_usage(struct postcar_frame *pcf, uintptr_t addr, size_t size, int type) { @@ -159,6 +177,9 @@ */ if (!acpi_is_wakeup_s3()) enable_top_of_dram_cache(addr, size); + + enable_tseg_cache(pcf); + postcar_frame_add_mtrr(pcf, addr, size, type); }
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#2).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch helps to save additional ~6ms of booting time in normal boot and s3 resume on CML-hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c 1 file changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/2
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#3).
Change subject: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API ......................................................................
arch/x86: Add postcar_frame_setup_top_of_dram_usage() API
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also helps to save additional ~6ms of booting time in normal boot and s3 resume on CML-hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c 2 files changed, 36 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... PS3, Line 153: MTRR_TYPE_WRBACK); I only want the four lines above added to cannonlake/romstage/romstage.c: car_stage_entry().
Everything else in this commit about adding a new API about top_of_dram remains to be questionable, and for the time being I am not ready to +2 that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... PS3, Line 153: MTRR_TYPE_WRBACK);
I only want the four lines above added to cannonlake/romstage/romstage.c: car_stage_entry(). […]
I understood but isn't it also true that we want to also pass this savings into some common code like postcar_loader.c for all soc to get benefit out ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/3//COMMIT_MSG@7 PS3, Line 7: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API intel/cannonlake: Cache the TSEG region
Patchset #2 commit message was just fine.
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... PS3, Line 153: MTRR_TYPE_WRBACK);
I understood but isn't it also true that we want to also pass this savings into some common code lik […]
Remember what I wrote earlier about postcar_frame_setup_top_of_dram_usage() setting new requirements for the alignments, should you call set_var_mtrr() from inside of it? That's why it is currently being rejected. Or at least it will not receive +2 from me, and I anticipate that Aaron and Furguain may share my opinion.
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#4).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also helps to save additional ~6ms of booting time in normal boot and s3 resume on CML-hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c 2 files changed, 36 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... PS3, Line 153: MTRR_TYPE_WRBACK);
Remember what I wrote earlier about postcar_frame_setup_top_of_dram_usage() setting new requirements for the alignments, should you call set_var_mtrr() from inside of it? That's why it is currently being rejected.
set_var_mtrr() been used here to enable immediate caching of give range. if we had to relying on "postcar_frame_add_mtrr()", it would be taken care during next stage entry point only while setting up new MTRR range.
Also just to note: i'm not calling "set_var_mtrr()" function here atleast in this CL. isn't it ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
sigh
I only want the four lines above added to cannonlake/romstage/romstage.c: car_stage_entry().
intel/cannonlake: Cache the TSEG region
^^^ This change is not gonna touch src/arch/x86.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you want it conditionally enabled even within the platform, intel/cannonlake here?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you mean, we should check if TSEG_STAGE_CACHE Kconfig is enable by platform, if yes then only cache tseg region else not ? thus might be good feedback.
I thought keeping inside common x86 code might help platform to call the required function ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you want it conditionally enabled even within the platform, intel/cannonlake here?
By 'this' I meant enabling WB MTRR to speed up loading. I think there's merit to adding smm region to the postcar solution -- just like apollolake. That is definitely not there in cannonlake. And yes we should probably not open code that everywhere...
I don't think we should do this unconditionally. I think it's going to depend on CAR implementation and uarch -- it's platform dependent, and we know that the current code doesn't explicitly flush all DRAM it's touching during loading.
Do you mean, we should check if TSEG_STAGE_CACHE Kconfig is enable by platform, if yes then only cache tseg region else not ? thus might be good feedback.
I thought keeping inside common x86 code might help platform to call the required function ?
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
All that said, I don't think the top of ram nomenclature is helpful here any longer nor do I think we need an API that doesn't save us anything over postcar_frame_add_mtrr() for the top of ram WB case.
I know these patches have churned a bit, but that's ok. I think we've sorted out previous assumptions and learned what's currently problematic.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
CB:34897
I think platform only has to provide an upper boundary for WB which is not allowed to move upwards and a lower boundary (cbmem_top() - 16*MiB) that common code is allowed to move downwards if it results with lesser use of MTRRs. Are there any cases where we would need UC hole there in the middle?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
Got your point, we can create a common API and call that API from run_postcar_phase() or romstage.c but do you think we need any dedicated kconfig when we have TSEG_STAGE_CACHE. My concern is that how do SoC user know if they have to enable TSEG caching as well via another kconfig. I thought it should automatically cache TSEG region but there might be one catch by marking that region as WB as we already learn the WB implication. I guess you are advocating new kconfig just for WB purpose ?
All that said, I don't think the top of ram nomenclature is helpful here any longer nor do I think we need an API that doesn't save us anything over postcar_frame_add_mtrr() for the top of ram WB case.
top_of_dram_usage name was given that because we were doing few operation there ins same function related to DRAM (setting up postcar frame with DRAM context, now caching TSEG region, enable intermediate caching for DRAM region WP)
I know these patches have churned a bit, but that's ok. I think we've sorted out previous assumptions and learned what's currently problematic.
yes, agree, i have learned a lot in terms of those assumption made in past and with current situation with CAT enabling.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
CB:34897
I think platform only has to provide an upper boundary for WB which is not allowed to move upwards and a lower boundary (cbmem_top() - 16*MiB) that common code is allowed to move downwards if it results with lesser use of MTRRs. Are there any cases where we would need UC hole there in the middle?
There are specific features, when enabled, create a non-contiguous memory map such that one can't just lay down a single MTRR. We can optimize things if we want for merging purposes, but I'm not sure the optimization is warranted at this point. Even in cases where one MTRR could be used, the waste is only 1 MTRR.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
I like the idea of adding smm region as WB to the postar mtrr solution. A common API for doing that is helpful . To do it automatically we can add a Kconfig and do this conditional call in run_postcar_phase() or just make the appropriate call in the appropriate romstage.c file.
Got your point, we can create a common API and call that API from run_postcar_phase() or romstage.c but do you think we need any dedicated kconfig when we have TSEG_STAGE_CACHE. My concern is that how do SoC user know if they have to enable TSEG caching as well via another kconfig. I thought it should automatically cache TSEG region but there might be one catch by marking that region as WB as we already learn the WB implication. I guess you are advocating new kconfig just for WB purpose ?
That or just put in calls at the proper places. e.g. replace the smm_region()/postcar_frame_add_mtrr() sequence in apollolake with the correct call. We can follow up with patches which reduce the call sequences from the various code bases with more shared code that could be driven by Kconfigs.
All that said, I don't think the top of ram nomenclature is helpful here any longer nor do I think we need an API that doesn't save us anything over postcar_frame_add_mtrr() for the top of ram WB case.
top_of_dram_usage name was given that because we were doing few operation there ins same function related to DRAM (setting up postcar frame with DRAM context, now caching TSEG region, enable intermediate caching for DRAM region WP)
Ya. I know where it started from, but in what it is in this patch it doesn't add anything. The automatic addition of TSEG for writeback would be the logical new thing that could be shared.
I know these patches have churned a bit, but that's ok. I think we've sorted out previous assumptions and learned what's currently problematic.
yes, agree, i have learned a lot in terms of those assumption made in past and with current situation with CAT enabling.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Ya. I'm not sure we're in agreement on the best option forward there. Thanks for the pointer.
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
I'm thinking that might be true as well. Anyway, we can have the discussion on that commit.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Ya. I'm not sure we're in agreement on the best option forward there. Thanks for the pointer.
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
I'm thinking that might be true as well. Anyway, we can have the discussion on that commit.
There are two topic branches; x86-smm-tseg and x86-romstage together already form a unified function/file layout over TSEG. IMHO those should go in first before discussing an API to set TSEG regions WB cacheable. I believe I also saw some numbers suggesting write-combining for TSEG would give the same performance boost without consuming cachelines at end of romstage. I did not find (or really look for) that data now, but it sort of makes sense as TSEG_STAGE_CACHE is mostly writes to TSEG until PARALLEL_MP init in ramstage. Was it the case that WB has some properties we want to avoid, while WC does not?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Ya. I'm not sure we're in agreement on the best option forward there. Thanks for the pointer.
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
I'm thinking that might be true as well. Anyway, we can have the discussion on that commit.
There are two topic branches; x86-smm-tseg and x86-romstage together already form a unified function/file layout over TSEG. IMHO those should go in first before discussing an API to set TSEG regions WB cacheable. I believe I also saw some numbers suggesting write-combining for TSEG would give the same performance boost without consuming cachelines at end of romstage. I did not find (or really look for) that data now, but it sort of makes sense as TSEG_STAGE_CACHE is mostly writes to TSEG until PARALLEL_MP init in ramstage. Was it the case that WB has some properties we want to avoid, while WC does not?
The only properties is that WB will go through the cache while WC will just uarch buffers. When we're in CAR the former can lead to issues (dependent on CAR scheme and uarch).
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#5).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also helps to save additional ~6ms of booting time in normal boot and s3 resume on CML-hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
Patch Set 4:
Patch Set 4:
Aaron, please visit CB:34805 patchset #6 comments and how you wish to address the alignment requirement about parameters passed to postcar_frame_setup_top_of_dram_usage(), if it will be extended to call set_var_mtrr().
Ya. I'm not sure we're in agreement on the best option forward there. Thanks for the pointer.
CB:35029 will hopefully address the concern raised due to set_var_mtrr() usage inside postcar_frame_setup_top_of_dram_usage() function. As we will pass aligned cbmem_top() address now to postcar_frame_setup_top_of_dram_usage(), so set_var_mtrr() should limit into single MTRR itself.
Subrata, can we have cbmem -t for this commit posted as well. My concern is that after all this work, POSTCAR_STAGE=y is still the better performing solution, winning POSTCAR_STAGE=n by some 7 ms.
I'm thinking that might be true as well. Anyway, we can have the discussion on that commit.
Just now i have finished running 500 cycle each with POSTCAR_STAGE=y and POSTCAR_STAGE=n coreboot image on same unit.
I have analysis the boot average time, here is my observation
1. Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
2. Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Now for the 7ms delta that we were referring above is higher in case of POSTCAR_STAGE=n is due to stage_cache_add() call while running run_ramstage_phase().
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
In case of romstage loading ramstage directly due to stage_cache_add with higher ramstage size we are seeing that delta times, if i remove those stage_cache_add() in normal path and pick stages from cbfs even on S3. i don't see those 7ms extra.
We should discuss the scope of POSTCAR_STAGE=n option in that CL itself rather here but like to highlight that we should also consider the user configuration choice of stages like to load and SPI savings as well.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
I have analysis the boot average time, here is my observation
- Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
All references by commit hash also please. We will find the source from that.
- Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Try to explain the case of 1100 being slower with #1. 4ms of 800ms is 0.5% saving?
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
I would prefer to make HAVE_ACPI_RESUME a user selectable option in menuconfig. Don't select it and stage cache will remain disabled. We can have devicetree config to override S3 support off.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
Patch Set 5:
I have analysis the boot average time, here is my observation
- Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
All references by commit hash also please. We will find the source from that.
- Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Try to explain the case of 1100 being slower with #1. 4ms of 800ms is 0.5% saving?
It sounds like the code caching related behavior, the more code cache range we enable in cb code the more time it takes in "1100:finished vboot kernel verification". With additional MTRRs with multiple of 16MB caching range i could see this time is actually increasing. I'm just hoping we are not exhausting the max cache capacity for SKU/core.
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
I would prefer to make HAVE_ACPI_RESUME a user selectable option in menuconfig. Don't select it and stage cache will remain disabled. We can have devicetree config to override S3 support off.
Don't select HAVE_ACPI_RESUME would impact S3 resume failure isn't it ? we might still like to continue to single CB image supports s0ix and S3 but we might agree to drop the S3 resume savings in cost of increase in normal boot time stage_cache_add.
Btw, we already has devicetree config "s0ix_enable" so all these stage_cache_add has to be guarded against that ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I have analysis the boot average time, here is my observation
- Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
All references by commit hash also please. We will find the source from that.
- Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Try to explain the case of 1100 being slower with #1. 4ms of 800ms is 0.5% saving?
It sounds like the code caching related behavior, the more code cache range we enable in cb code the more time it takes in "1100:finished vboot kernel verification". With additional MTRRs with multiple of 16MB caching range i could see this time is actually increasing. I'm just hoping we are not exhausting the max cache capacity for SKU/core.
Something like that, yes. But my expectation was that since MTRRs are reprogrammed during MP INIT near timestamp 30, and we have called wbinvd and toggled CD bits in CR0(?), availability of cachelines for step 1100 should have remained the same. Unless there is some bug with non-evict mode being disabled and none of the modified cachelines for MTRR WB regions get evicted like we assume. Type WC would not fill cachelines, did you experiment with it for TSEG?
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
I would prefer to make HAVE_ACPI_RESUME a user selectable option in menuconfig. Don't select it and stage cache will remain disabled. We can have devicetree config to override S3 support off.
Don't select HAVE_ACPI_RESUME would impact S3 resume failure isn't it ? we might still like to continue to single CB image supports s0ix and S3 but we might agree to drop the S3 resume savings in cost of increase in normal boot time stage_cache_add.
Yes, you could disable stage cache based on devicetree config.
Hello Kyösti Mälkki, Aaron Durbin, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#7).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
BUG=b:140008206 TEST=Verified normal boot time on CML-Hatch with latest coreboot
Without this CL: Total Time: 929ms
With this CL: (TSEG marked as WB) Total Time: 910ms
For test marked TSEG as WP/WC: Total Time: ~920ms
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/7/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/7/src/arch/x86/postcar_loader... PS7, Line 205: if (!CONFIG(TSEG_STAGE_CACHE)) If this is controlled by a Kconfig why not just make this static to the compilation unit and call it from run_postcar_phase()?
Hello Kyösti Mälkki, Aaron Durbin, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#8).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also removes dedicated function call to make TSEG region cache from soc and refers to enable_tseg_cache().
BUG=b:140008206 TEST=Verified normal boot time on CML-Hatch with latest coreboot
Without this CL: Total Time: 929ms
With this CL: (TSEG marked as WB) Total Time: 910ms
For test marked TSEG as WP/WC: Total Time: ~920ms
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c M src/soc/amd/picasso/romstage.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/denverton_ns/memmap.c M src/soc/intel/skylake/memmap.c 6 files changed, 25 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/7/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/7/src/arch/x86/postcar_loader... PS7, Line 205: if (!CONFIG(TSEG_STAGE_CACHE))
If this is controlled by a Kconfig why not just make this static to the compilation unit and call it […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/3//COMMIT_MSG@7 PS3, Line 7: arch/x86: Add postcar_frame_setup_top_of_dram_usage() API
intel/cannonlake: Cache the TSEG region […]
Done
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/3/src/arch/x86/postcar_loader... PS3, Line 153: MTRR_TYPE_WRBACK);
Remember what I wrote earlier about postcar_frame_setup_top_of_dram_usage() setting new requiremen […]
Ack
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Vanny E, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), David Guckian, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#9).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also removes dedicated function call to make TSEG region cache from soc and refers to enable_tseg_cache().
BUG=b:140008206 TEST=Verified normal boot time on CML-Hatch with latest coreboot
Without this CL : Total Time: 929ms
With this CL : (TSEG marked as WB) Total Time: 910ms
For test marked TSEG as WP/WC : Total Time: ~920ms
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c M src/soc/amd/picasso/romstage.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/denverton_ns/memmap.c M src/soc/intel/skylake/memmap.c 6 files changed, 24 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/9
David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 9: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@16 PS9, Line 16: TEST=Verified normal boot time on CML-Hatch with latest coreboot That's SOC_INTEL_COMETLAKE, building from soc/intel/cannonlake, and this commit does not even touch that?
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@22 PS9, Line 22: Total Time: 910ms Huh? You only move the function call from one file to another and you get this improvement??
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@28 PS9, Line 28: Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Remove my signed-off-by.
While I may have suggested that we need some common API around TSEG, I never co-authored this commit or the source file changes you have made here.
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return; I did see Aaron had made a comment around these lines on patchset #7 and I choose to disagree with him here.
Use of conditional here is not logical and seems like a workaround. If you are allowing TSEG as WB with TSEG_STAGE_CACHE, what is the reasoning for disallowing it without TSEG_STAGE_CACHE? I feel this is just creating conditions where you test platform with HAVE_ACPI_RESUME=n but they potentially start to have random boot failures (even on cold boots) with HAVE_ACPI_RESUME=y,TSEG_STAGE_CACHE=y. That's what we saw when we had marked low-ram 0..CACHE_TMP_RAMTOP as WB...
I think it is better to have fill_postcar_frame() in platform code explicitly call this instead of embedding it in run_postcar_phase():
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 210: MTRR_TYPE_WRBACK); There were many iterations, I don't know which were valid for which platform. My conclusion was WB for CBMEM is sometimes harmful, so I would have assumed WB for TSEG is equally sometimes harmful. If WC provided perfomance boost while WB was unstable, the type can be provided as parameter.
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 220: See comments above. I think this needs to be called from platform code not common code.
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... PS9, Line 89: run_postcar_phase(&pcf); Everything about postcar in amd/picasso is wrong and work-in-progress. Since you don't need to, better not make soc/amd changes any dependency to this work, just let them catch up later when API is ready.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@16 PS9, Line 16: TEST=Verified normal boot time on CML-Hatch with latest coreboot
That's SOC_INTEL_COMETLAKE, building from soc/intel/cannonlake, and this commit does not even touch […]
Ah right.. you were touching common code, that's where cometlake got it from. You should split cometlake TSEG as WB to separate commit, that one should be easy to +2.
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@22 PS9, Line 22: Total Time: 910ms
Huh? You only move the function call from one file to another and you get this improvement??
It was not so obvious at first sight that pretty much every x86 platform gets TSEG as WB with this commit.
Some of the older CPUs are very limited on VAR MTRRs, I am not convinced we can globally add TSEG as WB everywhere, so rest of the API will take some time to discuss and find shape.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 9:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@16 PS9, Line 16: TEST=Verified normal boot time on CML-Hatch with latest coreboot
Ah right.. you were touching common code, that's where cometlake got it from. […]
see my comments below
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@22 PS9, Line 22: Total Time: 910ms
It was not so obvious at first sight that pretty much every x86 platform gets TSEG as WB with this c […]
earlier i have platform specific CLs to enable thus feature 1. CB:34995 as common API 2. CB:35025 make other soc using specific code to refer #1 common API 3. CB:35026 add for CNL and ICL TSEG caching
Aaron had commented to make it common hence i have move everything into 1 CL and here
now looks like you are advocating for split approach like earlier.
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return;
I did see Aaron had made a comment around these lines on patchset #7 and I choose to disagree with h […]
will wait for Aaron to reply as its been originally done in that way
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 210: MTRR_TYPE_WRBACK);
There were many iterations, I don't know which were valid for which platform. […]
WB for TSEG been working for almost all platform, CB marking WB has different problem because codes are running from cache and we are going to run invd during tear down. code will only run from TSEG in S3 resume so i don't think we will run into any problem with existing code setup
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 220:
See comments above. I think this needs to be called from platform code not common code.
i have replied in my latest comments
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, David Guckian, Vanny E, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), David Guckian, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#10).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
Also removes dedicated function call to make TSEG region cache from soc and refers to enable_tseg_cache().
BUG=b:140008206 TEST=Verified normal boot time on CML-Hatch with latest coreboot
Without this CL : Total Time: 929ms
With this CL : (TSEG marked as WB) Total Time: 910ms
For test marked TSEG as WP/WC : Total Time: ~920ms
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/postcar_loader.c M src/soc/amd/picasso/romstage.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/memmap.c M src/soc/intel/denverton_ns/memmap.c M src/soc/intel/skylake/memmap.c 6 files changed, 24 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/9//COMMIT_MSG@28 PS9, Line 28: Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com
Remove my signed-off-by. […]
Done
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/soc/amd/picasso/romstag... PS9, Line 89: run_postcar_phase(&pcf);
Everything about postcar in amd/picasso is wrong and work-in-progress. […]
CB:35025 was just referring to common API nothing new been done here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 210: MTRR_TYPE_WRBACK);
WB for TSEG been working for almost all platform, CB marking WB has different problem because codes […]
Minor correection: For S3 resume path, code does not run from TSEG, stage_cache_load_stage() is the reverse memcpy() of stage_cache_add().
I really would like to do just the following and always have TSEG as WB, but your words 'almost all platforms' makes it difficult to have the call in common code.
if (CONFIG(SMM_TSEG)) enable_tseg_cache();
I tried something similar with 0..CACHE_TMP_RAMTOP as WB and got feedback all recent soc/intel failed. See CB:35187, maybe soc/intel just has buggy CAR teardown? There is still that unexplained behaviour around timestamp 1100 in previously provided logs, where marking TSEG as WB in postcar has triggered 10 ms delay late in payload.
At least intel/cannonlake has this:
* Skip Pre MP init MTRR programming as MTRRs are mirrored from BSP, * that are set prior to ramstage.
Since memcpy() for SMI handler does not happen in SMM mode, this relies on variable MTRRs. Not sure what was the motivation to delay the final MTRRs here?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return;
will wait for Aaron to reply as its been originally done in that way
@Aaron, if you could take a further look on this. Do you think keeping in common place helps or should i move this into 3 cl as its been done previously ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return;
@Aaron, if you could take a further look on this. […]
The control can be done by open coding the call at all the call sites that are needed or utilize a Kconfig option in a common path with the various platforms performing the 'select'. Kyosti is recommending the former. I don't really care; we can see how much of the same code is duplicated or not and if that is a goal to optimize away. Those are the trade-offs about one way vs the other. We already have an idea about what it'll look like: duplicated sequences in the files already in this patch plus others to get parity in the configuration.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, David Guckian, Vanny E, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), David Guckian, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#11).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
BUG=b:140008206 TEST=Verified normal boot time on CML-Hatch with latest coreboot
Without this CL: Total Time: 929ms
With this CL: (TSEG marked as WB) Total Time: 910ms
For test marked TSEG as WP/WC: Total Time: ~920ms
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/11
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 203: return;
The control can be done by open coding the call at all the call sites that are needed or utilize a K […]
i have spitted this CL in former approach. Kindly take a look
https://review.coreboot.org/c/coreboot/+/34995/9/src/arch/x86/postcar_loader... PS9, Line 210: MTRR_TYPE_WRBACK);
Minor correection: For S3 resume path, code does not run from TSEG, stage_cache_load_stage() is the reverse memcpy() of stage_cache_add().
I really would like to do just the following and always have TSEG as WB, but your words 'almost all platforms' makes it difficult to have the call in common code.
if (CONFIG(SMM_TSEG)) enable_tseg_cache();
did the same from platform code now
I tried something similar with 0..CACHE_TMP_RAMTOP as WB and got feedback all recent soc/intel failed. See CB:35187, maybe soc/intel just has buggy CAR teardown? There is still that unexplained behaviour around timestamp 1100 in previously provided logs, where marking TSEG as WB in postcar has triggered 10 ms delay late in payload.
surprisingly i don't see 1100 is taking the extra 10ms now when marked TSEG as WB with latest chromium code
At least intel/cannonlake has this:
* Skip Pre MP init MTRR programming as MTRRs are mirrored from BSP, * that are set prior to ramstage.Since memcpy() for SMI handler does not happen in SMM mode, this relies on variable MTRRs. Not sure what was the motivation to delay the final MTRRs here?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 11: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/11/src/arch/x86/include/arch/... File src/arch/x86/include/arch/romstage.h:
https://review.coreboot.org/c/coreboot/+/34995/11/src/arch/x86/include/arch/... PS11, Line 98: void enable_tseg_cache(struct postcar_frame *pcf); Probably should have postcar in the API name for consistency.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34995/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/11//COMMIT_MSG@23 PS11, Line 23: Measurements belong to the commit where CML calls the function added here.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, David Guckian, Vanny E, Kane Chen, Aamir Bohra, V Sowmya, build bot (Jenkins), David Guckian, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34995
to look at the new patch set (#12).
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
BUG=b:140008206 TEST=Build and boot CML-Hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/34995/12
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34995/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34995/11//COMMIT_MSG@23 PS11, Line 23:
Measurements belong to the commit where CML calls the function added here.
Done
https://review.coreboot.org/c/coreboot/+/34995/11/src/arch/x86/include/arch/... File src/arch/x86/include/arch/romstage.h:
https://review.coreboot.org/c/coreboot/+/34995/11/src/arch/x86/include/arch/... PS11, Line 98: void enable_tseg_cache(struct postcar_frame *pcf);
Probably should have postcar in the API name for consistency.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 12: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
arch/x86: Cache the TSEG region at the top of ram
This patch adds new API for enabling caching for the TSEG region and setting up required MTRR for next stage.
BUG=b:140008206 TEST=Build and boot CML-Hatch.
Change-Id: I59432c02e04af1b931d77de3f6652b0327ca82bb Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34995 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 27 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/include/arch/romstage.h b/src/arch/x86/include/arch/romstage.h index 2ac2258..15c93f2 100644 --- a/src/arch/x86/include/arch/romstage.h +++ b/src/arch/x86/include/arch/romstage.h @@ -88,4 +88,13 @@ */ void late_car_teardown(void);
+/* + * Cache the TSEG region at the top of ram. This region is + * not restricted to SMM mode until SMM has been relocated. + * By setting the region to cacheable it provides faster access + * when relocating the SMM handler as well as using the TSEG + * region for other purposes. + */ +void postcar_enable_tseg_cache(struct postcar_frame *pcf); + #endif /* __ARCH_ROMSTAGE_H__ */ diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 10a9ca2..c6149ab 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -17,6 +17,7 @@ #include <cpu/cpu.h> #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> +#include <cpu/x86/smm.h> #include <program_loading.h> #include <rmodule.h> #include <romstage_handoff.h> @@ -187,6 +188,23 @@ stage_cache_add(STAGE_POSTCAR, prog); }
+/* + * Cache the TSEG region at the top of ram. This region is + * not restricted to SMM mode until SMM has been relocated. + * By setting the region to cacheable it provides faster access + * when relocating the SMM handler as well as using the TSEG + * region for other purposes. + */ +void postcar_enable_tseg_cache(struct postcar_frame *pcf) +{ + uintptr_t smm_base; + size_t smm_size; + + smm_region(&smm_base, &smm_size); + postcar_frame_add_mtrr(pcf, smm_base, smm_size, + MTRR_TYPE_WRBACK); +} + void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog =
-
Aaron Durbin (Code Review) -
David Guckian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)