coreboot-gerrit February 2025

coreboot-gerrit@coreboot.org

1 participants
6 discussions

[S] Change in coreboot[main]: security: Allow vboot when INTEL_TXT enabled
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Christian Walter, Filip Lewiński, Michał Żygowski. Julius Werner has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82695?usp=email ) Change subject: security: Allow vboot when INTEL_TXT enabled ...................................................................... Patch Set 12: (4 comments) File src/lib/bootblock.c: https://review.coreboot.org/c/coreboot/+/82695/comment/2e8143fe_081bb993?us… : PS12, Line 65: */ nit: not sure this comment explains anything that isn't obvious here tbh, and since this function is a very central piece of code and none of the other calls are commented it feels a bit odd and out of place here File src/security/intel/txt/Kconfig: https://review.coreboot.org/c/coreboot/+/82695/comment/0f0fa208_8e4a83b0?us… : PS12, Line 19: select TPM_STARTUP_IGNORE_POSTINIT Sorry, I don't understand why we need these changes here now. Doesn't the change you make to `vboot_setup_tpm()` make `VBOOT_STARTS_IN_BOOTBLOCK` compatible with `TPM_MEASURED_BOOT_INIT_BOOTBLOCK`? So why are you still adding the `&& !VBOOT_STARTS_IN_BOOTBLOCK` here? Also, doesn't that change ensure that the TPM doesn't get initialized twice anymore (because vboot will not try to initialize it again)? So why do you still have the `TPM_STARTUP_IGNORE_POSTINIT` here? I think you can just leave this file as it was, and your changes to the other files alone should be enough to make this patch work. File src/security/vboot/tpm_common.c: https://review.coreboot.org/c/coreboot/+/82695/comment/37891da8_e06d0137?us… : PS12, Line 20: probe for TPM. This doesn't probe for TPM, `tpm_setup()` does. The whole point here is that we don't need to probe again because we know we already did. https://review.coreboot.org/c/coreboot/+/82695/comment/a9bb1f06_9229fba0?us… : PS12, Line 27: printk(BIOS_ERR, "TPM Error (%#x): Can't initialize.\n", rc); nit: this seems a bit redundant here since we know `tpm_setup()` was already called and would've already printed this (and logged the post code). I think you can literally just make this ``` if (CONFIG(TPM_MEASURED_BOOT_INIT_BOOTBLOCK)) return tlcl_lib_init(); ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/82695?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I19dc3d910c23fcfd8732465c488f47dd86a96781 Gerrit-Change-Number: 82695 Gerrit-PatchSet: 12 Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Comment-Date: Sat, 01 Feb 2025 01:31:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: soc/intel/adl: Display low battery message on screen
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86224?usp=email ) Change subject: soc/intel/adl: Display low battery message on screen ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: BTW why is all this code in `intel/alderlake`? Shouldn't it be in `intel/common` instead? It doesn't look SoC-generation-specific. -- To view, visit https://review.coreboot.org/c/coreboot/+/86224?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I3b24d2c89ade8cc62b7e47c487d52d47b7f3376d Gerrit-Change-Number: 86224 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Sat, 01 Feb 2025 00:41:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: util/cbfstool/eventlog: Add low battery event type
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Karthik Ramasubramanian, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86223?usp=email ) Change subject: util/cbfstool/eventlog: Add low battery event type ...................................................................... Patch Set 2: (1 comment) File util/cbfstool/eventlog.c: https://review.coreboot.org/c/coreboot/+/86223/comment/b5cb238b_85f90d33?us… : PS2, Line 496: {ELOG_FW_ISSUE_SHUTDOWN, "Poweroff: Low Battery"}, nit: As I understand elogtool would display this whole line as `Low Battery Boot | Poweroff: Low Battery`, which seems a bit redundant. I think you can just write `"Power Off"` or `"Shutdown"` here. -- To view, visit https://review.coreboot.org/c/coreboot/+/86223?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5cc5e5f540657c7dfd174a4928e697a272da813a Gerrit-Change-Number: 86223 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 01 Feb 2025 00:39:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: vc/google/chromeos: Add low battery indicator screen
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Karthik Ramasubramanian, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86225?usp=email ) Change subject: vc/google/chromeos: Add low battery indicator screen ...................................................................... Patch Set 2: (2 comments) File src/vendorcode/google/chromeos/Kconfig: https://review.coreboot.org/c/coreboot/+/86225/comment/ca3ab813_ab3d23bf?us… : PS2, Line 120: bool "Display Early Low Battery Indicator in firmware" nit: would be better to more explicitly say "in romstage" or "before memory initialization", rather than just "early". https://review.coreboot.org/c/coreboot/+/86225/comment/a5159338_cee01cc1?us… : PS2, Line 124: and defer the firmware update. How is this related to updates? -- To view, visit https://review.coreboot.org/c/coreboot/+/86225?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I711c53455639b449fe85903139bbc06cdab08d09 Gerrit-Change-Number: 86225 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 01 Feb 2025 00:37:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: drivers/intel/fsp2_0: Add platform callback for critical shutdown
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Andrey Petrov, Intel coreboot Reviewers, Karthik Ramasubramanian, Ronak Kanabar, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86226?usp=email ) Change subject: drivers/intel/fsp2_0: Add platform callback for critical shutdown ...................................................................... Patch Set 2: (1 comment) File src/drivers/intel/fsp2_0/silicon_init.c: https://review.coreboot.org/c/coreboot/+/86226/comment/7534644f_eac9ecff?us… : PS2, Line 263: platform_is_critical_shutdown_needed(); Having a function that sounds like a question hang the boot flow is not very intuitive. Also, it's not good that `google_chromeec_is_below_critical_threshold()` is called twice (once from this and once from `bmp_logo_filename()`) — if the EC should return different answers to the two host commands for some reason, you're gonna end up in a very weird state. I don't think we need to tie this into `HAVE_CUSTOM_BMP_LOGO`, since the only real purpose for that option was to allow SKU differentiation in the splash screen, and we probably won't need SKU differentiation in our low battery screens. So I would say just hardcode the filename for the low battery case in `bmp_load_logo()` and don't even call `bmp_logo_filename()` for that case. I would suggest you make the `platform_is_critical_shutdown_needed()` function a function that only returns true or false without hanging the system itself, but caches that result so that it can't suddenly change when you call it a second time. (Maybe also name it something that explicitly says "low battery" because it's otherwise not quite clear that it's related to that.) ``` #if CONFIG(...) bool platform_is_low_battery_shutdown_needed(void) { static bool result = false; static bool checked = false; if (!checked) { if (google_chromeec_is_below_critical_threshold()) result = true; checked = true; }; return result; } #endif ``` Then, in `bmp_load_logo()`, you do something like ``` const char *logo_name; if (platform_is_low_battery_shutdown_needed()) logo_name = "low_battery.bmp"; else logo_name = bmp_logo_filename(); *logo_size = cbfs_load(logo_name, ...); ``` And then here, you do the ``` if (platform_is_low_battery_shutdown_needed()) { elog_add_event_byte(ELOG_TYPE_LOW_BATTERY_INDICATOR, ELOG_FW_ISSUE_SHUTDOWN); delay(3); poweroff(); } ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/86226?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2d6677d70dea3d24f5a19d70608fd21229a271a0 Gerrit-Change-Number: 86226 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Sat, 01 Feb 2025 00:34:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: soc/intel/adl: Handle critical low battery early in romstage
by Julius Werner (Code Review) 31 Jan '25

31 Jan '25

Attention is currently required from: Dinesh Gehlot, Intel coreboot Reviewers, Jayvik Desai, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Subrata Banik. Julius Werner has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/86227?usp=email ) Change subject: soc/intel/adl: Handle critical low battery early in romstage ...................................................................... Patch Set 3: (2 comments) File src/soc/intel/alderlake/romstage/fsp_params.c: https://review.coreboot.org/c/coreboot/+/86227/comment/fd870af4_7068bc5a?us… : PS3, Line 425: elog_add_event_byte(ELOG_TYPE_LOW_BATTERY_INDICATOR, ELOG_FW_ISSUE_SHUTDOWN); You should log the event before you delay, in case the system browns out in those 3 seconds. https://review.coreboot.org/c/coreboot/+/86227/comment/470476dd_43d2123d?us… : PS3, Line 446: early_shutdown_if_battery_below_critical_threshold(); Why is this inserted here? Isn't this the code path that only happens when MRC cache data was not found? Do you only want to shut down on low battery if you need to do memory training? (The commit message doesn't really explain it that way...) In general, I don't understand why this would be in the FSP code at all. What does this have to do with FSP? I would rather expect this to just be at the start of `mainboard_romstage_entry()` somewhere, or even directly called from `romstage_main()`. -- To view, visit https://review.coreboot.org/c/coreboot/+/86227?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ib4be86ed17818ee05b7bec0337a90f80017183c2 Gerrit-Change-Number: 86227 Gerrit-PatchSet: 3 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Sat, 01 Feb 2025 00:13:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2025