Re: RFC: Post-build control of serial console

I continue to believe that this is not a good idea and would add a lot of
complication (and another competing system for something we already have
many solutions for) without sufficient benefit to justify it. Manipulating
variables in a bootblock image would add yet more complexity to the already
stretched cbfstool, require more magic numbers (a CBFS attribute is not a
solution because not all bootblocks are CBFS files), may cause issues with
verification or compression, etc.

If we want easier control of serial loglevel, the obvious solution is to
integrate into any of the existing configuration mechanisms that have
already been mentioned rather than invent a new one. I mean we already have
that implemented today through the option backend (CMOS), after all. We
also already had a proposal to implement this as a CBFS file two years ago (
https://review.coreboot.org/54385) which we seemed to have general
consensus on and started implementing, they were just never finished
because Patrick moved on to work on other things (
https://review.coreboot.org/56210, https://review.coreboot.org/45208,
https://review.coreboot.org/55397). We could also tie into the FW_CONFIG
system that was added since then, if we do that we basically get both CBFS
file support and ChromeOS CBI support automatically while tying into an
existing, widely-used system. Really, anything would be so much better than
inventing something completely new and yet again different, which doesn't
come with a convenient solution to have the information available in later
stages and requires cbfstool to make yet more complicated bootblock surgery.

The only real reason you have to reject all of those other options is so
that this can capture the serial output from the very first few bootblock
lines, but where is the pressing use case for that which justifies ignoring
all these other concerns? Does anyone really ever have a situation where
they're running an image they didn't build themselves (so they can't
quickly change the Kconfig and rebuild), but suddenly something in the very
early bootblock starts to break and they need to be able to debug it
without recompiling? The bootblock has the least interaction with any
outside components that could induce a sudden change in behavior or a rare
flake, and thus is the most stable piece of coreboot. You usually debug
bootblock issues early on when working with a new SoC, at a point where you
have the console enabled through Kconfig anyway. And then once you get it
working it tends to stay working. We're just talking about a tiny piece of
coreboot with hardly any dependencies here which only tends to break on the
developer's desk right after they wrote some new code, not randomly on
someone else's machine (like e.g. memory training or PCI init or all the
more complicated pieces that could be served perfectly fine by a CBFS file
based solution). In fact, on x86 devices CBFS is basically available right
away so this only really makes a difference on non-x86 devices to begin
with, and on those we're taking about half of a small stage.

This is a very complex feature that will become a notable ongoing
maintenance burden and further fracture the already complicated runtime
configuration landscape for coreboot, for an extremely marginal benefit
that I honestly think nobody needs.


On Mon, Oct 2, 2023 at 11:17 AM Simon Glass <[email protected]> wrote:
>
> Hi Martin,
>
> On Fri, 29 Sept 2023 at 14:22, Martin Roth <[email protected]> wrote:
> >
> >
> > Hi Simon,
> > Sep 29, 2023, 13:04 by [email protected]:
> >
> > > Hi Martin,
> > >
> > > On Mon, 25 Sept 2023 at 13:51, Martin Roth <[email protected]>
wrote:
> > >
> > >>
> > >> Hey Simon, I think this is a good summary of the current situation,
though the table doesn't come through in the text version of the email at
all. Maybe upload an RFC to the documentation directory so it can be
commented on in gerrit?
> > >>
> > >
> > > Can you see this?
> > >
> > >
https://docs.google.com/document/d/17vLMUFYuUVZnJkHdG2My4iyX_FJW185HosHSIwRfiOI/edit?usp=sharing
> > >
> >
> > Yes, thanks.
> > >
> > > I did upload a PoC but I think I was ahead of the game as it needed an
> > > RFC first.
> > >
> > >> As I mentioned in the leadership meeting, I have concerns about a
solution that relies  solely on embedding this in bootblock. Some of these
issues are specific to AMD, but if we're doing this, it should be a
solution that we can use across different platforms and architectures.
> > >>
> > >> Issues I see with embedding it the option mechanism in bootblock:1)
Bootblock can be signed for verification by cbfs. Modifying the bootblock
will cause the CBFS verification to fail.
> > >> 2) The AMD bootblock is part of the overall AMDFW binary, and not at
the top of the ROM as it used to be. This makes locating it and modifying
it more difficult - it's not a separate CBFS area as you specified.
> > >> 3) For AMD, the bootblock can be compressed, which makes it
difficult (or impossible) to update this table.4) AMD has 3 copies of the
bootblock. Which one gets updated? If all need to be updated, that's very
inconvenient and creates issues keeping them in sync.
> > >> 5) Assuming that you want this to be available to the 1st stage
that's run, on AMD, that doesn't have to be bootblock, that can be
PSP_verstage instead.
> > >> 6) What happens when there's only one bootblock and it's in RO, so
the options can't be updated? Maybe that's not an issue for ChromeOS
because the table would only be used for debug, but if we're adding
something new, let's design it so that it's useful for everyone.
> > >>
> > >>
> > >> I think it would be better to have a single table at a known fixed
location in the firmware boot media so that it can be accessed before CBFS
is enabled. This can be a separate partition in FMAP and parsed out by Make
at build-time, or set as a Kconfig option and put anywhere in flash that is
convenient to the architecture or platform (while still being writable). As
Arthur said, it's inconvenient when the firmware boot media isn't memory
mapped, so we probably need to design this with both options in mind.
> > >>
> > >> I'm fine with an option that lets us choose between embedding
something in the bootblock and placing the table somewhere else, so long as
we use the same format in either place and only have a single copy in any
given coreboot.rom image.
> > >>
> > >
> > > OK, got it.
> > >
> > >>
> > >> Also, I'd really like to see this use an existing configuration
mechanism instead of adding yet another configuration API. Here are 3
options I see:
> > >>
> > >> CMOS_BUT_IN_FLASH
> > >> One option is to use the same interface inside coreboot that we
currently use for CMOS. We should be able to just read the options from the
boot media (or memory after copying it from flash) instead of CMOS, but use
the rest of the parsing mechanisms as they already exist. CCB would just be
added as another OPTION_BACKEND in Kconfig. I do like the way we have a
method of adding default values for CMOS, so I'd prefer to see the exact
same mechanism that we use in CMOS, but stored in flash instead. This would
work well as an option for either putting into the bootblock or somewhere
else in flash.
> > >>
> > >
> > > It looks like only one backend can be active, but for x86 boards we
> > > would want CMOS RAM (for normal settings) as well as SPI flash for
> > > CCB). Also the options are accessed using strings which is less
> > > efficient than an enum.
> > >
> > I'm not sure why we'd need to use multiple backends, or why we need
CMOS for "normal" settings. Is there a reason we couldn't use the SPI flash
for all of the settings instead of CMOS?
> >
> > I guess if you want to store the information just in the bootblock,
that could be problematic, or we'd have to come up with a way to keep that
data around for other stages.
>
> I don't mean to limit it to bootblock, but my concern is that if we
> have to choose between CMOS RAM and SPI flash, then my solution is not
> universal. There are many boards which use CMOS RAM for settings. They
> don't necessarily want to change just because they want to enable the
> CCB.
>
> >
> >
> > >> SMMSTORE_VIA_ANOTHER_MECHANISM
> > >> If that's not practical, we could use the same partition and format
used by SMMstore, and create non-SMM routines to be cross-platform and
allow it to be used earlier. It looks like SMMstore can already be used as
another OPTION_BACKEND. I haven't looked into SMMstore, so I don't know if
this is practical, but this still seems better than creating something
completely new. Obviously, this doesn't work well to embed into the
bootblock.
> > >>
> > >
> > > That looks like a ramstage thing that does some sort of system call to
> > > access the data. See for example smmstore_exec(). I am not sure how I
> > > could use that here.
> > >
> > I was really suggesting that we implement a new routine to access the
SMMSTORE data directly from flash early in the boot process. Just from the
name, I assumed that it wasn't available to pre-memory stages.
>
> Yes, that seems to be for later stages. I am not sure that it would be
> feasible to access this data earlier...it looks to have quite a bit of
> code associated with it, so would expand bootblock quite a bit. Also,
> changing a value in SMMSTORE is non-trivial. It has 64KB alignment and
> requires an SMM handler to do the reads, from what I can tell.
>
> >
> > >>
> > >> CBI_IN_FLASH
> > >> Even extending CBI to read data from the firmware boot media instead
of (or in addition to) the EC would be better than creating an entirely new
interface. There's a CBI interface to read the bits directly, but CBI also
hooks into devicetree allowing devices to be enabled or disabled at
runtime. CBI is currently limited to 64 bits, but that could be extended.
This also works well for either embedding into bootblock or placing
somewhere else in flash.
> > >>
> > >
> > > That looks more plausible, since it allows reading from multiple
> > > places in fw_config_get(). As you say it is only a 64-bit word. Plus
> > > it doesn't seem to be standardised across boards. Each one defines its
> > > own bits!
> > >
> > > Having said that, it looks like the top bit is free in all cases, so I
> > > could perhaps add a 'global' config which is used by all boards?
> > >
> > > Alternatively, I could rename perhaps fw_config_get() to
> > > fw_config_get_base() and add an enum parameter to fw_config_get().
> > > But I would need to plumb this into coreboot's 'devicetree'. That
> > > doesn't seem to allow for configs which are common to all boards.
> > >
> > > What do you think?
> > >
> > Well, There's a reason that CBI was my 3rd option. If we're
implementing something new, it would be really nice to implement something
that can store an "arbitrarily large" amount of data in various different
formats. CBI is perfect for flags and options that can be stored in a few
bits, but it wasn't designed to handle strings or really even byte values.
We might be able to overcome that, but since the offsets are currently
bit-based, it'd take some rearchitecting or it could get ugly quickly.
Maybe we could just update the fields from bit#s to dword:bit pairs. I
think current implementations all only use the first dword, so that
shouldn't be too difficult.
> >
> > If we do go with CBI, I think having a global list of names that could
be used for CBI settings to enable/disable common code blocks makes sense.
If you want a setting to affect something like enabling/disabling serial
console, you would use a specific CBI name for it. If a platform doesn't
have that name in CBI, it just gets ignored, and no option is used.
> >
> > One of the things that I like about the CMOS option mechanism is that
it allows for a clean fallback path if an option is not defined for a board
or if the options feature is disabled. Whatever setting options method we
use, I'd like to continue that architecture. Allowing a clean fallback if a
name isn't in CBI kind of implies that as well.
>
> So if we go with CMOS, we could perhaps have a fallback to CCB as a
> backend,  If CMOS is not enabled (as perhaps is the case in
> bootblock?) or if the setting is not present in the cmos layout, then
> the CCB would be used. Does that sound right?
>
> We still have the problem of strings vs. enums for identifying each
> setting, but perhaps we could solve that by having an API that uses an
> enum, but then looks up a string to use calling the CMOS API?
>
> For example, we have:
>
> unsigned int get_uint_option(const char *name, const unsigned int
fallback)
>
> so could add:
>
> enum option_t {
>    OPTIONT_SILENT_CONSOLE = 1,
>
>    OPTIONT_COUNT,
> };
>
> static const char *const option_lookup[OPTIONT_COUNT] = {
>    [OPTIONT_SILENT_CONSOLE] = "silent_console"
> };
>
> unsigned int get_uint_option_enum(enum option_t opt, const unsigned
> int fallback)
> {
>    // try CCB first if enabled
>    if IS_ENABLED(CONFIG_CCB)
>       return ccb_get_uint(opt, fallback);
>
>    // fallback to string API
>    const char *name = option_lookup[opt];
>    return get_uint_option(name, fallback)
> }
>
> >
> > Since CBI was a Google implementation and there's a Google internal
spec, if we go that route, I think we'd want that spec to be public.
Actually, since it's implemented in coreboot, and a part of devicetree, I
think it probably should be public anyway. And maybe renamed from "CrOS
board information" to "coreboot board information".
> >
> >  We could then extend the CBI spec to be able to handle more than just
the EC. We'd need to define a way to identify, read from and update
different storage mediums based on the dword. It would definitely be nice
to have something that can actually handle multiple storage mediums at the
same time. I could definitely see a lot of use to having something so that
different options are read from various size areas in EC, flash, CMOS, a
data segment in the code, hardcoded build-time values, or any other future
mechanism, all at the same time.
>
> Hmmm, so that is quite involved. I see why CBI doesn't seem so great.
> On the one hand, integrating with the coreboot 'device tree' would be
> quite natural...on the other hand it is pretty complicated since I
> don't see an easy way to have standard settings globally across all
> boards.
>
> So probably CMOS + fallback is the easier approach?
>
> >
> > Take careMartin
> >
>
> Thanks for all the detail and for helping with this.
>
> Regards,
> Simon
> _______________________________________________
> coreboot mailing list -- [email protected]
> To unsubscribe send an email to [email protected]

_______________________________________________
coreboot mailing list -- [email protected]
To unsubscribe send an email to [email protected]