coreboot-gerrit September 2023

coreboot-gerrit@coreboot.org

1 participants
3824 discussions

[M] Change in coreboot[main]: soc/intel/cse: Back up PSR data during CSE FW downgrade
by Subrata Banik (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Anil Kumar K, Jérémy Compostella, Krishna P Bhat D, Paul Menzel, Rizwan Qureshi, sridhar siricilla. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email ) Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade ...................................................................... Patch Set 52: (2 comments) File src/soc/intel/common/block/cse/cse_lite.c: https://review.coreboot.org/c/coreboot/+/74577/comment/9cbd3f81_b98eb8e0 : PS50, Line 1125: An attempt to send PSR back-up command has been made. > > something like "Back-up failed", is what i'm looking for record purposes. I just wanted to ensure there is a tracker in FW (CMOS or elogtool)to justify the PSR data loss. If google cloud missed the device telemetry then there has to some tracker to tell what might have happened which causes the data lose. Like a heart beat implementation where we would like to keep the last status (good or bad). > > Yes, instead of adding a new CMOS state, please do that > we are creating an ELOG event in case of failures. That is in the subsequent patches. yes, i have reviewed that. would you mind to move this discussion merits into your original PSR design doc? so we can do a retro later for adding feature for next gen ? File src/soc/intel/common/block/cse/cse_lite.c: https://review.coreboot.org/c/coreboot/+/74577/comment/9c1a3ea1_bc288f14 : PS52, Line 1115: if (backup_psr_resp.status != PSR_STATUS_SUCCESS) > if we have this check for PSR bit (https://review.coreboot.org/c/coreboot/+/74874) > we will not be invoking this on non VPRO skus that i understand but I'm not sure if its okay to send PSR_BACKUP_DONE during failure and fix later in some patch train. may be worth puling these two lines inside the if block and return immediately ? ``` update_psr_backup_status(PSR_BACKUP_FAILED); return; ``` of assume `update_and_exit` block will always PSR_BACKUP_FAILED hence, you need to bail out if PSR is successful and PSR_BACKUP_SUCCESS at earliest. -- To view, visit https://review.coreboot.org/c/coreboot/+/74577?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I135d197b5df0a20def823fe615860b5ead4391f8 Gerrit-Change-Number: 74577 Gerrit-PatchSet: 52 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.corp-partner.google.com> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 18:24:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Comment-In-Reply-To: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: soc/amd/common: use common physical address bit reservation code
by Felix Held (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Fred Reitberger, Jason Glenesk, Jérémy Compostella, Marshall Dawson, Matt DeVillier. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78074?usp=email ) Change subject: soc/amd/common: use common physical address bit reservation code ...................................................................... Patch Set 2: (1 comment) File src/soc/amd/common/block/cpu/noncar/cpu.c: https://review.coreboot.org/c/coreboot/+/78074/comment/04b464ef_78afb502 : PS2, Line 45: else > This is not a blocker to me, I just noticed it so I posted the comment, feel free to do whatever you […] oh, i like that one. will add that as a follow up and add a suggested-by tag. what i had tried and didn't like too much was to just remove the else and move the return 0 to the indentation level the else was at -- To view, visit https://review.coreboot.org/c/coreboot/+/78074?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iad65144006f1116cd82efc3c94e1d6d1ccb31b6e Gerrit-Change-Number: 78074 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 18:19:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/cse: Back up PSR data during CSE FW downgrade
by Rizwan Qureshi (Code Review) 28 Sep '23

28 Sep '23

1 0

[XS] Change in coreboot[main]: src/security/tpm: Enable Hibernate on setup failure
by Jon Murphy (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Christian Walter, Jason Nien, Martin Roth, Paul Menzel. Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78098?usp=email ) Change subject: src/security/tpm: Enable Hibernate on setup failure ...................................................................... Patch Set 8: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/78098/comment/d383eb5f_3e16e163 : PS8, Line 11: GSC > What is GSC? Google Security Chip. It's our TPM that is used across ChromeBooks. Cr50 was the first version, Ti50 is the new version, so collectively we refer to them as GSC -- To view, visit https://review.coreboot.org/c/coreboot/+/78098?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2d9e8f75b25fb6c530a333024c342bea871eb85d Gerrit-Change-Number: 78098 Gerrit-PatchSet: 8 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 18:15:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/google/starmie: Modify AW37503 Power IC timing for panel
by Paul Menzel (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Hung-Te Lin, Yidi Lin, Yu-Ping Wu, cong yang. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78185?usp=email ) Change subject: mb/google/starmie: Modify AW37503 Power IC timing for panel ...................................................................... Patch Set 5: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/78185/comment/b1ce583a_ba45bd95 : PS5, Line 7: Modify AW37503 Power IC timing for panel Maybe more specific: > Add 3 ms delay to AW37503 Power IC panel timing -- To view, visit https://review.coreboot.org/c/coreboot/+/78185?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I488c746d1fcfc165125b0ecccb0bccbb99231b00 Gerrit-Change-Number: 78185 Gerrit-PatchSet: 5 Gerrit-Owner: cong yang <yangcong5(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: cong yang <yangcong5(a)huaqin.corp-partner.google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 17:57:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/cse: Back up PSR data during CSE FW downgrade
by Subrata Banik (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Anil Kumar K, Jérémy Compostella, Krishna P Bhat D, Paul Menzel, Rizwan Qureshi, sridhar siricilla. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email ) Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade ...................................................................... Patch Set 52: (1 comment) File src/soc/intel/common/block/cse/cse_lite.c: https://review.coreboot.org/c/coreboot/+/74577/comment/da51bcc6_5fa4e524 : PS50, Line 1125: An attempt to send PSR back-up command has been made. > > What do you mean by false logic? cse_boot_to_rw is not a false logic. if the device is unable to boot to RW and you aren't able to send PSR_HECI_FW_DOWNGRADE_BACKUP cmd, hence, you finally marked things as PSR backup done ? i don't understand the motivation here. > > > if you started ignoring all the failure cases and corner cases then I'm afraid we shall miss the original intent. What I'm asking is don't call the PSR backup done when it's not actually attempted or succeeded. Rather use something as PSR_BACKUP_ABORTED. At least it will help us to know whether the attempt is successful genuinely or not > > It makes sense, We can add definitely add another status to the CMOS variable state. something like "Back-up failed", but the action that will be taken subsequently would be same i.e., set the value to PSR back-up pending after doing a data clear. So the intermediate new state is anyway not visible or can be acted upon. For a back-up failed scenario there is no credible action that the firmware can take, but to allow boot and let the application in OS figure out the failure. > > Note that we are working under constraints. This behavior is only in chrome system where a downgrade has to be supported, PSR in windows doesn't need the back-up flow since they do not support downgrade and hence no data clear. > > We had brainstormed how we can preserve the data without this implementation. Multiple options were considered and none of them can be scoped in the current platform, require significant work and security reviews. > Eg: > option 1: Store the PSR data outside of CSE purview, H1 can get the data and store in it's flash > option 2: CSE will store the data outside of the normal data partition, which means it will not be encrypted and be decoded by old and new firmware. > > I understand your concern and while we work with Google and Intel teams to improve the design which better fits the chrome-intel system constraints, for the current platform this is the best way forward. ` something like "Back-up failed", ` is what i'm looking for record purposes. I just wanted to ensure there is a tracker in FW (CMOS or elogtool)to justify the PSR data loss. If google cloud missed the device telemetry then there has to some tracker to tell what might have happened which causes the data lose. Like a heart beat implementation where we would like to keep the last status (good or bad). -- To view, visit https://review.coreboot.org/c/coreboot/+/74577?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I135d197b5df0a20def823fe615860b5ead4391f8 Gerrit-Change-Number: 74577 Gerrit-PatchSet: 52 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.corp-partner.google.com> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 17:52:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Comment-In-Reply-To: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: src/security/tpm: Enable Hibernate on setup failure
by Paul Menzel (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Christian Walter, Jason Nien, Jon Murphy, Martin Roth. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78098?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: src/security/tpm: Enable Hibernate on setup failure ...................................................................... Patch Set 8: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/78098/comment/20c33cc5_3da0e3a6 : PS8, Line 11: GSC What is GSC? -- To view, visit https://review.coreboot.org/c/coreboot/+/78098?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2d9e8f75b25fb6c530a333024c342bea871eb85d Gerrit-Change-Number: 78098 Gerrit-PatchSet: 8 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 17:50:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/intel/cse: Select SOC_INTEL_CSE_LITE_SYNC_IN_RAMSTAGE when PSR en...
by Subrata Banik (Code Review) 28 Sep '23

28 Sep '23

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/78055?usp=email ) Change subject: soc/intel/cse: Select SOC_INTEL_CSE_LITE_SYNC_IN_RAMSTAGE when PSR enabled ...................................................................... soc/intel/cse: Select SOC_INTEL_CSE_LITE_SYNC_IN_RAMSTAGE when PSR enabled PSR data is created and stored in CSE data partition. In platforms that employ CSE Lite SKU firmware, a firmware downgrade involves clearing of CSE data partition which results in PSR data being lost. The PSR data needs to be preserved across the firmware downgrade flow. CSE Lite SKU firmware supports command to backup PSR data, and this command can be sent only in post-RAM stages. So the cse_fw_sync actions needs to be moved to ramstage. This patch ensures SOC_INTEL_CSE_LITE_SYNC_IN_RAMSTAGE is selected when PSR is enabled. BUG=b:273207144 Change-Id: I7c9bf8b8606cf68ec798ff35129e92cd60bbb137 Signed-off-by: Krishna Prasad Bhat <krishna.p.bhat.d(a)intel.com> Signed-off-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/78055 Reviewed-by: Anil Kumar K <anil.kumar.k(a)intel.com> Reviewed-by: Subrata Banik <subratabanik(a)google.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/soc/intel/common/block/cse/Kconfig 1 file changed, 5 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Anil Kumar K: Looks good to me, but someone else must approve Subrata Banik: Looks good to me, approved diff --git a/src/soc/intel/common/block/cse/Kconfig b/src/soc/intel/common/block/cse/Kconfig index e46becc..581cecd 100644 --- a/src/soc/intel/common/block/cse/Kconfig +++ b/src/soc/intel/common/block/cse/Kconfig @@ -144,10 +144,14 @@ bool default n depends on SOC_INTEL_CSE_LITE_SKU + select SOC_INTEL_CSE_LITE_SYNC_IN_RAMSTAGE help Select this config if Platform Service Record(PSR) is supported by the platform. This config is applicable only for Lite SKU, where PSR data backup is required prior to a - CSE firmware downgrade during which CSE data is cleared. + CSE firmware downgrade during which CSE data is cleared. PSR services in CSE FW is + enabled only post DRAM init and the command to backup PSR data is also supported only + post DRAM init. Hence platform that selects PSR would need to perform CSE firmware sync + in ramstage. config SOC_INTEL_CSE_SERVER_SKU bool -- To view, visit https://review.coreboot.org/c/coreboot/+/78055?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I7c9bf8b8606cf68ec798ff35129e92cd60bbb137 Gerrit-Change-Number: 78055 Gerrit-PatchSet: 5 Gerrit-Owner: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[main]: soc/intel/mtl: Override SOC_INTEL_CSE_FW_PARTITION_CMOS_OFFSET
by Subrata Banik (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Anil Kumar K, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Krishna P Bhat D, Rizwan Qureshi, Tarun. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78054?usp=email ) Change subject: soc/intel/mtl: Override SOC_INTEL_CSE_FW_PARTITION_CMOS_OFFSET ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/78054?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I8bae5245f93b99be15b4e59cfeffbc23eec95001 Gerrit-Change-Number: 78054 Gerrit-PatchSet: 4 Gerrit-Owner: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun <tstuli(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Attention: Tarun <tstuli(a)gmail.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 17:47:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: soc/intel/cse: Back up PSR data during CSE FW downgrade
by Rizwan Qureshi (Code Review) 28 Sep '23

28 Sep '23

Attention is currently required from: Anil Kumar K, Jérémy Compostella, Krishna P Bhat D, Paul Menzel, Subrata Banik, sridhar siricilla. Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email ) Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade ...................................................................... Patch Set 52: (2 comments) File src/soc/intel/common/block/cse/cse_lite.c: https://review.coreboot.org/c/coreboot/+/74577/comment/e51862b1_50211af6 : PS50, Line 1081: cse_boot_to_rw > > as explained above we do not want to prevent the system to boot whenever there is a failure to PSR […] While I understand you concern about losing the data and not being bothered, please have look at my reply below. I hope you understand. https://review.coreboot.org/c/coreboot/+/74577/comment/22ac6b48_5b0edb13 : PS50, Line 1125: An attempt to send PSR back-up command has been made. > What do you mean by false logic? cse_boot_to_rw is not a false logic. if the device is unable to boot to RW and you aren't able to send PSR_HECI_FW_DOWNGRADE_BACKUP cmd, hence, you finally marked things as PSR backup done ? i don't understand the motivation here. > if you started ignoring all the failure cases and corner cases then I'm afraid we shall miss the original intent. What I'm asking is don't call the PSR backup done when it's not actually attempted or succeeded. Rather use something as PSR_BACKUP_ABORTED. At least it will help us to know whether the attempt is successful genuinely or not It makes sense, We can add definitely add another status to the CMOS variable state. something like "Back-up failed", but the action that will be taken subsequently would be same i.e., set the value to PSR back-up pending after doing a data clear. So the intermediate new state is anyway not visible or can be acted upon. For a back-up failed scenario there is no credible action that the firmware can take, but to allow boot and let the application in OS figure out the failure. Note that we are working under constraints. This behavior is only in chrome system where a downgrade has to be supported, PSR in windows doesn't need the back-up flow since they do not support downgrade and hence no data clear. We had brainstormed how we can preserve the data without this implementation. Multiple options were considered and none of them can be scoped in the current platform, require significant work and security reviews. Eg: option 1: Store the PSR data outside of CSE purview, H1 can get the data and store in it's flash option 2: CSE will store the data outside of the normal data partition, which means it will not be encrypted and be decoded by old and new firmware. I understand your concern and while we work with Google and Intel teams to improve the design which better fits the chrome-intel system constraints, for the current platform this is the best way forward. -- To view, visit https://review.coreboot.org/c/coreboot/+/74577?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I135d197b5df0a20def823fe615860b5ead4391f8 Gerrit-Change-Number: 74577 Gerrit-PatchSet: 52 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.corp-partner.google.com> Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-CC: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: sridhar siricilla <siricillasridhar(a)gmail.com> Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-Comment-Date: Thu, 28 Sep 2023 17:23:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Comment-In-Reply-To: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit September 2023