Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31887
Change subject: vboot: make vboot_working_data available to payload ......................................................................
vboot: make vboot_working_data available to payload
* Create a new cbtable entry called VBOOT_WD for storing a pointer to the vboot vboot_working_data structure.
* Copy relevant parts of vboot_working_data struct to a new libpayload header (libpayload/include/vboot.h).
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h A payloads/libpayload/include/vboot.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 8 files changed, 98 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/1
diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index 1568526..4a31b4e 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -202,6 +202,7 @@
#define CB_TAG_VBNV 0x0019 #define CB_TAG_VBOOT_HANDOFF 0x0020 +#define CB_TAG_VBOOT_WD 0x0034 #define CB_TAG_DMA 0x0022 #define CB_TAG_RAM_OOPS 0x0023 #define CB_TAG_MTC 0x002b diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 845b7c4..db5f7fd 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -40,6 +40,7 @@
#include <coreboot_tables.h>
+struct vboot_working_data; struct cb_serial;
/* @@ -97,6 +98,7 @@
void *vboot_handoff; u32 vboot_handoff_size; + struct vboot_working_data *vboot_wd;
#if CONFIG(LP_ARCH_X86) int x86_rom_var_mtrr_index; diff --git a/payloads/libpayload/include/vboot.h b/payloads/libpayload/include/vboot.h new file mode 100644 index 0000000..3004531 --- /dev/null +++ b/payloads/libpayload/include/vboot.h @@ -0,0 +1,42 @@ +/* + * This file is part of the libpayload project. + * + * Copyright 2019 The Chromium OS Authors. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. The name of the author may not be used to endorse or promote products + * derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef __LIBPAYLOAD_VBOOT_H__ +#define __LIBPAYLOAD_VBOOT_H__ + +#include <stdint.h> + +struct vboot_working_data { + uint32_t reserved0[2]; + /* offset of the buffer from the start of this struct */ + uint32_t buffer_offset; + uint32_t buffer_size; +}; + +#endif /* __LIBPAYLOAD_VBOOT_H__ */ diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index ba5bb27..c59e70a 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -86,6 +86,13 @@ info->vboot_handoff_size = vbho->range_size; }
+static void cb_parse_vboot_wd(unsigned char *ptr, struct sysinfo_t *info) +{ + struct lb_range *vbwd = (struct lb_range *)ptr; + + info->vboot_wd = (void *)(uintptr_t)vbwd->range_start; +} + static void cb_parse_vbnv(unsigned char *ptr, struct sysinfo_t *info) { struct lb_range *vbnv = (struct lb_range *)ptr; @@ -355,6 +362,9 @@ case CB_TAG_VBOOT_HANDOFF: cb_parse_vboot_handoff(ptr, info); break; + case CB_TAG_VBOOT_WD: + cb_parse_vboot_wd(ptr, info); + break; case CB_TAG_MAC_ADDRS: cb_parse_mac_addresses(ptr, info); break; diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 6ca0f77..9b33522 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -292,6 +292,7 @@
#define LB_TAG_VBNV 0x0019 #define LB_TAB_VBOOT_HANDOFF 0x0020 +#define LB_TAB_VBOOT_WD 0x0034 #define LB_TAB_DMA 0x0022 #define LB_TAG_RAM_OOPS 0x0023 #define LB_TAG_MTC 0x002b diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 960ab0f..28f6713 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -43,6 +43,9 @@ #include <vendorcode/google/chromeos/chromeos.h> #include <vendorcode/google/chromeos/gnvs.h> #endif +#if CONFIG(VBOOT) +#include <security/vboot/misc.h> +#endif
static struct lb_header *lb_table_init(unsigned long addr) { @@ -223,8 +226,24 @@ vbho->range_start = (intptr_t)addr; vbho->range_size = size; } + +static void lb_vboot_wd(struct lb_header *header) +{ + struct lb_range *vbwd; + struct vboot_working_data *wd; + + if ((wd = vboot_get_working_data()) == NULL) + return; + + vbwd = (struct lb_range *)lb_new_record(header); + vbwd->tag = LB_TAB_VBOOT_WD; + vbwd->size = sizeof(*vbwd); + vbwd->range_start = (uintptr_t)wd; + vbwd->range_size = wd->buffer_offset + wd->buffer_size; +} #else static inline void lb_vboot_handoff(struct lb_header *header) {} +static inline void lb_vboot_wd(struct lb_header *header) {} #endif /* CONFIG_VBOOT */ #endif /* CONFIG_CHROMEOS */
@@ -538,6 +557,9 @@
/* pass along the vboot_handoff address. */ lb_vboot_handoff(head); + + /* pass along the vboot working data address. */ + lb_vboot_wd(head); #endif
/* Add strapping IDs if available */ diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 496ab78..21be031 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -25,24 +25,6 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
-struct selected_region { - uint32_t offset; - uint32_t size; -}; - -/* - * this is placed at the start of the vboot work buffer. selected_region is used - * for the verstage to return the location of the selected slot. buffer is used - * by the vboot2 core. Keep the struct CPU architecture agnostic as it crosses - * stage boundaries. - */ -struct vboot_working_data { - struct selected_region selected_region; - /* offset of the buffer from the start of this struct */ - uint32_t buffer_offset; - uint32_t buffer_size; -}; - /* TODO(kitching): Use VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE instead. */ static size_t vboot_working_data_size(void) { @@ -56,7 +38,7 @@ die("impossible!"); }
-static struct vboot_working_data * const vboot_get_working_data(void) +struct vboot_working_data * const vboot_get_working_data(void) { struct vboot_working_data *wd = NULL;
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 24e349d..27317ad 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -21,9 +21,28 @@ struct vb2_context; struct vb2_shared_data;
+struct selected_region { + uint32_t offset; + uint32_t size; +}; + +/* + * this is placed at the start of the vboot work buffer. selected_region is used + * for the verstage to return the location of the selected slot. buffer is used + * by the vboot2 core. Keep the struct CPU architecture agnostic as it crosses + * stage boundaries. + */ +struct vboot_working_data { + struct selected_region selected_region; + /* offset of the buffer from the start of this struct */ + uint32_t buffer_offset; + uint32_t buffer_size; +}; + /* * Source: security/vboot/common.c */ +struct vboot_working_data * const vboot_get_working_data(void); void vboot_init_work_context(struct vb2_context *ctx); void vboot_finalize_work_context(struct vb2_context *ctx); struct vb2_shared_data *vboot_get_shared_data(void);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot_working_data available to payload ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@235 PS1, Line 235: if ((wd = vboot_get_working_data()) == NULL) do not use assignment in if condition
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot_working_data available to payload ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c@... PS1, Line 93: info->vboot_wd = (void *)(uintptr_t)vbwd->range_start; I'm starting to wonder if we *should* just pass vb2_shared_data + workbuf only, and use lb_range's range_size to store the size of vb2_shared_data + workbuf...
That would certainly simplify not requiring us to expose vboot_working_data to libpayload, and also simplify the code to read the pointer on the other end (in depthcharge).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot_working_data available to payload ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@46 PS1, Line 46: #if CONFIG(VBOOT) don't conditionally include this bit. It should be able to be unconditionally included.
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@562 PS1, Line 562: lb_vboot_wd(head); This shouldn't be within #if CHROMEOS. It should be based on if (CONFIG(VBOOT)) -- c runtime.
https://review.coreboot.org/#/c/31887/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/1/src/security/vboot/misc.h@39 PS1, Line 39: uint32_t buffer_size; This size field should be documented for what it represents. Full size of area from start of struct?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot_working_data available to payload ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c@... PS1, Line 93: info->vboot_wd = (void *)(uintptr_t)vbwd->range_start;
I'm starting to wonder if we *should* just pass vb2_shared_data + workbuf only, and use lb_range's r […]
Yeah, actually, I think that sounds like a good solution. Let's try that.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot_working_data available to payload ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31887/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/2/src/lib/coreboot_table.c@235 PS2, Line 235: if ((wd = vboot_get_working_data()) == NULL) do not use assignment in if condition
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#3).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 55 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31887/3/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/3/src/lib/coreboot_table.c@235 PS3, Line 235: if ((wd = vboot_get_working_data()) == NULL) do not use assignment in if condition
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#4).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 56 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/4
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#5).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 60 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... PS5, Line 100: void *vb2_workbuf; Don't you need to store the size somewhere as well?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT) Do we still need this guard? Can we maybe get rid of it if we define some of the vboot_xxx() functions as empty stubs in the header when CONFIG_VBOOT is not enabled? (Compare how timestamp_get() is done in <timestamp.h>.)
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@560 PS5, Line 560: lb_vboot_handoff(head); Maybe keep this where it is because we want to get rid of it very soon anyway?
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#6).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 61 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/6
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#7).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 58 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/7
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 7: Code-Review+1
(6 comments)
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... PS5, Line 100: void *vb2_workbuf;
Don't you need to store the size somewhere as well?
Done
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c@... PS1, Line 93: info->vboot_wd = (void *)(uintptr_t)vbwd->range_start;
Yeah, actually, I think that sounds like a good solution. Let's try that.
Done
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@46 PS1, Line 46: #if CONFIG(VBOOT)
don't conditionally include this bit. It should be able to be unconditionally included.
Seems like there's a whole bunch of conditional includes above this one... has the policy changed? Or am I misunderstanding something?
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@562 PS1, Line 562: lb_vboot_wd(head);
This shouldn't be within #if CHROMEOS. It should be based on if (CONFIG(VBOOT)) -- c runtime.
Done -- but this means also changing lb_vboot_handoff.
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
Do we still need this guard? Can we maybe get rid of it if we define some of the vboot_xxx() functio […]
I don't think that strategy quite works here. Say we put the following in src/include/boot/coreboot_tables.h:
#if CONFIG(VBOOT) #define lb_vboot_handoff() 0 #define lb_vboot_workbuf() 0 #endif /* CONFIG_VBOOT */
If we remove the guards around the function definitions in this file, the macro will attempt to expand the lb_vboot_handoff and lb_vboot_workbuff symbols in the function definitions, leading to compilation errors.
That said, since the actual calls to these two functions themselves are in a CONFIG(VBOOT) conditional, if we leave out the guard, and these functions are not called, they will be removed somewhere in the build process, correct?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@560 PS5, Line 560: lb_vboot_handoff(head);
Maybe keep this where it is because we want to get rid of it very soon anyway?
Sure.
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#8).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 58 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/8
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 8: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 8:
(4 comments)
LGTM to me after you fix the compiler error.
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@46 PS1, Line 46: #if CONFIG(VBOOT)
Seems like there's a whole bunch of conditional includes above this one... […]
Yes, a lot of this code is quite old and we wouldn't write it like that anymore today.
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
I don't think that strategy quite works here. […]
No, I think you misunderstand. I meant remove the guard from these functions as they are and then stub out vboot_get_handoff_info() and vboot_get_working_data() in their respective headers. That way these functions will get called but return immediately if !CONFIG_VBOOT (and since they're static the compiler can eliminate them entirely).
Although on second thought, since you already guard the only place that calls these functions, they're gonna get eliminated anyway. So you don't even need to stub anything out. Just removing the guard should already work.
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c@234 PS8, Line 234: if (wd == NULL) nit: this can't really return NULL anyway.
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c@553 PS8, Line 553: lb_vboot_handoff(head); Okay, sorry, now you have to move this into the block below after all to get around the unused function error. ;) I didn't think that far.
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31887
to look at the new patch set (#9).
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 57 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/31887/9
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 9: Code-Review+1
(4 comments)
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@46 PS1, Line 46: #if CONFIG(VBOOT)
Yes, a lot of this code is quite old and we wouldn't write it like that anymore today.
Ack
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
No, I think you misunderstand. […]
I indeed did misunderstand. But sounds like we can just remove the guards and leave as is.
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c@234 PS8, Line 234: if (wd == NULL)
nit: this can't really return NULL anyway.
Done
https://review.coreboot.org/#/c/31887/8/src/lib/coreboot_table.c@553 PS8, Line 553: lb_vboot_handoff(head);
Okay, sorry, now you have to move this into the block below after all to get around the unused funct […]
¯_(ツ)_/¯
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 9: Code-Review+2
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... PS9, Line 205: 0x0034 Should this be at the bottom for ordering?
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... PS9, Line 100: void *vboot_workbuf; Does coreboot have a convention of commenting struct members? It doesn't look like it, but I thought I'd check.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct const?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
I indeed did misunderstand. But sounds like we can just remove the guards and leave as is.
Re your penultimate comment, yes, the compiler will drop the code if it can tell it isn't used. At least it does if you don't use -O0
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT) I saw the thread on this and it seems like it is decided to use the CONFIG() macro. One problem with it is hard to search for CONFIG options without getting a lot of unwanted matches.
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h@32 PS9, Line 32: CPU architecture agnostic CPU-architecture-agnostic
Are all these comments still true?
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... PS9, Line 205: 0x0034
Should this be at the bottom for ordering?
Doesn't really seem to be in order of memory layout... so probably makes more sense to group by related items as I've done here.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... PS9, Line 100: void *vboot_workbuf;
Does coreboot have a convention of commenting struct members? It doesn't look like it, but I thought […]
As it writing a comment for each member? Doesn't look like it... would it make sense to add some description here?
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
const?
If it makes sense to use const here, perhaps we can do that for all functions in coreboot.c in a follow-up CL.
Actually, I'm still not so certain on how const should be used. What is the rule people typically use to decide whether or not to include it? E.g.
* Everywhere and anywhere that the pointer/value is not modified * Only function signatures where there should be a contract of the function not modifying the pointer/value * Never?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
Re your penultimate comment, yes, the compiler will drop the code if it can tell it isn't used. […]
Ack
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT)
I saw the thread on this and it seems like it is decided to use the CONFIG() macro. […]
Yeah, seems like both Aaron and Julius are in heavy favour of using C conditionals wherever possible, in order to reduce bit rot. When we use the preprocessor to comment out specific sections, it can also be easy to introduce typos, etc. in sections of code that don't necessarily get compiled.
What kind of situations are you talking about for searching?
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h@32 PS9, Line 32: CPU architecture agnostic
CPU-architecture-agnostic […]
Well, I suppose it would be more accurate to say that vboot_working_data "is placed just before the start of the vboot work buffer". Would it be clearer to update the text to this?
But AFAIK everything else still holds true.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... PS9, Line 205: 0x0034
Doesn't really seem to be in order of memory layout... […]
Ack
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... PS9, Line 100: void *vboot_workbuf;
As it writing a comment for each member? Doesn't look like it... […]
As you say, it doesn't look like it, so don't worry.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
If it makes sense to use const here, perhaps we can do that for all functions in coreboot. […]
I think const should be included whenever possible :-)
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT)
Yeah, seems like both Aaron and Julius are in heavy favour of using C conditionals wherever possible […]
Yes if() is better than #ifdef for several reasons.
For search, if I want to find all uses of CONFIG_VBOOT it's a bit hard. If we consistently use CONFIG(VBOOT) (with if() or #if) then I suppose it is easier. But just searching for VBOOT is not that useful.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
I think const should be included whenever possible :-)
In that case, we can add some consts in a follow-up CL.
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT)
Yes if() is better than #ifdef for several reasons. […]
jwerner pushed a huge CL to clean up this inconsistency: https://review.coreboot.org/c/coreboot/+/31774
So you should be able to just search for CONFIG(...) now.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 10: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
In that case, we can add some consts in a follow-up CL.
OK but start small, as I am not an authority in coreboot at all. It has different conventions from U-Boot where I am more familiar.
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h@32 PS9, Line 32: CPU architecture agnostic
Well, I suppose it would be more accurate to say that vboot_working_data "is placed just before the […]
Ack
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 10: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
vboot: make vboot workbuf available to payload
Create a new cbtable entry called VBOOT_WORKBUF for storing a pointer to the vboot workbuf within the vboot_working_data structure.
BUG=b:124141368, b:124192753 TEST=Build and deploy to eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: Id68f43c282939d9e1b419e927a14fe8baa290d91 Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31887 Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/security/vboot/common.c M src/security/vboot/misc.h 7 files changed, 57 insertions(+), 27 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Simon Glass: Looks good to me, but someone else must approve Simon Glass: Looks good to me, approved Joel Kitching: Looks good to me, but someone else must approve
diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index 1568526..92e3f26 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -202,6 +202,7 @@
#define CB_TAG_VBNV 0x0019 #define CB_TAG_VBOOT_HANDOFF 0x0020 +#define CB_TAG_VBOOT_WORKBUF 0x0034 #define CB_TAG_DMA 0x0022 #define CB_TAG_RAM_OOPS 0x0023 #define CB_TAG_MTC 0x002b diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 845b7c4..7e6e748 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -97,6 +97,8 @@
void *vboot_handoff; u32 vboot_handoff_size; + void *vboot_workbuf; + uint32_t vboot_workbuf_size;
#if CONFIG(LP_ARCH_X86) int x86_rom_var_mtrr_index; diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index ba5bb27..3982e47 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -86,6 +86,14 @@ info->vboot_handoff_size = vbho->range_size; }
+static void cb_parse_vboot_workbuf(unsigned char *ptr, struct sysinfo_t *info) +{ + struct lb_range *vbwb = (struct lb_range *)ptr; + + info->vboot_workbuf = (void *)(uintptr_t)vbwb->range_start; + info->vboot_workbuf_size = vbwb->range_size; +} + static void cb_parse_vbnv(unsigned char *ptr, struct sysinfo_t *info) { struct lb_range *vbnv = (struct lb_range *)ptr; @@ -355,6 +363,9 @@ case CB_TAG_VBOOT_HANDOFF: cb_parse_vboot_handoff(ptr, info); break; + case CB_TAG_VBOOT_WORKBUF: + cb_parse_vboot_workbuf(ptr, info); + break; case CB_TAG_MAC_ADDRS: cb_parse_mac_addresses(ptr, info); break; diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 6ca0f77..198ad27 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -292,6 +292,7 @@
#define LB_TAG_VBNV 0x0019 #define LB_TAB_VBOOT_HANDOFF 0x0020 +#define LB_TAB_VBOOT_WORKBUF 0x0034 #define LB_TAB_DMA 0x0022 #define LB_TAG_RAM_OOPS 0x0023 #define LB_TAG_MTC 0x002b diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 960ab0f..6e44f5d 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -32,6 +32,7 @@ #include <cbmem.h> #include <bootmem.h> #include <spi_flash.h> +#include <security/vboot/misc.h> #include <security/vboot/vbnv_layout.h> #if CONFIG(USE_OPTION_TABLE) #include <option_table.h> @@ -206,8 +207,8 @@ vbnv->range_size = VBOOT_VBNV_BLOCK_SIZE; #endif } +#endif /* CONFIG_CHROMEOS */
-#if CONFIG(VBOOT) static void lb_vboot_handoff(struct lb_header *header) { void *addr; @@ -223,10 +224,18 @@ vbho->range_start = (intptr_t)addr; vbho->range_size = size; } -#else -static inline void lb_vboot_handoff(struct lb_header *header) {} -#endif /* CONFIG_VBOOT */ -#endif /* CONFIG_CHROMEOS */ + +static void lb_vboot_workbuf(struct lb_header *header) +{ + struct lb_range *vbwb; + struct vboot_working_data *wd = vboot_get_working_data(); + + vbwb = (struct lb_range *)lb_new_record(header); + vbwb->tag = LB_TAB_VBOOT_WORKBUF; + vbwb->size = sizeof(*vbwb); + vbwb->range_start = (uintptr_t)wd + wd->buffer_offset; + vbwb->range_size = wd->buffer_size; +}
__weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; } __weak uint32_t ram_code(void) { return UNDEFINED_STRAPPING_ID; } @@ -535,11 +544,16 @@
/* pass along VBNV offsets in CMOS */ lb_vbnv(head); - - /* pass along the vboot_handoff address. */ - lb_vboot_handoff(head); #endif
+ if (CONFIG(VBOOT)) { + /* pass along the vboot_handoff address. */ + lb_vboot_handoff(head); + + /* pass along the vboot workbuf address. */ + lb_vboot_workbuf(head); + } + /* Add strapping IDs if available */ lb_board_id(head); lb_ram_code(head); diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 47e1aa4..2348d31 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -25,24 +25,6 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
-struct selected_region { - uint32_t offset; - uint32_t size; -}; - -/* - * this is placed at the start of the vboot work buffer. selected_region is used - * for the verstage to return the location of the selected slot. buffer is used - * by the vboot2 core. Keep the struct CPU architecture agnostic as it crosses - * stage boundaries. - */ -struct vboot_working_data { - struct selected_region selected_region; - /* offset of the buffer from the start of this struct */ - uint32_t buffer_offset; - uint32_t buffer_size; -}; - /* TODO(kitching): Use VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE instead. */ static size_t vboot_working_data_size(void) { @@ -56,7 +38,7 @@ die("impossible!"); }
-static struct vboot_working_data * const vboot_get_working_data(void) +struct vboot_working_data * const vboot_get_working_data(void) { struct vboot_working_data *wd = NULL;
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 24e349d..27317ad 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -21,9 +21,28 @@ struct vb2_context; struct vb2_shared_data;
+struct selected_region { + uint32_t offset; + uint32_t size; +}; + +/* + * this is placed at the start of the vboot work buffer. selected_region is used + * for the verstage to return the location of the selected slot. buffer is used + * by the vboot2 core. Keep the struct CPU architecture agnostic as it crosses + * stage boundaries. + */ +struct vboot_working_data { + struct selected_region selected_region; + /* offset of the buffer from the start of this struct */ + uint32_t buffer_offset; + uint32_t buffer_size; +}; + /* * Source: security/vboot/common.c */ +struct vboot_working_data * const vboot_get_working_data(void); void vboot_init_work_context(struct vb2_context *ctx); void vboot_finalize_work_context(struct vb2_context *ctx); struct vb2_shared_data *vboot_get_shared_data(void);
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... PS9, Line 205: 0x0034
Ack
Actually, I agree that putting this at the bottom of this list (i.e. below CB_TAG_VPD) would probably be better, so that at least that part is ordered. It's already hard enough to figure out what the highest used tag is, no need to make it even harder.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
OK but start small, as I am not an authority in coreboot at all. […]
There are no hard rules on const. It's certainly nicer to use it judiciously if you want to, but many people don't bother. I agree that since this pattern repeats a lot in this file, you should probably pick to what the other functions do and maybe constify the whole file afterwards if you want.
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT)
jwerner pushed a huge CL to clean up this inconsistency: […]
Yeah, CONFIG(VBOOT) should be used in all .c files which is enforced by the linter (Makefiles would still use CONFIG_VBOOT). I guess you have a point about grepping if you want to just find everything at once... but the change is already through and I think the advantages still outweigh (the IS_ENABLED(CONFIG_...) thing really got tedious imo). FWIW when I grep for config options I usually grep without the CONFIG_ (e.g. just "VBOOT") anyway, because that's the only way you also see depends on VBOOT or default y if VBOOT in Kconfig files (for VBOOT this would be annoying but most Kconfig options have a more unique name).
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h@32 PS9, Line 32: CPU architecture agnostic
Ack
The architecture agnostic is mostly a warning that you shouldn't use things like size_t in here (e.g. that's why it's not using coreboot's usual 'struct region'). We have some platforms (like t210) where earlier stages run on a different processor with different address width then later ones.
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Simon Glass (Code Review)