Tim Wawrzynczak 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:
(7 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 27: #define alloca(size) __builtin_alloca(size)
Please put this in commonlib/include/commonlib/helpers.h in a separate patch.
Done
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. […]
Done
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.
-Wimplicit-fallthrough is enabled, I'll just add the mdelay here, too.
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. […]
Ah, I see. Yes, I did misunderstand. Will update.
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.
Done
https://review.coreboot.org/c/coreboot/+/36208/3/src/security/vboot/ec_sync.... PS3, Line 379: /* Unsupported */
nit: should explain why this cannot happen.
Done
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. […]
b/143381650