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 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 54: /* Set up context and work buffer */ nit: They're already set up you're just fetching them. (In general, I don't really think you need comments for lines with a single function call when the function name is already pretty clear about what it does.)
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 58: ctx->flags &= ~VB2_CONTEXT_EC_SYNC_SLOW; There should be no way this could be set here, so no need to clear it.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 61: vbnv_init(ctx->nvdata); No, persistent context takes care of that already now.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 66: printk(BIOS_ERR, "EC software sync failed (%#x)\n", retval); We actually need to reboot into recovery mode on error, not just print and move on. Also, we need to write back nvdata if necessary. So you should do something like
retval = vb2api_ec_sync(ctx);
assert(!(ctx->flags & (VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED | VB2_CONTEXT_SECDATA_KERNEL_CHANGED))); if (ctx->flags & VB2_CONTEXT_NVDATA_CHANGED) { save_vbnv(ctx->nvdata); ctx->flags &= ~VB2_CONTEXT_NVDATA_CHANGED; }
if (retval != VB2_SUCCESS) { printk(BIOS_ERR, "EC software sync failed (%#x), rebooting\n", retval); vboot_reboot(); }
We'll probably want to factor the saving nvdata stuff out and also use it in save_if_needed() in vboot_logic.c. (The reason I don't want the secdata saving code here is because it shouldn't be necessary and it will bloat the romstage with a bunch of TPM code that otherwise doesn't need to be there.)
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 68: return retval; Probably doesn't need a return value, then.
https://review.coreboot.org/c/coreboot/+/36208/20/src/security/vboot/ec_sync... PS20, Line 156: nit: maybe personal preference but I tend to not make a space between number and unit. (couple more times below, too)