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 17:
(9 comments)
https://review.coreboot.org/c/coreboot/+/36208/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36208/17//COMMIT_MSG@12 PS17, Line 12: This patch assumes that the EC image is added to CBFS : uncompressed. How is this guaranteed in Chrome OS build? Are you adding a new USE flag which ensures that it selects VBOOT_EARLY_EC_SYNC in coreboot and adds the ecrw image uncompressed when building chromeos-bootimage?
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/Kconfig... PS17, Line 233: nit: extra space
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 108: rv = google_chromeec_get_vboot_hash(hash_offset, &resp); : if (rv) : return VB2_ERROR_UNKNOWN; nit:
if (google_chromeec_get_vboot_hash(hash_offset, &resp)) return VB2_ERROR_UNKNOWN;
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 167: nit: I think it would be helpful to print out how long it took to get the hash calculations using stopwatch_duration_usecs(). I know you are adding some new timestamps (have to check the latest revision of that CL). If this is already covered somewhere, please feel free to ignore.
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 268: (burst / resp_flash.write_block_size) * : resp_flash.write_block_size; ALIGN_DOWN(burst, resp_flash.write_block_size);
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 362: rv = VB2_ERROR_UNKNOWN Did you intend to do: return VB2_ERROR_UNKNOWN;
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 373: uint8_t const
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 417: google_chromeec_get_current_image Why is this call required? google_ec_running_ro() calls google_chromeec_get_current_image() internally.
https://review.coreboot.org/c/coreboot/+/36208/17/src/security/vboot/ec_sync... PS17, Line 541: 3000 nit: Probably good to define this as macros at the start of the file just like other timeout values?