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:
(23 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. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 37: boot_partition_status
This hasn't been addressed yet?
Done
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_e […]
Done
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;
Ack
Done
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 […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 116: cse_send_set_boot_partition_info_cmd
Not addressed?
The command name is SET_BOOT_PARTITION_INFO. So, I will rename function as cse_set_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 132: "
%d\n", partition_id); […]
Done
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.
Done
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.
It's correct as command name is SET BOOT PARTITION INFO,
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 137: switch_req.next_bp = partition_id;
Not addressed?
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 144: goto failed;
return false;
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 149: goto failed;
return false;
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 164: *
Not addressed?
Done
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. […]
Done
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;
Not addressed?
Suggested code looks good but it does not consider error scenario!
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?
If any boot partition is not valid, then it indicates RW partion is invalid.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 312: 2
This should be cse_bp_info. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/55/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/55/src/soc/intel/common/block... PS55, Line 101: if (!cse_update_boot_partition_info()) {
braces {} are not necessary for single statement blocks
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 31: boot_partition_status
Please see comment on patchset 44.
Done
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... PS59, Line 65: ;
Since the memset is removed now, you would need the initialization here: […]
Thanks..I missed it ..
-
Sridhar Siricilla (Code Review)