Furquan Shaikh 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 79:
(43 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?
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?
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. But it would be good to be consistent.
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.
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.
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.
https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@44 PS79, Line 44: Verified TEST=
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... File Documentation/soc/intel/cse_fw_update/cse_fw_update.md:
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 12: are nit: that are
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 26: RW Partition update is supported by CSE FW. I am not really sure what this statement means. You already mentioned above that CSE FW layout has two partitions.
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 38: Start/End nit: 's' and 'e' should be lowercase
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 58: components nit: '.' at the end.
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 63: next boot partition just say "partition"
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 78: ME I mentioned this on the commit message as well. ME, CSME and CSE are used interchangeably. Can you make it consistent so that a single terminology is used?
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 94: accommodate larger CSE binary This is confusing. I posted a comment about this on commit message as well.
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 95: respective RW What does this mean?
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... 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.
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... 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.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 24: "Name of CSE Region in FMAP" I don't think this has to be a user-visible option.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 25: default "SI_ME" Can you please add a help text indicating what this option is?
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 28: "CBFS entry name" Same here.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 29: 4 sizeof(uint32_t)
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 176: static void cse_trigger_recovery(uint8_t rec_sub_code) : { : if (CONFIG(VBOOT)) { : struct vb2_context *ctx; : ctx = vboot_get_context(); : vb2api_fail(ctx, VB2_RECOVERY_CSME_FAILURE, rec_sub_code); : vboot_save_data(ctx); : vboot_reboot(); : } else : do_global_reset(); : } : Looking at the entire CL, I think addition of just this function can be put as a separate CL and can be added before this change. We can push this in sooner and in smaller parts. That way we have the CSME Lite SKU booting setup complete. And this CL would be the next step i.e. performing an actual update.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 225: return cse_get_bp_entry_version(RW, cse_bp_info); Just place the contents of cse_get_bp_entry_version here?
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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 function results in a reset and so it will never return control.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 381: die("cse_bp: Failed to reset system\n"); We should trigger recovery in this case as well.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 404: uint32_t size_t
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 405: uint32_t size_t
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 415: locate %s in CBFS\n" This is not locating in CBFS. It is actually finding the region using FMAP and returning a region device pointer to it.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 447: It calculates CSE RW region's start offset and size This function does more than just calculating the offset and size. In fact, offset and size are already present in cse_bp_info, right?
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 458: region_device_offset(&cse_region_rdev)); This can fit on the previous line.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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().
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 486: cse_get_cbfs_rw_version(cse_rw_n_ver_data) Why create a function? It is not really doing much. BTW, you can simply pass source_rdev here and it can do a rdev_readat() instead of doing a rdev_mmap_full() right away.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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. No use of a function which just calls another function.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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. As mentioned above you can simply pass the source_rdev and then use rdev_chain here to clip the region device as per what you want. Then you don't really have to do any of this math.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 592: if (!cse_hmrfpo_enable()) : return false; : : return true; Just "return cse_hmrfo_enable();"
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 609: cse_cbfs_rw_n_ver cse_cbfs_rw_n_ver is confusing. cse_cbfs_rw should be fine? Also, you don't really need to map the whole thing right away. You can pass in source_rdev structure to the functions and the function that cares only about the version can do a rdev_readat(). Then the function which does the write can do a rdev_chain followed by rdev_mmap and use that to write the RW partition.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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:
if (cse_is_update_required()) ret = cse_trigger_update();
rdev_munmap(...);
Then you don't need to sprinkle the munmap calls in multiple places.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... 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 not very clear what functions really return and which don't. Can we plumb back all failure cases to the top such that the triggering of recovery happens by the top-level function cse_fw_sync? You can change the return type to status code i.e. 0 = success non-zero = recovery reason.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 631: rdev_munmap(source_rdev, cse_cbfs_rw_n_ver); Why is rdev_munmap() called conditionally?
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 660: Otherwise, nit: CSE is set to boot from RW in both cases right? i.e. whether CSME RW binary is present in CBFS or not.