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 73:
(23 comments)
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/cannonlake/m... PS73, Line 175: #if CONFIG(SOC_INTEL_CSE_FW_SKU_CUSTOM) : BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_EXIT, cse_fw_sync, NULL); : #endif I am not sure if this was already answered somewhere. But, why are we waiting until BS_DEV_ENABLE boot state to run the sync step? The only requirement currently as I understand it is DRAM init should be done. So, can this sync happen as part of romstage post DRAM init? What are the challenges?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 16: SOC_INTEL_CSE_FW_SKU_CUSTOM nit: Should this be SOC_INTEL_CSE_CUSTOM_SKU?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 15: #define GET_BP(bp_index) (bp_index + 1) Is this used anywhere?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 20: CSE Firmware Custom SKU contains 2 boot partitions It would be good to add information here that CSE really supports upto 3 boot partitions. For custom SKU, only 2 boot partitions are used and 3rd is set to BP_STATUS_PARTITION_NOT_PRESENT. And then rest of the comment here.
It will make it clear why CSE_MAX_BOOT_PARTITIONS is set to 3 here.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 32: some other some?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 126: static bool cse_is_bp_cmd_info_possible(void) Can you please add comment indicating what this function really does? Right now its not very clear unless you dig through the documents as to why the different checks are being performed below.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 152: if (!cse_is_bp_cmd_info_possible()) { : printk(BIOS_ERR, "cse_bp: CSE does not meet prerequisites\n"); : return false; : } Can this check be moved to cse_fw_sync() so that you don't have to repeat this in both get and set functions.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 174: and issues reset based : * on the reset type argument. This function does not really do the reset.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 176: be nit: can be
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 179: NOTE: During CSE FW update, coreboot can notify CSE to boot from RO(BP1), and shall issue : * GLOBAL_RESET(CSE Reset Only) command. This change doesn't really deal with CSE FW update. I don't think it is right to reference that in this CL. Also, based on the bug discussions, I think we don't really want o do CSE only reset.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 227: cse_set_next_boot_partition Some parts of the comment for the function above really apply to this function e.g. reset. Those should be moved here.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 233: if (rst_type != CSE_RESET_ONLY) I don't see this being requested anymore. Probably we can get rid of this check?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 276: rw_bp->status == BP_STATUS_GENERAL_FAILURE Would the status be set to BP_STATUS_GENERAL_FAILURE if coreboot had asked CSME to jump to RW last time and that failed for some reason?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 283: cse_is_rw_info_valid Why have this extra function which is just calling cse_is_rw_status_valid()? I think we can make a call to cse_is_rw_status_valid() directly from cse_fw_sync()
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 312: for (bp = 0; bp < cse_bp_info->total_number_of_bp; bp++) Why run a separate loop? I think it would be helpful to have all information about a partition printed out together:
ME: RO version = x.y.z.w (Status = Valid, Start offset = 0x..., End offset = 0x...) ME: RW version = a.b.c.d (Status = Valid, Start offset = 0x..., End offset = 0x...)
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 326: get_bp_info Just bp_info since it holds boot partition information.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 326: struct static. I think its pretty big and probably not a good idea to store it on stack?
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 330: BIOS_DEBUG BIOS_ERR? I believe we should never reach this point if CSE FW CUSTOM config is not selected.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 339: /*TODO: Check CONFIG_GBB_FLAG_DISABLE_CSE_FW_SYNC is not set */ I think we can skip the TODO here since you already have the partner issue raised to track this.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 345: if (!cse_boot_to_ro(&get_bp_info.cse_bp_info)) { : printk(BIOS_ERR, "cse_bp: Failed to boot to RO\n"); : goto failed; : } This is not required. We can just continue booting with whatever partition is currently being used for CSME. This also ensures that we do not get stuck in a boot loop in case CSME does not jump back to RO for any reason. See https://docs.google.com/document/d/1qzFaj1B1R-7s1f54LlHoCkgKgEpKnAKuDY0IWHrr...
Also, this check for recovery mode can be performed before cse_get_bp_info() since we do not really need to do anything with boot partitions in recovery mode.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 364: : Just a note so that we don't forget to update this before pushing this in - We need to set recovery reason and then reboot.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 227: Sets boot partition of CSE firmware This function actually does more than just setting boot partition.
https://review.coreboot.org/c/coreboot/+/35402/73/src/soc/intel/common/block... PS73, Line 227: SKU Custom nit: Custom SKU
-
Furquan Shaikh (Code Review)