Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34375 )
Change subject: nb/amd/amdht: Use standard coreboot assertions ......................................................................
nb/amd/amdht: Use standard coreboot assertions
The amdht code currently relies on an idiosyncratic ASSERT() macro, which actually doesn't do anything right now, and even it did would only print a janky error message. Replace this with the normal ASSERT() macro from <assert.h>. The default behaviour now is to print an error message but do nothing else, and failed assertions will only halt if you enable FATAL_ASSERT, in which case, well, you asked for it.
Change-Id: I6db7565171a345f9afbc9fb37cff8fda58f942df Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1402076 --- M src/northbridge/amd/amdht/AsPsNb.c M src/northbridge/amd/amdht/comlib.c M src/northbridge/amd/amdht/comlib.h M src/northbridge/amd/amdht/h3ncmn.c M src/northbridge/amd/amdht/ht_wrapper.c M src/northbridge/amd/amdht/porting.h 6 files changed, 3 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/34375/1
diff --git a/src/northbridge/amd/amdht/AsPsNb.c b/src/northbridge/amd/amdht/AsPsNb.c index 2e6c103..70dbacf 100644 --- a/src/northbridge/amd/amdht/AsPsNb.c +++ b/src/northbridge/amd/amdht/AsPsNb.c @@ -13,9 +13,6 @@ * GNU General Public License for more details. */
-#undef FILECODE -#define FILECODE 0xCCCC - #include "comlib.h" #include "AsPsDefs.h" #include "AsPsNb.h" diff --git a/src/northbridge/amd/amdht/comlib.c b/src/northbridge/amd/amdht/comlib.c index 3c2477c..b36d4b4 100644 --- a/src/northbridge/amd/amdht/comlib.c +++ b/src/northbridge/amd/amdht/comlib.c @@ -13,8 +13,6 @@ * GNU General Public License for more details. */
-#undef FILECODE -#define FILECODE 0xCCCC #include "comlib.h"
#include <device/pci.h> @@ -239,55 +237,3 @@ msr.hi = Value->hi; wrmsr(Address, msr); } - - -void ErrorStop(u32 value) -{ - printk(BIOS_DEBUG, "Error: %08x ", value); - -} - -/*;---------------------------------------------------------------------------- -; void __pascal ErrorStop(DWORD Value); -; -; This implementation provides a rotating display of the error code on the -; a port 80h POST display card. The rotation is used to make it easier to -; view the error on both a 16-bit as well as a 32-bit display card. -; -; For use with SimNow the unrotated error code is also written to port 84h -ErrorStop PROC FAR PASCAL PUBLIC Value:DWORD - pushad - mov eax, Value - mov bx, 0DEADh - out 84h, eax - -ErrorStopTop: - out 80h, eax - - mov cx, 4 ; Rotate the display by one nibble -@@: - bt bx, 15 - rcl eax, 1 - rcl bx, 1 - loop @B - - - push eax ; Delay a few hundred milliseconds - push ebx - mov ecx, 10h ; TSC - db 00Fh, 032h ; RDMSR - mov ebx, eax -@@: - db 00Fh, 032h ; RDMSR - sub eax, ebx - cmp eax, 500000000 - jb @B - pop ebx - pop eax - - jmp ErrorStopTop - - popad - ret -ErrorStop ENDP -*/ diff --git a/src/northbridge/amd/amdht/comlib.h b/src/northbridge/amd/amdht/comlib.h index d497fd2..dbc19f3 100644 --- a/src/northbridge/amd/amdht/comlib.h +++ b/src/northbridge/amd/amdht/comlib.h @@ -16,25 +16,15 @@ #ifndef COMLIB_H #define COMLIB_H
-#undef FILECODE -#define FILECODE 0xF001 - +#include <assert.h> #include <inttypes.h> #include <stdlib.h> #include "porting.h"
-#ifdef AMD_DEBUG - #define ASSERT(x) ((x) ? 0 : ErrorStop(((uint32)FILECODE)*0x10000 + ((__LINE__)%10) + (((__LINE__/10)%10)*0x10) + (((__LINE__/100)%10)*0x100) +(((__LINE__/1000)%10)*0x1000))) -#else - #define ASSERT(x) -#endif - #ifdef AMD_DEBUG_ERROR_STOP - /* Macro to aid debugging, causes program to halt and display the line number of the halt in decimal */ - #define STOP_HERE ErrorStop(((uint32)FILECODE)*0x10000 + ((__LINE__)%10) + (((__LINE__/10)%10)*0x10) + (((__LINE__/100)%10)*0x100) +(((__LINE__/1000)%10)*0x1000)) + /* Macro to aid debugging, causes program to halt and display the line number of the halt */ + #define STOP_HERE ASSERT(0) #else - /* Macro to aid debugging, causes program to halt and display the line number of the halt in decimal */ - /* #define STOP_HERE STOP_HERE_OnlyForDebugUse */ #define STOP_HERE #endif
diff --git a/src/northbridge/amd/amdht/h3ncmn.c b/src/northbridge/amd/amdht/h3ncmn.c index 8370b58..830e988 100644 --- a/src/northbridge/amd/amdht/h3ncmn.c +++ b/src/northbridge/amd/amdht/h3ncmn.c @@ -21,8 +21,6 @@ *---------------------------------------------------------------------------- */
-#undef FILECODE -#define FILECODE 0xF002 #include "h3ncmn.h" #include "h3finit.h" #include "h3ffeat.h" diff --git a/src/northbridge/amd/amdht/ht_wrapper.c b/src/northbridge/amd/amdht/ht_wrapper.c index bad8993..89ff46e 100644 --- a/src/northbridge/amd/amdht/ht_wrapper.c +++ b/src/northbridge/amd/amdht/ht_wrapper.c @@ -36,7 +36,6 @@ #endif
/* Debugging Options */ -#define AMD_DEBUG 1 //#define AMD_DEBUG_ERROR_STOP 1
/*---------------------------------------------------------------------------- @@ -45,8 +44,6 @@ *---------------------------------------------------------------------------- */
-#undef FILECODE -#define FILECODE 0xFF01 #include "comlib.h" #include "h3gtopo.h" #include "h3finit.h" diff --git a/src/northbridge/amd/amdht/porting.h b/src/northbridge/amd/amdht/porting.h index a0f8878..9058d4d 100644 --- a/src/northbridge/amd/amdht/porting.h +++ b/src/northbridge/amd/amdht/porting.h @@ -71,7 +71,6 @@ void CALLCONV AmdPCIRead(SBDFO loc, uint32 *Value); void CALLCONV AmdPCIWrite(SBDFO loc, uint32 *Value); void CALLCONV AmdCPUIDRead(uint32 Address, uint32 Regs[4]); -void CALLCONV ErrorStop(uint32 Value);
#define BYTESIZE 1 #define WORDSIZE 2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34375 )
Change subject: nb/amd/amdht: Use standard coreboot assertions ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/34375/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34375/1//COMMIT_MSG@14 PS1, Line 14: in which case, well, you asked for it. lol
https://review.coreboot.org/c/coreboot/+/34375/1/src/northbridge/amd/amdht/c... File src/northbridge/amd/amdht/comlib.c:
https://review.coreboot.org/c/coreboot/+/34375/1/src/northbridge/amd/amdht/c... PS1, Line 251: I don't think I want to ask about this.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34375 )
Change subject: nb/amd/amdht: Use standard coreboot assertions ......................................................................
nb/amd/amdht: Use standard coreboot assertions
The amdht code currently relies on an idiosyncratic ASSERT() macro, which actually doesn't do anything right now, and even it did would only print a janky error message. Replace this with the normal ASSERT() macro from <assert.h>. The default behaviour now is to print an error message but do nothing else, and failed assertions will only halt if you enable FATAL_ASSERT, in which case, well, you asked for it.
Change-Id: I6db7565171a345f9afbc9fb37cff8fda58f942df Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1402076 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34375 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/amd/amdht/AsPsNb.c M src/northbridge/amd/amdht/comlib.c M src/northbridge/amd/amdht/comlib.h M src/northbridge/amd/amdht/h3ncmn.c M src/northbridge/amd/amdht/ht_wrapper.c M src/northbridge/amd/amdht/porting.h 6 files changed, 3 insertions(+), 76 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/amd/amdht/AsPsNb.c b/src/northbridge/amd/amdht/AsPsNb.c index 2e6c103..70dbacf 100644 --- a/src/northbridge/amd/amdht/AsPsNb.c +++ b/src/northbridge/amd/amdht/AsPsNb.c @@ -13,9 +13,6 @@ * GNU General Public License for more details. */
-#undef FILECODE -#define FILECODE 0xCCCC - #include "comlib.h" #include "AsPsDefs.h" #include "AsPsNb.h" diff --git a/src/northbridge/amd/amdht/comlib.c b/src/northbridge/amd/amdht/comlib.c index 3c2477c..b36d4b4 100644 --- a/src/northbridge/amd/amdht/comlib.c +++ b/src/northbridge/amd/amdht/comlib.c @@ -13,8 +13,6 @@ * GNU General Public License for more details. */
-#undef FILECODE -#define FILECODE 0xCCCC #include "comlib.h"
#include <device/pci.h> @@ -239,55 +237,3 @@ msr.hi = Value->hi; wrmsr(Address, msr); } - - -void ErrorStop(u32 value) -{ - printk(BIOS_DEBUG, "Error: %08x ", value); - -} - -/*;---------------------------------------------------------------------------- -; void __pascal ErrorStop(DWORD Value); -; -; This implementation provides a rotating display of the error code on the -; a port 80h POST display card. The rotation is used to make it easier to -; view the error on both a 16-bit as well as a 32-bit display card. -; -; For use with SimNow the unrotated error code is also written to port 84h -ErrorStop PROC FAR PASCAL PUBLIC Value:DWORD - pushad - mov eax, Value - mov bx, 0DEADh - out 84h, eax - -ErrorStopTop: - out 80h, eax - - mov cx, 4 ; Rotate the display by one nibble -@@: - bt bx, 15 - rcl eax, 1 - rcl bx, 1 - loop @B - - - push eax ; Delay a few hundred milliseconds - push ebx - mov ecx, 10h ; TSC - db 00Fh, 032h ; RDMSR - mov ebx, eax -@@: - db 00Fh, 032h ; RDMSR - sub eax, ebx - cmp eax, 500000000 - jb @B - pop ebx - pop eax - - jmp ErrorStopTop - - popad - ret -ErrorStop ENDP -*/ diff --git a/src/northbridge/amd/amdht/comlib.h b/src/northbridge/amd/amdht/comlib.h index d497fd2..dbc19f3 100644 --- a/src/northbridge/amd/amdht/comlib.h +++ b/src/northbridge/amd/amdht/comlib.h @@ -16,25 +16,15 @@ #ifndef COMLIB_H #define COMLIB_H
-#undef FILECODE -#define FILECODE 0xF001 - +#include <assert.h> #include <inttypes.h> #include <stdlib.h> #include "porting.h"
-#ifdef AMD_DEBUG - #define ASSERT(x) ((x) ? 0 : ErrorStop(((uint32)FILECODE)*0x10000 + ((__LINE__)%10) + (((__LINE__/10)%10)*0x10) + (((__LINE__/100)%10)*0x100) +(((__LINE__/1000)%10)*0x1000))) -#else - #define ASSERT(x) -#endif - #ifdef AMD_DEBUG_ERROR_STOP - /* Macro to aid debugging, causes program to halt and display the line number of the halt in decimal */ - #define STOP_HERE ErrorStop(((uint32)FILECODE)*0x10000 + ((__LINE__)%10) + (((__LINE__/10)%10)*0x10) + (((__LINE__/100)%10)*0x100) +(((__LINE__/1000)%10)*0x1000)) + /* Macro to aid debugging, causes program to halt and display the line number of the halt */ + #define STOP_HERE ASSERT(0) #else - /* Macro to aid debugging, causes program to halt and display the line number of the halt in decimal */ - /* #define STOP_HERE STOP_HERE_OnlyForDebugUse */ #define STOP_HERE #endif
diff --git a/src/northbridge/amd/amdht/h3ncmn.c b/src/northbridge/amd/amdht/h3ncmn.c index 8370b58..830e988 100644 --- a/src/northbridge/amd/amdht/h3ncmn.c +++ b/src/northbridge/amd/amdht/h3ncmn.c @@ -21,8 +21,6 @@ *---------------------------------------------------------------------------- */
-#undef FILECODE -#define FILECODE 0xF002 #include "h3ncmn.h" #include "h3finit.h" #include "h3ffeat.h" diff --git a/src/northbridge/amd/amdht/ht_wrapper.c b/src/northbridge/amd/amdht/ht_wrapper.c index bad8993..89ff46e 100644 --- a/src/northbridge/amd/amdht/ht_wrapper.c +++ b/src/northbridge/amd/amdht/ht_wrapper.c @@ -36,7 +36,6 @@ #endif
/* Debugging Options */ -#define AMD_DEBUG 1 //#define AMD_DEBUG_ERROR_STOP 1
/*---------------------------------------------------------------------------- @@ -45,8 +44,6 @@ *---------------------------------------------------------------------------- */
-#undef FILECODE -#define FILECODE 0xFF01 #include "comlib.h" #include "h3gtopo.h" #include "h3finit.h" diff --git a/src/northbridge/amd/amdht/porting.h b/src/northbridge/amd/amdht/porting.h index a0f8878..9058d4d 100644 --- a/src/northbridge/amd/amdht/porting.h +++ b/src/northbridge/amd/amdht/porting.h @@ -71,7 +71,6 @@ void CALLCONV AmdPCIRead(SBDFO loc, uint32 *Value); void CALLCONV AmdPCIWrite(SBDFO loc, uint32 *Value); void CALLCONV AmdCPUIDRead(uint32 Address, uint32 Regs[4]); -void CALLCONV ErrorStop(uint32 Value);
#define BYTESIZE 1 #define WORDSIZE 2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34375 )
Change subject: nb/amd/amdht: Use standard coreboot assertions ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34375/1/src/northbridge/amd/amdht/c... File src/northbridge/amd/amdht/comlib.c:
https://review.coreboot.org/c/coreboot/+/34375/1/src/northbridge/amd/amdht/c... PS1, Line 251:
I don't think I want to ask about this.
But since you did: it's a calling convention and symbol mangling hint: pascal has case insensitive symbols and might differ in some places when it comes to argument passing.
-
Angel Pons (Code Review) -
Jacob Garber (Code Review)
-
Patrick Georgi (Code Review)