Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39906 )
Change subject: drivers/pc80/rtc: Always load cmos.default if measured boot is enabled ......................................................................
Patch Set 5: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39906/1//COMMIT_MSG@7 PS1, Line 7: Always load cmos.default to satisfy measured boot
Done
Thank you for understanding.
Reading back, I see now that I didn't make my point clear enough. So, given what happened with CB:35077, I suppose that my -1 score looked like the beginning of a similar story and was thus not well-received. However, it was never my intention to hold this change hostage!
I only gave a -1 (which does not block submit) to express dissatisfaction with the unconditional loading, as it leaves no indication in the code that measured boot relies on always loading that file. By placing a check in the code, it provides a clue for any future readers so that they don't think it's a bug and "fix" it.
https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... File src/drivers/pc80/rtc/option.c:
https://review.coreboot.org/c/coreboot/+/39906/5/src/drivers/pc80/rtc/option... PS5, Line 246:
Done as you wish, but I believe we do not need such complexity just to save a file loading which in […]
I'd say the complexity is more or less the same as before. However, having that check provides any future readers with a hint that something needs the file to be loaded even when not used at all.
In any case, I've tried to make it simple as possible and came up with this:
void sanitize_cmos(void) { const unsigned char *cmos_default; const bool cmos_need_reset = CONFIG(STATIC_OPTION_TABLE) || cmos_error() || !cmos_lb_cks_valid();
size_t length = 128; size_t i;
if (CONFIG(TPM_MEASURED_BOOT) || cmos_need_reset) { cmos_default = cbfs_boot_map_with_leak("cmos.default", CBFS_COMPONENT_CMOS_DEFAULT, &length);
if (!cmos_default || !cmos_need_reset) return;
u8 control_state = cmos_disable_rtc(); for (i = 14; i < MIN(128, length); i++) cmos_write_inner(cmos_default[i], i); cmos_restore_rtc(control_state); } }