Change in coreboot[master]: [WIP]cpu/x86/cache: CLFLUSH programs to memory before running
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: [WIP]cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
[WIP]cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that this are all in UC memory such that calling invd in postcar is ok.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage. Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR to make that UC.
TODO be nice to reviewers and spit some parts off.
UNTESTED
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 4 files changed, 68 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index b53cbf8..53ab4f9 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -208,6 +208,21 @@ MTRR_TYPE_WRBACK); }
+/* + * POSTCAR will call invd so don't make assumptions on cbmem + * and external stage cache being UC. + */ +static void postcar_flush_cache(void) +{ + uintptr_t stage_cache_base; + size_t stage_cache_size; + prog_segment_loaded((uintptr_t)cbmem_top(), cbmem_overhead_size(), SEG_FINAL); + if (CONFIG(TSEG_STAGE_CACHE)) { + stage_cache_external_region((void **)&stage_cache_base, &stage_cache_size); + prog_segment_loaded(stage_cache_base, stage_cache_size, SEG_FINAL); + } +} + void run_postcar_phase(struct postcar_frame *pcf) { struct prog prog = @@ -222,8 +237,10 @@ parameters between S3 resume and normal boot. On the platforms where the values are the same it's a nop. */ finalize_load(prog.arg, pcf->stack); - } else + } else { load_postcar_cbfs(&prog, pcf); + postcar_flush_cache(); + }
/* As postcar exist, it's end of romstage here */ timestamp_add_now(TS_END_ROMSTAGE); diff --git a/src/cpu/x86/cache/Makefile.inc b/src/cpu/x86/cache/Makefile.inc index b33b9ee..759488e 100644 --- a/src/cpu/x86/cache/Makefile.inc +++ b/src/cpu/x86/cache/Makefile.inc @@ -1 +1,2 @@ +romstage-y += cache.c ramstage-y += cache.c diff --git a/src/cpu/x86/cache/cache.c b/src/cpu/x86/cache/cache.c index 2313c4d..ac2c45b 100644 --- a/src/cpu/x86/cache/cache.c +++ b/src/cpu/x86/cache/cache.c @@ -11,8 +11,11 @@ * GNU General Public License for more details. */
+#include <cbmem.h> +#include <program_loading.h> #include <console/console.h> #include <cpu/x86/cache.h> +#include <arch/cpu.h>
void x86_enable_cache(void) { @@ -20,3 +23,47 @@ printk(BIOS_INFO, "Enabling cache\n"); enable_cache(); } + +int clflush_supported(void) +{ + return (cpuid_edx(1) >> 19) & 1; +} + +static void clflush_region(uintptr_t start, size_t size) +{ + uintptr_t addr; + size_t cl_size = (cpuid_ebx(1) >> 8) & 0xff; + if (!clflush_supported()) + return; + for (addr = (start / cl_size) * cl_size; addr < start + size; addr += cl_size) + clflush((void *)addr); +} + +static int dram_ready; + +/* + * For each segment of a program loaded this function is called + * to invalidate caches for the addresses of the loaded segment + */ +void arch_segment_loaded(uintptr_t start, size_t size, int flags) +{ + /* INVD is only called in postcar stage so we only need + to make sure that our things hit dram during romstage. */ + if (!ENV_ROMSTAGE) + return; + if (flags != SEG_FINAL) + return; + if (!dram_ready) + return; + + if (!clflush_supported()) + printk(BIOS_DEBUG, "Not flushing cache to ram, CLFLUSH not supported\n"); + clflush_region(start, size); +} + +static void set_dram_ready(int unused) +{ + dram_ready = 1; +} + +ROMSTAGE_CBMEM_INIT_HOOK(set_dram_ready); diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h index 713ca32..2d2727f 100644 --- a/src/include/cpu/x86/cache.h +++ b/src/include/cpu/x86/cache.h @@ -55,6 +55,8 @@ asm volatile ("clflush (%0)"::"r" (addr)); }
+int clflush_supported(void); + /* The following functions require the __always_inline due to AMD * function STOP_CAR_AND_CPU that disables cache as * RAM, the cache as RAM stack can no longer be used. Called
Hello Subrata Banik, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#2).
Change subject: [WIP]cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
[WIP]cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that this are all in UC memory such that calling invd in postcar is ok.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage. Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR to make that UC.
TODO be nice to reviewers and spit some parts off.
UNTESTED
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 4 files changed, 75 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/2
Hello Subrata Banik, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#5).
Change subject: [WIP]cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
[WIP]cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that this are all in UC memory such that calling invd in postcar is ok.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage. Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR to make that UC.
TODO be nice to reviewers and spit some parts off.
TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 4 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/5
Hello Subrata Banik, Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#7).
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that these are all in UC memory such that calling INVD in postcar is ok.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage. Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR's to make that UC.
TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 4 files changed, 65 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 27: int clflush_supported(void) This is not sufficient. When CAR is active it's microarchitecturally defined how a clflush is handled in CAR mode. You need a way for the chipset to indicate this is valid sequence that can be performed.
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 29: 19 Aren't there defines for these bits instead of open coding them?
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@6... PS7, Line 60: if (!cbmem_ready()) Why is a cbmem check in here? It doesn't immediately make sense why this would be the case. Likewise, this function is also active when not loading postcar, e.g. FSP. What about that? Or are you using cbmem to be an indirect proxy for postcar loading?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 27: int clflush_supported(void)
This is not sufficient. When CAR is active it's microarchitecturally defined how a clflush is handled in CAR mode. You need a way for the chipset to indicate this is valid sequence that can be performed.
This seems to work for Intel CPUs with a NEM and also older ones lacking it. Subrata tested a similar concept with WB caching cbmem on much more recent Intel CPU's (probably NEM enhanced). So at first sight it looks like this will be ok for most Intel? Probably worth checking for some AMD.
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 29: 19
Aren't there defines for these bits instead of open coding them?
Not yet afaik.
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@6... PS7, Line 60: if (!cbmem_ready())
Why is a cbmem check in here? It doesn't immediately make sense why this would be the case. Likewise, this function is also active when not loading postcar, e.g. FSP. What about that? Or are you using cbmem to be an indirect proxy for postcar loading?
It wanted to use that as proxy that dram is ready and avoid it when things are loaded into CAR like FSP (if non-XIP). Is checking for [start, start + size] being inside the CAR region sufficient/better?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 27: int clflush_supported(void)
This is not sufficient. When CAR is active it's microarchitecturally defined how a clflush is handled in CAR mode. You need a way for the chipset to indicate this is valid sequence that can be performed.
This seems to work for Intel CPUs with a NEM and also older ones lacking it. Subrata tested a similar concept with WB caching cbmem on much more recent Intel CPU's (probably NEM enhanced). So at first sight it looks like this will be ok for most Intel? Probably worth checking for some AMD.
Those results are clearly empirical which suggests that the chipsets need to opt-in to affirm this will work.
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 29: 19
Aren't there defines for these bits instead of open coding them? […]
Can you start?
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@6... PS7, Line 60: if (!cbmem_ready())
Why is a cbmem check in here? It doesn't immediately make sense why this would be the case. Likewise, this function is also active when not loading postcar, e.g. FSP. What about that? Or are you using cbmem to be an indirect proxy for postcar loading?
It wanted to use that as proxy that dram is ready and avoid it when things are loaded into CAR like FSP (if non-XIP). Is checking for [start, start + size] being inside the CAR region sufficient/better?
If that's the case then please comment the purpose of what the check is for. It's clearly indirect w.r.t. to what you want accomplish from your explanation.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Subrata Banik,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#8).
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that these are all in UC memory such that calling INVD in postcar is OK.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage.
Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR's to make that UC.
TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/Kconfig M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 5 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/8
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Subrata Banik,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#10).
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that these are all in UC memory such that calling INVD in postcar is OK.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage.
Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR's to make that UC.
TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/Kconfig M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 5 files changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/10
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Subrata Banik,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37196
to look at the new patch set (#11).
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
cpu/x86/cache: CLFLUSH programs to memory before running
When cbmem is initialized in romstage and postcar placed in the stage cache + cbmem where it is run, the assumption is made that these are all in UC memory such that calling INVD in postcar is OK.
For performance reasons (e.g. postcar decompression) it is desirable to cache cbmem and the stage cache during romstage.
Another reason is that AGESA sets up MTRR during romstage to cache all dram, which is currently worked around by using additional MTRR's to make that UC.
TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Change-Id: I7ff2a57aee620908b71829457ea0f5a0c410ec5b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c M src/cpu/x86/Kconfig M src/cpu/x86/cache/Makefile.inc M src/cpu/x86/cache/cache.c M src/include/cpu/x86/cache.h 5 files changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/37196/11
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 27: int clflush_supported(void)
This is not sufficient. […]
Done. Added a Kconfig flag X86_CLFLUSH_CAR
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 29: 19
Can you start?
Done
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@6... PS7, Line 60: if (!cbmem_ready())
Why is a cbmem check in here? It doesn't immediately make sense why this would be the case. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37196/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37196/12//COMMIT_MSG@20 PS12, Line 20: TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath. Could you verify speed-ups (above you mention “performance reasons”)?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37196/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37196/12//COMMIT_MSG@20 PS12, Line 20: TESTED on ASUS P5QL-EM on both regular and S3 resume bootpath.
Could you verify speed-ups (above you mention “performance reasons”)?
I mean, if you have the timestamps before and after, could you please add them?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 16: int bool?
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 21: ENV_POSTCAR || ENV_RAMSTAGE Isn't there a symbol for pre-RAM stages?
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 24: CONFIG(X86_CLFLUSH_CAR) Why not place the constant on the left-hand side of the operation?
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 29: uintptr_t types.h
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 33: ram RAM
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 21: ENV_POSTCAR || ENV_RAMSTAGE
Isn't there a symbol for pre-RAM stages?
`!ENV_CACHE_AS_RAM` might be a good choice, it also covers the `RESET_VECTOR_IN_RAM` case.
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 24: CONFIG(X86_CLFLUSH_CAR)
Why not place the constant on the left-hand side of the operation?
I'm curious, does the compiler optimize it differently?
As the the operand has no side effects, it should be safe to treat it the same.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/12/src/cpu/x86/cache/cache.c@... PS12, Line 24: CONFIG(X86_CLFLUSH_CAR)
I'm curious, does the compiler optimize it differently? […]
I'm not sure if the compiler can infer that cpuid_edx has no side-effect, so it may not be able to optimize out the code as-is.
In any case, I'd write this function as follows:
bool clflush_supported(void) { /* CLFLUSH while operating in CAR might not be supported by platforms */ if (ENV_CACHE_AS_RAM && !CONFIG(X86_CLFLUSH_CAR)) return false;
return !!(cpuid_edx(1) >> CPUID_FEATURE_CLFLUSH_BIT); }
Attention is currently required from: Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 14: Code-Review+1
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)