Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
cpu/x86/mtrr: add helper for setting multiple MTRRs
Introduce concept of var_mtrr_context object for tracking and assigning MTRR values. The algorithm is lifted from postcar_loader code, but it's generalized for different type of users: setting MSRs explicitly or deferring to a particular caller's desired actions.
BUG=b:155426691,b:155322763
Change-Id: Ic03b4b617196f04071093bbba4cf28d23fa482d8 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/x86/mtrr/earlymtrr.c M src/include/cpu/x86/mtrr.h 2 files changed, 100 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41849/1
diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c index 5a2ce55..008d9fd 100644 --- a/src/cpu/x86/mtrr/earlymtrr.c +++ b/src/cpu/x86/mtrr/earlymtrr.c @@ -65,3 +65,72 @@ wrmsr(MTRR_PHYS_BASE(i), mtrr); } } + +void var_mtrr_context_init(struct var_mtrr_context *ctx, void *arg) +{ + ctx->upper_mask = (1U << (cpu_phys_address_size() - 32)) - 1; + ctx->max_var_mtrrs = get_var_mtrr_count(); + ctx->used_var_mtrrs = 0; + ctx->arg = arg; +} + +int var_mtrr_set_with_cb(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, + int type, void (*callback)(const struct var_mtrr_context *ctx, + uintptr_t base_addr, size_t size, + msr_t base, msr_t mask)) +{ + /* + * 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; + msr_t base; + msr_t mask; + + if (ctx->used_var_mtrrs >= ctx->max_var_mtrrs) { + printk(BIOS_ERR, "No more variable MTRRs: %d\n", + ctx->max_var_mtrrs); + return -1; + } + + 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; + + base.hi = (uint64_t)addr >> 32; + base.lo = addr | type; + mask.hi = ctx->upper_mask; + mask.lo = ~(mtrr_size - 1) | MTRR_PHYS_MASK_VALID; + callback(ctx, addr, mtrr_size, base, mask); + ctx->used_var_mtrrs++; + + size -= mtrr_size; + addr += mtrr_size; + } + + return 0; +} + +static void set_mtrr(const struct var_mtrr_context *ctx, uintptr_t base_addr, size_t size, + msr_t base, msr_t mask) +{ + int i = var_mtrr_context_current_mtrr(ctx); + + wrmsr(MTRR_PHYS_BASE(i), base); + wrmsr(MTRR_PHYS_MASK(i), mask); +} + +int var_mtrr_set(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, int type) +{ + return var_mtrr_set_with_cb(ctx, addr, size, type, set_mtrr); +} diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index 50148ff..552941e 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -111,6 +111,37 @@
asmlinkage void display_mtrrs(void);
+/* Variable MTRR structure to help track and set MTRRs prior to ramstage. This + * and the following APIs can be used to set up more complex MTRR solutions + * instead of open coding get_free_var_mtrr() and set_var_mtrr() or for determining + * a future solution, such as postcar_loader. */ +struct var_mtrr_context { + uint32_t upper_mask; + int max_var_mtrrs; + int used_var_mtrrs; + void *arg; /* optional callback parameter */ +}; + +/* Returns 0-relative MTRR from context. Use MTRR_PHYS_BASE|MASK macros for calculating + MSR address value. */ +static inline int var_mtrr_context_current_mtrr(const struct var_mtrr_context *ctx) +{ + return ctx->used_var_mtrrs; +} + +/* Initialize var_mtrr_context object. Assumes all variable MTRRs are not yet used. */ +void var_mtrr_context_init(struct var_mtrr_context *ctx, void *arg); +/* Allocate a variable mtrr base and mask, calling the provided callback for each MTRR + * MSR base-mask pair needed to accommodate the address and size request. Returns < 0 + * on error and 0 on success. */ +int var_mtrr_set_with_cb(struct var_mtrr_context *ctx, + uintptr_t addr, size_t size, int type, + void (*callback)(const struct var_mtrr_context *ctx, + uintptr_t base_addr, size_t size, + msr_t base, msr_t mask)); +/* Same as var_mtrr_set_with_cb() but just write the MSRs directly. */ +int var_mtrr_set(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, int type); + /* * Set the MTRRs using the data on the stack from setup_stack_and_mtrrs. * Return a new top_of_stack value which removes the setup_stack_and_mtrrs data.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 104: * min base bit set and maximum size bit set. */ https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 108: mtrr_size = 1 << addr_lsb; Some prefere the ternary operator to directly graps that it’s about assignment to `mtrr_size`, but it’s subjective and not important.
mtrr_size = (addr_lsb > size_msb) ? 1 << size_msb : 1 << addr_lsb;
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 117: * a future solution, such as postcar_loader. */ https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 121: int used_var_mtrrs; Can these be made of type unsigned or size_t?
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 127: int size_t/unsigned int
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 135: Returns < 0 Put that on the next line?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 104: * min base bit set and maximum size bit set. */
I can change the comments, but this is lifted from existing code in postcar_loader.c.
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h File src/include/cpu/x86/mtrr.h:
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 117: * a future solution, such as postcar_loader. */
Done
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 121: int used_var_mtrrs;
Can these be made of type unsigned or size_t?
They could. I don't think it matters.
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 127: int
size_t/unsigned int
I don't think it matters.
https://review.coreboot.org/c/coreboot/+/41849/1/src/include/cpu/x86/mtrr.h@... PS1, Line 135: Returns < 0
Put that on the next line?
Done
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41849
to look at the new patch set (#2).
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
cpu/x86/mtrr: add helper for setting multiple MTRRs
Introduce concept of var_mtrr_context object for tracking and assigning MTRR values. The algorithm is lifted from postcar_loader code, but it's generalized for different type of users: setting MSRs explicitly or deferring to a particular caller's desired actions.
BUG=b:155426691,b:155322763
Change-Id: Ic03b4b617196f04071093bbba4cf28d23fa482d8 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/x86/mtrr/earlymtrr.c M src/include/cpu/x86/mtrr.h 2 files changed, 98 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/41849/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... PS2, Line 72: ctx->max_var_mtrrs = get_var_mtrr_count(); Should this call clear_all_var_mtrr() to be sure that we start with no variable MTRRs already set?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... PS2, Line 72: ctx->max_var_mtrrs = get_var_mtrr_count();
Should this call clear_all_var_mtrr() to be sure that we start with no variable MTRRs already set?
I thought about that as well. We either need to clear all or sync the state. Sync'ing the state is way more complicated because all it can do is track the msr usage.
In the end I wanted to leave it to the callers because setting the MSRs directly may not be the intention. postcar_loader doesn't directly manipulate the MSRs, e.g.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/41849/2/src/cpu/x86/mtrr/earlymtrr.... PS2, Line 72: ctx->max_var_mtrrs = get_var_mtrr_count();
In the end I wanted to leave it to the callers because setting the MSRs directly may not be the intention. postcar_loader doesn't directly manipulate the MSRs, e.g
That makes sense. I was thinking that the caller could pass in a flag to ask var_mtrr_context_init() to do the clearing. But, it can as well call clear_all_var_mtrr() itself too.
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
cpu/x86/mtrr: add helper for setting multiple MTRRs
Introduce concept of var_mtrr_context object for tracking and assigning MTRR values. The algorithm is lifted from postcar_loader code, but it's generalized for different type of users: setting MSRs explicitly or deferring to a particular caller's desired actions.
BUG=b:155426691,b:155322763
Change-Id: Ic03b4b617196f04071093bbba4cf28d23fa482d8 Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41849 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/cpu/x86/mtrr/earlymtrr.c M src/include/cpu/x86/mtrr.h 2 files changed, 98 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c index 5a2ce55..879b8ef 100644 --- a/src/cpu/x86/mtrr/earlymtrr.c +++ b/src/cpu/x86/mtrr/earlymtrr.c @@ -65,3 +65,70 @@ wrmsr(MTRR_PHYS_BASE(i), mtrr); } } + +void var_mtrr_context_init(struct var_mtrr_context *ctx, void *arg) +{ + ctx->upper_mask = (1U << (cpu_phys_address_size() - 32)) - 1; + ctx->max_var_mtrrs = get_var_mtrr_count(); + ctx->used_var_mtrrs = 0; + ctx->arg = arg; +} + +int var_mtrr_set_with_cb(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, + int type, void (*callback)(const struct var_mtrr_context *ctx, + uintptr_t base_addr, size_t size, + msr_t base, msr_t mask)) +{ + /* 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; + msr_t base; + msr_t mask; + + if (ctx->used_var_mtrrs >= ctx->max_var_mtrrs) { + printk(BIOS_ERR, "No more variable MTRRs: %d\n", + ctx->max_var_mtrrs); + return -1; + } + + 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; + + base.hi = (uint64_t)addr >> 32; + base.lo = addr | type; + mask.hi = ctx->upper_mask; + mask.lo = ~(mtrr_size - 1) | MTRR_PHYS_MASK_VALID; + callback(ctx, addr, mtrr_size, base, mask); + ctx->used_var_mtrrs++; + + size -= mtrr_size; + addr += mtrr_size; + } + + return 0; +} + +static void set_mtrr(const struct var_mtrr_context *ctx, uintptr_t base_addr, size_t size, + msr_t base, msr_t mask) +{ + int i = var_mtrr_context_current_mtrr(ctx); + + wrmsr(MTRR_PHYS_BASE(i), base); + wrmsr(MTRR_PHYS_MASK(i), mask); +} + +int var_mtrr_set(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, int type) +{ + return var_mtrr_set_with_cb(ctx, addr, size, type, set_mtrr); +} diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index 301ea30..c1ee51a 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -110,6 +110,37 @@
asmlinkage void display_mtrrs(void);
+/* Variable MTRR structure to help track and set MTRRs prior to ramstage. This + and the following APIs can be used to set up more complex MTRR solutions + instead of open coding get_free_var_mtrr() and set_var_mtrr() or for determining + a future solution, such as postcar_loader. */ +struct var_mtrr_context { + uint32_t upper_mask; + int max_var_mtrrs; + int used_var_mtrrs; + void *arg; /* optional callback parameter */ +}; + +/* Returns 0-relative MTRR from context. Use MTRR_PHYS_BASE|MASK macros for calculating + MSR address value. */ +static inline int var_mtrr_context_current_mtrr(const struct var_mtrr_context *ctx) +{ + return ctx->used_var_mtrrs; +} + +/* Initialize var_mtrr_context object. Assumes all variable MTRRs are not yet used. */ +void var_mtrr_context_init(struct var_mtrr_context *ctx, void *arg); +/* Allocate a variable mtrr base and mask, calling the provided callback for each MTRR + MSR base-mask pair needed to accommodate the address and size request. + Returns < 0 on error and 0 on success. */ +int var_mtrr_set_with_cb(struct var_mtrr_context *ctx, + uintptr_t addr, size_t size, int type, + void (*callback)(const struct var_mtrr_context *ctx, + uintptr_t base_addr, size_t size, + msr_t base, msr_t mask)); +/* Same as var_mtrr_set_with_cb() but just write the MSRs directly. */ +int var_mtrr_set(struct var_mtrr_context *ctx, uintptr_t addr, size_t size, int type); + /* * Set the MTRRs using the data on the stack from setup_stack_and_mtrrs. * Return a new top_of_stack value which removes the setup_stack_and_mtrrs data.
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41849 )
Change subject: cpu/x86/mtrr: add helper for setting multiple MTRRs ......................................................................
Patch Set 3:
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/4685 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4684 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4683 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4682
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)