[PATCH v5] firmware: google: Implement cbmem in sysfs driver

[PATCH v5] firmware: google: Implement cbmem in sysfs driver

[Jack Rosenthal]

cbmem entries can be read from coreboot table
0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
entries in sysfs under /sys/firmware/coreboot/cbmem-*.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Reported-by: kernel test robot <lkp@intel.com>
Tested-by: Jack Rosenthal <jrosenth@chromium.org>
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
---
v5: Corrected documentation warnings reported by kernel test robot.
 .../ABI/testing/sysfs-firmware-coreboot       |  17 ++
 drivers/firmware/google/Kconfig               |   8 +
 drivers/firmware/google/Makefile              |   3 +
 drivers/firmware/google/cbmem.c               | 232 ++++++++++++++++++
 drivers/firmware/google/coreboot_table.h      |  11 +
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
 create mode 100644 drivers/firmware/google/cbmem.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
new file mode 100644
index 000000000000..2401483bb86c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,17 @@
+What:		/sys/firmware/coreboot/
+Date:		July 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		Coreboot-based BIOS firmware provides a variety of information
+		in CBMEM.  Each CBMEM entry can be found via Coreboot tables.
+		For each CBMEM entry, the following are exposed:
+
+		======= =======================================================
+		address A hexidecimal value of the memory address the data for
+			the entry begins at.
+		size	The size of the data stored.
+		id	The id corresponding to the entry. A list of ids known
+			to coreboot can be found in the coreboot source tree at
+			``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``
+		mem	A file exposing the raw memory for the entry.
+		======= =======================================================
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..bf8316d1cb31 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,6 +19,14 @@ config GOOGLE_SMI
 	  driver provides an interface for reading and writing NVRAM
 	  variables.
 
+config GOOGLE_CBMEM
+	tristate "CBMEM entries in sysfs"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to search for Coreboot CBMEM
+	  entries, and expose the memory for each entry in sysfs under
+	  /sys/firmware/coreboot.
+
 config GOOGLE_COREBOOT_TABLE
 	tristate "Coreboot Table Access"
 	depends on HAS_IOMEM && (ACPI || OF)
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..8151e323cc43 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
 
+# Must come after coreboot_table.o, as this driver depends on that bus type.
+obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
+
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
new file mode 100644
index 000000000000..9646a8047742
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "coreboot_table.h"
+
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+static struct kobject *coreboot_kobj;
+
+struct cbmem_entry;
+struct cbmem_entry_attr {
+	struct kobj_attribute kobj_attr;
+	struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+	char *kobj_name;
+	struct kobject *kobj;
+	struct coreboot_device *dev;
+	struct bin_attribute mem_file;
+	char *mem_file_buf;
+	struct cbmem_entry_attr address_file;
+	struct cbmem_entry_attr size_file;
+	struct cbmem_entry_attr id_file;
+};
+
+static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
+{
+	return container_of(a, struct cbmem_entry_attr, kobj_attr);
+}
+
+static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
+				    struct bin_attribute *bin_attr, char *buf,
+				    loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+				       bin_attr->size);
+}
+
+static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
+				     struct bin_attribute *bin_attr, char *buf,
+				     loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	if (pos < 0 || pos >= bin_attr->size)
+		return -EINVAL;
+	if (count > bin_attr->size - pos)
+		count = bin_attr->size - pos;
+
+	memcpy(entry->mem_file_buf + pos, buf, count);
+	return count;
+}
+
+static ssize_t cbmem_entry_address_show(struct kobject *kobj,
+					struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%llx\n",
+			  entry_attr->entry->dev->cbmem_entry.address);
+}
+
+static ssize_t cbmem_entry_size_show(struct kobject *kobj,
+				     struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.entry_size);
+}
+
+static ssize_t cbmem_entry_id_show(struct kobject *kobj,
+				   struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_setup(struct cbmem_entry *entry)
+{
+	int ret;
+
+	entry->mem_file_buf =
+		devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
+			      entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);
+	if (!entry->mem_file_buf)
+		return -ENOMEM;
+
+	entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
+					  "cbmem-%08x",
+					  entry->dev->cbmem_entry.id);
+	if (!entry->kobj_name)
+		return -ENOMEM;
+
+	entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
+	if (!entry->kobj)
+		return -ENOMEM;
+
+	sysfs_bin_attr_init(&entry->mem_file);
+	entry->mem_file.attr.name = "mem";
+	entry->mem_file.attr.mode = 0664;
+	entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
+	entry->mem_file.read = cbmem_entry_mem_read;
+	entry->mem_file.write = cbmem_entry_mem_write;
+	entry->mem_file.private = entry;
+	ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
+	if (ret)
+		goto free_kobj;
+
+	sysfs_attr_init(&entry->address_file.kobj_attr.attr);
+	entry->address_file.kobj_attr.attr.name = "address";
+	entry->address_file.kobj_attr.attr.mode = 0444;
+	entry->address_file.kobj_attr.show = cbmem_entry_address_show;
+	entry->address_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj,
+				&entry->address_file.kobj_attr.attr);
+	if (ret)
+		goto free_mem_file;
+
+	sysfs_attr_init(&entry->size_file.kobj_attr.attr);
+	entry->size_file.kobj_attr.attr.name = "size";
+	entry->size_file.kobj_attr.attr.mode = 0444;
+	entry->size_file.kobj_attr.show = cbmem_entry_size_show;
+	entry->size_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	if (ret)
+		goto free_address_file;
+
+	sysfs_attr_init(&entry->id_file.kobj_attr.attr);
+	entry->id_file.kobj_attr.attr.name = "id";
+	entry->id_file.kobj_attr.attr.mode = 0444;
+	entry->id_file.kobj_attr.show = cbmem_entry_id_show;
+	entry->id_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	if (ret)
+		goto free_size_file;
+
+	return 0;
+
+free_size_file:
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+free_address_file:
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+free_mem_file:
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+free_kobj:
+	kobject_put(entry->kobj);
+	return ret;
+}
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry;
+
+	entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dev->dev, entry);
+	entry->dev = dev;
+	return cbmem_entry_setup(entry);
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	kobject_put(entry->kobj);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+	.probe = cbmem_entry_probe,
+	.remove = cbmem_entry_remove,
+	.drv = {
+		.name = "cbmem",
+	},
+	.tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+	int ret;
+
+	coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+	if (!coreboot_kobj)
+		return -ENOMEM;
+
+	ret = coreboot_driver_register(&cbmem_entry_driver);
+	if (ret) {
+		kobject_put(coreboot_kobj);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+	kobject_put(coreboot_kobj);
+	coreboot_driver_unregister(&cbmem_entry_driver);
+}
+module_exit(cbmem_exit);
+
+MODULE_AUTHOR("Jack Rosenthal <jrosenth@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..6c03a8852d1b 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -39,6 +39,16 @@ struct lb_cbmem_ref {
 	u64 cbmem_addr;
 };
 
+/* Corresponds to LB_TAG_CBMEM_ENTRY. */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -65,6 +75,7 @@ struct coreboot_device {
 	union {
 		struct coreboot_table_entry entry;
 		struct lb_cbmem_ref cbmem_ref;
+		struct lb_cbmem_entry cbmem_entry;
 		struct lb_framebuffer framebuffer;
 	};
 };
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v5] firmware: google: Implement cbmem in sysfs driver

[Julius Werner]

Reviewed-by: Julius Werner <jwerner@chromium.org>

[PATCH v6] firmware: google: Implement cbmem in sysfs driver

[Jack Rosenthal]

cbmem entries can be read from coreboot table
0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
entries in sysfs under /sys/firmware/coreboot/cbmem-*.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Reported-by: kernel test robot <lkp@intel.com>
Tested-by: Jack Rosenthal <jrosenth@chromium.org>
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
---
v6: Modified commit message.
 .../ABI/testing/sysfs-firmware-coreboot       |  17 ++
 drivers/firmware/google/Kconfig               |   8 +
 drivers/firmware/google/Makefile              |   3 +
 drivers/firmware/google/cbmem.c               | 232 ++++++++++++++++++
 drivers/firmware/google/coreboot_table.h      |  11 +
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
 create mode 100644 drivers/firmware/google/cbmem.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
new file mode 100644
index 000000000000..2401483bb86c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,17 @@
+What:		/sys/firmware/coreboot/
+Date:		July 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		Coreboot-based BIOS firmware provides a variety of information
+		in CBMEM.  Each CBMEM entry can be found via Coreboot tables.
+		For each CBMEM entry, the following are exposed:
+
+		======= =======================================================
+		address A hexidecimal value of the memory address the data for
+			the entry begins at.
+		size	The size of the data stored.
+		id	The id corresponding to the entry. A list of ids known
+			to coreboot can be found in the coreboot source tree at
+			``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``
+		mem	A file exposing the raw memory for the entry.
+		======= =======================================================
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..bf8316d1cb31 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,6 +19,14 @@ config GOOGLE_SMI
 	  driver provides an interface for reading and writing NVRAM
 	  variables.
 
+config GOOGLE_CBMEM
+	tristate "CBMEM entries in sysfs"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to search for Coreboot CBMEM
+	  entries, and expose the memory for each entry in sysfs under
+	  /sys/firmware/coreboot.
+
 config GOOGLE_COREBOOT_TABLE
 	tristate "Coreboot Table Access"
 	depends on HAS_IOMEM && (ACPI || OF)
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..8151e323cc43 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
 
+# Must come after coreboot_table.o, as this driver depends on that bus type.
+obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
+
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
new file mode 100644
index 000000000000..9646a8047742
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "coreboot_table.h"
+
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+static struct kobject *coreboot_kobj;
+
+struct cbmem_entry;
+struct cbmem_entry_attr {
+	struct kobj_attribute kobj_attr;
+	struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+	char *kobj_name;
+	struct kobject *kobj;
+	struct coreboot_device *dev;
+	struct bin_attribute mem_file;
+	char *mem_file_buf;
+	struct cbmem_entry_attr address_file;
+	struct cbmem_entry_attr size_file;
+	struct cbmem_entry_attr id_file;
+};
+
+static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
+{
+	return container_of(a, struct cbmem_entry_attr, kobj_attr);
+}
+
+static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
+				    struct bin_attribute *bin_attr, char *buf,
+				    loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+				       bin_attr->size);
+}
+
+static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
+				     struct bin_attribute *bin_attr, char *buf,
+				     loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	if (pos < 0 || pos >= bin_attr->size)
+		return -EINVAL;
+	if (count > bin_attr->size - pos)
+		count = bin_attr->size - pos;
+
+	memcpy(entry->mem_file_buf + pos, buf, count);
+	return count;
+}
+
+static ssize_t cbmem_entry_address_show(struct kobject *kobj,
+					struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%llx\n",
+			  entry_attr->entry->dev->cbmem_entry.address);
+}
+
+static ssize_t cbmem_entry_size_show(struct kobject *kobj,
+				     struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.entry_size);
+}
+
+static ssize_t cbmem_entry_id_show(struct kobject *kobj,
+				   struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_setup(struct cbmem_entry *entry)
+{
+	int ret;
+
+	entry->mem_file_buf =
+		devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
+			      entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);
+	if (!entry->mem_file_buf)
+		return -ENOMEM;
+
+	entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
+					  "cbmem-%08x",
+					  entry->dev->cbmem_entry.id);
+	if (!entry->kobj_name)
+		return -ENOMEM;
+
+	entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
+	if (!entry->kobj)
+		return -ENOMEM;
+
+	sysfs_bin_attr_init(&entry->mem_file);
+	entry->mem_file.attr.name = "mem";
+	entry->mem_file.attr.mode = 0664;
+	entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
+	entry->mem_file.read = cbmem_entry_mem_read;
+	entry->mem_file.write = cbmem_entry_mem_write;
+	entry->mem_file.private = entry;
+	ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
+	if (ret)
+		goto free_kobj;
+
+	sysfs_attr_init(&entry->address_file.kobj_attr.attr);
+	entry->address_file.kobj_attr.attr.name = "address";
+	entry->address_file.kobj_attr.attr.mode = 0444;
+	entry->address_file.kobj_attr.show = cbmem_entry_address_show;
+	entry->address_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj,
+				&entry->address_file.kobj_attr.attr);
+	if (ret)
+		goto free_mem_file;
+
+	sysfs_attr_init(&entry->size_file.kobj_attr.attr);
+	entry->size_file.kobj_attr.attr.name = "size";
+	entry->size_file.kobj_attr.attr.mode = 0444;
+	entry->size_file.kobj_attr.show = cbmem_entry_size_show;
+	entry->size_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	if (ret)
+		goto free_address_file;
+
+	sysfs_attr_init(&entry->id_file.kobj_attr.attr);
+	entry->id_file.kobj_attr.attr.name = "id";
+	entry->id_file.kobj_attr.attr.mode = 0444;
+	entry->id_file.kobj_attr.show = cbmem_entry_id_show;
+	entry->id_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	if (ret)
+		goto free_size_file;
+
+	return 0;
+
+free_size_file:
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+free_address_file:
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+free_mem_file:
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+free_kobj:
+	kobject_put(entry->kobj);
+	return ret;
+}
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry;
+
+	entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dev->dev, entry);
+	entry->dev = dev;
+	return cbmem_entry_setup(entry);
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	kobject_put(entry->kobj);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+	.probe = cbmem_entry_probe,
+	.remove = cbmem_entry_remove,
+	.drv = {
+		.name = "cbmem",
+	},
+	.tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+	int ret;
+
+	coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+	if (!coreboot_kobj)
+		return -ENOMEM;
+
+	ret = coreboot_driver_register(&cbmem_entry_driver);
+	if (ret) {
+		kobject_put(coreboot_kobj);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+	kobject_put(coreboot_kobj);
+	coreboot_driver_unregister(&cbmem_entry_driver);
+}
+module_exit(cbmem_exit);
+
+MODULE_AUTHOR("Jack Rosenthal <jrosenth@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..6c03a8852d1b 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -39,6 +39,16 @@ struct lb_cbmem_ref {
 	u64 cbmem_addr;
 };
 
+/* Corresponds to LB_TAG_CBMEM_ENTRY. */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -65,6 +75,7 @@ struct coreboot_device {
 	union {
 		struct coreboot_table_entry entry;
 		struct lb_cbmem_ref cbmem_ref;
+		struct lb_cbmem_entry cbmem_entry;
 		struct lb_framebuffer framebuffer;
 	};
 };
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH v6] firmware: google: Implement cbmem in sysfs driver

[Guenter Roeck]

On Tue, Aug 2, 2022 at 7:04 AM Jack Rosenthal <jrosenth@chromium.org> wrote:
>
> cbmem entries can be read from coreboot table
> 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> entries in sysfs under /sys/firmware/coreboot/cbmem-*.
>
> Link: https://issuetracker.google.com/239604743
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Reviewed-by: Julius Werner <jwerner@chromium.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Tested-by: Jack Rosenthal <jrosenth@chromium.org>
> Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> v6: Modified commit message.
>  .../ABI/testing/sysfs-firmware-coreboot       |  17 ++
>  drivers/firmware/google/Kconfig               |   8 +
>  drivers/firmware/google/Makefile              |   3 +
>  drivers/firmware/google/cbmem.c               | 232 ++++++++++++++++++
>  drivers/firmware/google/coreboot_table.h      |  11 +
>  5 files changed, 271 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
>  create mode 100644 drivers/firmware/google/cbmem.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
> new file mode 100644
> index 000000000000..2401483bb86c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
> @@ -0,0 +1,17 @@
> +What:          /sys/firmware/coreboot/
> +Date:          July 2022
> +Contact:       Jack Rosenthal <jrosenth@chromium.org>
> +Description:
> +               Coreboot-based BIOS firmware provides a variety of information
> +               in CBMEM.  Each CBMEM entry can be found via Coreboot tables.
> +               For each CBMEM entry, the following are exposed:
> +
> +               ======= =======================================================
> +               address A hexidecimal value of the memory address the data for
> +                       the entry begins at.
> +               size    The size of the data stored.
> +               id      The id corresponding to the entry. A list of ids known
> +                       to coreboot can be found in the coreboot source tree at
> +                       ``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``
> +               mem     A file exposing the raw memory for the entry.
> +               ======= =======================================================
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 983e07dc022e..bf8316d1cb31 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -19,6 +19,14 @@ config GOOGLE_SMI
>           driver provides an interface for reading and writing NVRAM
>           variables.
>
> +config GOOGLE_CBMEM
> +       tristate "CBMEM entries in sysfs"
> +       depends on GOOGLE_COREBOOT_TABLE
> +       help
> +         This option enables the kernel to search for Coreboot CBMEM
> +         entries, and expose the memory for each entry in sysfs under
> +         /sys/firmware/coreboot.
> +
>  config GOOGLE_COREBOOT_TABLE
>         tristate "Coreboot Table Access"
>         depends on HAS_IOMEM && (ACPI || OF)
> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> index d17caded5d88..8151e323cc43 100644
> --- a/drivers/firmware/google/Makefile
> +++ b/drivers/firmware/google/Makefile
> @@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
>  obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
>  obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
>
> +# Must come after coreboot_table.o, as this driver depends on that bus type.
> +obj-$(CONFIG_GOOGLE_CBMEM)             += cbmem.o
> +
>  vpd-sysfs-y := vpd.o vpd_decode.o
>  obj-$(CONFIG_GOOGLE_VPD)               += vpd-sysfs.o
> diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
> new file mode 100644
> index 000000000000..9646a8047742
> --- /dev/null
> +++ b/drivers/firmware/google/cbmem.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * cbmem.c
> + *
> + * Driver for exporting cbmem entries in sysfs.
> + *
> + * Copyright 2022 Google LLC
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#include "coreboot_table.h"
> +
> +#define LB_TAG_CBMEM_ENTRY 0x31
> +
> +static struct kobject *coreboot_kobj;
> +
> +struct cbmem_entry;
> +struct cbmem_entry_attr {
> +       struct kobj_attribute kobj_attr;
> +       struct cbmem_entry *entry;
> +};
> +
> +struct cbmem_entry {
> +       char *kobj_name;
> +       struct kobject *kobj;
> +       struct coreboot_device *dev;
> +       struct bin_attribute mem_file;
> +       char *mem_file_buf;
> +       struct cbmem_entry_attr address_file;
> +       struct cbmem_entry_attr size_file;
> +       struct cbmem_entry_attr id_file;
> +};
> +
> +static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
> +{
> +       return container_of(a, struct cbmem_entry_attr, kobj_attr);
> +}
> +
> +static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
> +                                   struct bin_attribute *bin_attr, char *buf,
> +                                   loff_t pos, size_t count)
> +{
> +       struct cbmem_entry *entry = bin_attr->private;
> +
> +       return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
> +                                      bin_attr->size);
> +}
> +
> +static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
> +                                    struct bin_attribute *bin_attr, char *buf,
> +                                    loff_t pos, size_t count)
> +{
> +       struct cbmem_entry *entry = bin_attr->private;
> +
> +       if (pos < 0 || pos >= bin_attr->size)
> +               return -EINVAL;
> +       if (count > bin_attr->size - pos)
> +               count = bin_attr->size - pos;
> +
> +       memcpy(entry->mem_file_buf + pos, buf, count);
> +       return count;
> +}
> +
> +static ssize_t cbmem_entry_address_show(struct kobject *kobj,
> +                                       struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%llx\n",
> +                         entry_attr->entry->dev->cbmem_entry.address);
> +}
> +
> +static ssize_t cbmem_entry_size_show(struct kobject *kobj,
> +                                    struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%x\n",
> +                         entry_attr->entry->dev->cbmem_entry.entry_size);
> +}
> +
> +static ssize_t cbmem_entry_id_show(struct kobject *kobj,
> +                                  struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%x\n",
> +                         entry_attr->entry->dev->cbmem_entry.id);
> +}
> +
> +static int cbmem_entry_setup(struct cbmem_entry *entry)
> +{
> +       int ret;
> +
> +       entry->mem_file_buf =
> +               devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
> +                             entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);
> +       if (!entry->mem_file_buf)
> +               return -ENOMEM;
> +
> +       entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
> +                                         "cbmem-%08x",
> +                                         entry->dev->cbmem_entry.id);
> +       if (!entry->kobj_name)
> +               return -ENOMEM;
> +
> +       entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
> +       if (!entry->kobj)
> +               return -ENOMEM;
> +
> +       sysfs_bin_attr_init(&entry->mem_file);
> +       entry->mem_file.attr.name = "mem";
> +       entry->mem_file.attr.mode = 0664;
> +       entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
> +       entry->mem_file.read = cbmem_entry_mem_read;
> +       entry->mem_file.write = cbmem_entry_mem_write;
> +       entry->mem_file.private = entry;
> +       ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
> +       if (ret)
> +               goto free_kobj;
> +
> +       sysfs_attr_init(&entry->address_file.kobj_attr.attr);
> +       entry->address_file.kobj_attr.attr.name = "address";
> +       entry->address_file.kobj_attr.attr.mode = 0444;
> +       entry->address_file.kobj_attr.show = cbmem_entry_address_show;
> +       entry->address_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj,
> +                               &entry->address_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_mem_file;
> +
> +       sysfs_attr_init(&entry->size_file.kobj_attr.attr);
> +       entry->size_file.kobj_attr.attr.name = "size";
> +       entry->size_file.kobj_attr.attr.mode = 0444;
> +       entry->size_file.kobj_attr.show = cbmem_entry_size_show;
> +       entry->size_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_address_file;
> +
> +       sysfs_attr_init(&entry->id_file.kobj_attr.attr);
> +       entry->id_file.kobj_attr.attr.name = "id";
> +       entry->id_file.kobj_attr.attr.mode = 0444;
> +       entry->id_file.kobj_attr.show = cbmem_entry_id_show;
> +       entry->id_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_size_file;
> +
> +       return 0;
> +
> +free_size_file:
> +       sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +free_address_file:
> +       sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
> +free_mem_file:
> +       sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
> +free_kobj:
> +       kobject_put(entry->kobj);
> +       return ret;
> +}
> +
> +static int cbmem_entry_probe(struct coreboot_device *dev)
> +{
> +       struct cbmem_entry *entry;
> +
> +       entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(&dev->dev, entry);
> +       entry->dev = dev;
> +       return cbmem_entry_setup(entry);
> +}
> +
> +static void cbmem_entry_remove(struct coreboot_device *dev)
> +{
> +       struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
> +
> +       sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
> +       sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
> +       sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +       sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
> +       kobject_put(entry->kobj);
> +}
> +
> +static struct coreboot_driver cbmem_entry_driver = {
> +       .probe = cbmem_entry_probe,
> +       .remove = cbmem_entry_remove,
> +       .drv = {
> +               .name = "cbmem",
> +       },
> +       .tag = LB_TAG_CBMEM_ENTRY,
> +};
> +
> +static int __init cbmem_init(void)
> +{
> +       int ret;
> +
> +       coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
> +       if (!coreboot_kobj)
> +               return -ENOMEM;
> +
> +       ret = coreboot_driver_register(&cbmem_entry_driver);
> +       if (ret) {
> +               kobject_put(coreboot_kobj);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +module_init(cbmem_init);
> +
> +static void __exit cbmem_exit(void)
> +{
> +       kobject_put(coreboot_kobj);
> +       coreboot_driver_unregister(&cbmem_entry_driver);
> +}
> +module_exit(cbmem_exit);
> +
> +MODULE_AUTHOR("Jack Rosenthal <jrosenth@chromium.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index beb778674acd..6c03a8852d1b 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -39,6 +39,16 @@ struct lb_cbmem_ref {
>         u64 cbmem_addr;
>  };
>
> +/* Corresponds to LB_TAG_CBMEM_ENTRY. */
> +struct lb_cbmem_entry {
> +       u32 tag;
> +       u32 size;
> +
> +       u64 address;
> +       u32 entry_size;
> +       u32 id;
> +};
> +
>  /* Describes framebuffer setup by coreboot */
>  struct lb_framebuffer {
>         u32 tag;
> @@ -65,6 +75,7 @@ struct coreboot_device {
>         union {
>                 struct coreboot_table_entry entry;
>                 struct lb_cbmem_ref cbmem_ref;
> +               struct lb_cbmem_entry cbmem_entry;
>                 struct lb_framebuffer framebuffer;
>         };
>  };
> --
> 2.37.1.455.g008518b4e5-goog
>

Re: [PATCH v6] firmware: google: Implement cbmem in sysfs driver

[Tzung-Bi Shih]

On Tue, Aug 02, 2022 at 07:03:43AM -0700, Jack Rosenthal wrote:
> cbmem entries can be read from coreboot table
> 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> entries in sysfs under /sys/firmware/coreboot/cbmem-*.
> 
> Link: https://issuetracker.google.com/239604743

I doubt if the Link tag would be useful for public.

[...]

> Reported-by: kernel test robot <lkp@intel.com>

Suggest to drop the Reported-by tag even if the robot reported some compile
errors during the development.  Given that the patch isn't a fixup but
supporting something new (which isn't "reported by" the robot).

[PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Jack Rosenthal]

cbmem entries can be read from coreboot table
0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
entries in sysfs under /sys/firmware/coreboot/cbmem-*.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Tested-by: Jack Rosenthal <jrosenth@chromium.org>
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
---
v7: Updated commit message.
 .../ABI/testing/sysfs-firmware-coreboot       |  17 ++
 drivers/firmware/google/Kconfig               |   8 +
 drivers/firmware/google/Makefile              |   3 +
 drivers/firmware/google/cbmem.c               | 232 ++++++++++++++++++
 drivers/firmware/google/coreboot_table.h      |  11 +
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
 create mode 100644 drivers/firmware/google/cbmem.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
new file mode 100644
index 000000000000..2401483bb86c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,17 @@
+What:		/sys/firmware/coreboot/
+Date:		July 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		Coreboot-based BIOS firmware provides a variety of information
+		in CBMEM.  Each CBMEM entry can be found via Coreboot tables.
+		For each CBMEM entry, the following are exposed:
+
+		======= =======================================================
+		address A hexidecimal value of the memory address the data for
+			the entry begins at.
+		size	The size of the data stored.
+		id	The id corresponding to the entry. A list of ids known
+			to coreboot can be found in the coreboot source tree at
+			``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``
+		mem	A file exposing the raw memory for the entry.
+		======= =======================================================
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..bf8316d1cb31 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,6 +19,14 @@ config GOOGLE_SMI
 	  driver provides an interface for reading and writing NVRAM
 	  variables.
 
+config GOOGLE_CBMEM
+	tristate "CBMEM entries in sysfs"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to search for Coreboot CBMEM
+	  entries, and expose the memory for each entry in sysfs under
+	  /sys/firmware/coreboot.
+
 config GOOGLE_COREBOOT_TABLE
 	tristate "Coreboot Table Access"
 	depends on HAS_IOMEM && (ACPI || OF)
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..8151e323cc43 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
 
+# Must come after coreboot_table.o, as this driver depends on that bus type.
+obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
+
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
new file mode 100644
index 000000000000..9646a8047742
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "coreboot_table.h"
+
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+static struct kobject *coreboot_kobj;
+
+struct cbmem_entry;
+struct cbmem_entry_attr {
+	struct kobj_attribute kobj_attr;
+	struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+	char *kobj_name;
+	struct kobject *kobj;
+	struct coreboot_device *dev;
+	struct bin_attribute mem_file;
+	char *mem_file_buf;
+	struct cbmem_entry_attr address_file;
+	struct cbmem_entry_attr size_file;
+	struct cbmem_entry_attr id_file;
+};
+
+static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
+{
+	return container_of(a, struct cbmem_entry_attr, kobj_attr);
+}
+
+static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
+				    struct bin_attribute *bin_attr, char *buf,
+				    loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+				       bin_attr->size);
+}
+
+static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
+				     struct bin_attribute *bin_attr, char *buf,
+				     loff_t pos, size_t count)
+{
+	struct cbmem_entry *entry = bin_attr->private;
+
+	if (pos < 0 || pos >= bin_attr->size)
+		return -EINVAL;
+	if (count > bin_attr->size - pos)
+		count = bin_attr->size - pos;
+
+	memcpy(entry->mem_file_buf + pos, buf, count);
+	return count;
+}
+
+static ssize_t cbmem_entry_address_show(struct kobject *kobj,
+					struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%llx\n",
+			  entry_attr->entry->dev->cbmem_entry.address);
+}
+
+static ssize_t cbmem_entry_size_show(struct kobject *kobj,
+				     struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.entry_size);
+}
+
+static ssize_t cbmem_entry_id_show(struct kobject *kobj,
+				   struct kobj_attribute *a, char *buf)
+{
+	struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+	return sysfs_emit(buf, "0x%x\n",
+			  entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_setup(struct cbmem_entry *entry)
+{
+	int ret;
+
+	entry->mem_file_buf =
+		devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
+			      entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);
+	if (!entry->mem_file_buf)
+		return -ENOMEM;
+
+	entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
+					  "cbmem-%08x",
+					  entry->dev->cbmem_entry.id);
+	if (!entry->kobj_name)
+		return -ENOMEM;
+
+	entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
+	if (!entry->kobj)
+		return -ENOMEM;
+
+	sysfs_bin_attr_init(&entry->mem_file);
+	entry->mem_file.attr.name = "mem";
+	entry->mem_file.attr.mode = 0664;
+	entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
+	entry->mem_file.read = cbmem_entry_mem_read;
+	entry->mem_file.write = cbmem_entry_mem_write;
+	entry->mem_file.private = entry;
+	ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
+	if (ret)
+		goto free_kobj;
+
+	sysfs_attr_init(&entry->address_file.kobj_attr.attr);
+	entry->address_file.kobj_attr.attr.name = "address";
+	entry->address_file.kobj_attr.attr.mode = 0444;
+	entry->address_file.kobj_attr.show = cbmem_entry_address_show;
+	entry->address_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj,
+				&entry->address_file.kobj_attr.attr);
+	if (ret)
+		goto free_mem_file;
+
+	sysfs_attr_init(&entry->size_file.kobj_attr.attr);
+	entry->size_file.kobj_attr.attr.name = "size";
+	entry->size_file.kobj_attr.attr.mode = 0444;
+	entry->size_file.kobj_attr.show = cbmem_entry_size_show;
+	entry->size_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	if (ret)
+		goto free_address_file;
+
+	sysfs_attr_init(&entry->id_file.kobj_attr.attr);
+	entry->id_file.kobj_attr.attr.name = "id";
+	entry->id_file.kobj_attr.attr.mode = 0444;
+	entry->id_file.kobj_attr.show = cbmem_entry_id_show;
+	entry->id_file.entry = entry;
+	ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	if (ret)
+		goto free_size_file;
+
+	return 0;
+
+free_size_file:
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+free_address_file:
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+free_mem_file:
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+free_kobj:
+	kobject_put(entry->kobj);
+	return ret;
+}
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry;
+
+	entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dev->dev, entry);
+	entry->dev = dev;
+	return cbmem_entry_setup(entry);
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+	sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+	sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+	sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+	kobject_put(entry->kobj);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+	.probe = cbmem_entry_probe,
+	.remove = cbmem_entry_remove,
+	.drv = {
+		.name = "cbmem",
+	},
+	.tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+	int ret;
+
+	coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+	if (!coreboot_kobj)
+		return -ENOMEM;
+
+	ret = coreboot_driver_register(&cbmem_entry_driver);
+	if (ret) {
+		kobject_put(coreboot_kobj);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+	kobject_put(coreboot_kobj);
+	coreboot_driver_unregister(&cbmem_entry_driver);
+}
+module_exit(cbmem_exit);
+
+MODULE_AUTHOR("Jack Rosenthal <jrosenth@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..6c03a8852d1b 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -39,6 +39,16 @@ struct lb_cbmem_ref {
 	u64 cbmem_addr;
 };
 
+/* Corresponds to LB_TAG_CBMEM_ENTRY. */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -65,6 +75,7 @@ struct coreboot_device {
 	union {
 		struct coreboot_table_entry entry;
 		struct lb_cbmem_ref cbmem_ref;
+		struct lb_cbmem_entry cbmem_entry;
 		struct lb_framebuffer framebuffer;
 	};
 };
-- 
2.37.1.559.g78731f0fdb-goog

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Stephen Boyd]

Quoting Jack Rosenthal (2022-08-04 07:28:56)
> cbmem entries can be read from coreboot table
> 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> entries in sysfs under /sys/firmware/coreboot/cbmem-*.
>
> Link: https://issuetracker.google.com/239604743

Is what you intend to use from here essentially an nvram? If so, it may
make more sense to expose just that entry in drivers/nvmem/ as a
read-only sort of nvmem.

> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> Reviewed-by: Julius Werner <jwerner@chromium.org>
> Tested-by: Jack Rosenthal <jrosenth@chromium.org>
> Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
> ---

Please don't send patches as replies to previous rounds. It makes it
harder to dig out the latest series.

> v7: Updated commit message.
>  .../ABI/testing/sysfs-firmware-coreboot       |  17 ++
>  drivers/firmware/google/Kconfig               |   8 +
>  drivers/firmware/google/Makefile              |   3 +
>  drivers/firmware/google/cbmem.c               | 232 ++++++++++++++++++
>  drivers/firmware/google/coreboot_table.h      |  11 +
>  5 files changed, 271 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
>  create mode 100644 drivers/firmware/google/cbmem.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
> new file mode 100644
> index 000000000000..2401483bb86c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
> @@ -0,0 +1,17 @@
> +What:          /sys/firmware/coreboot/
> +Date:          July 2022
> +Contact:       Jack Rosenthal <jrosenth@chromium.org>
> +Description:
> +               Coreboot-based BIOS firmware provides a variety of information
> +               in CBMEM.  Each CBMEM entry can be found via Coreboot tables.
> +               For each CBMEM entry, the following are exposed:
> +
> +               ======= =======================================================
> +               address A hexidecimal value of the memory address the data for
> +                       the entry begins at.
> +               size    The size of the data stored.
> +               id      The id corresponding to the entry. A list of ids known
> +                       to coreboot can be found in the coreboot source tree at
> +                       ``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``
> +               mem     A file exposing the raw memory for the entry.
> +               ======= =======================================================

It isn't clear to me what the structure of this path is. I'd expect to
see an entry for each 'address', 'size', 'id', 'mem' with a different
What/Date/Contact/Description. If those attributes are all within a
directory in sysfs then there could be a top-level description for that
as well.

   What:          /sys/firmware/coreboot/cbmem-<id>
   Date:          July 2022
   Contact:       Jack Rosenthal <jrosenth@chromium.org>
   Description:
	

   What:          /sys/firmware/coreboot/cbmem-<id>/mem
   Date:          July 2022
   Contact:       Jack Rosenthal <jrosenth@chromium.org>
   Description:

   What:          /sys/firmware/coreboot/cbmem-<id>/size
   Date:          July 2022
   Contact:       Jack Rosenthal <jrosenth@chromium.org>
   Description:

etc..

> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 983e07dc022e..bf8316d1cb31 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -19,6 +19,14 @@ config GOOGLE_SMI
>           driver provides an interface for reading and writing NVRAM
>           variables.
>
> +config GOOGLE_CBMEM
> +       tristate "CBMEM entries in sysfs"
> +       depends on GOOGLE_COREBOOT_TABLE
> +       help
> +         This option enables the kernel to search for Coreboot CBMEM
> +         entries, and expose the memory for each entry in sysfs under
> +         /sys/firmware/coreboot.
> +
>  config GOOGLE_COREBOOT_TABLE
>         tristate "Coreboot Table Access"
>         depends on HAS_IOMEM && (ACPI || OF)
> diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
> index d17caded5d88..8151e323cc43 100644
> --- a/drivers/firmware/google/Makefile
> +++ b/drivers/firmware/google/Makefile
> @@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
>  obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
>  obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
>
> +# Must come after coreboot_table.o, as this driver depends on that bus type.
> +obj-$(CONFIG_GOOGLE_CBMEM)             += cbmem.o
> +
>  vpd-sysfs-y := vpd.o vpd_decode.o
>  obj-$(CONFIG_GOOGLE_VPD)               += vpd-sysfs.o
> diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
> new file mode 100644
> index 000000000000..9646a8047742
> --- /dev/null
> +++ b/drivers/firmware/google/cbmem.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * cbmem.c
> + *
> + * Driver for exporting cbmem entries in sysfs.
> + *
> + * Copyright 2022 Google LLC
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#include "coreboot_table.h"
> +
> +#define LB_TAG_CBMEM_ENTRY 0x31
> +
> +static struct kobject *coreboot_kobj;
> +
> +struct cbmem_entry;
> +struct cbmem_entry_attr {
> +       struct kobj_attribute kobj_attr;
> +       struct cbmem_entry *entry;
> +};
> +
> +struct cbmem_entry {
> +       char *kobj_name;
> +       struct kobject *kobj;
> +       struct coreboot_device *dev;
> +       struct bin_attribute mem_file;
> +       char *mem_file_buf;
> +       struct cbmem_entry_attr address_file;
> +       struct cbmem_entry_attr size_file;
> +       struct cbmem_entry_attr id_file;
> +};
> +
> +static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
> +{
> +       return container_of(a, struct cbmem_entry_attr, kobj_attr);
> +}
> +
> +static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
> +                                   struct bin_attribute *bin_attr, char *buf,
> +                                   loff_t pos, size_t count)
> +{
> +       struct cbmem_entry *entry = bin_attr->private;
> +
> +       return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
> +                                      bin_attr->size);
> +}
> +
> +static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
> +                                    struct bin_attribute *bin_attr, char *buf,
> +                                    loff_t pos, size_t count)
> +{
> +       struct cbmem_entry *entry = bin_attr->private;
> +
> +       if (pos < 0 || pos >= bin_attr->size)
> +               return -EINVAL;
> +       if (count > bin_attr->size - pos)
> +               count = bin_attr->size - pos;
> +
> +       memcpy(entry->mem_file_buf + pos, buf, count);
> +       return count;
> +}

Do you need to be able to write cbmem entries?

> +
> +static ssize_t cbmem_entry_address_show(struct kobject *kobj,
> +                                       struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%llx\n",
> +                         entry_attr->entry->dev->cbmem_entry.address);
> +}
> +
> +static ssize_t cbmem_entry_size_show(struct kobject *kobj,
> +                                    struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%x\n",
> +                         entry_attr->entry->dev->cbmem_entry.entry_size);
> +}
> +
> +static ssize_t cbmem_entry_id_show(struct kobject *kobj,
> +                                  struct kobj_attribute *a, char *buf)
> +{
> +       struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
> +
> +       return sysfs_emit(buf, "0x%x\n",
> +                         entry_attr->entry->dev->cbmem_entry.id);
> +}
> +
> +static int cbmem_entry_setup(struct cbmem_entry *entry)
> +{
> +       int ret;
> +
> +       entry->mem_file_buf =
> +               devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
> +                             entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);

I suspect this won't work because the driver can be unbound, and thus
the mapping can be destroyed by devm, but then the kobject and sysfs
file like 'mem' can be held by userspace after the driver is unbound.
Userspace access after that time will try to read/write unmapped memory.

> +       if (!entry->mem_file_buf)
> +               return -ENOMEM;
> +
> +       entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
> +                                         "cbmem-%08x",
> +                                         entry->dev->cbmem_entry.id);
> +       if (!entry->kobj_name)
> +               return -ENOMEM;
> +
> +       entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
> +       if (!entry->kobj)
> +               return -ENOMEM;
> +
> +       sysfs_bin_attr_init(&entry->mem_file);
> +       entry->mem_file.attr.name = "mem";
> +       entry->mem_file.attr.mode = 0664;
> +       entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
> +       entry->mem_file.read = cbmem_entry_mem_read;
> +       entry->mem_file.write = cbmem_entry_mem_write;
> +       entry->mem_file.private = entry;
> +       ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
> +       if (ret)
> +               goto free_kobj;
> +
> +       sysfs_attr_init(&entry->address_file.kobj_attr.attr);
> +       entry->address_file.kobj_attr.attr.name = "address";
> +       entry->address_file.kobj_attr.attr.mode = 0444;
> +       entry->address_file.kobj_attr.show = cbmem_entry_address_show;
> +       entry->address_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj,
> +                               &entry->address_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_mem_file;
> +
> +       sysfs_attr_init(&entry->size_file.kobj_attr.attr);
> +       entry->size_file.kobj_attr.attr.name = "size";
> +       entry->size_file.kobj_attr.attr.mode = 0444;
> +       entry->size_file.kobj_attr.show = cbmem_entry_size_show;
> +       entry->size_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_address_file;
> +
> +       sysfs_attr_init(&entry->id_file.kobj_attr.attr);
> +       entry->id_file.kobj_attr.attr.name = "id";
> +       entry->id_file.kobj_attr.attr.mode = 0444;
> +       entry->id_file.kobj_attr.show = cbmem_entry_id_show;
> +       entry->id_file.entry = entry;
> +       ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
> +       if (ret)
> +               goto free_size_file;
> +
> +       return 0;
> +
> +free_size_file:
> +       sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +free_address_file:
> +       sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
> +free_mem_file:
> +       sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
> +free_kobj:
> +       kobject_put(entry->kobj);
> +       return ret;
> +}
> +
> +static int cbmem_entry_probe(struct coreboot_device *dev)
> +{
> +       struct cbmem_entry *entry;
> +
> +       entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(&dev->dev, entry);
> +       entry->dev = dev;
> +       return cbmem_entry_setup(entry);
> +}
> +
> +static void cbmem_entry_remove(struct coreboot_device *dev)
> +{
> +       struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
> +
> +       sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
> +       sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
> +       sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> +       sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
> +       kobject_put(entry->kobj);
> +}
> +
> +static struct coreboot_driver cbmem_entry_driver = {
> +       .probe = cbmem_entry_probe,
> +       .remove = cbmem_entry_remove,
> +       .drv = {
> +               .name = "cbmem",
> +       },
> +       .tag = LB_TAG_CBMEM_ENTRY,
> +};
> +
> +static int __init cbmem_init(void)
> +{
> +       int ret;
> +
> +       coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);

Why not make this in the bus driver (coreboot-table.c)? In fact, most
things could probably be created there instead of in a 'driver' that
really isn't doing much 'driving' at all.

> +       if (!coreboot_kobj)
> +               return -ENOMEM;
> +
> +       ret = coreboot_driver_register(&cbmem_entry_driver);
> +       if (ret) {
> +               kobject_put(coreboot_kobj);

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Julius Werner]

> Quoting Jack Rosenthal (2022-08-04 07:28:56)
> > cbmem entries can be read from coreboot table
> > 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> > entries in sysfs under /sys/firmware/coreboot/cbmem-*.
> >
> > Link: https://issuetracker.google.com/239604743
>
> Is what you intend to use from here essentially an nvram? If so, it may
> make more sense to expose just that entry in drivers/nvmem/ as a
> read-only sort of nvmem.

No, it is not NV. It's just a normal memory buffer allocated and
filled by firmware on boot and placed in a region of normal DRAM
that's marked as reserved.

> It isn't clear to me what the structure of this path is. I'd expect to
> see an entry for each 'address', 'size', 'id', 'mem' with a different
> What/Date/Contact/Description. If those attributes are all within a
> directory in sysfs then there could be a top-level description for that
> as well.

CBMEM buffers are used by coreboot for all sorts of things and we
regularly define new ones. We can't maintain a full list of all IDs in
kernel sources because it would be a ton of churn for no reason --
best we could do is to add a link to the ID list in the coreboot repo
(e.g. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h).
We really only care about one of these right now and many of them will
never be relevant to the kernel, but I still thought that while we're
doing this we might as well provide a generic interface to all of them
because others may become useful in the future (and then you don't
have to update the kernel every time you get a new use case for some
userspace tool wanting to interact with some resident data structure
from coreboot).

> Do you need to be able to write cbmem entries?

Yes, see the use case in the bug (using the vboot workbuffer from
coreboot as a write-through cache for the vboot nvdata on flash).

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Stephen Boyd]

Quoting Julius Werner (2022-08-04 16:14:43)
> > Quoting Jack Rosenthal (2022-08-04 07:28:56)
> > > cbmem entries can be read from coreboot table
> > > 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> > > entries in sysfs under /sys/firmware/coreboot/cbmem-*.
> > >
> > > Link: https://issuetracker.google.com/239604743
> >
> > Is what you intend to use from here essentially an nvram? If so, it may
> > make more sense to expose just that entry in drivers/nvmem/ as a
> > read-only sort of nvmem.
>
> No, it is not NV. It's just a normal memory buffer allocated and
> filled by firmware on boot and placed in a region of normal DRAM
> that's marked as reserved.

Alright cool. The bug says 'vbnv' so I was guessing 'nv' meant
non-volatile.

>
> > It isn't clear to me what the structure of this path is. I'd expect to
> > see an entry for each 'address', 'size', 'id', 'mem' with a different
> > What/Date/Contact/Description. If those attributes are all within a
> > directory in sysfs then there could be a top-level description for that
> > as well.
>
> CBMEM buffers are used by coreboot for all sorts of things and we
> regularly define new ones. We can't maintain a full list of all IDs in
> kernel sources because it would be a ton of churn for no reason --
> best we could do is to add a link to the ID list in the coreboot repo
> (e.g. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h).

Agreed, that's a good approach.

> We really only care about one of these right now and many of them will
> never be relevant to the kernel, but I still thought that while we're
> doing this we might as well provide a generic interface to all of them
> because others may become useful in the future (and then you don't
> have to update the kernel every time you get a new use case for some
> userspace tool wanting to interact with some resident data structure
> from coreboot).

Exposing more than is necessary in the kernel ABI could get into
problems with backwards compatibility. It also means we have to review
the ABI with the consideration that something may change in the future
and cbmem would be exposing something we don't want exposed, or maybe it
is writeable when it shouldn't be?

Furthermore, if the ABI that the kernel can expose already exists then
we're better off because there may already be userspace programs written
for that ABI with lessons learned and bugs ironed out. In this case, I
was hoping that it was an nvmem, in which case we could tie it into the
nvmem framework and piggyback on the nvmem APIs. It also helps to expose
a more generic ABI to userspace instead of developing a bespoke solution
requiring boutique software. Of course sometimes things can't be so
generic so code has to be written.

>
> > Do you need to be able to write cbmem entries?
>
> Yes, see the use case in the bug (using the vboot workbuffer from
> coreboot as a write-through cache for the vboot nvdata on flash).

Is it a cached non-volatile memory?

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Julius Werner]

> Alright cool. The bug says 'vbnv' so I was guessing 'nv' meant
> non-volatile.

"vbnv" is non-volatile memory, but this driver doesn't actually access
vbnv (which is on SPI flash). This driver is meant to access an
in-memory cache of vbnv that was put there by firmware.

> > We really only care about one of these right now and many of them will
> > never be relevant to the kernel, but I still thought that while we're
> > doing this we might as well provide a generic interface to all of them
> > because others may become useful in the future (and then you don't
> > have to update the kernel every time you get a new use case for some
> > userspace tool wanting to interact with some resident data structure
> > from coreboot).
>
> Exposing more than is necessary in the kernel ABI could get into
> problems with backwards compatibility. It also means we have to review
> the ABI with the consideration that something may change in the future
> and cbmem would be exposing something we don't want exposed, or maybe it
> is writeable when it shouldn't be?

I think we have a bit of a different concept of what this is supposed
to be. Forget about the vbnv and vboot workbuffer parts for the
moment, those are where our overall motivation for doing this comes
from but those should not be concerns for the kernel itself. From the
kernel's point of view, all we have here is a firmware information
structure it already knows about (the coreboot tables) pointing to a
list of opaque, firmware-specific memory buffers labeled with an ID,
and we want it to expose read and write access to those buffers to
userspace to make it easier for firmware-specific userspace helper
tools to work with them. Note that we already have userspace tools
doing that today anyway, e.g. the `cbmem` utility uses /dev/mem to
search for and parse the whole coreboot table itself before then using
it to access individual CBMEM buffers. But implementing all the logic
to do that in each tool that wants to support anything
coreboot-specific separately is cumbersome, so we thought that since
the kernel already has this coreboot table parsing support anyway, we
might as well export the info to userspace to make this job easier. So
really all the kernel does here is expose the address, size and ID
values from the coreboot table itself, it doesn't (and isn't supposed
to) have any understanding about the pointed to buffer. (We could also
leave out the `mem` node from this driver and just let our userspace
utilities read the `address` and `size` nodes and then use that info
to go through /dev/mem. Giving them a direct `mem` node for the buffer
just makes that process a bit easier and cleaner.)

So backwards compatibility should not be a concern (unless we're
talking about changes in the coreboot table itself, which we strongly
try to avoid and which would be an issue for existing kernel drivers
already anyway). Understanding of these buffers' contents is
explicitly supposed to be the responsibility of the userspace tool
that has an easier time keeping up with frequent firmware data format
changes and maintaining long lists of back-translating routines for
older structure formats for the specific kind of buffer it looks for
if necessary (something I specifically wouldn't want to clutter the
kernel with).

In regards to write access, I don't really see a point restricting
this since all these buffers are already accessible through /dev/mem
anyway. But that should only be needed for very few of these buffers
anyway, so if people think it's a concern I think we could also keep a
small explicit allowlist for the IDs that can be writable (shouldn't
need to be updated frequently) and disallow it for everything else.
Also, of course there's still the normal file system access
permissions that makes these only writable for root and users
specifically granted access by root. (Actually, Jack, that reminds
me... having the `mem` nodes world-readable is maybe not a good idea,
there's usually not anything specifically secret in there, but since
/dev/mem also isn't world-readable I think this one probably shouldn't
either. I'd suggest changing the initial umask to 0660 or 0600.)

> Furthermore, if the ABI that the kernel can expose already exists then
> we're better off because there may already be userspace programs written
> for that ABI with lessons learned and bugs ironed out. In this case, I
> was hoping that it was an nvmem, in which case we could tie it into the
> nvmem framework and piggyback on the nvmem APIs. It also helps to expose
> a more generic ABI to userspace instead of developing a bespoke solution
> requiring boutique software. Of course sometimes things can't be so
> generic so code has to be written.

Yeah but this isn't anything that can be genericized, this is
specifically meant for boutique software to maintain its internal data
structures. vbnv and the vb2_shared_data structure in the workbuffer
are supposed to be completely internal to vboot and not interpreted by
any code other than vboot itself -- even coreboot never accesses them
directly (only by linking in vboot functions and calling those). We
explicitly don't want to deal with having to sync data structure
format changes to anywhere outside the vboot repository. The problem
is just that unlike most software, vboot is a framework that contains
both firmware and userspace components, so the latter necessarily need
some help from the kernel (as an oblivious forwarder of opaque data)
to be able to access the internal data structures passed on from the
former.

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Stephen Boyd]

Quoting Julius Werner (2022-08-10 15:26:30)
> > > We really only care about one of these right now and many of them will
> > > never be relevant to the kernel, but I still thought that while we're
> > > doing this we might as well provide a generic interface to all of them
> > > because others may become useful in the future (and then you don't
> > > have to update the kernel every time you get a new use case for some
> > > userspace tool wanting to interact with some resident data structure
> > > from coreboot).
> >
> > Exposing more than is necessary in the kernel ABI could get into
> > problems with backwards compatibility. It also means we have to review
> > the ABI with the consideration that something may change in the future
> > and cbmem would be exposing something we don't want exposed, or maybe it
> > is writeable when it shouldn't be?
>
> I think we have a bit of a different concept of what this is supposed
> to be. Forget about the vbnv and vboot workbuffer parts for the
> moment, those are where our overall motivation for doing this comes
> from but those should not be concerns for the kernel itself. From the
> kernel's point of view, all we have here is a firmware information
> structure it already knows about (the coreboot tables) pointing to a
> list of opaque, firmware-specific memory buffers labeled with an ID,
> and we want it to expose read and write access to those buffers to
> userspace to make it easier for firmware-specific userspace helper
> tools to work with them. Note that we already have userspace tools
> doing that today anyway, e.g. the `cbmem` utility uses /dev/mem to
> search for and parse the whole coreboot table itself before then using
> it to access individual CBMEM buffers. But implementing all the logic
> to do that in each tool that wants to support anything
> coreboot-specific separately is cumbersome, so we thought that since
> the kernel already has this coreboot table parsing support anyway, we
> might as well export the info to userspace to make this job easier. So
> really all the kernel does here is expose the address, size and ID
> values from the coreboot table itself, it doesn't (and isn't supposed
> to) have any understanding about the pointed to buffer. (We could also
> leave out the `mem` node from this driver and just let our userspace
> utilities read the `address` and `size` nodes and then use that info
> to go through /dev/mem. Giving them a direct `mem` node for the buffer
> just makes that process a bit easier and cleaner.)

Got it. Thanks for the background. Is it possible to create new entries
in the table? Or to resize existing entries? Or to delete entries
entirely?

>
> So backwards compatibility should not be a concern (unless we're
> talking about changes in the coreboot table itself, which we strongly
> try to avoid and which would be an issue for existing kernel drivers
> already anyway). Understanding of these buffers' contents is
> explicitly supposed to be the responsibility of the userspace tool
> that has an easier time keeping up with frequent firmware data format
> changes and maintaining long lists of back-translating routines for
> older structure formats for the specific kind of buffer it looks for
> if necessary (something I specifically wouldn't want to clutter the
> kernel with).

Maybe this answers the question above.

>
> In regards to write access, I don't really see a point restricting
> this since all these buffers are already accessible through /dev/mem
> anyway. But that should only be needed for very few of these buffers
> anyway, so if people think it's a concern I think we could also keep a
> small explicit allowlist for the IDs that can be writable (shouldn't
> need to be updated frequently) and disallow it for everything else.
> Also, of course there's still the normal file system access
> permissions that makes these only writable for root and users
> specifically granted access by root. (Actually, Jack, that reminds
> me... having the `mem` nodes world-readable is maybe not a good idea,
> there's usually not anything specifically secret in there, but since
> /dev/mem also isn't world-readable I think this one probably shouldn't
> either. I'd suggest changing the initial umask to 0660 or 0600.)

The /dev/mem interface has been restricted over the years. At this point
we're trying to steer users away from /dev/mem to anything else. I
suspect it happens to work right now because coreboot tells the kernel
that there isn't actually memory in this address range so that devmem
can map it. I don't know but I wonder if the memory is being mapped
uncached on ARM systems, leading to slower access times? Usually when
memory addresses aren't marked as normal memory that's reserved it
doesn't get mapped until the memremap() time, and that would be mapped
with whatever attributes are used in the call. /dev/mem doesn't optimize
this from what I recall.

>
> > Furthermore, if the ABI that the kernel can expose already exists then
> > we're better off because there may already be userspace programs written
> > for that ABI with lessons learned and bugs ironed out. In this case, I
> > was hoping that it was an nvmem, in which case we could tie it into the
> > nvmem framework and piggyback on the nvmem APIs. It also helps to expose
> > a more generic ABI to userspace instead of developing a bespoke solution
> > requiring boutique software. Of course sometimes things can't be so
> > generic so code has to be written.
>
> Yeah but this isn't anything that can be genericized, this is
> specifically meant for boutique software to maintain its internal data
> structures. vbnv and the vb2_shared_data structure in the workbuffer
> are supposed to be completely internal to vboot and not interpreted by
> any code other than vboot itself -- even coreboot never accesses them
> directly (only by linking in vboot functions and calling those). We
> explicitly don't want to deal with having to sync data structure
> format changes to anywhere outside the vboot repository. The problem
> is just that unlike most software, vboot is a framework that contains
> both firmware and userspace components, so the latter necessarily need
> some help from the kernel (as an oblivious forwarder of opaque data)
> to be able to access the internal data structures passed on from the
> former.

Fair enough. How similar is this to efivars? I don't know, and you may
not either, but at a high level it looks similar. The sysfs interface to
efivars was deprecated and I saw recently that there's an effort to
remove it entirely. The new way of interacting with those firmware
variables is through a filesystem that's mounted at
/sys/firmware/efi/efivars. The documentation[1] states that the sysfs
interface didn't work when the variables got large. Hopefully that won't
be a similar scenario here?

[1] https://www.kernel.org/doc/html/latest/filesystems/efivarfs.html`

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Julius Werner]

> Got it. Thanks for the background. Is it possible to create new entries
> in the table? Or to resize existing entries? Or to delete entries
> entirely?

Not easily (would have to see if there's still space at the end and
rewrite the table header), and more importantly there should be no
reason to ever do that at OS runtime. This table is only meant for
coreboot to publish information about itself or store data that needs
to stay resident for whatever reason. Userspace should be able to
access the things that are already there but it isn't meant as a
free-for-all for other environments to add on to.

> The /dev/mem interface has been restricted over the years. At this point
> we're trying to steer users away from /dev/mem to anything else. I
> suspect it happens to work right now because coreboot tells the kernel
> that there isn't actually memory in this address range so that devmem
> can map it. I don't know but I wonder if the memory is being mapped
> uncached on ARM systems, leading to slower access times? Usually when
> memory addresses aren't marked as normal memory that's reserved it
> doesn't get mapped until the memremap() time, and that would be mapped
> with whatever attributes are used in the call. /dev/mem doesn't optimize
> this from what I recall.

Yes, we mark those areas as reserved in the e820 / device tree. The
kernel drivers (this one and the older ones) use MEMREMAP_WB which
should do the right thing. `cbmem` uses mmap(MAP_SHARED) on /dev/mem
which I thought does the right thing but I'm not quite sure. That's
another good reason to implement a dedicated sysfs interface where we
have finer control about these things, once we have that we can make
the older tools use it as well on supporting kernels. (`cbmem` is
currently not called in any critical boot path on Chrome OS as far as
I know, so its performance is not that critical. The new use case Jack
wants to build _is_ going to be in a critical path, though, so we
should make sure it is as performant as it can be.)

> Fair enough. How similar is this to efivars? I don't know, and you may
> not either, but at a high level it looks similar. The sysfs interface to
> efivars was deprecated and I saw recently that there's an effort to
> remove it entirely. The new way of interacting with those firmware
> variables is through a filesystem that's mounted at
> /sys/firmware/efi/efivars. The documentation[1] states that the sysfs
> interface didn't work when the variables got large. Hopefully that won't
> be a similar scenario here?

I don't even know what efivars is tbh. But whatever other cases with
other firmwares may exist that may or may not be more standardized and
make more sense to implement high-touch drivers for in kernel space,
here we really have something that's really opaque and really not
meant to be tampered with by external code, so I think this agnostic
byte buffer access is the only thing that makes sense. I don't see any
reason why there would be size restrictions with the implementation
Jack proposed? FWIW, the total size of the CBMEM buffer we want to
access is 80K on current builds of coreboot (but our use case will
likely only need to read the first couple hundred bytes).

Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

[Jack Rosenthal]

On 2022-08-04 at 15:59 -0500, Stephen Boyd wrote:
> I suspect this won't work because the driver can be unbound, and thus
> the mapping can be destroyed by devm, but then the kobject and sysfs
> file like 'mem' can be held by userspace after the driver is unbound.
> Userspace access after that time will try to read/write unmapped memory.

When the driver is unbound, read()/write() will return -ENODEV, and
mmap() isn't supported.

> Why not make this in the bus driver (coreboot-table.c)? In fact, most
> things could probably be created there instead of in a 'driver' that
> really isn't doing much 'driving' at all.

In v8, I moved the coreboot_kobj to coreboot-table.c.  I imagine in the
future we could move some of the other things (e.g., "vpd") under this
object for consistency.  So now the structure is:

   /sys/firmware/coreboot/cbmem/<id>/...

Re: [PATCH v6] firmware: google: Implement cbmem in sysfs driver

[Jack Rosenthal]

On 2022-08-04 at 08:22 +0000, Tzung-Bi Shih wrote:
> On Tue, Aug 02, 2022 at 07:03:43AM -0700, Jack Rosenthal wrote:
> > Link: https://issuetracker.google.com/239604743
> 
> I doubt if the Link tag would be useful for public.

Thanks, made the bug public and kept the link.

> > Reported-by: kernel test robot <lkp@intel.com>
> 
> Suggest to drop the Reported-by tag even if the robot reported some compile
> errors during the development.  Given that the patch isn't a fixup but
> supporting something new (which isn't "reported by" the robot).

Done.