Hello Marc Jones,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to review the following change.
Change subject: cpu/x86/mtrr: add support for base address bigger than 4GB ......................................................................
cpu/x86/mtrr: add support for base address bigger than 4GB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
The base address (after 4k page size adjustment) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t.
Add fls64() to find least significant bit set in a 64-bit word. Right now a stop-gap method is used. The 64-bit assembly command causes compilation error.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c M src/include/cpu/x86/mtrr.h 2 files changed, 25 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/1
diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c index 77525a7..27384cc 100644 --- a/src/cpu/x86/mtrr/mtrr.c +++ b/src/cpu/x86/mtrr/mtrr.c @@ -121,12 +121,12 @@ #define MTRR_ALGO_SHIFT (8) #define MTRR_TAG_MASK ((1 << MTRR_ALGO_SHIFT) - 1)
-static inline uint32_t range_entry_base_mtrr_addr(struct range_entry *r) +static inline uint64_t range_entry_base_mtrr_addr(struct range_entry *r) { return PHYS_TO_RANGE_ADDR(range_entry_base(r)); }
-static inline uint32_t range_entry_end_mtrr_addr(struct range_entry *r) +static inline uint64_t range_entry_end_mtrr_addr(struct range_entry *r) { return PHYS_TO_RANGE_ADDR(range_entry_end(r)); } @@ -402,7 +402,7 @@ }
static void prep_var_mtrr(struct var_mtrr_state *var_state, - uint32_t base, uint32_t size, int mtrr_type) + uint64_t base, uint64_t size, int mtrr_type) { struct var_mtrr_regs *regs; resource_t rbase; @@ -444,15 +444,30 @@ regs->mask.hi = rsize >> 32; }
+/* fls64: find least significant bit set in a 64-bit word*/ +static uint32_t fls64(uint64_t x) +{ + if (x == 0) return 0; + uint32_t pos = 0; + while (!(x & 1)) { + x >>= 1; + ++pos; + } + return pos; +} + static void calc_var_mtrr_range(struct var_mtrr_state *var_state, - uint32_t base, uint32_t size, int mtrr_type) + uint64_t base, uint64_t size, int mtrr_type) { while (size != 0) { uint32_t addr_lsb; uint32_t size_msb; - uint32_t mtrr_size; + uint64_t mtrr_size;
- addr_lsb = fls(base); + if (base > 0x100000000) + addr_lsb = fls64(base); + else + addr_lsb = fls(base); size_msb = fms(size);
/* All MTRR entries need to have their base aligned to the mask @@ -531,7 +546,7 @@ static void calc_var_mtrrs_with_hole(struct var_mtrr_state *var_state, struct range_entry *r) { - uint32_t a1, a2, b1, b2; + uint64_t a1, a2, b1, b2; int mtrr_type, carve_hole;
/* @@ -671,6 +686,8 @@ wb_deftype_count += var_state.mtrr_index; } } + printk(BIOS_DEBUG, "%s %d, uc_deftype_count: %d, wb_deftype_count: %d\n", __func__, __LINE__, uc_deftype_count, wb_deftype_count); + *num_def_wb_mtrrs = wb_deftype_count; *num_def_uc_mtrrs = uc_deftype_count; } diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index 3bf8301c..1497387 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -173,6 +173,7 @@ "1:" : "=r" (r) : "mr" (x)); return r; } + #endif /* !defined(__ASSEMBLER__) */
/* Align up/down to next power of 2, suitable for assembler
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for base address bigger than 4GB ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46435/1/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/1/src/cpu/x86/mtrr/mtrr.c@450 PS1, Line 450: if (x == 0) return 0; trailing statements should be on next line
https://review.coreboot.org/c/coreboot/+/46435/1/src/cpu/x86/mtrr/mtrr.c@689 PS1, Line 689: printk(BIOS_DEBUG, "%s %d, uc_deftype_count: %d, wb_deftype_count: %d\n", __func__, __LINE__, uc_deftype_count, wb_deftype_count); line over 96 characters
Hello Marc Jones, Anjaneya "Reddy" Chagam, Johnny Lin, Rocky Phagura, Aaron Durbin, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to look at the new patch set (#2).
Change subject: cpu/x86/mtrr: add support for base address bigger than 4GB ......................................................................
cpu/x86/mtrr: add support for base address bigger than 4GB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
The base address (after 4k page size adjustment) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t.
Add fls64() to find least significant bit set in a 64-bit word. Right now a stop-gap method is used. The 64-bit assembly command causes compilation error.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 23 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for base address bigger than 4GB ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@450 PS2, Line 450: if (x == 0) return 0; trailing statements should be on next line
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for base address bigger than 4GB ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@108 PS2, Line 108: 32-bit numbers. comment needs an update since it seems 16TiB of address space is being exceeded.
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@119 PS2, Line 119: 1 Change these to ULL
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@468 PS2, Line 468: addr_lsb = fls64(base); Why wouldn't you unconditionally call fls64()? And in the implementation split the 64 bit value into 2 32-bit ones using fls()?
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Johnny Lin, Rocky Phagura, Aaron Durbin, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to look at the new patch set (#3).
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
cpu/x86/mtrr: add support for address space higher than 16TiB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
In this case, the base address (with 4k granularity) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t. Add fls64() to find least significant bit set in a 64-bit word. Add fms64() to find most significant bit set in a 64-bit word.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 38 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46435/3/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/3/src/cpu/x86/mtrr/mtrr.c@451 PS3, Line 451: else { else is not generally useful after a break or return
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@108 PS2, Line 108: 32-bit numbers.
comment needs an update since it seems 16TiB of address space is being exceeded.
Done
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@119 PS2, Line 119: 1
Change these to ULL
Done
https://review.coreboot.org/c/coreboot/+/46435/2/src/cpu/x86/mtrr/mtrr.c@468 PS2, Line 468: addr_lsb = fls64(base);
Why wouldn't you unconditionally call fls64()? And in the implementation split the 64 bit value into […]
Done
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Johnny Lin, Rocky Phagura, Aaron Durbin, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to look at the new patch set (#4).
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
cpu/x86/mtrr: add support for address space higher than 16TiB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
In this case, the base address (with 4k granularity) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t. Add fls64() to find least significant bit set in a 64-bit word. Add fms64() to find most significant bit set in a 64-bit word.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 35 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c@120 PS4, Line 120: 1 Update this as well?
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c@463 PS4, Line 463: return fms(x >> 32) + 32; Did you double check the semantics of fls and fms? I recall those being sorta odd.
Looks like fms() returns 0 on no bits set and fls() returns 32 when not bits set.
I think you have the conditions wrong for the failing cases. Please double check and confirm.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 4:
(2 comments)
Thanks for the review!
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c@120 PS4, Line 120: 1
Update this as well?
MTRR_TAG_MASK is only used inrange_entry_mtrr_type(): return range_entry_tag(r) & MTRR_TAG_MASK;
range_entry_tag() returns (unsigned) long. So we do not need to update this with ULL.
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c@463 PS4, Line 463: return fms(x >> 32) + 32;
Did you double check the semantics of fls and fms? I recall those being sorta odd. […]
Done
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Johnny Lin, Rocky Phagura, Aaron Durbin, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to look at the new patch set (#5).
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
cpu/x86/mtrr: add support for address space higher than 16TiB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
In this case, the base address (with 4k granularity) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t. Add fls64() to find least significant bit set in a 64-bit word. Add fms64() to find most significant bit set in a 64-bit word.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 48 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46435/5/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/5/src/cpu/x86/mtrr/mtrr.c@477 PS5, Line 477: printk(BIOS_DEBUG, "fls(4400):%d fms(4400):%d fls64(4400):%d fms64(4400):%d\n", fls(0x4400), line over 96 characters
https://review.coreboot.org/c/coreboot/+/46435/5/src/cpu/x86/mtrr/mtrr.c@479 PS5, Line 479: printk(BIOS_DEBUG, "fls(0x404):%d fms(0x404):%d fls64(40400000000):%d fms64(40400000000):%d\n", fls(0x404), line over 96 characters
Hello build bot (Jenkins), Marc Jones, Anjaneya "Reddy" Chagam, Johnny Lin, Rocky Phagura, Aaron Durbin, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46435
to look at the new patch set (#6).
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
cpu/x86/mtrr: add support for address space higher than 16TiB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
In this case, the base address (with 4k granularity) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t. Add fls64() to find least significant bit set in a 64-bit word. Add fms64() to find most significant bit set in a 64-bit word.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 41 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/46435/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 6: Code-Review+2
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/4/src/cpu/x86/mtrr/mtrr.c@120 PS4, Line 120: 1
MTRR_TAG_MASK is only used inrange_entry_mtrr_type(): […]
Ack
Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
cpu/x86/mtrr: add support for address space higher than 16TiB
On DeltaLake server, there are following entry in MTRR address space: 0x0000201000000000 - 0x0000201000400000 size 0x00400000 type 0
In this case, the base address (with 4k granularity) cannot be held in uint32_t. This results incorrect MTRR register setup. As the consequence UEFI forum FWTS reports following critical error: Memory range 0x100000000 to 0x183fffffff (System RAM) has incorrect attribute Uncached.
Change appropriate variables' data type from uint32_t to uint64_t. Add fls64() to find least significant bit set in a 64-bit word. Add fms64() to find most significant bit set in a 64-bit word.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Marc Jones marcjones@sysproconsulting.com Change-Id: I41bc5befcc1374c838c91b9f7c5279ea76dd67c7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46435 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 41 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c index 77525a7..284a113 100644 --- a/src/cpu/x86/mtrr/mtrr.c +++ b/src/cpu/x86/mtrr/mtrr.c @@ -104,9 +104,7 @@
#define MTRR_VERBOSE_LEVEL BIOS_NEVER
-/* MTRRs are at a 4KiB granularity. Therefore all address calculations can - * be done with 32-bit numbers. This allows for the MTRR code to handle - * up to 2^44 bytes (16 TiB) of address space. */ +/* MTRRs are at a 4KiB granularity. */ #define RANGE_SHIFT 12 #define ADDR_SHIFT_TO_RANGE_SHIFT(x) \ (((x) > RANGE_SHIFT) ? ((x) - RANGE_SHIFT) : RANGE_SHIFT) @@ -115,18 +113,18 @@ #define NUM_FIXED_MTRRS (NUM_FIXED_RANGES / RANGES_PER_FIXED_MTRR)
/* Helpful constants. */ -#define RANGE_1MB PHYS_TO_RANGE_ADDR(1 << 20) -#define RANGE_4GB (1 << (ADDR_SHIFT_TO_RANGE_SHIFT(32))) +#define RANGE_1MB PHYS_TO_RANGE_ADDR(1ULL << 20) +#define RANGE_4GB (1ULL << (ADDR_SHIFT_TO_RANGE_SHIFT(32)))
#define MTRR_ALGO_SHIFT (8) #define MTRR_TAG_MASK ((1 << MTRR_ALGO_SHIFT) - 1)
-static inline uint32_t range_entry_base_mtrr_addr(struct range_entry *r) +static inline uint64_t range_entry_base_mtrr_addr(struct range_entry *r) { return PHYS_TO_RANGE_ADDR(range_entry_base(r)); }
-static inline uint32_t range_entry_end_mtrr_addr(struct range_entry *r) +static inline uint64_t range_entry_end_mtrr_addr(struct range_entry *r) { return PHYS_TO_RANGE_ADDR(range_entry_end(r)); } @@ -402,7 +400,7 @@ }
static void prep_var_mtrr(struct var_mtrr_state *var_state, - uint32_t base, uint32_t size, int mtrr_type) + uint64_t base, uint64_t size, int mtrr_type) { struct var_mtrr_regs *regs; resource_t rbase; @@ -444,16 +442,43 @@ regs->mask.hi = rsize >> 32; }
+/* + * fls64: find least significant bit set in a 64-bit word + * As samples, fls64(0x0) = 64; fls64(0x4400) = 10; + * fls64(0x40400000000) = 34. + */ +static uint32_t fls64(uint64_t x) +{ + uint32_t lo = (uint32_t)x; + if (lo) + return fls(lo); + uint32_t hi = x >> 32; + return fls(hi) + 32; +} + +/* + * fms64: find most significant bit set in a 64-bit word + * As samples, fms64(0x0) = 0; fms64(0x4400) = 14; + * fms64(0x40400000000) = 42. + */ +static uint32_t fms64(uint64_t x) +{ + uint32_t hi = (uint32_t)(x >> 32); + if (!hi) + return fms((uint32_t)x); + return fms(hi) + 32; +} + static void calc_var_mtrr_range(struct var_mtrr_state *var_state, - uint32_t base, uint32_t size, int mtrr_type) + uint64_t base, uint64_t size, int mtrr_type) { while (size != 0) { uint32_t addr_lsb; uint32_t size_msb; - uint32_t mtrr_size; + uint64_t mtrr_size;
- addr_lsb = fls(base); - size_msb = fms(size); + addr_lsb = fls64(base); + size_msb = fms64(size);
/* All MTRR entries need to have their base aligned to the mask * size. The maximum size is calculated by a function of the @@ -472,8 +497,8 @@ } }
-static uint32_t optimize_var_mtrr_hole(const uint32_t base, - const uint32_t hole, +static uint64_t optimize_var_mtrr_hole(const uint64_t base, + const uint64_t hole, const uint64_t limit, const int carve_hole) { @@ -531,7 +556,7 @@ static void calc_var_mtrrs_with_hole(struct var_mtrr_state *var_state, struct range_entry *r) { - uint32_t a1, a2, b1, b2; + uint64_t a1, a2, b1, b2; int mtrr_type, carve_hole;
/* @@ -671,6 +696,7 @@ wb_deftype_count += var_state.mtrr_index; } } + *num_def_wb_mtrrs = wb_deftype_count; *num_def_uc_mtrrs = uc_deftype_count; }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c@487 PS7, Line 487: mtrr_size = 1 << size_msb; We need to make these 1ULL as well
** CID 1435826: Integer handling issues (OVERFLOW_BEFORE_WIDEN) /src/cpu/x86/mtrr/mtrr.c: 487 in calc_var_mtrr_range()
________________________________________________________________________________________________________ *** CID 1435826: Integer handling issues (OVERFLOW_BEFORE_WIDEN) /src/cpu/x86/mtrr/mtrr.c: 487 in calc_var_mtrr_range() 481 size_msb = fms64(size); 482 483 /* All MTRR entries need to have their base aligned to the mask 484 * size. The maximum size is calculated by a function of the 485 * min base bit set and maximum size bit set. */ 486 if (addr_lsb > size_msb)
CID 1435826: Integer handling issues (OVERFLOW_BEFORE_WIDEN) Potentially overflowing expression "1 << size_msb" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned).
487 mtrr_size = 1 << size_msb; 488 else 489 mtrr_size = 1 << addr_lsb; 490 491 if (var_state->prepare_msrs) 492 prep_var_mtrr(var_state, base, mtrr_size, mtrr_type);
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c@489 PS7, Line 489: mtrr_size = 1 << addr_lsb; ** CID 1435825: Integer handling issues (OVERFLOW_BEFORE_WIDEN) /src/cpu/x86/mtrr/mtrr.c: 489 in calc_var_mtrr_range()
________________________________________________________________________________________________________ *** CID 1435825: Integer handling issues (OVERFLOW_BEFORE_WIDEN) /src/cpu/x86/mtrr/mtrr.c: 489 in calc_var_mtrr_range() 483 /* All MTRR entries need to have their base aligned to the mask 484 * size. The maximum size is calculated by a function of the 485 * min base bit set and maximum size bit set. */ 486 if (addr_lsb > size_msb) 487 mtrr_size = 1 << size_msb; 488 else
CID 1435825: Integer handling issues (OVERFLOW_BEFORE_WIDEN) Potentially overflowing expression "1 << addr_lsb" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned).
489 mtrr_size = 1 << addr_lsb; 490 491 if (var_state->prepare_msrs) 492 prep_var_mtrr(var_state, base, mtrr_size, mtrr_type); 493 494 size -= mtrr_size;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 7:
Is there any point left to do the 4K shifting?
There seem to be cases of uint32_t left, please check.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 7:
Patch Set 7:
Is there any point left to do the 4K shifting?
Probably not, but minimum granularity is 4KiB in variable MTRRs so that would need to be taken into account if the shift was removed.
There seem to be cases of uint32_t left, please check.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 7:
Is there any point left to do the 4K shifting?
Probably not, but minimum granularity is 4KiB in variable MTRRs so that would need to be taken into account if the shift was removed.
Well, that's a point :)
Let's keep it for now. I'll have a look at the inputs.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46435 )
Change subject: cpu/x86/mtrr: add support for address space higher than 16TiB ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c@487 PS7, Line 487: mtrr_size = 1 << size_msb;
We need to make these 1ULL as well […]
Sorry for missing this. I submitted [CB:46711].
https://review.coreboot.org/c/coreboot/+/46435/7/src/cpu/x86/mtrr/mtrr.c@489 PS7, Line 489: mtrr_size = 1 << addr_lsb;
** CID 1435825: Integer handling issues (OVERFLOW_BEFORE_WIDEN) […]
Sorry for missing this. I submitted [CB:46711].
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Nico Huber (Code Review)