The CBMEM area is a downward-growing memory region used by coreboot to
dynamically allocate tagged data structures ("CBMEM entries") that
remain resident during boot.
This implements a driver which exports access to the CBMEM entries
via sysfs under /sys/bus/coreboot/devices/cbmem-<id>.
This implementation is quite versatile. Examples of how it could be
used are given below:
* Tools like util/cbmem from the coreboot tree could use this driver
instead of finding CBMEM in /dev/mem directly. Alternatively,
firmware developers debugging an issue may find the sysfs interface
more ergonomic than the cbmem tool and choose to use it directly.
* The crossystem tool, which exposes verified boot variables, can use
this driver to read the vboot work buffer.
* Tools which read the BIOS SPI flash (e.g., flashrom) can find the
flash layout in CBMEM directly, which is significantly faster than
searching the flash directly.
Write access is provided to all CBMEM regions via
/sys/bus/coreboot/devices/cbmem-<id>/mem, as the existing cbmem
tooling updates this memory region, and envisioned use cases with
crossystem can benefit from updating memory regions.
Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <[email protected]>
Cc: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
Reviewed-by: Julius Werner <[email protected]>
Tested-by: Jack Rosenthal <[email protected]>
Signed-off-by: Jack Rosenthal <[email protected]>
---
Changes in v12:
* Removed symlink from /sys/firmware/cbmem to the device.
* Device is now named cbmem-<id>, allowing location of the device in
sysfs by the CBMEM id.
* Documentation and Kconfig help text expanded.
Documentation/ABI/testing/sysfs-bus-coreboot | 50 +++++++
drivers/firmware/google/Kconfig | 14 ++
drivers/firmware/google/Makefile | 3 +
drivers/firmware/google/cbmem.c | 139 +++++++++++++++++++
drivers/firmware/google/coreboot_table.c | 11 +-
drivers/firmware/google/coreboot_table.h | 18 +++
6 files changed, 234 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-coreboot
create mode 100644 drivers/firmware/google/cbmem.c
diff --git a/Documentation/ABI/testing/sysfs-bus-coreboot b/Documentation/ABI/testing/sysfs-bus-coreboot
new file mode 100644
index 000000000000..886a39758896
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coreboot
@@ -0,0 +1,50 @@
+What: /sys/bus/coreboot
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ The coreboot bus provides a variety of virtual devices used to
+ access data structures created by the Coreboot BIOS.
+
+What: /sys/bus/coreboot/devices/cbmem-<id>
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ CBMEM is a downwards-growing memory region created by Coreboot,
+ and contains tagged data structures to be shared with payloads
+ in the boot process and the OS. Each CBMEM entry is given a
+ directory in /sys/bus/coreboot/devices based on its id.
+ 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``.
+
+What: /sys/bus/coreboot/devices/cbmem-<id>/address
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ This is the pyhsical memory address that the CBMEM entry's data
+ begins at.
+
+What: /sys/bus/coreboot/devices/cbmem-<id>/size
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ This is the size of the CBMEM entry's data.
+
+What: /sys/bus/coreboot/devices/cbmem-<id>/id
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ This is the CBMEM id corresponding to the entry.
+
+What: /sys/bus/coreboot/devices/cbmem-<id>/mem
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ A file exposing read/write access to the entry's data. Note
+ that this file does not support mmap(), as coreboot
+ does not guarantee that the data will be page-aligned.
+
+ The mode of this file is 0600. While there shouldn't be
+ anything security-sensitive contained in CBMEM, read access
+ requires root privileges given this is exposing a small subset
+ of physical memory.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..a9b246e67b23 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,6 +19,20 @@ 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
+ CBMEM is a downwards-growing memory region created by the
+ Coreboot BIOS containing tagged data structures from the
+ BIOS. These data structures expose things like the verified
+ boot firmware variables, flash layout, firmware event log,
+ and more.
+
+ Say Y here to enable the kernel to search for Coreboot CBMEM
+ entries, and expose the memory for each entry in sysfs under
+ /sys/bus/coreboot/devices/cbmem-<id>.
+
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..e4bb20432854
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#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"
+
+struct cbmem_entry {
+ char *mem_file_buf;
+ u32 size;
+};
+
+static struct cbmem_entry *to_cbmem_entry(struct kobject *kobj)
+{
+ return dev_get_drvdata(kobj_to_dev(kobj));
+}
+
+static ssize_t mem_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf, loff_t pos,
+ size_t count)
+{
+ struct cbmem_entry *entry = to_cbmem_entry(kobj);
+
+ return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+ entry->size);
+}
+
+static ssize_t mem_write(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf, loff_t pos,
+ size_t count)
+{
+ struct cbmem_entry *entry = to_cbmem_entry(kobj);
+
+ if (pos < 0 || pos >= entry->size)
+ return -EINVAL;
+ if (count > entry->size - pos)
+ count = entry->size - pos;
+
+ memcpy(entry->mem_file_buf + pos, buf, count);
+ return count;
+}
+static BIN_ATTR_ADMIN_RW(mem, 0);
+
+static ssize_t address_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+ return sysfs_emit(buf, "0x%llx\n", cbdev->cbmem_entry.address);
+}
+static DEVICE_ATTR_RO(address);
+
+static ssize_t size_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+ return sysfs_emit(buf, "0x%x\n", cbdev->cbmem_entry.entry_size);
+}
+static DEVICE_ATTR_RO(size);
+
+static ssize_t id_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+ return sysfs_emit(buf, "0x%08x\n", cbdev->cbmem_entry.id);
+}
+static DEVICE_ATTR_RO(id);
+
+static struct attribute *attrs[] = {
+ &dev_attr_address.attr,
+ &dev_attr_size.attr,
+ &dev_attr_id.attr,
+ NULL,
+};
+
+static struct bin_attribute *bin_attrs[] = {
+ &bin_attr_mem,
+ NULL,
+};
+
+static const struct attribute_group cbmem_entry_group = {
+ .attrs = attrs,
+ .bin_attrs = bin_attrs,
+};
+
+static const struct attribute_group *dev_groups[] = {
+ &cbmem_entry_group,
+ NULL,
+};
+
+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->mem_file_buf = devm_memremap(&dev->dev, dev->cbmem_entry.address,
+ dev->cbmem_entry.entry_size,
+ MEMREMAP_WB);
+ if (!entry->mem_file_buf)
+ return -ENOMEM;
+
+ entry->size = dev->cbmem_entry.entry_size;
+
+ return 0;
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+ .probe = cbmem_entry_probe,
+ .drv = {
+ .name = "cbmem",
+ .owner = THIS_MODULE,
+ .dev_groups = dev_groups,
+ },
+ .tag = LB_TAG_CBMEM_ENTRY,
+};
+module_coreboot_driver(cbmem_entry_driver);
+
+MODULE_AUTHOR("Jack Rosenthal <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index c52bcaa9def6..7748067eb9e6 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -97,12 +97,21 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
if (!device)
return -ENOMEM;
- dev_set_name(&device->dev, "coreboot%d", i);
device->dev.parent = dev;
device->dev.bus = &coreboot_bus_type;
device->dev.release = coreboot_device_release;
memcpy(&device->entry, ptr_entry, entry->size);
+ switch (device->entry.tag) {
+ case LB_TAG_CBMEM_ENTRY:
+ dev_set_name(&device->dev, "cbmem-%08x",
+ device->cbmem_entry.id);
+ break;
+ default:
+ dev_set_name(&device->dev, "coreboot%d", i);
+ break;
+ }
+
ret = device_register(&device->dev);
if (ret) {
put_device(&device->dev);
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..37f4d335a606 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -39,6 +39,18 @@ struct lb_cbmem_ref {
u64 cbmem_addr;
};
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+/* 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,10 +77,16 @@ 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;
};
};
+static inline struct coreboot_device *dev_to_coreboot_device(struct device *dev)
+{
+ return container_of(dev, struct coreboot_device, dev);
+}
+
/* A driver for handling devices described in coreboot tables. */
struct coreboot_driver {
int (*probe)(struct coreboot_device *);
--
2.38.0.rc1.362.ged0d419d3c-goog
On Mon, Oct 03, 2022 at 06:38:11PM -0600, Jack Rosenthal wrote:
> The CBMEM area is a downward-growing memory region used by coreboot to
> dynamically allocate tagged data structures ("CBMEM entries") that
> remain resident during boot.
>
> This implements a driver which exports access to the CBMEM entries
> via sysfs under /sys/bus/coreboot/devices/cbmem-<id>.
>
> This implementation is quite versatile. Examples of how it could be
> used are given below:
>
> * Tools like util/cbmem from the coreboot tree could use this driver
> instead of finding CBMEM in /dev/mem directly. Alternatively,
> firmware developers debugging an issue may find the sysfs interface
> more ergonomic than the cbmem tool and choose to use it directly.
>
> * The crossystem tool, which exposes verified boot variables, can use
> this driver to read the vboot work buffer.
>
> * Tools which read the BIOS SPI flash (e.g., flashrom) can find the
> flash layout in CBMEM directly, which is significantly faster than
> searching the flash directly.
>
> Write access is provided to all CBMEM regions via
> /sys/bus/coreboot/devices/cbmem-<id>/mem, as the existing cbmem
> tooling updates this memory region, and envisioned use cases with
> crossystem can benefit from updating memory regions.
>
> Link: https://issuetracker.google.com/239604743
> Cc: Stephen Boyd <[email protected]>
> Cc: Tzung-Bi Shih <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> Reviewed-by: Julius Werner <[email protected]>
> Tested-by: Jack Rosenthal <[email protected]>
> Signed-off-by: Jack Rosenthal <[email protected]>
> ---
> Changes in v12:
> * Removed symlink from /sys/firmware/cbmem to the device.
> * Device is now named cbmem-<id>, allowing location of the device in
> sysfs by the CBMEM id.
> * Documentation and Kconfig help text expanded.
>
> Documentation/ABI/testing/sysfs-bus-coreboot | 50 +++++++
> drivers/firmware/google/Kconfig | 14 ++
> drivers/firmware/google/Makefile | 3 +
> drivers/firmware/google/cbmem.c | 139 +++++++++++++++++++
> drivers/firmware/google/coreboot_table.c | 11 +-
> drivers/firmware/google/coreboot_table.h | 18 +++
> 6 files changed, 234 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-coreboot
> create mode 100644 drivers/firmware/google/cbmem.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-coreboot b/Documentation/ABI/testing/sysfs-bus-coreboot
> new file mode 100644
> index 000000000000..886a39758896
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coreboot
> @@ -0,0 +1,50 @@
> +What: /sys/bus/coreboot
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + The coreboot bus provides a variety of virtual devices used to
> + access data structures created by the Coreboot BIOS.
> +
> +What: /sys/bus/coreboot/devices/cbmem-<id>
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + CBMEM is a downwards-growing memory region created by Coreboot,
> + and contains tagged data structures to be shared with payloads
> + in the boot process and the OS. Each CBMEM entry is given a
> + directory in /sys/bus/coreboot/devices based on its id.
> + 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``.
That will not age well, why not point to the reference in the kernel
tree instead?
> +
> +What: /sys/bus/coreboot/devices/cbmem-<id>/address
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + This is the pyhsical memory address that the CBMEM entry's data
> + begins at.
In hex? Decimal?
> +
> +What: /sys/bus/coreboot/devices/cbmem-<id>/size
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + This is the size of the CBMEM entry's data.
In hex? Decimal? Octal? Binary? Be specific please :)
> +
> +What: /sys/bus/coreboot/devices/cbmem-<id>/id
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + This is the CBMEM id corresponding to the entry.
so "id" is the same as "<id>" here? Why is that needed?
> +
> +What: /sys/bus/coreboot/devices/cbmem-<id>/mem
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + A file exposing read/write access to the entry's data. Note
> + that this file does not support mmap(), as coreboot
> + does not guarantee that the data will be page-aligned.
> +
> + The mode of this file is 0600. While there shouldn't be
> + anything security-sensitive contained in CBMEM, read access
> + requires root privileges given this is exposing a small subset
> + of physical memory.
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 983e07dc022e..a9b246e67b23 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -19,6 +19,20 @@ 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
> + CBMEM is a downwards-growing memory region created by the
> + Coreboot BIOS containing tagged data structures from the
> + BIOS. These data structures expose things like the verified
> + boot firmware variables, flash layout, firmware event log,
> + and more.
> +
> + Say Y here to enable the kernel to search for Coreboot CBMEM
> + entries, and expose the memory for each entry in sysfs under
> + /sys/bus/coreboot/devices/cbmem-<id>.
Module name?
> +
> 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.
Doesn't the linker handle this for us?
> +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..e4bb20432854
> --- /dev/null
> +++ b/drivers/firmware/google/cbmem.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * cbmem.c
> + *
> + * Driver for exporting cbmem entries in sysfs.
> + *
> + * Copyright 2022 Google LLC
> + */
> +
> +#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"
> +
> +struct cbmem_entry {
> + char *mem_file_buf;
> + u32 size;
> +};
> +
> +static struct cbmem_entry *to_cbmem_entry(struct kobject *kobj)
> +{
> + return dev_get_drvdata(kobj_to_dev(kobj));
> +}
> +
> +static ssize_t mem_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf, loff_t pos,
> + size_t count)
> +{
> + struct cbmem_entry *entry = to_cbmem_entry(kobj);
> +
> + return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
> + entry->size);
> +}
> +
> +static ssize_t mem_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf, loff_t pos,
> + size_t count)
> +{
> + struct cbmem_entry *entry = to_cbmem_entry(kobj);
> +
> + if (pos < 0 || pos >= entry->size)
> + return -EINVAL;
> + if (count > entry->size - pos)
> + count = entry->size - pos;
> +
> + memcpy(entry->mem_file_buf + pos, buf, count);
> + return count;
> +}
> +static BIN_ATTR_ADMIN_RW(mem, 0);
Userspace can handle a size of 0 for this file ok?
> +
> +static ssize_t address_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
> +
> + return sysfs_emit(buf, "0x%llx\n", cbdev->cbmem_entry.address);
> +}
> +static DEVICE_ATTR_RO(address);
> +
> +static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
> +
> + return sysfs_emit(buf, "0x%x\n", cbdev->cbmem_entry.entry_size);
> +}
> +static DEVICE_ATTR_RO(size);
> +
> +static ssize_t id_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
> +
> + return sysfs_emit(buf, "0x%08x\n", cbdev->cbmem_entry.id);
> +}
> +static DEVICE_ATTR_RO(id);
> +
> +static struct attribute *attrs[] = {
> + &dev_attr_address.attr,
> + &dev_attr_size.attr,
> + &dev_attr_id.attr,
> + NULL,
> +};
> +
> +static struct bin_attribute *bin_attrs[] = {
> + &bin_attr_mem,
> + NULL,
> +};
> +
> +static const struct attribute_group cbmem_entry_group = {
> + .attrs = attrs,
> + .bin_attrs = bin_attrs,
> +};
> +
> +static const struct attribute_group *dev_groups[] = {
> + &cbmem_entry_group,
> + NULL,
> +};
> +
> +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->mem_file_buf = devm_memremap(&dev->dev, dev->cbmem_entry.address,
> + dev->cbmem_entry.entry_size,
> + MEMREMAP_WB);
> + if (!entry->mem_file_buf)
> + return -ENOMEM;
> +
> + entry->size = dev->cbmem_entry.entry_size;
Ah nevermind you set the size here.
> +
> + return 0;
> +}
> +
> +static struct coreboot_driver cbmem_entry_driver = {
> + .probe = cbmem_entry_probe,
> + .drv = {
> + .name = "cbmem",
> + .owner = THIS_MODULE,
> + .dev_groups = dev_groups,
> + },
> + .tag = LB_TAG_CBMEM_ENTRY,
> +};
> +module_coreboot_driver(cbmem_entry_driver);
> +
> +MODULE_AUTHOR("Jack Rosenthal <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index c52bcaa9def6..7748067eb9e6 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -97,12 +97,21 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
> if (!device)
> return -ENOMEM;
>
> - dev_set_name(&device->dev, "coreboot%d", i);
> device->dev.parent = dev;
> device->dev.bus = &coreboot_bus_type;
> device->dev.release = coreboot_device_release;
> memcpy(&device->entry, ptr_entry, entry->size);
>
> + switch (device->entry.tag) {
> + case LB_TAG_CBMEM_ENTRY:
> + dev_set_name(&device->dev, "cbmem-%08x",
> + device->cbmem_entry.id);
> + break;
> + default:
> + dev_set_name(&device->dev, "coreboot%d", i);
> + break;
> + }
> +
> ret = device_register(&device->dev);
> if (ret) {
> put_device(&device->dev);
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index beb778674acd..37f4d335a606 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -39,6 +39,18 @@ struct lb_cbmem_ref {
> u64 cbmem_addr;
> };
>
> +#define LB_TAG_CBMEM_ENTRY 0x31
> +
> +/* Corresponds to LB_TAG_CBMEM_ENTRY */
> +struct lb_cbmem_entry {
> + u32 tag;
> + u32 size;
little or big endian?
Overall looks much better than before, thanks for the changes.
greg k-h
On 2022-10-04 at 10:51 +0200, Greg KH wrote:
> > + 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``.
>
> That will not age well, why not point to the reference in the kernel
> tree instead?
There is no copy in the kernel tree.
> > +What: /sys/bus/coreboot/devices/cbmem-<id>/address
> > +Date: August 2022
> > +Contact: Jack Rosenthal <[email protected]>
> > +Description:
> > + This is the pyhsical memory address that the CBMEM entry's data
> > + begins at.
>
> In hex? Decimal?
>
> > +
> > +What: /sys/bus/coreboot/devices/cbmem-<id>/size
> > +Date: August 2022
> > +Contact: Jack Rosenthal <[email protected]>
> > +Description:
> > + This is the size of the CBMEM entry's data.
>
> In hex? Decimal? Octal? Binary? Be specific please :)
Added "hexadecimal" and an example for both in v13.
> > +What: /sys/bus/coreboot/devices/cbmem-<id>/id
> > +Date: August 2022
> > +Contact: Jack Rosenthal <[email protected]>
> > +Description:
> > + This is the CBMEM id corresponding to the entry.
>
> so "id" is the same as "<id>" here? Why is that needed?
Removed in v13, agreee it's reduntant with the device name now.
> > + Say Y here to enable the kernel to search for Coreboot CBMEM
> > + entries, and expose the memory for each entry in sysfs under
> > + /sys/bus/coreboot/devices/cbmem-<id>.
>
> Module name?
Added in v13.
>
> > +
> > 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.
>
> Doesn't the linker handle this for us?
Not in the case of compiling as a built-in module: I observed in this
scenario the order in the Makefile deterimined the module initialization
order, and, if this were to be listed alphabetically, the coreboot_table
module would not have been loaded before the cbmem module.
> > + entry->size = dev->cbmem_entry.entry_size;
>
> Ah nevermind you set the size here.
The size that stat reports is still 0, as when creating this as a device
attribute, the size is not known until the driver is probed. I observed
this in some other sysfs attributes, so I imagine it's a common pattern.
> > +/* Corresponds to LB_TAG_CBMEM_ENTRY */
> > +struct lb_cbmem_entry {
> > + u32 tag;
> > + u32 size;
>
> little or big endian?
It's the native host endianness, as coreboot wrote these tables from the
same CPU that Linux is running on.
On Tue, Oct 04, 2022 at 10:56:58AM -0600, Jack Rosenthal wrote:
> On 2022-10-04 at 10:51 +0200, Greg KH wrote:
> > > + 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``.
> >
> > That will not age well, why not point to the reference in the kernel
> > tree instead?
>
> There is no copy in the kernel tree.
Then how does the kernel know what to print out? Why not add such a
reference somewhere?
> > > 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.
> >
> > Doesn't the linker handle this for us?
>
> Not in the case of compiling as a built-in module: I observed in this
> scenario the order in the Makefile deterimined the module initialization
> order, and, if this were to be listed alphabetically, the coreboot_table
> module would not have been loaded before the cbmem module.
So is this a runtime dependancy or a symbol/link dependancy?
link one is easy, we always go off of the Makefile order, and if you
move it and it breaks, well obviously move it back. If it's a runtime
order, then how will you handle one being a module and the other not?
thanks,
greg k-h
On 2022-10-04 at 19:26 +0200, Greg KH wrote:
> On Tue, Oct 04, 2022 at 10:56:58AM -0600, Jack Rosenthal wrote:
> > On 2022-10-04 at 10:51 +0200, Greg KH wrote:
> > > > + 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``.
> > >
> > > That will not age well, why not point to the reference in the kernel
> > > tree instead?
> >
> > There is no copy in the kernel tree.
>
> Then how does the kernel know what to print out? Why not add such a
> reference somewhere?
The kernel really doesn't need to know the list of possible ids: it
simply reads what coreboot gives it from the coreboot tables and proxies
that on up to sysfs nodes.
In an earlier version of this patch
(https://lore.kernel.org/chrome-platform/CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com/T/#u),
I actually had this list so that the directory names were human-readable
instead of using the 32-bit CBMEM id, but Julius didn't like it citing
that we'd have to keep the kernel tree and the coreboot tree in sync.
I'm fine with either solution ... just want to make all parties happy
here =)
>
> > > > 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.
> > >
> > > Doesn't the linker handle this for us?
> >
> > Not in the case of compiling as a built-in module: I observed in this
> > scenario the order in the Makefile deterimined the module initialization
> > order, and, if this were to be listed alphabetically, the coreboot_table
> > module would not have been loaded before the cbmem module.
>
> So is this a runtime dependancy or a symbol/link dependancy?
>
> link one is easy, we always go off of the Makefile order, and if you
> move it and it breaks, well obviously move it back. If it's a runtime
> order, then how will you handle one being a module and the other not?
link/symbol dependency ... it's just the cbmem driver depends on the
coreboot bus. Existing EXPORT_SYMBOL facilities should have this
figured out for the module case, but the Makefile just needs to be in
the right order for the built-in module case.
> > Then how does the kernel know what to print out? Why not add such a
> > reference somewhere?
>
> The kernel really doesn't need to know the list of possible ids: it
> simply reads what coreboot gives it from the coreboot tables and proxies
> that on up to sysfs nodes.
>
> In an earlier version of this patch
> (https://lore.kernel.org/chrome-platform/CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com/T/#u),
> I actually had this list so that the directory names were human-readable
> instead of using the 32-bit CBMEM id, but Julius didn't like it citing
> that we'd have to keep the kernel tree and the coreboot tree in sync.
>
> I'm fine with either solution ... just want to make all parties happy
> here =)
There is quite a long list of possible IDs (79 at the current count),
many of them are just coreboot-internal implementation details for
specific platforms that are really not interesting to the running OS
after we're done booting, and new ones get added all the time. I don't
think there's any practical value in trying to maintain a
corresponding list in the kernel, it would just be unnecessary bloat
and a maintenance nightmare to keep in sync.
This whole driver is supposed to be a thin bridge between coreboot and
coreboot-specific userspace tools. Those tools will know about the
specific meaning of individual IDs and the data format of their
contents, and they are much easier to keep updated and in sync with
new coreboot releases than the kernel itself. So the whole goal of the
design is to leave all those details to the userspace tools and have
the kernel involved as little as possible, just passing the raw
information through without being involved in its interpretation.
On Tue, Oct 04, 2022 at 06:10:01PM -0700, Julius Werner wrote:
> > > Then how does the kernel know what to print out? Why not add such a
> > > reference somewhere?
> >
> > The kernel really doesn't need to know the list of possible ids: it
> > simply reads what coreboot gives it from the coreboot tables and proxies
> > that on up to sysfs nodes.
> >
> > In an earlier version of this patch
> > (https://lore.kernel.org/chrome-platform/CAODwPW-JzXXsEANaS+6n695YqriAQ0j0LXm31R2u1OP3MhX9Uw@mail.gmail.com/T/#u),
> > I actually had this list so that the directory names were human-readable
> > instead of using the 32-bit CBMEM id, but Julius didn't like it citing
> > that we'd have to keep the kernel tree and the coreboot tree in sync.
> >
> > I'm fine with either solution ... just want to make all parties happy
> > here =)
>
> There is quite a long list of possible IDs (79 at the current count),
> many of them are just coreboot-internal implementation details for
> specific platforms that are really not interesting to the running OS
> after we're done booting, and new ones get added all the time. I don't
> think there's any practical value in trying to maintain a
> corresponding list in the kernel, it would just be unnecessary bloat
> and a maintenance nightmare to keep in sync.
>
> This whole driver is supposed to be a thin bridge between coreboot and
> coreboot-specific userspace tools. Those tools will know about the
> specific meaning of individual IDs and the data format of their
> contents, and they are much easier to keep updated and in sync with
> new coreboot releases than the kernel itself. So the whole goal of the
> design is to leave all those details to the userspace tools and have
> the kernel involved as little as possible, just passing the raw
> information through without being involved in its interpretation.
If the kernel is reporting a value, that value needs to be documented
somewhere. If the kernel is acting on that value, it needs to know what
those values are.
In this specific instance it seems that the kernel knows a subset of the
values, and some random userspace tool knows all of them? Think about
what you would want to see here if you knew nothing about this at all.
thanks,
greg k-h
> If the kernel is reporting a value, that value needs to be documented
> somewhere. If the kernel is acting on that value, it needs to know what
> those values are.
>
> In this specific instance it seems that the kernel knows a subset of the
> values, and some random userspace tool knows all of them? Think about
> what you would want to see here if you knew nothing about this at all.
The kernel doesn't know any of the values. The kernel is just telling
userspace that spaces with these IDs exist and providing an interface
to access them, it's not supposed to know what any of them mean.
In terms of what you'd want to see in the documentation, I think what
Jack's patch provides is already the best solution? We're referring to
the definitions in the coreboot source tree as the source of truth for
exact details about what each of these IDs mean. Do you want that
documentation to say more explicitly that these are coreboot-internal
data structures exposed for use by coreboot-aware userspace tools and
that their exact meaning and format is only described within coreboot
sources? Or do you want us to put a full link to coreboot's gitiles
page for the file instead of just the file name? Other than that I'm
not sure how we could make this more explicit -- we don't have a big
official documentation page separate from the source code for all
these IDs in coreboot, unfortunately (like I said, some of them a
large and standardized but most of them are small, platform-specific
things for communicating between different firmware stages that don't
need much explanation beyond the source code itself and don't always
have a fixed format).
On Wed, Oct 05, 2022 at 04:26:55PM -0700, Julius Werner wrote:
> > If the kernel is reporting a value, that value needs to be documented
> > somewhere. If the kernel is acting on that value, it needs to know what
> > those values are.
> >
> > In this specific instance it seems that the kernel knows a subset of the
> > values, and some random userspace tool knows all of them? Think about
> > what you would want to see here if you knew nothing about this at all.
>
> The kernel doesn't know any of the values. The kernel is just telling
> userspace that spaces with these IDs exist and providing an interface
> to access them, it's not supposed to know what any of them mean.
Ah, ok, that was not obvious to me, sorry. If the kernel is just a
pass-through, no objections.
thanks,
greg k-h
On 2022-10-06 at 08:33 +0200, Greg KH wrote:
> On Wed, Oct 05, 2022 at 04:26:55PM -0700, Julius Werner wrote:
> > > If the kernel is reporting a value, that value needs to be documented
> > > somewhere. If the kernel is acting on that value, it needs to know what
> > > those values are.
> > >
> > > In this specific instance it seems that the kernel knows a subset of the
> > > values, and some random userspace tool knows all of them? Think about
> > > what you would want to see here if you knew nothing about this at all.
> >
> > The kernel doesn't know any of the values. The kernel is just telling
> > userspace that spaces with these IDs exist and providing an interface
> > to access them, it's not supposed to know what any of them mean.
>
> Ah, ok, that was not obvious to me, sorry. If the kernel is just a
> pass-through, no objections.
Does this mean PATCH v13 looks good to you?
thanks,
jack
On Thu, Oct 13, 2022 at 03:25:58PM -0600, Jack Rosenthal wrote:
> On 2022-10-06 at 08:33 +0200, Greg KH wrote:
> > On Wed, Oct 05, 2022 at 04:26:55PM -0700, Julius Werner wrote:
> > > > If the kernel is reporting a value, that value needs to be documented
> > > > somewhere. If the kernel is acting on that value, it needs to know what
> > > > those values are.
> > > >
> > > > In this specific instance it seems that the kernel knows a subset of the
> > > > values, and some random userspace tool knows all of them? Think about
> > > > what you would want to see here if you knew nothing about this at all.
> > >
> > > The kernel doesn't know any of the values. The kernel is just telling
> > > userspace that spaces with these IDs exist and providing an interface
> > > to access them, it's not supposed to know what any of them mean.
> >
> > Ah, ok, that was not obvious to me, sorry. If the kernel is just a
> > pass-through, no objections.
>
> Does this mean PATCH v13 looks good to you?
No idea, sorry. It's the middle of the merge window, as my bot told
you, so I can't do anything with changes until after it is over. Please
relax, there is no rush here...
greg k-h