Attention is currently required from: Arthur Heymans, Felix Held, Jérémy Compostella, Karthik Ramasubramanian, Paul Menzel, Simon Glass, Simon Glass.
Julius Werner has posted comments on this change by Simon Glass. ( https://review.coreboot.org/c/coreboot/+/77712?usp=email )
Change subject: Introduce a coreboot Control Block (CCB) ......................................................................
Patch Set 12: Code-Review-2
(4 comments)
Patchset:
PS10:
OK I have updated this to support FMAP. […]
My concern is about *not* adding the option that causes so many issues and requires this to be so complicated (embedding in bootblock and then having to pass it on in memory). I'm not just asking for additional options. For now you have only added additional stuff to this series, that's not really addressing my concerns. I think you should *remove* the bootblock-embedded option and settle on one or more of the others (whether that's a CBFS file or an extra FMAP section I don't care too much, although CBFS file seems cleaner to me).
Arthur's suggestion of dumping early lines from the CBMEM console to the UART once it has reached the point where it could decide to enable UART also sounds fine to me. Again, I really don't think the "hang in super early messages" argument makes sense here because that super early code doesn't really suddenly start failing on stable systems, you only get hangs there when bringing up a new platform and in those cases people have their UARTs compile-time enabled anyway.
PS10:
The goal of CCB (for console) is to allow all output to be produced, or none. […]
You keep saying that but you're not really explaining why that is true. Why are those first few bootblock messages so important for this (we're just fighting over a few lines compared to all the rest of coreboot output here, that's not really "half-baked" it's more like "98% baked")? Which user in what practical use case who doesn't already have the console compile-time enabled cares about those few lines? coreboot just doesn't really hang that early on stable platforms in practice.
I'm not really sure who you are still doing this for anyway, to be honest. We are not going to use this at ChromeOS. I haven't seen anyone else here say that they'd be going to use this yet (and if there are people interested, I'd really like to hear from them whether they think super early hangs in the bootblock are going to be important to them, and why). I just don't want to make bad architectural decisions and add all this unnecessarily complex stuff to address a concern that no actual user is really going to care about afterwards.
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/77712/comment/9def3752_13ce9dc7?usp... : PS10, Line 45: CCB(., 0x10)
Do timestamps suffer from the same issue? What is the solution to resolve this?
Yes but timestamps have been there for decades and are widely used by the majority of coreboot users. I am not saying that features are never allowed to need an extra memlayout region, I'm just saying that it is a notable maintenance burden that should be justified by how useful the feature is and we should try to avoid it if other alternatives are possible. In this case there's an alternative that doesn't require this (storing console loglevel in a CBFS file) and I still don't feel like you have justified why we need this way more complicated and burdensome option when we could just do that instead (because nobody who doesn't have serial output enabled at compile time already because they're doing early bring-up tends to care about those first few bootblock messages before CBFS becomes accessible).
File src/lib/hardwaremain.c:
https://review.coreboot.org/c/coreboot/+/77712/comment/874f349f_050bf820?usp... : PS10, Line 449: cbmem_initialize();
But in that case you wouldn't set the console to silent, would you?
No, but the console still won't start printing until `console_init()` is called, even if it is enabled at compile time. That's the problem here, you can't move `console_init()` further down because you would be breaking important debugging use cases for people who are actually enabling the console via Kconfig.
-
Julius Werner (Code Review)