Change in ...coreboot[master]: RFC: smbios: allow setting BIOS-version string from VPD variable

[Łukasz Siudut (Code Review)]

Łukasz Siudut has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32905

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

RFC: smbios: allow setting BIOS-version string from VPD variable

As in the title. Such approach allows for convenient setting of the
firmware version and keep it in partition that is easily readable
from both - coreboot binary and already running operating system.

vpd_find is used directly as it returns pointer to vpd value which,
by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
way we avoid copying string around and coming up with temporary
buffer.

Change-Id: If0e9d90ed0941c4e76e3e48cdcccf830ef789458
Signed-off-by: Lukasz Siudut 
---
M src/arch/x86/smbios.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32905/1

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index 589f4f0..2ffd2dd 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -30,6 +30,9 @@
 #include <memory_info.h>
 #include <spd.h>
 #include <cbmem.h>
+#if CONFIG(VPD)
+#include <drivers/vpd/vpd.h>
+#endif
 #if CONFIG(CHROMEOS)
 #include <vendorcode/google/chromeos/gnvs.h>
 #endif
@@ -347,6 +350,15 @@

const char *__weak smbios_mainboard_bios_version(void)
 {
+#if CONFIG(VPD)
+	/* size is unused for now */
+	int size;
+	const char *coreboot_vpd_version = vpd_find("version", &size, VPD_RO);
+
+	/* if found, vpd value will be terminated by 0x00 (VPD_TYPE_TERMINATOR) */
+	if(coreboot_vpd_version)
+		return coreboot_vpd_version;
+#endif
    if (strlen(CONFIG_LOCALVERSION))
    	return CONFIG_LOCALVERSION;
    else

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32905/1/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/32905/1/src/arch/x86/smbios.c@358 
PS1, Line 358: 	/* if found, vpd value will be terminated by 0x00 (VPD_TYPE_TERMINATOR) */
line over 80 characters

https://review.coreboot.org/#/c/32905/1/src/arch/x86/smbios.c@359 
PS1, Line 359: 	if(coreboot_vpd_version)
space required before the open parenthesis '('

[Łukasz Siudut (Code Review)]

Łukasz Siudut has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

RFC: smbios: allow setting BIOS-version string from VPD variable

As in the title. Such approach allows for convenient setting of the
firmware version and keep it in partition that is easily readable
from both - coreboot binary and already running operating system.

vpd_find is used directly as it returns pointer to vpd value which,
by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
way we avoid copying string around and coming up with temporary
buffer.

Change-Id: If0e9d90ed0941c4e76e3e48cdcccf830ef789458
Signed-off-by: Lukasz Siudut 
---
M src/arch/x86/smbios.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32905/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32905/2/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/32905/2/src/arch/x86/smbios.c@358 
PS2, Line 358: 	/* if found, vpd value will be terminated by 0x00 (VPD_TYPE_TERMINATOR) */
line over 80 characters

https://review.coreboot.org/#/c/32905/2/src/arch/x86/smbios.c@359 
PS2, Line 359: 	if(coreboot_vpd_version)
space required before the open parenthesis '('

[Łukasz Siudut (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32905

to look at the new patch set (#3).

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

RFC: smbios: allow setting BIOS-version string from VPD variable

As in the title. Such approach allows for convenient setting of the
firmware version and keep it in partition that is easily readable
from both - coreboot binary and already running operating system.

vpd_find is used directly as it returns pointer to vpd value which,
by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
way we avoid copying string around and coming up with temporary
buffer.

Change-Id: If0e9d90ed0941c4e76e3e48cdcccf830ef789458
Signed-off-by: Lukasz Siudut 
---
M src/arch/x86/smbios.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32905/3

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/32905/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32905/3//COMMIT_MSG@7 
PS3, Line 7: BIOS-version
No hyphon: BIOS version

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c@353 
PS3, Line 353: #if CONFIG(VPD)
Can this be done in C and not the preprocessor?

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c@354 
PS3, Line 354: 	/* size is unused for now */
What does this comment mean?

[Łukasz Siudut (Code Review)]

Łukasz Siudut has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

Patch Set 3:

(3 comments)

I'll push updates in a moment.

https://review.coreboot.org/#/c/32905/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32905/3//COMMIT_MSG@7 
PS3, Line 7: BIOS-version
> No hyphon: BIOS version

Ack

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c@353 
PS3, Line 353: #if CONFIG(VPD)
> Can this be done in C and not the preprocessor?

If VPD support is not there then linking will fail later, as we reference vpd_find function.

https://review.coreboot.org/#/c/32905/3/src/arch/x86/smbios.c@354 
PS3, Line 354: 	/* size is unused for now */
> What does this comment mean?

I should have elaborate on that more - vpd_find writes size of the variable (second argument), but it is not necessary for us as (there's no real limitation for string size in smbios). Therefore it's not used.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS-version string from VPD variable
......................................................................

Patch Set 3:

Please explain why it should be useful. Why does CONFIG_LOCALVERSION not work for you. If you can read VPD from OS, why do you need to provide the information in smbios tables?

Change in ...coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Łukasz Siudut (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32905

to look at the new patch set (#4).

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

RFC: smbios: allow setting BIOS version string from VPD variable

As in the title. Such approach allows for convenient setting of the
firmware version and keep it in partition that is easily readable
from both - coreboot binary and already running operating system.

vpd_find is used directly as it returns pointer to vpd value which,
by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
way we avoid copying string around and coming up with temporary
buffer.

Change-Id: If0e9d90ed0941c4e76e3e48cdcccf830ef789458
Signed-off-by: Lukasz Siudut 
---
M src/arch/x86/smbios.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32905/4

Change in ...coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

Patch Set 4: Code-Review+1

Change in ...coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Łukasz Siudut (Code Review)]

Łukasz Siudut has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

Patch Set 4:
> Patch Set 3:
>
> Please explain why it should be useful. Why does CONFIG_LOCALVERSION not work for you. If you can read VPD from OS, why do you need to provide the information in smbios tables?

We do have a lot of software that rely on SMBIOS and is not easily / not possible to modify. This also allows to have a compatibility layer with older / not maintained anymore / closed source software that rely on information in SMBIOS.

Change in ...coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 3:
> >
> > Please explain why it should be useful. Why does CONFIG_LOCALVERSION not work for you. If you can read VPD from OS, why do you need to provide the information in smbios tables?
>
> We do have a lot of software that rely on SMBIOS and is not easily / not possible to modify. This also allows to have a compatibility layer with older / not maintained anymore / closed source software that rely on information in SMBIOS.

That's a good argument, that should be added to the commit message to explain why the change is done. It doesn't explain why it needs to be runtime configurable and why CONFIG_LOCALVERSION doesn't work for you.

Change in ...coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Łukasz Siudut (Code Review)]

Hello Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32905

to look at the new patch set (#5).

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

RFC: smbios: allow setting BIOS version string from VPD variable

As in the title. Such approach allows for convenient setting of the
firmware version and keep it in partition that is easily readable
from both - coreboot binary and already running operating system.

It can be used in cases when we still need to rely on SMBIOS for
legacy reasons (old closed source software that is not aware of
presence of VPD).

vpd_find is used directly as it returns pointer to vpd value which,
by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
way we avoid copying string around and coming up with temporary
buffer.

Change-Id: If0e9d90ed0941c4e76e3e48cdcccf830ef789458
Signed-off-by: Lukasz Siudut 
---
M src/arch/x86/smbios.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/32905/5

Change in coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32905 )

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

Patch Set 5:

(5 comments)
> It doesn't explain why it needs to be runtime configurable and why CONFIG_LOCALVERSION doesn't work for you.

+1... VPD has the advantage that it's easier to modify after the image is already built, but since the coreboot build version should only change with, you know, a new build, it's not clear to me why CONFIG_LOCALVERSION wouldn't be enough.

https://review.coreboot.org/c/coreboot/+/32905/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/32905/5//COMMIT_MSG@18 
PS5, Line 18: by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
No, this is *not* how that works! VPD_TYPE_TERMINATOR only comes after the last key/value pair in the VPD. You're just getting lucky right now that the version field comes last for you (you're probably testing with only a single field in the VPD? Try adding a few more afterwards and this should break).

Please actually use vpd_gets().

https://review.coreboot.org/c/coreboot/+/32905/5/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/32905/5/src/arch/x86/smbios.c@40 
PS5, Line 40: #if CONFIG(VPD)
There's no need to #if guard a header.

https://review.coreboot.org/c/coreboot/+/32905/5/src/arch/x86/smbios.c@395 
PS5, Line 395: const char *__weak smbios_mainboard_bios_version(void)
I don't think we should keep adding more functionality to a weak function. Weak functions should really only be used when the default implementation is basically a no-op or it becomes too confusing, and this is far from a no-op by now. I would recommend rearchitecting this as I outlined in https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c#13

https://review.coreboot.org/c/coreboot/+/32905/5/src/arch/x86/smbios.c@397 
PS5, Line 397: #if CONFIG(VPD)
Use a C

if (CONFIG(VPD))

rather than a preprocessor

#if CONFIG(VPD)

unless really necessary (it isn't here).

Also, you should probably make a new Kconfig for this that depends on VPD (and is default disabled). Otherwise, you're changing behavior for everyone else using VPD too, and they don't necessarily want this feature.

https://review.coreboot.org/c/coreboot/+/32905/5/src/arch/x86/smbios.c@401 
PS5, Line 401: version
Can we pick a more unique name here to avoid clashing with other potential uses? How about "smbios_version"?

[S] Change in coreboot[master]: RFC: smbios: allow setting BIOS version string from VPD variable

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32905?usp=email )

Change subject: RFC: smbios: allow setting BIOS version string from VPD variable
......................................................................

Abandoned