Attention is currently required from: Benjamin Doron, Paul Menzel, 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:
(4 comments)
File src/drivers/option/cfr.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/aef47be3_c2194edc 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 structure, but the length of the string. The structure is a bit longer, as it has some trailing bytes so that the next structure is aligned to `LB_ENTRY_ALIGN`.
https://review.coreboot.org/c/coreboot/+/74121/comment/4f0406f3_6186dd89 PS1, Line 56: /* struct cfr_string ui_name; */ These members are intentionally commented out so that `sizeof()` of this structure only contains the fields in the structure itself. Uncommenting this results in the `length` field of `cfr_string` getting counted twice (once in the `sizeof()` for this structure, once when writing the `cfr_string` structure).
Things get much worse when having multiple `cfr_string` members in a struct, because there's a flexible array member not at the end of the structure. Any members after a `cfr_string` member will have an invalid offset.
https://review.coreboot.org/c/coreboot/+/74121/comment/7ad3d2de_08d22cf9 PS1, Line 71: /* struct cfr_string opt_name; */ : /* struct cfr_string ui_name; */ : /* struct lb_cfr_enum_value enum_values[]; */ Might have a problem parsing these variable length fields because the length field of a `cfr_string` structure does not correspond to the actual size of the entire structure. Currently, anyone trying to parse these tables needs to know that the length field of a `cfr_string` needs to be aligned up to `LB_ENTRY_ALIGN` to find the next record.
For strings, we could make the length field store the aligned length (and maybe fill in the padding with zeroes so that weird things don't happen), but this won't work for byte buffers as there's no "NULL terminator" equivalent.
TL;DR it seems that the current approach is the simplest one, but this needs to be properly documented.
File src/drivers/option/cfr.c:
https://review.coreboot.org/c/coreboot/+/74121/comment/147968ce_9edc14b3 PS1, Line 19: uint32_t
Any reason to limit the length?
cbtables records have a `uint32_t size;` field, and its width cannot be changed without breaking backwards compatibility. The values returned by this function are stored in these `size` fields, so it makes sense to limit the length accordingly.
[The rest of this comment is written in a jocular tone, and should not be taken personally]
Also, if your setup menu needs more than 4 GiB of RAM, you may have a problem... Several problems, actually:
- You'd need to have enough RAM to store all the data. - A huge chunk of RAM that the OS can't use (it could be reclaimed if you're willing to open another can of worms). - You'd need to put cbtables above 4 GiB, and you'd either need PAE (Physical Address Extension) or 64-bit coreboot to fill in the cbtables. You might need to do something to maintain backwards compatibility. - Why is your setup menu so huge, are you trying to micromanage the hardware?
-
Angel Pons (Code Review)