Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42208 )
Change subject: soc/amd/picasso/bootblock: Clear BSS ......................................................................
soc/amd/picasso/bootblock: Clear BSS
The PSP does not currently reload bootblock on S3 resume. This means the memory retains the previous values from the run. We need to clear bss to avoid sharing state from the last run.
BUG=b:147042464 TEST=Verified staic variables are 0 on S3 resume.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I6071600b14e9edb7b09510b595e9380ae8957d9f --- M src/soc/amd/picasso/bootblock/pre_c.S 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/42208/1
diff --git a/src/soc/amd/picasso/bootblock/pre_c.S b/src/soc/amd/picasso/bootblock/pre_c.S index 5c186f1..da9ecb9 100644 --- a/src/soc/amd/picasso/bootblock/pre_c.S +++ b/src/soc/amd/picasso/bootblock/pre_c.S @@ -18,6 +18,15 @@ and $0xfffffff0, %esp sub $8, %esp
+ /* clear .bss section since it is not cleared on S3 resume. */ + cld + xor %eax, %eax + movl $(_ebss), %ecx + movl $(_bss), %edi + sub %edi, %ecx + shrl $2, %ecx + rep stosl + movd %mm2, %eax pushl %eax /* tsc[63:32] */ movd %mm1, %eax
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42208 )
Change subject: soc/amd/picasso/bootblock: Clear BSS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42208/1/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/pre_c.S:
https://review.coreboot.org/c/coreboot/+/42208/1/src/soc/amd/picasso/bootblo... PS1, Line 21: /* clear .bss section since it is not cleared on S3 resume. * I'd probably skip saying "since it's not cleared on S3 resume. The next PSP bl drop will reload, and therefore rezero. However, I think it's a good measure to zero anyway, as we're currently relying on the compression util pre-zeroing the image for us.
Also, for clarity, I'd move this new block of code above line 15 so that we keep all the stack pointer manipulation as close as possible to the call bootblock_c_entry.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42208 )
Change subject: soc/amd/picasso/bootblock: Clear BSS ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG@9 PS1, Line 9: The PSP does not currently reload bootblock on S3 resume. This means the Sorry for being ignorant about the bootflow, but why would it be useful to reload the bootblock? (bootblock is `bootblock` in CBFS, right?)
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG@9 PS1, Line 9: This means the : memory retains the previous values from the run. What visible problem does that cause? Resume problems?
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG@12 PS1, Line 12: Please mention, that this does not noticeably affect the resume time.
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG@14 PS1, Line 14: staic static
https://review.coreboot.org/c/coreboot/+/42208/1/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/pre_c.S:
https://review.coreboot.org/c/coreboot/+/42208/1/src/soc/amd/picasso/bootblo... PS1, Line 21: clear Clear
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42208 )
Change subject: soc/amd/picasso/bootblock: Clear BSS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42208/1//COMMIT_MSG@9 PS1, Line 9: The PSP does not currently reload bootblock on S3 resume. This means the
Sorry for being ignorant about the bootflow, but why would it be useful to reload the bootblock? (bo […]
It was not clear from the couple commit messages, but mentioned somewhere in the comments; future PSP will reload the bootblock.
Picasso has a very different bootflow to other x86, bootblock is not execute-in-place, and may reside "anywhere" in the SPI flash. Instead, executable is linked and loaded at a static memory location, low enough to be guaranteed to reside in DRAM and not overlap MMIO windows. As I understand it, region could only be marked as reserved from the OS, not write-protected.
It is forbidden for S3 resume path to execute code that may have been altered from the OS. So choices are reloading or creating a relocatable bootblock rmodule that would get relocated in SMRAM. Former is easier to develop, latter might have had a slightly faster S3 resume path.
Raul Rangel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42208 )
Change subject: soc/amd/picasso/bootblock: Clear BSS ......................................................................
Abandoned
Squashing