Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27369 )
Change subject: soc/intel/basecode: Add support for updating ucode loaded via FIT ......................................................................
Patch Set 51:
(8 comments)
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@52 PS49, Line 52: single MCU
Humm that is true. You would need multiple entries for each MCU.
Thanks Arthur.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 1: ramstage
Why is this being done so late? Why not just right after vboot selects slot and jumps to it?
Yes, that is the plan. Earlier version of the patchset was doing loading the new ucode to the CPU before deciding to update in staging area. This loading required that the NEM is disabled. Hence ramstage.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 33: __weak
Does this need to be weak? What is the recommendation on the type of reset required after microcode […]
It is recommended to do a cold reset after the ucode update.
Also, the SoC/Mainboard will be the eventual caller of the update API, my intention was to provide an opportunity to the caller to do any cleanup/housekeeping before reset by implementing this function.
But I guess this can be removed since the caller is aware that the system would reboot if an update is required. And, also I can avoid the risk of caller not implementing a cold reset.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 39: ucode_update_rec_mode_enabled
I don't understand the meaning of this function. […]
My intention was to make this update procedure generic and not tightly bound to to vboot. And the state of the system can be provided by the caller who could be using vboot or not.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 98: boot_device_wp_region
So, this doesn't care about the HW/SW WP state when doing the locking like we do for MRC cache?
Yes, do we need to check? we can apply the spi ctrlr protection anyway. Do we need to do this only when the overall write protection is applied?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 119: ucode_update_rec_mode_enabled
We should just use the APIs provided by vboot.
same reason as at line 139
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 199: /* TODO: Write Elog */
TODO for? Bug#?
Wanted to add a ELOG entry for failure case. This can also be done on the caller side.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 200: return
Shouldn't this reboot into recovery if the update failed?
I put the error handling in the caller side like https://review.coreboot.org/c/coreboot/+/33938/13/src/soc/intel/cannonlake/u...