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/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
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 38: Start/End
nit: 's' and 'e' should be lowercase
Done
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 58: components
nit: '.' at the end.
Done
https://review.coreboot.org/c/coreboot/+/35403/79/Documentation/soc/intel/cs... PS79, Line 63: next boot partition
just say "partition"
Done
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. […]
Done
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.
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/cs... PS79, Line 95: respective RW
What does this mean?
Rephrased the statement.
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.
Ack
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.
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/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.
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/block... 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/block... PS79, Line 28: "CBFS entry name"
Same here.
Done
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
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/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.
Done
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 fun […]
Done
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.
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/block... 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/block... 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/block... 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/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. […]
The comment is landed at wrong function.
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.
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/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().
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/block... 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/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. […]
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/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. […]
Done
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();"
Ack
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.
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/block... 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/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: […]
Yes, we can add one more indirection. Done.
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 no […]
Ack
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?
It requires to unmap the mapped CBFS RW region.
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. […]
Ack
-
Sridhar Siricilla (Code Review)