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 71:
(41 comments)
https://review.coreboot.org/c/coreboot/+/35403/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35403/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/common/basecode: Implement CSE update flow
Please add documentation for this feature.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Kconfig:
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 1: INTEL_CSE_UPDATE
I don't think this Kconfig is required. […]
Fine with build process. But, please note in below two cases, coreboot has same action that is skipping CSE FW update. 1. ME_RW_FILE is not available in CBFS 2. Failed to read ME_RW_FILE from CBFS in any case
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 13: ME_RW_VER_CBFS
I have no clue what this config means from its name or string description.
It's being used as CBFS name for blob ME Image+Version
https://review.coreboot.org/c/coreboot/+/35403/14/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/cse_update.c:
https://review.coreboot.org/c/coreboot/+/35403/14/src/soc/intel/common/basec... PS14, Line 33: #define CBFS_SI_ME "SI_ME"
should be defined in a common header, discussable if that should be in commonlib […]
Moved it to Kconfig since possibility of different name from board to board!
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/cse_update.c:
PS67:
What are the reasons behind adding this to basecode instead of block/cse? […]
Initially CSE FW Update is seen as a separate feature, so implemented in basecode. Now, it's all specific to CSE Firmware custom sku. Moving this code to block/cse make sense.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 33: #define CBFS_SI_ME "SI_ME"
This is making an assumption that the FMAP name is always the same. […]
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 36: #define ME_RW_VERSION_SZ 16
Humm... I am working on some proposals to see if we can do this differently. […]
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 41: #define ME_BP_SIGN_SIZE 4
What exactly does this mean?
Every CSE BP starts with fixed signature. During FW Update, after RW(BP2) partition's erase, CB starts update without signature bytes. At the end, it updates signature bytes to indicate CSE RW update is done without interrupts. So, the signature helps to detect partial updates of CSE Firmware. Later, If coreboot finds RW without signature, coreboot triggers FW Update. The followed flags are linked to this concept.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 43: /* ME RW's signature is valid */ : #define ME_RW_SIGN_VALID 0 : /* ME RW's Signature is not valid */ : #define ME_RW_SIGN_INVALID 1 :
Are these flags in some field?
The flags relevance to above explanation.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 47: : /* ME RW's status is valid */ : #define ME_RW_STATE_VALID 0 : /* ME RW's status is not valid */ : #define ME_RW_STATE_INVALID 1
Same here? Some context in comments would be helpful.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 68: CONFIG_ME_RW_VER_CBFS
Is this used for storing the CSE RW version as a separate file within CBFS? My understanding based o […]
The CBFS entry used to store CSE RW blob containing RW code and Version info.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 87: Master
What does Master mean here?
It's not relevant now. I remove it.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 88: me_region
This is a region_device and not simply a region. […]
CSE provides BP1 & BP2 ranges in response to GET_BOOT_PARTITION_INFO,which are relative to only CSE region not FMAP. So, it's important to get starting offset of CSE region to calculate BP2's start offset.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 98: bp2_start_offset + region_device_offset(&me_region);
This looks wrong. Shouldn't it be bp2_start_offset - region_device_offset(&me_region);? […]
Boot partitions' ranges returned by CSE are relative to CSE region not with respect to FMAP. RW region device should be derived from CSE response as CSE aware BP2 range (start and end offsets). So, to define start offset of RW rdev, starting offset of BP2 should be added to starting offset ME region as defined in the FMAP.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 99: bp2_end_offset + 1
This doesn't look right. It should be end_offset - start_offset + 1; […]
Yes,this is right -> end_offset - start_offset + 1;
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 122: eturn -1;
This is a bigger problem. […]
If coreboot cann't talk to CSE, then do_global_reset() triggers guaranteed global reset. So, coreboot is not required to talk to H1/EC to do host reset.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 130: heci_reset
Why is a heci_reset() required here?
This is must after CSE goes for reset. It's to re-syncronize CSE and HOST for heci communication.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 138: hfs1.data = me_read_config32(PCI_ME_HFSTS1); : printk(BIOS_DEBUG, "me_fwu: ME HFSTS1=0x%x\n", hfs1.data);
What is the purpose of reading and printing HFSTS1 here and after jumping to RO?
These messages are added for dubgging sake only. No significance as such.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 141: cse_get_current_boot_partition
There is no need to read it here. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 146: (current_bp != BP1)
This check can be moved to cse_boot_to_ro(). […]
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 159: cse_hmrfpo_get_status
Why is the status checked again? This already happens as part of cse_hmrfpo_enable(). […]
It's for doule confirmation. It's not required.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 178: cse_get_bp_entry_version
Add helper for cse_get_rw_version() and that can call cse_get_bp_entry_version() for BP2.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 213: uint32_t
size_t
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 222: uint8_t *cbfs_me_rw_wo_sign = (uint8_t *)cbfs_me_rw + ME_BP_SIGN_SIZE;
Why is the signature split and updated separately? Since the partition is erased, CSE-RW would not p […]
SIGNATURE split is to identify interrupted CSE FW update . If CSE FW update is interrupted for any reason, CSE will boot from RO (as update is being initiated after switching to RO). In this scenario, RW's signature is empty so coreboot can re-start FW Update process unconditionally.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 258: if (rdev_readat(me_rw_part, (void *) &me_bp_sign, 0, ME_BP_SIGN_SIZE) : != ME_BP_SIGN_SIZE)
Why is this check required? If last update was incomplete, what does the CSE report for Boot Partiti […]
CSE returns General failure if there is a incomplete update. It's good to have the test on top of CSE's boot partition status to differentiate. Here logic is, if RW's signature is good and if CSE reports GENRAL FAILURE for boot partition status id, then coreboot marks RW partition is bad.In this scneario, system recovery is required. If CSE RW signature is bad, then just recover the RW partition through CSE FW Upgrade. No need to place system in recovery mode.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 277: me_rw_part
The naming of variables is kind of confusing overall here. How about using: […]
Hmm.. I renamed it as source_rw_n_ver_rdev, target_rdev. Are they make sense?
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 285: CONFIG_GBB_FLAG_DISABLE_CSE_FW_SYNC
There are no details about this. […]
Sure I will raise
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 291:
I have mentioned this on cse.c as well. Just copying here for simplicity to compare: […]
I would like to make small change for point E) Check if RW update is required. It can be based on two things: (i) RW's signature is not matching BTW,If RW signature is not valid, status id for RW partition is set to 1. (ii) If status ID for RW partition is 0, and RW version in boot info does not match RW version in CBFS. If either of the above is yes, update is required
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 292: int rec_mode = vboot_recovery_mode_enabled();
At this point, the function should call cse_get_bp_info(&cse_bp_info); […]
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 294: if (rec_mode) { : printk(BIOS_DEBUG, "me_fwu: Skip FW update in the recovery path\n"); : return; : }
If it is recovery mode, then this function should ensure that it jumps to RO.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 304: /* : * TODO: : * Check for data mismatch through boot partition status, then : * - send DATA_CLEAR command to CSE : * - Switch to RW(BP2) : * - Issue Global Reset : */
Please raise bugs for any TODOs that you plan to address later.
DONE
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 312: get_me_rw_part
For consistency can all functions be named with _cse_ instead of _me_.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 317: if (get_cbfs_me_rw_ver(&me_rw_ver_cbfs) < 0) { : printk(BIOS_ERR, "me_fwu: Failed to get cbfs ME for update\n"); : goto failed; : } :
I am very confused why version is being looked for as a separate file in CBFS.
The same blob contains RW FW + Version.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 330: uint32_t
size_t
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 332: rdev_mmap
Okay after getting to this point, I realize that it is the same RW binary in CBFS and not two separa […]
I organized the code as above but deviated little bit. Please check.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 335: if (cbfs_me_rw_sz > region_device_sz(&me_rw_part)) { : printk(BIOS_ERR, : "me_fwu: cbfs me_rw size(0x%x) is more than me_rw partition size(0x%x)," : "skip update\n", cbfs_me_rw_sz, (uint32_t)region_device_sz(&me_rw_part)); : goto failed; : }
This will be checked by rdev_writeat using write_rdev. There is no need for an explicit check here.
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 346: if (!cse_check_rw_status(&me_rw_part_status)) { : printk(BIOS_ERR, "me_fwu: me rw status check fail\n"); : goto failed; : }
I think this check should happen initially as part of cse_get_boot_info() or maybe after that call s […]
I pushed this code after recovery check. If system is already in recovery, then it avoids putting system again in recovery.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/include/intelbasecode/cse_update.h:
PS67:
This file won't be required at all. See my comments on cse. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 868: enum boot_partition_id current_bp;
I would recommend maintaining `static struct cse_boot_partition_info cse_bp_info;` here and passing […]
I think static declaration is not required for cse_bp_info as it is being passed to all funcitons..
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 872: BIOS_DEBUG
Now, this will be an error since custom_sku. […]
Right
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 893: #if !CONFIG(INTEL_CSE_UPDATE)
There is no need to guard this or have a separate call just for updating RW. […]
Agreed