The fallacy of the CONFIG_* plague

[mrnuke]

=== START_TLDR /* Skip to END_TLDR if you're short on time */ ===

It seems that we're using "#if CONFIG_" like free coffee and cookies in a 
conference. Without thinking too much of it. And we're using very much of this 
in generic code, which is, as I will point out why shortly, a very bad idea.

It wouldn't matter as much if we only used CONFIGs to control the build 
process, or some mundane board-specific options. One problem is, that we've 
started using them excessively in generic code. Generic code should be just 
that, generic. If we need it to behave one way or another, we should do so by 
the vales we pass it not by board-specific Kconfigs.

Take lib/ for example. We should be able to compile, the entire directory into 
a set of static libraries:
* libcorebootlib_rom.i386.a
* libcorebootlib_ram.i386.a
* libcorebootlib_bootblock.armv7.a
* libcorebootlib_rom.armv7.a
* libcorebootlib_ram.armv7.a
And use those libraries as-are, in every single board and component, without 
their behavior depending on Kconfigs. I am allowing for stage-specific 
versioning here due to the joys early init may throw at us.

The same reasoning applies to chip-generic code -- basically most things _not_ 
under src/mainboard/.

Case in point: http://review.coreboot.org/#/c/5199/
The intent is to use CONFIG_, not for the purpose of solving a practical 
problem, but just for the sake of fixing a build failure. It feels like a "if 
it compiles, let's ship it" mindset.

Who cares, really? There is a very practical aspect to lib-izing generic code, 
and that is the abuild step. We can speed that up tremendously by being smart. 
ccache helps a bit, but only so much.
Another practical aspect is eliminating confusion relating to "what does the 
code do now, whith _this_ option instead of _that_ option?".

=== END_TLDR ===

So, instead of having a generic function doing
 void function(void)
 {
 #if IS_ENABLED(CONFIG_HAVE_LOLLIPOP)
	printk(BIOS_YUMMY, "Sucking on a lollipop\n");
 #else
	printk(BIOS_SUCKY, "Meanie!!\n");
 #endif
 }

We make it dynamically polymorphic:
 void function(bool has_lollipop)
 {
 	if (has_lollipop)
		printk(BIOS_YUMMY, "Sucking on a lollipop\n");
 	else
		printk(BIOS_INFO, "Mister stole my lollipop\n");
 }

Let the mainboard pass the lollipop option instead of Kconfig-izing it. The 
only place where code flow/values should depend on anything CONFIG_* should be 
in (tentative list warning):
* board-specific code
* enabling debug options not used in production code (irrelevant for abuild)
* linker scripts
* very early init (assembly part of the bootblock)

And of course, we use the linker to garbage-collect unused functions for the 
sake of space.

Q.E.D.

Thoughts, comments, concerns, paint ideas?

Alex

[Peter Stuge]

mrnuke wrote:
> So, instead of having a generic function doing
>  #if IS_ENABLED(CONFIG_HAVE_LOLLIPOP)
>   printk(BIOS_YUMMY, "Sucking on a lollipop\n");
>  #else
>   printk(BIOS_SUCKY, "Meanie!!\n");
>  #endif
>
> We make it dynamically polymorphic:
>   if (has_lollipop)
>       printk(BIOS_YUMMY, "Sucking on a lollipop\n");
>   else
>       printk(BIOS_INFO, "Mister stole my lollipop\n");
> ..
> Thoughts, comments, concerns, paint ideas?

There is significant benefit in not having any run-time data at all,
because then there can not be run-time issues caused by incorrect or
simply unexpected data.

There is also significant benefit in having generic code.

But there is generic and there is generic. If something is known
already at build-time then I think it's a bad idea to push the
decision to run-time.


//Peter

[mrnuke]

On Wednesday, February 12, 2014 07:40:58 PM Peter Stuge wrote:
> But there is generic and there is generic. If something is known
> already at build-time then I think it's a bad idea to push the
> decision to run-time.

And yet, we have many places where we take decisions which belong at runtime, 
and make them at compile-time. One such example is limiting the SATA speed on 
butterfly.

Alex

[Peter Stuge]

mrnuke wrote:
> > If something is known already at build-time then I think it's a
> > bad idea to push the decision to run-time.
>
> And yet, we have many places where we take decisions which belong
> at runtime, and make them at compile-time. One such example is
> limiting the SATA speed on butterfly.

Don't confuse the product that ships with coreboot for what it
becomes after you've modified it.

Do you expect Intel to give you support after you've overclocked
their CPU way out of the spec of the shipped product?


//Peter

[mrnuke]

On Thursday, February 13, 2014 02:21:14 AM Peter Stuge wrote:
> Don't confuse the product that ships with coreboot for what it
> becomes after you've modified it.

Are we bikeshedding the example here?
> Do you expect Intel to give you support after you've overclocked
> their CPU way out of the spec of the shipped product?

And this is relevant how?

Alex

[Patrick Georgi]

Am Mittwoch, den 12.02.2014, 12:30 -0600 schrieb mrnuke:
> Thoughts,
> No.
> comments, 
> Really?
> concerns,
> I like solving things at compile time that can be solved at compile time
> (and in fact even earlier, if possible). No need to store "Mister stole
> my lollipop" if the board is guaranteed to ship with one.

A simple solution to your problem would be to store everything as AST in
object files and letting the linker resolv everything - that would fit
your "static library" requirement while still having Kconfig options
work. So it's rather arbitrary. (and the fun thing is, our toolchain is
even capable of that given the right switches, so it's not purely
theoretical)

Having a coreboot that determines at runtime again and again and again
what kind of device it's running on is quite a departure from the design
so far.

Also, to dissect your example, what's the difference between:
 void function(bool has_lollipop)
 {
        if (has_lollipop)
                printk(BIOS_YUMMY, "Sucking on a lollipop\n");
        else
                printk(BIOS_INFO, "Mister stole my lollipop\n");
 }

and

 void function(bool has_lollipop)
 {
        if (CONFIG_EXISTS(CONFIG_HAS_LOLLIPOP))
                printk(BIOS_YUMMY, "Sucking on a lollipop\n");
        else
                printk(BIOS_INFO, "Mister stole my lollipop\n");
 }

from a stilistic point of view? (except for the upper case - which we
could fix, if we want...)
It's a huge difference in what ends up in the binary.
> paint ideas?
> Turquoise, please.

Patrick

[Aaron Durbin]

On Wed, Feb 12, 2014 at 12:44 PM, Patrick Georgi <[e-mail]> wrote:
> Am Mittwoch, den 12.02.2014, 12:30 -0600 schrieb mrnuke:
> > Thoughts,
> > No.
> > comments,
> > Really?
> > concerns,
> > I like solving things at compile time that can be solved at compile time
> > (and in fact even earlier, if possible). No need to store "Mister stole
> > my lollipop" if the board is guaranteed to ship with one.
>
> A simple solution to your problem would be to store everything as AST in
> object files and letting the linker resolv everything - that would fit
> your "static library" requirement while still having Kconfig options
> work. So it's rather arbitrary. (and the fun thing is, our toolchain is
> even capable of that given the right switches, so it's not purely
> theoretical)
>
> Having a coreboot that determines at runtime again and again and again
> what kind of device it's running on is quite a departure from the design
> so far.
>
> Also, to dissect your example, what's the difference between:
>  void function(bool has_lollipop)
>  {
>         if (has_lollipop)
>                 printk(BIOS_YUMMY, "Sucking on a lollipop\n");
>         else
>                 printk(BIOS_INFO, "Mister stole my lollipop\n");
>  }
>
> and
>
> void function(bool has_lollipop)
>  {
>         if (CONFIG_EXISTS(CONFIG_HAS_LOLLIPOP))
>                 printk(BIOS_YUMMY, "Sucking on a lollipop\n");
>         else
>                 printk(BIOS_INFO, "Mister stole my lollipop\n");
>  }
>
> from a stilistic point of view? (except for the upper case - which we
> could fix, if we want...)
> It's a huge difference in what ends up in the binary.

To add to this. I think archive files as a static target is
oversimplifying things. We have to be very careful about weak
functions and how the linker resolves said functions. Moreover, the
compiler's ability is being underestimated. Unless one is going to go
out of their way to have global variable in another compilation unit
be the value of CONFIG_FOO the compiler will cull all the dead code it
knows it won't hit.

-Aaron

[mrnuke]

On Wednesday, February 12, 2014 07:44:42 PM Patrick Georgi wrote:
> I like solving things at compile time that can be solved at compile time
> (and in fact even earlier, if possible). No need to store "Mister stole
> my lollipop" if the board is guaranteed to ship with one.

That's something we can fix by smart design and linker garbage-collecting. I 
made my example as simple as possible for the sake of argument. The "smart" 
way to approach the problem will of course depend on the specific problem.

I can also turn that around and say why store all debug strings if a 
production system will never run above BIOS_SPEW ? Quite frankly, I like the 
way we reduced this problem for the console.

Coming back to the lollipop example, it makes sense to store "Mister stole my 
lollipop" even if the board is guaranteed to come with one. If the lollipop 
malfunctions for whatever reason, then the board can indicate so in 
"has_lollipop". It's much more elegant that a silent crash or silent 
"somethin' ain't workin'".

I've been doing exactly this for the PMU on the cubieboard. If for some 
obscure reason, it doesn't identify itself properly, we handle this case 
elegantly, and still boot, albeit at a reduced cock speed.
> Turquoise, please.

Dark shade or a lighter shade?

Alex

[mrnuke]

On Wednesday, February 12, 2014 01:00:59 PM you wrote:
> I can also turn that around and say why store all debug strings if a
> production system will never run above BIOS_SPEW ?
> I meant BIOS_INFO here.
> elegantly, and still boot, albeit at a reduced cock speed.

Please read that as "clock".

Alex