Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33381
Change subject: nb/intel/nehalem: Tidy quickpath_reserved calculation ......................................................................
nb/intel/nehalem: Tidy quickpath_reserved calculation
- Remove unnecessary braces - Move variable assignment out of function call - Do not find lowest bit set of 0, which is undefined - Use unsigned integer when bit shifting
Change-Id: I8651f8cd04165d8d31c44f7919ad5e43499d3d4c Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229562 --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 9 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/33381/1
diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c index 15d6abb..af9e620 100644 --- a/src/northbridge/intel/nehalem/raminit.c +++ b/src/northbridge/intel/nehalem/raminit.c @@ -1431,14 +1431,17 @@ memory_map[2] = TOUUD | 1; quickpath_reserved = 0;
- { - u32 t; + u32 t = pci_read_config32(PCI_DEV(QUICKPATH_BUS, 0, 1), 0x68);
- gav(t = pci_read_config32(PCI_DEV(QUICKPATH_BUS, 0, 1), 0x68)); - if (t & 0x800) - quickpath_reserved = - (1 << find_lowest_bit_set32(t >> 20)); + gav(t); + + if (t & 0x800) { + u32 shift = t >> 20; + if (shift == 0) + die("Quickpath value is 0\n"); + quickpath_reserved = (u32)1 << find_lowest_bit_set32(shift); } + if (memory_remap) TOUUD -= quickpath_reserved;
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33381 )
Change subject: nb/intel/nehalem: Tidy quickpath_reserved calculation ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33381 )
Change subject: nb/intel/nehalem: Tidy quickpath_reserved calculation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33381/1/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33381/1/src/northbridge/intel/nehalem/ramini... PS1, Line 1442: (u32)1 I've usually seen '1UL' or '1ULL'
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33381 )
Change subject: nb/intel/nehalem: Tidy quickpath_reserved calculation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33381/1/src/northbridge/intel/nehalem/ramini... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/#/c/33381/1/src/northbridge/intel/nehalem/ramini... PS1, Line 1442: (u32)1
I've usually seen '1UL' or '1ULL'
I've also seen those, but their widths depend on the target architecture. Currently 1UL would also work since sizeof(unsigned long) == 32, but there is no guarantee this will stay that way in the future, so I prefer explicit widths.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33381 )
Change subject: nb/intel/nehalem: Tidy quickpath_reserved calculation ......................................................................
nb/intel/nehalem: Tidy quickpath_reserved calculation
- Remove unnecessary braces - Move variable assignment out of function call - Do not find lowest bit set of 0, which is undefined - Use unsigned integer when bit shifting
Change-Id: I8651f8cd04165d8d31c44f7919ad5e43499d3d4c Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1229562 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33381 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/nehalem/raminit.c 1 file changed, 9 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c index 0b5f44c..b4ff85c 100644 --- a/src/northbridge/intel/nehalem/raminit.c +++ b/src/northbridge/intel/nehalem/raminit.c @@ -1434,14 +1434,17 @@ memory_map[2] = TOUUD | 1; quickpath_reserved = 0;
- { - u32 t; + u32 t = pci_read_config32(PCI_DEV(QUICKPATH_BUS, 0, 1), 0x68);
- gav(t = pci_read_config32(PCI_DEV(QUICKPATH_BUS, 0, 1), 0x68)); - if (t & 0x800) - quickpath_reserved = - (1 << find_lowest_bit_set32(t >> 20)); + gav(t); + + if (t & 0x800) { + u32 shift = t >> 20; + if (shift == 0) + die("Quickpath value is 0\n"); + quickpath_reserved = (u32)1 << find_lowest_bit_set32(shift); } + if (memory_remap) TOUUD -= quickpath_reserved;
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Jacob Garber (Code Review)
-
Patrick Georgi (Code Review)