Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
arch/x86/postcar_loader: utilize var_mtrr_context API
Now that there is a generic solution in mtrr subsystem utilize the API.
BUG=b:155426691,b:155322763
Change-Id: Ie349d5669808928c7470c99d25c57c784174b4e9 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c 2 files changed, 21 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41850/1
diff --git a/src/arch/x86/include/arch/romstage.h b/src/arch/x86/include/arch/romstage.h index 8418366..28d29bf 100644 --- a/src/arch/x86/include/arch/romstage.h +++ b/src/arch/x86/include/arch/romstage.h @@ -5,6 +5,7 @@
#include <stddef.h> #include <stdint.h> +#include <cpu/x86/mtrr.h>
void mainboard_romstage_entry(void);
@@ -16,10 +17,8 @@
struct postcar_frame { uintptr_t stack; - uint32_t upper_mask; - int max_var_mtrrs; - int num_var_mtrrs; int skip_common_mtrr; + struct var_mtrr_context ctx; };
/* diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 58c6db9..084384e 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -26,12 +26,7 @@
static void postcar_frame_prepare(struct postcar_frame *pcf) { - msr_t msr; - msr = rdmsr(MTRR_CAP_MSR); - - pcf->upper_mask = (1 << (cpu_phys_address_size() - 32)) - 1; - pcf->max_var_mtrrs = msr.lo & MTRR_CAP_VCNT; - pcf->num_var_mtrrs = 0; + var_mtrr_context_init(&pcf->ctx, pcf); }
int postcar_frame_init(struct postcar_frame *pcf, size_t stack_size) @@ -60,47 +55,25 @@ return 0; }
+static void postcar_var_mtrr_set(const struct var_mtrr_context *ctx, + uintptr_t addr, size_t size, + msr_t base, msr_t mask) +{ + struct postcar_frame *pcf = ctx->arg; + + printk(BIOS_DEBUG, "MTRR Range: Start=%lx End=%lx (Size %zx)\n", + addr, addr + size, size); + + stack_push(pcf, mask.hi); + stack_push(pcf, mask.lo); + stack_push(pcf, base.hi); + stack_push(pcf, base.lo); +} + void postcar_frame_add_mtrr(struct postcar_frame *pcf, uintptr_t addr, size_t size, int type) { - /* - * Utilize additional MTRRs if the specified size is greater than the - * base address alignment. - */ - while (size != 0) { - uint32_t addr_lsb; - uint32_t size_msb; - uint32_t mtrr_size; - - if (pcf->num_var_mtrrs >= pcf->max_var_mtrrs) { - printk(BIOS_ERR, "No more variable MTRRs: %d\n", - pcf->max_var_mtrrs); - return; - } - - addr_lsb = fls(addr); - size_msb = fms(size); - - /* All MTRR entries need to have their base aligned to the mask - * size. The maximum size is calculated by a function of the - * min base bit set and maximum size bit set. */ - if (addr_lsb > size_msb) - mtrr_size = 1 << size_msb; - else - mtrr_size = 1 << addr_lsb; - - printk(BIOS_DEBUG, "MTRR Range: Start=%lx End=%lx (Size %x)\n", - addr, addr + mtrr_size, mtrr_size); - - stack_push(pcf, pcf->upper_mask); - stack_push(pcf, ~(mtrr_size - 1) | MTRR_PHYS_MASK_VALID); - stack_push(pcf, 0); - stack_push(pcf, addr | type); - pcf->num_var_mtrrs++; - - size -= mtrr_size; - addr += mtrr_size; - } + var_mtrr_set_with_cb(&pcf->ctx, addr, size, type, postcar_var_mtrr_set); }
void postcar_frame_add_romcache(struct postcar_frame *pcf, int type) @@ -140,8 +113,8 @@ * Place the number of used variable MTRRs on stack then max number * of variable MTRRs supported in the system. */ - stack_push(pcf, pcf->num_var_mtrrs); - stack_push(pcf, pcf->max_var_mtrrs); + stack_push(pcf, pcf->ctx.used_var_mtrrs); + stack_push(pcf, pcf->ctx.max_var_mtrrs); }
static void finalize_load(uintptr_t *stack_top_ptr, uintptr_t stack_top)
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41850
to look at the new patch set (#2).
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
arch/x86/postcar_loader: utilize var_mtrr_context API
Now that there is a generic solution in mtrr subsystem utilize the API.
BUG=b:155426691,b:155322763
Change-Id: Ie349d5669808928c7470c99d25c57c784174b4e9 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c M src/drivers/amd/agesa/mtrr_fixme.c 3 files changed, 22 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41850/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41850/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41850/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:155426691,b:155322763 If this fixes problems, it’d be great if you summarized those.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41850/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41850/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:155426691,b:155322763
If this fixes problems, it’d be great if you summarized those.
It's using the earlymtrr common code. The algorithm in the previous patch, and noted in commit description, is lifted from this compilation unit. It's largely moving code around and making it more generic/usable in other contexts.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
arch/x86/postcar_loader: utilize var_mtrr_context API
Now that there is a generic solution in mtrr subsystem utilize the API.
BUG=b:155426691,b:155322763
Change-Id: Ie349d5669808928c7470c99d25c57c784174b4e9 Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41850 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/include/arch/romstage.h M src/arch/x86/postcar_loader.c M src/drivers/amd/agesa/mtrr_fixme.c 3 files changed, 22 insertions(+), 50 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/arch/x86/include/arch/romstage.h b/src/arch/x86/include/arch/romstage.h index 8418366..28d29bf 100644 --- a/src/arch/x86/include/arch/romstage.h +++ b/src/arch/x86/include/arch/romstage.h @@ -5,6 +5,7 @@
#include <stddef.h> #include <stdint.h> +#include <cpu/x86/mtrr.h>
void mainboard_romstage_entry(void);
@@ -16,10 +17,8 @@
struct postcar_frame { uintptr_t stack; - uint32_t upper_mask; - int max_var_mtrrs; - int num_var_mtrrs; int skip_common_mtrr; + struct var_mtrr_context ctx; };
/* diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 58c6db9..084384e 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -26,12 +26,7 @@
static void postcar_frame_prepare(struct postcar_frame *pcf) { - msr_t msr; - msr = rdmsr(MTRR_CAP_MSR); - - pcf->upper_mask = (1 << (cpu_phys_address_size() - 32)) - 1; - pcf->max_var_mtrrs = msr.lo & MTRR_CAP_VCNT; - pcf->num_var_mtrrs = 0; + var_mtrr_context_init(&pcf->ctx, pcf); }
int postcar_frame_init(struct postcar_frame *pcf, size_t stack_size) @@ -60,47 +55,25 @@ return 0; }
+static void postcar_var_mtrr_set(const struct var_mtrr_context *ctx, + uintptr_t addr, size_t size, + msr_t base, msr_t mask) +{ + struct postcar_frame *pcf = ctx->arg; + + printk(BIOS_DEBUG, "MTRR Range: Start=%lx End=%lx (Size %zx)\n", + addr, addr + size, size); + + stack_push(pcf, mask.hi); + stack_push(pcf, mask.lo); + stack_push(pcf, base.hi); + stack_push(pcf, base.lo); +} + void postcar_frame_add_mtrr(struct postcar_frame *pcf, uintptr_t addr, size_t size, int type) { - /* - * Utilize additional MTRRs if the specified size is greater than the - * base address alignment. - */ - while (size != 0) { - uint32_t addr_lsb; - uint32_t size_msb; - uint32_t mtrr_size; - - if (pcf->num_var_mtrrs >= pcf->max_var_mtrrs) { - printk(BIOS_ERR, "No more variable MTRRs: %d\n", - pcf->max_var_mtrrs); - return; - } - - addr_lsb = fls(addr); - size_msb = fms(size); - - /* All MTRR entries need to have their base aligned to the mask - * size. The maximum size is calculated by a function of the - * min base bit set and maximum size bit set. */ - if (addr_lsb > size_msb) - mtrr_size = 1 << size_msb; - else - mtrr_size = 1 << addr_lsb; - - printk(BIOS_DEBUG, "MTRR Range: Start=%lx End=%lx (Size %x)\n", - addr, addr + mtrr_size, mtrr_size); - - stack_push(pcf, pcf->upper_mask); - stack_push(pcf, ~(mtrr_size - 1) | MTRR_PHYS_MASK_VALID); - stack_push(pcf, 0); - stack_push(pcf, addr | type); - pcf->num_var_mtrrs++; - - size -= mtrr_size; - addr += mtrr_size; - } + var_mtrr_set_with_cb(&pcf->ctx, addr, size, type, postcar_var_mtrr_set); }
void postcar_frame_add_romcache(struct postcar_frame *pcf, int type) @@ -140,8 +113,8 @@ * Place the number of used variable MTRRs on stack then max number * of variable MTRRs supported in the system. */ - stack_push(pcf, pcf->num_var_mtrrs); - stack_push(pcf, pcf->max_var_mtrrs); + stack_push(pcf, pcf->ctx.used_var_mtrrs); + stack_push(pcf, pcf->ctx.max_var_mtrrs); }
static void finalize_load(uintptr_t *stack_top_ptr, uintptr_t stack_top) diff --git a/src/drivers/amd/agesa/mtrr_fixme.c b/src/drivers/amd/agesa/mtrr_fixme.c index 0a4f29a..a880ce7 100644 --- a/src/drivers/amd/agesa/mtrr_fixme.c +++ b/src/drivers/amd/agesa/mtrr_fixme.c @@ -59,7 +59,7 @@
/* Replicate non-UC MTRRs as left behind by AGESA. */ - for (i = 0; i < pcf->max_var_mtrrs; i++) { + for (i = 0; i < pcf->ctx.max_var_mtrrs; i++) { mask = rdmsr(MTRR_PHYS_MASK(i)); base = rdmsr(MTRR_PHYS_BASE(i)); u32 size = ~(mask.lo & ~0xfff) + 1;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41850 )
Change subject: arch/x86/postcar_loader: utilize var_mtrr_context API ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4681 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4680 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4679 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4678
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)