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 60:
(36 comments)
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 136: Info
It's correct as command name is SET BOOT PARTITION INFO,
Done
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... PS59, Line 65: ;
Thanks..I missed it ..
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 171: layout
Can you also add the graphic you put in commit message here? I think that is very helpful in underst […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 172:
nit: Just use spaces instead of tab?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 173:
nit: same here
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 179: typedef
Instead of typedef, just use enum boot_partition_id { […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 180: R
nit: lowercase 'r'.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 184: CSME
I know it's referring to the same entity, but for consistency, let's use CSE everywhere.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 184: M
nit: lowercase 'm'.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 193: */
It would be good to add a sentence indicating that this is returned in response to MKHI command for […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 194: typedef
same comment as above about typedef.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 195:
nit: period (.) at the end.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 199: shall
nit: should?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 210: This value isn't applicable to chrome platforms
This is a common driver code file. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 214: BP_STATUS_MANIFEST_AND_INFO_OK
What is the difference between BP_STATUS_SUCCESS and BP_STATUS_MANIFEST_AND_INFO_OK? Does BP_STATUS_ […]
BP_STATUS_MANIFEST_AND_INFO_OK is no longer valid status code. I removed it.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 232: RO
nit: RO(BP1)
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 234: 2
nit: (1 << 1). I think it makes it easier to read.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 239: relavant
nit: relevant
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 244: ME RW Status
What does this indicate? Is this returned in response to some command?
This status is not returned in response to any command. Basically CSE FW update library determines the status based on boot partition status and boot partition's signature.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 253: ME_RW_SIGN_INVALID
What is the difference between ME_RW_INVALID and ME_RW_SIGN_INVALID? How is the difference determine […]
ME_RW_INVALID/VALID are determined based on the boot partition status (BP_STATUS_GENERAL_FAILURE) Whereas ME_RW_SIGN_INVALID status will be indicated when there is partial update due to power failure/any reason.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 257: partition_fw_version
Shouldn't this be cse_fw_version? I don't think it is really tied to boot partition. E.g. […]
BP2 and BP3 always have same versions.But RO(BP1) and RW(BP2/BP3) can have different versions. By the way, no BP3 in the latest CSE FW. Although, I'm not convinced to name it as cse_fw_version.Thanks
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 267: CSE Reset
I am not sure how CSE reset works okay in this case. […]
Yes, GLOBAL RESET(GLOBAL_RESET_TYPE) is must to take effect of CSE's boot partition change. DEVIATION: During CSE FW update, coreboot can notify CSE to boot from RO(BP1), and shall issue GLOBAL_RESET(CSE Reset Only) command.This case should be used during CSE FW Upgrade, and coreboot should issue GLOBAL_RESET(GLOBAL_RESET_TYPE) command after CSE FW update.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 270: Returns false on failure and true on success.
I think this is fine, but I would really like the CSE functions to have a consistent scheme for retu […]
I agree with you on having common scheme for return values. Thanks
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 272: uint8_t
Use "enum boot_partition_id" instead?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 278: uint8_t
enum boot_partition_id
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: uint8_t
enum boot_partition_id
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 290: uint8_t
enum bp_info_flags
API returns member field "flags" of cse_boot_partition_info struct.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 296: uint8_t
enum boot_partition_id
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 296: uint8_t
enum boot_partition_status
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 302: uint8_t
enum boot_partition_id
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 308: uint8_t
enum boot_partition_id
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 308: start_end_offset
range? Just to keep the name shorter?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: enabled to support CSE FW Update
Why? Shouldn't it be redudancy and not min recovery?
In the latest CSE FW, this feature is not a selectable setting(or option). By default, it is enabled. Please note min recovery feature is relevant only if redundancy is enabled. RO also must support minimum recovery mode to support FW update.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: must
nit: must not required here.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 316: bool
Can return value be used to indicate true = enabled, false = disabled/error?
Error and disabled cases are handled differently in CSE FW Update library so modifying return values of the API is not required.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 328: bool
Why is a return value required for debug/print function?
Done
-
Sridhar Siricilla (Code Review)