Hello coreboot folks, At the time of writing coreboot is lacking good firmware menus or user-space applications to change options consumed by coreboot. While there's support for changing CMOS options in SeaBIOS or from within Linux on some x86 mainboards it's quite limited in user friendliness. While CMOS was a great thing some decades ago, it cannot compete with today's flash based non-volatile option store, like UEFI variables, SMMSTORE or VPD.
Our best engineers at 9elements came up with a possible solution: coreboot forms representation, see [1], [2], [3] CFR allows to have nice firmware menus in coreboot payloads as done in example for tianocore/EDK2 [5]. While it was designed with EDK2 in mind it's possible to use it on any payload with any variable store back-end (UEFI, SMMSTORE, VPD, CMOS, ...), as it's really about providing meta data about the options, not how to actually store it's value in a non-volatile fashion. The coreboot forms representation is provided to a payload in a new coreboot table and is generated at runtime by using C-style lookup tables, but in order to reduce code duplicates can be defined in common code as done in [4].
Please review and give feedback. Regards, Patrick
1: https://review.coreboot.org/c/coreboot/+/77882/ 2: https://review.coreboot.org/c/coreboot/+/74121/ 3: https://review.coreboot.org/c/coreboot/+/78506/ 4: https://review.coreboot.org/c/coreboot/+/78296/ 5: https://github.com/9elements/edk2/commit/f34f17996444ef811cc0223b4f875c45daa...
Hi Patrick, I put a bunch of comments into the reviews, but I thought I'd reply directly on the mailing list as well for anyone who isn't looking at the patches.
We discussed this at today's leadership meeting where I actually brought up the patches and discussed some of the things I'd seen.
I may not completely understand the intentions here, and I'd like to understand the requirements that led to this solution.
Really, I have 3 main objections:
1) The name - “Forms Representation” may be the single most UEFI phrase I've ever read. Use "Config Option Data", "Option Data Structures", "Menu Option Data", anything other than "Forms Representation". Is that maybe a mechanical translation from a different language, or does someone actually talk that way?
2) As I said, I don't understand your requirements, but this seems hugely more complex than it needs to be. I also think this entire structure should be generated at build time and not runtime. My personal opinion is that the default for any item shouldn't be something read from EC, SPI, wherever at runtime. Instead it should be a flag that says to update the (non-default) setting from someplace at runtime.
3) It should be to populate and modify the menus from any different level in the coreboot hierarchy - Arch, SoC, (Vendor), Mainboard. Right now, the menus seem to be very hardcoded, without a good way to insert items from any other place in the tree other than where the menu is created.
I guess I'd prefer to have something more like how Kconfig or devicetree work, taking text files from all of the different levels in coreboot, combining them into a tree, then generating the structures from that.
Unfortunately, Items 2 and 3 are completely different from your proposal, and would require a complete rewrite. Sorry about that.
Maybe we can set up a meeting time to discuss things between 9E and interested parties in the coreboot community?
Take care. Martin
Oct 31, 2023, 10:45 by patrick.rudolph@9elements.com:
Hello coreboot folks, At the time of writing coreboot is lacking good firmware menus or user-space applications to change options consumed by coreboot. While there's support for changing CMOS options in SeaBIOS or from within Linux on some x86 mainboards it's quite limited in user friendliness. While CMOS was a great thing some decades ago, it cannot compete with today's flash based non-volatile option store, like UEFI variables, SMMSTORE or VPD.
Our best engineers at 9elements came up with a possible solution: coreboot forms representation, see [1], [2], [3] CFR allows to have nice firmware menus in coreboot payloads as done in example for tianocore/EDK2 [5]. While it was designed with EDK2 in mind it's possible to use it on any payload with any variable store back-end (UEFI, SMMSTORE, VPD, CMOS, ...), as it's really about providing meta data about the options, not how to actually store it's value in a non-volatile fashion. The coreboot forms representation is provided to a payload in a new coreboot table and is generated at runtime by using C-style lookup tables, but in order to reduce code duplicates can be defined in common code as done in [4].
Please review and give feedback. Regards, Patrick
1: https://review.coreboot.org/c/coreboot/+/77882/ 2: https://review.coreboot.org/c/coreboot/+/74121/ 3: https://review.coreboot.org/c/coreboot/+/78506/ 4: https://review.coreboot.org/c/coreboot/+/78296/ 5: https://github.com/9elements/edk2/commit/f34f17996444ef811cc0223b4f875c45daa... _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Hi all,
I was surprised of the many thoughts that came when I started to look into this. Please read this as a flush of suggestions, nothing that I have thought through or insist on.
On 01.11.23 20:54, Martin Roth via coreboot wrote:
Hi Patrick, I put a bunch of comments into the reviews, but I thought I'd reply directly on the mailing list as well for anyone who isn't looking at the patches.
We discussed this at today's leadership meeting where I actually brought up the patches and discussed some of the things I'd seen.
I may not completely understand the intentions here, and I'd like to understand the requirements that led to this solution.
Really, I have 3 main objections:
- The name - “Forms Representation” may be the single most UEFI phrase I've ever read. ...
We already have a name for it "option table". And much of what CFR does and what Martin asks for is already implemented. The current implemen- tation is limited and tailored to the CMOS backend. But I believe the two can and should be merged. Looking at the structures (`struct cmos_*` in `coreboot_tables.h` and the proposed CFR[1]), they are quite similar. CFR being a little more abstract (with redundancies that might vanish during review anyway). And the current option table, well, has its references to CMOS bits.
The CFR structures, however, do not look like they are designed to be a superset. So I suggest, we have a close look at the structures and figure out what we actually need.
CFR seems to add the following: * unique option IDs (used for references) * dependencies (referencing other options' IDs) (Not sure if a single reference is flexible enough?) * grouping of options * help texts!! :) * flags * individual default values (for the CMOS option table these were gathered in a single blob, that might not be flexible enough)
The CMOS option table structures have: * enum types (no need to repeat enum values for every option instance) * bit locations in CMOS
I suggest to keep the way we currently handle enums. And backend specific information like the CMOS bit locations could be handled by optional sub-structures, i.e. when looking for CMOS information check the tag of additional objects at the end of a `struct lb_cfg_*_ option` until it is found. Non-CMOS implementations don't have to look, and if an option wasn't allocated any CMOS space, we don't have to add the optional object.
For backwards compatibility, we could keep redundant entries in the coreboot tables (if the CMOS backend is used).
I'm not sure how to handle default values... for the CMOS option table, we have a separate file for the user to fill. But that doesn't need to change, even if the values will be spread across many coreboot table entries later in the process.
- As I said, I don't understand your requirements, but this seems hugely more complex than it needs to be. I also think this entire structure should be generated at build time and not runtime. My personal opinion is that the default for any item shouldn't be something read from EC, SPI, wherever at runtime. Instead it should be a flag that says to update the (non-default) setting from someplace at runtime.
Do you mean like a reference, e.g. "for the default value, look 'there'"? Or some "this needs to be reset"?
Beside resetting to a default at runtime, it's also good to be able to present it to the user. Or display, that the current value is not the default. This seems to be the intention of the default values in CFR, I guess.
- It should be to populate and modify the menus from any different level in the coreboot hierarchy - Arch, SoC, (Vendor), Mainboard. Right now, the menus seem to be very hardcoded, without a good way to insert items from any other place in the tree other than where the menu is created.
This is something that many people miss in the current option- table implementation. The big issue with CMOS options is allocation, i.e. where to place the values. But that should be completely separate.
Right now I imagine something like one set of files (per arch/soc/board) that lists all the CFR information, and only if somebody wants to use the CMOS back end, additionally a file that only lists the option names + offsets (hand written, or maybe even automatically allocated, shouldn't matter).
I guess I'd prefer to have something more like how Kconfig or devicetree work, taking text files from all of the different levels in coreboot, combining them into a tree, then generating the structures from that.
Unfortunately, Items 2 and 3 are completely different from your proposal, and would require a complete rewrite. Sorry about that.
I don't see that. A lot of the proposed code is about serialization. Even if not compiled into coreboot, it can be used, e.g. to extend (or rewrite?) nvramtool.
Best, Nico
PS. Very spontaneous thought: Should we adopt the Kconfig syntax?
1: https://review.coreboot.org/c/coreboot/+/77882/ 2: https://review.coreboot.org/c/coreboot/+/74121/ 3: https://review.coreboot.org/c/coreboot/+/78506/ 4: https://review.coreboot.org/c/coreboot/+/78296/ 5: https://github.com/9elements/edk2/commit/f34f17996444ef811cc0223b4f875c45daa...
-
Martin Roth -
Nico Huber -
Patrick Rudolph