The "cbfs master header" cbfs file is considered a legacy feature in coreboot and is planned for removal in the master branch. Since 2015 the coreboot tables have exported info about the active cbfs.
This change uses the cbfs information in the coreboot tables instead of following the cbfs master header pointer in the bootblock to find cbfs. This change makes it possible to access CBFS fmap regions that don't feature a "cbfs master header", which is the case with Googles VBOOT solution.
This breaks compatibility with very old coreboot build (build before fb5d5b16 "2015-07-14, cbtable: describe boot media"). Keeping backward compatibility with the "cbfs master header" would complicate the code.
Signed-off-by: Arthur Heymans arthur@aheymans.xyz
diff --git a/src/Kconfig b/src/Kconfig index 3a8ffa1..51ba827 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -93,15 +93,6 @@ endchoice help Support CBFS files compressed using the lzma decompression algorithm. - config CBFS_LOCATION - depends on COREBOOT_FLASH - hex "CBFS memory end location" - default 0 - help - Memory address of where the CBFS data ends. This should - be zero for normal builds. It may be a non-zero value if - the CBFS filesystem is at a non-standard location (eg, - 0xffe00000 if CBFS ends 2Meg below the end of flash).
config MULTIBOOT depends on COREBOOT diff --git a/src/fw/coreboot.c b/src/fw/coreboot.c index 7c0954b..8a35c1d 100644 --- a/src/fw/coreboot.c +++ b/src/fw/coreboot.c @@ -88,6 +88,18 @@ struct cbmem_console { #define CBMC_OVERFLOW (1 << 31) static struct cbmem_console *cbcon = NULL;
+#define CB_TAG_BOOT_MEDIA_PARAMS 0x30 + +struct cb_boot_media_params { + u32 tag; + u32 size; + /* offsets are relative to start of boot media */ + u64 fmap_offset; + u64 cbfs_offset; + u64 cbfs_size; + u64 boot_media_size; +}; + static u16 ipchksum(char *buf, int count) { @@ -146,7 +158,12 @@ find_cb_subtable(struct cb_header *cbh, u32 tag) struct cb_header * find_cb_table(void) { - struct cb_header *cbh = find_cb_header(0, 0x1000); + static struct cb_header *cbh; + + if (cbh) + return cbh; + + cbh= find_cb_header(0, 0x1000); if (!cbh) return NULL; struct cb_forward *cbf = find_cb_subtable(cbh, CB_TAG_FORWARD); @@ -317,20 +334,8 @@ ulzma(u8 *dst, u32 maxlen, const u8 *src, u32 srclen) * Coreboot flash format ****************************************************************/
-#define CBFS_HEADER_MAGIC 0x4F524243 -#define CBFS_VERSION1 0x31313131 - -struct cbfs_header { - u32 magic; - u32 version; - u32 romsize; - u32 bootblocksize; - u32 align; - u32 offset; - u32 pad[2]; -} PACKED; - #define CBFS_FILE_MAGIC 0x455649484352414cLL // LARCHIVE +#define CBFS_ALIGNMENT 64
struct cbfs_file { u64 magic; @@ -429,30 +434,50 @@ coreboot_cbfs_init(void) if (!CONFIG_COREBOOT_FLASH) return;
- struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4); - if ((u32)hdr & 0x03) { - dprintf(1, "Invalid CBFS pointer %p\n", hdr); + dprintf(3, "Attempting to initialize coreboot cbfs\n"); + + struct cb_header *cbh = find_cb_table(); + if (!cbh) { + dprintf(1, "cbfs_init: unable to find cb table\n"); return; } - if (CONFIG_CBFS_LOCATION && (u32)hdr > CONFIG_CBFS_LOCATION) - // Looks like the pointer is relative to CONFIG_CBFS_LOCATION - hdr = (void*)hdr + CONFIG_CBFS_LOCATION; - if (hdr->magic != cpu_to_be32(CBFS_HEADER_MAGIC)) { - dprintf(1, "Unable to find CBFS (ptr=%p; got %x not %x)\n" - , hdr, hdr->magic, cpu_to_be32(CBFS_HEADER_MAGIC)); + + struct cb_boot_media_params *cbbmp = + find_cb_subtable(cbh, CB_TAG_BOOT_MEDIA_PARAMS); + + if (!cbbmp) { + dprintf(1, "cbfs_init: unable to find boot media params\n"); return; } - dprintf(1, "Found CBFS header at %p\n", hdr);
- u32 romsize = be32_to_cpu(hdr->romsize); - u32 romstart = CONFIG_CBFS_LOCATION - romsize; - struct cbfs_file *fhdr = (void*)romstart + be32_to_cpu(hdr->offset); + const u32 romsize = cbbmp->boot_media_size; + const u32 cbfs_start = cbbmp->cbfs_offset - romsize; + const u32 cbfs_end = cbfs_start + cbbmp->cbfs_size - 1; + + dprintf(3, "cbfs_init: cbfs_start=0x%08x, cbfs_end=0x%08x\n", cbfs_start, + cbfs_end); + + u32 offset = cbfs_start; + for (;;) { - if ((u32)fhdr - romstart > romsize) - break; - u64 magic = fhdr->magic; - if (magic != CBFS_FILE_MAGIC) + if (offset > cbfs_end || offset < cbfs_start) break; + + struct cbfs_file *fhdr = (void*)offset; + + if (fhdr->magic != CBFS_FILE_MAGIC) { + offset++; + offset = ALIGN(offset, CBFS_ALIGNMENT); + continue; + } + + /* Skip empty space */ + if (strcmp(fhdr->filename, "") == 0) { + offset += ALIGN(be32_to_cpu(fhdr->offset) + be32_to_cpu(fhdr->len) + , CBFS_ALIGNMENT); + continue; + } + struct cbfs_romfile_s *cfile = malloc_tmp(sizeof(*cfile)); if (!cfile) { warn_noalloc(); @@ -473,8 +498,8 @@ coreboot_cbfs_init(void) } romfile_add(&cfile->file);
- fhdr = (void*)ALIGN((u32)cfile->data + cfile->rawsize - , be32_to_cpu(hdr->align)); + offset = ALIGN((u32)cfile->data + cfile->rawsize + , CBFS_ALIGNMENT); }
process_links_file();
On Fri, Apr 30, 2021 at 10:28:26AM +0200, Arthur Heymans wrote:
Thanks and sorry for the delay in responding. In general the change looks fine to me. If there are no further comments I'll look to commit next week.
-Kevin
Arthur Heymans wrote:
This breaks compatibility with very old coreboot build (build before fb5d5b16 "2015-07-14, cbtable: describe boot media").
Is that really acceptable in SeaBIOS master at some random time?
At the very least I would expect a flag day with advance publicity.
One way of accomplishing that would be to include a notice of this change with the next SeaBIOS release and if at all only delete backwards compatibility in the release after.
Keeping backward compatibility with the "cbfs master header" would complicate the code.
Obviously, but is undeniably valuable, even if not to you.
Proper advance notice of a breaking change allows others to invest effort into coming up with a compatibility solution.
//Peter
On Thu, May 20, 2021 at 06:09:55PM +0000, Peter Stuge wrote:
As far I know there is no policy on that written down somewhere. In general we try avoid breaking backward compatibility (and thus requiring lockstep updates). But maintaining backward compatibility has a cost too, so this isn't set in stone.
Well, maintaining compatibility with a version released more than five years ago isn't that valuable IMHO, but comes with the cost of adding compatibility code which most likely will never ever be actually used.
Proper advance notice of a breaking change allows others to invest effort into coming up with a compatibility solution.
Well. The compatible solution exists since 2015-07-14 ...
take care, Gerd
Hi Gerd,
Gerd Hoffmann wrote:
Sure, but backwards compatibility is highly valuable, so will offset quite some cost. See Windows or the Linux kernel ABI.
Here we are talking about a firmware compatibility, arguably even more valuable than a kernel ABI, because firmware often, and ironically this is probably no less true for coreboot than IBV products, simply can not be updated.
I expect payloads to value backwards compatibility quite high.
I know that five years is forever in QEMU, and perhaps in particular at Red Hat.
Firmware is not QEMU.
At a minimum please at least announce a flag day a month or two out, to give those not on this list a chance.
Thanks
//Peter
Hi,
Well, depends ...
The linux kernel's userspace ABI maintains backward compatibility, yes. And there is countless software in the world depending on that. Still there are exceptions (see cgroups move from v1 to v2).
Note that the linux kernel's in-kernel interfaces are explicitly *not* backward compatible though.
I fail to see the problem. seabios is part of the firmware, users are not going to freely mix and match versions. So if you are stuck with an old coreboot version for whatever reason, just continue using an old seabios version. It's not like seabios does see heavy development, and the changes going in are mostly for new hardware support (recent example is nvme) which doesn't buy you much on old machines.
Well, we are at eight years right now, maintaining backward compatibility to qemu 1.4, released in Feb 2013. Compatibility code for older versions has been dropped (last year I think).
Which reminds me that we can drop CONFIG_ACPI_DSDT + dependencies from seabios.
and perhaps in particular at Red Hat.
?
Firmware is not QEMU.
Note that coreboot apparently considers 5 years of backward compatibility enough. They supported both old and new method for finding cbfs that long.
take care, Gerd
Hi,
Gerd Hoffmann wrote:
Note that the linux kernel's in-kernel interfaces are explicitly *not* backward compatible though.
..
I fail to see the problem. seabios is part of the firmware,
So that's important, I hope to help create some understanding:
coreboot and SeaBIOS are cleanly separated.
This separation compares quite well to the clean separation between Linux kernel and the many applications you mention which depend on kernel APIs.
I agree that coreboot should also pull weight to maintain compatibility here, but now and then all the burden falls on payloads. :\
users are not going to freely mix and match versions.
You do recognize that this is a self-fulfilling prophecy, I hope?
They're certainly not going to do it if it was made impossible!
So if you are stuck with an old coreboot version for whatever reason, just continue using an old seabios version.
I find that attitude absolutely obnoxious, which is why I question whether SeaBIOS indeed wants to proceed with such an offensive change.
Boards are removed from coreboot in most every release so "getting stuck" is a real thing.
Announcing a deprecation flag day in the next SeaBIOS release to give people not reading every patch on this list an opportunity to engage is an easy ask. Why the rush?
This just sounds like privilege, especially given Volker's recent threading/interrupt bugfix. That's a perfect example of a significant improvement which should be available also with older coreboot.
Could of course create some SeaBIOS branches for backports and release master to move fast and break stuff, but I don't think that's the best option here.
Do you mean within the coreboot codebase?
I agree with you if you suggest that coreboot should not remove backwards compatibility in the payload interface willy-nilly!
But I assume that's off the table, so SeaBIOS can only decide how it wants to behave.
//Peter
On Thu, May 20, 2021 at 06:09:55PM +0000, Peter Stuge wrote:
I have not kept up on coreboot changes. If there are many active users with boards that can not support this change then I will hold off on committing this patch. We can further discuss how to best handle the change.
-Kevin
Hi
It has now been close to a year since I posted this. Can we get it merged? Or do we really expect people using a pre-2015 coreboot build to be updated to use a 2022 SeaBIOS build?
Arthur
On Wed, May 26, 2021 at 3:44 PM Kevin O'Connor kevin@koconnor.net wrote:
-
Arthur Heymans -
Gerd Hoffmann -
Kevin O'Connor -
Peter Stuge