Attention is currently required from: Benjamin Doron, Maximilian Brune, Lean Sheng Tan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74121 )
Change subject: [RFC] drivers/option: Add option list in cbtable ......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS1:
Looking at EDK2's UefiInternalFormRespresentation header, I had some ideas:
- Should we have per-option help text?
We could, but we either need to make it required (so options without a help text would need to provide an empty string) or we need to add a way to know if the CFR string for the help text is present (which would complicate parsing; for enum options, you'd have an optional help text field followed by a variable-length list of enum values).
- Currently, I mark all options as RESET_REQUIRED. If it's likely some options won't require a reset to apply, can we add a flag?
coreboot options always need a reset, because the flow is as follows:
- coreboot starts executing - coreboot reads option values from backend - coreboot writes CFR structures - coreboot jumps to payload - payload starts - payload processes CFR structures - payload reads current option values from backend - payload displays setup menu to user - user changes some coreboot options and saves them - payload writes new option values to backend
At this point, the backend contains the new option values, but coreboot doesn't know about them yet. Let's suppose someone (the user or the payload) requests a reset, this is what happens next:
- payload issues system/full reset - system resets - coreboot starts executing - coreboot reads option values from backend - coreboot writes CFR structures
... and so on.
However, payload-specific options (e.g. boot order) may not need a reset. If coreboot provides these options to the payload through CFR, then CFR indeed needs a field to know that these options don't need a reset.
- Please add an `OPTION_TEXT`, so we can notify the user. For instance, boards might display "EEPROM corrupted, contact manufacturer"
`OPTION_TEXT` or `OPTION_STRING` would work to display a read-only message in the setup menu; will add structures for this. For out-of-menu warnings, we probably want something else.
- When we think about dependencies between options, we should consider logical relationships - `suppressif X AND NOT Y` - Since there is also OR, maybe tag these and we'll process them in order - if necessary, we can provide a field(s) for equality operators. Another thing to consider behaviour if the dependencies fail - suppress, grayout or allow the behaviour to be determined? The last one adds more complexity.
There's a lot more that's possible, such as arithmetic, bitwise logic and shifts, 'casting' type operators (including string `TO_LOWER` and `TO_UPPER`), but these get harder to justify and I'd rather neither of us implement the entire IFR language
Yeah... Right now, the suppress/grayout stuff is static, i.e. `suppressif TRUE/FALSE`. To express dynamic dependencies, the idea would be to add new records that map to options using option ID values. We could avoid several pitfalls by disallowing forward references to options, i.e. a dependency expression cannot use option IDs that haven't been defined yet.
Not sure what we'll need to do in the future, but it should be possible to extend the structures.
File src/drivers/option/cfr.c:
https://review.coreboot.org/c/coreboot/+/74121/comment/e3382301_e9236dd6 PS1, Line 22: /* TODO: error handling? */ : die("%s: bad size (start = %" PRIxPTR ", end = %" PRIxPTR ")", : __func__, start, end); :
Should we remove this record from the list by returning 0? Then, `lb_new_record()` will find this sa […]
This is a sanity check so that `end - start` can safely be casted to `uint32_t`. `start > end` (size would be negative) shouldn't happen unless there's a bug in the code. `end - start > UINT32_MAX` means that one of the CFR records is larger than 4 GiB, which shouldn't happen unless there's a bug in the code or someone tried to add an absurdly humongous amount of information in CFR. Given that cbtables are located below 4 GiB, one would run into some other problem (hitting already-used memory addresses, e.g. TSEG or MMIO) well before a record's size exceeds 4 GiB.
TL;DR the TODO can be removed. Maybe replace the comment with a summary of this comment.