Sridhar Siricilla 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 72:
(29 comments)
https://review.coreboot.org/c/coreboot/+/35402/69//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/69//COMMIT_MSG@15 PS69, Line 15: |C
nit: add a space?
Done
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 39: __packed
Does this need to be packed?
Yes, it required since the data of this type returned by CSE.
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 56: __packed
And this one?
Yes, it required since the data of this type returned by CSE.
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 60: cse_check_prereq_bp_cmd_info
I would rename this function so that it looks like a question. For example: […]
Done
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 62: if (cse_is_hfs1_cws_normal() && (cse_is_hfs1_com_normal() || : cse_is_hfs1_com_secover_mei_msg() || cse_is_hfs1_com_soft_temp_disable())) : return true; : return false;
How about splitting this out for readability? […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 85: struct
static […]
I renamed it.
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 b […]
I increased the buffer, now no copying is required.
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.
Yes, it's no longer 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.
This is not required.
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.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 190: bool
Same comments here as above.
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 239: memcpy
There is no need to memcpy. […]
Done
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.
Done
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
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 43:
nit: Use tabs for consistency
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 229: CSE
Isn't this specific to custom SKU?
Any CSE Firmware SKU has max 3 boot partitions.But, they are layout is specific to CSE SKU. Though Custom has SKU 2 Boot partitions(BP1 & BP2), CSE still returns 3 boot partitions, but third boot partition's status is set to BP_STATUS_PARTITION_NOT_PRESENT always. I modify the layout/comment to match with CSE 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. […]
Agreed.
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. […]
Your understanding is correct with respect to CSME Custom SKU. Since the CL is for CSME SKU Custom only, the BP3 definition can be removed safely.
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?
Done
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.
Relevant only during CSE FW downgrade support. I remove it now.
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?
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 287: Minimum
Minimal?
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 306: it's
nit: its
It's missed.. I will address it later.
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 308: be
nit: can be
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 308: Inorder
nit: In order
Done
https://review.coreboot.org/c/coreboot/+/35402/71/src/soc/intel/common/block... PS71, Line 325: /
I think: […]
Done
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/69/src/soc/intel/common/block... PS69, Line 231: |
There's some misalignment around here
Done
-
Sridhar Siricilla (Code Review)