Attention is currently required from: Angel Pons, Benjamin Doron, Lean Sheng Tan, Maximilian Brune, Nico Huber, Patrick Rudolph, Sean Rhodes.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74121?usp=email )
Change subject: [RFC] drivers/option: Add forms in cbtables ......................................................................
Patch Set 21:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74121/comment/dba64c92_07f45274 : PS18, Line 15: The system currently lacks a way : to describe where to find option values.
Technically, backends that store the name of an option as a string would […]
Because storage areas could differ completely from one mainboard to another, so I think it would be best to allow for as much flexibility as we can, and for the storage locations to be specified completely separately from the option declarations.
For most platforms, I'm thinking that we want a platform to say "Here's my default storage method, figure out where to put the options."
``` # Put everything in CMOS default_storage cmos flag cmos byte_aligned # Put any option a byte or larger on a byte boundary ```
For the hopefully small subset of platforms that would need to be more explicit, we'd probably want to be able to define a default, then override that default for individual settings.
I'd prefer that each setup option already contained the information about how many bits or bytes it needed, so that the *only* piece we'd be setting with this is the location. If the size needed doesn't fit, it would throw an error. We'd generate a list of all of the setting locations after they're assigned, both for use in storing & retrieving the data and for debug. Those values would get added to rest of the options data.
Maybe something like this:
``` # Most values get put into SMM_Store default_storage smmstore option option0 smmstore 0.0 # This needs to go in the smmstore before anything else option option1 smmstore -1.0 # Put this at the very end of the available storage #options2 - option99 get automatically assigned.
# Put some stuff in cmos flag cmos packed # Specify that options are not byte aligned option binary_option1 cmos 5.0 # Byte 5, bit 0 (This is an index into the CMOS area, not a CMOS address) option binary_option3 cmos # Takes the next used bit option dword_option0 cmos # option cmos_checksum cmos # 2 bytes, but that doesn't need to be specified here.
# We also have some read-only options from Google's CBI option wwan_present cbi #location is defined in coreboot devicetree option fp_present cbi ```
What I'm thinking is that all this file would specify about is the medium and the offset for the setting, plus flags that affect the layout. This shouldn't know or care about the particulars of any storage medium. I'd rather we defined the specifics of each storage medium somewhere else. So for CMOS, we'd have a map of the CMOS layout in a different file. Smmstore is defined in FMAP, CBI is defined in devicetree. NewMedium is defined wherever.
Nico's thought about looking for options in the various storage mediums is interesting, but at least initially, I don't think I'm a fan. It seems like we'd really just want options to be stored in a single medium. A single source of truth and all that. There might be a reason to do it that I'm just not currently seeing though.
-
Martin L Roth (Code Review)