Joel Kitching 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:
(6 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 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?
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.
In fact it *shouldn't* be. If you only want the constants and not the full API, then vb2_constants.h is provided.
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.
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 […]
Yeah, I think we can consider that a blocker at this point. I don't think it makes sense to re-initialize vb2_context here.
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 73: %d %#x
https://review.coreboot.org/c/coreboot/+/36208/2/src/security/vboot/sync_ec.... PS2, Line 73: (int) No need to cast.