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?