Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
cpu/x86: Cache stages for RESET_VECTOR_IN_RAM
Implement the platform_prog_run() hook to allocate and program a variable MTRR for the program about to run.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I57caccceac8550efe33f7273d279275cd6af6a68 --- M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/nonxip_cache.c 2 files changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38728/1
diff --git a/src/cpu/x86/mtrr/Makefile.inc b/src/cpu/x86/mtrr/Makefile.inc index 129d05d..72572a0 100644 --- a/src/cpu/x86/mtrr/Makefile.inc +++ b/src/cpu/x86/mtrr/Makefile.inc @@ -11,3 +11,7 @@
bootblock-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c verstage-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c + +bootblock-$(CONFIG_RESET_VECTOR_IN_RAM) += nonxip_cache.c +verstage-$(CONFIG_RESET_VECTOR_IN_RAM) += nonxip_cache.c +romstage-$(CONFIG_RESET_VECTOR_IN_RAM) += nonxip_cache.c diff --git a/src/cpu/x86/mtrr/nonxip_cache.c b/src/cpu/x86/mtrr/nonxip_cache.c new file mode 100644 index 0000000..334e829 --- /dev/null +++ b/src/cpu/x86/mtrr/nonxip_cache.c @@ -0,0 +1,66 @@ +/* + * 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 <stdint.h> +#include <cpu/x86/mtrr.h> +#include <console/console.h> +#include <program_loading.h> + +void reserve_non_xip_prog(struct prog *prog); // move me + +/* Set a variable MTRR to WB for the program about to run. + * It is up to the designer to ensure ranges are chosen carefully to avoid + * overlapping MTRRs + * TODO: consider creating a function for inserting ranges into existing + * var. MTRR settings instead. + */ +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; + int mtrr_num; + uint32_t mtrr_base; + uint32_t mtrr_size = 4 * KiB; + + mtrr_num = get_free_var_mtrr(); + 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 (1) { + if (ALIGN_DOWN(base, mtrr_size) + mtrr_size >= end) + break; + mtrr_size *= 2; + } + + mtrr_base = ALIGN_DOWN(base, mtrr_size); + if (mtrr_base + mtrr_size < end) { + printk(BIOS_NOTICE, "PROG_RUN: Limiting 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 stage. base: 0x%08x, size: 0x%08x\n", + mtrr_base, mtrr_size); + + set_var_mtrr(mtrr_num, mtrr_base, mtrr_size, MTRR_TYPE_WRBACK); +}
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 1:
@Aaron and @Arthur, curious what you think of this and the next one of "Set/get DRAM consumed by..." as opposed to trying to do a lot of it in soc//picasso. BTW, I would really prefer to cache _before_ loading and not just before running the early stages, but this may be a reasonable intermediate step.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 1:
Patch Set 1:
@Aaron and @Arthur, curious what you think of this and the next one of "Set/get DRAM consumed by..." as opposed to trying to do a lot of it in soc//picasso. BTW, I would really prefer to cache _before_ loading and not just before running the early stages, but this may be a reasonable intermediate step.
topic:"WIP_wb_cache_postcar" attempts to set up caching before loading anything into cbmem/TSEG (postcar+ramstage). A guess a similar/unified approach could be used for romstage in dram?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38728/1/src/cpu/x86/mtrr/nonxip_cac... File src/cpu/x86/mtrr/nonxip_cache.c:
https://review.coreboot.org/c/coreboot/+/38728/1/src/cpu/x86/mtrr/nonxip_cac... PS1, Line 65: MTRR_TYPE_WRBACK Is this not pretty much the same as in cpu/x86/mtrr/xip_cache.c except for having no upper limit for size and also the type of the MTRR? Could the code be made common?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 1:
Note that you can compress romstage but you want bootblock to set up caching before that. See 36613.
Felix Held has uploaded a new patch set (#2) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
cpu/x86: Cache stages for RESET_VECTOR_IN_RAM
Implement the platform_prog_run() hook to allocate and program a variable MTRR for the program about to run.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I57caccceac8550efe33f7273d279275cd6af6a68 --- M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/nonxip_cache.c 2 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38728/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/38728/1/src/cpu/x86/mtrr/nonxip_cac... File src/cpu/x86/mtrr/nonxip_cache.c:
https://review.coreboot.org/c/coreboot/+/38728/1/src/cpu/x86/mtrr/nonxip_cac... PS1, Line 19: void reserve_non_xip_prog(struct prog *prog); // move me What is this doing here?
https://review.coreboot.org/c/coreboot/+/38728/1/src/cpu/x86/mtrr/nonxip_cac... PS1, Line 65: MTRR_TYPE_WRBACK
Is this not pretty much the same as in cpu/x86/mtrr/xip_cache. […]
I'm sure you could extract some functionality, but I think it's fine the way it is. It's easy and straight forward to read.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 2: -Code-Review
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 2:
If we really want to land this let's move this thing to amd/picasso. It's fairly generic, and I think we can replace it with a longer term solution.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Patch Set 2:
The Picasso MTRR patches might have superseded this one
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38728 )
Change subject: cpu/x86: Cache stages for RESET_VECTOR_IN_RAM ......................................................................
Abandoned
CB:42103
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Raul Rangel (Code Review)