Change in coreboot[master]: drivers/intel/fsp2_0 - Change boot state notification Change boot sta...
Rocky Phagura has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0 - Change boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code ......................................................................
drivers/intel/fsp2_0 - Change boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code
This patch changes the flow to: 1. Notify FSP of boot state 2. Load payload into memory 3. Jump to payload code
This prevents code corruption of payload. At this stage of boot, there is no concept of memory management, therefore FSP can use any memory it needs. After making these changes, Tianocore payload is able to load properly where previously its memory was getting corrupted.
TEST=build for Tiogapass platform under OCP mainboard and select Tianocore payload. Boot the system and ensure target OS loads.
Change-Id: I122edc12abf992cb3e5ec53747a7cef9c94aee8e Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/drivers/intel/fsp2_0/notify.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/43979/1
diff --git a/src/drivers/intel/fsp2_0/notify.c b/src/drivers/intel/fsp2_0/notify.c index 76cdf12..ba8ef9e 100644 --- a/src/drivers/intel/fsp2_0/notify.c +++ b/src/drivers/intel/fsp2_0/notify.c @@ -68,7 +68,7 @@
BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, fsp_notify_dummy, (void *) AFTER_PCI_ENUM); -BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_EXIT, fsp_notify_dummy, +BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_ENTRY, fsp_notify_dummy, (void *) READY_TO_BOOT); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, fsp_notify_dummy, (void *) READY_TO_BOOT);
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43979
to look at the new patch set (#2).
Change subject: drivers/intel/fsp2_0: ZZChange boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code ......................................................................
drivers/intel/fsp2_0: ZZChange boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code
This patch changes the flow to: 1. Notify FSP of boot state 2. Load payload into memory 3. Jump to payload code
This prevents code corruption of payload. At this stage of boot, there is no concept of memory management, therefore FSP can use any memory it needs. After making these changes, Tianocore payload is able to load properly where previously its memory was getting corrupted.
TEST=build for Tiogapass platform under OCP mainboard and select Tianocore payload. Boot the system and ensure target OS loads.
Change-Id: I122edc12abf992cb3e5ec53747a7cef9c94aee8e Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/drivers/intel/fsp2_0/notify.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/43979/2
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43979
to look at the new patch set (#3).
Change subject: drivers/intel/fsp2_0: Change boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code ......................................................................
drivers/intel/fsp2_0: Change boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code
This patch changes the flow to: 1. Notify FSP of boot state 2. Load payload into memory 3. Jump to payload code
This prevents code corruption of payload. At this stage of boot, there is no concept of memory management, therefore FSP can use any memory it needs. After making these changes, Tianocore payload is able to load properly where previously its memory was getting corrupted.
TEST=build for Tiogapass platform under OCP mainboard and select Tianocore payload. Boot the system and ensure target OS loads.
Change-Id: I122edc12abf992cb3e5ec53747a7cef9c94aee8e Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/drivers/intel/fsp2_0/notify.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/43979/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG@7 PS3, Line 7: drivers/intel/fsp2_0: Change boot state notification Add an empty line after the commit summary
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG@19 PS3, Line 19: This prevents code corruption of payload. At this stage of boot, there is no nit: wrap at 72 characters
Hello Subrata Banik, build bot (Jenkins), Jonathan Zhang, Angel Pons, Subrata Banik, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43979
to look at the new patch set (#4).
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
drivers/intel/fsp2_0: Change boot state notification
Change boot state notification of FSP to before payload is loaded into memory. Currently the flow is as follows: 1. Load payload into memory 2. Notify FSP of boot state init 3. Jump to payload code
This patch changes the flow to: 1. Notify FSP of boot state 2. Load payload into memory 3. Jump to payload code
This prevents code corruption of payload. At this stage of boot, there is no concept of memory management, therefore FSP can use any memory it needs. After making these changes, Tianocore payload is able to load properly where previously its memory was getting corrupted.
TEST=build for Tiogapass platform under OCP mainboard and select Tianocore payload. Boot the system and ensure target OS loads.
Change-Id: I122edc12abf992cb3e5ec53747a7cef9c94aee8e Signed-off-by: Rocky Phagura rphagura@fb.com --- M src/drivers/intel/fsp2_0/notify.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/43979/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG@21 PS4, Line 21: This prevents code corruption of payload. At this stage of boot, there : is no concept of memory management, therefore FSP can use any memory it : needs. What memory is FSP using which is causing corruption? Coreboot already reserves the memory where FSP is loaded. I don't think it should be accessing anything outside that reservation. If it is, then it seems like a FSP bug.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG@21 PS4, Line 21: This prevents code corruption of payload. At this stage of boot, there : is no concept of memory management, therefore FSP can use any memory it : needs.
What memory is FSP using which is causing corruption? Coreboot already reserves the memory where FSP […]
I think FSP can produce multiple reserved memory HOBs, but we only handle one.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43979/4//COMMIT_MSG@21 PS4, Line 21: This prevents code corruption of payload. At this stage of boot, there : is no concept of memory management, therefore FSP can use any memory it : needs.
I think FSP can produce multiple reserved memory HOBs, but we only handle one.
If we are missing the handling of multiple reserved memory HOBs published by FSP, then I think that needs to be fixed rather than changing the order here. This reserved memory HOB details will also have to be used to reserve the memory from OS as well.
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to: 1. notify FSP of boot state init 2. load payload into memory 3. run payload
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG@7 PS3, Line 7: drivers/intel/fsp2_0: Change boot state notification
Add an empty line after the commit summary
Done
https://review.coreboot.org/c/coreboot/+/43979/3//COMMIT_MSG@19 PS3, Line 19: This prevents code corruption of payload. At this stage of boot, there is no
nit: wrap at 72 characters
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
- notify FSP of boot state init
- load payload into memory
- run payload
I'm not sure why we do it like that, but if FSP unexpectedly uses some memory, we have a problem. Maybe FSP is doing something wrong, or we're not properly handling its reserved memory HOBs (there can be more than one).
Rocky Phagura has removed Eugene Myers from this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Removed reviewer Eugene Myers.
Rocky Phagura has removed Eugene Myers from this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Removed reviewer Eugene Myers.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
- notify FSP of boot state init
- load payload into memory
- run payload
we have 2 FSP callback 1. Post PCI enumeration 2. Ready to Boot I guess the purpose of NotifyPhaseApi() - Begin [Phase: 00000040] FSP Ready To Boot ... event to ensure we have a done with all operation and bootloader is ready to handoff to payload/os
Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
- notify FSP of boot state init
- load payload into memory
- run payload
we have 2 FSP callback
- Post PCI enumeration
- Ready to Boot
I guess the purpose of NotifyPhaseApi() - Begin [Phase: 00000040] FSP Ready To Boot ... event to ensure we have a done with all operation and bootloader is ready to handoff to payload/os
Subrata, Is it possible for FSP to overwrite memory where the payload is loaded? That's what I'm seeing in some cases. Hence, I wanted to change the order in which FSP is being notified just prior to the payload being loaded.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
- notify FSP of boot state init
- load payload into memory
- run payload
we have 2 FSP callback
- Post PCI enumeration
- Ready to Boot
I guess the purpose of NotifyPhaseApi() - Begin [Phase: 00000040] FSP Ready To Boot ... event to ensure we have a done with all operation and bootloader is ready to handoff to payload/os
Subrata, Is it possible for FSP to overwrite memory where the payload is loaded? That's what I'm seeing in some cases. Hence, I wanted to change the order in which FSP is being notified just prior to the payload being loaded.
I still suspect that FSP might be outputting multiple reserved memory HOBs, and we only account for the first one.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
(2 comments)
Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
- notify FSP of boot state init
- load payload into memory
- run payload
we have 2 FSP callback
- Post PCI enumeration
- Ready to Boot
I guess the purpose of NotifyPhaseApi() - Begin [Phase: 00000040] FSP Ready To Boot ... event to ensure we have a done with all operation and bootloader is ready to handoff to payload/os
Subrata, Is it possible for FSP to overwrite memory where the payload is loaded? That's what I'm seeing in some cases. Hence, I wanted to change the order in which FSP is being notified just prior to the payload being loaded.
I still suspect that FSP might be outputting multiple reserved memory HOBs, and we only account for the first one.
+1. Either that or FSP actually has a bug where it is writing to memory it does not reserve. Both are bad since it can lead to OS memory corruption as well in case of S3 resume. You can enable CONFIG_DISPLAY_HOBS to check the HOBs that are provided by FSP. From FSP spec, any memory reserved by FSP would be reported as FSP_RESERVED_MEMORY_RESOURCE_HOB.
Rocky Phagura has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43979 )
Change subject: drivers/intel/fsp2_0: Change boot state notification ......................................................................
Abandoned
The issue no longer exists.
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Rocky Phagura (Code Review)
-
Subrata Banik (Code Review)