Attention is currently required from: Angel Pons, Benjamin Doron, Lean Sheng Tan, Maximilian Brune, Nico Huber, Patrick Rudolph, Sean Rhodes.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74121?usp=email )
Change subject: [RFC] drivers/option: Add forms in cbtables ......................................................................
Patch Set 18:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74121/comment/44bb257b_6e0dc683 : PS18, Line 15: The system currently lacks a way : to describe where to find option values. Right - I think that's a critical piece that needs to be added before we implement this. It can be as simple as an enum for different storage medias. I'd also like to make sure that different options can be stored in different media, not just assuming a single media for every option.
https://review.coreboot.org/c/coreboot/+/74121/comment/64612936_4b09a606 : PS18, Line 20: cursed forms representation Config Option Data - COD?
"Forms representation" is awful.
https://review.coreboot.org/c/coreboot/+/74121/comment/474f353d_b33ec38a : PS18, Line 31: which is undesired Agreed!
Patchset:
PS18: I like the concept, but have some quibbles.
It seems that all of this is to support a setup engine. The setup engine should not be a part of coreboot itself though. I don't understand why anything needs to be parsed at runtime in coreboot to generate all of these structures. This seems like a table of values that could be created at build time and just populated into a cbfs region.
Looking at the data that's included in the table, the fields all seem static: Name, display name, help text, flags, and default value. None of that needs to be modified at runtime.
Why does any of this need to go into coreboot? All coreboot cares about is the name and the current value of the setting, which isn't even defined yet.
Maybe move all of this into a separate tool that parses everything at build time and generates the final structures? coreboot can then just copy the data from cbfs into cbmem, or better still, just leave it in cbfs.