Julius Werner 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 5:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 14: #include <2ec_sync.h> Joel would know better but I think you're not supposed to do this (should be chain-included from vb2_api.h instead, and maybe defined in 2api.h instead of 2ec_sync.h).
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 27: #define alloca(size) __builtin_alloca(size) Please put this in commonlib/include/commonlib/helpers.h in a separate patch.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 38: static void *ec_image_mapping; Please don't use globals to pass parameters around. You should instead pass a (struct region_device *) parameter into ec_flash_write() and do the mmap/unmap inside that function.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 136: break; I agree with Furquan, I think the fall-through here would be a good idea.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 233: uint8_t *file_buf = (uint8_t *)ec_image_mapping; No, I think you misunderstood what Furquan meant. If you rdev_mmap_full() the whole thing on a non-x86 device, it will blow the CBFS cache immediately. You need to only map one burst size at a time (with rdev_mmap()) and then unmap it again before you map the next chunk.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 353: * so we just use the 32-byte buffer for reading the hash I think for 32 bytes it's okay to just use cbfs_boot_map_with_leak(). We can afford that much.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 379: /* Unsupported */ nit: should explain why this cannot happen.
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 464: while (google_chromeec_hello()) { This is going to be somewhat problematic on SPI devices, because the coreboot ec_spi.c driver doesn't yet have the improvements that were made to depthcharge in CL:371642. That doesn't block this patch but it would be great if you could pull that over later once you got this working for LPC devices.
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 241: rdev_readat
If you use cbfs_boot_locate() --> rdev_mmap() --> actions --> rdev_munmap() then you wouldn't need t […]
That's just sorta pushing the problem around for SRAM systems... either you put it on the stack or in the CBFS cache, but both of them might be pretty small and run out. But okay, we can try the mmap() method first and see how well that works once we have SRAM platforms actually using this.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 336: /* Unsupported */
unless we have some corner cases where EC SW sync is taking a super long time.
Well, Wilco is one of those cases, apparently. So at least for that we'll need to keep the depthcharge path around for a while. Let's hope we can get rid of Wilco ECs soon. (But yeah, for everything else we should make the romstage sync the new default.)