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 33:
(2 comments)
Patch Set 33:
The more I'm thinking about this the more I realize that I don't understand the purpose of this update mechanism.
For being able to perform the update, you already need a RO MCU that works good enough to get you to ramstage. At this point, you can already apply additional MCUs from any RW partition. So what kind of issue would have to be fixed by an update that makes use of this mechanism?
In other words, what problem can this new update mechanism fix, that current mechanisms can't? And is it worth the added complexity and accompanying security degradation (more code is always more error- prone)?
Microcode patch contains patch for Punit as well, and that has to be applied prior to reset.
Ah, understood. Thanks for the clarification.
Ofc, it's too late for current products. But is there any change planned for the FIT update mechanism. e.g. a rule that multiple updates for one processor signature are allowed and the newest would be applied. That would make this much easier, would allow a single RO FIT with some entries pointing to RO and some to the MCU RW and we wouldn't need the top-swap feature any more.
If you haven't tried, add multiple microcode patches in FIT for the same processor signature, you will find that FIT will pick the latest revision and load. However there is no fallback if there is a failure.
2 FITs, 1 for normal boot, another for recovery boot. Even if the CPU manages to load the latest revision (from the normal boot FIT) and that results in a hang/reboot loop. We need a tried and tested patch to recover from the hang/reboot for which the top-swap FIT will be used.
Hence the effort to load the updated microcode via FIT.
This is probably the most urgent thing to mention in the commit message. Without this reasoning, it looks like you are just playing around and Intel adds complexity just because they can.
Ok. will update the commit message.
https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... File src/soc/intel/common/basecode/fw_update/ucode_update.c:
https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... PS30, Line 103: static int protect_staging(void)
should be called in BS_POST_DEVICE
Claim an FPR and set the range as soon as you get an opportunity. Do you see a problem?
https://review.coreboot.org/#/c/27369/30/src/soc/intel/common/basecode/fw_up... PS30, Line 118: check_and_update_ucode
I think it would be good to check the FIT table itself too?
Because the FIT table will be in RO. I don't think we need to check that.