Change in coreboot[master]: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Hello Rizwan Qureshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to review the following change.
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 139 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/1
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 9585a09..d85209b 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -76,6 +76,15 @@ /* RST Origin */ #define GR_ORIGIN_BIOS_POST 2
+#define MKHI_HMRFPO_GROUP_ID 5 + +/* HMRFPO Command Ids */ +#define MKHI_HMRFPO_ENABLE 1 +#define MKHI_HMRFPO_GET_STATUS 3 + +#define ME_HFS_CWS_NORMAL 5 +#define ME_HFS_MODE_NORMAL 0 + static struct cse_device { uintptr_t sec_bar; } g_cse; @@ -104,6 +113,16 @@ } __packed fields; };
+ +struct mkhi_hdr { + uint8_t group_id; + uint8_t command:7; + uint8_t is_resp:1; + uint8_t rsvd; + uint8_t result; +}__packed; + + /* * Initialize the device with provided temporary BAR. If BAR is 0 use a * default. This is intended for pre-mem usage only where BARs haven't been @@ -652,6 +671,109 @@ return 0; }
+/* + * Sends HMRFPO Enable command to CSE + */ +int send_hmrfpo_enable_msg(void) +{ + + struct hmrfpo_enable_msg { + struct mkhi_hdr hdr; + uint32_t nonce[2]; + }__packed; + + /* HMRFPO Enable message */ + struct hmrfpo_enable_msg msg = { + .hdr = { + .group_id = MKHI_HMRFPO_GROUP_ID, + .command = MKHI_HMRFPO_ENABLE, + }, + .nonce = {0}, + }; + + /* HMRFPO Enable response */ + struct hmrfpo_enable_resp { + struct mkhi_hdr hdr; + uint32_t fct_base; + uint32_t fct_limit; + uint8_t status; + uint8_t padding[3]; + }__packed; + + struct hmrfpo_enable_resp resp; + size_t resp_size = sizeof(struct hmrfpo_enable_resp); + union me_hfs hfs; + + printk(BIOS_DEBUG, "Send HMRFPO Enable Command\n"); + hfs.data = me_read_config32(PCI_ME_HFSTS1); + /* + * This command can be run only if: + * - Working state is normal and + * - Operation mode is normal. + */ + if ((hfs.fields.working_state != ME_HFS_CWS_NORMAL) || + (hfs.fields.operation_mode != ME_HFS_MODE_NORMAL)) { + printk(BIOS_ERR, "ME not in normal Mode\n"); + goto failed; + } + + if (!heci_send_receive(&msg, sizeof(struct hmrfpo_enable_msg), + &resp, &resp_size)) + goto failed; + + if (resp.hdr.result) { + printk(BIOS_ERR, "Heci resp Failed:%d\n", resp.hdr.result); + goto failed; + } + return 1; + +failed: + return 0; +} + +/* + * Sends HMRFPO Get Status command to CSE to get the HMRFPO status. + * The status can be DISABLES/LOCKED/ENABLED + */ +int send_hmrfpo_get_status_msg(void) +{ + struct hmrfpo_get_status_msg { + struct mkhi_hdr hdr; + }__packed; + + struct hmrfpo_get_status_resp { + struct mkhi_hdr hdr; + uint8_t status; + uint8_t padding[3]; + }__packed; + + /* HMRFPO STATUS message */ + struct hmrfpo_get_status_msg msg = { + .hdr = { + .group_id = MKHI_HMRFPO_GROUP_ID, + .command = MKHI_HMRFPO_GET_STATUS, + }, + }; + struct hmrfpo_get_status_resp resp; + size_t resp_size = sizeof(struct hmrfpo_get_status_resp); + + printk(BIOS_ERR, "HMRFPO Status Command\n"); + + if (!heci_send_receive(&msg, sizeof(struct hmrfpo_get_status_msg), + &resp, &resp_size)) + goto failed; + + if (resp.hdr.result) { + printk(BIOS_ERR, "Heci resp Failed:%d\n", resp.hdr.result); + goto failed; + } + + return resp.status; + +failed: + return -1; +} + #if ENV_RAMSTAGE
static void update_sec_bar(struct device *dev) diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index ceeca88e..c0a5fef 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -77,6 +77,19 @@ */ int send_heci_reset_req_message(u8 rst_type);
+/* + * Send HMRFPO_ENABLE command. + * returns 0 on failure and 1 on success. + */ +int send_hmrfpo_enable_msg(void); + +/* + * Send HMRFPO_GET_STATUS command. + * returns -1 on failure and 0 (DISABLED) or 1 (LOCKED) or 2 (ENABLED). + */ +int send_hmrfpo_get_status_msg(void); + + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
@@ -96,5 +109,9 @@ #define HOST_RESET_ONLY 2 #define CSE_RESET_ONLY 3
+/*HMRFPO Status types */ +#define MKHI_HMRFPO_DISABLED 0 +#define MKHI_HMRFPO_LOCKED 1 +#define MKHI_HMRFPO_ENABLED 2
#endif // SOC_INTEL_COMMON_MSR_H
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#2).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
* Fix all comments on indentation/spacing issues
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/2//COMMIT_MSG@13 PS2, Line 13: * Fix all comments on indentation/spacing issues Not required
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#3).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/3
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#8).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/8
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#9).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/9
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#10).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/10
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35229/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/2//COMMIT_MSG@13 PS2, Line 13: * Fix all comments on indentation/spacing issues
Not required
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#13).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/13
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#14).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 142 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/14
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... PS14, Line 595: u8 group_id; : u8 command; : u8 reserved; : u8 result; can be replaced by mkhi_hdr
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... PS14, Line 601: u8 group_id; : u8 cmd; : u8 reserved; : u8 result; : u8 req_origin; can be replaced by mkhi_hdr
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#15).
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib
Below new function are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 132 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/15
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... PS14, Line 595: u8 group_id; : u8 command; : u8 reserved; : u8 result;
can be replaced by mkhi_hdr
Done
https://review.coreboot.org/c/coreboot/+/35229/14/src/soc/intel/common/block... PS14, Line 601: u8 group_id; : u8 cmd; : u8 reserved; : u8 result; : u8 req_origin;
can be replaced by mkhi_hdr
Done
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 16:
This change is ready for review.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/inte/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 16:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG@7 PS16, Line 7: inte intel
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG@9 PS16, Line 9: function functions
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 632: /* : * Sends HMRFPO Enable command to CSE : */ nit: multiline comment not needed.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 639: uint32_t nonce[2]; used for? Can we have description on the fields?
uint32_t nonce[2]; /* <Used for/purpose/details> */
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 654: uint32_t fct_base; : uint32_t fct_limit; same.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 660: struct hmrfpo_enable_resp resp; define variables at start of the function
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 661: size_t resp_size same.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 662: union me_hfsts1 hfs1; same
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 669: * - Operation mode is normal or : * temporary disable mode. indentation.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 672: ( nit: braces can go.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 715: struct hmrfpo_get_status_resp resp; : size_t resp_size define variables at start of function?
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#17).
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib
Below new functions are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 129 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/17
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 17:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 632: /* : * Sends HMRFPO Enable command to CSE : */
nit: multiline comment not needed.
Done. thanks
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 639: uint32_t nonce[2];
used for? Can we have description on the fields? […]
In client platforms, those are reserved fields.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 654: uint32_t fct_base; : uint32_t fct_limit;
same.
Those are reserved fields in the client platforms.
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 660: struct hmrfpo_enable_resp resp;
define variables at start of the function
It must be here only due to structure "hmrfpo_enable_resp" declatration
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 661: size_t resp_size
same.
Not required!
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 662: union me_hfsts1 hfs1;
same
Not required!
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 669: * - Operation mode is normal or : * temporary disable mode.
indentation.
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 672: (
nit: braces can go.
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 715: struct hmrfpo_get_status_resp resp; : size_t resp_size
define variables at start of function?
Not required
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG@7 PS16, Line 7: inte
intel
Done
https://review.coreboot.org/c/coreboot/+/35229/16//COMMIT_MSG@9 PS16, Line 9: function
functions
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 639: uint32_t nonce[2];
In client platforms, those are reserved fields.
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 654: uint32_t fct_base; : uint32_t fct_limit;
Those are reserved fields in the client platforms.
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 660: struct hmrfpo_enable_resp resp;
It must be here only due to structure "hmrfpo_enable_resp" declatration
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 661: size_t resp_size
Not required!
Done
https://review.coreboot.org/c/coreboot/+/35229/16/src/soc/intel/common/block... PS16, Line 662: union me_hfsts1 hfs1;
Not required!
Done
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 17: Code-Review+2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35229/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/17//COMMIT_MSG@9 PS17, Line 9: Below new functions are added: : * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE : * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE What is HMRFPO? a little description as why we need this would be good.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35229
to look at the new patch set (#18).
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib
Below new functions are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE. This API sets ME in SEC_OVERRIDE mode. The mode prevents CSE to execute SPI I/O cycles to CSE region, and unlocks the CSE region to perfom updates to it. * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 129 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/35229/18
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35229/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35229/17//COMMIT_MSG@9 PS17, Line 9: Below new functions are added: : * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE : * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
What is HMRFPO? a little description as why we need this would be good.
Done
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib
Below new functions are added: * send_hmrfpo_enable_msg() - Sends HMRFPO Enable command to CSE. This API sets ME in SEC_OVERRIDE mode. The mode prevents CSE to execute SPI I/O cycles to CSE region, and unlocks the CSE region to perfom updates to it. * send_hmrfpo_get_status_msg() - Sends HMRFPO Get Status command to CSE
TEST=Verified sending HMRFPO_ENABLE & HMRFPO_GET_STATUS HECI commands on CML RVP & hatch board
Change-Id: I559bc4641e12df7ed39b1c97097bf068f9a232db Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35229 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/include/intelblocks/cse.h 2 files changed, 129 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 01b2050..debbf1f 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -76,6 +76,16 @@ /* RST Origin */ #define GR_ORIGIN_BIOS_POST 2
+#define MKHI_HMRFPO_GROUP_ID 5 + +/* HMRFPO Command Ids */ +#define MKHI_HMRFPO_ENABLE 1 +#define MKHI_HMRFPO_GET_STATUS 3 + +#define ME_HFS_CWS_NORMAL 5 +#define ME_HFS_MODE_NORMAL 0 +#define ME_HFS_TEMP_DISABLE 3 + static struct cse_device { uintptr_t sec_bar; } g_cse; @@ -619,6 +629,106 @@ return 0; }
+/* Sends HMRFPO Enable command to CSE */ +int send_hmrfpo_enable_msg(void) +{ + struct hmrfpo_enable_msg { + struct mkhi_hdr hdr; + uint32_t nonce[2]; + } __packed; + + /* HMRFPO Enable message */ + struct hmrfpo_enable_msg msg = { + .hdr = { + .group_id = MKHI_HMRFPO_GROUP_ID, + .command = MKHI_HMRFPO_ENABLE, + }, + .nonce = {0}, + }; + + /* HMRFPO Enable response */ + struct hmrfpo_enable_resp { + struct mkhi_hdr hdr; + uint32_t fct_base; + uint32_t fct_limit; + uint8_t status; + uint8_t padding[3]; + } __packed; + + struct hmrfpo_enable_resp resp; + size_t resp_size = sizeof(struct hmrfpo_enable_resp); + union me_hfsts1 hfs1; + + printk(BIOS_DEBUG, "HECI: Send HMRFPO Enable Command\n"); + hfs1.data = me_read_config32(PCI_ME_HFSTS1); + /* + * This command can be run only if: + * - Working state is normal and + * - Operation mode is normal or temporary disable mode. + */ + if (hfs1.fields.working_state != ME_HFS_CWS_NORMAL || + (hfs1.fields.operation_mode != ME_HFS_MODE_NORMAL && + hfs1.fields.operation_mode != ME_HFS_TEMP_DISABLE)) { + printk(BIOS_ERR, "HECI: ME not in required Mode\n"); + goto failed; + } + + if (!heci_send_receive(&msg, sizeof(struct hmrfpo_enable_msg), + &resp, &resp_size)) + goto failed; + + if (resp.hdr.result) { + printk(BIOS_ERR, "HECI: Resp Failed:%d\n", resp.hdr.result); + goto failed; + } + return 1; + +failed: + return 0; +} + +/* + * Sends HMRFPO Get Status command to CSE to get the HMRFPO status. + * The status can be DISABLES/LOCKED/ENABLED + */ +int send_hmrfpo_get_status_msg(void) +{ + struct hmrfpo_get_status_msg { + struct mkhi_hdr hdr; + } __packed; + + struct hmrfpo_get_status_resp { + struct mkhi_hdr hdr; + uint8_t status; + uint8_t padding[3]; + } __packed; + + struct hmrfpo_get_status_msg msg = { + .hdr = { + .group_id = MKHI_HMRFPO_GROUP_ID, + .command = MKHI_HMRFPO_GET_STATUS, + }, + }; + struct hmrfpo_get_status_resp resp; + size_t resp_size = sizeof(struct hmrfpo_get_status_resp); + + printk(BIOS_INFO, "HECI: Sending Get HMRFPO Status Command\n"); + + if (!heci_send_receive(&msg, sizeof(struct hmrfpo_get_status_msg), + &resp, &resp_size)) { + printk(BIOS_ERR, "HECI: HMRFPO send/receive fail\n"); + return -1; + } + + if (resp.hdr.result) { + printk(BIOS_ERR, "HECI: HMRFPO Resp Failed:%d\n", + resp.hdr.result); + return -1; + } + + return resp.status; +} + #if ENV_RAMSTAGE
static void update_sec_bar(struct device *dev) diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 1b08b4d..378f417 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -113,6 +113,20 @@ */ int send_heci_reset_req_message(uint8_t rst_type);
+/* + * Send HMRFPO_ENABLE command. + * returns 0 on failure and 1 on success. + */ +int send_hmrfpo_enable_msg(void); + +/* + * Send HMRFPO_GET_STATUS command. + * returns -1 on failure and 0 (DISABLED)/ 1 (LOCKED)/ 2 (ENABLED) + * on success. + */ +int send_hmrfpo_get_status_msg(void); + + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
@@ -121,4 +135,9 @@ #define HOST_RESET_ONLY 2 #define CSE_RESET_ONLY 3
+/*HMRFPO Status types */ +#define MKHI_HMRFPO_DISABLED 0 +#define MKHI_HMRFPO_LOCKED 1 +#define MKHI_HMRFPO_ENABLED 2 + #endif // SOC_INTEL_COMMON_MSR_H
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 19:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 79: MKHI_HMRFPO_GROUP_ID Can you please make the names more consistent. Line 74 and line 79 use very different ways of defining these constants. Also, I would like to see related things grouped together appropriately.
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 652: uint32_t fct_base; : uint32_t fct_limit; : uint8_t status; What are these fields?
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 683: } Don't you care about the status?
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 117: Send HMRFPO_ENABLE command. This should have a better explanation of what this command really means.
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 118: failure Does failure mean failure to send message or failure to enable?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35229 )
Change subject: src/soc/intel/common/block/cse: Add hmrfpo related functions to cse lib ......................................................................
Patch Set 19:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 79: MKHI_HMRFPO_GROUP_ID
Can you please make the names more consistent. […]
Addressed as part of patch- https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 652: uint32_t fct_base; : uint32_t fct_limit; : uint8_t status;
What are these fields?
fct_base/fct_limit are not applicable to client platforms. These fields refers to factory data area. Status field is a value returned by CSE for HMRFPO_ENABLE command. Status Value 0 indicates enabling HMRFPO success, and non-zero value indicates error.
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 683: }
Don't you care about the status?
status check is implemented in the patch: https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 117: Send HMRFPO_ENABLE command.
This should have a better explanation of what this command really means.
We have added more information as part of patch:https://review.coreboot.org/c/coreboot/+/35546
https://review.coreboot.org/c/coreboot/+/35229/19/src/soc/intel/common/block... PS19, Line 118: failure
Does failure mean failure to send message or failure to enable?
It indicates failure to send command and to enable hmrfpo mode. I updated the comment to reflect the same as part of patch: https://review.coreboot.org/c/coreboot/+/35546
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)