From: Patrick Rudolph <[email protected]>
Expose the name of the active CBFS partition under
/sys/firmware/cbfs_active_partition
In case of Google's VBOOT[1] that currently can be one of:
"FW_MAIN_A", "FW_MAIN_B" or "COREBOOT"
Will be used by fwupd[2] to determinde the active partition in the
VBOOT A/B partition update scheme.
[1]: https://doc.coreboot.org/security/vboot/index.html
[2]: https://fwupd.org/
Signed-off-by: Patrick Rudolph <[email protected]>
---
drivers/firmware/google/Kconfig | 10 ++
drivers/firmware/google/Makefile | 1 +
drivers/firmware/google/bootmedia-coreboot.c | 115 +++++++++++++++++++
drivers/firmware/google/coreboot_table.h | 13 +++
4 files changed, 139 insertions(+)
create mode 100644 drivers/firmware/google/bootmedia-coreboot.c
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 5fbbd7b8fef6..f58b723d77de 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -82,4 +82,14 @@ config GOOGLE_FMAP
the coreboot table. If found, this binary file is exported to userland
in the file /sys/firmware/fmap.
+config GOOGLE_BOOTMEDIA
+ tristate "Coreboot bootmedia params access"
+ depends on GOOGLE_COREBOOT_TABLE
+ depends on GOOGLE_FMAP
+ help
+ This option enables the kernel to search for a boot media params
+ table, providing the active CBFS partition. If found, the name of
+ the partition is exported to userland in the file
+ /sys/firmware/cbfs_active_partition.
+
endif # GOOGLE_FIRMWARE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index 6d31fe167700..e47c59376566 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o
obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o
obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
obj-$(CONFIG_GOOGLE_FMAP) += fmap-coreboot.o
+obj-$(CONFIG_GOOGLE_BOOTMEDIA) += bootmedia-coreboot.o
vpd-sysfs-y := vpd.o vpd_decode.o
obj-$(CONFIG_GOOGLE_VPD) += vpd-sysfs.o
diff --git a/drivers/firmware/google/bootmedia-coreboot.c b/drivers/firmware/google/bootmedia-coreboot.c
new file mode 100644
index 000000000000..09c0caedaf05
--- /dev/null
+++ b/drivers/firmware/google/bootmedia-coreboot.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * bootmedia-coreboot.c
+ *
+ * Exports the active VBOOT partition name through boot media params.
+ *
+ * Copyright 2012-2013 David Herrmann <[email protected]>
+ * Copyright 2017 Google Inc.
+ * Copyright 2019 Patrick Rudolph <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+
+#include "coreboot_table.h"
+#include "fmap-coreboot.h"
+
+#define CB_TAG_BOOT_MEDIA_PARAMS 0x30
+
+/* The current CBFS partition */
+static char *name;
+
+static ssize_t cbfs_active_partition_read(struct file *filp,
+ struct kobject *kobp,
+ struct bin_attribute *bin_attr,
+ char *buf,
+ loff_t pos, size_t count)
+{
+ if (!name)
+ return -ENODEV;
+
+ return memory_read_from_buffer(buf, count, &pos, name, strlen(name));
+}
+
+static int bmp_probe(struct coreboot_device *dev)
+{
+ struct lb_boot_media_params *b = &dev->bmp;
+ const char *tmp;
+
+ /* Sanity checks on the data we got */
+ if ((b->cbfs_offset == ~0) ||
+ b->cbfs_size == 0 ||
+ ((b->cbfs_offset + b->cbfs_size) > b->boot_media_size)) {
+ pr_warn("coreboot: Boot media params contains invalid data\n");
+ return -ENODEV;
+ }
+
+ tmp = coreboot_fmap_region_to_name(b->cbfs_offset, b->cbfs_size);
+ if (!tmp) {
+ pr_warn("coreboot: Active CBFS region not found in FMAP\n");
+ return -ENODEV;
+ }
+
+ name = devm_kmalloc(&dev->dev, strlen(tmp) + 2, GFP_KERNEL);
+ if (!name) {
+ kfree(tmp);
+ return -ENODEV;
+ }
+ snprintf(name, strlen(tmp) + 2, "%s\n", tmp);
+
+ kfree(tmp);
+
+ return 0;
+}
+
+static int bmp_remove(struct coreboot_device *dev)
+{
+ struct platform_device *pdev = dev_get_drvdata(&dev->dev);
+
+ platform_device_unregister(pdev);
+
+ return 0;
+}
+
+static struct coreboot_driver bmp_driver = {
+ .probe = bmp_probe,
+ .remove = bmp_remove,
+ .drv = {
+ .name = "bootmediaparams",
+ },
+ .tag = CB_TAG_BOOT_MEDIA_PARAMS,
+};
+
+static struct bin_attribute cbfs_partition_bin_attr = {
+ .attr = {.name = "cbfs_active_partition", .mode = 0444},
+ .read = cbfs_active_partition_read,
+};
+
+static int __init coreboot_bmp_init(void)
+{
+ int err;
+
+ err = sysfs_create_bin_file(firmware_kobj, &cbfs_partition_bin_attr);
+ if (err)
+ return err;
+
+ return coreboot_driver_register(&bmp_driver);
+}
+
+static void coreboot_bmp_exit(void)
+{
+ coreboot_driver_unregister(&bmp_driver);
+ sysfs_remove_bin_file(firmware_kobj, &cbfs_partition_bin_attr);
+}
+
+module_init(coreboot_bmp_init);
+module_exit(coreboot_bmp_exit);
+
+MODULE_AUTHOR("9elements Agency GmbH");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 7b7b4a6eedda..a03e039425b8 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -59,6 +59,18 @@ struct lb_framebuffer {
u8 reserved_mask_size;
};
+/* coreboot table with TAG 0x30 */
+struct lb_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;
+};
+
/* A device, additionally with information from coreboot. */
struct coreboot_device {
struct device dev;
@@ -66,6 +78,7 @@ struct coreboot_device {
struct coreboot_table_entry entry;
struct lb_cbmem_ref cbmem_ref;
struct lb_framebuffer framebuffer;
+ struct lb_boot_media_params bmp;
};
};
--
2.21.0
Quoting [email protected] (2019-10-08 04:53:26)
> From: Patrick Rudolph <[email protected]>
>
> Expose the name of the active CBFS partition under
> /sys/firmware/cbfs_active_partition
Somehow we've gotten /sys/firmware/log to be the coreboot log, and quite
frankly that blows my mind that this path was accepted upstream.
Userspace has to know it's running on coreboot firmware to know that
/sys/firmware/log is actually the coreboot log. But it is what it is so
we're not going to change it.
Why don't we expose fmap "areas" under some coreboot fmap class that has
sysfs nodes for the properties of that fmap_area? Basically
/sys/class/coreboot_fmap/section0/version
/sys/class/coreboot_fmap/section0/<area name>/{size,offset,flags}
/sys/class/coreboot_fmap/section1/<area name>/{size,offset,flags}
I made 'section' an incrementing number because I'm not sure that the
fmap header, struct fmap::name, is unique for all fmaps. Is it? If it is
unique then we can have
/sys/class/coreboot_fmap/<fmap name>/<area name>/{size,flags}
And we may want to make the fmap area a class too, so it would be
/sys/class/coreboot_fmap/<fmap name>/coreboot_fmap_area/<area name>/{size,offset,flags}
But I also wonder why this is being exposed by the kernel at all? Why
can't userspace read the fmap table from the flash chip itself and then
parse it to understand how to update the firmware on the chip? Isn't
that better than relying on coreboot to populate something for us in
memory that describes the flash chip so we can seek around it? I guess
this is faster this way because reading flash chips may be slow?
>
> In case of Google's VBOOT[1] that currently can be one of:
> "FW_MAIN_A", "FW_MAIN_B" or "COREBOOT"
>
> Will be used by fwupd[2] to determinde the active partition in the
s/determinde/determine/ ?
> VBOOT A/B partition update scheme.
>
> [1]: https://doc.coreboot.org/security/vboot/index.html
> [2]: https://fwupd.org/
Can you link to the code in fwupd that is using this stuff from the
kernel?
>
> Signed-off-by: Patrick Rudolph <[email protected]>
> ---
> drivers/firmware/google/Kconfig | 10 ++
> drivers/firmware/google/Makefile | 1 +
> drivers/firmware/google/bootmedia-coreboot.c | 115 +++++++++++++++++++
> drivers/firmware/google/coreboot_table.h | 13 +++
> 4 files changed, 139 insertions(+)
> create mode 100644 drivers/firmware/google/bootmedia-coreboot.c
>
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 5fbbd7b8fef6..f58b723d77de 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -82,4 +82,14 @@ config GOOGLE_FMAP
> the coreboot table. If found, this binary file is exported to userland
> in the file /sys/firmware/fmap.
>
> +config GOOGLE_BOOTMEDIA
Please try to add this Kconfig somewhere alphabetically. Otherwise
people keep adding to the end of the list and it becomes sort of hard to
manage merges.
> + tristate "Coreboot bootmedia params access"
> + depends on GOOGLE_COREBOOT_TABLE
> + depends on GOOGLE_FMAP
> + help
> + This option enables the kernel to search for a boot media params
> + table, providing the active CBFS partition. If found, the name of
> + the partition is exported to userland in the file
> + /sys/firmware/cbfs_active_partition.
> +
> endif # GOOGLE_FIRMWARE
> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> index 6d31fe167700..e47c59376566 100644
> --- a/drivers/firmware/google/Makefile
> +++ b/drivers/firmware/google/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o
> obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o
> obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
> obj-$(CONFIG_GOOGLE_FMAP) += fmap-coreboot.o
> +obj-$(CONFIG_GOOGLE_BOOTMEDIA) += bootmedia-coreboot.o
Same here, although it looks like FMAP would need to move in the
previous patch to be sorted by Kconfig name.
>
> vpd-sysfs-y := vpd.o vpd_decode.o
> obj-$(CONFIG_GOOGLE_VPD) += vpd-sysfs.o
> diff --git a/drivers/firmware/google/bootmedia-coreboot.c b/drivers/firmware/google/bootmedia-coreboot.c
> new file mode 100644
> index 000000000000..09c0caedaf05
> --- /dev/null
> +++ b/drivers/firmware/google/bootmedia-coreboot.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * bootmedia-coreboot.c
> + *
> + * Exports the active VBOOT partition name through boot media params.
> + *
> + * Copyright 2012-2013 David Herrmann <[email protected]>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 Patrick Rudolph <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +
> +#include "coreboot_table.h"
> +#include "fmap-coreboot.h"
> +
> +#define CB_TAG_BOOT_MEDIA_PARAMS 0x30
> +
> +/* The current CBFS partition */
> +static char *name;
Can this be passed through some file pointer data member instead of
being a global?
> +
> +static ssize_t cbfs_active_partition_read(struct file *filp,
> + struct kobject *kobp,
> + struct bin_attribute *bin_attr,
> + char *buf,
> + loff_t pos, size_t count)
> +{
> + if (!name)
> + return -ENODEV;
> +
> + return memory_read_from_buffer(buf, count, &pos, name, strlen(name));
> +}
> +
> +static int bmp_probe(struct coreboot_device *dev)
Maybe call this cdev instead of dev so the code reads as &cdev->dev. Or
'cbdev' maybe.
> +{
> + struct lb_boot_media_params *b = &dev->bmp;
> + const char *tmp;
> +
> + /* Sanity checks on the data we got */
> + if ((b->cbfs_offset == ~0) ||
Please drop useless parenthesis.
> + b->cbfs_size == 0 ||
> + ((b->cbfs_offset + b->cbfs_size) > b->boot_media_size)) {
> + pr_warn("coreboot: Boot media params contains invalid data\n");
dev_err()
> + return -ENODEV;
> + }
> +
> + tmp = coreboot_fmap_region_to_name(b->cbfs_offset, b->cbfs_size);
> + if (!tmp) {
> + pr_warn("coreboot: Active CBFS region not found in FMAP\n");
Use dev_warn() or dev_err() so 'coreboot:' can be dropped.
> + return -ENODEV;
> + }
> +
> + name = devm_kmalloc(&dev->dev, strlen(tmp) + 2, GFP_KERNEL);
> + if (!name) {
> + kfree(tmp);
> + return -ENODEV;
> + }
> + snprintf(name, strlen(tmp) + 2, "%s\n", tmp);
devm_kasprintf() can handle this all.
> +
> + kfree(tmp);
> +
> + return 0;
> +}
> +
> +static int bmp_remove(struct coreboot_device *dev)
> +{
> + struct platform_device *pdev = dev_get_drvdata(&dev->dev);
> +
> + platform_device_unregister(pdev);
What is this doing? Was some sort of platform device created?
> +
> + return 0;
> +}
> +
> +static struct coreboot_driver bmp_driver = {
> + .probe = bmp_probe,
> + .remove = bmp_remove,
> + .drv = {
> + .name = "bootmediaparams",
> + },
> + .tag = CB_TAG_BOOT_MEDIA_PARAMS,
> +};
> +
> +static struct bin_attribute cbfs_partition_bin_attr = {
> + .attr = {.name = "cbfs_active_partition", .mode = 0444},
> + .read = cbfs_active_partition_read,
> +};
> +
> +static int __init coreboot_bmp_init(void)
> +{
> + int err;
> +
> + err = sysfs_create_bin_file(firmware_kobj, &cbfs_partition_bin_attr);
> + if (err)
> + return err;
I don't think we want to create a sysfs object whenever this driver is
loaded. We should only make sysfs nodes when a device matches a driver.
> +
> + return coreboot_driver_register(&bmp_driver);
> +}
> +
> +static void coreboot_bmp_exit(void)
> +{
> + coreboot_driver_unregister(&bmp_driver);
> + sysfs_remove_bin_file(firmware_kobj, &cbfs_partition_bin_attr);
> +}
> +
> +module_init(coreboot_bmp_init);
> +module_exit(coreboot_bmp_exit);
Just use module_coreboot_driver() instead.
> +
> +MODULE_AUTHOR("9elements Agency GmbH");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index 7b7b4a6eedda..a03e039425b8 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -59,6 +59,18 @@ struct lb_framebuffer {
> u8 reserved_mask_size;
> };
>
> +/* coreboot table with TAG 0x30 */
> +struct lb_boot_media_params {
> + u32 tag;
> + u32 size;
Not a problem with this patch, but I think we should make these two
common fields 'struct coreboot_table_entry' that is inside this struct
and also make all these fields __le{32,64} and do endian conversions in
the code.
> +
> + /* offsets are relative to start of boot media */
> + u64 fmap_offset;
> + u64 cbfs_offset;
> + u64 cbfs_size;
> + u64 boot_media_size;
> +};
> +
> /* A device, additionally with information from coreboot. */
> struct coreboot_device {
> struct device dev;
> Somehow we've gotten /sys/firmware/log to be the coreboot log, and quite
> frankly that blows my mind that this path was accepted upstream.
> Userspace has to know it's running on coreboot firmware to know that
> /sys/firmware/log is actually the coreboot log.
Not really sure I understand your concern here? That's the generic
node for the log from the mainboard firmware, whatever it is. It was
originally added for non-coreboot firmware and that use is still
supported. If some other non-coreboot firmware wants to join in, it's
welcome to do so -- the interface is separated out enough to make it
easy to add more backends.
I do agree that if we want to add other, more coreboot-specific nodes,
they should be explicitly namespaced.
> But I also wonder why this is being exposed by the kernel at all?
I share Stephen's concern that I'm not sure this belongs in the kernel
at all. There are existing ways for userspace to access this
information like the cbmem utility does... if you want it accessible
from fwupd, it could chain-call into cbmem or we could factor that
functionality out into a library. If you want to get away from using
/dev/mem for this, we could maybe add a driver that exports CBMEM or
coreboot table areas via sysfs... but then I think that should be a
generic driver which makes them all accessible in one go, rather than
having to add yet another driver whenever someone needs to parse
another coreboot table blob for some reason. We could design an
interface like /sys/firmware/coreboot/table/<tag> where every entry in
the table gets exported as a binary file.
I think a specific sysfs driver only makes sense for things that are
human readable and that you'd actually expect a human to want to go
read directly, like the log. Maybe exporting FMAP entries one by one
like Stephen suggests could be such a case, but I doubt that there's a
common enough need for that since there are plenty of existing ways to
show FMAP entries from userspace (and if there was a generic interface
like /sys/firmware/coreboot/table/37 to access it, we could just add a
new flag to the dump_fmap utility to read it from there).
On Wed, 2019-10-09 at 14:19 -0700, Julius Werner wrote:
> > Somehow we've gotten /sys/firmware/log to be the coreboot log, and
> > quite
> > frankly that blows my mind that this path was accepted upstream.
> > Userspace has to know it's running on coreboot firmware to know
> > that
> > /sys/firmware/log is actually the coreboot log.
>
> Not really sure I understand your concern here? That's the generic
> node for the log from the mainboard firmware, whatever it is. It was
> originally added for non-coreboot firmware and that use is still
> supported. If some other non-coreboot firmware wants to join in, it's
> welcome to do so -- the interface is separated out enough to make it
> easy to add more backends.
>
> I do agree that if we want to add other, more coreboot-specific
> nodes,
> they should be explicitly namespaced.
>
I'll add a new namespace called 'coreboot'.
> > But I also wonder why this is being exposed by the kernel at all?
>
It's difficult for userspace tools to find out how to access the flash
and then to find the FMAP, which resides somewhere in it on 4KiB boundary.
The "boot media params" usually give the offset to the FMAP from beginning
of the flash, but tell nothing about how to access it.
> I share Stephen's concern that I'm not sure this belongs in the
> kernel
> at all. There are existing ways for userspace to access this
> information like the cbmem utility does... if you want it accessible
> from fwupd, it could chain-call into cbmem or we could factor that
> functionality out into a library. If you want to get away from using
> /dev/mem for this, we could maybe add a driver that exports CBMEM or
> coreboot table areas via sysfs... but then I think that should be a
> generic driver which makes them all accessible in one go, rather than
> having to add yet another driver whenever someone needs to parse
> another coreboot table blob for some reason. We could design an
> interface like /sys/firmware/coreboot/table/<tag> where every entry
> in
> the table gets exported as a binary file.
I don't even consider using binaries that operate on /dev/mem.
In my opinion CBMEM is something coreboot internal, the OS or userspace
shouldn't even known about.
> I think a specific sysfs driver only makes sense for things that are
> human readable and that you'd actually expect a human to want to go
> read directly, like the log. Maybe exporting FMAP entries one by one
> like Stephen suggests could be such a case, but I doubt that there's
> a
> common enough need for that since there are plenty of existing ways
> to
> show FMAP entries from userspace (and if there was a generic
> interface
> like /sys/firmware/coreboot/table/37 to access it, we could just add
> a
> new flag to the dump_fmap utility to read it from there).
I'll expose the coreboot tables using a sysfs driver, which then can be
used by coreboot tools instead of accessing /dev/mem. As it holds the
FMAP and "boot media params" that's all I need for now.
The downside is that the userspace tools need to be keep in sync with
the binary interface the coreboot tables are providing.
-Filipe bounce
Quoting [email protected] (2019-10-10 02:46:53)
> On Wed, 2019-10-09 at 14:19 -0700, Julius Werner wrote:
>
> > > But I also wonder why this is being exposed by the kernel at all?
> >
>
> It's difficult for userspace tools to find out how to access the flash
> and then to find the FMAP, which resides somewhere in it on 4KiB boundary.
> The "boot media params" usually give the offset to the FMAP from beginning
> of the flash, but tell nothing about how to access it.
Great! That's what I wanted to know. If it's difficult to find then it
must be easier to use the coreboot tables to find it and it improves
overall speed for the firmware update.
>
> > I share Stephen's concern that I'm not sure this belongs in the
> > kernel
> > at all. There are existing ways for userspace to access this
> > information like the cbmem utility does... if you want it accessible
> > from fwupd, it could chain-call into cbmem or we could factor that
> > functionality out into a library. If you want to get away from using
> > /dev/mem for this, we could maybe add a driver that exports CBMEM or
> > coreboot table areas via sysfs... but then I think that should be a
> > generic driver which makes them all accessible in one go, rather than
> > having to add yet another driver whenever someone needs to parse
> > another coreboot table blob for some reason. We could design an
> > interface like /sys/firmware/coreboot/table/<tag> where every entry
> > in
> > the table gets exported as a binary file.
>
> I don't even consider using binaries that operate on /dev/mem.
> In my opinion CBMEM is something coreboot internal, the OS or userspace
> shouldn't even known about.
Yes. To be clear I'm not suggesting we make this a binary file design,
although patch 1 is exporting a binary file. I was just wondering why we
couldn't search the flash itself.
>
> > I think a specific sysfs driver only makes sense for things that are
> > human readable and that you'd actually expect a human to want to go
> > read directly, like the log. Maybe exporting FMAP entries one by one
> > like Stephen suggests could be such a case, but I doubt that there's
> > a
> > common enough need for that since there are plenty of existing ways
> > to
> > show FMAP entries from userspace (and if there was a generic
> > interface
> > like /sys/firmware/coreboot/table/37 to access it, we could just add
> > a
> > new flag to the dump_fmap utility to read it from there).
>
> I'll expose the coreboot tables using a sysfs driver, which then can be
> used by coreboot tools instead of accessing /dev/mem. As it holds the
> FMAP and "boot media params" that's all I need for now.
>
> The downside is that the userspace tools need to be keep in sync with
> the binary interface the coreboot tables are providing.
>
I'd rather we export this information in sysfs via some coreboot_fmap
class and then make the "boot media params" another property of one of
the fmap devices. Then userspace can search through all the fmap devices
and find the "boot media params" one. Is anything else needed?
> > I'll expose the coreboot tables using a sysfs driver, which then can be
> > used by coreboot tools instead of accessing /dev/mem. As it holds the
> > FMAP and "boot media params" that's all I need for now.
> >
> > The downside is that the userspace tools need to be keep in sync with
> > the binary interface the coreboot tables are providing.
Well, in the other version the kernel needs to be kept in sync
instead. No matter where you do the parsing, something needs to be
kept in sync. I think userspace would be easier, especially if we
would host a small userspace library in the coreboot repository that
other tools could just link.
> I'd rather we export this information in sysfs via some coreboot_fmap
> class and then make the "boot media params" another property of one of
> the fmap devices. Then userspace can search through all the fmap devices
> and find the "boot media params" one. Is anything else needed?
Okay, this is the fundamental question we need to answer first... do
you really think it's better to add a separate interface for each of
these, Stephen? The coreboot table[1] currently contains 49 entries
with all sorts of random firmware information, and most of these will
never be interesting to the kernel. Do we want to establish a pattern
where we add a new sysfs interface for each of them whenever someone
has a use case for reading it from userspace? I think this might be a
good point to implement a generic interface to read any coreboot table
entry from userspace instead, and say that if the kernel doesn't need
the information in a specific entry itself, it shouldn't need to know
how to parse it.
[1] https://review.coreboot.org/cgit/coreboot.git/tree/src/commonlib/include/commonlib/coreboot_tables.h
On 10/9/19 4:19 PM, Julius Werner wrote:
>> Somehow we've gotten /sys/firmware/log to be the coreboot log, and quite
>> frankly that blows my mind that this path was accepted upstream.
>> Userspace has to know it's running on coreboot firmware to know that
>> /sys/firmware/log is actually the coreboot log.
>
> Not really sure I understand your concern here? That's the generic
> node for the log from the mainboard firmware, whatever it is. It was
> originally added for non-coreboot firmware and that use is still
> supported. If some other non-coreboot firmware wants to join in, it's
> welcome to do so -- the interface is separated out enough to make it
> easy to add more backends.
>
> I do agree that if we want to add other, more coreboot-specific nodes,
> they should be explicitly namespaced.
>
>> But I also wonder why this is being exposed by the kernel at all?
>
> I share Stephen's concern that I'm not sure this belongs in the kernel
> at all. There are existing ways for userspace to access this
> information like the cbmem utility does... if you want it accessible
> from fwupd, it could chain-call into cbmem or we could factor that
> functionality out into a library. If you want to get away from using
> /dev/mem for this, we could maybe add a driver that exports CBMEM or
> coreboot table areas via sysfs... but then I think that should be a
> generic driver which makes them all accessible in one go, rather than
> having to add yet another driver whenever someone needs to parse
> another coreboot table blob for some reason. We could design an
> interface like /sys/firmware/coreboot/table/<tag> where every entry in
> the table gets exported as a binary file.
>
> I think a specific sysfs driver only makes sense for things that are
> human readable and that you'd actually expect a human to want to go
> read directly, like the log. Maybe exporting FMAP entries one by one
> like Stephen suggests could be such a case, but I doubt that there's a
> common enough need for that since there are plenty of existing ways to
> show FMAP entries from userspace (and if there was a generic interface
> like /sys/firmware/coreboot/table/37 to access it, we could just add a
> new flag to the dump_fmap utility to read it from there)
There's already a node in sysfs for every coreboot table entry:
/sys/bus/coreboot/devices/corebootN
where N is the index of the entry in the coreboot table. I suggest
1) Rename the devices based on their tag instead of their position in the table,
so the names are stable and meaningful. I don't know why I didn't do that in
the first place. Doing that gives you a device kobject you can hang
additional sysfs attributes off of.
2) If we want userspace to have access to the raw binary table entry (which
sounds like a good idea to me), add that sysfs attribute in
coreboot_table_populate() after creating each device. That way it would be
present regardless of whether or not there's a specific driver loaded for
that table entry.
This solves the immediate problem of needing /dev/mem to access coreboot tables,
while leaving open the possibility of writing kernel drivers in the future if
that would be more beneficial than parsing the data in userspace (i.e. if the
kernel can do something more with the data than just exporting it).
Samuel
Quoting Julius Werner (2019-10-10 11:37:58)
> > > I'll expose the coreboot tables using a sysfs driver, which then can be
> > > used by coreboot tools instead of accessing /dev/mem. As it holds the
> > > FMAP and "boot media params" that's all I need for now.
> > >
> > > The downside is that the userspace tools need to be keep in sync with
> > > the binary interface the coreboot tables are providing.
>
> Well, in the other version the kernel needs to be kept in sync
> instead. No matter where you do the parsing, something needs to be
> kept in sync. I think userspace would be easier, especially if we
> would host a small userspace library in the coreboot repository that
> other tools could just link.
>
> > I'd rather we export this information in sysfs via some coreboot_fmap
> > class and then make the "boot media params" another property of one of
> > the fmap devices. Then userspace can search through all the fmap devices
> > and find the "boot media params" one. Is anything else needed?
>
> Okay, this is the fundamental question we need to answer first... do
> you really think it's better to add a separate interface for each of
> these, Stephen? The coreboot table[1] currently contains 49 entries
> with all sorts of random firmware information, and most of these will
> never be interesting to the kernel. Do we want to establish a pattern
> where we add a new sysfs interface for each of them whenever someone
> has a use case for reading it from userspace? I think this might be a
> good point to implement a generic interface to read any coreboot table
> entry from userspace instead, and say that if the kernel doesn't need
> the information in a specific entry itself, it shouldn't need to know
> how to parse it.
I don't know why we need to draw a line in the sand and say that if the
kernel doesn't need to know about it then it shouldn't parse it. I want
there to be a consistent userspace ABI that doesn't just move things
straight from memory to userspace in some binary format. I'd rather we
have an ABI that decodes and exposes information about the coreboot
tables through existing frameworks/subsystems where possible and makes
up new ones otherwise.
One concern I have is endianness of the binary data. Is it big endian or
little endian or CPU native endian? The kernel can boot into big or
little endian on ARM platforms and userspace can be different vs. the
bootloader too. Userspace shouldn't need to know this detail, the kernel
should know and do the conversions and expose it somehow. That's why I'm
suggesting in this case we describe fmap as a sysfs class. I don't see
how we could export that information otherwise, besides in a binary blob
that falls into traps like this.
Right now we make devices for all the coreboot table entries, which is
pretty weird considering that some table entries are things like
LB_TAG_LINKER. That isn't a device. It's some information about how
coreboot was linked. We should probably blacklist tags so we don't make
devices and capture these ones in the bus code and expose them in
/sys/firware/coreboot/ somehow. We should also add device randomness
from the serial numbers, etc. that coreboot has stashed away in the
tables.
>
> [1] https://review.coreboot.org/cgit/coreboot.git/tree/src/commonlib/include/commonlib/coreboot_tables.h
> I don't know why we need to draw a line in the sand and say that if the
> kernel doesn't need to know about it then it shouldn't parse it. I want
> there to be a consistent userspace ABI that doesn't just move things
> straight from memory to userspace in some binary format. I'd rather we
> have an ABI that decodes and exposes information about the coreboot
> tables through existing frameworks/subsystems where possible and makes
> up new ones otherwise.
Okay... I'm just saying this might grow to become a lot of stuff as
people start having more and more use cases they want to support. But
if you think the kernel should be the one parsing all that, I'm happy
to defer to your expertise there (I'm not really a kernel guy after
all).
> One concern I have is endianness of the binary data. Is it big endian or
> little endian or CPU native endian? The kernel can boot into big or
> little endian on ARM platforms and userspace can be different vs. the
> bootloader too. Userspace shouldn't need to know this detail, the kernel
> should know and do the conversions and expose it somehow. That's why I'm
> suggesting in this case we describe fmap as a sysfs class. I don't see
> how we could export that information otherwise, besides in a binary blob
> that falls into traps like this.
Right now it's just always CPU byte order of what coreboot happened to
run at, and it's not exporting that info in any way either. We don't
really have support for big-endian architectures anyway so it's not
something we really thought about yet. If it ever comes to that, I
assume the byte order of the table header's magic number could be used
to tell the difference.
> Right now we make devices for all the coreboot table entries, which is
> pretty weird considering that some table entries are things like
> LB_TAG_LINKER. That isn't a device. It's some information about how
> coreboot was linked. We should probably blacklist tags so we don't make
> devices and capture these ones in the bus code and expose them in
> /sys/firware/coreboot/ somehow. We should also add device randomness
> from the serial numbers, etc. that coreboot has stashed away in the
> tables.
I mean... should any of them be devices, then? All table entries are
just "some information", where are you defining the difference there?
I'm not sure if the current representation is the right one, but I
think they should probably all be handled in a consistent way.