Change in coreboot[master]: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen

[Name of user not set (Code Review)]

[e-mail] has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen
......................................................................

vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64_t instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/36081/1

diff --git a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
index 856a21a..b842c81 100644
--- a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
+++ b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
@@ -716,7 +716,7 @@
   UINT8 i;

DctMemLimit = 0;
-  if (DctInterleavedMemSize == NBPtr->DCTPtr->Timings.DctMemSize << 6) {
+  if (DctInterleavedMemSize == (uint64_t)(NBPtr->DCTPtr->Timings.DctMemSize) << 6) {
     // The whole memory range is interleaved for the DCTs with the minimum memory size
     for (i = 0; i < NBPtr->DctCount; i++) {
       DctMemLimit += DctInterleavedMemSize;

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36081/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36081/1//COMMIT_MSG@7 
PS1, Line 7: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen
Please make this a statement by adding a verb in imperative mood.
> Cast to UINT64 to avoid overflow

or something similar.

Change in coreboot[master]: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid...

[Name of user not set (Code Review)]

Hello Jacob Garber, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36081

to look at the new patch set (#2).

Change subject: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid overflow
......................................................................

vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid overflow

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64_t instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/36081/2

Change in coreboot[master]: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid...

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid overflow
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG@7 
PS2, Line 7: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI
This can be shortened to something like vc/amd/agesa/f16kb

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG@10 
PS2, Line 10: UINT64_t
UINT64 to be consistent.

https://review.coreboot.org/c/coreboot/+/36081/2/src/vendorcode/amd/agesa/f1... 
File src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c:

https://review.coreboot.org/c/coreboot/+/36081/2/src/vendorcode/amd/agesa/f1... 
PS2, Line 719: uint64_t
Though the AGESA types are more ugly, it's probably better to use UINT64 for consistency.

Change in coreboot[master]: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid...

[Name of user not set (Code Review)]

Hello Jacob Garber, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36081

to look at the new patch set (#3).

Change subject: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid overflow
......................................................................

vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Cast to UINT64 to avoid overflow

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64 instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/36081/3

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Name of user not set (Code Review)]

Hello Jacob Garber, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36081

to look at the new patch set (#4).

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64 instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/36081/4

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36081/4/src/vendorcode/amd/agesa/f1... 
File src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c:

https://review.coreboot.org/c/coreboot/+/36081/4/src/vendorcode/amd/agesa/f1... 
PS4, Line 719: DctMemSize
I don't think a DTC on this platform manage 64G of dram, so you could add a sanity check instead of the cast?

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Name of user not set (Code Review)]

Hello Jacob Garber, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36081

to look at the new patch set (#5).

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64 instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/36081/5

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 4:
>
> could add a sanity check instead of the cast?

I think I can, but Coverity would still complain, right? There are "intentional" and
"bug" categories in Coverity. Would you prefer a sanity check + "intentional" vs. a cast +
"bug"? I am fine with either as long as Coverity no longer claims it as an outstanding issue.

https://review.coreboot.org/c/coreboot/+/36081/2/src/vendorcode/amd/agesa/f1... 
File src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c:

https://review.coreboot.org/c/coreboot/+/36081/2/src/vendorcode/amd/agesa/f1... 
PS2, Line 719: uint64_t
> Though the AGESA types are more ugly, it's probably better to use UINT64 for consistency.

Done

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

Patch Set 5: Code-Review+2

(4 comments)

https://review.coreboot.org/c/coreboot/+/36081/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36081/1//COMMIT_MSG@7 
PS1, Line 7: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI: Overflow before widen
> Please make this a statement by adding a verb in imperative mood. […]

Done

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG@7 
PS2, Line 7: vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI
> This can be shortened to something like vc/amd/agesa/f16kb

Done

https://review.coreboot.org/c/coreboot/+/36081/2//COMMIT_MSG@10 
PS2, Line 10: UINT64_t
> UINT64 to be consistent.

Done

https://review.coreboot.org/c/coreboot/+/36081/4/src/vendorcode/amd/agesa/f1... 
File src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c:

https://review.coreboot.org/c/coreboot/+/36081/4/src/vendorcode/amd/agesa/f1... 
PS4, Line 719: DctMemSize
> I don't think a DTC on this platform manage 64G of dram, so you could add a sanity check instead of  […]

Doesn't hurt to line up your types though.

Change in coreboot[master]: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36081 )

Change subject: vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow
......................................................................

vc/amd/agesa/f16kb: Cast to UINT64 to avoid overflow

Evaluated using 32-bit arithmetic, then used in a context that
expects an expression of type UINT64. Cast to UINT64 instead.

Change-Id: I4f0aa26e116b47505633897c790ca8e86ea5dc4e
Signed-off-by: Joe Moore 
Found-by: Coverity CID 1241847
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36081
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
index 856a21a..2f919d7 100644
--- a/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
+++ b/src/vendorcode/amd/agesa/f16kb/Proc/Mem/Feat/DMI/mfDMI.c
@@ -716,7 +716,7 @@
   UINT8 i;

DctMemLimit = 0;
-  if (DctInterleavedMemSize == NBPtr->DCTPtr->Timings.DctMemSize << 6) {
+  if (DctInterleavedMemSize == (UINT64)(NBPtr->DCTPtr->Timings.DctMemSize) << 6) {
     // The whole memory range is interleaved for the DCTs with the minimum memory size
     for (i = 0; i < NBPtr->DctCount; i++) {
       DctMemLimit += DctInterleavedMemSize;