Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44775 )
Change subject: treewide: stop using hexdumps for SPD files ......................................................................
Patch Set 27:
Patch Set 27: Code-Review-1
Patch Set 27: Code-Review+1
Patch Set 27:
I'd vote for not checking binaries into the coreboot source tree at all. Personally, I'd like to get rid of the vbt binaries and have a tool to compile them as well. My understanding was that there wasn't documentation for the VBT fields, but it seems that maybe this is no longer the case. [1]
In general I agree, but it seems hard to get authoritative sources on the precise format, even for SPD dumps - that ought to be standardized but get repurposed in lots of "wonderful" ways by memory reference code. As far as we know, the "spec" to these files is the reference code, and that's not public.
Some of the SPD tools we use on newer chipsets (for lp4x?) may come close to that. In which case: why ship hex dumps or binaries at all?
As far as vendors adding custom flags, I don't see how binary vs text solves any problem there.
The transition here removes the hex-to-bin translation that's been fragile. Michael offers a patch that fixes things for him - that I'm relatively sure breaks building that part of the tree on other UNIX systems, since I vaguely remember having had these issues myself. My guess would be Solaris but it might be some odd BSD as well.
The text files we ship are plain hex dumps. They don't add anything of value, do they? So why should we pretend that they're "source" and make our live miserable?
My proposal is to get this in to remove the pain point that this is dealing with (the fragile hex-to-bin translation) and whoever is motivated to do so looks into providing a higher level description for these files and translating things to that, removing the opaque datasets we have (no matter the format) from the tree.
spd_tools was already updated to output binary SPD files, so now spd_tools is not usable for adding parts until this change goes in. So this is a 2nd vote for getting this in as is to unblock spd_tools.
spd_tools generates SPD binaries from a json input. This could certainly be made part of the build so the generated SPD binaries do not need to be checked in. The only blocker is golang support in the build or rewriting the tool in a supported language.
Why not revert the change that made spd_tools output binaries instead? I'm not convinced using binaries would help with the issue at hand.
So revert or patch spd_tools to support both hex and binary with a commandline flag. Michael, do you have a preference?
-
Rob Barnes (Code Review)