Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso: Write EIP to secure S3 ......................................................................
Uploaded patch set 5.
(8 comments)
https://review.coreboot.org/c/coreboot/+/42088/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42088/1//COMMIT_MSG@7 PS1, Line 7: picass
picass*o*
Done
https://review.coreboot.org/c/coreboot/+/42088/1//COMMIT_MSG@9 PS1, Line 9: boot block
bootblock
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 96: .hi = (uint64_t)(uintptr_t)bootblock_protected_mode_entry >> 32,
If you had a x86_64 bootblock, uintptr_t is already uint64_t so the cast seems unnecessary.
But we currently don't. Without this cast the compiler will yell about the >> 32 operator.
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 progra […]
Done
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 103: C6
Let's call it "CC6" instead"
Done
https://review.coreboot.org/c/coreboot/+/42088/3/src/soc/amd/picasso/bootblo... PS3, Line 110: write_eip();
The symbol bootblock_protected_mode_entry will not be known outside bootblock. […]
Ack
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. […]
Done