[M] Change in coreboot[main]: drivers/option: Add CBFS file based option backend

[Julius Werner (Code Review)]

Attention is currently required from: Jérémy Compostella, Nicholas Chin, Subrata Banik.

Julius Werner has posted comments on this change by Nicholas Chin. ( https://review.coreboot.org/c/coreboot/+/85905?usp=email )

Change subject: drivers/option: Add CBFS file based option backend
......................................................................

Patch Set 9:

(6 comments)

Patchset:

PS4:
> Done

Wait, why would we want this? I don't see why a mainboard should select an option backend by default. Mainboard code is really just supposed to describe the hardware, not configure policy.

If you were planning to select this from ChromeOS mainboards, we could just do `default USE_CBFS_FILE_OPTION_BACKEND if CONFIG_CHROMEOS` instead?

File src/Kconfig:

https://review.coreboot.org/c/coreboot/+/85905/comment/80230c2a_cddd0efc?usp... :
PS9, Line 172: option-name
should say `-n option/<option-name>`

File src/drivers/option/Makefile.mk:

https://review.coreboot.org/c/coreboot/+/85905/comment/742bde6e_49bac45e?usp... :
PS6, Line 6: smm-$(CONFIG_USE_CBFS_FILE_OPTION_BACKEND) += cbfs_file_option.c
> > Seems like the only user of the option api in SMM is the `power_on_after_fail` option, which defau […]

Yeah, CBFS access from SMM is a terrible idea anyway and should generally be discouraged. I think some odd platforms may be using it, but it's not compatible with a number of options (e.g. CBFS_VERIFICATION, in this case), and is generally something we really don't want to make larger.

I would say just disallow this in SMM mode and always return the fallback value there.

File src/drivers/option/cbfs_file_option.c:

https://review.coreboot.org/c/coreboot/+/85905/comment/9b96d0df_886d9776?usp... :
PS9, Line 7: unsigned int do_get_uint_option(const char *name, const unsigned int fallback)
> Why do we store a uint64_t to retrieve to get an unsigned int (uint32_t)?

`cbfstool add-int` creates `uint64_t`s by default, I think it's easier to just use that everywhere than to create separate commands to generate separate kinds of integers (the extra 4 bytes aren't gonna blow the budget).

File src/include/option.h:

https://review.coreboot.org/c/coreboot/+/85905/comment/a07675a7_e2ffab45?usp... :
PS9, Line 16: 	return fallback;
> Shouldn't an error be printed ? A call to get_uint_option() in SMM should be considered an issue as  […]

I don't think the option API itself is not available in SMM (e.g. CMOS options should still work?), just CBFS. So this shouldn't be an error, and it probably also shouldn't print because we need options access for printing, among other things.

https://review.coreboot.org/c/coreboot/+/85905/comment/89a88b9b_13f614be?usp... :
PS9, Line 27: #define set_uint_option(name, value) do_set_uint_option("option/" name, value)
I guess this works but isn't it a bit overkill just to prepend the string? You could've just done something like
```
char full_name[CBFS_METADATA_MAX_SIZE];
snprintf(full_name, sizeof(full_name), "option/%s", name);
```