coreboot-gerrit December 2024

coreboot-gerrit@coreboot.org

1 participants
3018 discussions

[M] Change in coreboot[main]: lib/{fit,fit_payload}.c: Enhance support for FIT images
by Benjamin Doron (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Felix Held, Julius Werner, Lean Sheng Tan, Matt DeVillier, Maximilian Brune. Benjamin Doron has posted comments on this change by Benjamin Doron. ( https://review.coreboot.org/c/coreboot/+/84796?usp=email ) Change subject: lib/{fit,fit_payload}.c: Enhance support for FIT images ...................................................................... Patch Set 6: (2 comments) File src/lib/fit_payload.c: https://review.coreboot.org/c/coreboot/+/84796/comment/6b07322a_490b5c50?us… : PS6, Line 18: __weak void platform_fdt_add_secondaries(struct device_tree *tree, const char *name, struct region *secondary) > I think weak functions should only be used when the base case is a very trivial "default" implementa […] Ah, that's fair. Overusing weak functions is the sort of thing that makes platform/feature bringup more difficult too, if they hide the fact that the developer is obligated to provide the function definition. I disagree that they shouldn't be allowed to implement significant logic though, because sometimes there is a reasonable default case that we just want to be able to override in limited cases, but that's not the point here. This code makes another mistake. The point was that implementations of this are only going to differ in what's in `snprintf`'s string, but I didn't want to implement that independently: it's not a constant string, and we can't return strings that are on our stack: a char array is a pointer, and this stack frame is about to get lost. Now that I'm thinking about it, I could turn the weak part into a 'return-path-prefix' that's called by a function in this file, but we can probably just remove this because of the below. About `/firmware/coreboot/image@...`: yes, I made it up. I didn't realise `/firmware/coreboot/` is actually specced, and I especially didn't realise that the Linux kernel defines it. I figured there was an unspoken agreement, but I didn't think any longer about how they'd want it specced because I don't develop it. UPL wants to use `/options/upl-images/image@%llx`. Ultimately, extra images aren't needed by others at the moment, so I'm thinking I'll just print a warning here, maybe a `BUG()` and implement it properly in the UPL-specific followup. If this gets wider use, we should probably define something in the FIT spec for everybody to use, rather than letting everybody pick their own path. https://review.coreboot.org/c/coreboot/+/84796/comment/e80f8f94_1b8d6ee3?us… : PS6, Line 306: dt_print_node(dt->root); > This prints the whole DT, right? I don't think we should do that by default? If you think it is usef […] Yes. I extracted it out of upl_fdt_table (the follow-up) since the FDT is changed here, and then I figured it may as well stay since it can be useful to know. I'm happy dropping it now, aligning with CBTABLEs, and because it is quite verbose. -- To view, visit https://review.coreboot.org/c/coreboot/+/84796?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6a21954c0dc5fd820d135e8cd0599ce87903a1c0 Gerrit-Change-Number: 84796 Gerrit-PatchSet: 6 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 18 Dec 2024 21:47:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

[XS] Change in coreboot[main]: util/intelmetool: Add Ice Lake support
by Christian Walter (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Name of user not set #1005688. Christian Walter has posted comments on this change by Name of user not set #1005688. ( https://review.coreboot.org/c/coreboot/+/85660?usp=email ) Change subject: util/intelmetool: Add Ice Lake support ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: I think we removed Icelake a couple of releases ago from the tree - are you planning to bring it back in? -- To view, visit https://review.coreboot.org/c/coreboot/+/85660?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I11045c6456e73057719a9811f5bb4e4ca4fb4543 Gerrit-Change-Number: 85660 Gerrit-PatchSet: 2 Gerrit-Owner: Name of user not set #1005688 Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Name of user not set #1005688 Gerrit-Comment-Date: Wed, 18 Dec 2024 21:32:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in coreboot[main]: util/intelmetool: Add Ice Lake support
by Name of user not set (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Christian Walter. Hello Christian Walter, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85660?usp=email to look at the new patch set (#2). Change subject: util/intelmetool: Add Ice Lake support ...................................................................... util/intelmetool: Add Ice Lake support Tested on an Asus X415JA with an i3-1005G1, seems to be working fine. Change-Id: I11045c6456e73057719a9811f5bb4e4ca4fb4543 Signed-off-by: libreandre <openrc(a)posteo.de> --- M util/intelmetool/intelmetool.h 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/85660/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85660?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I11045c6456e73057719a9811f5bb4e4ca4fb4543 Gerrit-Change-Number: 85660 Gerrit-PatchSet: 2 Gerrit-Owner: Name of user not set #1005688 Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>

1 0

[M] Change in coreboot[main]: mb/erying/tgl: Cleanup the code
by Alicja Michalska (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Alicja Michalska. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85641?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: mb/erying/tgl: Cleanup the code ...................................................................... mb/erying/tgl: Cleanup the code This patch cleans the code by: - Removing CMOS layout (I need to sit down and write layout) - Moving SuperIO configuration to bootblock to make devtree cleaner Additionally, it fixes responsiveness of the power button (it was flaky before), and S3 state *almost* works now (RAM content doesn't get lost, but for some reason Memory Controller wipes MRC Cache on resume from S3). Change-Id: I9e395220025fc13e0589472e4732ea1495668554 Signed-off-by: Alicja Michalska <ahplka19(a)gmail.com> --- M src/mainboard/erying/tgl/Kconfig M src/mainboard/erying/tgl/bootblock.c D src/mainboard/erying/tgl/cmos.layout M src/mainboard/erying/tgl/devicetree.cb 4 files changed, 21 insertions(+), 61 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/85641/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/85641?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I9e395220025fc13e0589472e4732ea1495668554 Gerrit-Change-Number: 85641 Gerrit-PatchSet: 3 Gerrit-Owner: Alicja Michalska <ahplka19(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alicja Michalska <ahplka19(a)gmail.com>

1 0

[XS] Change in coreboot[main]: util/intelmetool: Add Ice Lake support
by Name of user not set (Code Review) 18 Dec '24

18 Dec '24

Name of user not set #1005688 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85660?usp=email ) Change subject: util/intelmetool: Add Ice Lake support ...................................................................... util/intelmetool: Add Ice Lake support Change-Id: I11045c6456e73057719a9811f5bb4e4ca4fb4543 Signed-off-by: libreandre <openrc(a)posteo.de> --- M util/intelmetool/intelmetool.h 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/85660/1 diff --git a/util/intelmetool/intelmetool.h b/util/intelmetool/intelmetool.h index 3946f95..e4d012a 100644 --- a/util/intelmetool/intelmetool.h +++ b/util/intelmetool/intelmetool.h @@ -457,6 +457,7 @@ #define PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI1 0xA2BA /* Union Point MEI #1 */ #define PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI2 0xA2BB /* Union Point MEI #2 */ #define PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI3 0xA2BE /* Union Point MEI #3 */ +#define PCI_DEVICE_ID_INTEL_ICELAKE 0x34E0 /* Ice Lake */ #define PCI_DEV_HAS_SUPPORTED_ME(x) ( \ ((x) == PCI_DEVICE_ID_INTEL_COUGARPOINT_1) || \ @@ -515,4 +516,5 @@ ((x) == PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI1) || \ ((x) == PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI2) || \ ((x) == PCI_DEVICE_ID_INTEL_UNIONPOINT_MEI3) || \ + ((x) == PCI_DEVICE_ID_INTEL_ICELAKE) || \ 0) -- To view, visit https://review.coreboot.org/c/coreboot/+/85660?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I11045c6456e73057719a9811f5bb4e4ca4fb4543 Gerrit-Change-Number: 85660 Gerrit-PatchSet: 1 Gerrit-Owner: Name of user not set #1005688

1 0

[M] Change in coreboot[main]: payloads/external/Makefile.mk: build iPXE for EDK2 with custom boot o...
by Benjamin Doron (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Felix Singer, Filip Lewiński, Krystian Hebel, Lean Sheng Tan, Maciej Pijanowski, Martin L Roth, Michał Kopeć, Michał Żygowski. Benjamin Doron has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82721?usp=email ) Change subject: payloads/external/Makefile.mk: build iPXE for EDK2 with custom boot option name ...................................................................... Patch Set 5: (1 comment) File payloads/external/edk2/Makefile: https://review.coreboot.org/c/coreboot/+/82721/comment/e876301f_c39d0a11?us… : PS3, Line 152: The Dasharo repository has the following additional options: > > Could you follow the similar pattern from MrChromebox to create this kconfig (EDK2_REPO_DASHARO or […] I agree. People add a feature here that's in their fork. If it's a fairly popular one though, it's bound to end up either upstreamed or cherry-picked. For example, secure boot is set as conditional to your fork, Matt, but we've upstreamed an implementation. I'd really like to stop making features conditional on particular people's repos. The EDK2 buildsystem won't mind; if some build variable doesn't exist, it just continues. It might confuse users though, who will come running to all their different downstreams wondering why CSM doesn't work (I'm picking a fairly uncommon example for this, and I know that I haven't upstreamed my old implementation, nor 3mdeb their AMD-compatible one). -- To view, visit https://review.coreboot.org/c/coreboot/+/82721?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ied61c7b8aa7a34261d6c6f7fd089b1affdc7d3f6 Gerrit-Change-Number: 82721 Gerrit-PatchSet: 5 Gerrit-Owner: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Filip Lewiński <filip.lewinski(a)3mdeb.com> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 20:46:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Filip Lewiński <filip.lewinski(a)3mdeb.com> Comment-In-Reply-To: Lean Sheng Tan <sheng.tan(a)9elements.com>

1 0

[S] Change in coreboot[main]: util/crossgcc: Add libstdcxx target
by Raul Rangel (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Felix Singer, Jon Murphy, Karthik Ramasubramanian, Martin L Roth. Raul Rangel has posted comments on this change by Jon Murphy. ( https://review.coreboot.org/c/coreboot/+/85275?usp=email ) Change subject: util/crossgcc: Add libstdcxx target ...................................................................... Patch Set 14: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/85275?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ie0c06ffaeab632c27a992dee8abcc403cceabeed Gerrit-Change-Number: 85275 Gerrit-PatchSet: 14 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 19:14:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[XL] Change in coreboot[main]: drivers/option: Add forms in cbtables
by Lean Sheng Tan (Code Review) 18 Dec '24

18 Dec '24

Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/74121?usp=email ) Change subject: drivers/option: Add forms in cbtables ...................................................................... drivers/option: Add forms in cbtables Introduce a mechanism so that coreboot can provide a list of options to post-coreboot code. The options are grouped together into forms and have a meaning name and optional help text. This can be used to let payloads know which options should be displayed in a setup menu, for instance. Although this system was written to be used with edk2, it has been designed with flexibility in mind so that other payloads can also make use of this mechanism. The system currently lacks a way to describe where to find option values. This information is stored in a set of data structures specifically created for this purpose. This format is known as CFR, which means "coreboot forms representation" or "cursed forms representation". Although the "forms representation" is borrowed from UEFI, CFR can be used in non-UEFI scenarios as well. The data structures are implemented as an extension of cbtables records to support nesting. It should not break backwards compatibility because the CFR root record (LB_TAG_CFR_ROOT) size includes all of its children records. The concept of record nesting is borrowed from the records for CMOS options. It is not possible to reuse the CMOS records because they are too closely coupled with CMOS options; using these structures would needlessly restrict more capable backends to what can be done with CMOS options, which is undesired. Because CFR supports variable-length components, directly transforming options into CFR structures is not a trivial process. Furthermore, CFR structures need to be written in one go. Because of this, abstractions exist to generate CFR structures from a set of "setup menu" structures that are coreboot-specific and could be integrated with the devicetree at some point. Note that `struct sm_object` is a tagged union. This is used to have lists of options in an array, as building linked lists of options at runtime is extremely impractical because options would have to be added at the end of the linked list to maintain option order. To avoid mistakes defining `struct sm_object` values, helper macros exist for supported option types. The macros also provide some type checking as they initialise specific union members. It should be possible to extend CFR support for more sophisticated options like fan curve points. Feedback about this is highly appreciated. Change-Id: I304de7d26d79245a2e31a6d01f6c5643b31cb772 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/74121 Reviewed-by: Christian Walter <christian.walter(a)9elements.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- A Documentation/drivers/cfr.md A Documentation/drivers/cfr_internal.md M Documentation/drivers/index.md A src/commonlib/include/commonlib/cfr.h M src/commonlib/include/commonlib/coreboot_tables.h A src/drivers/option/Kconfig A src/drivers/option/Makefile.mk A src/drivers/option/cfr.c A src/drivers/option/cfr_frontend.h M src/include/boot/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/program.ld 12 files changed, 1,100 insertions(+), 0 deletions(-) Approvals: Christian Walter: Looks good to me, approved build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/Documentation/drivers/cfr.md b/Documentation/drivers/cfr.md new file mode 100644 index 0000000..ade6d2b --- /dev/null +++ b/Documentation/drivers/cfr.md @@ -0,0 +1,297 @@ +# CFR - coreboot form representation + +This documents the API exposed by coreboot to be consumed by +loaded OS image or payload. + +## Problem Statement + +As per coreboot design there's no UI present to change firmware +related options like "Hyper-Theading Enable". There's no way of +knowing what options are supported, if they are supported in the +current configuration and what they do. + +The `USE_OPTION_TABLE` Kconfig allows to integrate a list of +mainboard specific options into coreboot tables when the option +API is using the *CMOS NVRAM*. It has no meaning if another option +API is being used. + +## Design Proposal + +Propose a new coreboot table that is independent from the option +backend. The coreboot table is generated from coreboot ramstage +code. + +Every possible boot option is described by its name, the user +visible name, a help text, a default value and status flags. +All strings are in ASCII. + +The boot options are grouped into forms, where each form hold +one ore more options. Boot options that are not used in the current +boot flow, and are never reachable should be marked as hidden. +Dependecies between options can be defined in the code and should +be evaluated by the CFR parser/UI. + +A boot option can be one of the following types: +- boolean +- number +- enum +- string + +All of the information is *Position Independent Data*. That is, it is +safe to relocate any of the information without its meaning/correctness +changing. + +CFR records form a tree structure. Every record starts with a `tag` +and a `size` field as generic header: + +```C +struct __packed lb_cfr_header { + uint32_t tag; + uint32_t size; +}; +``` + +The size of a record includes the size of its own fields plus the size of all +child records. A record can have none or multiple child records. +The record `tag` must be known by the parser to parse the record and its +sub records. If it is not known to the parser it can simply skip it by +jumping `size` bytes forward. + +The coreboot table containing the CFR tree has the tag `LB_TAG_CFR`. + +The public API can be found in +`src/commonlib/include/commonlib/cfr.h` and +`src/commonlib/include/commonlib/coreboot_tables.h`. + +## Implementation design +### Tags +Tags identify the structure defined in `src/commonlib/include/commonlib/cfr.h`. +Every struct might be immideatly followed by additional structs (so called +sub nodes), having their own tag and size field. The sum of all sub nodes size +fields plus the size of the struct itself equals the size field. + +* CFR_TAG_OPTION_FORM + + Used in `struct lb_cfr_option_form` to describe a group of options. Every + sub node is one option that should be displayed in the order found in + memory. + + Allowed sub nodes: + - `CFR_TAG_OPTION_ENUM` + - `CFR_TAG_OPTION_NUMBER` + - `CFR_TAG_OPTION_BOOL` + - `CFR_TAG_OPTION_VARCHAR` + - `CFR_TAG_OPTION_FORM` + - `CFR_TAG_OPTION_COMMENT` + - `CFR_TAG_VARCHAR_UI_NAME` + + Required sub nodes: + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_ENUM_VALUE + + Used in `struct lb_cfr_enum_value` to describe a numeric value to be + used in a parent `CFR_TAG_OPTION_ENUM`. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_UI_NAME` + + Required sub nodes: + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_OPTION_ENUM + + Used in `struct lb_cfr_numeric_option` to describe a numeric variable with + a predefined selection of possible values in the referenced variable. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_ENUM_VALUE` + - `CFR_TAG_VARCHAR_UI_HELPTEXT` + + Required sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_ENUM_VALUE` + +* CFR_TAG_OPTION_NUMBER + + Used in `struct lb_cfr_numeric_option` to describe a numeric variable with + any possible value in the referenced variable. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_VARCHAR_UI_HELPTEXT` + + Required sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_OPTION_BOOL + + Used in `struct lb_cfr_numeric_option` to describe a numeric variable with + the possible values [0, 1] in the referenced variable. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_VARCHAR_UI_HELPTEXT` + + Required sub nodes: + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_OPTION_VARCHAR + + Used in `struct lb_cfr_varchar_option` to describe an ASCII string + stored in the referenced variable. + + *Example:* Linux kernel cmdline. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_DEF_VALUE` + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_VARCHAR_UI_HELPTEXT` + + Required sub nodes: + - `CFR_TAG_VARCHAR_DEF_VALUE` + - `CFR_TAG_VARCHAR_OPT_NAME` + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_OPTION_COMMENT + + Used in `struct lb_cfr_option_comment` to describe an ASCII string visible + to the user, but doesn't reference a variable. Informal use only. + + Allowed sub nodes: + - `CFR_TAG_VARCHAR_UI_NAME` + - `CFR_TAG_VARCHAR_UI_HELPTEXT` + + Required sub nodes: + - `CFR_TAG_VARCHAR_UI_NAME` + +* CFR_TAG_VARCHAR_OPT_NAME + + Used in `struct lb_cfr_varbinary` to describe the option name used by + coreboot's code. It thus must match what is used in code by + `get_uint_option()`. + + Is not user visible. + +* CFR_TAG_VARCHAR_UI_NAME + + Used in `struct lb_cfr_varbinary` + + User visible name of the option. + +* CFR_TAG_VARCHAR_UI_HELPTEXT + + Used in `struct lb_cfr_varbinary` + + Optional user visible description what is changed by this option. + +* CFR_TAG_VARCHAR_DEF_VALUE + + Used in `struct lb_cfr_varbinary` + + Default value in case the variable is not present. + +### Flags + +The optional flags describe the visibilty of the option and the +effect on the non-volatile variable. + +* `CFR_OPTFLAG_READONLY` + + Prevents writes to the variable. + +* `CFR_OPTFLAG_GRAYOUT` + + Implies `READONLY`. The option is visible, but cannot be modified + because one of the dependencies are not given. However there's a + possibility to enable the option by changing runtime configuration. + + *For example:* Setting SATA mode, but SATA is globally disabled. + +* `CFR_OPTFLAG_SUPPRESS` + + Runtime code sets this flag to indicate that the option has no effect + and is never reachable, not even by changing runtime configuration. + This option is never shown in the UI. + +* `CFR_OPTFLAG_VOLATILE` + + Implies `READONLY`. + The option is not backed by a non-volatile variable. This is useful + to display the current state of a specific component, a dependency or + a serial number. This information could be passed in a new coreboot + table, but it not useful other than to be shown at this spot in the + UI. + +* `CFR_OPTFLAG_RUNTIME` + + The option is allowed to be changed by a post payload entity. On UEFI + this sets the `EFI_VARIABLE_RUNTIME_ACCESS` attribute. + It is out of scope of this specification how non runtime variables + are protected after the payload has hand over control. + +### Example + +To display a boolean option with the label `Boolean`, that default value +is `true`, on a form called `test`, that modifies the variable `First` +the following structure will be generated: + +``` +struct lb_cfr_option_form { + uint32_t tag; = CFR_TAG_OPTION_FORM + uint32_t size; = sizeof(struct lb_cfr_option_form) + + sizeof(struct lb_cfr_varbinary) + + strlen(name) + 1 + 3 + + sizeof(struct lb_cfr_numeric_option) + + sizeof(struct lb_cfr_varbinary) + + strlen(optname) + 1 + 2 + + sizeof(struct lb_cfr_varbinary) + + strlen(uiname) + 1 = 120 + uint64_t object_id; = 1 + uint64_t dependency_id; = 0 + uint32_t flags; = 0 +} + struct lb_cfr_varbinary { + uint32_t tag; = CFR_TAG_VARCHAR_UI_NAME + uint32_t size; = sizeof(struct lb_cfr_varbinary) + + strlen(name) + 1 + 3 = 20 + uint32_t data_length; = strlen(name) + 1 + }; + char name[5]; = "test" + char padding[3]; + struct lb_cfr_numeric_option { + uint32_t tag; = CFR_TAG_OPTION_BOOL + uint32_t size; = sizeof(struct lb_cfr_numeric_option) + + sizeof(struct lb_cfr_varbinary) + + strlen(optname) + 1 + 2 + + sizeof(struct lb_cfr_varbinary) + + strlen(uiname) + 1 = 72 + uint64_t object_id; = 2 + uint64_t dependency_id; = 0 + uint32_t flags; = 0 + uint32_t default_value; = true + }; + struct lb_cfr_varbinary { + uint32_t tag; = CFR_TAG_VARCHAR_OPT_NAME + uint32_t size; = sizeof(struct lb_cfr_varbinary) + + strlen(optname) + 1 + 2 = 20 + uint32_t data_length; = strlen(optname) + 1 = 6 + }; + char optname[6]; = "First" + char padding[2]; + struct lb_cfr_varbinary { + uint32_t tag; = CFR_TAG_VARCHAR_UI_NAME + uint32_t size; = sizeof(struct lb_cfr_varbinary) + + strlen(uiname) + 1 = 20 + uint32_t data_length; = strlen(uiname) + 1 = 8 + }; + char uiname[8]; = "Boolean" +``` \ No newline at end of file diff --git a/Documentation/drivers/cfr_internal.md b/Documentation/drivers/cfr_internal.md new file mode 100644 index 0000000..95fef66 --- /dev/null +++ b/Documentation/drivers/cfr_internal.md @@ -0,0 +1,114 @@ +# CFR - coreboot form representation - Internals + +This documents the API internally used by coreboot to be used +by ramstage code. + +Please read [CFR](cfr.md) first. + +## Enabling CFR support + +Users should select `DRIVERS_OPTION_CFR` in Kconfig to enable CFR +support. Mainboards should select `DRIVERS_OPTION_CFR_ENABLED` to +enable `DRIVERS_OPTION_CFR` by default. + +## Using CFR + +When CFR support is enabled there are two possibilites to generate +the records: + +- mainboard specific code +- automatic collection + +In both cases you have to add `C` structs in ramstage to describe the +option and group them together into a form. CFR objects should reside +on the heap as they can be modified to match the current boot flow. + +### Updating CFR options + +The CFR options should be updated before tables are written. +You can use a callback, using the `WITH_CALLBACK()` macro, to update +single or multiple options when the CFR table is written into the +coreboot table. + +**Example:** Updates the option serial_number with the contents from the +EMI eeprom. + +``` +static void update_serial(const struct sm_object *obj, struct sm_object *new) +{ + new->sm_varchar.default_value = get_emi_eeprom_vpd()->serial_number; +} + +static const struct sm_object serial_number = SM_DECLARE_VARCHAR({ + .flags = CFR_OPTFLAG_READONLY | CFR_OPTFLAG_VOLATILE, + .opt_name = "serial_number", + .ui_name = "Serial Number", +}, WITH_CALLBACK(update_serial)); +``` + +### Dependencies in CFR options + +The CFR options can have a dependency that must be evaluated at runtime by +the OS/payload that parses the CFR record and displays the UI. +By using the `WITH_DEP()` macro you can specify another numberic option that +is checked to hide the current option. + +**Example:** Declares a dependency from `sata_disable_port0` to `sata_enable`. +The option `sata_disable_port0` will be hidden as long as "sata_enable" is 0. +When the user changes "sata_enable" to 1 or it was 1 then the option +`sata_disable_port0` should be visible. + +``` +static struct sm_object sata_enable = SM_DECLARE_BOOL({ + .flags = CFR_OPTFLAG_RUNTIME, + .opt_name = "sata_enable", + .ui_name = "Enable SATA controller", + .ui_helptext = NULL, + .default_value = true, +}); + +static struct sm_object sata_disable_port0 = SM_DECLARE_BOOL({ + .flags = CFR_OPTFLAG_RUNTIME, + .opt_name = "sata_disable_port0", + .ui_name = "Disable SATA port #0", + .ui_helptext = NULL, + .default_value = false, +}, WITH_DEP(&sata_enable)); +``` + +### Providing mainboard custom options + +A mainboard that uses CFR can provide a list of custom options +be overwriting the weak `void mb_cfr_setup_menu(struct lb_cfr *cfr_root);` +function in ramstage. + +### Automatic CFR collection + +CFR forms that have the `__cfr_form` attribute are automatically collected +and inserted into the coreboot table. + +## Example + +The following CFR form `southbridge` will be automatically added to the +coreboot table and it will have a single option called `Enable NMI` that +allows the variable `nmi` to be changed to *0* or *1*. + +**Example:** +```C +static struct sm_object nmi = SM_DECLARE_BOOL({ + .flags = CFR_OPTFLAG_RUNTIME, + .opt_name = "nmi", + .ui_name = "Enable NMI", + .ui_helptext = NULL, + .default_value = false, +}); + +static const __cfr_form struct sm_obj_form southbridge = { + .flags = 0, + .ui_name = "Southbridge", + .obj_list = (const struct sm_object *[]) { + &nmi, + NULL + }, +}; +``` \ No newline at end of file diff --git a/Documentation/drivers/index.md b/Documentation/drivers/index.md index efbc186..45c491b 100644 --- a/Documentation/drivers/index.md +++ b/Documentation/drivers/index.md @@ -18,6 +18,8 @@ ```{toctree} :maxdepth: 1 +CFR <cfr.md> +CFR use within coreboot <cfr_internal.md> Intel DPTF <dptf.md> IPMI KCS <ipmi_kcs.md> SMMSTORE <smmstore.md> diff --git a/src/commonlib/include/commonlib/cfr.h b/src/commonlib/include/commonlib/cfr.h new file mode 100644 index 0000000..22058e9 --- /dev/null +++ b/src/commonlib/include/commonlib/cfr.h @@ -0,0 +1,196 @@ +/* SPDX-License-Identifier: BSD-3-Clause */ + +#ifndef COMMONLIB_OPTION_CFR_H +#define COMMONLIB_OPTION_CFR_H + +#include <types.h> + +/* + * PUBLIC API + * + * The CFR table information is for conveying available boot + * option of the firmware to the loaded OS image or payload. + * Primarily this is expected to be information that is user + * visible. + * + * The boot options are grouped into forms, where each form hold + * one ore more options. + * Boot options that are not used in the current boot flow, but + * are reachable under certain conditions should be marked as hidden. + * + * A boot option can be one of the following types: + * - boolean + * - number + * - enum + * - string + * + * Each boot option has an UI name and UI help text that is free to + * choose. All strings are in ASCII. + * + * All of the information should be Position Independent Data. + * That is it should be safe to relocated any of the information + * without it's meaning/correctness changing. For table that + * can reasonably be used on multiple architectures the data + * size should be fixed. This should ease the transition between + * 32 bit and 64 bit architectures etc. + * + * CFR records form a tree structure. The size of a record includes + * the size of its own fields plus the size of all children records. + * CFR tags can appear multiple times except for `LB_TAG_CFR` which + * is used for the root record. + * + * The following structures have comments that describe the supported + * children records. These comments cannot be replaced with code! The + * structures are variable-length, so the offsets won't be valid most + * of the time. Besides, the implementation uses `sizeof()` to obtain + * the size of the "record header" (the fixed-length members); adding + * the children structures as struct members will increase the length + * returned by `sizeof()`, which complicates things for zero reason. + * + */ + +enum cfr_tags { + CFR_TAG_OPTION_FORM = 1, + CFR_TAG_ENUM_VALUE = 2, + CFR_TAG_OPTION_ENUM = 3, + CFR_TAG_OPTION_NUMBER = 4, + CFR_TAG_OPTION_BOOL = 5, + CFR_TAG_OPTION_VARCHAR = 6, + CFR_TAG_VARCHAR_OPT_NAME = 7, + CFR_TAG_VARCHAR_UI_NAME = 8, + CFR_TAG_VARCHAR_UI_HELPTEXT = 9, + CFR_TAG_VARCHAR_DEF_VALUE = 10, + CFR_TAG_OPTION_COMMENT = 11, +}; + +/* + * The optional flags describe the visibilty of the option and the + * effect on the non-volatile variable. + * CFR_OPTFLAG_READONLY: + * Prevents writes to the variable. + * CFR_OPTFLAG_GRAYOUT: + * Implies READONLY. The option is visible, but cannot be modified + * because one of the dependencies are not given. However there's a + * possibility to enable the option by changing runtime configuration. + * + * For example: Setting SATA mode, but SATA is globally disabled. + * CFR_OPTFLAG_SUPPRESS: + * Runtime code sets this flag to indicate that the option has no effect + * and is never reachable, not even by changing runtime configuration. + * This option is never shown in the UI. + * CFR_OPTFLAG_VOLATILE: + * Implies READONLY. + * The option is not backed by a non-volatile variable. This is useful + * to display the current state of a specific component, a dependency or + * a serial number. This information could be passed in a new coreboot + * table, but it not useful other than to be shown at this spot in the + * UI. + * CFR_OPTFLAG_RUNTIME: + * The option is allowed to be changed by a post payload entity. On UEFI + * this sets the EFI_VARIABLE_RUNTIME_ACCESS attribute. + * It is out of scope of this specification how non runtime variables + * are protected after the payload has hand over control. + */ +enum cfr_option_flags { + CFR_OPTFLAG_READONLY = 1 << 0, + CFR_OPTFLAG_GRAYOUT = 1 << 1, + CFR_OPTFLAG_SUPPRESS = 1 << 2, + CFR_OPTFLAG_VOLATILE = 1 << 3, + CFR_OPTFLAG_RUNTIME = 1 << 4, +}; + +struct __packed lb_cfr_varbinary { + uint32_t tag; /* + * CFR_TAG_VARCHAR_OPT_NAME, CFR_TAG_VARCHAR_UI_NAME, + * CFR_TAG_VARCHAR_UI_HELPTEXT or CFR_TAG_VARCHAR_DEF_VALUE + */ + uint32_t size; /* Length of the entire structure */ + uint32_t data_length; /* Length of data, including NULL terminator for strings */ +}; + +struct __packed lb_cfr_enum_value { + uint32_t tag; /* CFR_TAG_ENUM_VALUE */ + uint32_t size; + uint32_t value; + /* + * struct lb_cfr_varbinary ui_name + */ +}; + +/* Supports multiple option types: ENUM, NUMBER, BOOL */ +struct __packed lb_cfr_numeric_option { + uint32_t tag; /* + * CFR_TAG_OPTION_ENUM, CFR_TAG_OPTION_NUMBER or + * CFR_TAG_OPTION_BOOL + */ + uint32_t size; + uint64_t object_id; /* Uniqueue ID */ + uint64_t dependency_id; /* Grayout if value of lb_cfr_numeric_option with given ID is 0. + * Ignore if field is 0. + */ + uint32_t flags; /* enum cfr_option_flags */ + uint32_t default_value; + /* + * struct lb_cfr_varbinary opt_name + * struct lb_cfr_varbinary ui_name + * struct lb_cfr_varbinary ui_helptext (Optional) + * struct lb_cfr_enum_value enum_values[] + */ +}; + +struct __packed lb_cfr_varchar_option { + uint32_t tag; /* CFR_TAG_OPTION_VARCHAR */ + uint32_t size; + uint64_t object_id; /* Uniqueue ID */ + uint64_t dependency_id; /* Grayout if value of lb_cfr_numeric_option with given ID is 0. + * Ignore if field is 0. + */ + uint32_t flags; /* enum cfr_option_flags */ + /* + * struct lb_cfr_varbinary default_value + * struct lb_cfr_varbinary opt_name + * struct lb_cfr_varbinary ui_name + * struct lb_cfr_varbinary ui_helptext (Optional) + */ +}; + +/* + * A CFR option comment is roughly equivalent to a Kconfig comment. + * Option comments are *NOT* string options (see CFR_OPTION_VARCHAR + * instead) but they're considered an option for simplicity's sake. + */ +struct __packed lb_cfr_option_comment { + uint32_t tag; /* CFR_TAG_OPTION_COMMENT */ + uint32_t size; + uint64_t object_id; /* Uniqueue ID */ + uint64_t dependency_id; /* Grayout if value of lb_cfr_numeric_option with given ID is 0. + * Ignore if field is 0. + */ + uint32_t flags; /* enum cfr_option_flags */ + /* + * struct lb_cfr_varbinary ui_name + * struct lb_cfr_varbinary ui_helptext (Optional) + */ +}; + +/* CFR forms are considered options as they can be nested inside other forms */ +struct __packed lb_cfr_option_form { + uint32_t tag; /* CFR_TAG_OPTION_FORM */ + uint32_t size; + uint64_t object_id; /* Uniqueue ID */ + uint64_t dependency_id; /* Grayout if value of lb_cfr_numeric_option with given ID is 0. + * Ignore if field is 0. + */ + uint32_t flags; /* enum cfr_option_flags */ + /* + * struct lb_cfr_varbinary ui_name + * struct lb_cfr_varchar_option options[] + */ +}; + +struct __packed lb_cfr_header { + uint32_t tag; + uint32_t size; +}; + +#endif /* DRIVERS_OPTION_CFR_H */ diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 7cbc7da..def48b7 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -89,6 +89,7 @@ LB_TAG_PCIE = 0x0044, LB_TAG_EFI_FW_INFO = 0x0045, LB_TAG_CAPSULE = 0x0046, + LB_TAG_CFR_ROOT = 0x0047, /* The following options are CMOS-related */ LB_TAG_CMOS_OPTION_TABLE = 0x00c8, LB_TAG_OPTION = 0x00c9, @@ -598,5 +599,11 @@ uint32_t lowest_supported_version; /* Lowest allowed version for downgrades */ uint32_t fw_size; /* Size of firmware in bytes */ } __packed; +struct lb_cfr { + uint32_t tag; + uint32_t size; + uint32_t checksum; /* Checksum of the variable payload. */ + /* struct lb_cfr_option_form forms[] */ +}; #endif diff --git a/src/drivers/option/Kconfig b/src/drivers/option/Kconfig new file mode 100644 index 0000000..6c63bb4 --- /dev/null +++ b/src/drivers/option/Kconfig @@ -0,0 +1,6 @@ +config DRIVERS_OPTION_CFR_ENABLED + def_bool n + +config DRIVERS_OPTION_CFR + bool "Support generating a CFR list of options" + default y if DRIVERS_OPTION_CFR_ENABLED diff --git a/src/drivers/option/Makefile.mk b/src/drivers/option/Makefile.mk new file mode 100644 index 0000000..7ca439d --- /dev/null +++ b/src/drivers/option/Makefile.mk @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only + +ramstage-$(CONFIG_DRIVERS_OPTION_CFR) += cfr.c diff --git a/src/drivers/option/cfr.c b/src/drivers/option/cfr.c new file mode 100644 index 0000000..b323b43 --- /dev/null +++ b/src/drivers/option/cfr.c @@ -0,0 +1,330 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <assert.h> +#include <boot/coreboot_tables.h> +#include <commonlib/coreboot_tables.h> +#include <console/console.h> +#include <crc_byte.h> +#include <drivers/option/cfr_frontend.h> +#include <inttypes.h> +#include <string.h> +#include <types.h> + +static uint32_t cfr_record_size(const char *startp, const char *endp) +{ + const uintptr_t start = (uintptr_t)startp; + const uintptr_t end = (uintptr_t)endp; + + if (start > end || end - start > UINT32_MAX) { + /* + * Should never be reached unless something went really + * wrong. Record size can never be negative, and things + * would break long before record length exceeds 4 GiB. + */ + die("%s: bad record size (start = %" PRIxPTR ", end = %" PRIxPTR ")", + __func__, start, end); + } + return (uint32_t)(end - start); +} + +static uint32_t write_cfr_varchar(char *current, const char *string, uint32_t tag) +{ + uint8_t *data; + size_t padding; + + ASSERT(string); + if (!string) + return 0; + + struct lb_cfr_varbinary *cfr_str = (struct lb_cfr_varbinary *)current; + cfr_str->tag = tag; + cfr_str->data_length = strlen(string) + 1; + char *data = current + sizeof(*cfr_str); + memcpy(data, string, cfr_str->data_length); + + /* Make sure that every TAG/SIZE field is always aligned to LB_ENTRY_ALIGN */ + cfr_str->size = ALIGN_UP(sizeof(*cfr_str) + cfr_str->data_length, LB_ENTRY_ALIGN); + + return cfr_str->size; +} + +static uint32_t sm_write_string_default_value(char *current, const char *string) +{ + return write_cfr_varchar(current, string ? string : "", CFR_TAG_VARCHAR_DEF_VALUE); +} + +static uint32_t sm_write_opt_name(char *current, const char *string) +{ + return write_cfr_varchar(current, string, CFR_TAG_VARCHAR_OPT_NAME); +} + +static uint32_t sm_write_ui_name(char *current, const char *string) +{ + return write_cfr_varchar(current, string, CFR_TAG_VARCHAR_UI_NAME); +} + +static uint32_t sm_write_ui_helptext(char *current, const char *string) +{ + /* UI Helptext is optional, return if nothing to display */ + if (!string || !strlen(string)) + return 0; + + return write_cfr_varchar(current, string, CFR_TAG_VARCHAR_UI_HELPTEXT); +} + +static uint32_t sm_write_enum_value(char *current, const struct sm_enum_value *e) +{ + struct lb_cfr_enum_value *enum_val = (struct lb_cfr_enum_value *)current; + enum_val->tag = CFR_TAG_ENUM_VALUE; + enum_val->value = e->value; + enum_val->size = sizeof(*enum_val); + + current += enum_val->size; + current += sm_write_ui_name(current, e->ui_name); + + enum_val->size = cfr_record_size((char *)enum_val, current); + return enum_val->size; +} + +static uint32_t write_numeric_option(char *current, uint32_t tag, const uint64_t object_id, + const char *opt_name, const char *ui_name, const char *ui_helptext, + uint32_t flags, uint32_t default_value, const struct sm_enum_value *values, + const uint64_t dep_id) +{ + struct lb_cfr_numeric_option *option = (struct lb_cfr_numeric_option *)current; + size_t len; + + option->tag = tag; + option->object_id = object_id; + option->dependency_id = dep_id; + option->flags = flags; + if (option->flags & (CFR_OPTFLAG_GRAYOUT | CFR_OPTFLAG_VOLATILE)) + option->flags |= CFR_OPTFLAG_READONLY; + option->default_value = default_value; + option->size = sizeof(*option); + + current += option->size; + len = sm_write_opt_name(current, opt_name); + if (!len) + return 0; + current += len; + len = sm_write_ui_name(current, ui_name); + if (!len) + return 0; + current += len; + current += sm_write_ui_helptext(current, ui_helptext); + + if (option->tag == CFR_TAG_OPTION_ENUM && values) { + for (const struct sm_enum_value *e = values; e->ui_name; e++) { + current += sm_write_enum_value(current, e); + } + } + + option->size = cfr_record_size((char *)option, current); + return option->size; +} + +static uint32_t sm_write_opt_enum(char *current, const struct sm_obj_enum *sm_enum, + const uint64_t object_id, const uint64_t dep_id) + +{ + return write_numeric_option(current, CFR_TAG_OPTION_ENUM, object_id, + sm_enum->opt_name, sm_enum->ui_name, sm_enum->ui_helptext, + sm_enum->flags, sm_enum->default_value, sm_enum->values, + dep_id); +} + +static uint32_t sm_write_opt_number(char *current, const struct sm_obj_number *sm_number, + const uint64_t object_id, const uint64_t dep_id) + +{ + return write_numeric_option(current, CFR_TAG_OPTION_NUMBER, object_id, + sm_number->opt_name, sm_number->ui_name, sm_number->ui_helptext, + sm_number->flags, sm_number->default_value, NULL, dep_id); +} + +static uint32_t sm_write_opt_bool(char *current, const struct sm_obj_bool *sm_bool, + const uint64_t object_id, const uint64_t dep_id) + +{ + return write_numeric_option(current, CFR_TAG_OPTION_BOOL, object_id, + sm_bool->opt_name, sm_bool->ui_name, sm_bool->ui_helptext, + sm_bool->flags, sm_bool->default_value, NULL, dep_id); +} + +static uint32_t sm_write_opt_varchar(char *current, const struct sm_obj_varchar *sm_varchar, + const uint64_t object_id, const uint64_t dep_id) + +{ + struct lb_cfr_varchar_option *option = (struct lb_cfr_varchar_option *)current; + size_t len; + + option->tag = CFR_TAG_OPTION_VARCHAR; + option->object_id = object_id; + option->dependency_id = dep_id; + option->flags = sm_varchar->flags; + if (option->flags & (CFR_OPTFLAG_GRAYOUT | CFR_OPTFLAG_VOLATILE)) + option->flags |= CFR_OPTFLAG_READONLY; + option->size = sizeof(*option); + + current += option->size; + current += sm_write_string_default_value(current, sm_varchar->default_value); + len = sm_write_opt_name(current, sm_varchar->opt_name); + if (!len) + return 0; + current += len; + len = sm_write_ui_name(current, sm_varchar->ui_name); + if (!len) + return 0; + current += len; + current += sm_write_ui_helptext(current, sm_varchar->ui_helptext); + + option->size = cfr_record_size((char *)option, current); + return option->size; +} + +static uint32_t sm_write_opt_comment(char *current, const struct sm_obj_comment *sm_comment, + const uint32_t object_id, const uint32_t dep_id) +{ + struct lb_cfr_option_comment *comment = (struct lb_cfr_option_comment *)current; + size_t len; + + comment->tag = CFR_TAG_OPTION_COMMENT; + comment->object_id = object_id; + comment->dependency_id = dep_id; + comment->flags = sm_comment->flags; + if (comment->flags & (CFR_OPTFLAG_GRAYOUT | CFR_OPTFLAG_VOLATILE)) + comment->flags |= CFR_OPTFLAG_READONLY; + comment->size = sizeof(*comment); + + current += comment->size; + len = sm_write_ui_name(current, sm_comment->ui_name); + if (!len) + return 0; + current += len; + current += sm_write_ui_helptext(current, sm_comment->ui_helptext); + + comment->size = cfr_record_size((char *)comment, current); + return comment->size; +} + +static uint64_t sm_gen_obj_id(void *ptr) +{ + uintptr_t id = (uintptr_t)ptr; + /* Convert pointer to unique ID */ + return id ^ 0xffffcafecafecafe; +} + +static uint32_t sm_write_object(char *current, const struct sm_object *sm_obj); + +static uint32_t sm_write_form(char *current, struct sm_obj_form *sm_form, + const uint64_t object_id, const uint64_t dep_id) +{ + struct lb_cfr_option_form *form = (struct lb_cfr_option_form *)current; + size_t len; + size_t i = 0; + + form->tag = CFR_TAG_OPTION_FORM; + form->object_id = object_id; + form->dependency_id = dep_id; + form->flags = sm_form->flags; + if (form->flags & (CFR_OPTFLAG_GRAYOUT | CFR_OPTFLAG_VOLATILE)) + form->flags |= CFR_OPTFLAG_READONLY; + form->size = sizeof(*form); + + current += form->size; + len = sm_write_ui_name(current, sm_form->ui_name); + if (!len) + return 0; + current += len; + + while (sm_form->obj_list[i]) + current += sm_write_object(current, sm_form->obj_list[i++]); + + form->size = cfr_record_size((char *)form, current); + return form->size; +} + +static uint32_t sm_write_object(char *current, const struct sm_object *sm_obj) +{ + uint64_t dep_id, obj_id; + struct sm_object sm_obj_copy; + assert(sm_obj); + + /* Assign uniqueue ID */ + obj_id = sm_gen_obj_id((void *)sm_obj); + + /* Set dependency ID */ + dep_id = 0; + if (sm_obj->dep) { + assert(sm_obj->dep->kind == SM_OBJ_BOOL); + if (sm_obj->dep->kind == SM_OBJ_BOOL) + dep_id = sm_gen_obj_id((void *)sm_obj->dep); + } + + /* Invoke callback to update fields */ + if (sm_obj->ctor) { + memcpy(&sm_obj_copy, sm_obj, sizeof(*sm_obj)); + sm_obj->ctor(sm_obj, &sm_obj_copy); + + assert(sm_obj->kind == sm_obj_copy.kind); + sm_obj = (const struct sm_object *)&sm_obj_copy; + } + + switch (sm_obj->kind) { + case SM_OBJ_NONE: + return 0; + case SM_OBJ_ENUM: + return sm_write_opt_enum(current, &sm_obj->sm_enum, obj_id, + dep_id); + case SM_OBJ_NUMBER: + return sm_write_opt_number(current, &sm_obj->sm_number, obj_id, + dep_id); + case SM_OBJ_BOOL: + return sm_write_opt_bool(current, &sm_obj->sm_bool, obj_id, + dep_id); + case SM_OBJ_VARCHAR: + return sm_write_opt_varchar(current, &sm_obj->sm_varchar, obj_id, + dep_id); + case SM_OBJ_COMMENT: + return sm_write_opt_comment(current, &sm_obj->sm_comment, obj_id, + dep_id); + case SM_OBJ_FORM: + return sm_write_form(current, (struct sm_obj_form *)&sm_obj->sm_form, obj_id, dep_id); + default: + BUG(); + printk(BIOS_ERR, "Unknown setup menu object kind %u, ignoring\n", sm_obj->kind); + return 0; + } +} + +void cfr_write_setup_menu(struct lb_cfr *cfr_root, struct sm_obj_form *sm_root[]) +{ + void *current = cfr_root; + struct sm_obj_form *obj; + size_t i = 0; + + ASSERT(cfr_root); + if (!cfr_root) + return; + + cfr_root->tag = LB_TAG_CFR_ROOT; + cfr_root->size = sizeof(*cfr_root); + + current += cfr_root->size; + while (sm_root && sm_root[i]) + current += sm_write_form(current, sm_root[i++], 0, 0); + + /* + * Add generic forms. + */ + for (obj = &_cfr_forms[0]; obj != &_ecfr_forms[0]; obj++) + current += sm_write_form(current, obj, 0, 0); + + cfr_root->size = cfr_record_size((char *)cfr_root, current); + + cfr_root->checksum = CRC(cfr_root + 1, cfr_root->size - sizeof(*cfr_root), crc32_byte); + + printk(BIOS_DEBUG, "CFR: Written %u bytes of CFR structures at %p, with CRC32 0x%08x\n", + cfr_root->size, cfr_root, cfr_root->checksum); +} diff --git a/src/drivers/option/cfr_frontend.h b/src/drivers/option/cfr_frontend.h new file mode 100644 index 0000000..2161853 --- /dev/null +++ b/src/drivers/option/cfr_frontend.h @@ -0,0 +1,119 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef DRIVERS_OPTION_CFR_H +#define DRIVERS_OPTION_CFR_H + +#include <commonlib/coreboot_tables.h> +#include <commonlib/cfr.h> +#include <types.h> + +/* Front-end */ +struct sm_enum_value { + const char *ui_name; + uint32_t value; +}; + +#define SM_ENUM_VALUE_END ((struct sm_enum_value) {0}) + +struct sm_obj_enum { + uint32_t flags; /* enum cfr_option_flags */ + const char *opt_name; + const char *ui_name; + const char *ui_helptext; + uint32_t default_value; + const struct sm_enum_value *values; +}; + +struct sm_obj_number { + uint32_t flags; /* enum cfr_option_flags */ + const char *opt_name; + const char *ui_name; + const char *ui_helptext; + uint32_t default_value; +}; + +struct sm_obj_bool { + uint32_t flags; /* enum cfr_option_flags */ + const char *opt_name; + const char *ui_name; + const char *ui_helptext; + bool default_value; +}; + +struct sm_obj_varchar { + uint32_t flags; /* enum cfr_option_flags */ + const char *opt_name; + const char *ui_name; + const char *ui_helptext; + const char *default_value; +}; + +struct sm_obj_comment { + uint32_t flags; /* enum cfr_option_flags */ + const char *ui_name; + const char *ui_helptext; +}; + +struct sm_object; + +struct sm_obj_form { + uint32_t flags; /* enum cfr_option_flags */ + const char *ui_name; + const struct sm_object **obj_list; /* NULL terminated */ +}; + +enum sm_object_kind { + SM_OBJ_NONE = 0, + SM_OBJ_ENUM, + SM_OBJ_NUMBER, + SM_OBJ_BOOL, + SM_OBJ_VARCHAR, + SM_OBJ_COMMENT, + SM_OBJ_FORM, +}; + +struct sm_object { + enum sm_object_kind kind; + const struct sm_object *dep; + void (*ctor)(const struct sm_object *obj, struct sm_object *new); /* Called on object creation */ + union { + struct sm_obj_enum sm_enum; + struct sm_obj_number sm_number; + struct sm_obj_bool sm_bool; + struct sm_obj_varchar sm_varchar; + struct sm_obj_comment sm_comment; + struct sm_obj_form sm_form; + }; +}; + +/* sm_object helpers with type checking */ +#define SM_DECLARE_ENUM(...) { .kind = SM_OBJ_ENUM, .dep = NULL, \ + .ctor = NULL, .sm_enum = __VA_ARGS__ } +#define SM_DECLARE_NUMBER(...) { .kind = SM_OBJ_NUMBER, .dep = NULL, \ + .ctor = NULL, .sm_number = __VA_ARGS__ } +#define SM_DECLARE_BOOL(...) { .kind = SM_OBJ_BOOL, .dep = NULL, \ + .ctor = NULL, .sm_bool = __VA_ARGS__ } +#define SM_DECLARE_VARCHAR(...) { .kind = SM_OBJ_VARCHAR, .dep = NULL, \ + .ctor = NULL, .sm_varchar = __VA_ARGS__ } +#define SM_DECLARE_COMMENT(...) { .kind = SM_OBJ_COMMENT, .dep = NULL, \ + .ctor = NULL, .sm_comment = __VA_ARGS__ } +#define SM_DECLARE_FORM(...) { .kind = SM_OBJ_FORM, .dep = NULL, \ + .ctor = NULL, .sm_form = __VA_ARGS__ } + +#define WITH_CALLBACK(c) .ctor = (c) +#define WITH_DEP(d) .dep = (d) + +void cfr_write_setup_menu(struct lb_cfr *cfr_root, struct sm_obj_form *sm_root[]); + +#if ENV_RAMSTAGE +#define __cfr_form __attribute__((used, __section__(".rodata.cfr_forms"))) +#else +#define __cfr_form __maybe_unused +#endif + +/** start of compile time generated cfr form array */ +extern struct sm_obj_form _cfr_forms[]; +/** end of compile time generated cfr form array */ +extern struct sm_obj_form _ecfr_forms[]; + +#endif /* DRIVERS_OPTION_CFR_H */ diff --git a/src/include/boot/coreboot_tables.h b/src/include/boot/coreboot_tables.h index 090889c..9c258c6 100644 --- a/src/include/boot/coreboot_tables.h +++ b/src/include/boot/coreboot_tables.h @@ -53,4 +53,7 @@ /* Add VBOOT VBNV offsets. */ void lb_table_add_vbnv_cmos(struct lb_header *header); +/* Define this in mainboard.c to add board specific CFR entries */ +void mb_cfr_setup_menu(struct lb_cfr *cfr_root); + #endif /* COREBOOT_TABLES_H */ diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index e954edb..3902c88 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -13,6 +13,7 @@ #include <boardid.h> #include <device/device.h> #include <drivers/tpm/tpm_ppi.h> +#include <drivers/option/cfr_frontend.h> #include <fmap.h> #include <fw_config.h> #include <cbfs.h> @@ -346,6 +347,19 @@ return config; } +void __weak mb_cfr_setup_menu(struct lb_cfr *cfr_root) +{ + cfr_write_setup_menu(cfr_root, NULL); +} + +static void lb_cfr_setup_menu(struct lb_header *header) +{ + char *current = (char *)lb_new_record(header); + struct lb_cfr *cfr_root = (struct lb_cfr *)current; + + mb_cfr_setup_menu(cfr_root); +} + #if CONFIG(USE_OPTION_TABLE) static struct cmos_checksum *lb_cmos_checksum(struct lb_header *header) { @@ -491,6 +505,10 @@ } #endif + /* Generate CFR entry for setup menus */ + if (CONFIG(DRIVERS_OPTION_CFR)) + lb_cfr_setup_menu(head); + /* Serialize resource map into mem table types (LB_MEM_*) */ bootmem_write_memory_table(lb_memory(head)); diff --git a/src/lib/program.ld b/src/lib/program.ld index b3ddc0b..93bfb06 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -57,6 +57,11 @@ KEEP(*(.rodata.cpu_driver)); _ecpu_drivers = .; RECORD_SIZE(cpu_drivers) + . = ALIGN(ARCH_POINTER_ALIGN_SIZE); + _cfr_forms = .; + KEEP(*(.rodata.cfr_forms)); + _ecfr_forms = .; + RECORD_SIZE(cfr_forms) #endif . = ALIGN(ARCH_POINTER_ALIGN_SIZE); -- To view, visit https://review.coreboot.org/c/coreboot/+/74121?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I304de7d26d79245a2e31a6d01f6c5643b31cb772 Gerrit-Change-Number: 74121 Gerrit-PatchSet: 30 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Maslowski <info(a)orangecms.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>

1 0

[XL] Change in coreboot[main]: drivers/option: Add forms in cbtables
by Lean Sheng Tan (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Angel Pons, Benjamin Doron, Martin L Roth, Maximilian Brune, Nico Huber, Patrick Rudolph, Sean Rhodes, Werner Zeh. Lean Sheng Tan has posted comments on this change by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/74121?usp=email ) Change subject: drivers/option: Add forms in cbtables ...................................................................... Patch Set 29: (1 comment) Patchset: PS29: > Just two small things. I marked them as resolved right away in order to not block things here. […] Thank you Werner for putting your time into reviewing this. I will create a follow up patch to fix those. Much appreciated! -- To view, visit https://review.coreboot.org/c/coreboot/+/74121?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I304de7d26d79245a2e31a6d01f6c5643b31cb772 Gerrit-Change-Number: 74121 Gerrit-PatchSet: 29 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Maslowski <info(a)orangecms.org> Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 18:16:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>

1 0

[S] Change in coreboot[main]: ec/google/chromeec: Add debug timestamp for host EC commands
by Julius Werner (Code Review) 18 Dec '24

18 Dec '24

Attention is currently required from: Boris Mittelberg, Caveh Jalali, Jayvik Desai, Kapil Porwal, Paul Menzel, Subrata Banik. Julius Werner has posted comments on this change by Jayvik Desai. ( https://review.coreboot.org/c/coreboot/+/85326?usp=email ) Change subject: ec/google/chromeec: Add debug timestamp for host EC commands ...................................................................... Patch Set 11: (1 comment) File src/ec/google/chromeec/ec_lpc.c: https://review.coreboot.org/c/coreboot/+/85326/comment/e9730130_b71b8831?us… : PS9, Line 286: printk > If we have better ways to append the host cmd number into the timestamp string, then we can avoid making several entries. Timestamps aren't stored with a string, only with an ID. The only way to associate extra information with them is to make a separate ID for every possible state. > There are timestamps before and after certain coreboot APIs that may call into other EC host commands. Therefore, when EC started populating more timestamps, it would only be in between those two outer layer timestamps. Yes, but not all coreboot timestamps have a start or an end. If you normally have e.g. "cbmem post", "write tables" and "finalize chips" following one after the other, but turning on this debugging Kconfig adds a ton of EC host command timestamps in between, things will get confusing. I still think that the timestamp system is fundamentally not the right tool to add detailed debugging information that can be turned on or off with a Kconfig. The timestamp system is meant to extract a consistent high-level overview of the boot time situation from production systems for metrics analysis (e.g. what we do with feedback reports). For detailed debugging on individual units, the console log is a much better tool. -- To view, visit https://review.coreboot.org/c/coreboot/+/85326?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I8ab89830ede940d2237ad21187b137dca9689fb0 Gerrit-Change-Number: 85326 Gerrit-PatchSet: 11 Gerrit-Owner: Jayvik Desai <jayvik(a)google.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jayvik Desai <jayvik(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Wed, 18 Dec 2024 17:43:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jayvik Desai <jayvik(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Kapil Porwal <kapilporwal(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2024