Hello Martin Roth, Marc Jones, Johnny Lin, Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45774
to review the following change.
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
[RFC] Introduce vpd_get_option()
Allows to read options from VPD regions in flash. This is currently very limited as type information is missing both in the get_option() interface and the backend. Hence, every option to be queried needs to be added manually to vpd_get_option().
Storing options in VPD should only be an interim solution, VPD doesn't seem to be meant to be used like that. The lack of typing also makes this a bit ugly. But this way, we can keep that local.
Change-Id: I825388f767d1d87f44923eff5ae6680105fa801e Signed-off-by: Nico Huber nico.h@gmx.de --- M src/Kconfig M src/drivers/vpd/vpd.c M src/drivers/vpd/vpd.h M src/include/option.h 4 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45774/1
diff --git a/src/Kconfig b/src/Kconfig index 40310bb..40e275b 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -135,6 +135,46 @@ every boot. Use this if you want the NVRAM configuration to never be modified from its default values.
+config USE_VPD_OPTIONS + bool "Use VPD for configuration values" + depends on VPD + help + Enable this option if coreboot shall read options from the VPD + regions in flash. + This is currently very limited as type information is missing + both in the get_option() interface and the backend. Hence, every + option to be queried needs to be added manually to vpd_get_option(). + +if USE_VPD_OPTIONS + +choice + prompt "VPD region to query" + default VPD_OPTION_REGION_RW_THEN_RO + +config VPD_OPTION_REGION_RO + bool "RO" + +config VPD_OPTION_REGION_RW + bool "RW" + +config VPD_OPTION_REGION_RO_THEN_RW + bool "RO, then RW" + +config VPD_OPTION_REGION_RW_THEN_RO + bool "RW, then RO" + +endchoice + +# Keep in sync with `drivers/vpd/vpd.h`. +config VPD_OPTION_REGION + int + default 3 if VPD_OPTION_REGION_RW_THEN_RO + default 2 if VPD_OPTION_REGION_RO_THEN_RW + default 1 if VPD_OPTION_REGION_RW + default 0 + +endif # USE_VPD_OPTIONS + config USE_MAINBOARD_OPTIONS bool help diff --git a/src/drivers/vpd/vpd.c b/src/drivers/vpd/vpd.c index d3ff370..727a616 100644 --- a/src/drivers/vpd/vpd.c +++ b/src/drivers/vpd/vpd.c @@ -5,6 +5,7 @@ #include <cbmem.h> #include <ctype.h> #include <fmap.h> +#include <option.h> #include <program_loading.h> #include <string.h> #include <timestamp.h> @@ -297,4 +298,17 @@ return true; }
+enum cb_err vpd_get_option(void *const dest, const char *const name) +{ + if (strcmp(name, "debug_level") == 0) { + int i; + if (vpd_get_int("coreboot_log_level", CONFIG_VPD_OPTION_REGION, &i)) { + *(char *)dest = (char)i; + return CB_SUCCESS; + } + } + + return CB_CMOS_OPTION_NOT_FOUND; +} + ROMSTAGE_CBMEM_INIT_HOOK(cbmem_add_cros_vpd) diff --git a/src/drivers/vpd/vpd.h b/src/drivers/vpd/vpd.h index 817867a..5c2a80f 100644 --- a/src/drivers/vpd/vpd.h +++ b/src/drivers/vpd/vpd.h @@ -7,6 +7,7 @@
#define GOOGLE_VPD_2_0_OFFSET 0x600
+/* Keep in sync with Kconfig VPD_OPTION_REGION. */ enum vpd_region { VPD_RO, VPD_RW, diff --git a/src/include/option.h b/src/include/option.h index 86c8ff1..2822bcb 100644 --- a/src/include/option.h +++ b/src/include/option.h @@ -11,6 +11,7 @@ enum cb_err cmos_get_option(void *dest, const char *name);
enum cb_err mainboard_get_option(void *dest, const char *name); +enum cb_err vpd_get_option(void *dest, const char *name);
static inline enum cb_err set_option(const char *name, void *val) { @@ -26,6 +27,8 @@
if (CONFIG(USE_OPTION_TABLE)) ret = cmos_get_option(dest, name); + if (CONFIG(USE_VPD_OPTIONS) && ret != CB_SUCCESS) + ret = vpd_get_option(dest, name); if (CONFIG(USE_MAINBOARD_OPTIONS) && ret != CB_SUCCESS) ret = mainboard_get_option(dest, name);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 1: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig@146 PS3, Line 146: option to be queried needs to be added manually to vpd_get_option(). I still maintain that using this to control the console is generally not a good idea. Maybe this help text should warn that once you enable this option, you will re-read the whole VPD from flash in every stage and any kind of FMAP or flash access problem can result in the device hanging before you see anything on the console?
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h@10 PS3, Line 10: /* Keep in sync with Kconfig VPD_OPTION_REGION. */ Don't you just want to make a little function with
if (CONFIG(VPD_OPTION_REGION_RO)) return VPD_RO; if (CONFIG(VPD_OPTION_REGION_RW)) return VPD_RW; ...
? I think that would be much cleaner that trying to manually stay in sync with some enum.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c@306 PS3, Line 306: char Why is this `char`? init_log_level() is passing a pointer to `int`. (In fact, I think you could pass `dest` straight through to vpd_get_int() if you want...)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
(3 comments)
Thanks for your thoughts, Julius.
This was mostly a teaser how a clean API could look like. It doesn't seem that there is much interest. Beside some "we can do this later" and rebasing my patches in a broken state, there wasn't any feedback from the stakeholders.
So I'll likely abandon this in the next fex days if nobody takes over.
I suggest that we discuss the whole topic on the mailing list. There are some general questions like should the bootblock console be configurable? and should we make VPD available for such settings or keep it downstream?
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig@146 PS3, Line 146: option to be queried needs to be added manually to vpd_get_option().
I still maintain that using this to control the console is generally not a good idea. […]
I completely agree. But people asked to use VPD for it... I'm not sure if we should deny it.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h@10 PS3, Line 10: /* Keep in sync with Kconfig VPD_OPTION_REGION. */
Don't you just want to make a little function with […]
Good idea, thank you.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c@306 PS3, Line 306: char
Why is this `char`? init_log_level() is passing a pointer to `int`. […]
Oh, that's part of the heavily broken traditional get_option() API. There is no length given, hence it is decided based on `name`. And for `debug_level` that always was a single byte.
Any, yes, init_log_level() only works because the more significant bytes were always initialize to zero and we run it on little endian.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig@146 PS3, Line 146: option to be queried needs to be added manually to vpd_get_option().
I completely agree. But people asked to use VPD for it... I'm not sure if […]
We need a way to enable image behavior customization at boot time. VPD may not seem to be ideal, but it is still more feature rich than other coreboot options. Unless other coreboot options reaches feature parity with VPD, VPD is still the best choice. Re-reading VPD from flash in every stage is okay for OCP servers. If there is FMAP or flash access problem, we would fall back to use default values.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3: Code-Review+1
Hi all,
This patch set is a great initial step, it as-is fulfills DeltaLake project's immediate needs. Once it is merged, some enhancements could be planned: a. An option name is associated with a data type, regardless of data source. Based on the data type, appropriate vpd_get_* can be used. get_option() takes option name as input, it looks up at a table to get the data type, pass the option name and data type to vpd_get_option(). vpd_get_option() calls appropriate vpd_get_* based on data type, to look up the option name. In other words, the vpd_get_option() design is very similar to cmos_get_option(). b. there are 6 existing options used in DeltaLake mainboard, they will be converted to above framework. c. For a specific option, there are several possible sources (cmos/cbfs/vpd/gpio/i2c, etc). For a mainboard, not all options may come from same source. A mainboard will have a table to enumerate which option comes from which source.
Thanks, Jonathan
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
a. An option name is associated with a data type, regardless of data source. Based on the data type, appropriate vpd_get_* can be used. get_option() takes option name as input, it looks up at a table to get the data type, pass the option name and data type to vpd_get_option(). vpd_get_option() calls appropriate vpd_get_* based on data type, to look up the option name. In other words, the vpd_get_option() design is very similar to cmos_get_option().
There is no general need for tables. The CMOS "option table" is only a table because CMOS space is so tight that we have to pack things. The current get_option() API is definitely at an impasse. Beside the lack of typing, it's dangerous because the caller has no way to pass the length of storage available.
We have all the type information in C code and should make use of it. That needs a new API, ofc. For instance, integer and enum values could simply be read with a get_int_option() function, booleans with a get_bool_option(). Well, for enums there could be look-up tables if one wants to store strings in the backend, for instance. But such information should be provided by the caller, not the backend.
c. For a specific option, there are several possible sources (cmos/cbfs/vpd/gpio/i2c, etc). For a mainboard, not all options may come from same source. A mainboard will have a table to enumerate which option comes from which source.
Sounds possible, but should be optional. Not everybody needs multiple sources. In the core I would keep things as simple as possible.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45774?usp=email )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Jonathan Zhang (Code Review)
-
Julius Werner (Code Review)
-
Martin L Roth (Code Review)
-
Nico Huber (Code Review)