Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso: Write EIP to secure S3 ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 92: write_eip nit: write_resume_eip?
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 100: acpi_s3_resume_allowed() && Since the whole reason is to mitigate potential attacks, I think it's probably a good idea to program the register on 100% of non-resume boots. It doesn't actually change the result of the logic below, but I'd remove this portion to make the code more obvious. (Then you can probably remove the variable, this line, etc.)
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 103: C6 Let's call it "CC6" instead"
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/include... PS3, Line 9: AFAIK, AMD has never redefined any "cool" registers. I don't know why this couldn't go into include/cpu/amd/msr.h instead.