nvramtool and CONFIG_HAVE_OPTION_TABLE (or CONFIG_USE_OPTION_TABLE)

A coworker noticed that nvramtool (and the nvramtool man page) both refer to the option CONFIG_HAVE_OPTION_TABLE as the option set to include cmos.layout into the coreboot tables.

nofound_msg_cmos_opt_table[] =
    "%s: Item %s not found in coreboot table.  Apparently, the "
    "coreboot installed on this system was built without specifying "
    "CONFIG_HAVE_OPTION_TABLE.\n";

However, CONFIG_HAVE_OPTION_TABLE is set by the board to indicate whether the board is cmos.layout capable. The actual option is CONFIG_USE_OPTION_TABLE, which depends upon CONFIG_HAVE_OPTION_TABLE. The code in src/lib/coreboot_table.c only adds the lb record to the coreboot tables if CONFIG_USE_OPTION_TABLE is set.

The other choice is to make src/lib/coreboot_table.c add the lb record if the board has a cmos.layout (CONFIG_HAVE_OPTION_TABLE) but then userland will have access to a layout that is ignored. I suppose this is what happens if you set CONFIG_STATIC_OPTION_TABLE. This doesn't seem like the right answer to me.

I think this is only cosmetic, but should I submit a patch to change this so that others are not also sidetracked?

Thanks,
Kevin

[Michal Zygowski]

Hello Kevin,

Thank You for interesting in the coreboot project. Feel free to submit
the patch via gerrit.

Please include the reasoning You have written here in a commit message
and push the patch. You will get Your review from coreboot developers
there and they will decide whether the patch is eligible to be
submitted. Even if the change does not bring anything significant, it
can be still abandoned, so You shouldn't hesitate or be affraid of
pushing patches.

Best regards,
Michał

On 31.05.2019 02:57, [e-mail] wrote:
> A coworker noticed that nvramtool (and the nvramtool man page) both refer to the option CONFIG_HAVE_OPTION_TABLE as the option set to include cmos.layout into the coreboot tables.
>
> nofound_msg_cmos_opt_table[] =
>     "%s: Item %s not found in coreboot table.  Apparently, the "
>     "coreboot installed on this system was built without specifying "
>     "CONFIG_HAVE_OPTION_TABLE.\n";
>
> However, CONFIG_HAVE_OPTION_TABLE is set by the board to indicate whether the board is cmos.layout capable. The actual option is CONFIG_USE_OPTION_TABLE, which depends upon CONFIG_HAVE_OPTION_TABLE. The code in src/lib/coreboot_table.c only adds the lb record to the coreboot tables if CONFIG_USE_OPTION_TABLE is set.
>
> The other choice is to make src/lib/coreboot_table.c add the lb record if the board has a cmos.layout (CONFIG_HAVE_OPTION_TABLE) but then userland will have access to a layout that is ignored. I suppose this is what happens if you set CONFIG_STATIC_OPTION_TABLE. This doesn't seem like the right answer to me.
>
> I think this is only cosmetic, but should I submit a patch to change this so that others are not also sidetracked?
>
> Thanks,
> Kevin
> _______________________________________________
> coreboot mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]