Hello Philipp Deppenwiese, Bill XIE, Werner Zeh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39993
to review the following change.
Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................
security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
CB:35077 pulled TPM measurement code into the bootblock, with the catch
that we'll only cache PCR extensions and not actually write them to the
TPM until it gets initialized in a later stage. The goal of this was to
keep the heavy TPM driver code out of the size-constrained bootblock.
Unfortunately, a small mistake in the tspi_tpm_is_setup() function
prevents the compiler from eliminating references to the TPM driver
code in the bootblock on platforms with CONFIG_VBOOT and
CONFIG_SEPARATE_VERSTAGE. In those cases vboot_logic_executed() is known
at compile-time to be 0, but that still makes the final expression
`return 0 || tpm_is_setup;`. We know that tpm_is_setup can never be set
to 1 in the bootblock, but the compiler doesn't.
This patch rewrites the logic slightly to achieve the same effect in a
way that the compiler can follow (because we only really need to check
tpm_is_setup in the stage that actually runs the vboot code).
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
---
M src/security/tpm/tspi/tspi.c
1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39993/1
diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 4f0cc97..4b32657 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -104,8 +104,11 @@
static int tpm_is_setup;
static inline int tspi_tpm_is_setup(void)
{
- if (CONFIG(VBOOT))
- return vboot_logic_executed() || tpm_is_setup;
+ if (CONFIG(VBOOT)) {
+ if (verification_should_run())
+ return tpm_is_setup;
+ return vboot_logic_executed();
+ }
if (ENV_RAMSTAGE)
return tpm_is_setup;
--
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: newchange
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39980 )
Change subject: soc/intel/skylake: vr_config: enable PSI3 and PSI4 by default
......................................................................
Patch Set 3:
I tried to find out more about the PSI3/4 states. Some facts:
- "optional for server" in IMVP7 (doc#397113) simply means there are VRs for the server segment which don't support PS3 or PS4
- PS4 is only available on SoCs supporting C10
- Intel doc#334661-006 says PS4 is "shut the VR nearly completely off"
- Intel doc#334661-006 says that C-States should be limited to C9 when the SoC supports C10 but the platform (well, I guess they mean the whole machine but basicly the VR itself) does not support PS4
- There are VRs where the DS says "compliant with IMVP8 PS4", e.g. MP2949A
- PS3 means only one phase is on in dynamic mode
- PS4 means all PWM outputs are tristated / all phases are disabled. The VR is still enabled/alive to be able to respond with nearly no delay
- The PS4 command MUST be acknowledged by the VR according to some Intel doc I forgot to note the id for :/
Long story short: if we have the datasheet for the VR of the machine, we can simply check if it supports PSI3, PSI4, both, or none. If we don't have a datasheet, we have a problem.
I could not find out, yet, what would happen when the SoC tries to request PSI3/4 but the VR does not acknowlege it. In case some error flag gets set, we could simply test it by letting the machine go to C10 and check that flag.
--
To view, visit https://review.coreboot.org/c/coreboot/+/39980
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5b5fd9fb3b9b89e80c47f15d706e2dd62dcc0748
Gerrit-Change-Number: 39980
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Matthew Garrett <mjg59(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:24:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39980 )
Change subject: soc/intel/skylake: vr_config: enable PSI3 and PSI4 by default
......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39980/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/39980/3//COMMIT_MSG@18
PS3, Line 18: Boards that have a domain_vr_config and set their specific settings are
> I've found that a lot of information is available in the vendor's advanced settings. […]
Those settings are not in the bios/uefi on any of the firmwares I have checked. That's simply too low-level I assume
--
To view, visit https://review.coreboot.org/c/coreboot/+/39980
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5b5fd9fb3b9b89e80c47f15d706e2dd62dcc0748
Gerrit-Change-Number: 39980
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Matthew Garrett <mjg59(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:14:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com>
Comment-In-Reply-To: Michael Niewöhner
Gerrit-MessageType: comment
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39980 )
Change subject: soc/intel/skylake: vr_config: enable PSI3 and PSI4 by default
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/39980
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5b5fd9fb3b9b89e80c47f15d706e2dd62dcc0748
Gerrit-Change-Number: 39980
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Matthew Garrett <mjg59(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 20:37:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39980 )
Change subject: soc/intel/skylake: vr_config: enable PSI3 and PSI4 by default
......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39980/3//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/39980/3//COMMIT_MSG@18
PS3, Line 18: Boards that have a domain_vr_config and set their specific settings are
> I'd like to have a way to dump the vendor information from the vendor image and/or read it by intelt […]
I've found that a lot of information is available in the vendor's advanced settings.
For me, they were not visible by default (laptops seem to be particularly guilty in this regard) and I de-suppressed and enabled an option named "i-Page" (Intel page, perhaps) in the IFR (by hex) of the vendor's Setup utility UEFI module.
--
To view, visit https://review.coreboot.org/c/coreboot/+/39980
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5b5fd9fb3b9b89e80c47f15d706e2dd62dcc0748
Gerrit-Change-Number: 39980
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Matthew Garrett <mjg59(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 20:34:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michael Niewöhner
Gerrit-MessageType: comment
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39980 )
Change subject: soc/intel/skylake: vr_config: enable PSI3 and PSI4 by default
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39980/3/src/soc/intel/skylake/vr_c…
File src/soc/intel/skylake/vr_config.c:
https://review.coreboot.org/c/coreboot/+/39980/3/src/soc/intel/skylake/vr_c…
PS3, Line 24: /* Default values for domain configuration. PSI3 and PSI4 are disabled. */
Update comment?
--
To view, visit https://review.coreboot.org/c/coreboot/+/39980
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5b5fd9fb3b9b89e80c47f15d706e2dd62dcc0748
Gerrit-Change-Number: 39980
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Matthew Garrett <mjg59(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:48:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Hello Marshall Dawson,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40017
to review the following change.
Change subject: soc/amd/common/psp: make BootDone command conditional
......................................................................
soc/amd/common/psp: make BootDone command conditional
The common PSP code automatically sends the command, however for fam17h
FSP also sends it in response to a Notify. The 2nd instance of the
command generates a timeout error.
Change-Id: I819ab0b80d2a3cf6cd24f3e7170cae779dcc7cef
Signed-off-by: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Reviewed-on: https://chromium-review.googlesource.com/2020369
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
---
M src/soc/amd/common/block/psp/psp.c
1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/40017/1
diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c
index b48ff30..3aa3b86 100644
--- a/src/soc/amd/common/block/psp/psp.c
+++ b/src/soc/amd/common/block/psp/psp.c
@@ -180,10 +180,12 @@
return cmd_status;
}
+#if CONFIG(SOC_AMD_STONEYRIDGE)
/*
* Notify the PSP that the system is completing the boot process. Upon
* receiving this command, the PSP will only honor commands where the buffer
- * is in SMM space.
+ * is in SMM space. In FSP-based families, this is done by the FSP code and
+ * calling it a second time results in a hang.
*/
static void psp_notify_boot_done(void *unused)
{
@@ -201,6 +203,7 @@
/* buffer's status shouldn't change but report it if it does */
print_cmd_status(cmd_status, &buffer);
}
+#endif
/* Notify PSP the system is going to a sleep state. */
void psp_notify_sx_info(u8 sleep_type)
@@ -276,5 +279,8 @@
return cmd_status;
}
+#if CONFIG(SOC_AMD_STONEYRIDGE)
+/* only run psp_notify_boot_done on non-FSP families */
BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_BOOT, BS_ON_ENTRY,
psp_notify_boot_done, NULL);
+#endif
--
To view, visit https://review.coreboot.org/c/coreboot/+/40017
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I819ab0b80d2a3cf6cd24f3e7170cae779dcc7cef
Gerrit-Change-Number: 40017
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-MessageType: newchange
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39774 )
Change subject: soc/intel/tigerlake: Remove Jasper Lake SoC references
......................................................................
Patch Set 14:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1981
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1980
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1979
Please note: This test is under development and might not be accurate at all!
--
To view, visit https://review.coreboot.org/c/coreboot/+/39774
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I18853aba8b1e6ff7d37c03e8dae2521719c7c727
Gerrit-Change-Number: 39774
Gerrit-PatchSet: 14
Gerrit-Owner: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com>
Gerrit-CC: Justin TerAvest <teravest(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-CC: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:23:31 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment