Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31527
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
cpu/x86/mtrr/mtrr.c:Avoid static scan false positive
Static scan does not know the contents of the fixed MTRR descriptor, so it has no way to eval the result for variable num_ranges. If num_ranges is less or equal to 0, the for loop will not be entered, and the values of fixed_msrs will not be set. Asserting that num_ranges is greater than 0 ensures the loop enters at least once.
BUG=b:112253891 TEST=build grunt
Change-Id: Ieec0ac432c745bde4b1700539c266625da6cfd77 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/31527/1
diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c index 23473df..3b23926 100644 --- a/src/cpu/x86/mtrr/mtrr.c +++ b/src/cpu/x86/mtrr/mtrr.c @@ -36,6 +36,7 @@ #include <arch/acpi.h> #include <memrange.h> #include <cpu/amd/mtrr.h> +#include <assert.h> #if IS_ENABLED(CONFIG_X86_AMD_FIXED_MTRRS) #define MTRR_FIXED_WRBACK_BITS (MTRR_READ_MEM | MTRR_WRITE_MEM) #else @@ -331,6 +332,7 @@
desc = &fixed_mtrr_desc[i]; num_ranges = (desc->end - desc->begin) / desc->step; + ASSERT (num_ranges > 0); for (j = 0; j < num_ranges; j += RANGES_PER_FIXED_MTRR) { msr_index[msr_num] = desc->msr_index_base + (j / RANGES_PER_FIXED_MTRR);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31527 )
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c@335 PS1, Line 335: ASSERT (num_ranges > 0); space prohibited between function name and open parenthesis '('
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31527 )
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c@335 PS1, Line 335: If you could fix this. Excluding vendorcode, the only location with spaces after assert is in src/northbridge/amd/amdht/h3finit.c
$ git grep 'ASSERT (' | grep -v vendorcode
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31527 )
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/#/c/31527/1/src/cpu/x86/mtrr/mtrr.c@335 PS1, Line 335:
If you could fix this. […]
Ok, will do.
Hello Marshall Dawson, build bot (Jenkins), Martin Roth, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31527
to look at the new patch set (#2).
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
cpu/x86/mtrr/mtrr.c:Avoid static scan false positive
Static scan does not know the contents of the fixed MTRR descriptor, so it has no way to eval the result for variable num_ranges. If num_ranges is less or equal to 0, the for loop will not be entered, and the values of fixed_msrs will not be set. Asserting that num_ranges is greater than 0 ensures the loop enters at least once.
BUG=b:112253891 TEST=build grunt
Change-Id: Ieec0ac432c745bde4b1700539c266625da6cfd77 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/31527/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31527 )
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31527 )
Change subject: cpu/x86/mtrr/mtrr.c:Avoid static scan false positive ......................................................................
cpu/x86/mtrr/mtrr.c:Avoid static scan false positive
Static scan does not know the contents of the fixed MTRR descriptor, so it has no way to eval the result for variable num_ranges. If num_ranges is less or equal to 0, the for loop will not be entered, and the values of fixed_msrs will not be set. Asserting that num_ranges is greater than 0 ensures the loop enters at least once.
BUG=b:112253891 TEST=build grunt
Change-Id: Ieec0ac432c745bde4b1700539c266625da6cfd77 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-on: https://review.coreboot.org/c/31527 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/cpu/x86/mtrr/mtrr.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/cpu/x86/mtrr/mtrr.c b/src/cpu/x86/mtrr/mtrr.c index 23473df..249d631 100644 --- a/src/cpu/x86/mtrr/mtrr.c +++ b/src/cpu/x86/mtrr/mtrr.c @@ -36,6 +36,7 @@ #include <arch/acpi.h> #include <memrange.h> #include <cpu/amd/mtrr.h> +#include <assert.h> #if IS_ENABLED(CONFIG_X86_AMD_FIXED_MTRRS) #define MTRR_FIXED_WRBACK_BITS (MTRR_READ_MEM | MTRR_WRITE_MEM) #else @@ -331,6 +332,7 @@
desc = &fixed_mtrr_desc[i]; num_ranges = (desc->end - desc->begin) / desc->step; + ASSERT(num_ranges > 0); for (j = 0; j < num_ranges; j += RANGES_PER_FIXED_MTRR) { msr_index[msr_num] = desc->msr_index_base + (j / RANGES_PER_FIXED_MTRR);
-
build bot (Jenkins) (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Richard Spiegel (Code Review)