Change in coreboot[master]: cpu/x86: Add a prog_run hook to set up caching of XIP stages
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/1
diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index a8cf54d..b316c1f 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -78,6 +78,16 @@ depends on !NO_FIXED_XIP_ROM_SIZE default 0x10000
+config SETUP_XIP_CACHE + bool + depends on C_ENVIRONMENT_BOOTBLOCK + depends on !NO_XIP_EARLY_STAGES + help + Select this option to set up an MTRR to cache XIP stages loaded + from the bootblock. This is useful on platforms lacking a + non-eviction mode and therefore need to be careful to avoid + eviction. + config CPU_ADDR_BITS int default 36 diff --git a/src/cpu/x86/mtrr/Makefile.inc b/src/cpu/x86/mtrr/Makefile.inc index caa6e9c..dc914de 100644 --- a/src/cpu/x86/mtrr/Makefile.inc +++ b/src/cpu/x86/mtrr/Makefile.inc @@ -7,3 +7,5 @@ romstage-y += debug.c postcar-y += debug.c ramstage-y += debug.c + +bootblock-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c diff --git a/src/cpu/x86/mtrr/xip_cache.c b/src/cpu/x86/mtrr/xip_cache.c new file mode 100644 index 0000000..a45944b --- /dev/null +++ b/src/cpu/x86/mtrr/xip_cache.c @@ -0,0 +1,61 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/cpu.h> +#include <program_loading.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <cpu/x86/mtrr.h> + +#define MAX_ROM_CACHE_SIZE (256 * KiB) + +void platform_prog_run(struct prog *prog) +{ + uint32_t base = region_device_offset(&prog->rdev); + uint32_t size = region_device_sz(&prog->rdev); + uint32_t end = base + size; + int mtrr_num = get_free_var_mtrr(); + uint32_t mtrr_mask_size = 4 * KiB; + struct cpuinfo_x86 cpu_info; + + get_fms(&cpu_info, cpuid_eax(1)); + if (cpu_info.x86 == 0xf) { + printk(BIOS_DEBUG, + "PROG_RUN: CPU does not support caching ROM\n" + "The next stage will run slowly\n"); + return; + } + + if (mtrr_num == -1) { + printk(BIOS_NOTICE, + "PROG_RUN: No MTRR available to cache ROM!\n" + "The next stage will run slowly!\n"); + return; + } + + while (ALIGN_DOWN(base, mtrr_mask_size) + mtrr_mask_size < end) + mtrr_mask_size *= 2; + base = ALIGN_DOWN(base, mtrr_mask_size); + if (mtrr_mask_size > MAX_ROM_CACHE_SIZE) { + printk(BIOS_WARNING, + "PROG_RUN: %dKiB XIP cache requested but limiting to %dKiB!", + mtrr_mask_size, MAX_ROM_CACHE_SIZE); + mtrr_mask_size = MAX_ROM_CACHE_SIZE; + } + + printk(BIOS_DEBUG, + "PROG_RUN: Setting MTRR to cache XIP stage. base: 0x%08x, size: 0x%08x\n", + base, mtrr_mask_size); + + set_var_mtrr(mtrr_num, base, mtrr_mask_size, MTRR_TYPE_WRPROT); +}
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#3).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/3
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#4).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 75 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex. I'm not an expert, but would feel safer with CD set. Unless there is some implicit invalidation ofc.
https://review.coreboot.org/c/coreboot/+/35993/5/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/5/src/cpu/x86/mtrr/xip_cache.... PS5, Line 48: base = ALIGN_DOWN(base, mtrr_mask_size); Move below the `if`, so we don't align down too far in the >max case?
With arbitrary bases, it could even make sense to align up.
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#6).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35993/5/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/5/src/cpu/x86/mtrr/xip_cache.... PS5, Line 48: base = ALIGN_DOWN(base, mtrr_mask_size);
Move below the `if`, so we don't align down too far in the >max case? […]
Done
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... PS6, Line 59: mtrr_base = ALIGN_UP(base, mtrr_mask_size); Still, I think we need to override the ALIGN_DOWN() above.
else mtrr_base = ALIGN_DOWN(base, mtrr_mask_size);
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#8).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 83 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/8
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 20: #define MAX_ROM_CACHE_SIZE (256 * KiB) What's the reasoning for only limiting to 256KiB? Please document your assumptions/thinking.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog) If this is being ran multiple times aren't we just accumulating MTRRs? Admittedly it's currently only possible to run in bootblock and verstage, but we would have accumulated 2 MTRRs then.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) { I'm not sure this a valid check. x86 is already checked for 0xf in get_fms(). Can you please explain what this check is for?
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 52: >> 10 Just divide by 'KiB'. The compiler will turn it in to a shift.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 58: let's make the most use out of it */ Please indicate what the logic is doing. I believe you are just trying to cover the most amount of the program.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex.
I'm not an expert, but would feel safer with CD set. Unless there is some implicit invalidation ofc.
There is not from my experiments doing it in assembly. I don't think CAR is accessible with CD set (not tested), so an assembly version copying all the function parameters from stack to registers is required if one wants to do this. CB:35722 does more or less that but it needs to be written more cleanly though.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog)
If this is being ran multiple times aren't we just accumulating MTRRs? Admittedly it's currently only possible to run in bootblock and verstage, but we would have accumulated 2 MTRRs then.
That's correct and tested to work fine.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) {
I'm not sure this a valid check. x86 is already checked for 0xf in get_fms(). Can you please explain what this check is for?
family 0xf, pentium 4, has CAR issues when ROM is cached, this is also done in cpu/intel/car/p4/cache_as_ram.S:
/* * An unidentified combination of speculative reads and branch * predictions inside WRPROT-cacheable memory can cause invalidation * of cachelines and loss of stack on models based on NetBurst * microarchitecture. Therefore disable WRPROT region entirely for * all family F models. */
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 58: let's make the most use out of it */
Please indicate what the logic is doing. I believe you are just trying to cover the most amount of the program.
That's correct.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog)
If this is being ran multiple times aren't we just accumulating MTRRs? Admittedly it's currently o […]
Working fine is a function of the ROM layout and cpu cache microarchitecture. Please don't assume all variations will continue to work when those parameters change.
Also, please add a comment somewhere how this is known to accumulate MTRRs when it's executed multiple times.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) {
I'm not sure this a valid check. x86 is already checked for 0xf in get_fms(). […]
My point is that the x86 field is manipulated when it's == 0xf. Are you suggesting that after the manipulation the family will remain 0xf on netburst?
Either way, there needs to be a comment added describing what you are trying to accomplish.
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#9).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 122 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 60: if (cpu_info.x86 == 0xf) { Statements should start on a tabstop
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#10).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex.
I'm not an expert, but would feel safer with CD set. Unless there is some implicit invalidation ofc.
There is not from my experiments doing it in assembly. I don't think CAR is accessible with CD set (not tested), so an assembly version copying all the function parameters from stack to registers is required if one wants to do this. CB:35722 does more or less that but it needs to be written more cleanly though.
I recently tried CAR with CR0.CD set instead of unset. With the rom cached, no matter what type, it caused problems for CAR, with only a CAR region set up not. I'm not so sure if setting CR0.CD is a good idea here but I can try.
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... PS6, Line 59: mtrr_base = ALIGN_UP(base, mtrr_mask_size);
Still, I think we need to override the ALIGN_DOWN() above. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 20: #define MAX_ROM_CACHE_SIZE (256 * KiB)
What's the reasoning for only limiting to 256KiB? Please document your assumptions/thinking.
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog)
Working fine is a function of the ROM layout and cpu cache microarchitecture. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) {
My point is that the x86 field is manipulated when it's == 0xf. Are you suggesting that after the manipulation the family will remain 0xf on netburst?
Ack. Netburst is exactly 0xf even with addition of the extended family bits (0).
Either way, there needs to be a comment added describing what you are trying to accomplish.
ack.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 52: >> 10
Just divide by 'KiB'. The compiler will turn it in to a shift.
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 58: let's make the most use out of it */
Please indicate what the logic is doing. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex.
I'm not an expert, but would feel safer with CD set. Unless there is […]
To be clear, I meant having CD set during the MTRR write. So, clearing it again afterwards. It just seemed odd to me to change caching while caching is enabled, but my knowledge is really humble when it comes to caching...
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#11).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/11/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/11/src/cpu/x86/mtrr/xip_cache... PS11, Line 104: const uint32_t size_covered = MIN(mtrr_base + mtrr_size, end) - MAX(mtrr_base, base); line over 96 characters
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex.
To be clear, I meant having CD set during the MTRR write. So, clearing it again afterwards. It just seemed odd to me to change caching while caching is enabled, but my knowledge is really humble when it comes to caching...
Done in CB:36382
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#12).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 132 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/12
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 12:
This dropped the CR0.CD bit setting when adding MTRR to cache ROM. This should not be needed according to the "Intel® 64 and IA-32 Architectures Software Developer’s Manual".
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... PS12, Line 25: filed 'filled'
and non-overlapping.
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#13).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 132 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/13/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/13/src/cpu/x86/mtrr/xip_cache... PS13, Line 104: const uint32_t size_covered = MIN(mtrr_base + mtrr_size, end) - MAX(mtrr_base, base); line over 96 characters
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#14).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/14
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 14: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/14/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/14/src/cpu/x86/mtrr/xip_cache... PS14, Line 85: > Shouldn't this be >=?
... I see Aaron posted this question on a previous patchset, but I don't see a response to that.
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#15).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/15
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 15: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/14/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/14/src/cpu/x86/mtrr/xip_cache... PS14, Line 85: >
Shouldn't this be >=? […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 15: Code-Review+2
Hello Kyösti Mälkki, Aaron Durbin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#16).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/16
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 25: the MTRR are filed. */
And you are assuming there is no overlap of mtrrs.
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 26: uint32_t
why is this fixed width? size_t should be sufficient.
The set_var_mtrr only works uint32_t so that makes things easier.
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 29: u8
int
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 33: msr_t mtrr = rdmsr(MTRR_PHYS_MASK(i));
Add a comment about how we're ignoring caching type and why?
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 58: * all family F models.
So did we conclude that those models have an extended model number of 0? Or that it adds up to 0xf with the extended model number?
yes, Intel Netburst has exactly 0xf as model number with extended model (0) added .
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 82: >
You had < before. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 84: if (cache_used + mtrr_mask_size > MAX_CPU_CACHE)
This doesn't really prevent one from exceeding the MAX_CPU_CACHE as mtrr_mask_size may already have […]
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 104: base
Shouldn't this be mtrr_base ?
Done
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/12/src/cpu/x86/mtrr/xip_cache... PS12, Line 25: filed
'filled' […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 93: BIOS_WARNING Nit: why is this WARNING, when not caching at all is only NOTICE?
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 93: d Nit: %u (actually PRIu32, but %u is compatible to x86_64 anyway and looks better)
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 94: mtrr_size / KiB); Nit: looks like it fits 96 chars exactly.
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 111: } Why did the if turn into a loop? it seems easy to prove that it can't get better after the second iteration...
4 cases: 1. size_covered = end - base = size // maximum 2. size_covered = end - mtrr_base 3. size_covered = mtrr_base + mtrr_size - base 4. size_covered = mtrr_base + mtrr_size - mtrr_base = mtrr_size
The result of 2. is always worse than that of 4.
And the result of 4. is invariant.
In the first iteration we can only hit 1. or 3.
From the second iteration on we can only hit 2. or 4.
And if we hit 1. or 2., it's the last iteration.
So possible runs are (1|34*2).
1 => done, 34.. => can't get better, 32 => done
if (mtrr_base + mtrr_size < end && /* not 1 */ mtrr_base + mtrr_size - base < MIN(mtrr_base + 2 * mtrr_size, end) - (mtrr_base + mtrr_size) /* better of 3 or 2/4 */) mtrr_base += mtrr_size;
obviously doesn't look better than the loop... how about
mtrr_base = ALIGN_DOWN(base, mtrr_size); if (mtrr_base + mtrr_size < end) { printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!", mtrr_size / KiB); /* Check if we can cover a bigger range by aligning up. */ const uint32_t alt_base = ALIGN_UP(base, mtrr_size); const uint32_t lower_coverage = mtrr_base + mtrr_size - base; const uint32_t upper_coverage = MIN(alt_base + mtrr_size, end) - alt_base; if (upper_coverage > lower_coverage) mtrr_base = alt_base; }
Soooo, the loop is not wrong, and an exhaustive search doesn't hurt more than my urge for explicitness. I won't block it if you prefer it.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/16/src/cpu/x86/mtrr/xip_cache... PS16, Line 111: }
Why did the if turn into a loop? it seems easy to prove that it can't get better after the second iteration...
4 cases:
- size_covered = end - base = size // maximum
- size_covered = end - mtrr_base
- size_covered = mtrr_base + mtrr_size - base
- size_covered = mtrr_base + mtrr_size - mtrr_base = mtrr_size
The result of 2. is always worse than that of 4.
And the result of 4. is invariant.
In the first iteration we can only hit 1. or 3.
From the second iteration on we can only hit 2. or 4.
And if we hit 1. or 2., it's the last iteration.
So possible runs are (1|34*2).
1 => done, 34.. => can't get better, 32 => done
if (mtrr_base + mtrr_size < end && /* not 1 */ mtrr_base + mtrr_size - base < MIN(mtrr_base + 2 * mtrr_size, end) - (mtrr_base + mtrr_size) /* better of 3 or 2/4 */) mtrr_base += mtrr_size;obviously doesn't look better than the loop... how about
mtrr_base = ALIGN_DOWN(base, mtrr_size); if (mtrr_base + mtrr_size < end) { printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!", mtrr_size / KiB); /* Check if we can cover a bigger range by aligning up. */ const uint32_t alt_base = ALIGN_UP(base, mtrr_size); const uint32_t lower_coverage = mtrr_base + mtrr_size - base; const uint32_t upper_coverage = MIN(alt_base + mtrr_size, end) - alt_base; if (upper_coverage > lower_coverage) mtrr_base = alt_base; }Soooo, the loop is not wrong, and an exhaustive search doesn't hurt more than my urge for explicitness. I won't block it if you prefer it.
I like your approach better.
Hello Kyösti Mälkki, Aaron Durbin, Angel Pons, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#17).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 127 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... PS17, Line 92: if (ALIGN_DOWN(base, mtrr_size) + mtrr_size < end) { braces {} are not necessary for single statement blocks
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... PS17, Line 92: if (ALIGN_DOWN(base, mtrr_size) + mtrr_size < end) { : printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!", mtrr_size / KiB); : } : stale
Hello Kyösti Mälkki, Aaron Durbin, Angel Pons, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35993
to look at the new patch set (#18).
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 123 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/18
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/17/src/cpu/x86/mtrr/xip_cache... PS17, Line 92: if (ALIGN_DOWN(base, mtrr_size) + mtrr_size < end) { : printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!", mtrr_size / KiB); : } :
stale
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 18: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 21: total amount of cache
fwiw, the topology and microarchitecture behavior matters. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/9/src/cpu/x86/mtrr/xip_cache.... PS9, Line 26: uint32_t
why is this fixed width? size_t should be sufficient. […]
Done
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
cpu/x86: Add a prog_run hook to set up caching of XIP stages
Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage.
TODO: now a limit of 256KiB is set for the total amount of cache that can be used. This should fit most use cases for the time being.
Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/35993 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 123 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index a8cf54d..b316c1f 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -78,6 +78,16 @@ depends on !NO_FIXED_XIP_ROM_SIZE default 0x10000
+config SETUP_XIP_CACHE + bool + depends on C_ENVIRONMENT_BOOTBLOCK + depends on !NO_XIP_EARLY_STAGES + help + Select this option to set up an MTRR to cache XIP stages loaded + from the bootblock. This is useful on platforms lacking a + non-eviction mode and therefore need to be careful to avoid + eviction. + config CPU_ADDR_BITS int default 36 diff --git a/src/cpu/x86/mtrr/Makefile.inc b/src/cpu/x86/mtrr/Makefile.inc index caa6e9c..129d05d 100644 --- a/src/cpu/x86/mtrr/Makefile.inc +++ b/src/cpu/x86/mtrr/Makefile.inc @@ -2,8 +2,12 @@
romstage-y += earlymtrr.c bootblock-y += earlymtrr.c +verstage-y += earlymtrr.c
bootblock-y += debug.c romstage-y += debug.c postcar-y += debug.c ramstage-y += debug.c + +bootblock-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c +verstage-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c diff --git a/src/cpu/x86/mtrr/xip_cache.c b/src/cpu/x86/mtrr/xip_cache.c new file mode 100644 index 0000000..112c0dfb --- /dev/null +++ b/src/cpu/x86/mtrr/xip_cache.c @@ -0,0 +1,109 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/cpu.h> +#include <program_loading.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <cpu/x86/mtrr.h> + +/* For now this is a good lowest common denominator for the total CPU cache. + TODO: fetch the total amount of cache from CPUID leaf2. */ +#define MAX_CPU_CACHE (256 * KiB) + +/* This makes the 'worst' case assumption that all cachelines covered by + the MTRR, no matter the caching type, are filled and not overlapping. */ +static uint32_t max_cache_used(void) +{ + msr_t msr = rdmsr(MTRR_CAP_MSR); + int i, total_mtrrs = msr.lo & MTRR_CAP_VCNT; + uint32_t total_cache = 0; + + for (i = 0; i < total_mtrrs; i++) { + msr_t mtrr = rdmsr(MTRR_PHYS_MASK(i)); + if (!(mtrr.lo & MTRR_PHYS_MASK_VALID)) + continue; + total_cache += ~(mtrr.lo & 0xfffff000) + 1; + } + return total_cache; +} + +void platform_prog_run(struct prog *prog) +{ + const uint32_t base = region_device_offset(&prog->rdev); + const uint32_t size = region_device_sz(&prog->rdev); + const uint32_t end = base + size; + const uint32_t cache_used = max_cache_used(); + /* This will accumulate MTRR's as XIP stages are run. + For now this includes bootblock which sets ups its own + caching elsewhere, verstage and romstage */ + int mtrr_num = get_free_var_mtrr(); + uint32_t mtrr_base; + uint32_t mtrr_size = 4 * KiB; + struct cpuinfo_x86 cpu_info; + + get_fms(&cpu_info, cpuid_eax(1)); + /* + * An unidentified combination of speculative reads and branch + * predictions inside WRPROT-cacheable memory can cause invalidation + * of cachelines and loss of stack on models based on NetBurst + * microarchitecture. Therefore disable WRPROT region entirely for + * all family F models. + */ + if (cpu_info.x86 == 0xf) { + printk(BIOS_NOTICE, + "PROG_RUN: CPU does not support caching ROM\n" + "The next stage will run slowly\n"); + return; + } + + if (mtrr_num == -1) { + printk(BIOS_NOTICE, + "PROG_RUN: No MTRR available to cache ROM!\n" + "The next stage will run slowly!\n"); + return; + } + + if (cache_used + mtrr_size > MAX_CPU_CACHE) { + printk(BIOS_NOTICE, + "PROG_RUN: No more cache available for the next stage\n" + "The next stage will run slowly!\n"); + return; + } + + while (1) { + if (ALIGN_DOWN(base, mtrr_size) + mtrr_size >= end) + break; + if (cache_used + mtrr_size * 2 > MAX_CPU_CACHE) + break; + mtrr_size *= 2; + } + + mtrr_base = ALIGN_DOWN(base, mtrr_size); + if (mtrr_base + mtrr_size < end) { + printk(BIOS_NOTICE, "PROG_RUN: Limiting XIP cache to %uKiB!\n", + mtrr_size / KiB); + /* Check if we can cover a bigger range by aligning up. */ + const uint32_t alt_base = ALIGN_UP(base, mtrr_size); + const uint32_t lower_coverage = mtrr_base + mtrr_size - base; + const uint32_t upper_coverage = MIN(alt_base + mtrr_size, end) - alt_base; + if (upper_coverage > lower_coverage) + mtrr_base = alt_base; + } + + printk(BIOS_DEBUG, + "PROG_RUN: Setting MTRR to cache XIP stage. base: 0x%08x, size: 0x%08x\n", + mtrr_base, mtrr_size); + + set_var_mtrr(mtrr_num, mtrr_base, mtrr_size, MTRR_TYPE_WRPROT); +}
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35993/19/src/cpu/x86/mtrr/xip_cache... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/19/src/cpu/x86/mtrr/xip_cache... PS19, Line 66: The next stage will run slowly This line is missing the exclamation mark that the other two have...
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)