Change in coreboot[master]: drivers/vpd: Set bool type variable matching with key/value pair
jonzhang@fb.com has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: Set bool type variable matching with key/value pair ......................................................................
drivers/vpd: Set bool type variable matching with key/value pair
Summary: Give a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Reviewers: dhendrix, hpe, anpetrov
Subscribers:
Tasks:
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h 3 files changed, 77 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/1
diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc index 17019b5..368024b 100644 --- a/src/drivers/vpd/Makefile.inc +++ b/src/drivers/vpd/Makefile.inc @@ -1,2 +1,2 @@ -romstage-$(CONFIG_VPD) += vpd_decode.c +romstage-$(CONFIG_VPD) += vpd_decode.c vpd_fsp.c ramstage-$(CONFIG_VPD) += vpd.c vpd_decode.c diff --git a/src/drivers/vpd/vpd_fsp.c b/src/drivers/vpd/vpd_fsp.c new file mode 100644 index 0000000..adff8e9 --- /dev/null +++ b/src/drivers/vpd/vpd_fsp.c @@ -0,0 +1,44 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (c) 2019 The coreboot Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + * + */ +#include <string.h> +#include "vpd_fsp.h" + +/* + * Process UPD variable of boolean type. + * Match the variable name with key name in the key/value pair, + * use the value to set *val. + * During the process, necessary checking is done, such as making + * sure the value length is 1, and value is either '1' or '0'. + */ +bool set_upd_bool(const char *upd_var, + const uint8_t *key, const int32_t key_len, + const uint8_t *value, const int32_t value_len, + uint8_t *val) +{ + int i; + /* Check key length and value length */ + if (key_len != strlen(upd_var) || value_len != 1) + return false; + + /* Matching key with variable name */ + for (i = 0; i < key_len; i++) { + if (key[i] != upd_var[i]) + return false; + } + + /* Make sure the value is either '1' or '0' */ + if (*value == '1') { + *val = 1; + return true; + } else if (*value == '0') { + *val = 0; + return true; + } else + return false; +} diff --git a/src/drivers/vpd/vpd_fsp.h b/src/drivers/vpd/vpd_fsp.h new file mode 100644 index 0000000..c59f249 --- /dev/null +++ b/src/drivers/vpd/vpd_fsp.h @@ -0,0 +1,32 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (c) 2019 The coreboot Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + * + */ + +#ifndef __VPD_FSP__ +#define __VPD_FSP__ + +#include <inttypes.h> + +#define GOOGLE_VPD_2_0_OFFSET 0x600 + +/* + * Process UPD variable of boolean type. + * + * Match the variable name with key name in the key/value pair, + * use the value to set *val. + * During the process, necessary checking is done, such as making + * sure the value length is 1, and value is either '1' or '0'. + * + * If there is a match and checking is successful, set *val + * accordingly, and return true; otherwise return false. + */ +bool set_upd_bool(const char *upd_var, + const uint8_t *key, const int32_t key_len, + const uint8_t *value, const int32_t value_len, + uint8_t *val); +#endif /* __VPD_FSP__ */
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: Set bool type variable matching with key/value pair ......................................................................
Patch Set 1:
Thanks for your review of https://review.coreboot.org/c/coreboot/+/34501. This patchset is a replacement.
The framework is now implemented in board code (mb/ocp/monolake). If there is a consensus to implement it in soc (soc/intel/fsp_broadwell_de) instead, I will do so.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#2).
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
drivers/vpd: set bool type variable matching with key/value pair
Summary: Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Reviewers: dhendrix, hpe, anpetrov
Subscribers:
Tasks:
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h 3 files changed, 77 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@2... PS2, Line 26: || value_len != 1 Why does value_len need to be passed in at all? It's not used elsewhere in this function. A check that value != NULL should be sufficient.
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@4... PS2, Line 42: else : return false; : } There's some controversy in the community about braces, but the current style guide dictates (basically the same as Linux) dictates that you should use braces in cases where there is more than one action per branch (https://doc.coreboot.org/coding_style.html?highlight=coding#placing-braces-a...).
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#3).
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
drivers/vpd: set bool type variable matching with key/value pair
Summary: Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Reviewers: dhendrix, hpe, anpetrov
Subscribers:
Tasks:
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h 3 files changed, 78 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/3
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 3:
(2 comments)
Patch Set 2: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@2... PS2, Line 26: || value_len != 1
Why does value_len need to be passed in at all? It's not used elsewhere in this function. […]
In case of binary type variable, it is good to check the value length; this will find out situation where "HyperThreading" is set to "12", instead of "1". This function set_upd_bool() is the first function for checking/setting a specific UPD data type, more will be added, and thus keeping the input parameters related to key/value the same makes sense. This group of function is called in the callback as defined in src/drivers/vpd/vpd_decode.h, which has those input parameters related to key/value pair: /* Callback for vpd_decode_string to invoke. */ typedef int vpd_decode_callback( const u8 *key, u32 key_len, const u8 *value, u32 value_len, void *arg);
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@4... PS2, Line 42: else : return false; : }
There's some controversy in the community about braces, but the current style guide dictates (basica […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 3:
I still don't understand why you need to add all those complicated routes. Seems like you are using the VPD format that is compatible with Google VPD 2.0, then why not just call vpd_gets?
Even if we want to add a , this still looks bloated.
If you want to add a simple wrapper for identifying 1/0 values a better implementation is:
bool vpd_get_bool(const char *key, bool default_value);
or
int vpd_get_int(const char *key, int *result); // returns -1 on failure so we can detect 'non-exist'.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 3:
and to clarify, I have gave a comment that you should move your implementation to a different module instead of modifying vpd_decode.c, because at that time I thought you're creating a different VPD format, but from latest implementation it seems not...
And it's still true we don't want people to modify vpd_decode.c since that's a drop from upstream :) it's fine to modify vpd.c if you can have a better utility function in accessing VPD entries.
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 3:
Patch Set 3:
I still don't understand why you need to add all those complicated routes. Seems like you are using the VPD format that is compatible with Google VPD 2.0, then why not just call vpd_gets?
Even if we want to add a , this still looks bloated.
If you want to add a simple wrapper for identifying 1/0 values a better implementation is:
bool vpd_get_bool(const char *key, bool default_value);
or
int vpd_get_int(const char *key, int *result); // returns -1 on failure so we can detect 'non-exist'.
The reason is that this functionality is used before FSP API is called. At that time, DRAM is not initialized, and thus the (more convenient) functions such as vpd_gets() as defined in src/drivers/vpd/vpd.h are not available. src/drivers/vpd/vpd.c is not built for romstage.
After the split of a single patch into a patch set, this design constraint looks less obvious. I will update the commit message to state such constraint explicitly. Thanks!
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#4).
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
drivers/vpd: set bool type variable matching with key/value pair
Summary: Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
This function is designed to be called before FSP is initialized. Otherwise, more efficient functions such as vpd_gets() should be used.
This function is normally called iteratively for key/value pairs. For performance reason, following checks are executed in order: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Reviewers: dhendrix, hpe, anpetrov
Subscribers:
Tasks:
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc A src/drivers/vpd/vpd_fsp.c A src/drivers/vpd/vpd_fsp.h 3 files changed, 78 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/4
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 4:
Patch Set 3:
and to clarify, I have gave a comment that you should move your implementation to a different module instead of modifying vpd_decode.c, because at that time I thought you're creating a different VPD format, but from latest implementation it seems not...
And it's still true we don't want people to modify vpd_decode.c since that's a drop from upstream :) it's fine to modify vpd.c if you can have a better utility function in accessing VPD entries.
Makes sense not to touch vpd_decode.c, thanks. For this use case (use VPD data to configure FSP), functions defined in vpd.c is not available.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 4:
I see, so the problem is to load VPD in romstage.
In that case I think the right thing to do is to refactor (or create a sub function) cbmem_add_cros_vpd so it can load and store vpd raw data in rom stage. Also, vpd_gets and other functions should call a function that returns pointer to loaded vpd data, which may be from CBMEM in ramstage, or a static pointer set previously in romstage.
If needed we may even create vpd_preram or vpd_romstage for that.
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 4:
Patch Set 4:
I see, so the problem is to load VPD in romstage.
In that case I think the right thing to do is to refactor (or create a sub function) cbmem_add_cros_vpd so it can load and store vpd raw data in rom stage. Also, vpd_gets and other functions should call a function that returns pointer to loaded vpd data, which may be from CBMEM in ramstage, or a static pointer set previously in romstage.
If needed we may even create vpd_preram or vpd_romstage for that.
I suppose we want to preserve vpd_gets() and vpd_find() call interface as-is to ensure there is no impact to current callers in ramstage. What about: a. create a vpd_romstage_gets() and vpd_romstage_find() call interface for romstage callers. b. factor common code in vpd.c out to be used by both romstage library calls and ramstage library calls. Let me know if this aligns with your thinking.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#5).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage. Refactored common code out from the framework of searching VPD in ramstage.
Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c A src/drivers/vpd/vpd_common.c A src/drivers/vpd/vpd_common.h A src/drivers/vpd/vpd_romstage.c A src/drivers/vpd/vpd_romstage.h 6 files changed, 338 insertions(+), 102 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 5:
If needed we may even create vpd_preram or vpd_romstage for that.
When I say this I was referring to vpd_preram.c or vpd_romstage.c, not functions.
a. create a vpd_romstage_gets() and vpd_romstage_find() call interface for romstage callers.
Probably no. I think your current version made it harder for people to figure out the right calling sequence.
We can have stage name in file level if needed, but better not in function name. It's better to keep similar interfaces, but if needed we may still add extra param. I'd recommend
1. Change vpd_cbmem to vpd_cache 2. Change cbmem_add_cros_vpd to vpd_load_cache 3. inside vpd_load_cache, have a way to decide allocating from CBMEM, or a static variable (using malloc).
In this way we can get almost everything unchanged and keep the only minimal difference in single place.
jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 5:
Patch Set 5:
If needed we may even create vpd_preram or vpd_romstage for that.
When I say this I was referring to vpd_preram.c or vpd_romstage.c, not functions.
Got it. this is what I did in patch set 5.
a. create a vpd_romstage_gets() and vpd_romstage_find() call interface for romstage callers.
Probably no. I think your current version made it harder for people to figure out the right calling sequence.
Not sure why patch set 5 made it harder for people to figure out the right calling sequence. Do you mean the caller who uses the vpd_romstage functions? Or do you mean people who develop vpd_romstage functions and vpd_common functions? With patch set 5, the calling sequencer is the same as that of vpd.
We can have stage name in file level if needed, but better not in function name.
I would think having different function names helps code read-ability. But if you desire, I will change.
It's better to keep similar interfaces, but if needed we may still add extra param. I'd recommend
- Change vpd_cbmem to vpd_cache
- Change cbmem_add_cros_vpd to vpd_load_cache
I wonder if this is an over kill. In romstage, VPD data is accessed directly through memory mapped flash. All is needed for accessing is the base address and size. I am not sure if we want to add a vpd_cache structure to hold such info.
- inside vpd_load_cache, have a way to decide allocating from CBMEM, or a static variable (using malloc).
vpd_load_cache() is only for romstage, isn't it? In this case, it will not allocate from CBMEM.
In this way we can get almost everything unchanged and keep the only minimal difference in single place.
With patch set 5, the vpd.c is quite small. The majority code is to construct CB_MEM. Very light logic is in vpd.c for run time operations. Do we need to further reduce the logic?
Thanks for the review!
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#6).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage. Refactored common code out from the framework of searching VPD in ramstage.
Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c A src/drivers/vpd/vpd_common.c A src/drivers/vpd/vpd_common.h A src/drivers/vpd/vpd_romstage.c A src/drivers/vpd/vpd_romstage.h 6 files changed, 337 insertions(+), 102 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/6
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#7).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage. Refactored common code out from the framework of searching VPD in ramstage.
Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c A src/drivers/vpd/vpd_common.c A src/drivers/vpd/vpd_common.h A src/drivers/vpd/vpd_romstage.c A src/drivers/vpd/vpd_romstage.h 6 files changed, 337 insertions(+), 102 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/7
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#8).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage. Refactored common code out from the framework of searching VPD in ramstage.
Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match.
Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c A src/drivers/vpd/vpd_common.c A src/drivers/vpd/vpd_common.h A src/drivers/vpd/vpd_romstage.c A src/drivers/vpd/vpd_romstage.h 6 files changed, 336 insertions(+), 102 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/8
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 8:
(3 comments)
This is much better now, but I still think it'll be even better if we can have vpd_gets and vpd_getbool in same usage for both RAM stage and ROM stage. They can then live in a shared (maybe vpd_common) place.
In fact, from your current implementation I wonder if it'll make more sense to have
vpd.c // common utils, which based on a function like vpd_load_blob(type) vpd_cbmem.c // fetch and build cache in CBMEM, providing vpd_load_blob from CBMEM vpd_romstage.c, or vpd_nocache.c // providing vpd_load_blob directly
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... File src/drivers/vpd/vpd_common.c:
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 95: vpd_find_common maybe rename to vpd_find_from_blob, which is more descriptive than find_common.
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 116: bufcpy we don't encourage acronyms in naming unless if it's very well known/trivial.
Try something like
vpd_copy_buffer or vpd_copy_string.
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 122: if (size > (string_size + 1)) { what about
int copy_size = MIN(size - 1, string_size); memcpy(buffer, string_addr, copy_size); buffer[copy_size] = '\0';
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 8:
Patch Set 8:
(3 comments)
This is much better now, but I still think it'll be even better if we can have vpd_gets and vpd_getbool in same usage for both RAM stage and ROM stage. They can then live in a shared (maybe vpd_common) place.
In fact, from your current implementation I wonder if it'll make more sense to have
vpd.c // common utils, which based on a function like vpd_load_blob(type) vpd_cbmem.c // fetch and build cache in CBMEM, providing vpd_load_blob from CBMEM vpd_romstage.c, or vpd_nocache.c // providing vpd_load_blob directly
Thanks Hung-Te. I will take care of the 3 specific comments, and I am working on a version using CAR_GLOBAL to store blobs' base address and size.
On the other hand, my understanding of your general comments is that you are thinking about a different structure from the current design. With your design, vpd.c is linked to both stages, and provides library functions to both stages. In this case, build time #ifdef ENV_ROMSTAGE (or ENV_RAMSTAGE) is needed, isn't it? For example, RAMSTAGE_CBMEM_INIT_HOOK statement should only be built into ramstage; in vpd_gets() and vpd_find(), some different actions are needed for ramstage and for romstage. Is this preferred?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 8:
On the other hand, my understanding of your general comments is that you are thinking about a different structure from the current design. With your design, vpd.c is linked to both stages, and provides library functions to both stages. In this case, build time #ifdef ENV_ROMSTAGE (or ENV_RAMSTAGE) is needed, isn't it? For example, RAMSTAGE_CBMEM_INIT_HOOK statement should only be built into ramstage; in vpd_gets() and vpd_find(), some different actions are needed for ramstage and for romstage. Is this preferred?
No. As said, I think CBMEM specific (RAMSTAGE_CBMEM_INIT_HOOK) should go vpd_cbmem.c. vpd_gets and vpd_find should have the action abstracted into a function, that is provided by vpd_cbmem.c and/or cbmem_preram.c. For example,
// vpd.c, vpd_gets
void *buffer = vpd_get_buffer(vpd_type, &size); ....
// vpd_cbmem.c void *vpd_get_buffer(enum type, size_t *size) { // read data from CBMEM ... }
// vpd_preram.c void *vpd_get_buffer(enum type, size_t *size) { // use static buffer, or re-read if needed. }
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#9).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_lib.c A src/drivers/vpd/vpd_lib.h A src/drivers/vpd/vpd_premem.c A src/drivers/vpd/vpd_premem.h 8 files changed, 395 insertions(+), 164 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 57: blob->ro_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd)); line over 96 characters
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 68: blob->rw_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd)); line over 96 characters
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#10).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_lib.c A src/drivers/vpd/vpd_lib.h A src/drivers/vpd/vpd_premem.c A src/drivers/vpd/vpd_premem.h 8 files changed, 397 insertions(+), 164 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/10
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... File src/drivers/vpd/vpd_common.c:
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 95: vpd_find_common
maybe rename to vpd_find_from_blob, which is more descriptive than find_common.
Done
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 116: bufcpy
we don't encourage acronyms in naming unless if it's very well known/trivial. […]
Done
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 122: if (size > (string_size + 1)) {
what about […]
Done
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 10:
(4 comments)
Just a few drive-by comments...
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h@7 PS10, Line 7: #include "vpd_lib.h" This should go below the header guard
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c File src/drivers/vpd/vpd_lib.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 8: * : * SPDX-License-Identifier:.*GPL-2.0-or-later We're not using SPDX identifiers for coreboot.
Actually, there was a recent poll where we decides to do away with copyright lines all together: https://civs.cs.cornell.edu/cgi-bin/results.pl?id=E_9e4f5ea789b9ceb9
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 67: printk(BIOS_CRIT, "binary blob size:%d\n", size); Seems like a debug print that got left in by accident?
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 42: blob->ro_base = NULL; : blob->ro_size = -1; : blob->rw_base = NULL; : blob->rw_size = -1; Since these aren't actually used (only assigned), you can remove these lines. At the very least, the blob->ro_size and blob->rw_size assignments should probably go - Even though it's valid, I suspect it will throw compiler warnings which could lead to a failure depending on how pedantic the build rules are set up.
I noticed in vpd_cbmem.c, the equivalent ro_size and rw_size members of the cbmem struct are set to 0, so it might be best to just do that here as well to be consistent.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 10:
(10 comments)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@107 PS10, Line 107: void why not just do 'const char *value' here so we don't need to re-cast later?
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: vpd_find_romstage vpd_find
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: return returning
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@119 PS10, Line 119: char const char
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c File src/drivers/vpd/vpd_lib.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 23: get_vpd_size can we just move this to vpd.c ?
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 35: no VPD at all? nothing to do then Return if no VPD at all.
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 36: (ro_vpd_size == 0) && (rw_vpd_size == 0) if (a == b && c == d)
(no need to quote since == has higher precedence.
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 49: if (ro_vpd_size != 0) { if (ro_vpd_size) {
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 57: sizeof(struct google_vpd_info) + : rdev_mmap_full(&vpd) not a big deal, but we usually write (ptr + number) instead of (number + ptr).
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 61: if (rw_vpd_size != 0) { if (rw_vpd_size) {
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return; if we're already using car, why not just make calling vpd_load_blob automatically inside vpd_get_buffers? Then callers don't need to call vpd_load_blob explicitly, more like ram stage.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#11).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c A src/drivers/vpd/vpd_premem.h 6 files changed, 324 insertions(+), 120 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/11
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 11:
(20 comments)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h@7 PS10, Line 7: #include "vpd_lib.h"
This should go below the header guard
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@107 PS10, Line 107: void
why not just do 'const char *value' here so we don't need to re-cast later?
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: return
returning
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@111 PS10, Line 111: vpd_find_romstage
vpd_find
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.c@119 PS10, Line 119: char
const char
Done
https://review.coreboot.org/c/coreboot/+/34634/6/src/drivers/vpd/vpd_common.... File src/drivers/vpd/vpd_common.c:
https://review.coreboot.org/c/coreboot/+/34634/6/src/drivers/vpd/vpd_common.... PS6, Line 68: const uint8_t *value, int32_t value_len, void *arg)
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@2... PS2, Line 26: || value_len != 1
In case of binary type variable, it is good to check the value length; this will find out situation […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@4... PS2, Line 42: else : return false; : }
Done
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c File src/drivers/vpd/vpd_lib.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 8: * : * SPDX-License-Identifier:.*GPL-2.0-or-later
We're not using SPDX identifiers for coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 23: get_vpd_size
can we just move this to vpd. […]
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 67: printk(BIOS_CRIT, "binary blob size:%d\n", size);
Seems like a debug print that got left in by accident?
Ack
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 57: blob->ro_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd));
line over 96 characters
Ack
https://review.coreboot.org/c/coreboot/+/34634/9/src/drivers/vpd/vpd_premem.... PS9, Line 68: blob->rw_base = (uint8_t *)(sizeof(struct google_vpd_info) + rdev_mmap_full(&vpd));
line over 96 characters
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 35: no VPD at all? nothing to do then
Return if no VPD at all.
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 36: (ro_vpd_size == 0) && (rw_vpd_size == 0)
if (a == b && c == d) […]
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 42: blob->ro_base = NULL; : blob->ro_size = -1; : blob->rw_base = NULL; : blob->rw_size = -1;
Since these aren't actually used (only assigned), you can remove these lines. […]
Thanks David. These statements initialize global static variable, so it is better to have them. Now ro_size and rw_size are initialized to 0 instead of -1, to be consistent with vpd_cbmem.c. Good point.
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 49: if (ro_vpd_size != 0) {
if (ro_vpd_size) {
Ack
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 57: sizeof(struct google_vpd_info) + : rdev_mmap_full(&vpd)
not a big deal, but we usually write (ptr + number) instead of (number + ptr).
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 61: if (rw_vpd_size != 0) {
if (rw_vpd_size) {
Done
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return;
if we're already using car, why not just make calling vpd_load_blob automatically inside vpd_get_buf […]
vpd_load_blob() needs to be called only once during boot time, similar to "RAMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd)".
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return;
vpd_load_blob() needs to be called only once during boot time, similar to "RAMSTAGE_CBMEM_INIT_HOOK( […]
yes but I'm not sure why it can't be done automatically inside vpd_get_buffers. In fact, unless if you'd really want to pass same VPD buffer between different pre-ram stages (decompress, bootblock, romstage, verstage), I see no reason why we can't simply declare two static vars for it:
static bool initialized; static struct vpd_blob blob;
if (initialized) return &blob;
initialized = true; vpd_get_blob(&blob); return &blob;
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return;
yes but I'm not sure why it can't be done automatically inside vpd_get_buffers. […]
In fact, I'm an expert in how x86 CAR works or needs (this is not required for ARM), especially if it'll share data across stages. If it won't share data, maybe we can combine the functions so that
- vpd.c: vpd_load_blob, that always reads and stores into the CAR variable. - vpd_cbmem: calls vpd_load_blob and save into a specific CBMEM page
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#12).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 287 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/12
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 78: struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); : if (!blob) : return;
In fact, I'm an expert in how x86 CAR works or needs (this is not required for ARM), especially if i […]
Thanks Hung-Te. This makes total sense. I added "initialized" variable to the structure, and combined the functions. I do not have a Chromebook to test with, I could use some help here.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 12:
(5 comments)
Thanks! I think this is pretty close to what I'd expect now. Just few more nits so we can make the interface more clean:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@32 PS12, Line 32: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size If 'struct vpd_blob' is already declared also in header file, I think we should just replace this by vpd_blob., i.e,
void vpd_load_blob(struct vpd_blob *blob);
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@35 PS12, Line 35: /* : * returns the size of data in a VPD 2.0 formatted fmap region, or 0. : * Also sets *base as the region's base address. : */ : int32_t get_vpd_size(const char *fmap_name, int32_t *base); why do we need to publish this function?
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@45 PS12, Line 45: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size ... so vpd_get_buffers may use 'struct vpd_blob' as well.
If we can use static variable in pre-ram, this can even be
const struct vpd_blob *vpd_get_blobs(void);
And the caller can simply call it to figure out the buffer information.
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@52 PS12, Line 52: for consistency we want to keep <=80 columns.
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@89 PS12, Line 89: if (blob->initialized == false) : return; so who's going to update blob->initialized?
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#13).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 244 insertions(+), 95 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34634/13/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/13/src/drivers/vpd/vpd.c@122 PS13, Line 122: trailing whitespace
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#14).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 244 insertions(+), 95 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/14
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#15).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 95 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/15
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 15:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@32 PS12, Line 32: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size
If 'struct vpd_blob' is already declared also in header file, I think we should just replace this by […]
Done
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@35 PS12, Line 35: /* : * returns the size of data in a VPD 2.0 formatted fmap region, or 0. : * Also sets *base as the region's base address. : */ : int32_t get_vpd_size(const char *fmap_name, int32_t *base);
why do we need to publish this function?
Done
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.h@45 PS12, Line 45: void **ro_base, uint32_t *ro_size, : void **rw_base, uint32_t *rw_size
... so vpd_get_buffers may use 'struct vpd_blob' as well. […]
Done
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@52 PS12, Line 52:
for consistency we want to keep <=80 columns.
Done
https://review.coreboot.org/c/coreboot/+/34634/12/src/drivers/vpd/vpd.c@89 PS12, Line 89: if (blob->initialized == false) : return;
so who's going to update blob->initialized?
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h@26 PS15, Line 26: extern struct vpd_blob g_vpd_blob; : : /* : * This function loads g_vpd_blob CAR_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); Thanks for all the effort here. I think we're pretty close!
From my last comment in c#12:
If we can use static variable in pre-ram, this can even be const struct vpd_blob *vpd_get_blobs(void); And the caller can simply call it to figure out the buffer information.
This seems not really used in the end. Is there a reason for that?
I'm not super familiar with CAR (it's actually dummy for ARM) so maybe you can help me to justify why CAR is needed:
1. Does the CAR_GLOBAL help to share same vpd_blob data between bootblock/romstage / even ram stage? (i.e., so we don't need to read and initialize twice?
2. Callers should use vpd_get_buffers, so I'm not sure we need to put g_vpd_blob as public/extern? Shouldn't it be a provider (vpd_get_buffer) specific info?
I think a more common design style is to have the interface in public and details (and variable) in the individual module file, i.e:
// vpd.h -- struct vpd_blob { ... }; /* load the blob from SPI directly into, only used internally by vpd driver */ int vpd_load_blob(struct vpd_blob *blob); /* finds cached (and load if haven't) VPD blobs */ const struct *vpd_blob vpd_get_buffers(void);
// vpd_preram static struct vpd_blob vpd_blob;
const struct *vpd_blob vpd_get_buffers(void) { if (!vpd_blob.initialized) vpd_load_blob(&vpd_blob); return &vpd_blob; }
// vpd_cbmem static struct *vpd_blob vpd_blob;
const struct *vpd_blob vpd_get_buffers(void) { if (!vpd_blob.initialized) { // find CBMEM // fill vpd_blob if CBMEM is available // set initialized } return &vpd_blob; }
If there're reasons we can't do this and must rely on CAR please feel free to let me know (and put that in the comments or description). Thanks!
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@106 PS15, Line 106: - remove the extra space
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@201 PS15, Line 201: size add a "assert(size > 0)" before calling the MIN.
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... File src/drivers/vpd/vpd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... PS15, Line 83: blob should we set blob->initialized to True ?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 15:
(4 comments)
Thank you Hung-Te for your patience!
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h@26 PS15, Line 26: extern struct vpd_blob g_vpd_blob; : : /* : * This function loads g_vpd_blob CAR_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);
Thanks for all the effort here. I think we're pretty close! […]
Hi Hung-Te, thanks for your review!
The reason I did not do "const struct vpd_blob *vpd_get_blobs(void);" is because static variable cannot be used in romstage. If I use static variable in vpd_premem.c, I get this: Forbidden global variables in romstage: ffffff00 b vpd_blob
Therefore CAR has to be relied on (for romstage). The current overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage before FSP-M execution, or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blobs are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a struct vpd_blob variable. ** The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. ** The variable gets contents obtained from CBMEM.
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@106 PS15, Line 106: -
remove the extra space
Done
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.c@201 PS15, Line 201: size
add a "assert(size > 0)" before calling the MIN.
Done
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... File src/drivers/vpd/vpd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... PS15, Line 83: blob
should we set blob->initialized to True ?
There is no need because initialized field is not necessary for the blob variable in stack. The blob variable is created when vpd_find() is called, and is discard after vpd_find() execution finishes; In other words, it has limited scope, it is fine not to set initialized field. This field needs to be set for CAR_GLOBAL variable, since in romstage, there is no similar way as RAMSTAGE_CBMEM_INIT_HOOK(), and initialized field is needed to tell whether the CAR_GLOBAL was ever initialized.
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#16).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/16
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#17).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The current overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/17
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#18).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/18
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#19).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/19
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#20).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 244 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/20
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 20:
Hi Philipp/Patrick R.,
I believe I encountered a bug or a setup issue with Gerrit. Following is the sequence: a. I submitted a new version of patch set. b. I edited the commit message in Gerrit. c. I noticed that the build failed: https://qa.coreboot.org/job/coreboot-gerrit/102331/ . d. I do not know how to trigger a new build, I tried different ways. In the end, I had to introduce a dummy change.
Thanks for looking into this.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 20:
Patch Set 20:
Hi Philipp/Patrick R.,
I believe I encountered a bug or a setup issue with Gerrit. Following is the sequence: a. I submitted a new version of patch set. b. I edited the commit message in Gerrit. c. I noticed that the build failed: https://qa.coreboot.org/job/coreboot-gerrit/102331/ . d. I do not know how to trigger a new build, I tried different ways. In the end, I had to introduce a dummy change.
Thanks for looking into this.
What happened is that you updated the patch set (from PS16 to PS17) while PS16 was still being built. The system then killed the build (the log says "Aborted by new patch set.").
That repeated for patch sets 17-19, only patch set 20 got the time to complete the build.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 20: Code-Review+1
(1 comment)
Just some nits, and thanks for all the effort.
It's so unfortunately the cache as SRAM is still not-easy-to-use on x86 and I think that's probably the best approach we may do.
https://review.coreboot.org/c/coreboot/+/34634/20/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/20/src/drivers/vpd/vpd_premem... PS20, Line 26: struct vpd_blob I'd prefer to use '*b' instead.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 20:
(1 comment)
Patch Set 20:
Patch Set 20:
Hi Philipp/Patrick R.,
I believe I encountered a bug or a setup issue with Gerrit. Following is the sequence: a. I submitted a new version of patch set. b. I edited the commit message in Gerrit. c. I noticed that the build failed: https://qa.coreboot.org/job/coreboot-gerrit/102331/ . d. I do not know how to trigger a new build, I tried different ways. In the end, I had to introduce a dummy change.
Thanks for looking into this.
What happened is that you updated the patch set (from PS16 to PS17) while PS16 was still being built. The system then killed the build (the log says "Aborted by new patch set.").
That repeated for patch sets 17-19, only patch set 20 got the time to complete the build.
Thanks Patric for looking at this. I will remember not to update commit message while a system validation is in progress.
Cheers, Jonathan
https://review.coreboot.org/c/coreboot/+/34634/20/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/20/src/drivers/vpd/vpd_premem... PS20, Line 26: struct vpd_blob
I'd prefer to use '*b' instead.
Thanks! This makes sense.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 20:
(2 comments)
Thanks Hung-Te for the feedbacks! I am resolving the discussions, and made a change per your great comment.
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd.h@26 PS15, Line 26: extern struct vpd_blob g_vpd_blob; : : /* : * This function loads g_vpd_blob CAR_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);
Hi Hung-Te, thanks for your review! […]
Done
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... File src/drivers/vpd/vpd_cbmem.c:
https://review.coreboot.org/c/coreboot/+/34634/15/src/drivers/vpd/vpd_cbmem.... PS15, Line 83: blob
There is no need because initialized field is not necessary for the blob variable in stack. […]
Done
Hello Andrey Petrov, Patrick Rudolph, Łukasz Siudut, Paul Menzel, Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Huang Jin, David Hendricks, Philipp Deppenwiese, Nico Huber, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34634
to look at the new patch set (#21).
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 --- M src/drivers/vpd/Makefile.inc M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/34634/21
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 21: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
drivers/vpd: add framework to search VPD in romstage
Summary: Added a framework to search VPD in romstage before memory is avilable. vpd_cbmem.c and vpd_premem.c are added for code specific for premem environment and for environment that cbmem can be used.
Since global variable is forbidden in romstage. A CAR_GLOBAL variable is defined in vpd.c. This variable holds VPD binary blobs' base address and size from memory mapped flash.
The overall flow is: * The CAR variable g_vpd_blob is initialized if it was not, either at romstage (before FSP-M execution in case of FSP UPD customization), or at ramstage. * At ramstage, during CBMEM_INIT, the VPD binary blob contents are copied into CBMEM. * At vpd_find() which may be called at romstage or at ramstage, it sets storage for a local struct vpd_blob variable. * The variable gets contents duplicated from g_vpd_blob, if vpd_find() is called at romstage. * The variable gets contents obtained from CBMEM, if vpd_find() is called at ramstage.
Added a call vpd_get_bool(). Given a key/value pair in VPD binary blob, and name of a bool type variable, set the variable value if there is a match. Several checks are in place: * The key/value length needs to be correct. * The key name needs to match. * THe value is either '1' or '0'.
Test Plan: * Build an OCP MonoLake coreboot image, flash and run.
Tags: Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: Iebdba59419a555147fc40391cf17cc6879d9e1b2 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34634 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 A src/drivers/vpd/vpd_cbmem.c A src/drivers/vpd/vpd_premem.c 5 files changed, 243 insertions(+), 93 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 17019b5..cc276e4 100644 --- a/src/drivers/vpd/Makefile.inc +++ b/src/drivers/vpd/Makefile.inc @@ -1,2 +1,2 @@ -romstage-$(CONFIG_VPD) += vpd_decode.c -ramstage-$(CONFIG_VPD) += vpd.c vpd_decode.c +romstage-$(CONFIG_VPD) += vpd_decode.c vpd_premem.c vpd.c +ramstage-$(CONFIG_VPD) += vpd_decode.c vpd_cbmem.c vpd.c diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c index c6dd339..10f5703 100644 --- a/src/drivers/vpd/vpd.c +++ b/src/drivers/vpd/vpd.c @@ -4,6 +4,8 @@ * found in the LICENSE file. */
+#include <arch/early_variables.h> +#include <assert.h> #include <console/console.h> #include <cbmem.h> #include <fmap.h> @@ -15,13 +17,6 @@ #include "vpd_decode.h" #include "vpd_tables.h"
-/* Currently we only support Google VPD 2.0, which has a fixed offset. */ -enum { - GOOGLE_VPD_2_0_OFFSET = 0x600, - CROSVPD_CBMEM_MAGIC = 0x43524f53, - CROSVPD_CBMEM_VERSION = 0x0001, -}; - struct vpd_gets_arg { const uint8_t *key; const uint8_t *value; @@ -29,18 +24,12 @@ int matched; };
-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_blob g_vpd_blob CAR_GLOBAL = {0};
-/* returns the size of data in a VPD 2.0 formatted fmap region, or 0 */ +/* + * returns the size of data in a VPD 2.0 formatted fmap region, or 0. + * Also sets *base as the region's base address. + */ static int32_t get_vpd_size(const char *fmap_name, int32_t *base) { struct google_vpd_info info; @@ -86,34 +75,26 @@ return size; }
-static void cbmem_add_cros_vpd(int is_recovery) +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); + + /* Return if no VPD at all */ + if (ro_vpd_size == 0 && rw_vpd_size == 0) + return; + + struct vpd_blob *blob = car_get_var_ptr(&g_vpd_blob); + if (!blob) + return; + blob->ro_base = NULL; + blob->ro_size = 0; + blob->rw_base = NULL; + blob->rw_size = 0; + struct region_device vpd; - struct vpd_cbmem *cbmem; - int32_t ro_vpd_base = 0, rw_vpd_base = 0; - int32_t ro_vpd_size, rw_vpd_size; - - timestamp_add_now(TS_START_COPYVPD); - - ro_vpd_size = get_vpd_size("RO_VPD", &ro_vpd_base); - rw_vpd_size = get_vpd_size("RW_VPD", &rw_vpd_base); - - /* no VPD at all? nothing to do then */ - if ((ro_vpd_size == 0) && (rw_vpd_size == 0)) - return; - - cbmem = cbmem_add(CBMEM_ID_VPD, sizeof(*cbmem) + ro_vpd_size + - rw_vpd_size); - if (!cbmem) { - printk(BIOS_ERR, "%s: Failed to allocate CBMEM (%u+%u).\n", - __func__, ro_vpd_size, rw_vpd_size); - return; - } - - cbmem->magic = CROSVPD_CBMEM_MAGIC; - cbmem->version = CROSVPD_CBMEM_VERSION; - cbmem->ro_size = 0; - cbmem->rw_size = 0;
if (ro_vpd_size) { if (fmap_locate_area_as_rdev("RO_VPD", &vpd)) { @@ -124,20 +105,10 @@ } rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - - - if (rdev_readat(&vpd, cbmem->blob, ro_vpd_base, ro_vpd_size) == - ro_vpd_size) { - cbmem->ro_size = ro_vpd_size; - } else { - printk(BIOS_ERR, - "%s: Reading RO_VPD FMAP section failed.\n", - __func__); - ro_vpd_size = 0; - } - timestamp_add_now(TS_END_COPYVPD_RO); + blob->ro_base = (uint8_t *)(rdev_mmap_full(&vpd) + + sizeof(struct google_vpd_info)); + blob->ro_size = ro_vpd_size; } - if (rw_vpd_size) { if (fmap_locate_area_as_rdev("RW_VPD", &vpd)) { /* shouldn't happen, but let's be extra defensive */ @@ -147,17 +118,23 @@ } rdev_chain(&vpd, &vpd, GOOGLE_VPD_2_0_OFFSET, region_device_sz(&vpd) - GOOGLE_VPD_2_0_OFFSET); - - if (rdev_readat(&vpd, cbmem->blob + ro_vpd_size, rw_vpd_base, - rw_vpd_size) == rw_vpd_size) { - cbmem->rw_size = rw_vpd_size; - } else { - printk(BIOS_ERR, - "%s: Reading RW_VPD FMAP section failed.\n", - __func__); - } - timestamp_add_now(TS_END_COPYVPD_RW); + blob->rw_base = (uint8_t *)(rdev_mmap_full(&vpd) + + sizeof(struct google_vpd_info)); + blob->rw_size = rw_vpd_size; } + blob->initialized = true; +} + +const struct vpd_blob *vpd_load_blob(void) +{ + struct vpd_blob *blob = NULL; + + blob = car_get_var_ptr(&g_vpd_blob); + + if (blob && blob->initialized == false) + vpd_get_blob(); + + return blob; }
static int vpd_gets_callback(const uint8_t *key, uint32_t key_len, @@ -179,30 +156,29 @@
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; - const struct vpd_cbmem *vpd; - - vpd = cbmem_find(CBMEM_ID_VPD); - if (!vpd || !vpd->ro_size) - return NULL;
arg.key = (const uint8_t *)key; arg.key_len = strlen(key);
- if (region == VPD_ANY || region == VPD_RO) { - while (vpd_decode_string( - vpd->ro_size, vpd->blob, &consumed, - vpd_gets_callback, &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ + 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. */ } } - if (!arg.matched && region != VPD_RO) { - while (vpd_decode_string( - vpd->rw_size, vpd->blob + vpd->ro_size, - &consumed, vpd_gets_callback, - &arg) == VPD_DECODE_OK) { - /* Iterate until found or no more entries. */ + + 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. */ } }
@@ -223,14 +199,40 @@ if (!string_address) return NULL;
- if (size > (string_size + 1)) { - memcpy(buffer, string_address, string_size); - buffer[string_size] = '\0'; - } else { - memcpy(buffer, string_address, size - 1); - buffer[size - 1] = '\0'; - } + assert(size > 0); + int copy_size = MIN(size - 1, string_size); + memcpy(buffer, string_address, copy_size); + buffer[copy_size] = '\0'; return buffer; }
-RAMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) +/* + * Find value of boolean type vpd key. + * + * During the process, necessary checking is done, such as making + * sure the value length is 1, and value is either '1' or '0'. + */ +bool vpd_get_bool(const char *key, enum vpd_region region, uint8_t *val) +{ + int size; + const char *value; + + value = vpd_find(key, &size, region); + if (!value) { + printk(BIOS_CRIT, "problem returning from vpd_find.\n"); + return false; + } + + if (size != 1) + return false; + + /* Make sure the value is either '1' or '0' */ + if (*value == '1') { + *val = 1; + return true; + } else if (*value == '0') { + *val = 0; + return true; + } else + return false; +} diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h index 6009b8b..14b002c 100644 --- a/src/drivers/vpd/vpd.h +++ b/src/drivers/vpd/vpd.h @@ -7,11 +7,37 @@ #ifndef __VPD_H__ #define __VPD_H__
+#define GOOGLE_VPD_2_0_OFFSET 0x600 + enum vpd_region { VPD_ANY = 0, VPD_RO = 1, 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 CAR_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. * @@ -39,4 +65,13 @@
const void *vpd_find(const char *key, int *size, enum vpd_region region);
+/* + * Find value of boolean type vpd key. + * + * During the process, necessary checking is done, such as making + * sure the value length is 1, and value is either '1' or '0'. + */ +bool vpd_get_bool(const char *key, enum vpd_region region, + uint8_t *val); + #endif /* __VPD_H__ */ diff --git a/src/drivers/vpd/vpd_cbmem.c b/src/drivers/vpd/vpd_cbmem.c new file mode 100644 index 0000000..5b68506 --- /dev/null +++ b/src/drivers/vpd/vpd_cbmem.c @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2014 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include <console/console.h> +#include <cbmem.h> +#include <fmap.h> +#include <stdlib.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 new file mode 100644 index 0000000..14e8032 --- /dev/null +++ b/src/drivers/vpd/vpd_premem.c @@ -0,0 +1,27 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (c) 2019 Facebook, Inc. + * + * 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. + */ + +#include <console/console.h> +#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) -
David Hendricks (Code Review)
-
Hung-Te Lin (Code Review)
-
Jonathan Zhang (Code Review)
-
Name of user not set (Code Review)
-
Patrick Georgi (Code Review)