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 2:
(21 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.
Yeah, I forget the reason that I did that. Done.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... File src/security/vboot/sync_ec.c:
PS2:
We might want to call this file ec_sync.c instead to be more consistent with vboot_reference naming.
That makes sense. Doing it this way made it easier to keep myself from getting confused as to which ec_sync file I had open at the time :). Done.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 3: * : * Copyright 2019 Google LLC : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
I thought we decided in the last vote to stop putting full copyright notices?
vote? I can take it out if it's not supposed to be there anymore. I didn't know.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 17: #include <2constants.h>
In fact it *shouldn't* be. If you only want the constants and not the full API, then vb2_constants. […]
Done
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 35: #define EC_PRIMARY_IDX 0
Can we just get rid of the concept of "primary" and "alternate" EC altogether? There will only ever […]
Done
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. […]
That's true, we could use the stack; isn't the stack in CAR as well ?
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 50: vboot_sync_ec
With persistent context, we don't even need this function.
Understood, is persistent context implemented yet?
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.
See below.
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... […]
I wasn't sure if it was okay to reuse the work buffer directly (does depthcharge need data from it?) , so this creating a new one and copying the old data into it.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 68: vbnv_init(ctx.nvdata);
Yeah, I think we can consider that a blocker at this point. […]
Any idea on when persistent context will be ready? We're trying to move forward with this as the solution for the hatch platform's booting from low/dead battery issue.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 73: %d
%#x
Done
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 73: (int)
No need to cast.
Done
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 80: Flash
Probably no good reason to capitalize Flash here (and in below comments).
Done
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 201: if (resp.flags & EC_FLASH_PROTECT_ALL_AT_BOOT) {
braces {} are not necessary for single statement blocks
Done
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)).
Gotcha. Missed that one.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 320: google_chromeec_init();
This is very awkward. […]
That's a fair point. I'll split that up.
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.
Won't depthcharge handle that aspect when it does its EC sync routine? It should notice the hash matches immediately, and then invoke its VbootDone ?
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 336: /* Unsupported */
We'll need to support this too.
Depthcharge won't be able to notice this still needs to be done and take care of it?
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.
I'll look into that.
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'. […]
Yeah, missed that. The mdelay was originally an "inner" stopwatch, and so that one was the "outer" one :). Done.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 439: %lus, and "
... […]
and I don't remember now :) Done.