Attention is currently required from: Sean Rhodes, 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 3:
(7 comments)
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/6efafaf4_d37a0625 PS1, Line 99: LB_TAG_CFR_STRING = 0x0102,
This is currently unused, as CFR strings are not a cbtables record. […]
Done
File src/drivers/option/cfr.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/7ffa4a00_ea264213 PS2, Line 10: SETUP_MENU_VAR_FLAG_READONLY = 1 << 0, : SETUP_MENU_VAR_FLAG_GRAYOUT = 1 << 1, : SETUP_MENU_VAR_FLAG_SUPPRESS = 1 << 2,
yeah this sounds better
Done
File src/drivers/option/cfr.h:
https://review.coreboot.org/c/coreboot/+/74121/comment/3fc555be_b6b4bca8 PS1, Line 48: struct cfr_string { : uint32_t length; : uint8_t str[]; : };
Even if it's redundant, making this a cbtables record means CFR sinks (consumers) can be simpler (no […]
Done
https://review.coreboot.org/c/coreboot/+/74121/comment/ecdf4a4f_265245b9 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 optio […]
Done
https://review.coreboot.org/c/coreboot/+/74121/comment/09f34c5d_ed3d10cc PS1, Line 71: /* struct cfr_string opt_name; */ : /* struct cfr_string ui_name; */ : /* struct lb_cfr_enum_value enum_values[]; */
Yes, CFR strings (and byte buffers, if added in the future) would either need the two length fields […]
Done
https://review.coreboot.org/c/coreboot/+/74121/comment/17dcd795_5f412297 PS1, Line 80: struct lb_cfr_form forms[];
Should be commented out too. […]
Done
File src/drivers/option/cfr.c:
https://review.coreboot.org/c/coreboot/+/74121/comment/7a300a28_7f1e7e8a PS1, Line 22: /* TODO: error handling? */ : die("%s: bad size (start = %" PRIxPTR ", end = %" PRIxPTR ")", : __func__, start, end); :
This is a sanity check so that `end - start` can safely be casted to `uint32_t`. […]
Done
-
Angel Pons (Code Review)