[PATCH 1/2] firmware: coreboot: Export the binary FMAP

[PATCH 1/2] firmware: coreboot: Export the binary FMAP

[patrick.rudolph]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Expose coreboot's binary FMAP[1] to /sys/firmware/fmap.

coreboot copies the FMAP to a CBMEM buffer at boot since CB:35377[2],
allowing an architecture independ way of exposing the FMAP to userspace.

Will be used by fwupd[3] to determine the current firmware layout.

[1]: https://doc.coreboot.org/lib/flashmap.html
[2]: https://review.coreboot.org/c/coreboot/+/35377
[3]: https://fwupd.org/

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 drivers/firmware/google/Kconfig           |   8 ++
 drivers/firmware/google/Makefile          |   1 +
 drivers/firmware/google/fmap-coreboot.c   | 156 ++++++++++++++++++++++
 drivers/firmware/google/fmap-coreboot.h   |  13 ++
 drivers/firmware/google/fmap_serialized.h |  59 ++++++++
 5 files changed, 237 insertions(+)
 create mode 100644 drivers/firmware/google/fmap-coreboot.c
 create mode 100644 drivers/firmware/google/fmap-coreboot.h
 create mode 100644 drivers/firmware/google/fmap_serialized.h

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index a3a6ca659ffa..5fbbd7b8fef6 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -74,4 +74,12 @@ config GOOGLE_VPD
 	  This option enables the kernel to expose the content of Google VPD
 	  under /sys/firmware/vpd.
 
+config GOOGLE_FMAP
+	tristate "Coreboot FMAP access"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to search for a Google FMAP in
+	  the coreboot table.  If found, this binary file is exported to userland
+	  in the file /sys/firmware/fmap.
+
 endif # GOOGLE_FIRMWARE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..6d31fe167700 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
 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
 
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/fmap-coreboot.c b/drivers/firmware/google/fmap-coreboot.c
new file mode 100644
index 000000000000..14050030ebc6
--- /dev/null
+++ b/drivers/firmware/google/fmap-coreboot.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * fmap-coreboot.c
+ *
+ * Exports the binary FMAP through coreboot table.
+ *
+ * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
+ * Copyright 2017 Google Inc.
+ * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com>
+ */
+
+#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/io.h>
+
+#include "coreboot_table.h"
+#include "fmap_serialized.h"
+
+#define CB_TAG_FMAP 0x37
+
+static void *fmap;
+static u32 fmap_size;
+
+/*
+ * Convert FMAP region to name.
+ * The caller has to free the string.
+ * Return NULL if no containing region was found.
+ */
+const char *coreboot_fmap_region_to_name(const u32 start, const u32 size)
+{
+	const char *name = NULL;
+	struct fmap *iter;
+	u32 size_old = ~0;
+	int i;
+
+	iter = fmap;
+	/* Find smallest containing region */
+	for (i = 0; i < iter->nareas && fmap; i++) {
+		if (iter->areas[i].offset <= start &&
+		    iter->areas[i].size >= size &&
+		    iter->areas[i].size <= size_old) {
+			size_old = iter->areas[i].size;
+			name = iter->areas[i].name;
+		}
+	}
+
+	if (name)
+		return kstrdup(name, GFP_KERNEL);
+	return NULL;
+}
+EXPORT_SYMBOL(coreboot_fmap_region_to_name);
+
+static ssize_t fmap_read(struct file *filp, struct kobject *kobp,
+			 struct bin_attribute *bin_attr, char *buf,
+			 loff_t pos, size_t count)
+{
+	if (!fmap)
+		return -ENODEV;
+
+	return memory_read_from_buffer(buf, count, &pos, fmap, fmap_size);
+}
+
+static int fmap_probe(struct coreboot_device *dev)
+{
+	struct lb_cbmem_ref *cbmem_ref = &dev->cbmem_ref;
+	struct fmap *header;
+
+	if (!cbmem_ref)
+		return -ENODEV;
+
+	header = memremap(cbmem_ref->cbmem_addr, sizeof(*header), MEMREMAP_WB);
+	if (!header) {
+		pr_warn("coreboot: Failed to remap FMAP\n");
+		return -ENOMEM;
+	}
+
+	/* Validate FMAP signature */
+	if (memcmp(header->signature, FMAP_SIGNATURE,
+		   sizeof(header->signature))) {
+		pr_warn("coreboot: FMAP signature mismatch\n");
+		memunmap(header);
+		return -ENODEV;
+	}
+
+	/* Validate FMAP version */
+	if (header->ver_major != FMAP_VER_MAJOR) {
+		pr_warn("coreboot: FMAP version not supported\n");
+		memunmap(header);
+		return -ENODEV;
+	}
+
+	pr_info("coreboot: Got valid FMAP v%u.%u for 0x%x byte ROM\n",
+		header->ver_major, header->ver_minor, header->size);
+
+	fmap_size = sizeof(*header) + header->nareas * sizeof(struct fmap_area);
+	memunmap(header);
+
+	fmap = devm_memremap(&dev->dev, cbmem_ref->cbmem_addr, fmap_size,
+			     MEMREMAP_WB);
+	if (!fmap) {
+		pr_warn("coreboot: Failed to remap FMAP\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int fmap_remove(struct coreboot_device *dev)
+{
+	struct platform_device *pdev = dev_get_drvdata(&dev->dev);
+
+	platform_device_unregister(pdev);
+
+	return 0;
+}
+
+static struct coreboot_driver fmap_driver = {
+	.probe = fmap_probe,
+	.remove = fmap_remove,
+	.drv = {
+		.name = "fmap",
+	},
+	.tag = CB_TAG_FMAP,
+};
+
+static struct bin_attribute fmap_bin_attr = {
+	.attr = {.name = "fmap", .mode = 0444},
+	.read = fmap_read,
+};
+
+static int __init coreboot_fmap_init(void)
+{
+	int err;
+
+	err = sysfs_create_bin_file(firmware_kobj, &fmap_bin_attr);
+	if (err)
+		return err;
+
+	return coreboot_driver_register(&fmap_driver);
+}
+
+static void coreboot_fmap_exit(void)
+{
+	coreboot_driver_unregister(&fmap_driver);
+	sysfs_remove_bin_file(firmware_kobj, &fmap_bin_attr);
+}
+
+module_init(coreboot_fmap_init);
+module_exit(coreboot_fmap_exit);
+
+MODULE_AUTHOR("9elements Agency GmbH");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/fmap-coreboot.h b/drivers/firmware/google/fmap-coreboot.h
new file mode 100644
index 000000000000..7107a01af0e3
--- /dev/null
+++ b/drivers/firmware/google/fmap-coreboot.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * bootmedia-coreboot.h
+ *
+ * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com>
+ */
+
+#ifndef __FMAP_COREBOOT_H
+#define __FMAP_COREBOOT_H
+
+const char *coreboot_fmap_region_to_name(const u32 start, const u32 size);
+
+#endif /* __FMAP_COREBOOT_H */
diff --git a/drivers/firmware/google/fmap_serialized.h b/drivers/firmware/google/fmap_serialized.h
new file mode 100644
index 000000000000..a001e47fa244
--- /dev/null
+++ b/drivers/firmware/google/fmap_serialized.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2010, Google Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *    * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *    * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ */
+
+#ifndef FLASHMAP_SERIALIZED_H__
+#define FLASHMAP_SERIALIZED_H__
+
+#define FMAP_SIGNATURE		"__FMAP__"
+#define FMAP_VER_MAJOR		1	/* this header's FMAP major version */
+#define FMAP_VER_MINOR		1	/* this header's FMAP minor version */
+#define FMAP_STRLEN		32	/* maximum length for strings,
+					 * including null-terminator
+					 */
+
+enum fmap_flags {
+	FMAP_AREA_STATIC	= 1 << 0,
+	FMAP_AREA_COMPRESSED	= 1 << 1,
+	FMAP_AREA_RO		= 1 << 2,
+	FMAP_AREA_PRESERVE	= 1 << 3,
+};
+
+/* Mapping of volatile and static regions in firmware binary */
+struct fmap_area {
+	u32 offset;                /* offset relative to base */
+	u32 size;                  /* size in bytes */
+	u8  name[FMAP_STRLEN];     /* descriptive name */
+	u16 flags;                 /* flags for this area */
+} __packed;
+
+struct fmap {
+	u8  signature[8];	/* "__FMAP__" (0x5F5F464D41505F5F) */
+	u8  ver_major;		/* major version */
+	u8  ver_minor;		/* minor version */
+	u64 base;		/* address of the firmware binary */
+	u32 size;		/* size of firmware binary in bytes */
+	u8  name[FMAP_STRLEN];	/* name of this firmware binary */
+	u16 nareas;		/* number of areas described by
+				 * fmap_areas[] below
+				 */
+	struct fmap_area areas[];
+} __packed;
+
+#endif	/* FLASHMAP_SERIALIZED_H__ */
-- 
2.21.0

[PATCH 2/2] firmware: coreboot: Export active CBFS partition

[patrick.rudolph]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

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 <patrick.rudolph@9elements.com>
---
 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 <dh.herrmann@gmail.com>
+ * Copyright 2017 Google Inc.
+ * Copyright 2019 Patrick Rudolph <patrick.rudolph@9elements.com>
+ */
+
+#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

Re: [PATCH 1/2] firmware: coreboot: Export the binary FMAP

[Greg Kroah-Hartman]

On Tue, Oct 08, 2019 at 01:53:25PM +0200, patrick.rudolph@9elements.com wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Expose coreboot's binary FMAP[1] to /sys/firmware/fmap.
> 
> coreboot copies the FMAP to a CBMEM buffer at boot since CB:35377[2],
> allowing an architecture independ way of exposing the FMAP to userspace.
> 
> Will be used by fwupd[3] to determine the current firmware layout.
> 
> [1]: https://doc.coreboot.org/lib/flashmap.html
> [2]: https://review.coreboot.org/c/coreboot/+/35377
> [3]: https://fwupd.org/
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  drivers/firmware/google/Kconfig           |   8 ++
>  drivers/firmware/google/Makefile          |   1 +
>  drivers/firmware/google/fmap-coreboot.c   | 156 ++++++++++++++++++++++
>  drivers/firmware/google/fmap-coreboot.h   |  13 ++
>  drivers/firmware/google/fmap_serialized.h |  59 ++++++++
>  5 files changed, 237 insertions(+)
>  create mode 100644 drivers/firmware/google/fmap-coreboot.c
>  create mode 100644 drivers/firmware/google/fmap-coreboot.h
>  create mode 100644 drivers/firmware/google/fmap_serialized.h
> 
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index a3a6ca659ffa..5fbbd7b8fef6 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -74,4 +74,12 @@ config GOOGLE_VPD
>  	  This option enables the kernel to expose the content of Google VPD
>  	  under /sys/firmware/vpd.
>  
> +config GOOGLE_FMAP
> +	tristate "Coreboot FMAP access"
> +	depends on GOOGLE_COREBOOT_TABLE
> +	help
> +	  This option enables the kernel to search for a Google FMAP in
> +	  the coreboot table.  If found, this binary file is exported to userland
> +	  in the file /sys/firmware/fmap.
> +
>  endif # GOOGLE_FIRMWARE
> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> index d17caded5d88..6d31fe167700 100644
> --- a/drivers/firmware/google/Makefile
> +++ b/drivers/firmware/google/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
>  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
>  
>  vpd-sysfs-y := vpd.o vpd_decode.o
>  obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
> diff --git a/drivers/firmware/google/fmap-coreboot.c b/drivers/firmware/google/fmap-coreboot.c
> new file mode 100644
> index 000000000000..14050030ebc6
> --- /dev/null
> +++ b/drivers/firmware/google/fmap-coreboot.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * fmap-coreboot.c
> + *
> + * Exports the binary FMAP through coreboot table.
> + *
> + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com>
> + */
> +
> +#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/io.h>
> +
> +#include "coreboot_table.h"
> +#include "fmap_serialized.h"
> +
> +#define CB_TAG_FMAP 0x37
> +
> +static void *fmap;
> +static u32 fmap_size;
> +
> +/*
> + * Convert FMAP region to name.
> + * The caller has to free the string.
> + * Return NULL if no containing region was found.
> + */
> +const char *coreboot_fmap_region_to_name(const u32 start, const u32 size)
> +{
> +	const char *name = NULL;
> +	struct fmap *iter;
> +	u32 size_old = ~0;
> +	int i;
> +
> +	iter = fmap;
> +	/* Find smallest containing region */
> +	for (i = 0; i < iter->nareas && fmap; i++) {
> +		if (iter->areas[i].offset <= start &&
> +		    iter->areas[i].size >= size &&
> +		    iter->areas[i].size <= size_old) {
> +			size_old = iter->areas[i].size;
> +			name = iter->areas[i].name;
> +		}
> +	}
> +
> +	if (name)
> +		return kstrdup(name, GFP_KERNEL);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(coreboot_fmap_region_to_name);
> +
> +static ssize_t fmap_read(struct file *filp, struct kobject *kobp,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t pos, size_t count)
> +{
> +	if (!fmap)
> +		return -ENODEV;
> +
> +	return memory_read_from_buffer(buf, count, &pos, fmap, fmap_size);
> +}
> +
> +static int fmap_probe(struct coreboot_device *dev)
> +{
> +	struct lb_cbmem_ref *cbmem_ref = &dev->cbmem_ref;
> +	struct fmap *header;
> +
> +	if (!cbmem_ref)
> +		return -ENODEV;
> +
> +	header = memremap(cbmem_ref->cbmem_addr, sizeof(*header), MEMREMAP_WB);
> +	if (!header) {
> +		pr_warn("coreboot: Failed to remap FMAP\n");

Doesn't memremap print an error if it fails?

> +		return -ENOMEM;
> +	}
> +
> +	/* Validate FMAP signature */
> +	if (memcmp(header->signature, FMAP_SIGNATURE,
> +		   sizeof(header->signature))) {
> +		pr_warn("coreboot: FMAP signature mismatch\n");

How can this happen?  Shouldn't it be an error?

> +		memunmap(header);
> +		return -ENODEV;
> +	}
> +
> +	/* Validate FMAP version */
> +	if (header->ver_major != FMAP_VER_MAJOR) {
> +		pr_warn("coreboot: FMAP version not supported\n");

error?

And why are these not dev_err() and friends?

> +		memunmap(header);
> +		return -ENODEV;
> +	}
> +
> +	pr_info("coreboot: Got valid FMAP v%u.%u for 0x%x byte ROM\n",
> +		header->ver_major, header->ver_minor, header->size);

Do not be noisy if all goes well.  This should be debugging only.


> +
> +	fmap_size = sizeof(*header) + header->nareas * sizeof(struct fmap_area);
> +	memunmap(header);
> +
> +	fmap = devm_memremap(&dev->dev, cbmem_ref->cbmem_addr, fmap_size,
> +			     MEMREMAP_WB);
> +	if (!fmap) {
> +		pr_warn("coreboot: Failed to remap FMAP\n");

Same here as above.

> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fmap_remove(struct coreboot_device *dev)
> +{
> +	struct platform_device *pdev = dev_get_drvdata(&dev->dev);
> +
> +	platform_device_unregister(pdev);
> +
> +	return 0;
> +}
> +
> +static struct coreboot_driver fmap_driver = {
> +	.probe = fmap_probe,
> +	.remove = fmap_remove,
> +	.drv = {
> +		.name = "fmap",
> +	},
> +	.tag = CB_TAG_FMAP,
> +};
> +
> +static struct bin_attribute fmap_bin_attr = {
> +	.attr = {.name = "fmap", .mode = 0444},
> +	.read = fmap_read,
> +};

BIN_ATTR_RO()?

And you forgot the Documentation/ABI/ update for your new sysfs file :(

I'm guessing all of these same issues are in your 2/2 patch, so I'll let
you fix them up and resend the series.

thanks,

greg k-h

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Stephen Boyd]

Quoting patrick.rudolph@9elements.com (2019-10-08 04:53:26)
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> 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 <patrick.rudolph@9elements.com>
> ---
>  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 <dh.herrmann@gmail.com>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 Patrick Rudolph <patrick.rudolph@9elements.com>
> + */
> +
> +#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;

Re: [PATCH 1/2] firmware: coreboot: Export the binary FMAP

[Stephen Boyd]

Quoting patrick.rudolph@9elements.com (2019-10-08 04:53:25)
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Expose coreboot's binary FMAP[1] to /sys/firmware/fmap.
> 
> coreboot copies the FMAP to a CBMEM buffer at boot since CB:35377[2],
> allowing an architecture independ way of exposing the FMAP to userspace.
> 
> Will be used by fwupd[3] to determine the current firmware layout.
> 
> [1]: https://doc.coreboot.org/lib/flashmap.html
> [2]: https://review.coreboot.org/c/coreboot/+/35377
> [3]: https://fwupd.org/
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  drivers/firmware/google/Kconfig           |   8 ++
>  drivers/firmware/google/Makefile          |   1 +
>  drivers/firmware/google/fmap-coreboot.c   | 156 ++++++++++++++++++++++
>  drivers/firmware/google/fmap-coreboot.h   |  13 ++
>  drivers/firmware/google/fmap_serialized.h |  59 ++++++++
>  5 files changed, 237 insertions(+)
>  create mode 100644 drivers/firmware/google/fmap-coreboot.c
>  create mode 100644 drivers/firmware/google/fmap-coreboot.h
>  create mode 100644 drivers/firmware/google/fmap_serialized.h
> 
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index a3a6ca659ffa..5fbbd7b8fef6 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -74,4 +74,12 @@ config GOOGLE_VPD
>           This option enables the kernel to expose the content of Google VPD
>           under /sys/firmware/vpd.
>  
> +config GOOGLE_FMAP
> +       tristate "Coreboot FMAP access"
> +       depends on GOOGLE_COREBOOT_TABLE
> +       help
> +         This option enables the kernel to search for a Google FMAP in
> +         the coreboot table.  If found, this binary file is exported to userland
> +         in the file /sys/firmware/fmap.
> +
>  endif # GOOGLE_FIRMWARE
> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> index d17caded5d88..6d31fe167700 100644
> --- a/drivers/firmware/google/Makefile
> +++ b/drivers/firmware/google/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
>  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
>  
>  vpd-sysfs-y := vpd.o vpd_decode.o
>  obj-$(CONFIG_GOOGLE_VPD)               += vpd-sysfs.o
> diff --git a/drivers/firmware/google/fmap-coreboot.c b/drivers/firmware/google/fmap-coreboot.c
> new file mode 100644
> index 000000000000..14050030ebc6
> --- /dev/null
> +++ b/drivers/firmware/google/fmap-coreboot.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * fmap-coreboot.c
> + *
> + * Exports the binary FMAP through coreboot table.
> + *
> + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com>
> + */
> +
> +#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/io.h>
> +
> +#include "coreboot_table.h"
> +#include "fmap_serialized.h"
> +
> +#define CB_TAG_FMAP 0x37
> +
> +static void *fmap;
> +static u32 fmap_size;
> +
> +/*
> + * Convert FMAP region to name.
> + * The caller has to free the string.
> + * Return NULL if no containing region was found.
> + */
> +const char *coreboot_fmap_region_to_name(const u32 start, const u32 size)
> +{
> +       const char *name = NULL;
> +       struct fmap *iter;
> +       u32 size_old = ~0;
> +       int i;
> +
> +       iter = fmap;
> +       /* Find smallest containing region */
> +       for (i = 0; i < iter->nareas && fmap; i++) {
> +               if (iter->areas[i].offset <= start &&
> +                   iter->areas[i].size >= size &&
> +                   iter->areas[i].size <= size_old) {
> +                       size_old = iter->areas[i].size;
> +                       name = iter->areas[i].name;
> +               }
> +       }
> +
> +       if (name)
> +               return kstrdup(name, GFP_KERNEL);
> +       return NULL;
> +}
> +EXPORT_SYMBOL(coreboot_fmap_region_to_name);
> +
> +static ssize_t fmap_read(struct file *filp, struct kobject *kobp,
> +                        struct bin_attribute *bin_attr, char *buf,
> +                        loff_t pos, size_t count)
> +{
> +       if (!fmap)
> +               return -ENODEV;
> +
> +       return memory_read_from_buffer(buf, count, &pos, fmap, fmap_size);
> +}
> +
> +static int fmap_probe(struct coreboot_device *dev)
> +{
> +       struct lb_cbmem_ref *cbmem_ref = &dev->cbmem_ref;
> +       struct fmap *header;
> +
> +       if (!cbmem_ref)
> +               return -ENODEV;

This is impossible.

> +
> +       header = memremap(cbmem_ref->cbmem_addr, sizeof(*header), MEMREMAP_WB);
> +       if (!header) {
> +               pr_warn("coreboot: Failed to remap FMAP\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* Validate FMAP signature */
> +       if (memcmp(header->signature, FMAP_SIGNATURE,
> +                  sizeof(header->signature))) {
> +               pr_warn("coreboot: FMAP signature mismatch\n");
> +               memunmap(header);
> +               return -ENODEV;
> +       }
> +
> +       /* Validate FMAP version */
> +       if (header->ver_major != FMAP_VER_MAJOR) {
> +               pr_warn("coreboot: FMAP version not supported\n");
> +               memunmap(header);
> +               return -ENODEV;
> +       }
> +
> +       pr_info("coreboot: Got valid FMAP v%u.%u for 0x%x byte ROM\n",
> +               header->ver_major, header->ver_minor, header->size);
> +
> +       fmap_size = sizeof(*header) + header->nareas * sizeof(struct fmap_area);
> +       memunmap(header);
> +
> +       fmap = devm_memremap(&dev->dev, cbmem_ref->cbmem_addr, fmap_size,
> +                            MEMREMAP_WB);
> +       if (!fmap) {
> +               pr_warn("coreboot: Failed to remap FMAP\n");
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +static int fmap_remove(struct coreboot_device *dev)
> +{
> +       struct platform_device *pdev = dev_get_drvdata(&dev->dev);
> +
> +       platform_device_unregister(pdev);
> +
> +       return 0;
> +}
> +
> +static struct coreboot_driver fmap_driver = {
> +       .probe = fmap_probe,
> +       .remove = fmap_remove,
> +       .drv = {
> +               .name = "fmap",
> +       },
> +       .tag = CB_TAG_FMAP,
> +};
> +
> +static struct bin_attribute fmap_bin_attr = {
> +       .attr = {.name = "fmap", .mode = 0444},
> +       .read = fmap_read,
> +};
> +
> +static int __init coreboot_fmap_init(void)
> +{
> +       int err;
> +
> +       err = sysfs_create_bin_file(firmware_kobj, &fmap_bin_attr);
> +       if (err)
> +               return err;
> +
> +       return coreboot_driver_register(&fmap_driver);
> +}
> +
> +static void coreboot_fmap_exit(void)
> +{
> +       coreboot_driver_unregister(&fmap_driver);
> +       sysfs_remove_bin_file(firmware_kobj, &fmap_bin_attr);
> +}
> +
> +module_init(coreboot_fmap_init);
> +module_exit(coreboot_fmap_exit);
> +
> +MODULE_AUTHOR("9elements Agency GmbH");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/google/fmap-coreboot.h b/drivers/firmware/google/fmap-coreboot.h
> new file mode 100644
> index 000000000000..7107a01af0e3
> --- /dev/null
> +++ b/drivers/firmware/google/fmap-coreboot.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * bootmedia-coreboot.h
> + *
> + * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com>
> + */
> +
> +#ifndef __FMAP_COREBOOT_H
> +#define __FMAP_COREBOOT_H
> +
> +const char *coreboot_fmap_region_to_name(const u32 start, const u32 size);
> +
> +#endif /* __FMAP_COREBOOT_H */
> diff --git a/drivers/firmware/google/fmap_serialized.h b/drivers/firmware/google/fmap_serialized.h
> new file mode 100644
> index 000000000000..a001e47fa244
> --- /dev/null
> +++ b/drivers/firmware/google/fmap_serialized.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2010, Google Inc.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *    * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *    * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *    * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + */
> +
> +#ifndef FLASHMAP_SERIALIZED_H__
> +#define FLASHMAP_SERIALIZED_H__
> +
> +#define FMAP_SIGNATURE         "__FMAP__"
> +#define FMAP_VER_MAJOR         1       /* this header's FMAP major version */
> +#define FMAP_VER_MINOR         1       /* this header's FMAP minor version */
> +#define FMAP_STRLEN            32      /* maximum length for strings,
> +                                        * including null-terminator
> +                                        */
> +
> +enum fmap_flags {
> +       FMAP_AREA_STATIC        = 1 << 0,
> +       FMAP_AREA_COMPRESSED    = 1 << 1,
> +       FMAP_AREA_RO            = 1 << 2,
> +       FMAP_AREA_PRESERVE      = 1 << 3,
> +};
> +
> +/* Mapping of volatile and static regions in firmware binary */
> +struct fmap_area {
> +       u32 offset;                /* offset relative to base */
> +       u32 size;                  /* size in bytes */
> +       u8  name[FMAP_STRLEN];     /* descriptive name */
> +       u16 flags;                 /* flags for this area */
> +} __packed;
> +
> +struct fmap {
> +       u8  signature[8];       /* "__FMAP__" (0x5F5F464D41505F5F) */
> +       u8  ver_major;          /* major version */
> +       u8  ver_minor;          /* minor version */
> +       u64 base;               /* address of the firmware binary */
> +       u32 size;               /* size of firmware binary in bytes */
> +       u8  name[FMAP_STRLEN];  /* name of this firmware binary */
> +       u16 nareas;             /* number of areas described by
> +                                * fmap_areas[] below
> +                                */
> +       struct fmap_area areas[];
> +} __packed;
> +
> +#endif /* FLASHMAP_SERIALIZED_H__ */
> -- 
> 2.21.0
> 

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

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

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[patrick.rudolph]

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.

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Stephen Boyd]

-Filipe bounce

Quoting patrick.rudolph@9elements.com (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?

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

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

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Samuel Holland]

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

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Stephen Boyd]

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

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

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