Hello Hung-Te Lin, Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41757
to review the following change.
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 140 insertions(+), 202 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/41757/1
diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc index cc276e4..f54c4d0 100644 --- a/src/drivers/vpd/Makefile.inc +++ b/src/drivers/vpd/Makefile.inc @@ -1,2 +1,7 @@ -romstage-$(CONFIG_VPD) += vpd_decode.c vpd_premem.c vpd.c -ramstage-$(CONFIG_VPD) += vpd_decode.c vpd_cbmem.c vpd.c +# SPDX-License-Identifier: GPL-2.0-only + +bootblock-$(CONFIG_VPD) += vpd_decode.c vpd.c +verstage-$(CONFIG_VPD) += vpd_decode.c vpd.c +romstage-$(CONFIG_VPD) += vpd_decode.c vpd.c +postcar-$(CONFIG_VPD) += vpd_decode.c vpd.c +ramstage-$(CONFIG_VPD) += vpd_decode.c vpd.c diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c index 7314c35..e6f5d44 100644 --- a/src/drivers/vpd/vpd.c +++ b/src/drivers/vpd/vpd.c @@ -11,6 +11,23 @@ #include "vpd_decode.h" #include "vpd_tables.h"
+/* Currently we only support Google VPD 2.0, which has a fixed offset. */ +enum { + CROSVPD_CBMEM_MAGIC = 0x43524f53, + CROSVPD_CBMEM_VERSION = 0x0001, +}; + +struct vpd_cbmem { + uint32_t magic; + uint32_t version; + uint32_t ro_size; + uint32_t rw_size; + uint8_t blob[0]; + /* The blob contains both RO and RW data. It starts with RO (0 .. + * ro_size) and then RW (ro_size .. ro_size+rw_size). + */ +}; + struct vpd_gets_arg { const uint8_t *key; const uint8_t *value; @@ -18,110 +35,140 @@ int matched; };
-struct vpd_blob vpd_blob; +static struct region_device ro_vpd, rw_vpd;
/* - * returns the size of data in a VPD 2.0 formatted fmap region, or 0. - * Also sets *base as the region's base address. + * Initializes a region_device to represent the requested VPD 2.0 formatted + * region on flash. On errors rdev->size will be set to 0. */ -static int32_t get_vpd_size(const char *fmap_name, int32_t *base) +static void init_vpd_rdev(const char *fmap_name, struct region_device *rdev) { struct google_vpd_info info; - struct region_device vpd; int32_t size;
- if (fmap_locate_area_as_rdev(fmap_name, &vpd)) { + if (fmap_locate_area_as_rdev(fmap_name, rdev)) { printk(BIOS_ERR, "%s: No %s FMAP section.\n", __func__, fmap_name); - return 0; + goto fail; }
- size = region_device_sz(&vpd); + size = region_device_sz(rdev);
if ((size < GOOGLE_VPD_2_0_OFFSET + sizeof(info)) || - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, + rdev_chain(rdev, rdev, GOOGLE_VPD_2_0_OFFSET, size - GOOGLE_VPD_2_0_OFFSET)) { printk(BIOS_ERR, "%s: Too small (%d) for Google VPD 2.0.\n", __func__, size); - return 0; + goto fail; }
/* Try if we can find a google_vpd_info, otherwise read whole VPD. */ - if (rdev_readat(&vpd, &info, *base, sizeof(info)) != sizeof(info)) { + if (rdev_readat(rdev, &info, 0, sizeof(info)) != sizeof(info)) { printk(BIOS_ERR, "ERROR: Failed to read %s header.\n", fmap_name); - return 0; + goto fail; }
if (memcmp(info.header.magic, VPD_INFO_MAGIC, sizeof(info.header.magic)) - == 0 && size >= info.size + sizeof(info)) { - *base += sizeof(info); - size = info.size; + == 0) { + if (rdev_chain(rdev, rdev, sizeof(info), info.size)) { + printk(BIOS_ERR, "ERROR: %s info size too large.\n", + fmap_name); + goto fail; + } } else if (info.header.tlv.type == VPD_TYPE_TERMINATOR || info.header.tlv.type == VPD_TYPE_IMPLICIT_TERMINATOR) { printk(BIOS_WARNING, "WARNING: %s is uninitialized or empty.\n", fmap_name); - size = 0; - } else { - size -= GOOGLE_VPD_2_0_OFFSET; + goto fail; }
- return size; + return; + +fail: + memset(rdev, 0, sizeof(*rdev)); }
-static void vpd_get_blob(void) -{ - int32_t ro_vpd_base = 0; - int32_t rw_vpd_base = 0; - int32_t ro_vpd_size = get_vpd_size("RO_VPD", &ro_vpd_base); - int32_t rw_vpd_size = get_vpd_size("RW_VPD", &rw_vpd_base); +static int init_vpd_rdevs_from_cbmem(void) { + if (!cbmem_possibly_online()) + return -1;
- /* Return if no VPD at all */ - if (ro_vpd_size == 0 && rw_vpd_size == 0) + struct vpd_cbmem *cbmem = cbmem_find(CBMEM_ID_VPD); + if (!cbmem) + return -1; + + static struct mem_region_device mdev; + mdev = MEM_REGION_DEV_RO_INIT(cbmem->blob, + cbmem->ro_size + cbmem->rw_size); + + rdev_chain(&ro_vpd, &mdev.rdev, 0, cbmem->ro_size); + rdev_chain(&rw_vpd, &mdev.rdev, cbmem->ro_size, cbmem->rw_size); + + return 0; +} + +static void init_vpd_rdevs(void) +{ + static bool done = false; + + if (done) return;
- vpd_blob.ro_base = NULL; - vpd_blob.ro_size = 0; - vpd_blob.rw_base = NULL; - vpd_blob.rw_size = 0; - - struct region_device vpd; - - if (ro_vpd_size) { - if (fmap_locate_area_as_rdev("RO_VPD", &vpd)) { - /* shouldn't happen, but let's be extra defensive */ - printk(BIOS_ERR, "%s: No RO_VPD FMAP section.\n", - __func__); - return; - } - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, - region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - vpd_blob.ro_base = (uint8_t *)(rdev_mmap_full(&vpd) + - sizeof(struct google_vpd_info)); - vpd_blob.ro_size = ro_vpd_size; + if (init_vpd_rdevs_from_cbmem() != 0) { + init_vpd_rdev("RO_VPD", &ro_vpd); + init_vpd_rdev("RW_VPD", &rw_vpd); } - if (rw_vpd_size) { - if (fmap_locate_area_as_rdev("RW_VPD", &vpd)) { - /* shouldn't happen, but let's be extra defensive */ - printk(BIOS_ERR, "%s: No RW_VPD FMAP section.\n", - __func__); - return; - } - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, - region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - vpd_blob.rw_base = (uint8_t *)(rdev_mmap_full(&vpd) + - sizeof(struct google_vpd_info)); - vpd_blob.rw_size = rw_vpd_size; - } - vpd_blob.initialized = true; + + done = true; }
-const struct vpd_blob *vpd_load_blob(void) +static void cbmem_add_cros_vpd(int is_recovery) { - if (vpd_blob.initialized == false) - vpd_get_blob(); + struct vpd_cbmem *cbmem;
- return &vpd_blob; + timestamp_add_now(TS_START_COPYVPD); + + init_vpd_rdevs(); + + /* Return if no VPD at all */ + if (region_device_sz(&ro_vpd) == 0 && region_device_sz(&rw_vpd) == 0) + return; + + size_t ro_size = region_device_sz(&ro_vpd); + size_t rw_size = region_device_sz(&rw_vpd); + + cbmem = cbmem_add(CBMEM_ID_VPD, sizeof(*cbmem) + ro_size + rw_size); + if (!cbmem) { + printk(BIOS_ERR, "%s: Failed to allocate CBMEM (%zu+%zu).\n", + __func__, ro_size, rw_size); + return; + } + + cbmem->magic = CROSVPD_CBMEM_MAGIC; + cbmem->version = CROSVPD_CBMEM_VERSION; + cbmem->ro_size = ro_size; + cbmem->rw_size = rw_size; + + if (ro_size) { + if (rdev_readat(&ro_vpd, cbmem->blob, 0, ro_size) != ro_size) { + printk(BIOS_ERR, "ERROR: Couldn't read RO VPD\n"); + cbmem->ro_size = ro_size = 0; + } else { + + } + timestamp_add_now(TS_END_COPYVPD_RO); + } + + if (rw_size) { + if (rdev_readat(&rw_vpd, cbmem->blob + ro_size, 0, rw_size) + != rw_size) { + printk(BIOS_ERR, "ERROR: Couldn't read RW VPD\n"); + cbmem->rw_size = rw_size = 0; + } + timestamp_add_now(TS_END_COPYVPD_RW); + } + + init_vpd_rdevs_from_cbmem(); }
static int vpd_gets_callback(const uint8_t *key, uint32_t key_len, @@ -141,33 +188,34 @@ return VPD_DECODE_FAIL; }
+static void vpd_find_in(struct region_device *rdev, struct vpd_gets_arg *arg) +{ + if (region_device_sz(rdev) == 0) + return; + + uint32_t consumed = 0; + void *mapping = rdev_mmap_full(rdev); + while (vpd_decode_string(region_device_sz(rdev), mapping, + &consumed, vpd_gets_callback, arg) == VPD_DECODE_OK) { + /* Iterate until found or no more entries. */ + } + rdev_munmap(rdev, mapping); +} + const void *vpd_find(const char *key, int *size, enum vpd_region region) { - struct vpd_blob blob = {0}; - - vpd_get_buffers(&blob); - if (blob.ro_size == 0 && blob.rw_size == 0) - return NULL; - struct vpd_gets_arg arg = {0}; - uint32_t consumed = 0;
arg.key = (const uint8_t *)key; arg.key_len = strlen(key);
- if ((region == VPD_ANY || region == VPD_RO) && blob.ro_size != 0) { - while (vpd_decode_string(blob.ro_size, blob.ro_base, - &consumed, vpd_gets_callback, &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ - } - } + init_vpd_rdevs();
- if ((!arg.matched && region != VPD_RO) && blob.rw_size != 0) { - while (vpd_decode_string(blob.rw_size, blob.rw_base, - &consumed, vpd_gets_callback, &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ - } - } + if (region != VPD_RW) + vpd_find_in(&ro_vpd, &arg); + + if (!arg.matched && region != VPD_RO) + vpd_find_in(&rw_vpd, &arg);
if (!arg.matched) return NULL; @@ -223,3 +271,5 @@ } else return false; } + +ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h index a786625..d54cef8 100644 --- a/src/drivers/vpd/vpd.h +++ b/src/drivers/vpd/vpd.h @@ -13,29 +13,6 @@ VPD_RW = 2 };
-/* VPD 2.0 data blob structure */ -struct vpd_blob { - bool initialized; - uint8_t *ro_base; - uint32_t ro_size; - uint8_t *rw_base; - uint32_t rw_size; -}; -extern struct vpd_blob g_vpd_blob; - -/* - * This function loads g_vpd_blob global variable. - * The variable is initialized if it was not. - */ -const struct vpd_blob *vpd_load_blob(void); - -/* - * This function gets the base address and size of - * buffers for RO_VPD/RW_VPD binary blobs, and sets - * the struct. - */ -void vpd_get_buffers(struct vpd_blob *blob); - /* * Reads VPD string value by key. * diff --git a/src/drivers/vpd/vpd_cbmem.c b/src/drivers/vpd/vpd_cbmem.c deleted file mode 100644 index eadda52..0000000 --- a/src/drivers/vpd/vpd_cbmem.c +++ /dev/null @@ -1,81 +0,0 @@ -/* SPDX-License-Identifier: BSD-3-Clause */ - -#include <console/console.h> -#include <cbmem.h> -#include <fmap.h> -#include <string.h> -#include <timestamp.h> - -#include "vpd_tables.h" -#include "vpd.h" - -/* Currently we only support Google VPD 2.0, which has a fixed offset. */ -enum { - CROSVPD_CBMEM_MAGIC = 0x43524f53, - CROSVPD_CBMEM_VERSION = 0x0001, -}; - -struct vpd_cbmem { - uint32_t magic; - uint32_t version; - uint32_t ro_size; - uint32_t rw_size; - uint8_t blob[0]; - /* The blob contains both RO and RW data. It starts with RO (0 .. - * ro_size) and then RW (ro_size .. ro_size+rw_size). - */ -}; - -static void cbmem_add_cros_vpd(int is_recovery) -{ - struct vpd_cbmem *cbmem; - const struct vpd_blob *blob; - - timestamp_add_now(TS_START_COPYVPD); - - blob = vpd_load_blob(); - - /* Return if no VPD at all */ - if (blob->ro_size == 0 && blob->rw_size == 0) - return; - - cbmem = cbmem_add(CBMEM_ID_VPD, sizeof(*cbmem) + blob->ro_size + - blob->rw_size); - if (!cbmem) { - printk(BIOS_ERR, "%s: Failed to allocate CBMEM (%u+%u).\n", - __func__, blob->ro_size, blob->rw_size); - return; - } - - cbmem->magic = CROSVPD_CBMEM_MAGIC; - cbmem->version = CROSVPD_CBMEM_VERSION; - cbmem->ro_size = blob->ro_size; - cbmem->rw_size = blob->rw_size; - - if (blob->ro_size) { - memcpy(cbmem->blob, blob->ro_base, blob->ro_size); - timestamp_add_now(TS_END_COPYVPD_RO); - } - - if (blob->rw_size) { - memcpy(cbmem->blob + blob->ro_size, blob->rw_base, - blob->rw_size); - timestamp_add_now(TS_END_COPYVPD_RW); - } -} - -void vpd_get_buffers(struct vpd_blob *blob) -{ - const struct vpd_cbmem *vpd; - - vpd = cbmem_find(CBMEM_ID_VPD); - if (!vpd || !vpd->ro_size) - return; - - blob->ro_base = (void *)vpd->blob; - blob->ro_size = vpd->ro_size; - blob->rw_base = (void *)vpd->blob + vpd->ro_size; - blob->rw_size = vpd->rw_size; -} - -RAMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) diff --git a/src/drivers/vpd/vpd_premem.c b/src/drivers/vpd/vpd_premem.c deleted file mode 100644 index 641bf89..0000000 --- a/src/drivers/vpd/vpd_premem.c +++ /dev/null @@ -1,13 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <string.h> - -#include "vpd.h" - -void vpd_get_buffers(struct vpd_blob *blob) -{ - const struct vpd_blob *b; - - b = vpd_load_blob(); - memcpy(blob, b, sizeof(*b)); -}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41757/1/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/1/src/drivers/vpd/vpd.c@92 PS1, Line 92: static int init_vpd_rdevs_from_cbmem(void) { open brace '{' following function definitions go on the next line
Hello Hung-Te Lin, build bot (Jenkins), Martin Roth, Patrick Georgi, Jonathan Zhang, David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41757
to look at the new patch set (#2).
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 140 insertions(+), 202 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/41757/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41757/2/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/2/src/drivers/vpd/vpd.c@92 PS2, Line 92: static int init_vpd_rdevs_from_cbmem(void) { open brace '{' following function definitions go on the next line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41757/2/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/2/src/drivers/vpd/vpd.c@92 PS2, Line 92: static int init_vpd_rdevs_from_cbmem(void) {
open brace '{' following function definitions go on the next line
Done
Hello Hung-Te Lin, build bot (Jenkins), Martin Roth, Patrick Georgi, Jonathan Zhang, David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41757
to look at the new patch set (#3).
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 140 insertions(+), 201 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/41757/3
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 3:
(1 comment)
this looks good to me, but will that work on x86 bootblock/romstage since you've used static?
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c@157 PS3, Line 157: } else { do we need this else?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c@123 PS3, Line 123: done = true; If CBMEM is not available, the rdevs are initialized from memory mapped flash. Once CBMEM is available, the rdevs should then be initalized from CBMEM. The current logic checks "done" and return, thus there is no chance to run init_vpd_rdevs_from_cbmem() to check if CBMEM availability changes since last call.
Hello Hung-Te Lin, build bot (Jenkins), Martin Roth, Patrick Georgi, Jonathan Zhang, David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41757
to look at the new patch set (#4).
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 138 insertions(+), 201 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/41757/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 3:
(2 comments)
this looks good to me, but will that work on x86 bootblock/romstage since you've used static?
This restriction has been eliminated (at least for static data initialized to zero). See CB:40535.
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c@123 PS3, Line 123: done = true;
If CBMEM is not available, the rdevs are initialized from memory mapped flash. […]
That's why cbmem_add_cros_vpd() calls init_vpd_devs_from_cbmem() directly at the end, to switch the rdevs to CBMEM backing without going through this function again.
https://review.coreboot.org/c/coreboot/+/41757/3/src/drivers/vpd/vpd.c@157 PS3, Line 157: } else {
do we need this else?
Sorry, that was a leftover from another attempt. Fixed.
Hello Hung-Te Lin, build bot (Jenkins), Martin Roth, Patrick Georgi, Jonathan Zhang, David Hendricks,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41757
to look at the new patch set (#5).
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 137 insertions(+), 201 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/41757/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
Patch Set 5: Code-Review+2
this looks good to me, but will that work on x86 bootblock/romstage since you've used static?
This restriction has been eliminated (at least for static data initialized to zero). See CB:40535.
Good to know it's fixed. That was why we implemented the vpd in such way last time.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41757 )
Change subject: drivers/vpd: Fix VPD speed regressions on non-x86 devices ......................................................................
drivers/vpd: Fix VPD speed regressions on non-x86 devices
CB:34634 expanded the VPD code to also be usable from romstage, shuffling a few things around and adding some extra infrastructure in the process. Unfortunately, the changes seem to have only been written with x86 devices in mind and make coreboot always load the whole VPD FMAP section (not just the used part) on devices where rdev_mmap() is not a no-op.
This patch rewrites the VPD code to be based on region_device structures that only represent the VPD area actually used (rather than the whole FMAP section), and that only get mapped when accessed. (It would be even better to pull this concept into the VPD decoder itself, but since that is taken from third-party code and accesses in early stages aren't very common, let's not go there for now.) It also moves the copying into CBMEM to romstage so that late romstage accesses can already benefit from it, and makes early decoding available in all stages because at this point, why not.
Also fix a long-standing bug where the 'consumed' counter was not reset between vpd_decode_string() calls to the RO and the RW VPD.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I55a103180b290c1563e35a25496188b6a82e49ff Reviewed-on: https://review.coreboot.org/c/coreboot/+/41757 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h D src/drivers/vpd/vpd_cbmem.c D src/drivers/vpd/vpd_premem.c 5 files changed, 137 insertions(+), 201 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc index cc276e4..f54c4d0 100644 --- a/src/drivers/vpd/Makefile.inc +++ b/src/drivers/vpd/Makefile.inc @@ -1,2 +1,7 @@ -romstage-$(CONFIG_VPD) += vpd_decode.c vpd_premem.c vpd.c -ramstage-$(CONFIG_VPD) += vpd_decode.c vpd_cbmem.c vpd.c +# SPDX-License-Identifier: GPL-2.0-only + +bootblock-$(CONFIG_VPD) += vpd_decode.c vpd.c +verstage-$(CONFIG_VPD) += vpd_decode.c vpd.c +romstage-$(CONFIG_VPD) += vpd_decode.c vpd.c +postcar-$(CONFIG_VPD) += vpd_decode.c vpd.c +ramstage-$(CONFIG_VPD) += vpd_decode.c vpd.c diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c index 7314c35..f5ac81e 100644 --- a/src/drivers/vpd/vpd.c +++ b/src/drivers/vpd/vpd.c @@ -4,6 +4,7 @@ #include <console/console.h> #include <cbmem.h> #include <fmap.h> +#include <program_loading.h> #include <string.h> #include <timestamp.h>
@@ -11,6 +12,23 @@ #include "vpd_decode.h" #include "vpd_tables.h"
+/* Currently we only support Google VPD 2.0, which has a fixed offset. */ +enum { + CROSVPD_CBMEM_MAGIC = 0x43524f53, + CROSVPD_CBMEM_VERSION = 0x0001, +}; + +struct vpd_cbmem { + uint32_t magic; + uint32_t version; + uint32_t ro_size; + uint32_t rw_size; + uint8_t blob[0]; + /* The blob contains both RO and RW data. It starts with RO (0 .. + * ro_size) and then RW (ro_size .. ro_size+rw_size). + */ +}; + struct vpd_gets_arg { const uint8_t *key; const uint8_t *value; @@ -18,110 +36,137 @@ int matched; };
-struct vpd_blob vpd_blob; +static struct region_device ro_vpd, rw_vpd;
/* - * returns the size of data in a VPD 2.0 formatted fmap region, or 0. - * Also sets *base as the region's base address. + * Initializes a region_device to represent the requested VPD 2.0 formatted + * region on flash. On errors rdev->size will be set to 0. */ -static int32_t get_vpd_size(const char *fmap_name, int32_t *base) +static void init_vpd_rdev(const char *fmap_name, struct region_device *rdev) { struct google_vpd_info info; - struct region_device vpd; int32_t size;
- if (fmap_locate_area_as_rdev(fmap_name, &vpd)) { + if (fmap_locate_area_as_rdev(fmap_name, rdev)) { printk(BIOS_ERR, "%s: No %s FMAP section.\n", __func__, fmap_name); - return 0; + goto fail; }
- size = region_device_sz(&vpd); + size = region_device_sz(rdev);
if ((size < GOOGLE_VPD_2_0_OFFSET + sizeof(info)) || - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, + rdev_chain(rdev, rdev, GOOGLE_VPD_2_0_OFFSET, size - GOOGLE_VPD_2_0_OFFSET)) { printk(BIOS_ERR, "%s: Too small (%d) for Google VPD 2.0.\n", __func__, size); - return 0; + goto fail; }
/* Try if we can find a google_vpd_info, otherwise read whole VPD. */ - if (rdev_readat(&vpd, &info, *base, sizeof(info)) != sizeof(info)) { + if (rdev_readat(rdev, &info, 0, sizeof(info)) != sizeof(info)) { printk(BIOS_ERR, "ERROR: Failed to read %s header.\n", fmap_name); - return 0; + goto fail; }
if (memcmp(info.header.magic, VPD_INFO_MAGIC, sizeof(info.header.magic)) - == 0 && size >= info.size + sizeof(info)) { - *base += sizeof(info); - size = info.size; + == 0) { + if (rdev_chain(rdev, rdev, sizeof(info), info.size)) { + printk(BIOS_ERR, "ERROR: %s info size too large.\n", + fmap_name); + goto fail; + } } else if (info.header.tlv.type == VPD_TYPE_TERMINATOR || info.header.tlv.type == VPD_TYPE_IMPLICIT_TERMINATOR) { printk(BIOS_WARNING, "WARNING: %s is uninitialized or empty.\n", fmap_name); - size = 0; - } else { - size -= GOOGLE_VPD_2_0_OFFSET; + goto fail; }
- return size; + return; + +fail: + memset(rdev, 0, sizeof(*rdev)); }
-static void vpd_get_blob(void) +static int init_vpd_rdevs_from_cbmem(void) { - int32_t ro_vpd_base = 0; - int32_t rw_vpd_base = 0; - int32_t ro_vpd_size = get_vpd_size("RO_VPD", &ro_vpd_base); - int32_t rw_vpd_size = get_vpd_size("RW_VPD", &rw_vpd_base); + if (!cbmem_possibly_online()) + return -1;
- /* Return if no VPD at all */ - if (ro_vpd_size == 0 && rw_vpd_size == 0) + struct vpd_cbmem *cbmem = cbmem_find(CBMEM_ID_VPD); + if (!cbmem) + return -1; + + rdev_chain(&ro_vpd, &addrspace_32bit.rdev, + (uintptr_t)cbmem->blob, cbmem->ro_size); + rdev_chain(&rw_vpd, &addrspace_32bit.rdev, + (uintptr_t)cbmem->blob + cbmem->ro_size, cbmem->rw_size); + + return 0; +} + +static void init_vpd_rdevs(void) +{ + static bool done = false; + + if (done) return;
- vpd_blob.ro_base = NULL; - vpd_blob.ro_size = 0; - vpd_blob.rw_base = NULL; - vpd_blob.rw_size = 0; - - struct region_device vpd; - - if (ro_vpd_size) { - if (fmap_locate_area_as_rdev("RO_VPD", &vpd)) { - /* shouldn't happen, but let's be extra defensive */ - printk(BIOS_ERR, "%s: No RO_VPD FMAP section.\n", - __func__); - return; - } - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, - region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - vpd_blob.ro_base = (uint8_t *)(rdev_mmap_full(&vpd) + - sizeof(struct google_vpd_info)); - vpd_blob.ro_size = ro_vpd_size; + if (init_vpd_rdevs_from_cbmem() != 0) { + init_vpd_rdev("RO_VPD", &ro_vpd); + init_vpd_rdev("RW_VPD", &rw_vpd); } - if (rw_vpd_size) { - if (fmap_locate_area_as_rdev("RW_VPD", &vpd)) { - /* shouldn't happen, but let's be extra defensive */ - printk(BIOS_ERR, "%s: No RW_VPD FMAP section.\n", - __func__); - return; - } - rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, - region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - vpd_blob.rw_base = (uint8_t *)(rdev_mmap_full(&vpd) + - sizeof(struct google_vpd_info)); - vpd_blob.rw_size = rw_vpd_size; - } - vpd_blob.initialized = true; + + done = true; }
-const struct vpd_blob *vpd_load_blob(void) +static void cbmem_add_cros_vpd(int is_recovery) { - if (vpd_blob.initialized == false) - vpd_get_blob(); + struct vpd_cbmem *cbmem;
- return &vpd_blob; + timestamp_add_now(TS_START_COPYVPD); + + init_vpd_rdevs(); + + /* Return if no VPD at all */ + if (region_device_sz(&ro_vpd) == 0 && region_device_sz(&rw_vpd) == 0) + return; + + size_t ro_size = region_device_sz(&ro_vpd); + size_t rw_size = region_device_sz(&rw_vpd); + + cbmem = cbmem_add(CBMEM_ID_VPD, sizeof(*cbmem) + ro_size + rw_size); + if (!cbmem) { + printk(BIOS_ERR, "%s: Failed to allocate CBMEM (%zu+%zu).\n", + __func__, ro_size, rw_size); + return; + } + + cbmem->magic = CROSVPD_CBMEM_MAGIC; + cbmem->version = CROSVPD_CBMEM_VERSION; + cbmem->ro_size = ro_size; + cbmem->rw_size = rw_size; + + if (ro_size) { + if (rdev_readat(&ro_vpd, cbmem->blob, 0, ro_size) != ro_size) { + printk(BIOS_ERR, "ERROR: Couldn't read RO VPD\n"); + cbmem->ro_size = ro_size = 0; + } + timestamp_add_now(TS_END_COPYVPD_RO); + } + + if (rw_size) { + if (rdev_readat(&rw_vpd, cbmem->blob + ro_size, 0, rw_size) + != rw_size) { + printk(BIOS_ERR, "ERROR: Couldn't read RW VPD\n"); + cbmem->rw_size = rw_size = 0; + } + timestamp_add_now(TS_END_COPYVPD_RW); + } + + init_vpd_rdevs_from_cbmem(); }
static int vpd_gets_callback(const uint8_t *key, uint32_t key_len, @@ -141,33 +186,34 @@ return VPD_DECODE_FAIL; }
+static void vpd_find_in(struct region_device *rdev, struct vpd_gets_arg *arg) +{ + if (region_device_sz(rdev) == 0) + return; + + uint32_t consumed = 0; + void *mapping = rdev_mmap_full(rdev); + while (vpd_decode_string(region_device_sz(rdev), mapping, + &consumed, vpd_gets_callback, arg) == VPD_DECODE_OK) { + /* Iterate until found or no more entries. */ + } + rdev_munmap(rdev, mapping); +} + const void *vpd_find(const char *key, int *size, enum vpd_region region) { - struct vpd_blob blob = {0}; - - vpd_get_buffers(&blob); - if (blob.ro_size == 0 && blob.rw_size == 0) - return NULL; - struct vpd_gets_arg arg = {0}; - uint32_t consumed = 0;
arg.key = (const uint8_t *)key; arg.key_len = strlen(key);
- if ((region == VPD_ANY || region == VPD_RO) && blob.ro_size != 0) { - while (vpd_decode_string(blob.ro_size, blob.ro_base, - &consumed, vpd_gets_callback, &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ - } - } + init_vpd_rdevs();
- if ((!arg.matched && region != VPD_RO) && blob.rw_size != 0) { - while (vpd_decode_string(blob.rw_size, blob.rw_base, - &consumed, vpd_gets_callback, &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ - } - } + if (region != VPD_RW) + vpd_find_in(&ro_vpd, &arg); + + if (!arg.matched && region != VPD_RO) + vpd_find_in(&rw_vpd, &arg);
if (!arg.matched) return NULL; @@ -223,3 +269,5 @@ } else return false; } + +ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h index a786625..d54cef8 100644 --- a/src/drivers/vpd/vpd.h +++ b/src/drivers/vpd/vpd.h @@ -13,29 +13,6 @@ VPD_RW = 2 };
-/* VPD 2.0 data blob structure */ -struct vpd_blob { - bool initialized; - uint8_t *ro_base; - uint32_t ro_size; - uint8_t *rw_base; - uint32_t rw_size; -}; -extern struct vpd_blob g_vpd_blob; - -/* - * This function loads g_vpd_blob global variable. - * The variable is initialized if it was not. - */ -const struct vpd_blob *vpd_load_blob(void); - -/* - * This function gets the base address and size of - * buffers for RO_VPD/RW_VPD binary blobs, and sets - * the struct. - */ -void vpd_get_buffers(struct vpd_blob *blob); - /* * Reads VPD string value by key. * diff --git a/src/drivers/vpd/vpd_cbmem.c b/src/drivers/vpd/vpd_cbmem.c deleted file mode 100644 index eadda52..0000000 --- a/src/drivers/vpd/vpd_cbmem.c +++ /dev/null @@ -1,81 +0,0 @@ -/* SPDX-License-Identifier: BSD-3-Clause */ - -#include <console/console.h> -#include <cbmem.h> -#include <fmap.h> -#include <string.h> -#include <timestamp.h> - -#include "vpd_tables.h" -#include "vpd.h" - -/* Currently we only support Google VPD 2.0, which has a fixed offset. */ -enum { - CROSVPD_CBMEM_MAGIC = 0x43524f53, - CROSVPD_CBMEM_VERSION = 0x0001, -}; - -struct vpd_cbmem { - uint32_t magic; - uint32_t version; - uint32_t ro_size; - uint32_t rw_size; - uint8_t blob[0]; - /* The blob contains both RO and RW data. It starts with RO (0 .. - * ro_size) and then RW (ro_size .. ro_size+rw_size). - */ -}; - -static void cbmem_add_cros_vpd(int is_recovery) -{ - struct vpd_cbmem *cbmem; - const struct vpd_blob *blob; - - timestamp_add_now(TS_START_COPYVPD); - - blob = vpd_load_blob(); - - /* Return if no VPD at all */ - if (blob->ro_size == 0 && blob->rw_size == 0) - return; - - cbmem = cbmem_add(CBMEM_ID_VPD, sizeof(*cbmem) + blob->ro_size + - blob->rw_size); - if (!cbmem) { - printk(BIOS_ERR, "%s: Failed to allocate CBMEM (%u+%u).\n", - __func__, blob->ro_size, blob->rw_size); - return; - } - - cbmem->magic = CROSVPD_CBMEM_MAGIC; - cbmem->version = CROSVPD_CBMEM_VERSION; - cbmem->ro_size = blob->ro_size; - cbmem->rw_size = blob->rw_size; - - if (blob->ro_size) { - memcpy(cbmem->blob, blob->ro_base, blob->ro_size); - timestamp_add_now(TS_END_COPYVPD_RO); - } - - if (blob->rw_size) { - memcpy(cbmem->blob + blob->ro_size, blob->rw_base, - blob->rw_size); - timestamp_add_now(TS_END_COPYVPD_RW); - } -} - -void vpd_get_buffers(struct vpd_blob *blob) -{ - const struct vpd_cbmem *vpd; - - vpd = cbmem_find(CBMEM_ID_VPD); - if (!vpd || !vpd->ro_size) - return; - - blob->ro_base = (void *)vpd->blob; - blob->ro_size = vpd->ro_size; - blob->rw_base = (void *)vpd->blob + vpd->ro_size; - blob->rw_size = vpd->rw_size; -} - -RAMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) diff --git a/src/drivers/vpd/vpd_premem.c b/src/drivers/vpd/vpd_premem.c deleted file mode 100644 index 641bf89..0000000 --- a/src/drivers/vpd/vpd_premem.c +++ /dev/null @@ -1,13 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <string.h> - -#include "vpd.h" - -void vpd_get_buffers(struct vpd_blob *blob) -{ - const struct vpd_blob *b; - - b = vpd_load_blob(); - memcpy(blob, b, sizeof(*b)); -}
-
build bot (Jenkins) (Code Review) -
Hung-Te Lin (Code Review)
-
Jonathan Zhang (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)