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 71:
(23 comments)
I am posting comments as a whole on the patch series. Most comments are related so I have provided reference in a number of places.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 58: static struct cse_boot_partition_info cse_bp_info; There is no need to maintain a global state of this boot partition information. It can be passed in by the function that really needs it (cse_fw_sync() in follow-up CLs) and it can pass in a pointer to that to all functions needing it.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 85: struct static
This seems to be a pretty big structure.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 105: memcpy(&cse_bp_info, &info_resp.bp_info, sizeof(cse_bp_info)); i am actually thinking we should just pass in a big enough buffer that can hold the entire hdr and boot partition info from caller. Then you don't really need to memcpy the partition info.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 110: cse_init_bp_info This shouldn't be required.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 169: /* Force coreboot to initialize cse_bp_info */ : if (!cse_update_boot_partition_info()) { : printk(BIOS_ERR, "Boot partition update failed\n"); : return false; : } I don't think we need to maintain global state or keep it updated.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 181: bool This function can just return int or enum boot_partition_id.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 183: if (!cse_init_bp_info()) : return false; This can be removed from here and other functions with the global state gone.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 190: bool Same comments here as above.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 239: memcpy There is no need to memcpy. This function can simply return pointer to fw_version field within boot partition info.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 262: cse_check_rw_status See comments in follow-up CLs. I think this function needs to be slightly different.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 42: nit: Use tabs for consistency
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 43: nit: Use tabs for consistency
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 229: CSE Isn't this specific to custom SKU?
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 237: boot_partition_id Looking back at all the CLs, I think we shouldn't even expose these partition IDs. We can just keep this enum within custom_sku.c. And cse_fw_sync() can use explicit calls indicating ro or rw which I think is also easier to understand.
cse_switch_to_ro() cse_switch_to_rw()
And those can internally call cse_set_next_boot_partition(BP1/BP2).
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 247: /* BP3 contains other CSE FW modules, and not a bootable partition on it's own. */ : BP3 = 2 Why is BP3 being considered as a separate partition. As I understand it, CSME Custom SKU has two boot partitions -- BP1 (boot critical) and BP2 (boot critical + non-critical). What is the id BP3 used for? It is not really an independent partition of its own. Or is my understanding wrong?
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 253: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ Just MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO?
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 270: : /* : * Configuration data error occurred, it needs to be : * handled with MKHI DATA_CLEAR command. : */ : BP_STATUS_DATA_FAILURE = 3, I don't see this in BWG.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 280: MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ Just MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO?
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 287: Minimum Minimal?
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 306: it's nit: its
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 308: Inorder nit: In order
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 308: be nit: can be
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 325: / I think: a) All boot partition APIs should simply be added to custom_sku.c. It is only the custom SKU that really supports these. b) In the follow-up CL I have added this comment. All the CSE firmware update support should also be added to custom_sku.c c) Based on a) and b) none of the boot partition APIs need to be added to cse.h d) Also, most of these functions should be simplified to not have to deal with BP1/BP2, etc.
-
Furquan Shaikh (Code Review)