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 44:
(67 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 21: #define MKHI_GROUP_ID_BUP_COMMON 0xf0 Can we keep all the MKHI_GROUP_ID* in one place? Maybe in cse.h?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 23: /* Boot partition info and set boot partition info command ids */ : #define MKHI_GET_BOOT_PARTITION_INFO_CMD_REQ 0x1C : #define MKHI_SET_BOOT_PARTITION_CMD_REQ 0x1D Same as MKHI_GROUP_ID*. I think it would be good to have all of these in a single place. Also, I think the names should have the group in it for identifying what group it belongs to e.g. MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO, MKHI_BUP_COMMON_SET_BOOT_PARTITION
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 32: boot_partition_info_entry I think this can be just name cse_boot_partition_entry or cse_bp_entry?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 37: boot_partition_status This should be fixed width here since it is being used in a packed structure sent/received from CSE.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 40: bp_ What do you think about skipping prefix bp_ here since this is embedded within boot_partition_info_entry already?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 64: static struct cse_boot_partition_info cse_bp_info; Do we really need to maintain the global state of this? In general, I would expect this to be used only in the CSE FW update path and most of the information might be required within the same calling chain?
Wouldn't it be better if the caller just passed in a structure around so that it can also be invalidated when necessary?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 81: memset(&info_req, 0, sizeof(info_req)); Is this really required? If you initialize the structure while declaring it above, then memset might not be required?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 109: Boot partition update failed This is very confusing. At first, it seems like update to boot partition failed. I think it might be better to name the above function as cse_get_boot_partition_info() instead of cse_update_boot_partition_info() and this string can be updated accordingly. In fact, in the failure path there are 3 error messages that get printed: "Get partition info resp failed: %d\n" "Could not get partition info\n" "Boot partition update failed\n"
Not sure if we really need another print here.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 116: cse_send_set_boot_partition_info_cmd _info_ in the name is not correct. Just cse_send_set_bp(...)?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 132: " %d\n", partition_id);
... so that it is helpful when debugging.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 133: goto failed; Just return false; here directly. There is nothing happening under "failed:" label.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 136: Info I don't think adding "Info" here is correct.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 137: switch_req.next_bp = partition_id; Just set this when initialize switch_req above in L124?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 142: memset(&switch_resp, 0, sw_resp_sz); Why is this required?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 144: goto failed; return false;
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 149: goto failed; return false;
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 161: cse_send_set_boot_partition_info_cmd I am curious why CSE cannot just return a status in response to MKHI_SET_BOOT_PARTITION_CMD_REQ indicating whether it has updated the next boot partition or not.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 164: * space before *
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 212: cse_bp_info.bp_entries[boot_partition].bp_status; Before doing this shouldn't you also check if cse_bp_info.total_number_of_bp > boot_partition?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 223: boot_partition Same comment as above. Actually, it might make sense to have a helper function cse_get_bp_entry() and pass into it the response structure and the partition id. And then check for NULL to determine whether to return some value from the entry or just return false.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 254: if ((bp_info_flags & BP_INFO_MIN_RECOV_MODE_EN) != BP_INFO_MIN_RECOV_MODE_EN) { : printk(BIOS_INFO, "Minimum Recovery Mode is not enabled\n"); : *min_rec_mode_en = 0; : } else { : printk(BIOS_INFO, "Minimum Recovery Mode is enabled\n"); : *min_rec_mode_en = 1; : } : : return true; I think all of this can be just reduced to:
return !!(bp_info_flags & BP_INFO_MIN_RECOV_MODE_EN);
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: me_rw_status In fact, why is there a need to introduce a new ME RW status? I think this function can simply be:
static bool cse_is_bp_valid(enum boot_partition_id id) { uint8_t bp_status;
if (!cse_get_bp_entry_status(id, &bp_status)) return false;
if (bp_status != BP_STATUS_SUCCESS) return false;
return true; }
bool cse_is_rw_valid(void) { return cse_is_bp_valid(BP2) && cse_is_bp_valid(BP3); }
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: bool Does this really need to be a bool? Can't we just return the state directly as the return value?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 295: bp2_status | bp3_status So, if BP2 is valid and BP3 is invalid, me_rw_status would be both valid and invalid?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 312: 2 This should be cse_bp_info.total_number_of_bp
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: ME: %s version = %d.%d.%d.%d Each SoC has its own way of printing ME version. Now that the boards using redundancy within CSE print this in a different way, both the paths should be combined into a unified way of printing ME version without having to add knowledge into each SoC about redundancy being used or not.
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 understanding what the partitions and layout look like.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 172: nit: Just use spaces instead of tab?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 173: nit: same here
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 { ... };
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 180: R nit: lowercase 'r'.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 184: M nit: lowercase 'm'.
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.
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 GET_PARTITION_INFO.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 194: typedef same comment as above about typedef.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 195: nit: period (.) at the end.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 199: shall nit: should?
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. The comment should indicate what the status message means and not make any assumptions about Chrome OS Platforms.
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_MANIFEST_AND_INFO_OK indicate some kind of failure, but mainfest is good?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 216: I nit: lowercase 'i'.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 221: */ It would be good to mentioned that this is returned in response to MKHI command for GET_PARTITION_INFO.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 222: bp_info_flags I think it would be good to be consistent with the other enums i.e. use boot_partition_flags
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 224: Boot Partition Info Flag nit: All these are boot partition info flags and that is already indicated in the comment before the start of the enum. I think you can get rid of "Boot Partition Info Flag" text in front of the comments for each enum entry.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 232: RO nit: RO(BP1)
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.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 239: relavant nit: relevant
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 239: BP_INFO_MIN_RECOV_MODE_EN Why? Shouldn't this be BP_INFO_REDUNDANCY_EN?
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?
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 determined?
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. Can BP2 and BP3 have different firmware versions? Or would you treat BP2 and BP3 i.e. CSE-RW having a FW version? Similarly, CSE-RO could have a firmware version.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 264: * I see a number of functions being exposed here. Do you really expect all of these to be used outside of this driver? I would expect a lot of these to just be required within the cse driver path which performs an update.
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. Once a different partition is selected to boot, shouldn't global reset be a requirement? There are other components that come from this partition and so should be re-loaded after the reset?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 267: can/should?
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 return values. I think most other functions before this returned 0 on error and 1 on success.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 272: uint8_t Use "enum boot_partition_id" instead?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 278: uint8_t enum boot_partition_id
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: uint8_t enum boot_partition_id
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 290: uint8_t enum bp_info_flags
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 296: uint8_t enum boot_partition_status
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 296: uint8_t enum boot_partition_id
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 302: uint8_t enum boot_partition_id
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 308: uint8_t enum boot_partition_id
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?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: must nit: must not required here.
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?
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?
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?
-
Furquan Shaikh (Code Review)