Change in coreboot[master]: drivers/vpd: Add a config option that would override coreboot version...
Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
drivers/vpd: Add a config option that would override coreboot version with VPD variable
If VPD_COREBOOT_VERSION is selected, it would read VPD_RO variable to override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use VERSION_VPD_DEFAULT_IF_MISSING, if it's still not set it would use COREBOOT_VERSION. VPD_COREBOOT_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c 3 files changed, 49 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/1
diff --git a/src/drivers/vpd/Kconfig b/src/drivers/vpd/Kconfig index eda9130..566bec5 100644 --- a/src/drivers/vpd/Kconfig +++ b/src/drivers/vpd/Kconfig @@ -5,3 +5,27 @@ default n help Enable support for flash based vital product data. + +config VPD_COREBOOT_VERSION + bool "Read CONFIG_VERSION_VPD from VPD_RO for SMBIOS type 0 version" + default n + depends on VPD + help + After selecting this option, it would read CONFIG_VERSION_VPD from + VPD_RO and override SMBIOS type 0 version. + +config VERSION_VPD + string + default "version" + depends on VPD_COREBOOT_VERSION + help + The VPD variable name resides in VPD_RO for coreboot version, default + is "version". The value of the variable is limited to 20 characters for now. + +config VERSION_VPD_DEFAULT_IF_MISSING + string + default "" + depends on VPD_COREBOOT_VERSION + help + The fallback version value if VERSION_VPD cannot be read from VPD_RO, + if it's not set COREBOOT_VERSION would be used. diff --git a/src/drivers/vpd/Makefile.inc b/src/drivers/vpd/Makefile.inc index cc276e4..2237340 100644 --- a/src/drivers/vpd/Makefile.inc +++ b/src/drivers/vpd/Makefile.inc @@ -1,2 +1,3 @@ romstage-$(CONFIG_VPD) += vpd_decode.c vpd_premem.c vpd.c ramstage-$(CONFIG_VPD) += vpd_decode.c vpd_cbmem.c vpd.c +ramstage-$(CONFIG_VPD_COREBOOT_VERSION) += smbios.c diff --git a/src/drivers/vpd/smbios.c b/src/drivers/vpd/smbios.c new file mode 100644 index 0000000..682a4ce --- /dev/null +++ b/src/drivers/vpd/smbios.c @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ + +#include <console/console.h> +#include <string.h> +#include <smbios.h> +#include <version.h> + +#include "vpd.h" + +#define VER_LEN 20 +static char version[VER_LEN] = {0}; + +const char *smbios_mainboard_bios_version(void) +{ + if (vpd_gets(CONFIG_VERSION_VPD, version, VER_LEN, VPD_RO)) { + printk(BIOS_DEBUG, "Get version from VPD: %s\n", version); + return version; + } + printk(BIOS_ERR, "Get version from VPD failed\n"); + if (strlen(CONFIG_VERSION_VPD_DEFAULT_IF_MISSING)) + return CONFIG_VERSION_VPD_DEFAULT_IF_MISSING; + else + return coreboot_version; +}
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 1:
This is added for CB:41720.
Hello Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#2).
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
drivers/vpd: Add a config option that would override coreboot version with VPD variable
If VPD_COREBOOT_VERSION is selected, it would read VPD_RO variable to override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use VERSION_VPD_DEFAULT_IF_MISSING, if it's still not set it would use COREBOOT_VERSION. VPD_COREBOOT_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c 3 files changed, 49 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/2
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@15 PS2, Line 15: CONFIG_VERSION_VPD This should be checked to be a non-empty string
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: printk(BIOS_DEBUG, "Get version from VPD: %s\n", version); can you add the name of the VPD variable in the debug log?
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@19 PS2, Line 19: printk(BIOS_ERR, "Get version from VPD failed\n"); the name of the VPD variable would be useful here too
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@20 PS2, Line 20: if (strlen(CONFIG_VERSION_VPD_DEFAULT_IF_MISSING)) nit: for readability I'd consider the more explicit "strlen(..) != 0". I'm not sure if there's a preferred style about this though
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@21 PS2, Line 21: return CONFIG_VERSION_VPD_DEFAULT_IF_MISSING; another debug printk here and below would be helpful, something like:
printk("BIOS version set to CONFIG_VERSION_VPD_DEFAULT_IF_MISSING: '%s'", CONFIG_VERSION_VPD_DEFAULT_IF_MISSING)
and
printk("BIOS version set to coreboot version: '%s'", coreboot_version)
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@15 PS2, Line 15: VPD_RO and override SMBIOS type 0 version.
Selecting this option will read …
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@23 PS2, Line 23: is "version". The value of the variable is limited to 20 characters for now. Re-flow so both lines length are closer to each other.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@5 PS2, Line 5: #include <smbios.h> Sort the entries?
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: Get version from VPD: %s\n Just using present tense *Get* with the version from VPD printed is a little confusing to me. Maybe:
Firmware version from VPD: %s\n
or
Retrieved version from VPD: %s\n
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@22 PS2, Line 22: else The else is not needed, but it’s subjective.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@9 PS2, Line 9: VPD_COREBOOT_VERSION I would call this VPD_SMBIOS_VERSION since it's really more related to SMBIOS than to coreboot.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@12 PS2, Line 12: VPD && GENERATE_SMBIOS_TABLES
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@19 PS2, Line 19: default "version" Does this really need to be user configurable? Can't you just pick a slightly more expressive name (e.g. smbios_version or something) that would prevent clashes with other uses and work for everyone? (Just trying to limit config explosion here.)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@27 PS2, Line 27: default "" We already have CONFIG_LOCALVERSION, isn't that the same thing?
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@13 PS2, Line 13: smbios_mainboard_bios_version This is meant as an override by the mainboard, you can't just use it here (e.g. if you try to enable this option on a mainboard that also overrides this, you'll get multiple definition errors). You should handle this in smbios.c, e.g. make a shim function like
static const char *get_bios_version(void) { if (CONFIG(VPD_SMBIOS_VERSION)) { static char buffer[20]; if (vpd_gets("smbios_version", buffer, sizeof(buffer), VPD_RO)) return buffer; }
if (strlen(CONFIG_LOCALVERSION)) return CONFIG_LOCALVERSION;
if (smbios_mainboard_bios_version) return smbios_mainboard_bios_version();
return coreboot_version; }
and then use get_bios_version() in the code in that file where it was calling smbios_mainboard_bios_version() directly.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#3).
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
drivers/vpd: Add a config option that would override coreboot version with VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable to override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use CONFIG_LOCALVERSION, if it's still not set it would use COREBOOT_VERSION. VPD_SMBIOS_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c M src/drivers/vpd/vpd.h 4 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/3
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#4).
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
drivers/vpd: Add a function that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use CONFIG_LOCALVERSION, if it's still not set it would use COREBOOT_VERSION. VPD_SMBIOS_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c M src/drivers/vpd/vpd.h 4 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/4
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
Check and wait for similar change CB:32905
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4: Code-Review+1
This looks good to me
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
(2 comments)
The whole idea sounds pretty weird to me - we should not decide or report a "firmware version" by VPD - you should always have that version from the release BIOS code itself, otherwise it may be pretty hard to track "real" versions when the VPD is provisioned seperately.
That also makes me feel this is probably more like a board(family)-specific preference and no need to live in VPD drivers...
Having said that, since this is only adding a new function in a standalone C file, it's probably fine to merge this change.
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig@19 PS4, Line 19: version do you want to change the default name to coreboot_version or firmware_version or smbios_version?
it seems weird to have a generic term "version" without clarification.
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c@32 PS4, Line 32: BIOS SMBIOS ?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
That also makes me feel this is probably more like a board(family)-specific preference and no need to live in VPD drivers...
Agree that none of this should live in the VPD driver. If there's any real need for this it should live in smbios.c and just call the VPD functions from there, like CB:32905 was trying. But there were also good concerns raised with that patch that the existing ways to customize SMBIOS version should really be enough anyway.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
like CB:32905 was trying.
That one looks simple and good enough.
So I think there are two issues before moving on:
1. The commit message should tell more about the story of why CONFIG_LOCAL_VERSION is not enough and why reading version from VPD is needed.
2. If that really makes sense, we should decide the implementation - either CB:42029 or CB:32905.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
like CB:32905 was trying.
The story from that patch looks like they want to find a away for OS to read firmware version easily, without accessing SMBIOS. And that does not sound like a correct approach.
If Vboot is used, we have reserved few FMAP sections for version info: RO_FRID, RW_FWID_A, RW_FWID_B, and the build procedure will set correct strings when creating the firmware image. That's a better way if you want to manage version data, not hacking with VPD.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
The story from that patch looks like they want to find a away for OS to read firmware version easily, without accessing SMBIOS. And that does not sound like a correct approach.
Oh... I think I may have figured out what these two patches are planning to do.
To identify the version of a static firmware image (file), currently (if no vboot) there's no easy way. But if we put version in VPD, then you have the 'vpd' utility that can read (even set) from firmware. So they may have put the vpd stuff in the build procedure for that purpose.
But the VPD by design (although not strictly enforced) was expected to be empty in build procedure, and only provisioned when being manufactured. Moreover, if you use the reference updater (futility update), VPD will be preserved when updating BIOS, which means you'll not get the right version string.
IMHO a more correct implementation is:
1. If you use FMAP, define a version section explicitly, and fill the section in your build procedure where you can still access CONFIG_LOCAL_VERSION.
2. If FMAP is not an option, create a file like 'coreboot_version' and put that into CBFS may be easier, so you can still extract and read using cbfstool.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 4:
If you use FMAP, define a version section explicitly, and fill the section in your build procedure where you can still access CONFIG_LOCAL_VERSION.
If FMAP is not an option, create a file like 'coreboot_version' and put that into CBFS may be easier, so you can still extract and read using cbfstool.
Anyone who wants to do this, please check the existing discussions in CB:40376 and CB:40377 first, as there are some opinions about how best to do this. I still maintain (like explained there) that I think this should definitely be a CBFS file and not an FMAP section. Not sure if there was agreement about that in the end and Furquan just stopped working on it or the issue is still contentious.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#5).
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
drivers/vpd: Add a function that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use CONFIG_LOCALVERSION, if it's still not set it would use COREBOOT_VERSION.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without re-building from source. VPD_SMBIOS_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c M src/drivers/vpd/vpd.h 4 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/5
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#6).
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
drivers/vpd: Add a function that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use CONFIG_LOCALVERSION, if it's still not set it would use COREBOOT_VERSION.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without re-building from source. VPD_SMBIOS_VERSION default is n.
Tested on OCP Delta Lake, dmidecode -t 0 can see version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/drivers/vpd/Kconfig M src/drivers/vpd/Makefile.inc A src/drivers/vpd/smbios.c M src/drivers/vpd/vpd.h 4 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/6
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
(16 comments)
Resolve some comments, if it's better to move the function and Kconfig to x86/smbios.c will do. Updated the commit message to reflect the special usage for updating BIOS version w/o rebuilding from source, it's for internal build and test processes. Thanks for your suggestion for different approaches, wait for others opinions and decision.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@9 PS2, Line 9: VPD_COREBOOT_VERSION
I would call this VPD_SMBIOS_VERSION since it's really more related to SMBIOS than to coreboot.
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@12 PS2, Line 12: VPD
&& GENERATE_SMBIOS_TABLES
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@15 PS2, Line 15: VPD_RO and override SMBIOS type 0 version.
Selecting this option will read …
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@19 PS2, Line 19: default "version"
Does this really need to be user configurable? Can't you just pick a slightly more expressive name ( […]
Changed to firmware_version, I think using a config can provide more flexibility for internal build & test process.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@23 PS2, Line 23: is "version". The value of the variable is limited to 20 characters for now.
Re-flow so both lines length are closer to each other.
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@27 PS2, Line 27: default ""
We already have CONFIG_LOCALVERSION, isn't that the same thing?
Yes, use CONFIG_LOCALVERSION.
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig@19 PS4, Line 19: version
do you want to change the default name to coreboot_version or firmware_version or smbios_version? […]
Changed to firmware_version.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@5 PS2, Line 5: #include <smbios.h>
Sort the entries?
Do you mean sort the include files alphabetically? Just updated.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@15 PS2, Line 15: CONFIG_VERSION_VPD
This should be checked to be a non-empty string
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: Get version from VPD: %s\n
Just using present tense *Get* with the version from VPD printed is a little confusing to me. […]
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: printk(BIOS_DEBUG, "Get version from VPD: %s\n", version);
can you add the name of the VPD variable in the debug log?
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@19 PS2, Line 19: printk(BIOS_ERR, "Get version from VPD failed\n");
the name of the VPD variable would be useful here too
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@20 PS2, Line 20: if (strlen(CONFIG_VERSION_VPD_DEFAULT_IF_MISSING))
nit: for readability I'd consider the more explicit "strlen(..) != 0". […]
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@21 PS2, Line 21: return CONFIG_VERSION_VPD_DEFAULT_IF_MISSING;
another debug printk here and below would be helpful, something like: […]
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@22 PS2, Line 22: else
The else is not needed, but it’s subjective.
Done
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c@32 PS4, Line 32: BIOS
SMBIOS ?
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Updated the commit message to reflect the special usage for updating BIOS version w/o rebuilding from source, it's for internal build and test processes.
If this is simply for testing, I'd say cbfs file would be the better (and more correct) choice than VPD, since VPDs are supposed to be preserved (e.g., no update) during update while CBFS is not (will be updated).
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Patch Set 6:
Updated the commit message to reflect the special usage for updating BIOS version w/o rebuilding from source, it's for internal build and test processes.
If this is simply for testing, I'd say cbfs file would be the better (and more correct) choice than VPD, since VPDs are supposed to be preserved (e.g., no update) during update while CBFS is not (will be updated).
Our plan for VPD usage would be the place where we store the BIOS setting variables (e.g., turbo mode on/off, FSP UPD values .,etc). CB:41732 mentioned this, the RO_VPD would hold the default values and RW_VPD can be updated from user to overwrite the values under Linux. So it's likely we would update VPD for different versions.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Our plan for VPD usage would be the place where we store the BIOS setting variables (e.g., turbo mode on/off, FSP UPD values .,etc). CB:41732 mentioned this
VPD stands for Vital Product Data, and is where you put "device-specific data", especially those user will want to modify, or those only decided at manufacturing time (not firmware build time).
If you are making something for a whole family of firmware and won't be changed after firmware is flashed on device, that should be in mainboard Kconfig, or a CBFS file.
the RO_VPD would hold the default values and RW_VPD can be updated from user to overwrite the values under Linux.
Particular for that case (allowing user to turn on/off some settings) I think it may be fine to use VPD. That's the reasonable way to use it.
So it's likely we would update VPD for different versions.
That is the part I don't understand.
If you want to allow users overriding some settings, then you definitely can't update/overwrite VPD when updating firmware.
Also, for "version of firmware", that really doesn't sound like something user will want to override, or provisioned in manufacturing process.
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Patch Set 4:
Oh... I think I may have figured out what these two patches are planning to do.
To identify the version of a static firmware image (file), currently (if no vboot) there's no easy way. But if we put version in VPD, then you have the 'vpd' utility that can read (even set) from firmware. So they may have put the vpd stuff in the build procedure for that purpose.
That is correct: in our build process the firmware version is set after the original build. The reason is that we create release candidates with non-stable version numbers a coreboot-dirty-$HASH is fine for an RC). Then we test extensively the firmware image, and if everything is good we assign a "stable" version number (with a different numbering scheme from the non-stable ones).
But the VPD by design (although not strictly enforced) was expected to be empty in build procedure, and only provisioned when being manufactured. Moreover, if you use the reference updater (futility update), VPD will be preserved when updating BIOS, which means you'll not get the right version string.
IMHO a more correct implementation is:
If you use FMAP, define a version section explicitly, and fill the section in your build procedure where you can still access CONFIG_LOCAL_VERSION.
If FMAP is not an option, create a file like 'coreboot_version' and put that into CBFS may be easier, so you can still extract and read using cbfstool.
After reading your explanation on VPD, I strongly agree with your proposal #2: CBFS sounds like the right place where this information should go. Another reason to use VPD was that these variables are easily accessible via Linux's /sys/firmware/vpd , so no vpd tool is necessary. I was wondering if we should have a similar driver for Linux to access CBFS read-only, or if this is a bad idea.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Resolve some comments, if it's better to move the function and Kconfig to x86/smbios.c will do.
Yes, sorry if that wasn't clear, but I think our consensus is that CB:32905 is the better approach.
IMHO a more correct implementation is:
If you use FMAP, define a version section explicitly, and fill the section in your build procedure where you can still access CONFIG_LOCAL_VERSION.
If FMAP is not an option, create a file like 'coreboot_version' and put that into CBFS may be easier, so you can still extract and read using cbfstool.
After reading your explanation on VPD, I strongly agree with your proposal #2: CBFS sounds like the right place where this information should go. Another reason to use VPD was that these variables are easily accessible via Linux's /sys/firmware/vpd , so no vpd tool is necessary. I was wondering if we should have a similar driver for Linux to access CBFS read-only, or if this is a bad idea.
Well, no, I think if you *do* want to update the version number after the firmware image is finalized, putting it in VPD is probably the right thing after all. I guess this is a bit up to personal interpretation but I would consider the CBFS as the actual firmware (because all the stage binaries with the actual code are in there, and they're all mixed in together with other CBFS files) and the VPD as auxiliary data that can be modified independently. On Chrome OS, we use a hash of the whole CBFS to verify that our firmware code was unmodified, while the VPD can be written afterwards to adjust values that need to be different for each individual unit (e.g. serial number).
I still think it would be easier to just set the version number at build time instead. If you want to test different release candidates, why don't you just compile all your release candidates with the version number that your next official release is supposed to have, test them until you find one you're happy with, and then release that while throwing the others away? (While you're testing them you can still tell them apart by the other values in the 'revision' file in CBFS, e.g. Git SHA or build time.)
The /sys/firmware/vpd interface works by copying the whole VPD into memory and passing that to the kernel. Since CBFS is a lot bigger (and contains mostly stuff not interesting to the OS, like stage binaries), I don't think a similar interface makes sense for it. I think flashrom+cbfstool are the appropriate way to access it.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
That is correct: in our build process the firmware version is set after the original build. The reason is that we create release candidates with non-stable version numbers a coreboot-dirty-$HASH is fine for an RC). Then we test extensively the firmware image, and if everything is good we assign a "stable" version number (with a different numbering scheme from the non-stable ones).
If the stable is usually shorter than coreboot-dirty-$HASH, have you considered about applying a tag tool, that can replace a string in blob?
Another reason to use VPD was that these variables are easily accessible via Linux's /sys/firmware/vpd , so no vpd tool is necessary.
But if you are already dealing with smbios, dmidecode should give you same information by "sudo dmidecode -s bios-version".
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
Well, no, I think if you *do* want to update the version number after the firmware image is finalized, putting it in VPD is probably the right thing after all. I guess this is a bit up to personal interpretation but I would consider the CBFS as the actual firmware (because all the stage binaries with the actual code are in there, and they're all mixed in together with other CBFS files)
But shouldn't the "version" be part of actual firmware?
I still think it would be easier to just set the version number at build time instead. If you want to test different release candidates
Sounds like a different process. In Chrome OS, we do "tag a new release version and then test it", and they are "testing binaries then tag for new release".
Something I'd like to mention: when testing auto update, we had similar script in fm_and_key_version_test_prep.sh that will search and replace version string directly, but that's only for testing.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#7).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version, if the VPD variable is not found it would fallback to use CONFIG_LOCALVERSION, if it's still not set it would use COREBOOT_VERSION.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 55 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/7
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 8:
(2 comments)
Moved the implementation to x86/smbios.c, thanks for the suggestions, I'll just continue to finish the feature first. The VPD use in our plan for servers are not device-specific (for device-specific data is stored somewhere else called FRU) and normally the VPD should be erased and updated from version to version.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@5 PS2, Line 5: #include <smbios.h>
Do you mean sort the include files alphabetically? Just updated.
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@13 PS2, Line 13: smbios_mainboard_bios_version
This is meant as an override by the mainboard, you can't just use it here (e.g. […]
Moved to https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c#376 The idea is trying to make it the default behavior by calling from the weak function, so we don't need to override the weak function for every board.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 8:
(6 comments)
The VPD use in our plan for servers are not device-specific (for device-specific data is stored somewhere else called FRU) and normally the VPD should be erased and updated from version to version.
Well... honestly you're just using it in a way that it really wasn't meant to be used there. But this patch seems harmless enough so I'd be okay taking it if it makes your life easier.
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version" Does this need to be configurable? Can't we just hardcode this name and have everyone use the same?
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@22 PS8, Line 22: #if CONFIG(VPD_SMBIOS_VERSION) Header includes should not be guarded, they don't hurt anyone.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@374 PS8, Line 374: #if CONFIG(VPD_SMBIOS_VERSION) You shouldn't need to guard this (if you also don't guard the header include above). The linker will garbage collect the function automatically when it determines it to be unreachable.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@378 PS8, Line 378: static char buffer[VER_LEN] = {0}; Since this is ramstage-only (right?) maybe just use vpd_find() and malloc()? (We could also add a vpd_gets_alloc() wrapper to the VPD code for this if we want...)
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@394 PS8, Line 394: return CONFIG_LOCALVERSION; Don't do this, just have this function return NULL and then have the caller proceed to the next option on error.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@404 PS8, Line 404: const char *__weak smbios_mainboard_bios_version(void) I still think we should rewrite this so the weak smbios_mainboard_bios_version() just returns "" and there's another function that handles checking VPD, LOCALVERSION, etc. in turn. It's bad style for weak function stubs that can be overridden to contain any real logic, they should just be no-ops. Also, I don't think the current behavior that a mainboard override has precedence over CONFIG_LOCALVERSION (or now the VPD) is intentional... precedence order should be VPD > LOCALVERSION > mainboard override > coreboot_version.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#9).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 73 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/9
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version"
Does this need to be configurable? Can't we just hardcode this name and have everyone use the same?
Hi Andrea (insomniac), is this config needed for you? Thanks.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@22 PS8, Line 22: #if CONFIG(VPD_SMBIOS_VERSION)
Header includes should not be guarded, they don't hurt anyone.
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@374 PS8, Line 374: #if CONFIG(VPD_SMBIOS_VERSION)
You shouldn't need to guard this (if you also don't guard the header include above). […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@378 PS8, Line 378: static char buffer[VER_LEN] = {0};
Since this is ramstage-only (right?) maybe just use vpd_find() and malloc()? (We could also add a vp […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@394 PS8, Line 394: return CONFIG_LOCALVERSION;
Don't do this, just have this function return NULL and then have the caller proceed to the next opti […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@404 PS8, Line 404: const char *__weak smbios_mainboard_bios_version(void)
I still think we should rewrite this so the weak smbios_mainboard_bios_version() just returns "" and […]
Rewritten, as for the precedence order, although only drivers/i2c/at24rf08c/lenovo_serials.c overrides this function, if I change the order they would be affected so I still keep the same order. Maybe that can be done by another change.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@13 PS2, Line 13: smbios_mainboard_bios_version
Moved to […]
Done
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#11).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 75 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/11
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@373 PS11, Line 373: #if !CONFIG(CHROMEOS) Why is this here?
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@403 PS11, Line 403: static This doesn't need to be static.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#12).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 75 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/12
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@373 PS11, Line 373: #if !CONFIG(CHROMEOS)
Why is this here?
ChromeOS would see build error: src/arch/x86/smbios.c:400:20: error: 'get_bios_version' defined but not used [-Werror=unused-function] Need to rewrite smbios_write_type0() for CONFIG(CHROMEOS) cases to use get_bios_version(), if that's preferred.
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@403 PS11, Line 403: static
This doesn't need to be static.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@373 PS11, Line 373: #if !CONFIG(CHROMEOS)
ChromeOS would see build error: […]
Sorry, I did not realize there was so much existing ugliness for CONFIG(CHROMEOS) in here. Yes, if you could move the t->bios_version part out of the #if CONFIG(CHROMEOS) block down there, and then implement the spaces stuff equivalently (with a C if (CONFIG(CHROMEOS))) inside get_bios_version(), that would be great.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#13).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 82 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/13
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/11/src/arch/x86/smbios.c@373 PS11, Line 373: #if !CONFIG(CHROMEOS)
Sorry, I did not realize there was so much existing ugliness for CONFIG(CHROMEOS) in here. […]
Done. I don't have a Chromebook to verify hope I don't mess it up.
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version"
Hi Andrea (insomniac), is this config needed for you? […]
We don't have a strong requirement on making it configurable, and I am OK with either "version" or "firmware_version". My reasoning on making it configurable is to allow people to adapt it to their own scenario or naming scheme (e.g. smbios0_version? bios_version? system_firmware_version?). However if more people start using this feature I also understand that using a fixed name could be more desirable to avoid unpredictable behaviours, so I'm also OK with hardcoding it
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 13:
Also just tested on the intended platform, and this works beautifully
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version"
We don't have a strong requirement on making it configurable, and I am OK with either "version" or " […]
"firmware_version" sounds better, "version" is too unspecific.
I'm just trying to avoid making anything more complex than it'll likely ever need to be, so let's please just hardcode it for now. If anyone has a real need to use a different name later, we can still change it.
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c@452 PS13, Line 452: t->bios_version = smbios_add_string(t->eos, get_bios_version()); Just move this line further up so you can combine all the conditional stuff in a single #if block?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c@454 PS13, Line 454: /* SMBIOS offsets start at 1 rather than 0 */ : chromeos_get_chromeos_acpi()->vbt10 = (u32)t->eos + (version_offset - 1); maybe someone can double confirm, but I think this can be moved to before t->bios_version line, because version_offset& t->eos should not be changed when we add version to smbios?
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#14).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 72 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/14
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#15).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 71 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/15
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version"
"firmware_version" sounds better, "version" is too unspecific. […]
Done
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c@452 PS13, Line 452: t->bios_version = smbios_add_string(t->eos, get_bios_version());
Just move this line further up so you can combine all the conditional stuff in a single #if block?
I moved it down instead because version_offset calculated at https://review.coreboot.org/c/coreboot/+/42029/14/src/arch/x86/smbios.c#447 can be affected.
https://review.coreboot.org/c/coreboot/+/42029/13/src/arch/x86/smbios.c@454 PS13, Line 454: /* SMBIOS offsets start at 1 rather than 0 */ : chromeos_get_chromeos_acpi()->vbt10 = (u32)t->eos + (version_offset - 1);
maybe someone can double confirm, but I think this can be moved to before t->bios_version line, beca […]
Done
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 15:
(1 comment)
Just one small comment, otherwise looks good to me
https://review.coreboot.org/c/coreboot/+/42029/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/15/src/Kconfig@752 PS15, Line 752: bool "Read firmware_version from VPD_RO for SMBIOS type 0 version" what about "Populates SMBIOS type 0 version from the VPD_RO variable 'firmware_version' ?
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#16).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 71 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/16
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42029/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/15/src/Kconfig@752 PS15, Line 752: bool "Read firmware_version from VPD_RO for SMBIOS type 0 version"
what about "Populates SMBIOS type 0 version from the VPD_RO variable 'firmware_version' ?
Done
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 16:
It seems there's a merge conflict. After fixing that, if Julius and Hung-Te agree I think this is good to merge
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Zhang, insomniac, Jingle Hsu, Morgan Jang,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42029
to look at the new patch set (#17).
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 71 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42029/17
insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 17: Code-Review+1
Thanks Johnny, this looks good to me. I also verified that even updating the VPD variable at run time won't change the dmidecode output.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 17: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable
If VPD_SMBIOS_VERSION is selected, it would read VPD_RO variable that can override SMBIOS type 0 version.
One special scenario of using this feature is to assign a BIOS version to a coreboot image without the need to rebuild from source. VPD_SMBIOS_VERSION default is n.
Tested=On OCP Delta Lake, dmidecode -t 0 can see the version being updated from VPD.
Change-Id: Iee62ed900095001ffac225fc629b3f2f52045e30 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42029 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: insomniac insomniac@slackware.it Reviewed-by: Julius Werner jwerner@chromium.org --- M src/Kconfig M src/arch/x86/smbios.c 2 files changed, 71 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved insomniac: Looks good to me, but someone else must approve
diff --git a/src/Kconfig b/src/Kconfig index 1b49e2b..f9c3e6a 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -730,6 +730,16 @@ help Override the default Product name stored in SMBIOS structures.
+config VPD_SMBIOS_VERSION + bool "Populates SMBIOS type 0 version from the VPD_RO variable 'firmware_version'" + default n + depends on VPD && GENERATE_SMBIOS_TABLES + help + Selecting this option will read firmware_version from + VPD_RO and override SMBIOS type 0 version. One special + scenario of using this feature is to assign a BIOS version + to a coreboot image without the need to rebuild from source. + endmenu
source "payloads/Kconfig" diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index a8723aa..d102d10 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -19,6 +19,8 @@ #if CONFIG(CHROMEOS) #include <vendorcode/google/chromeos/gnvs.h> #endif +#include <drivers/vpd/vpd.h> +#include <stdlib.h>
#define update_max(len, max_len, stmt) \ do { \ @@ -368,12 +370,64 @@ return t->length + smbios_string_table_len(t->eos); }
+#define VERSION_VPD "firmware_version" +static const char *vpd_get_bios_version(void) +{ + int size; + const char *s; + char *version; + + s = vpd_find(VERSION_VPD, &size, VPD_RO); + if (!s) { + printk(BIOS_ERR, "Find version from VPD %s failed\n", VERSION_VPD); + return NULL; + } + + version = malloc(size + 1); + if (!version) { + printk(BIOS_ERR, "Failed to malloc %d bytes for VPD version\n", size + 1); + return NULL; + } + memcpy(version, s, size); + version[size] = '\0'; + printk(BIOS_DEBUG, "Firmware version %s from VPD %s\n", version, VERSION_VPD); + return version; +} + +static const char *get_bios_version(void) +{ + const char *s; + +#define SPACES \ + " " + + if (CONFIG(CHROMEOS)) + return SPACES; + + if (CONFIG(VPD_SMBIOS_VERSION)) { + s = vpd_get_bios_version(); + if (s != NULL) + return s; + } + + s = smbios_mainboard_bios_version(); + if (s != NULL) + return s; + + if (strlen(CONFIG_LOCALVERSION) != 0) { + printk(BIOS_DEBUG, "BIOS version set to CONFIG_LOCALVERSION: '%s'\n", + CONFIG_LOCALVERSION); + return CONFIG_LOCALVERSION; + } + + printk(BIOS_DEBUG, "SMBIOS firmware version is set to coreboot_version: '%s'\n", + coreboot_version); + return coreboot_version; +} + const char *__weak smbios_mainboard_bios_version(void) { - if (strlen(CONFIG_LOCALVERSION)) - return CONFIG_LOCALVERSION; - else - return coreboot_version; + return NULL; }
static int smbios_write_type0(unsigned long *current, int handle) @@ -387,27 +441,14 @@ t->length = len - 2;
t->vendor = smbios_add_string(t->eos, "coreboot"); -#if !CONFIG(CHROMEOS) t->bios_release_date = smbios_add_string(t->eos, coreboot_dmi_date);
- t->bios_version = smbios_add_string(t->eos, - smbios_mainboard_bios_version()); -#else -#define SPACES \ - " " - t->bios_release_date = smbios_add_string(t->eos, coreboot_dmi_date); -#if CONFIG(HAVE_ACPI_TABLES) +#if CONFIG(CHROMEOS) && CONFIG(HAVE_ACPI_TABLES) u32 version_offset = (u32)smbios_string_table_len(t->eos); -#endif - t->bios_version = smbios_add_string(t->eos, SPACES); - -#if CONFIG(HAVE_ACPI_TABLES) /* SMBIOS offsets start at 1 rather than 0 */ - chromeos_get_chromeos_acpi()->vbt10 = - (u32)t->eos + (version_offset - 1); + chromeos_get_chromeos_acpi()->vbt10 = (u32)t->eos + (version_offset - 1); #endif -#endif /* CONFIG_CHROMEOS */ - + t->bios_version = smbios_add_string(t->eos, get_bios_version()); uint32_t rom_size = CONFIG_ROM_SIZE; rom_size = MIN(CONFIG_ROM_SIZE, 16 * MiB); t->bios_rom_size = (rom_size / 65535) - 1;
-
Hung-Te Lin (Code Review) -
insomniac (Code Review)
-
Johnny Lin (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)