Attention is currently required from: Sean Rhodes, SRIDHAR SIRICILLA, Furquan Shaikh, Paul Menzel, Rizwan Qureshi, Tim Wawrzynczak, Subrata Banik, Angel Pons, Sridhar Siricilla, Arthur Heymans, Patrick Rudolph. Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52800 )
Change subject: soc/intel: Allow enable/disable ME via CMOS ......................................................................
Patch Set 68:
(5 comments)
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/52800/comment/7e48a617_34fafbf2 PS63, Line 948: 0.0.0.0
It's useful here for debugging, as print_me_fw_version runs after the global_reset
How? On the first boot after Disabled -> Normal, you get the message it's going to enable the ME before it resets. The ME version here has no value.
CMOS: me_state = 0 ME: Version: 0.0.0.0 HECI: Sending command to enable HECI: Enable ME success! HECI: Global Reset(Type:1) Command POST: 0x00
On subsequent boots:
- print_me_fw_version() prints the version - dump_me_status() tells you it's enabled by CoM = 0
On the first boot after Normal -> Disabled, no version is printed before the reset, and on subsequent boots:
- print_me_fw_version() prints "Unavailable" - dump_me_status() tells you it's disabled by CoM = 3
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/52800/comment/d484fec5_f63bd3b9 PS68, Line 958: status = heci_send_receive(&msg, sizeof(msg), &reply, &reply_size, : HECI_MKHI_ADDR); : printk(BIOS_DEBUG, "HECI: Enable ME %s!\n", status ? "success" : "failure");
See L974
Result still needs to be checked to see if the reset is required. These functions should return a value to indicate if the message was successful.
https://review.coreboot.org/c/coreboot/+/52800/comment/ba25e00e_892a1e17 PS68, Line 1034: do_global_reset(); This should happen immediately after disable_me(), and only if it's successful.
https://review.coreboot.org/c/coreboot/+/52800/comment/6c1b44c4_76535948 PS68, Line 1036: printk(BIOS_DEBUG, "ME: Version: %u.%u.%u.%u\n", resp.code.major, : resp.code.minor, resp.code.hotfix, resp.code.build); This just duplicates print_me_fw_version(). There's no reason to have it in this function.
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/52800/comment/7b8eaaf8_59a815d7 PS68, Line 20: #define MKHI_SET_ME_DISABLE 0x3
See previous comments
Previous commit just says it's for SET ME DISABLE (which it is) because you originally had it as SET ME STATE. 0x3 is for *all* "SET <rule>" commands in FWCAPS.
-
Tim Crawford (Code Review)