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:
(5 comments)
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 41: EC_LPC_HOST_PACKET_SIZE
That's true, we could use the stack; isn't the stack in CAR as well ?
Yes, on x86 the stack is in CAR. The difference is that we already allocate a stack anyway, whereas allocating static buffers increases the size of the stage image (and particularly on SRAM systems that may sometimes make us hit the limit). So it's generally always better to allocate things on the stack, but you need to of course be careful to not blow it. (We used to say that stacks may be as small as 2KB for some really restricted boards, although I think we never actually went that low on any board, and everything this will run on should probably at least use 4K or more. The compiler enforces individual functions to not use more than 1.5K as a vague heuristic, but it can't stack how many functions with big allocations we chain together.)
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 wasn't sure if it was okay to reuse the work buffer directly (does depthcharge need data from it?) […]
The workbuffer *has* to be the same across all calls to vboot, otherwise all sorts of things inside vboot won't work. With persistent context, this will work automatically. (If possible, I'd suggest you wait working on this particular part for now and hopefully Joel can get his stuff in sometime soon, then all of this should become way easier.)
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 68: vbnv_init(ctx.nvdata);
Any idea on when persistent context will be ready? We're trying to move forward with this as the so […]
Let's move schedule discussions to the bug (b/112198832).
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 331: vb2_error_t VbExEcVbootDone(int in_recovery)
Won't depthcharge handle that aspect when it does its EC sync routine? It should notice the hash ma […]
Yes, but the intention there was to run that check as soon as the EC is in RW. This is part of the same brownout problem... it makes sure we wait until the EC confirms it has negotiated a PD contract or charged the battery far enough or done whatever it thinks it needs to do to proceed to the more power-hungry parts of boot.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 336: /* Unsupported */
Depthcharge won't be able to notice this still needs to be done and take care of it?
I don't think the goal here should be to permanently leave vestigial parts of EC software sync running in depthcharge. Once this work is finished and running on all future platforms we can remove the respective stuff from depthcharge and the kernel verification flow. So this should do everything that EC software sync in depthcharge is currently doing (and for the battery cutoff stuff I don't see any reason not to pull it forward to here).