Attention is currently required from: Benjamin Doron, Maximilian Brune.
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 1:
(7 comments)
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/05df6c54_5fbc3341 PS1, Line 99: LB_TAG_CFR_STRING = 0x0102,
Unused? Is the CFR string structure intended to tag itself (as in, CFR implementation or service con […]
This is currently unused, as CFR strings are not a cbtables record. However, it makes little sense: CFR sinks (consumers) need to handle CFR strings specially (they don't have a record header, and the string length needs to be rounded up to find the next CFR structure).
So, this will be used.
File src/drivers/option/cfr.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/ad25d042_de848482 PS1, Line 48: struct cfr_string { : uint32_t length; : uint8_t str[]; : };
Note: This cannot be a cbtables record because `length` does not store the actual length of the stru […]
Even if it's redundant, making this a cbtables record means CFR sinks (consumers) can be simpler (no need to specially handle CFR strings' length) and more robust (everything is a cbtables record, so the tag can be used for runtime type-checking aka validating the syntax).
``` struct lb_cfr_string { uint32_t tag; uint32_t size; uint32_t str_length; uint8_t str[]; }; ```
https://review.coreboot.org/c/coreboot/+/74121/comment/6a925269_b4851072 PS1, Line 56: /* struct cfr_string ui_name; */ Hmmmmm, the relations between records should be documented. The whole thing is a tree (records inside other records' size are children of the enclosing record), but the children of each node can't be whatever. (syntax? grammar? can't English today...) Records and their children can be thought of as function signatures (which can be variable-length).
https://review.coreboot.org/c/coreboot/+/74121/comment/e0f02408_6611b4c0 PS1, Line 68: uint32_t size; We might want to add an "option ID" value here, which could be used in the future to reference options in some dependency expression mechanism (e.g. do not show `gfx_uma_size` if the iGPU is disabled by some other option).
Even if we currently don't provide any constructs to express such dependencies. Otherwise, we'd need to add new tags to cbtables.
https://review.coreboot.org/c/coreboot/+/74121/comment/dac3592e_8e72e09f PS1, Line 71: /* struct cfr_string opt_name; */ : /* struct cfr_string ui_name; */ : /* struct lb_cfr_enum_value enum_values[]; */
Ah, possibly 'string length' and 'actual length' fields? I think it's fine though: just need to alig […]
Yes, CFR strings (and byte buffers, if added in the future) would either need the two length fields you mentioned or CFR sinks (consumers) need to round up the "string length" value to obtain the "actual length" after which the next cbtables record resides.
That being said, it's very odd to have CFR strings *not* be a cbtables record, unlike everything else. In that case, we would use the record's size as "actual length" and we'd have an additional field for "string/data length" (for strings/byte buffers, respectively).
OK, decision made: even if it means storing additional data (8 bytes), CFR strings should be cbtables records like everything else.
https://review.coreboot.org/c/coreboot/+/74121/comment/c3bca06b_81a228e0 PS1, Line 80: struct lb_cfr_form forms[]; Should be commented out too. Accessing elements other than the first using this member will result in undefined behavior, as the array stride does not account for all the variable-length stuff in here. There's the flexible array members in CFR strings, as well as variable-length record lists (e.g. enum values for an option, options in a form).
File src/drivers/option/cfr.c:
https://review.coreboot.org/c/coreboot/+/74121/comment/85f1be60_d94bb940 PS1, Line 127: write_body(header); : : menu->size = sm_compute_record_size((char *)menu, header);
I should be sleeping at 2am instead - this would fragment the CFR into 'actual' tags rather than one […]
Not sure if cbtables can contain multiple records with the same tag. This is currently not a problem because `LB_TAG_CFR` is meant to be unique, and the other CFR tags are only used within the CFR-root record (the record tagged as `LB_TAG_CFR`). Since the CFR-root record's size contains all other CFR records, existing cbtables consumers will only see a huge record tagged as `LB_TAG_CFR` and simply skip it.
The current implementation allows CFR sinks (consumers) to traverse the entire structure in order, which should be possible without needing variable length temporary buffers (the destination buffer is still variable length). This should also minimise the amount of global state that needs to be kept track of. Heck, one could implement a CFR writer in a functional programming style.
If CFR structures are written in multiple chunks (e.g. each chunk contains a form for chipset/board/etc.), one could still process each form in order, but the ordering of forms wouldn't necessarily be stable (no idea which form is written first to cbtables). Then again, if it's that much of an issue, CFR sinks could build an array of references to the forms and sort it. However, the main drawback of this approach would be lack of flexibility: options would need to be grouped by coreboot compilation unit.
Another idea would be to replace forms as a cbtables record with a "form ID" value in options. This would provide a lot of flexibility, but the resulting structures would not be in order anymore. Computationally speaking, this gets expensive: one would either need to walk through all CFR structures for each form (after figuring out which forms there are, which would probably involve walking through all CFR structures), or store references to options in a variable-length array of lists (each array element corresponds to a form ID and contains a list of the options for that form). If the CFR structures are fragmented, one would then need to walk through the entire cbtables instead.
It would seem that splitting the generation of CFR structures is a no-go on the grounds of data entropy (the information is no longer sorted). However, note that CB:74122 doesn't write CFR structures directly: option data is stored in various `struct sm_*` variables (mostly constants, but `pch_pm_pcie_pll_ssc_values` is run-time generated), and then passed to the CFR writer code (this change).
[Note: the following paragraphs use `gfx_uma_size` as an example, but this applies to other options as well]
Although option values aren't always known in advance (e.g. the value for `gfx_uma_size` may be stored in CMOS), options themselves are known at build time (the code being built contains some reference to `gfx_uma_size`, currently a string literal passed to `get_uint_option()`; enum values are in mainboards' `cmos.layout`). After fully decoupling options from CMOS (e.g. declaring the `gfx_uma_size` option and its enum values once in chipset code, and having mainboards' `cmos.layout` reference the option declared in chipset code), one would have several "option descriptor" chunks of data scattered around the tree.
Hmmm, we already have a bunch of "chunks of data scattered around the tree" of a different kind: PCI drivers. We could hook up our option descriptors to device drivers (we might want to have options for non-PCI devices, too). This would allow us to override the visibility of some settings from mainboard code (which CB:74122 doesn't do yet, but will need to for `ATLAS_PROF_REALTIME_PERFORMANCE`).
This would also allow us to only show the options for enabled devices (e.g. suppress the `gfx_uma_size` option if the board does not have an iGPU; we still want to show the option when the iGPU is disabled but could be enabled later, to allow configuring `gfx_uma_size` after changing some other setting that will result in the iGPU being enabled next boot). Dynamically suppressing `gfx_uma_size` depending on the state of other options is something that cannot be described using CFR structures; additional structures could be added to describe such dependencies, but it would be best to add a numeric "option ID" field to avoid having to compare strings (option names).
If we're going to bind options to devices, we might as well go the extra mile and declare options in the (chipset) devicetree. We would then use SCONFIG to generate the option descriptors at build time. If per-device options ever become a thing, we could even hook up option descriptors to devices, so that something like this is possible:
``` static void pch_pcie_init(struct device *dev) { /* TODO: needs NULL check, might need a cast too */ const struct southbridge_intel_lynxpoint_pcie_rp_dynopts *dynopts = dev->dynopts;
const enum pcie_aspm aspm = get_uint_option(dynopts->aspm, ASPM_NONE); program_magic_aspm_registers(aspm);
const bool obff = get_uint_option(dynopts->obff, true); program_magic_obff_registers(obff);
const bool ltr = get_uint_option(dynopts->ltr, true); program_magic_ltr_registers(ltr); } ```
Where the `pch_pcie_init()` function is used as the `.init` device operation of a driver. Said driver is the same for all PCIe root port devices of the PCH (up to 8 on Lynx Point; the next generation PCH, Sunrise Point, has up to 24 PCIe root ports).