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 12:
(30 comments)
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 10: 3 Is this true for all platforms? I think there should not be any default here and instead the platforms that support it should set it accordingly.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 760: set_cse_next_boot_partition
It sends HECI command to notify CSE about it's next boot partition to boot from on next reset. […]
Can you please add all these expectations as comments in .h file where the function is declared?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 71: 0 0x0 to make it consistent with 0xf0 below.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 72: 5 0x5
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 72: MKHI_GROUP_ID_HMRFPO Commit message does not talk about this change in name for the macro. It would be ideal to have a separate change for reorganizing MKHI_GROUP_IDs.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 73: Use tabs for consistency.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 86: #define ME_HFS_CWS_NORMAL 5 : #define ME_HFS_MODE_NORMAL 0 : #define ME_HFS_TEMP_DISABLE 3 : #define HECI_OP_MODE_SEC_OVERRIDE 5 These are all named so differently -- it is not clear which field within HFS they apply to. Also are these values consistent across all different platforms?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 91: * Space before *
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 91: Boot partition info and switching command Ids All commands here typically start with MKHI_. Can you please follow the same conventions. Also, can all the host commands be grouped together in some way? In fact, all the macros used above are ordered very randomly. Can you please organize these in a better way?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 92: Use tabs for consistency.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 93: Use tabs for consistency.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 99: Why is this blank line added?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 749: Partition Info resp Failed Use lowercase for leading characters.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 757: Partition Same here.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 770: struct set_boot_partition_info_resp { : struct mkhi_hdr hdr; : } __packed; Why not just struct mkhi_hdr switch_resp;?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 781: partition_id != BP1 && partition_id != BP2 So, BP1 and BP2 are fine but not BP3? Why?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 153: enum Do we need enum for this at all? Can this be something like: #define BOOT_PARTITION(n) ((n) - 1)
If more partitions are supported in the future, you won't have to update it.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 160: typedef Let's just use "enum boot_partition_status" instead of the typedef.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 167: partition_fw_version How does this relate to the fw version that is currently decoded by each SoC driver? e.g. https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/cannonlake/...
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 175: struct partition_fw_version fw_version; : boot_partition_status bp_status; Please add comments.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 177: addr I believe you mean "CSE region start"?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 179: CSE region addr same here.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 185: /* : * Boot Partition Info Flag Redundancy Enabled : * It indicates CSE supports RO and RW regions. : */ : #define BP_INFO_REDUNDANCY_EN (1 << 0) : : /* : * Boot Partition Info Flag Minimum Recovery Mode Enabled : * It indicates RO supports Minimum Recovery Mode. : */ : #define BP_INFO_MIN_RECO_MODE_EN (1 << 1) : : /* : * Boot Partition Info Flag Read-only Config Enabled : * The flag indicate ME enabled HW protection to CSE RO region. : */ : #define BP_INFO_READ_ONLY_CFG (1 << 2) Can these be put together into a enum boot_partition_info_flags?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 210: /* : * Next requested boot partition is updated when : * set_cse_next_boot_partition() is invoked. : */ What does that mean?
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 216: * space before *
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 220: Just one space should be enough.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 225: a and?
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 147: MAX_ME_PARTITIONS
Ok, We should add a config to indicate if the FWU is supported on the platform, and then we can tie […]
Yes, that would be ideal.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 183: /* Boot Partition Info Flag Redundancy Enabled */ : #define BP_INFO_REDUNDANCY_EN (1 << 0) : : /* Boot Partition Info Flag Min. Recovery Mode Enabled */ : #define BP_INFO_MIN_RECO_MODE_EN (1 << 1) : : /* Boot Partition Info Flag Read only Config Enabled */ : #define BP_INFO_READ_ONLY_CFG (1 << 2)
In order to support CSE FW update functionality with resilience, the CSE FW logically divided into R […]
Can you please add this information to the comments? It would also be good to know what REDUNDANCY_EN or RECO_MODE_EN really means.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 212: /* Retrieves boot partitions Info CSE */
The get_boot_partition_info_resp is utilized in multiple places at different points in the update fl […]
I haven't looked at the CLs later in the train, but is it really necessary to expose get_boot_partition_info_resp in the header file? Can the required information be exposed via more granular APIs?
-
Furquan Shaikh (Code Review)