[SeaBIOS] [PATCH] fw/coreboot.c: Use coreboot table to find cbfs

Arthur Heymans posted 1 patch 2 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20210430082826.394372-1-arthur@aheymans.xyz
[SeaBIOS] [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Arthur Heymans 2 years, 11 months ago
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();
-- 
2.30.2

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Peter Stuge 2 years, 10 months ago
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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Gerd Hoffmann 2 years, 10 months ago
On Thu, May 20, 2021 at 06:09:55PM +0000, Peter Stuge wrote:
> 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?

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.

> > Keeping backward compatibility with the "cbfs master header" would
> > complicate the code.
> 
> Obviously, but is undeniably valuable, even if not to you.

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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Peter Stuge 2 years, 10 months ago
Hi Gerd,

Gerd Hoffmann 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?
> 
> 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.

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.


> > > Keeping backward compatibility with the "cbfs master header" would
> > > complicate the code.
> > 
> > Obviously, but is undeniably valuable, even if not to you.
> 
> 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.

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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Gerd Hoffmann 2 years, 10 months ago
  Hi,

> > 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.
> 
> Sure, but backwards compatibility is highly valuable, so will offset
> quite some cost. See Windows or the Linux kernel ABI.

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.

> 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 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.

> > > > Keeping backward compatibility with the "cbfs master header" would
> > > > complicate the code.
> > > 
> > > Obviously, but is undeniably valuable, even if not to you.
> > 
> > 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.
> 
> I know that five years is forever in QEMU,

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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH] fw/coreboot.c: Use coreboot table to find cbfs
Posted by Peter Stuge 2 years, 10 months ago
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?


> 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.

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.


> > 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.

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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org