Hi
We have runtime configuration arguments implemented for our platform in out fork. This is very simple, we just add few lines in SeaBIOS configuration file on CBFS (https://bit.ly/2qmX9nT — config example, https://bit.ly/2v5Era6 — handling code). There are only boolean values, no syntax, config file of constant size. Perhaps too simple, but it serves its purpose. We use it to enable/disable additional UART, EHCI and other features. Configuration is done in a payload, but it is supposed to affect hardware initialization in coreboot.
We would like to have this functionality in mainline coreboot. We expect that this proof-of-concept is not good enough to incorporate it, but the question is how much better we have to do that. We have read some discussion on this topic on mailing list and we have found many ideas how this should be done, however we haven't found any definite answer.
IMHO, a simple solution is the best one as long as it works well. It's also certainly better than nothing, especially that question appears again and again. A single fixed-size CBFS file seems to be enough for most applications. In fact, whole coreboot configuration is just 686 lines, in my configuration, 602 of them are boolean or disabled, so they could be packed in ceil(602/8)=76 bytes. I think 8 bytes for each other is quite safe to assume (most of them needs less, some needs more). 76+8*82=748 b. So even if we'd like to pack the whole configuration into a binary file we'd need 1kB or less. No moving, no resizing, just fixed schema binary file, it could be easily done with CBFS.
To make it easy we'd prepare build system so that we could mark some options runtime-configurable in menuconfig. Based on those choices runtime-config structure would be generated as C header so that it can be easily used in source files.
On the other hand, we have seen quite advanced considerations in the discussion so we've decided to ask for your opinion before we prepare final implementation.
Hello Bartek,
On 11.04.2018 22:48, bartek.pastudzki 3mdeb.com wrote:
We have runtime configuration arguments implemented for our platform in out fork. This is very simple, we just add few lines in SeaBIOS configuration file on CBFS (https://bit.ly/2qmX9nT — config example, https://bit.ly/2v5Era6 — handling code). There are only boolean values, no syntax, config file of constant size. Perhaps too simple, but it serves its purpose. We use it to enable/disable additional UART, EHCI and other features. Configuration is done in a payload, but it is supposed to affect hardware initialization in coreboot.
thanks for sharing your implementation. I guess it's really too simple; at some point you might want more than just booleans. And you seem to assume that CBFS is memory mapped which might not always be the case. Anyway, as you said "it serves its purpose" ;)
We would like to have this functionality in mainline coreboot. We expect that this proof-of-concept is not good enough to incorporate it, but the question is how much better we have to do that. We have read some discussion on this topic on mailing list and we have found many ideas how this should be done, however we haven't found any definite answer.
I think our primary focus should be to define a common interface, first, for different implementations. There's already coreboot's "option table" that is stored in RTC/CMOS RAM. Marcello has started some work to use VPD as a backend [1]. There's your implementation; and finally, Felix and me discussed a storage format specifically for dependable updates on NOR flashes [2].
All of these approaches have their pros and cons. We could also try to discuss which one is the best and focus on that, though I'd rather try to stay flexible instead.
IMHO, a simple solution is the best one as long as it works well. It's also certainly better than nothing, especially that question appears again and again. A single fixed-size CBFS file seems to be enough for most applications. In fact, whole coreboot configuration is just 686 lines, in my configuration, 602 of them are boolean or disabled, so they could be packed in ceil(602/8)=76 bytes. I think 8 bytes for each other is quite safe to assume (most of them needs less, some needs more). 76+8*82=748 b. So even if we'd like to pack the whole configuration into a binary file we'd need 1kB or less. No moving, no resizing, just fixed schema binary file, it could be easily done with CBFS.
Personally, I don't care about the space needed. I would wait with the implementation until somebody really needs it packed tight.
Regarding CBFS, FWIW, taking the CBFS as something immutable is the idea moving forward. Regions that are mutable (during firmware run-time) should have their own FMAP entry.
To make it easy we'd prepare build system so that we could mark some options runtime-configurable in menuconfig. Based on those choices runtime-config structure would be generated as C header so that it can be easily used in source files.
I've had something alike in mind for some time now: Having a list of options that can either be set at compile-time or run-time, defaults should be set in the devicetree. From that we could generate Kconfig entries and finally with a .config the values and / or code to read them during run-time.
Ideally we'd always have defined default values. So the code using the option interface won't be cluttered with error handling. Or we could keep the get_option() style, where the calling code defines the default value.
Nico
[1] https://review.coreboot.org/25550/ [2] https://mail.coreboot.org/pipermail/coreboot/2017-September/085215.html https://mail.coreboot.org/pipermail/coreboot/2017-October/085232.html
And you seem to assume that CBFS is memory mapped which might not always be the case.
This is a very important part to keep in mind when we are talking UART. There has been an attempt inside the Chrome OS team before to make the UART runtime-configurable, and it fell flat on its face because of this. On a non-x86 system, the purpose of the whole bootblock is essentially just to get the SPI flash up and running far enough that you can read it. So if you need to read SPI flash before you can initialize the UART, the whole bootblock and all the places where stuff might go wrong that you'd need a UART to debug is essentially already over. If you want to allow any runtime configuration for the UART, you'll probably have to contend that it only works in verstage/romstage or later.
Also, please keep in mind that some users care more than others about the boot speed impact from reading runtime configuration, so always leave the option open to keep it all hardcoded and fast.
Hello again,
on Fri, 13 Apr 2018 18:35:58, Nico Huber nico.h@gmx.de wrote:
I think our primary focus should be to define a common interface, first, for different implementations. There's already coreboot's "option table" that is stored in RTC/CMOS RAM. Marcello has started some work to use VPD as a backend [1]. There's your implementation; and finally, Felix and me discussed a storage format specifically for dependable updates on NOR flashes [2].
Thanks for the information, those patches are useful. I agree we should define a more flexible interface. There is even FIXME about it in <option.h> header. :) I've seen a solution from VPD based implementation ([1]) and adapted ours to implement similar interface. Generally, I think it doesn't need to change much, however:
1. get_option() function gets destination pointer (void*), but no maximum size, I think we should add size argument to avoid buffer overflow. I assume that we want to allow reading more than one byte. We could also consider adding functions that would also expect specific type of value (boolean, integer) and return suitable type.
2. In [1], I see there is get_option() made as a wrapper to get_option_cmos() function. Personaly I'd do it so:
in option.h:
#if CONFIG_USE_OPTION_TABLE
enum cb_err get_option(void *dest, size_t max_size, const char *key);
#else
inline enum cb_err get_option(void *dest, size_t max_size, const char *key) { return CB_OTABLE_DISABLED; }
#endif
I've changed get_option() prototype. I removed CMOS from the return value, because I think we should keep them implementation independent. With such a header we could link module of choice with proper implementation depending on constant like CONFIG_OPTION_TABLE_BACKEND. It would simplify headers and let us avoid many substitutions. It would also simplify testing if the feature is on. Testing many options at the same time would be inconvenient.
On the other hand, having backend functions with different names could make code searching a little bit easier. And perhaps I miss something important too?
Personally, I don't care about the space needed. I would wait with the implementation until somebody really needs it packed tight.
I didn't make myself clear. I wrote that because, somewhere in the discussion, fragmentation issue was raised. Keeping small file with static schema would help avoid that problem.
on Fri, 13 Apr 2018 21:55:15, jwerner@google.com wrote:
This is a very important part to keep in mind when we are talking UART. There has been an attempt inside the Chrome OS team before to make the UART runtime-configurable, and it fell flat on its face because of this.
Perhaps we should provide in Kconfig that CBFS implementation is possible.
Best regards Bartek Pastudzki 3mdeb
-
bartek.pastudzki 3mdeb.com -
Julius Werner -
Nico Huber