coreboot-gerrit May 2020

coreboot-gerrit@coreboot.org

1 participants
2588 discussions

Change in coreboot[master]: sb/intel/i82371eb: Move wakeup code to romstage
by Keith Hui (Code Review) 04 May '20

04 May '20

Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40965 ) Change subject: sb/intel/i82371eb: Move wakeup code to romstage ...................................................................... sb/intel/i82371eb: Move wakeup code to romstage This code is needed in romstage, not ramstage. Change-Id: Ic38c3c50fb135fba582864a242348ec29cec2991 Signed-off-by: Keith Hui <buurin(a)gmail.com> --- M src/southbridge/intel/i82371eb/Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/40965/1 diff --git a/src/southbridge/intel/i82371eb/Makefile.inc b/src/southbridge/intel/i82371eb/Makefile.inc index 1a1a0a1..327c424 100644 --- a/src/southbridge/intel/i82371eb/Makefile.inc +++ b/src/southbridge/intel/i82371eb/Makefile.inc @@ -24,8 +24,8 @@ ramstage-y += smbus.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += fadt.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi_tables.c -ramstage-$(CONFIG_HAVE_ACPI_RESUME) += wakeup.c +romstage-$(CONFIG_HAVE_ACPI_RESUME) += wakeup.c romstage-y += early_pm.c romstage-y += early_smbus.c -- To view, visit https://review.coreboot.org/c/coreboot/+/40965 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic38c3c50fb135fba582864a242348ec29cec2991 Gerrit-Change-Number: 40965 Gerrit-PatchSet: 1 Gerrit-Owner: Keith Hui <buurin(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

3 2

Change in coreboot[master]: soc/intel/common/basecode: Implement CSE update flow
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Furquan Shaikh (Code Review) 04 May '20

04 May '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 81: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 81 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 May 2020 07:09:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/basecode: Implement CSE update flow
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

1 0

Change in coreboot[master]: soc/intel/common/basecode: Implement CSE update flow
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35403 ) Change subject: soc/intel/common/basecode: Implement CSE update flow ...................................................................... Patch Set 80: (39 comments) https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@14 PS79, Line 14: The CSME size is 3.9MB. > For what platform? The CSE size is not correct. By the way, CSE custom SKU size is 2.41MB for CML platform. Adjustment in FMAP/Descriptor may require, but that depends on allocation is done for CSE region and FW_MAIN_A/B. I have tested CSE Custom SKU patchset on helios/hatch without making any FMAP changes. Also, I think we don't need to mention FMAP/Descriptor requirements here. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@17 PS79, Line 17: CSE binary. > What is the difference here between ME update binary and CSE binary? Here ME Update Binary -> CSE RW blob. CSE Binary -> CSE Custom SKU Image I will remove the requirement. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@18 PS79, Line 18: CSME > I understand that you are referring to the same thing with CSME, CSE and ME. […] Done https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@19 PS79, Line 19: which are ~1.5 MB > for what platform? I don't think this number is true for every Intel platform. Yes, numbers are not required here as they are not true for all Intel platforms. https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@33 PS79, Line 33: - Send global reset command to reset only the CSME > I thought we agreed on not doing a CSME only reset. Ack https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@34 PS79, Line 34: Wait for CSME to enter SOFT_TEMP_DISABLE operation mode : (indicated by HFSTS1 register bit 19:16) > BIOS/CPU wouldn't really wait. It gets reset as well. Ack https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… File Documentation/soc/intel/cse_fw_update/cse_fw_update.md: https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 12: are > nit: that are Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 26: RW Partition update is supported by CSE FW. > I am not really sure what this statement means. […] Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 38: Start/End > nit: 's' and 'e' should be lowercase Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 58: components > nit: '.' at the end. Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 63: next boot partition > just say "partition" Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 78: ME > I mentioned this on the commit message as well. ME, CSME and CSE are used interchangeably. […] Done https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 94: accommodate larger CSE binary > This is confusing. I posted a comment about this on commit message as well. This is no longer valid statement. It was written before CSE custom SKU is ready. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 95: respective RW > What does this mean? Rephrased the statement. https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 103: Send GLOBAL_RESET HECI command to reset only the CSE. : - Wait for CSE to enter Soft Temporary Disable Mode. > This needs update. Ack https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/c… PS79, Line 121: The RW FW checks if the system is is in recovery mode, if it's in then : skips CSE FW uupdate flow. > This statement doesn't make sense. In recovery mode, RW FW is not selected at all. Yes, vboot does not select FW_MAIN_A/B if system is in recovery. But, our RW code still check if system is in recovery mode or not as the same code sits in coreboot (READ_ONLY chunk) and FW_MAIN_A/B. As you know, it does to skip FW update flow in recovery mode. The statement has to be refined bit that can be true in both normal and recovery scenarios. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 24: "Name of CSE Region in FMAP" > I don't think this has to be a user-visible option. our assumption as disucssued in earlier patches, FMAP entry for CSE region can be platfrom specific.So, mainboard Kconfig can override the default option. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 25: default "SI_ME" > Can you please add a help text indicating what this option is? Done https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 28: "CBFS entry name" > Same here. Done https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/custom_bp.c: PS79: > This file name should be updated to cse_custom_sku or cse_lite_sku Yes, I will rename the file name and all references to custom SKU in a separate patch. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 31: /* : * If CSE RW signature is valid, : * then last update was successful : */ : #define CSE_RW_SIGN_VALID 1 : : /* : * If CSE RW signature is not valid, : * then last update was not successful : */ : #define CSE_RW_SIGN_INVALID 0 > These are unused. Done https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 375: cse_set_and_boot_from_next_bp > Can you please add comments to the functions? In this case, it is important to mention that this fun […] Done https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 381: die("cse_bp: Failed to reset system\n"); > We should trigger recovery in this case as well. Hmm.. Yes, the call chain triggers recovey on error path if I remove die() call. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 404: uint32_t > size_t uint32_t seems to be ok. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 405: uint32_t > size_t uint32_t seems to be ok. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 415: locate %s in CBFS\n" > This is not locating in CBFS. […] Yes. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 447: It calculates CSE RW region's start offset and size > This function does more than just calculating the offset and size. […] The comment is landed at wrong function. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 458: region_device_offset(&cse_region_rdev)); > This can fit on the previous line. It will not fit in one line. It will come to 97 chars if I merge both the lines into one. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 462: : if (boot_device_rw_subregion(&cse_rw_region, target_rdev) < 0) : return false; > There is no need to do the math for getting the actual offset. You can use rdev_chain(). I didn't understand your comment here. Are you talking about the logic defined in the cse_setup_rw_region()? If not, please elaborate. Thanks https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 486: cse_get_cbfs_rw_version(cse_rw_n_ver_data) > Why create a function? It is not really doing much. […] It was added helper function intentionally to have a placeholder to redefine/expand the logic( (if required) to retrieve version. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 550: return !cse_check_version_mismatch(cse_bp_info, cse_cbfs_rw_n_ver); : } > I think the contents of cse_check_version_mismatch() can simply be placed into this function. […] This is added for readability. There is follow-up patch, we have a helper function (cse_is_fw_downgrade) which check versions for downgrade. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 571: void *cse_cbfs_rw = cse_get_cbfs_rw_map(cse_cbfs_rw_n_ver); : size_t cse_cbfs_rw_sz = cse_cbfs_rw_n_ver_sz - CSE_RW_VERSION_SZ; : > You don't really have to do the math here. […] Done https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 592: if (!cse_hmrfpo_enable()) : return false; : : return true; > Just "return cse_hmrfo_enable();" Ack https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 608: Layout of SOC_INTEL_CSE_RW_CBFS = [<CSE RW Version>(16 bytes) + <CSE RW Firmware>] > I want to revisit this as mentioned on the bug. Yes. it's still under discussion. How about go ahead with upstreaming with current proposal? Latet, once we conclude on it, we can make changes accordingly. Let me know your comments. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 609: cse_cbfs_rw_n_ver > cse_cbfs_rw_n_ver is confusing. […] I thought cse_cbfs_rw(which I used in earlier patches) would be confusing. This does not represent CBFS RW blob. So, I chose the cse_cbfs_rw_n_ver ( CSE_CBFS_RW_and_Version blob). I switch to original naming. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 615: if (!cse_is_rw_bp_signature_valid(&target_rdev) || : !cse_is_rw_version_latest(cse_bp_info, cse_cbfs_rw_n_ver)) { > This whole if block can be arranged such that: […] Yes, we can add one more indirection. Done. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 622: cse_trigger_recovery(CSME_FAILURE_COMMUNICATION_ERROR); > It looks like the triggering of recovery is currently happening from multiple places and so it is no […] Ack https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 631: rdev_munmap(source_rdev, cse_cbfs_rw_n_ver); > Why is rdev_munmap() called conditionally? It requires to unmap the mapped CBFS RW region. https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/bloc… PS79, Line 660: Otherwise, > nit: CSE is set to boot from RW in both cases right? i.e. […] Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/35403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I12f6bba3324069d65edabaccd234006b0840e700 Gerrit-Change-Number: 35403 Gerrit-PatchSet: 80 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Andrey Petrov <anpetrov(a)fb.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 May 2020 06:23:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 81: (10 comments) https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/35402/80//COMMIT_MSG@32 PS80, Line 32: coreboot ensure CSE to boot from RO(BP1) if system is in Recovery mode. > This is no longer true. Ack https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/Kconfig: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 21: SKU Custom > Still pending. Done https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/custom_bp.c: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 23: > nit: No space before /* Ack https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 29: It's a valid bootable partition. > nit: Does this fit on the previous line? Done https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 44: present based on layout, but it is not valid. > nit: I think part of this comment can go to previous line. Done https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 50: present per initial image layout. > nit: This can go on previous line. Done https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 58: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ > nit: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO Ack https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 331: do_global_reset > Just making a note: This is not the right way to handle the failure case. […] Ack https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… File src/soc/intel/common/block/include/intelblocks/cse.h: https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 231: triggers recovery mode if CSE fails to jump to RW > Just a note: This is not yet implemented in this CL. It will be done in follow-up CLs. Ack https://review.coreboot.org/c/coreboot/+/35402/80/src/soc/intel/common/bloc… PS80, Line 232: RW partition > nit: whatever is the currently selected partition. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 81 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 May 2020 06:16:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: payloads/external/GRUB2: prevent rebuild without actual changes
by Werner Zeh (Code Review) 04 May '20

04 May '20

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40955 ) Change subject: payloads/external/GRUB2: prevent rebuild without actual changes ...................................................................... Patch Set 1: Code-Review+1 (1 comment) https://review.coreboot.org/c/coreboot/+/40955/1//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/40955/1//COMMIT_MSG@12 PS1, Line 12: There are three Either you have forgotten the third one or you should say "two cases" here? -- To view, visit https://review.coreboot.org/c/coreboot/+/40955 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibf5a316d0c9761426ec2ee306d78cd56d4bf19b5 Gerrit-Change-Number: 40955 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Denis Carikli <GNUtoo(a)no-log.org> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 04 May 2020 06:06:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/basecode: Implement CSE update flow
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Subrata Banik, Balaji Manigandan, Aamir Bohra, Patrick Rudolph, V Sowmya, Andrey Petrov, Jamie Ryu, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/35403 to look at the new patch set (#80). Change subject: soc/intel/common/basecode: Implement CSE update flow ...................................................................... soc/intel/common/basecode: Implement CSE update flow The following changes have been done in this patch: Get the CSE partition info containing version of CSE RW using GET_BOOT_PARTITION_INFO HECI command Get the me_rw.version from the currently selected RW slot. If the version from the above 2 locations don't match start the update - If CSE's current boot partition is not RO, then * Set the CSE's next boot partition to RO using SET_BOOT_PARTITION HECI command. * Send global reset command to reset the system. - Enable HMRFPO (Host ME Region Flash Protection Override) operation mode using HMRFPO_ENABLE HECI command - Erase and Copy the CBFS CSE RW to CSE RW partition - Set the CSE's next boot partition to RW using SET_BOOT_PARTITION HECI command - Trigger global reset The system should boot with the Updated CSE RW partition. TEST=Verified basic update flows on hatch and helios. BUG=b:111330995 Change-Id: I12f6bba3324069d65edabaccd234006b0840e700 Signed-off-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Signed-off-by: V Sowmya <v.sowmya(a)intel.com> --- A Documentation/soc/intel/cse_fw_update/Layout_after.svg A Documentation/soc/intel/cse_fw_update/Layout_before.svg A Documentation/soc/intel/cse_fw_update/cse_fw_update.md M Documentation/soc/intel/index.md M src/soc/intel/common/block/cse/Kconfig M src/soc/intel/common/block/cse/custom_bp.c 6 files changed, 703 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35403/80 -- To view, visit https://review.coreboot.org/c/coreboot/+/35403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I12f6bba3324069d65edabaccd234006b0840e700 Gerrit-Change-Number: 35403 Gerrit-PatchSet: 80 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Andrey Petrov <anpetrov(a)fb.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maxim Polyakov <max.senia.poliak(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Sridhar Siricilla (Code Review) 04 May '20

04 May '20

Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Wonkyu Kim, Sridhar Siricilla, Rizwan Qureshi, Subrata Banik, Balaji Manigandan, Aamir Bohra, Patrick Rudolph, V Sowmya, Nico Huber, Jamie Ryu, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/35402 to look at the new patch set (#81). Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... soc/intel/common/block/cse: Add boot partition related APIs In CSE Firmware Custom SKU, CSE region is logically divided into 2 boot partitions. These boot partitions are represented by BP1(RO), BP2(RW). With CSE Firmware Custom SKU, CSE can boot from either RO(BP1) or RW(BP2). The CSE Firmware Custom SKU layout appears as below: ------------- -------------------- --------------------- |CSE REGION | => | RO | RW | DATA | => | BP1 | BP2 | DATA | ------------- -------------------- --------------------- In order to support CSE FW update to RW region, below APIs help coreboot to get info about the boot partitions, and allows coreboot to set CSE to boot from required boot partition (either RO(BP1) or RW(BP2)). GET_BOOT_PARTITION_INFO - Provides info on available partitions in the CSE region. The API provides info on boot partitions like start/end offsets of a partition within CSE region, and their version and partition status. SET_BOOT_PARTITION_INFO - Sets CSE's next boot partition to boot from. With the HECI API, firmware can notify CSE to boot from RO(BP1) or RW(BP2) on next boot. As system having CSE Firmware Custom SKU, boots from RO(BP1) after G3, so coreboot sets CSE to boot from RW(BP2) in normal mode and further, coreboot ensure CSE to boot from whichever is selected boot partition if system is in recovery mode. BUG=b:145809764 TEST=Verified on hatch Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> --- M src/soc/intel/common/block/cse/Kconfig M src/soc/intel/common/block/cse/Makefile.inc A src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 4 files changed, 351 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35402/81 -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 81 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: payloads/external/GRUB2: Makefile: fix check for changed files
by Werner Zeh (Code Review) 04 May '20

04 May '20

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40953 ) Change subject: payloads/external/GRUB2: Makefile: fix check for changed files ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/40953 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie126e3d604990b2346f1f004f912080104e2789d Gerrit-Change-Number: 40953 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Niewöhner Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Denis Carikli <GNUtoo(a)no-log.org> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 04 May 2020 06:03:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2020