Hi,
On Mon, 2 Oct 2023 at 12:15, Simon Glass sjg@chromium.org wrote:
Hi Martin,
On Fri, 29 Sept 2023 at 14:22, Martin Roth gaumless@tutanota.com wrote:
Hi Simon, Sep 29, 2023, 13:04 by sjg@chromium.org:
Hi Martin,
On Mon, 25 Sept 2023 at 13:51, Martin Roth gaumless@tutanota.com 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_FJW185HosHSIwRf...
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.
I believe we agreed that this would be a valuable feature in coreboot. What do people think about the above design?
Regards, Simon