Attention is currently required from: Julius Werner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56210 )
Change subject: console: Handle verstage as "first non-bootblock stage" ......................................................................
Patch Set 1:
(1 comment)
File src/console/init.c:
https://review.coreboot.org/c/coreboot/+/56210/comment/ff21e811_82d83f95 PS1, Line 15: #endif
For one, some stages are _pretty_ packed, e.g. verstage on google/veyron. Especially with the follow-on that takes a couple more bytes to add CBFS support, verstage is overflowing there, so cutting down on that is worthwhile.
Well this patch would only decrease verstage size if the bootblock console is disabled and get_uint_option() isn't used otherwise. That's rather subtle.
Okay, I think we're all confused about what we're talking about now. ^^ What I mean is using ENV_INITIAL_STAGE instead of the the FIRST_CONSOLE macro that gets defined here. So the code in init_log_level() should just read:
console_loglevel = get_console_loglevel();
if (!ENV_INITIAL_STAGE) console_loglevel = get_uint_option("debug_level", console_loglevel);
(and then the stuff Patrick adds in the next patch would also fall under that). That way we don't need an extra FIRST_CONSOLE macro and all the vboot stage ordering stuff has already been taken care of correctly in the definition of ENV_INITIAL_STAGE.
That's what I already understood so far.
If the question is why we need any check at all and can't just call get_uint_option() in every stage... I don't know, I'm not super familiar with how CMOS stuff works on x86. Are there any platforms where accessing CMOS requires any sort of platform-specific setup beforehand? For CBFS that is definitely the case, so at least for CBFS we need to prevent accesses in the first stage. For CMOS, if you're sure that no point to call it would be too early, I guess it doesn't matter.
There are no known special requirements. And if there were, they should be handled behind the API and not here, right? It's also not the CBFS-API users that check ENV_INITIAL_STAGE.
The whole point of FIRST_CONSOLE really was just to keep complexity low for the very first effective print (no matter in what stage the console starts). So that can't be achieved with ENV_INITIAL_STAGE. I think we should either go with Patrick's patch or just always call get_uint_option().