Change in coreboot[master]: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD
Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35364 )
Change subject: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD ......................................................................
drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD
Check for a NULL pointer before displaying UPD information in case the new area wasn't allocated.
Change-Id: I28febfb04c2e5e68c43a0db499686bbbcd80afef Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/upd_display.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/35364/1
diff --git a/src/drivers/intel/fsp2_0/upd_display.c b/src/drivers/intel/fsp2_0/upd_display.c index 363ee36..8f6f937 100644 --- a/src/drivers/intel/fsp2_0/upd_display.c +++ b/src/drivers/intel/fsp2_0/upd_display.c @@ -28,6 +28,11 @@ static void fspm_display_arch_params(const FSPM_ARCH_UPD *old, const FSPM_ARCH_UPD *new) { + if (!new) { + printk(BIOS_SPEW, "Skipping architectural UPD display for MemoryInit - no new value region\n"); + return; + } + /* Display the architectural parameters for MemoryInit */ printk(BIOS_SPEW, "Architectural UPD values for MemoryInit at: 0x%p\n", new); @@ -53,6 +58,11 @@ const FSPM_UPD *fspm_old_upd, const FSPM_UPD *fspm_new_upd) { + if (!fspm_new_upd) { + printk(BIOS_SPEW, "Skipping MemoryInit UPD values - no new value region\n"); + return; + } + printk(BIOS_SPEW, "UPD values for MemoryInit:\n"); hexdump(fspm_new_upd, sizeof(*fspm_new_upd)); } @@ -70,6 +80,11 @@ const FSPS_UPD *fsps_old_upd, const FSPS_UPD *fsps_new_upd) { + if (!fsps_new_upd) { + printk(BIOS_SPEW, "Skipping SiliconInit UPD values - no new value region\n"); + return; + } + printk(BIOS_SPEW, "UPD values for SiliconInit:\n"); hexdump(fsps_new_upd, sizeof(*fsps_new_upd)); }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35364 )
Change subject: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG@9 PS1, Line 9: Check for a NULL pointer before displaying UPD information in case : the new area wasn't allocated. I am curious to understand the motivation behind this. Did you run into an issue with this? Looking at the call sequences, it doesn't look like you should ever get a NULL pointer in the places you are checking.
FSP_M --> Uses a storage on stack and creates a pointer to it FSP_S --> Uses xmalloc() which halts if allocation fails.
https://review.coreboot.org/c/coreboot/+/35364/1/src/drivers/intel/fsp2_0/up... File src/drivers/intel/fsp2_0/upd_display.c:
https://review.coreboot.org/c/coreboot/+/35364/1/src/drivers/intel/fsp2_0/up... PS1, Line 31: new Can this really be NULL? Looking at the call chain, it doesn't look like that is possible?
https://review.coreboot.org/c/coreboot/+/35364/1/src/drivers/intel/fsp2_0/up... PS1, Line 61: !fspm_new_upd Can this really be NULL? Looking at the call chain, it doesn't look like that is possible?
https://review.coreboot.org/c/coreboot/+/35364/1/src/drivers/intel/fsp2_0/up... PS1, Line 74: new If you plan to check new down in soc_display_fspm_upd_params(), then it will have to be checked here as well before calling soc_display_fspm_upd_params()
https://review.coreboot.org/c/coreboot/+/35364/1/src/drivers/intel/fsp2_0/up... PS1, Line 83: fsps_new_upd Same question as above about the possibility of this being NULL.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35364 )
Change subject: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG@9 PS1, Line 9: Check for a NULL pointer before displaying UPD information in case : the new area wasn't allocated.
I am curious to understand the motivation behind this. […]
I see now that you are changing the behavior for FSP_S such that new could be NULL. But, I don't think it is a valid case. Please see my comments on following CL.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35364 )
Change subject: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35364/1//COMMIT_MSG@9 PS1, Line 9: Check for a NULL pointer before displaying UPD information in case : the new area wasn't allocated.
I see now that you are changing the behavior for FSP_S such that new could be NULL. […]
Yes, that was it. Aaron suggested I check for NULL in the other patch, so I started looking at what might happen in that condition.
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35364 )
Change subject: drivers/intel/fsp2_0: Add NULL pointer debug messages to UPD ......................................................................
Abandoned
Not a reasonable scenario, so will prevent it from occurring instead.
-
Furquan Shaikh (Code Review) -
Marshall Dawson (Code Review)