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 14:
(27 comments)
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
Can you please add all these expectations as comments in . […]
Done
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.
Done
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. […]
I updated commit message.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 72: 5
0x5
Done
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. […]
I will address this in separate patch.
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. […]
Done.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 92:
Use tabs for consistency.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 93:
Use tabs for consistency.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 99:
Why is this blank line added?
Removed
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.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 757: Partition
Same here.
Done
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;?
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 802: if (!cse_get_boot_partition_info(&resp)) : goto failed;
paranoia? if CSME has reported no error I think we can skip this check.
Removed
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 154:
space only
Done
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.
Since we are using the enum type in boot_partition_info_entry sturcture, so typedef is required.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 161:
space
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 167: partition_fw_version
The structures are same, moving to common is part of another patch. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 177: addr
We mean starting offset of the partition within CSE region.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 179: CSE region addr
We mean end offset of the partition within CSE region.
Done
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?
Done
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?
I have modified the comment. The field always points to next boot partition of CSE. In normal scenario, current_bp and next_bp are points to same boot partitions (either BP1 or BP2). Only when coreboot notifies CSE to modify next boot partition to desired boot partition, then only next_bp gets updated. The same can be noted through cse_get_boot_partition() function.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 216: *
space before *
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 220:
Just one space should be enough.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 225: a
and?
Done
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 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)
Can you please add this information to the comments? It would also be good to know what REDUNDANCY_E […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 212: /* Retrieves boot partitions Info CSE */
I haven't looked at the CLs later in the train, but is it really necessary to expose get_boot_partit […]
it could be done, but we would have to add a static/global structure to store the response which could be then utilized for extracting information.