Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: WIP: soc/amd/picass: Write EIP to enable S3 ......................................................................
WIP: soc/amd/picass: Write EIP to enable S3
This now allows S3 to jump to boot block. The TSC value is still wrong.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/soc/amd/picasso/bootblock/bootblock.c A src/soc/amd/picasso/include/soc/msr.h M src/soc/amd/picasso/root_complex.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/1
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index 1ff6896..9c268af 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -11,7 +11,12 @@ #include <cpu/amd/mtrr.h> #include <soc/southbridge.h> #include <soc/i2c.h> +#include <soc/msr.h> #include <amdblocks/amd_pci_mmconf.h> +#include <stdbool.h> +#include <acpi/acpi.h> + +asmlinkage void bootblock_pre_c_entry(void);
static void set_caching(void) { @@ -39,6 +44,22 @@ enable_cache(); }
+static void write_eip(void) +{ + bool s3_resume; + msr_t s3_resume_entry = { + .hi = (uint64_t)(uintptr_t)bootblock_pre_c_entry >> 32, + .lo = (uintptr_t)bootblock_pre_c_entry & 0xffffffff, + }; + + s3_resume = acpi_s3_resume_allowed() && acpi_is_wakeup_s3(); + + /* Trigger the microcode to stash the CPU state and resume vector + * into the C6 save area. */ + if (!s3_resume) + wrmsr(S3_RESUME_EIP_MSR, s3_resume_entry); +} + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { set_caching(); @@ -57,5 +78,7 @@ u32 val = cpuid_eax(1); printk(BIOS_DEBUG, "Family_Model: %08x\n", val);
+ write_eip(); + fch_early_init(); } diff --git a/src/soc/amd/picasso/include/soc/msr.h b/src/soc/amd/picasso/include/soc/msr.h new file mode 100644 index 0000000..e7cdf9d --- /dev/null +++ b/src/soc/amd/picasso/include/soc/msr.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __PICASSO_MSR_H__ +#define __PICASSO_MSR_H__ + +#define S3_RESUME_EIP_MSR 0xC00110E0 + +#endif /* __PICASSO_MSR_H__ */ diff --git a/src/soc/amd/picasso/root_complex.c b/src/soc/amd/picasso/root_complex.c index f621eea..8002048 100644 --- a/src/soc/amd/picasso/root_complex.c +++ b/src/soc/amd/picasso/root_complex.c @@ -11,6 +11,8 @@ #include <fsp/util.h> #include <stdint.h>
+#define RESET_VECTOR_SIZE 0x10 + static void read_resources(struct device *dev) { uint32_t mem_usable = (uintptr_t)cbmem_top(); @@ -30,6 +32,13 @@ /* 1MB to top of low usable RAM */ ram_resource(dev, idx++, 1 * MiB / KiB, (mem_usable - 1 * MiB) / KiB);
+ /* HACK: Reserve bootblock */ + reserved_ram_resource( + dev, idx++, + (CONFIG_X86_RESET_VECTOR + RESET_VECTOR_SIZE - CONFIG_C_ENV_BOOTBLOCK_SIZE) + / KiB, + CONFIG_C_ENV_BOOTBLOCK_SIZE / KiB); + mmconf_resource(dev, MMIO_CONF_BASE);
if (!hob) {
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: WIP: soc/amd/picass: Write EIP to enable S3 ......................................................................
Patch Set 1:
(2 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*
https://review.coreboot.org/c/coreboot/+/42088/1//COMMIT_MSG@9 PS1, Line 9: boot block bootblock
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42088
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Write EIP to enable S3 ......................................................................
soc/amd/picasso: Write EIP to enable S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/soc/amd/picasso/bootblock/bootblock.c A src/soc/amd/picasso/include/soc/msr.h 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso: Write EIP to enable S3 ......................................................................
Uploaded patch set 2.
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 3.
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42088
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Write EIP to secure S3 ......................................................................
soc/amd/picasso: Write EIP to secure S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/soc/amd/picasso/bootblock/bootblock.c A src/soc/amd/picasso/include/soc/msr.h 2 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/3
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 ......................................................................
Patch Set 3:
This one is ready to review.
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.
Kyösti Mälkki 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:
(1 comment)
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 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.
Patrick Rudolph 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:
(1 comment)
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 110: write_eip(); This doesn't seem to be needed for later stages to work properly and thus should be part of ramstage code.
Kyösti Mälkki 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:
(1 comment)
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 110: write_eip();
This doesn't seem to be needed for later stages to work properly and thus should be part of ramstage […]
The symbol bootblock_protected_mode_entry will not be known outside bootblock. I think it's fine to do write that MSR unconditionally in bootblock.
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 4.
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42088
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Write EIP to secure S3 ......................................................................
soc/amd/picasso: Write EIP to secure S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
The PSP is responsible for loading bootblock into RAM. The bootblock image currently has a zeroed out BSS section. So when the image is loaded into RAM by the PSP we know that BSS is zero. On S3 resume, the PSP does not currently reload bootblock into RAM. This means that the previous BSS values will still be present. This violates the assumptions the program makes about global variables. So we need to explicitly clear it. The bss clearing code was copied from assembly_entry.S.
Until the PSP always reloads the bootblock, there is a risk of the OS modifying the bootblock. One we can switch to a new PSP version we can remove the BSS clearing code.
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/bootblock/pre_c.S A src/soc/amd/picasso/include/soc/msr.h 3 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/4
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42088
to look at the new patch set (#5).
Change subject: soc/amd/picasso: Write EIP to secure S3 ......................................................................
soc/amd/picasso: Write EIP to secure S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
The PSP is responsible for loading bootblock into RAM. The bootblock image currently has a zeroed out BSS section. So when the image is loaded into RAM by the PSP we know that BSS is zero. On S3 resume, the PSP does not currently reload bootblock into RAM. This means that the previous BSS values will still be present. This violates the assumptions the program makes about global variables. So we need to explicitly clear it. The bss clearing code was copied from assembly_entry.S.
Until the PSP always reloads the bootblock, there is a risk of the OS modifying the bootblock. One we can switch to a new PSP version we can remove the BSS clearing code.
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/bootblock/pre_c.S 3 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/5
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
Angel Pons 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 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 102: */ This comment ending should go on the next line
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 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG@22 PS5, Line 22: One Once
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG@23 PS5, Line 23: remove Maybe "consider removing"? We may just want to keep it so that we don't need to rely on the good graces of the compression utility.
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 15: #include <stdbool.h> I believe this can go away now.
Aaron Durbin 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 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG@19 PS5, Line 19: it. The bss clearing code was copied from assembly_entry.S. This is honestly why we should always clear bss unconditionally. However, without the reload data objects aren't re-initialized.
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching(); Do you want your bss clear to be faster? put the write to the s3 msr here.
Angel Pons 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching();
Do you want your bss clear to be faster? put the write to the s3 msr here.
After caching is magically (not really magic) faster?
Aaron Durbin 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching();
After caching is magically (not really magic) faster?
This is about S3 resume path. The S3 MSR write snapshots core state including MTRRs and CR0. If you are writing S3 MSR after caching is enabled the cpu resumes with caching enabled.
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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching();
This is about S3 resume path. The S3 MSR write snapshots core state including MTRRs and CR0. […]
+1 on what Aaron said re. the architectural state of the core. The BSP will resume w/whatever x86 state exists when the MSR is written, including caching enabled. If you're familiar with the ability to save away core state to DRAM during C6, then shove it back in when there's activity, I think that's a good analogy.
BTW Raul, I'd move the write to here too.
Angel Pons 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching();
+1 on what Aaron said re. the architectural state of the core. […]
Oh, I understand now. Yes, sounds worthwhile
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso/bootblock: Write EIP to secure S3 ......................................................................
Uploaded patch set 6.
(5 comments)
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG@22 PS5, Line 22: One
Once
Done
https://review.coreboot.org/c/coreboot/+/42088/5//COMMIT_MSG@23 PS5, Line 23: remove
Maybe "consider removing"? We may just want to keep it so that we don't need to rely on the good gr […]
Done
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 15: #include <stdbool.h>
I believe this can go away now.
Done
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 102: */
This comment ending should go on the next line
Done
https://review.coreboot.org/c/coreboot/+/42088/5/src/soc/amd/picasso/bootblo... PS5, Line 110: set_caching();
Oh, I understand now. […]
Done. I had it before so we could have as pristine state as possible.
Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42088
to look at the new patch set (#6).
Change subject: soc/amd/picasso/bootblock: Write EIP to secure S3 ......................................................................
soc/amd/picasso/bootblock: Write EIP to secure S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/bootblock/pre_c.S 3 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/42088/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso/bootblock: Write EIP to secure S3 ......................................................................
Patch Set 6: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso/bootblock: Write EIP to secure S3 ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42088 )
Change subject: soc/amd/picasso/bootblock: Write EIP to secure S3 ......................................................................
soc/amd/picasso/bootblock: Write EIP to secure S3
This change is required so we have a defined entry point on S3. Without this, the S3_RESUME_EIP_MSR register could in theory be written to later which would be a security risk.
BUG=b:147042464 TEST=Resume trembyle and see bootblock start.
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 smm starting (log level: 8)...
SMI# #6 SMI#: SLP = 0x0c01 Chrome EC: Set SMI mask to 0x0000000000000000 Chrome EC: Set SCI mask to 0x0000000000000000 Clearing pending EC events. Error code EC_RES_UNAVAILABLE(9) is expected. EC returned error result code 9 SMI#: Entering S3 (Suspend-To-RAM) PSP: Prepare to enter sleep state 3... OK SMU: Put system into S3/S4/S5 Timestamp - start of bootblock: 18446744070740509170
coreboot-4.12-512-g65779ebcf73f-dirty Thu Jun 4 22:38:17 UTC 2020 bootblock starting (log level: 8)... Family_Model: 00810f81 PMxC0 STATUS: 0x200800 SleepReset BIT11 I2C bus 3 version 0x3132322a DW I2C bus 3 at 0xfedc5000 (400 KHz) Timestamp - end of bootblock: 18446744070804450274 VBOOT: Loading verstage. FMAP: area COREBOOT found @ c75000 (3715072 bytes) CBFS: Locating 'fallback/verstage' CBFS: Found @ offset 61b80 size cee4 PROG_RUN: Setting MTRR to cache stage. base: 0x04000000, size: 0x00010000
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I4b0b0d0d576fc42b1628a4547a5c9a10bcbe9d37 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42088 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/include/cpu/amd/msr.h M src/soc/amd/picasso/bootblock/bootblock.c M src/soc/amd/picasso/bootblock/pre_c.S 3 files changed, 33 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Marshall Dawson: Looks good to me, approved
diff --git a/src/include/cpu/amd/msr.h b/src/include/cpu/amd/msr.h index 606ea23..e466e7b 100644 --- a/src/include/cpu/amd/msr.h +++ b/src/include/cpu/amd/msr.h @@ -72,6 +72,7 @@ #define EX_CFG_MSR 0xC001102C #define LS_CFG2_MSR 0xC001102D #define IBS_OP_DATA3_MSR 0xC0011037 +#define S3_RESUME_EIP_MSR 0xC00110E0
#define MSR_PATCH_LEVEL 0x0000008B #define CORE_PERF_BOOST_CTRL 0x15c diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index 606440b..a3935cc 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -12,6 +12,9 @@ #include <soc/southbridge.h> #include <soc/i2c.h> #include <amdblocks/amd_pci_mmconf.h> +#include <acpi/acpi.h> + +asmlinkage void bootblock_resume_entry(void);
/* PSP performs the memory training and setting up DRAM map prior to x86 cores being released. Honor TOP_MEM and set up caching from 0 til TOP_MEM. Likewise, @@ -84,9 +87,27 @@ enable_cache(); }
+static void write_resume_eip(void) +{ + msr_t s3_resume_entry = { + .hi = (uint64_t)(uintptr_t)bootblock_resume_entry >> 32, + .lo = (uintptr_t)bootblock_resume_entry & 0xffffffff, + }; + + /* + * Writing to the EIP register can only be done once, otherwise a fault is triggered. + * When this register is written, it will trigger the microcode to stash the CPU state + * (crX , mtrrs, registers, etc) into the CC6 save area. On resume, the state will be + * restored and execution will continue at the EIP. + */ + if (!acpi_is_wakeup_s3()) + wrmsr(S3_RESUME_EIP_MSR, s3_resume_entry); +} + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { set_caching(); + write_resume_eip(); enable_pci_mmconf();
bootblock_main_with_basetime(base_timestamp); diff --git a/src/soc/amd/picasso/bootblock/pre_c.S b/src/soc/amd/picasso/bootblock/pre_c.S index 5c186f1..83e5491 100644 --- a/src/soc/amd/picasso/bootblock/pre_c.S +++ b/src/soc/amd/picasso/bootblock/pre_c.S @@ -2,6 +2,17 @@
#include <cpu/x86/post_code.h>
+.global bootblock_resume_entry +bootblock_resume_entry: + post_code(0xb0) + + /* Get an early timestamp */ + rdtsc + movd %eax, %mm1 + movd %edx, %mm2 + + /* Fall through to bootblock_pre_c_entry */ + /* * on entry: * mm0: BIST (ignored)
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Raul Rangel (Code Review)