Change in coreboot[master]: soc/intel/basecode: Add support for updating ucode loaded via FIT
Furquan Shaikh 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 49:
(43 comments)
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@11 PS49, Line 11: Chromebooks nit: Chromebooks,
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@12 PS49, Line 12: reset nit: reset,
https://review.coreboot.org/c/coreboot/+/27369/49//COMMIT_MSG@52 PS49, Line 52: single MCU I believe microcode_blob.bin can be generated by stitching multiple MCUs together. Wouldn't that still work fine?
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... File Documentation/soc/intel/ucode_update/microcode_update_model.md:
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 8: . It would be good to mention here that MCU refers to the bundled package containing ucode and pcode.
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 42: bot boot
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 43: block
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 67: . and in case of RTC reset.
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 74: lke like
https://review.coreboot.org/c/coreboot/+/27369/49/Documentation/soc/intel/uc... PS49, Line 94: reset cold or warm?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Kconfig:
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 1: config TOP_SWAP_BASED_VBOOT_UCODE_UPDATE Can these configs be named with a specific prefix? INTEL_BASECODE_ or just INTEL_? Something like INTEL_MULTI_FIT_MCU? or INTEL_TS_MCU? or INTEL_TOP_SWAP_MCU?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 6: select Don't you also need to auto select "CONFIG_SUPPORT_CPU_UCODE_IN_CBFS"
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 6: select SOC_INTEL_COMMON_BLOCK_RTC Help text would be very helpful for these configs.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 8: INTEL_TOP_SWAP_FIT_ENTRY_FMAP_REG I don't understand what this name really means. Shouldn't this just be: INTEL_MULTI_FIT_MCU_RW_FMAP_NAME or INTEL_TOP_SWAP_MCU_RW_FMAP_NAME?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 12: Again, help text should be added for this.
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?
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 16: symbols What is this used for?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 17: assert What is this used for?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 27: spi_flash.h What is this used for?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 28: string Is this required?
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 update to staging area?
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. Is there a reason why this does not just call into vboot APIs to identify whether boot mode is recovery?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 50: int It would be good to add a comment indicating the return values and what they mean.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 58: printk(BIOS_ERR, "Error locating staging area!\n"); Why is only this particular failure error message logged and not the other failures in this function?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 68: incoherent_rdev_init Why not just return what incoherent_rdev_init() is returning? i.e. pointer to rdev or NULL.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 75: extra blank line
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 82: Could could
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 88: (size_t) I don't think this is required.
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?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 99: return -1; Why not just return whatever boot_device_wp_region() is returning?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 106: = NULL This is set in L144. No need to initialize to NULL.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 107: NULL This is set in L136. No need to initialize to NULL.
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.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 120: printk(BIOS_INFO, "UCODE: Recovery enabled\n"); Is this print really necessary? I believe this already must be getting printed somewhere in the boot flow?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 121: ts_strap ts_strap == TS_ENABLE
Even though TS_ENABLE is 1 and TS_DISABLE is 0, it is better to check since these enums could change value and then this check would break.
Also, you can just skip having the variable ts_strap. It is critical that you call get_rtc_buc_top_swap_status() every time this function wants to know about the TS state. Not having the variable will avoid any confusion.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 122: configure_rtc_buc_top_swap(TS_DISABLE); Instead a print here would be helpful indicating why we are doing a reset and what action is being taken.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 130: init_staging_irdev(&staging_irdev) staging_rdev = init_staging_irdev(...) and check staging_rdev for NULL.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 136: rdev_mmap_full I believe this is making the assumption that boot media is memory mapped and hence the memory leak is okay. Can you please add a comment to that effect and also an assert for CONFIG(BOOT_DEVICE_MEMORY_MAPPED).
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 144: intel_microcode_find This requires CONFIG_SUPPORT_CPU_UCODE_IN_CBFS to be auto-selected.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 161: memcmp(stage_microcode, cbfs_microcode, Have you checked how much impact this entire process of compare, update, adds to boot time?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 177: TODO TODO for? Is there a bug?
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 181: Finished writing Control can reach here in two cases:
1. Booting from RTC reset where TS is disabled. In that case you might not write to staging area. But still require TS to be enabled and hence the reset. 2. Other is when current boot used microcode from staging area but there is a newer version available and so you disabled TS and updated SPI flash. In this case too, you need to enable TS back and do the reset.
It would be good to document both these paths so that readers understand when these are taken.
https://review.coreboot.org/c/coreboot/+/27369/49/src/soc/intel/common/basec... PS49, Line 199: /* TODO: Write Elog */ TODO for? Bug#?
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?
-
Furquan Shaikh (Code Review)