Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
drivers/intel/fsp2_0: Add weak reservation for FSP-M memory
The function fsp_memory_init() is capable of executing in place or as non-XIP. For devices starting with DRAM enabled, this option increases performance but requires the memory to be reserved later.
Add a weak function so the soc may save the base and size of the memory consumed by FSP-M.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: I04b2fff4074b720d1910ff21e1a1f841cfea8efb --- M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38700/1
diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index 60adb98..a0366f6 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -50,6 +50,12 @@ void fsp_temp_ram_exit(void);
/* + * When fsp_memory_init() runs non-XIP, and if it affects any DRAM, allow + * the soc to note the memory to be reserved. + */ +void fsp_reserve_fspm_mem(uintptr_t base, size_t size); + +/* * Load FSP-S from stage cache or CBFS. This allows SoCs to load FSPS-S * separately from calling silicon init. It might be required in cases where * stage cache is no longer available by the point SoC calls into silicon init. diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 455dfa5..3e9da8e 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -42,6 +42,8 @@ CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "for TPM MRC hash functionality, vboot must start in bootblock");
+__weak void fsp_reserve_fspm_mem(uintptr_t base, size_t size) {} + static void save_memory_training_data(bool s3wake, uint32_t fsp_version) { size_t mrc_data_size; @@ -356,6 +358,8 @@ if (rdev_readat(rdev, (void *)fspm_begin, 0, fspm_end - fspm_begin) < 0) return CB_ERR;
+ fsp_reserve_fspm_mem(fspm_begin, fspm_end - fspm_begin); + return CB_SUCCESS; }
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 3: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 4: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@11 PS4, Line 11: increases performance but requires the memory to be reserved later. Why later? what happens further with the memory?
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@14 PS4, Line 14: memory consumed by FSP-M. What will it do with the information? What makes it platform specific?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 53: and if it affects any DRAM What does "affects any DRAM" mean?
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 54: the soc to note the memory to be reserved Is this for the resume case?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 4: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@14 PS4, Line 14: memory consumed by FSP-M.
What will it do with the information? What makes it platform specific?
Looks here for a better explanation: https://review.coreboot.org/c/coreboot/+/38702/4
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 56: fsp_reserve_fspm_mem Maybe we should rename this to fsp_fspm_loaded_callback(). We are using it as a notification to the chip code. I think the current name it too specific.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add weak reservation for FSP-M memory ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@11 PS4, Line 11: increases performance but requires the memory to be reserved later.
Why later? what happens further with the memory?
I believe the intended design of non-XIP memory init may use a feature where executable code can be run from CAR. Assuming that's correct, once CAR is torn down the code is effectively discard. However, Picasso doesn't have CAR, and when we run non-XIP it's in actual DRAM. We need to ensure this is reserved so the OS won't use it, otherwise on the S3 resume path OS memory gets clobbered.
(Thanks, Raul) The process of reserving later is in https://review.coreboot.org/c/coreboot/+/38702/4
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 53: and if it affects any DRAM
What does "affects any DRAM" mean?
If and DRAM is affected by the nature of fsp_memory_init() running non-XIP. In Picasso's case, non-XIP means it is loaded into DRAM. On an S3 resume, this would corrupt memory otherwise owned by the OS.
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 54: the soc to note the memory to be reserved
Is this for the resume case?
That's what we're guarding against. If not for S3 resume, we could simply give the memory to the OS.
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 56: fsp_reserve_fspm_mem
Maybe we should rename this to fsp_fspm_loaded_callback(). […]
I'm fine with changing the name, of course. I'm not 100% sold on "fsp_fspm_loaded_callback", although following some other examples "fsp_fspm_loaded" seems fine. Would that work for you?
Felix Held has uploaded a new patch set (#5) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load
fsp_memory_init() is capable of executing FSP-M either in place or copy it before running it in the non-XIP case.
Add a weak function to the non-XIP path that the SoC code can implement to get the base address and size of the memory region FSP-M got copied to. This function will be called right after FSP-M got copied to the memory region. This is needed on non-XIP platforms where the FSP-M image isn't written to CAR, but to DRAM, since the SoC code needs to mark the region as reserved, so that the S3 resume path doesn't clobber any memory regions used by the OS.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: I04b2fff4074b720d1910ff21e1a1f841cfea8efb --- M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38700/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@11 PS4, Line 11: increases performance but requires the memory to be reserved later.
I believe the intended design of non-XIP memory init may use a feature where executable code can be […]
Done
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@14 PS4, Line 14: memory consumed by FSP-M.
Looks here for a better explanation: https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 56: fsp_reserve_fspm_mem
I'm fine with changing the name, of course. […]
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 56: fsp_reserve_fspm_mem
Done
I'm fine if if you want to drop the `_callback`.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 56: fsp_reserve_fspm_mem
I'm fine if if you want to drop the `_callback`.
I'd like to keep the _callback to make clear that this is intended as a callback
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 53: and if it affects any DRAM
If and DRAM is affected by the nature of fsp_memory_init() running non-XIP. […]
added that information to the commit message; any objections to marking this as resolved?
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 54: the soc to note the memory to be reserved
That's what we're guarding against. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@14 PS4, Line 14: memory consumed by FSP-M.
Done
Well, as the answer to "what makes it platform specific?" is probably "nothing". I don't think it's done. But I don't object if you want to go on with it. I would prefer to leave a TODO, though, to ipmlement a generic solution.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/api.h:
https://review.coreboot.org/c/coreboot/+/38700/4/src/drivers/intel/fsp2_0/in... PS4, Line 53: and if it affects any DRAM
added that information to the commit message; any objections to marking this as resolved?
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38700/4//COMMIT_MSG@14 PS4, Line 14: memory consumed by FSP-M.
Well, as the answer to "what makes it platform specific?" is probably […]
I don't see how this isn't generic. non-XIP & non-CAR platform this information provided via the callback, but it's not specific to those
Felix Held has uploaded a new patch set (#6) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load
fsp_memory_init() is capable of executing FSP-M either in place or copy it before running it in the non-XIP case.
Add a weak function to the non-XIP path that the SoC code can implement to get the base address and size of the memory region FSP-M got copied to. This function will be called right after FSP-M got copied to the memory region. This is needed on non-XIP platforms where the FSP-M image isn't written to CAR, but to DRAM, since the SoC code needs to mark the region as reserved, so that the S3 resume path doesn't clobber any memory regions used by the OS. This common code change is however not specific to platforms that are non-XIP & non-CAR.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Change-Id: I04b2fff4074b720d1910ff21e1a1f841cfea8efb --- M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38700/6
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 6:
After talking to Nico on IRC it finally became clear what his point is here: The memory reservation should be done in the FSP driver and not the Picasso SoC code, since it's not specific to Picasso, but to the (non-XIP & non-CAR) FSP-M case. Not sure if it would be better to rework this now or fix it later, but before the next platform will land
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 6:
Patch Set 6:
After talking to Nico on IRC it finally became clear what his point is here: The memory reservation should be done in the FSP driver and not the Picasso SoC code, since it's not specific to Picasso, but to the (non-XIP & non-CAR) FSP-M case. Not sure if it would be better to rework this now or fix it later, but before the next platform will land
So the proposal is to just make FSP-M call bootmem_add_range() instead of adding the callback? I'm fine with that.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 6:
Patch Set 6:
After talking to Nico on IRC it finally became clear what his point is here: The memory reservation should be done in the FSP driver and not the Picasso SoC code, since it's not specific to Picasso, but to the (non-XIP & non-CAR) FSP-M case. Not sure if it would be better to rework this now or fix it later, but before the next platform will land
How close to a working ACPI S3 reausme is soc/amd/picasso code? You only need this feature functional and merged for that. I tried to ask the same thing in CB:38702, CB:38703 and CB:38792.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 6:
After talking to Nico on IRC it finally became clear what his point is here: The memory reservation should be done in the FSP driver and not the Picasso SoC code, since it's not specific to Picasso, but to the (non-XIP & non-CAR) FSP-M case. Not sure if it would be better to rework this now or fix it later, but before the next platform will land
So the proposal is to just make FSP-M call bootmem_add_range() instead of adding the callback? I'm fine with that.
It's a little more. `bootmem` is a ramstage feature. We'd have to cache the address somewhere (some structure on the stack, or global, doesn't matter) then add it to CBMEM once it's up (to pass the info to ramstage) and in ramstage call finally bootmem_add_range().
We could also implement checks that nothing else collides with the FSP-M location (don't know how it's location is decided), e.g. CBMEM.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 6: Code-Review-2
So the proposal is to just make FSP-M call bootmem_add_range() instead of adding the callback?
yep. this does look like a much better approach to me tbh. putting a -2 on this so it won't get merged before I've looked into and hopefully improved this.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 7:
This is not a requirement for getting Picasso booting. This is mostly an optimization for S3 and I think we still need to iron out all the details about S3. Can you please move this patch out of the way since it is not really a requirement for the series after this?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 7:
I'll let Felix manage this stack of patches, so I'll not abandon it right now.
However, now it seems like a cleaner approach is to allow the FSP to report its region (still reflective of its header) as reserved.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 7:
Patch Set 6:
After talking to Nico on IRC it finally became clear what his point is here: The memory reservation should be done in the FSP driver and not the Picasso SoC code, since it's not specific to Picasso, but to the (non-XIP & non-CAR) FSP-M case. Not sure if it would be better to rework this now or fix it later, but before the next platform will land
So the proposal is to just make FSP-M call bootmem_add_range() instead of adding the callback? I'm fine with that.
It's a little more. `bootmem` is a ramstage feature. We'd have to cache the address somewhere (some structure on the stack, or global, doesn't matter) then add it to CBMEM once it's up (to pass the info to ramstage) and in ramstage call finally bootmem_add_range().
We could also implement checks that nothing else collides with the FSP-M location (don't know how it's location is decided), e.g. CBMEM.
Correct. The broader issue is trying to mark memory reserved to the OS (and keeping its locations sorted out) to carry into ramstage for S3 resume purposes. I think I'll propose an alternative to work around the funkiness of picasso boot flow.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 7:
Patch Set 7:
I'll let Felix manage this stack of patches, so I'll not abandon it right now.
However, now it seems like a cleaner approach is to allow the FSP to report its region (still reflective of its header) as reserved.
I completely disagree on this. FSP should not mark itself as reserved. It doesn't know how it's used. That's up the user of FSP to determine.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Patch Set 7:
I completely disagree on this. FSP should not mark itself as reserved. It doesn't know how it's used. That's up the user of FSP to determine.
Yeah, the reservation should happen on the coreboot side
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38700 )
Change subject: drivers/intel/fsp2_0: Add callback after non-XIP FSP-M load ......................................................................
Abandoned
No longer necessary CB:42264
-
Aaron Durbin (Code Review) -
Andrey Petrov (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Nico Huber (Code Review)
-
Raul Rangel (Code Review)