| Message ID | 20221004003811.4075765-1-jrosenth@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v12] firmware: google: Implement cbmem in sysfs driver | expand |
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 <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> > --- > 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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org> > +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 <jrosenth@chromium.org>"); > +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 <jrosenth@chromium.org> > > +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 <jrosenth@chromium.org> > > +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 <jrosenth@chromium.org> > > +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
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 <jrosenth@chromium.org> +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 <jrosenth@chromium.org> +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 <jrosenth@chromium.org> +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 <jrosenth@chromium.org> +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 <jrosenth@chromium.org> +Description: + This is the CBMEM id corresponding to the entry. + +What: /sys/bus/coreboot/devices/cbmem-<id>/mem +Date: August 2022 +Contact: Jack Rosenthal <jrosenth@chromium.org> +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 <jrosenth@chromium.org>"); +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 *);
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 <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> --- 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