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 2:
(13 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 21: coerced to an int If you're returning a vb2_error_t, you should make that the return type.
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 17: #include <2constants.h> This is chain-included from <vb2_api.h> and shouldn't need to be included directly.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 41: EC_LPC_HOST_PACKET_SIZE Again, you shouldn't use this constant here. Probably want some sort of API to get the dynamically determined max request size.
Also, maybe allocate on the stack to save pre-RAM memory? (Should limit it to max 1KB then. Alternatively, I guess you could use the vboot work buffer too.)
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 52: static uint8_t temp_workbuf[VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE]; Yeah, this isn't gonna work. You need to get the real workbuffer, you can't just make a new one.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 64: memcpy(ctx.workbuf, (void *)p, wd->buffer_size); I have no idea what you're trying to do here...?
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 68: vbnv_init(ctx.nvdata); I hope we can get Joel's persistent context work in before this, because as written here this is all pretty awkward...
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 309: vb2_error_t VbExNvStorageWrite(const uint8_t *buf) Need to implement this (it's essentially just save_vbnv(buf)).
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 320: google_chromeec_init(); This is very awkward. google_chromeec_init() has always been kinda weird in coreboot in that only some platforms run it and only in some stages. We should either make that more consistent so that we always know it will run in romstage, or get rid of it and make functions which need version information run EC_CMD_GET_VERSION directly.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 331: vb2_error_t VbExEcVbootDone(int in_recovery) This will need to do the same "wait until EC has power" dance as it does in depthcharge.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 336: /* Unsupported */ We'll need to support this too.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 398: vb2_error_t VbExEcGetExpectedImage(int devidx, enum VbSelectFirmware_t select, We should really get rid of this callback in vboot before doing this.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 417: outer nit: weird name? (If you don't want to make up something specific, we usually just call it 'sw'.)
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 439: %lus, and " ...and what?