Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatible issues. To avoid the data compatible issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during adowngrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla <sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 89 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/1
diff --git a/src/soc/intel/common/block/cse/custom_bp.c b/src/soc/intel/common/block/cse/custom_bp.c index 9aea0ee..379eb03 100644 --- a/src/soc/intel/common/block/cse/custom_bp.c +++ b/src/soc/intel/common/block/cse/custom_bp.c @@ -80,6 +80,11 @@ */ BP_STATUS_PARTITION_NOT_PRESENT = 2,
+ /* + * This value is returned when unexpected issues detected in CSE Data area + * and CSE TCB-SVN downgrade scenario. + */ + BP_STATUS_DATA_MISMATCH_ERROR = 3, };
/* @@ -449,7 +454,45 @@ cse_bp->end_offset); }
-static bool cse_prep_for_rw_update(struct cse_bp_info *cse_bp_info) +static bool cse_data_clear_request(struct cse_bp_info *cse_bp_info) +{ + struct data_clr_request { + struct mkhi_hdr hdr; + uint8_t reserved[4]; + } __packed; + + struct data_clr_request data_clr_rq = { + .hdr.group_id = MKHI_GROUP_ID_BUP_COMMON, + .hdr.command = MKHI_BUP_COMMON_DATA_CLEAR, + .reserved = {0}, + }; + + if (!cse_is_hfs1_cws_normal() || !cse_is_hfs1_com_soft_temp_disable() || + cse_get_current_bp(cse_bp_info) != RO) { + printk(BIOS_ERR, "cse_bp: CSE doesnn't meet DATA CLEAR cmd prerequisites\n"); + return false; + } + + printk(BIOS_DEBUG, "cse_bp: Sending DATA CLEAR HECI command\n"); + + struct mkhi_hdr data_clr_rsp; + size_t data_clr_rsp_sz = sizeof(data_clr_rsp); + + if (!heci_send_receive(&data_clr_rq, sizeof(data_clr_rq), &data_clr_rsp, + &data_clr_rsp_sz)) { + return false; + } + + if (data_clr_rsp.result) { + printk(BIOS_ERR, "cse_bp: CSE DATA CLEAR command response failed: %d\n", + data_clr_rsp.result); + return false; + } + + return true; +} + +static bool cse_prep_for_rw_update(struct cse_bp_info *cse_bp_info, uint32_t fw_downgrade) { /* * To set CSE's operation mode to HMRFPO mode: @@ -459,6 +502,11 @@ if (!cse_boot_to_ro(cse_bp_info)) return false;
+ if (fw_downgrade) { + if (!cse_data_clear_request(cse_bp_info)) + return false; + } + if (!cse_hmrfpo_enable()) return false;
@@ -514,9 +562,42 @@ return cse_cbfs_rw_ver->build - cse_rw_ver->build; }
+/* Checks RW Data partition is valid or not */ +static bool cse_is_rw_dp_valid(struct cse_bp_info *cse_bp_info) +{ + struct cse_bp_entry *rw_bp; + + rw_bp = cse_get_bp_entry(RW, cse_bp_info); + if (rw_bp->status == BP_STATUS_DATA_MISMATCH_ERROR) { + printk(BIOS_ERR, "cse_bp: RW data partition is not valid\n"); + return false; + } + return true; +} + +static bool cse_is_fw_downgrade(struct cse_bp_info *cse_bp_info, void *cse_cbfs_rw_n_ver) +{ + return cse_check_version_mismatch(cse_bp_info, cse_cbfs_rw_n_ver) < 0; +} + +/* Issues DATA_CLEAR HECI command and reset if RW boot partition indicates DATA_ERROR */ +static bool cse_fix_data_mismatch_err(struct cse_bp_info *cse_bp_info) +{ + if (cse_is_rw_dp_valid(cse_bp_info)) + return true; + + if (!cse_data_clear_request(cse_bp_info)) + return false; + + /* no return is exepected */ + cse_boot_to_rw(cse_bp_info, 1); + + /* Control never reaches here */ + return false; +} + static bool cse_erase_rw_region(struct region_device *target_rdev) { - if (rdev_eraseat(target_rdev, 0, region_device_sz(target_rdev)) < 0) { printk(BIOS_ERR, "cse_bp: CSE RW partition could not be erased\n"); return false; @@ -589,8 +670,9 @@ static bool cse_write_to_rw(struct cse_bp_info *cse_bp_info, struct region_device *target_rdev, void *cse_cbfs_rw_n_ver, size_t cse_cbfs_rw_n_ver_sz) { + uint32_t fw_downgrade = cse_is_fw_downgrade(cse_bp_info, cse_cbfs_rw_n_ver);
- if (!cse_prep_for_rw_update(cse_bp_info)) + if (!cse_prep_for_rw_update(cse_bp_info, fw_downgrade)) return false;
if (!cse_erase_rw_region(target_rdev)) @@ -638,8 +720,6 @@ return false; }
- rdev_munmap(source_rw_n_ver_rdev, cse_cbfs_rw_n_ver); - /* Global Reset is required as CSE firmware update is successful */ *rst_req = 1; return true; @@ -674,6 +754,9 @@ return; }
+ if (!cse_fix_data_mismatch_err(&cse_bp_info.bp_info)) + goto failed; + struct region_device target_rdev; struct region_device source_rw_n_ver_rdev;
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 6b91a4d..2367b68 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -41,7 +41,7 @@ /* Boot partition info and set boot partition info command ids */ #define MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO 0x1c #define MKHI_BUP_COMMON_SET_BOOT_PARTITION_INFO 0x1d - +#define MKHI_BUP_COMMON_DATA_CLEAR 0x20 /* ME Current Working States */ #define ME_HFS1_CWS_NORMAL 0x5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40561/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/1/src/soc/intel/common/block/... PS1, Line 592: /* no return is exepected */ 'exepected' may be misspelled - perhaps 'expected'?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40561/2/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/2/src/soc/intel/common/block/... PS2, Line 592: /* no return is exepected */ 'exepected' may be misspelled - perhaps 'expected'?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40561/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/3/src/soc/intel/common/block/... PS3, Line 592: /* no return is exepected */ 'exepected' may be misspelled - perhaps 'expected'?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Subrata Banik, Angel Pons, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#4).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatible issues. To avoid the data compatible issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during adowngrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla <sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 89 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 5: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@10 PS5, Line 10: compatible compatibility
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@11 PS5, Line 11: the data compatible issues such issues
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@13 PS5, Line 13: adowngrade missing space
https://review.coreboot.org/c/coreboot/+/40561/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/5/src/soc/intel/common/block/... PS5, Line 472: nn remove one `n`
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#6).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla <sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 90 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/6
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@10 PS5, Line 10: compatible
compatibility
Done
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@11 PS5, Line 11: the data compatible issues
such issues
Done
https://review.coreboot.org/c/coreboot/+/40561/5//COMMIT_MSG@13 PS5, Line 13: adowngrade
missing space
Done
https://review.coreboot.org/c/coreboot/+/40561/5/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/5/src/soc/intel/common/block/... PS5, Line 472: nn
remove one `n`
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#7).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 91 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/7
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#8).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 87 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/8
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#9).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 93 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/9
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#11).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/custom_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 93 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/11
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 12:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 93: detected are detected
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 543: /* Check if CSE reports data mismatch error */ That’s the description for the next function?
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 556: triggers trigger (to be in line with *issue*)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false; Add an error message with `die()`?
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 667: return false; An error message should be returned here, saying that the downgrade is aborted.
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 729: /* If RW blob is present in CBFS, then trigger CSE firmware update */ Remove one space after tab?
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 731: uint8_t rv; Unnecessary reordering in change.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#13).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 92 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/13
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 13:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 93: detected
are detected
Done
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 543: /* Check if CSE reports data mismatch error */
That’s the description for the next function?
No, the comment is for cse_is_rw_dp_valid().
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 556: triggers
trigger (to be in line with *issue*)
Done
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false;
Add an error message with `die()`?
if this function fails, recovery mode is triggered in the error path.
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 667: return false;
An error message should be returned here, saying that the downgrade is aborted.
Done
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 729: /* If RW blob is present in CBFS, then trigger CSE firmware update */
Remove one space after tab?
Done
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 731: uint8_t rv;
Unnecessary reordering in change.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 543: /* Check if CSE reports data mismatch error */
No, the comment is for cse_is_rw_dp_valid().
Sorry, with the naming of the next function, why not name it `cse_check_data_mismatch_error`?
The define is has *failure* instead of *mismatch*. The wording should be consistent.
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false;
if this function fails, recovery mode is triggered in the error path.
1. Can dead_code() be used here somehow? 2. I still think, an error message needs to be printed, if unreachable code is executed.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false;
- Can dead_code() be used here somehow? […]
cse_boot_to_rw() should be marked as noreturn for dead_code() to work though
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#15).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 88 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/15
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 93: detected
Done
Done
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 543: /* Check if CSE reports data mismatch error */
Sorry, with the naming of the next function, why not name it `cse_check_data_mismatch_error`? […]
I fixed it,
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false;
cse_boot_to_rw() should be marked as noreturn for dead_code() to work though
Return value of cse_boot_to_rw() function must be passed to caller.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 15: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 15: -Code-Review
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/40561/12/src/soc/intel/common/block... PS12, Line 569: return false;
Return value of cse_boot_to_rw() function must be passed to caller.
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Tim Wawrzynczak, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#16).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware. When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on preventive basis during a downgrade and when CSE indicate the data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 88 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/16
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 19: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 22:
(11 comments)
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@10 PS22, Line 10: When CSE FW is downgraded, CSE may get into data compatibility issues. Please add a blank line above (between paragraphs).
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@12 PS22, Line 12: on preventive basis Isn’t *To avoid such issues* already conveying this?
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: the a
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: indicate indicates
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n"); Error messages have to be user understandable. Please elaborate, what the consequence is.
… CSE data is not cleared, and this causes …
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 548: printk(BIOS_ERR, "cse_lite: RW data partition is not valid\n"); The error messages should be moved to the caller, so the consequences can be added to the message.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err What error is fixed? Isn’t `heci_clear_data()` or something similar a better name?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info)) Please move the check to the caller, as from the function name, it does not belong here.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 561: return false; The error message should be printed here, and not in the function.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade Can you please improve the name?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
CSE data couldn’t be cleared, so abort CSE FW downgrade from … to …. Please check … and try again.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 22:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n");
Error messages have to be user understandable. Please elaborate, what the consequence is. […]
I'd say this is clear enough as is. The DATA CLEAR command has some prerequisites, and if they are not met, it can't be executed. And what happens next? Nothing special, the command didn't run. The caller could try again (and probably get the same error), skip this, or give up.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 548: printk(BIOS_ERR, "cse_lite: RW data partition is not valid\n");
The error messages should be moved to the caller, so the consequences can be added to the message.
As this is an "is" function, I'd prefer if it just returned a boolean and nothing else. This lets us have a single return statement:
return rw_bp->status != BP_STATUS_DATA_FAILURE;
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: bool What does the returned value indicate? It's not obvious at first glance.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
What error is fixed? Isn’t `heci_clear_data()` or something similar a better name?
Let's try... Ew, `cse_clear_data_if_invalid_then_boot_to_rw()` might be accurate, but it's a horrible name.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info))
Please move the check to the caller, as from the function name, it does not belong here.
Well, one can't fix things when they aren't broken.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade
Can you please improve the name?
Suggestions?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
CSE data couldn’t be cleared, so abort CSE FW downgrade from … to …. […]
I'd use something like this:
cse_lite: Could not perform CSE FW downgrade
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 22:
(6 comments)
As the file name is `cse_lite.c`, I’d remove `cse_` from the static function names.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n");
I'd say this is clear enough as is. […]
How can you assume, that it returns or continues? You can not, so it’s better to state it explicitly.
How would currently the error level messages look like? Will several messages be printed for the same issue?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: prerequisites Mentioning the prerequisites would be useful.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
Let's try... […]
Yes, the invalid check should be moved out.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info))
Well, one can't fix things when they aren't broken.
Yes, the caller should take care of that.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade
Suggestions?
I couldn’t come up with a good one yet. But two variables are passed, so from the name, the ordering of both is not clear to me for example.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
I'd use something like this: […]
And the user asks: “Why and what now?”.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Tim Wawrzynczak, Rizwan Qureshi, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#23).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware.
When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on proactive basis during a downgrade and when CSE indicates a data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 93 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/23
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 23:
(13 comments)
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@10 PS22, Line 10: When CSE FW is downgraded, CSE may get into data compatibility issues.
Please add a blank line above (between paragraphs).
Done
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@12 PS22, Line 12: on preventive basis
Isn’t *To avoid such issues* already conveying this?
Done
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: indicate
indicates
Ack
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: the
a
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n");
How can you assume, that it returns or continues? You can not, so it’s better to state it explicitly […]
If the command fails, coreboot logs FW status registers and triggers ChromeOS recovery. The FW status registers help decode CSE's current working state(CWS) and current operation mode(COM).Also, the log trail indicates CSE's current boot partition. So, I think current error log seems to be ok.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: prerequisites
Mentioning the prerequisites would be useful.
Please see my response above.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 548: printk(BIOS_ERR, "cse_lite: RW data partition is not valid\n");
As this is an "is" function, I'd prefer if it just returned a boolean and nothing else. […]
Ack
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: bool
What does the returned value indicate? It's not obvious at first glance.
Ack
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
Yes, the invalid check should be moved out.
I wanted to keep top level function abstracted, so I moved the if condition inside the function. I have added documentation to give more clarity. Let me know your comments.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info))
Yes, the caller should take care of that.
Please see my response above
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 561: return false;
The error message should be printed here, and not in the function.
I have added comment to provide clarity.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade
I couldn’t come up with a good one yet. […]
Ack
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
And the user asks: “Why and what now?”.
The chain of log message indicate CSE FW update is downgrade (one version to other) and abort of CSE FW downgrade action due to data clear command if it fails. So, I think "CSE FW downgrade is aborted" is sufficient.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 32:
(6 comments)
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: prerequisites
Please see my response above.
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n");
If the command fails, coreboot logs FW status registers and triggers ChromeOS recovery. […]
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
I wanted to keep top level function abstracted, so I moved the if condition inside the function. […]
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info))
Please see my response above
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 561: return false;
I have added comment to provide clarity.
Done
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
The chain of log message indicate CSE FW update is downgrade (one version to other) and abort of CSE […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 32: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 32:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... PS32, Line 665: if (cse_is_downgrade_instance(cse_bp_info, source_rdev)) { : if (!cse_data_clear_request(cse_bp_info)) suggestion: `if (cse_is_downgrade_instance(cse_bp_info, source_rdev) && !cse_data_clear_request(cse_bp_info))`
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... PS32, Line 30: #define MKHI_BUP_COMMON_DATA_CLEAR 0x20 nit: line 0x20 up with the previous lines
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Sridhar Siricilla, Tim Wawrzynczak, Rizwan Qureshi, Angel Pons, Subrata Banik, Balaji Manigandan, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40561
to look at the new patch set (#33).
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware.
When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on proactive basis during a downgrade and when CSE indicates a data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 92 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/40561/33
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 33:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... PS32, Line 665: if (cse_is_downgrade_instance(cse_bp_info, source_rdev)) { : if (!cse_data_clear_request(cse_bp_info))
suggestion: `if (cse_is_downgrade_instance(cse_bp_info, source_rdev) && !cse_data_clear_request(cse_ […]
Ack
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/40561/32/src/soc/intel/common/block... PS32, Line 30: #define MKHI_BUP_COMMON_DATA_CLEAR 0x20
nit: line 0x20 up with the previous lines
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 33: Code-Review+1
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 36: Code-Review+2
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 36: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
soc/intel/common: Add downgrade support for CSE Firmware
Add downgrade support for CSE RW firmware.
When CSE FW is downgraded, CSE may get into data compatibility issues. To avoid such issues, coreboot sends DATA CLEAR HECI command to CSE to clear CSE run time data on proactive basis during a downgrade and when CSE indicates a data mismatch error through GET_BOOT_PARTITION_INFO.
BUG=b:144894771 TEST=Verified on hatch
Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I0a3a3036e448e5a743398f6b27e8e62965dbff3c Reviewed-on: https://review.coreboot.org/c/coreboot/+/40561 Reviewed-by: V Sowmya v.sowmya@intel.com Reviewed-by: Jamie Ryu jamie.m.ryu@intel.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/cse/cse_lite.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 92 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified V Sowmya: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve Jamie Ryu: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c index a8948be..edb08da 100644 --- a/src/soc/intel/common/block/cse/cse_lite.c +++ b/src/soc/intel/common/block/cse/cse_lite.c @@ -84,6 +84,11 @@ /* This value is returned when a partition is not present per initial image layout */ BP_STATUS_PARTITION_NOT_PRESENT = 2,
+ /* + * This value is returned when unexpected issues are detected in CSE Data area + * and CSE TCB-SVN downgrade scenario. + */ + BP_STATUS_DATA_FAILURE = 3, };
/* @@ -459,6 +464,44 @@ return true; }
+static bool cse_data_clear_request(const struct cse_bp_info *cse_bp_info) +{ + struct data_clr_request { + struct mkhi_hdr hdr; + uint8_t reserved[4]; + } __packed; + + struct data_clr_request data_clr_rq = { + .hdr.group_id = MKHI_GROUP_ID_BUP_COMMON, + .hdr.command = MKHI_BUP_COMMON_DATA_CLEAR, + .reserved = {0}, + }; + + if (!cse_is_hfs1_cws_normal() || !cse_is_hfs1_com_soft_temp_disable() || + cse_get_current_bp(cse_bp_info) != RO) { + printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n"); + return false; + } + + printk(BIOS_DEBUG, "cse_lite: Sending DATA CLEAR HECI command\n"); + + struct mkhi_hdr data_clr_rsp; + size_t data_clr_rsp_sz = sizeof(data_clr_rsp); + + if (!heci_send_receive(&data_clr_rq, sizeof(data_clr_rq), &data_clr_rsp, + &data_clr_rsp_sz)) { + return false; + } + + if (data_clr_rsp.result) { + printk(BIOS_ERR, "cse_lite: CSE DATA CLEAR command response failed: %d\n", + data_clr_rsp.result); + return false; + } + + return true; +} + static bool cse_get_cbfs_rw_version(const struct region_device *source_rdev, void *cse_cbfs_rw_ver) { @@ -504,9 +547,38 @@ return cse_cbfs_rw_ver.build - cse_rw_ver->build; }
+/* Check if CSE RW data partition is valid or not */ +static bool cse_is_rw_dp_valid(const struct cse_bp_info *cse_bp_info) +{ + const struct cse_bp_entry *rw_bp; + + rw_bp = cse_get_bp_entry(RW, cse_bp_info); + return rw_bp->status != BP_STATUS_DATA_FAILURE; +} + +/* + * It returns true if RW partition doesn't indicate BP_STATUS_DATA_FAILURE + * otherwise false if any operation fails. + */ +static bool cse_fix_data_failure_err(const struct cse_bp_info *cse_bp_info) +{ + /* + * If RW partition status indicates BP_STATUS_DATA_FAILURE, + * - Send DATA CLEAR HECI command to CSE + * - Send SET BOOT PARTITION INFO(RW) command to set CSE's next partition + * - Issue GLOBAL RESET HECI command. + */ + if (cse_is_rw_dp_valid(cse_bp_info)) + return true; + + if (!cse_data_clear_request(cse_bp_info)) + return false; + + return cse_boot_to_rw(cse_bp_info); +} + static bool cse_erase_rw_region(const struct region_device *target_rdev) { - if (rdev_eraseat(target_rdev, 0, region_device_sz(target_rdev)) < 0) { printk(BIOS_ERR, "cse_lite: CSE RW partition could not be erased\n"); return false; @@ -531,6 +603,12 @@ return !cse_check_version_mismatch(cse_bp_info, source_rdev); }
+static bool cse_is_downgrade_instance(const struct cse_bp_info *cse_bp_info, + const struct region_device *source_rdev) +{ + return cse_check_version_mismatch(cse_bp_info, source_rdev) < 0; +} + static bool cse_is_update_required(const struct cse_bp_info *cse_bp_info, const struct region_device *source_rdev, struct region_device *target_rdev) { @@ -581,7 +659,8 @@ return true; }
-static bool cse_prep_for_rw_update(const struct cse_bp_info *cse_bp_info) +static bool cse_prep_for_rw_update(const struct cse_bp_info *cse_bp_info, + const struct region_device *source_rdev) { /* * To set CSE's operation mode to HMRFPO mode: @@ -591,13 +670,19 @@ if (!cse_boot_to_ro(cse_bp_info)) return false;
+ if (cse_is_downgrade_instance(cse_bp_info, source_rdev) && + !cse_data_clear_request(cse_bp_info)) { + printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n"); + return false; + } + return cse_hmrfpo_enable(); }
static uint8_t cse_trigger_fw_update(const struct cse_bp_info *cse_bp_info, const struct region_device *source_rdev, struct region_device *target_rdev) { - if (!cse_prep_for_rw_update(cse_bp_info)) + if (!cse_prep_for_rw_update(cse_bp_info, source_rdev)) return CSE_LITE_SKU_COMMUNICATION_ERROR;
if (!cse_update_rw(cse_bp_info, source_rdev, target_rdev)) @@ -647,6 +732,9 @@ cse_trigger_recovery(CSE_LITE_SKU_COMMUNICATION_ERROR); }
+ if (!cse_fix_data_failure_err(&cse_bp_info.bp_info)) + cse_trigger_recovery(CSE_LITE_SKU_DATA_WIPE_ERROR); + /* If RW blob is present in CBFS, then trigger CSE firmware update */ uint8_t rv; struct region_device source_rdev; diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 5466ba6..a67010c 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -27,6 +27,7 @@ /* Boot partition info and set boot partition info command ids */ #define MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO 0x1c #define MKHI_BUP_COMMON_SET_BOOT_PARTITION_INFO 0x1d +#define MKHI_BUP_COMMON_DATA_CLEAR 0x20
/* ME Current Working States */ #define ME_HFS1_CWS_NORMAL 0x5
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Jamie Ryu (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Sridhar Siricilla (Code Review)
-
Tim Wawrzynczak (Code Review)
-
V Sowmya (Code Review)