Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36208 )
Change subject: security/vboot: Add vboot callbacks to support EC software sync ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... File src/security/vboot/sync_ec.h:
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 23: vboot_sync_ec Since this will most likely be the only function in this header file, can it be instead added to vboot_common.h?
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... File src/security/vboot/sync_ec.c:
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 135: break; Should we just fall through to allow the mdelay() before calling google_chromeec_get_vboot_hash() again?
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 241: rdev_readat If you use cbfs_boot_locate() --> rdev_mmap() --> actions --> rdev_munmap() then you wouldn't need the local storage in this file at all. For systems using memory mapped SPI flash, you will read directly from memory mapped region. For systems without memory mapped SPI flash, rdev_mmap() will ensure that it maps to cbfs_cache and reads hit that cache. So, readat to a local buffer would be eliminated in this function.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 267: rv == VBERROR_EC_REBOOT_TO_RO_REQUIRED || rv != VB2_SUCCESS Shouldn't this just be: if (rv != VB2_SUCCESS) return rv;
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 320: google_chromeec_init();
That's a fair point. I'll split that up.
Yeah this function seems really weird. Do we really need it here? I know it is named init, but it doesn't really seem to initialize anything.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 321: if (google_ec_running_ro()) { : *in_rw = 0; : } else { : *in_rw = 1; : } *in_rw = !google_ec_running_ro();
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 336: /* Unsupported */
Done
We will probably have to look at this on a case-by-case basis to see if every board is okay with moving SW sync to depthcharge. Mostly I would think so unless we have some corner cases where EC SW sync is taking a super long time.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 364: cbfs_boot_map_with_leak This wouldn't work for non-x86 systems. See my comment about using cbfs_boot_locate(), etc.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 419: assert(devidx == 0); Probably goes away once you update vboot.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 433: -1 VB2_ERROR_UNKNOWN?