Skip to content

Cfr ng #638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: dasharo
Choose a base branch
from
Open

Cfr ng #638

wants to merge 11 commits into from

Conversation

miczyg1
Copy link
Contributor

@miczyg1 miczyg1 commented Mar 28, 2025

No description provided.

LeanSheng and others added 11 commits March 26, 2025 20:00
Fix some typos and also update the naming convention of
`CFR_OPTFLAG_GRAYOUT` to `CFR_OPTFLAG_INACTIVE` as per reviews.

Signed-off-by: Lean Sheng Tan <sheng.tan@9elements.com>
Change-Id: Id66808382b93e32c58024462c18b20c2a89d6d23
Reviewed-on: https://review.coreboot.org/c/coreboot/+/85780
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Christian Walter <christian.walter@9elements.com>
Add a version field to the CFR root struct so parsers can check
compatibility when parsing structs.

Change-Id: Ifcb950f1bdedc0ab925f3841befb7e7001c0f7f4
Signed-off-by: Filip Brozovic <fbrozovic@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/86080
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Matt DeVillier <matt.devillier@gmail.com>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Implements a way for CFR options to depend on another option
being set to one or more specific values. This is achieved
by writing a list of values as a varbinary struct.

Change-Id: Iaf7965551490969052eb27c207fa524470d4dd6a
Signed-off-by: Filip Brozovic <fbrozovic@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/85987
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Matt DeVillier <matt.devillier@gmail.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
This commit adds support for minimum/maximum limit values as well as
step sizes for CFR number options. Additionally, add a new flag that
specifies the option should be displayed in hexadecimal notation instead
of decimal.

Change-Id: I2e70f1430fb1911f1ad974832f8abfe76f928ac3
Signed-off-by: Filip Brozovic <fbrozovic@gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/86039
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Change-Id: I749ecf2eb041dfbe2f6f93b5e3a7b2af3c78bb1d
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: I123561aa82ac87e105c1a9592e05e1787963e72d
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: I3a12f8bcf7b5fd52c0153b29e88d5d3fc55312e3
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: Ib3d1df648e50dc85f432f8c9f14fb685edf6facd
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: Ide2a3a4b59be5b27bf7315690520c9392a98d044
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: I486d9faae85f79edb2d025578951ef55817198be
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
Change-Id: I957a0cbd0dbea20f4408775dde5115cbfa7892e7
Signed-off-by: Michał Kopeć <michal.kopec@3mdeb.com>
@mkopec
Copy link
Member

mkopec commented Apr 2, 2025

I've been using a thinkcentre tiny as my test mule. The current status is:

  • All applicable options for the platform are added in cfr
  • CFR option backend is hooked into dasharo vendorcode
  • Dasharo menu is hidden, CFR menu is on the edk2 frontpage as Advanced Options
  • The edk2 changes are a bit hacky: Cfr ng edk2#232 but I managed to hook cfr variables into our boot policies
  • I've sent cfr changes that are board specific for upstream review: https://review.coreboot.org/c/coreboot/+/87048

That's basically it. The CFR form is displayed as a single scrollable page, which is nice on a big monitor, but will wreak havoc on our tests over serial console. I didn't bother to call the options identically to our edk2 versions but it should be possible.

Overall it seems to work well, I haven't seen any major issues. Reset to default works fine even if you call it before entering the menu, which was a pain point with some previous dynamic form implementations.

@pietrushnic
Copy link
Contributor

@mkopec @miczyg1, can we describe the primary benefits to users, OEMs, and developers by introducing this change?

@mkopec
Copy link
Member

mkopec commented Apr 3, 2025

The benefit is that it will be easier to add options, especially board-specific options. Right now we have all options in the same place edk2 with PCDs to control their visibility, which adds a lot of code for each board specific option. CFR allows the mainboard to specify options for the payload. So options like these will be easier to add:

  • dGPU disable
  • SSD slot enable/disable
  • [some other board specific device] enable/disable

plus hopefully it makes us closer to upstream

@pietrushnic
Copy link
Contributor

@mkopec ok, make sense. I wonder how much time we spent on those operations in the past and how much we will have to spent now. This would prove if it improves situation. For now we just have hipothesis.

Easier control of board specific options, as well as those who customer want/may have according to their plan, is important feature. Hopefully we can leverage that more efficiently when it would be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants