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"?