| Message ID | 20220804142856.306032-1-jrosenth@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7] firmware: google: Implement cbmem in sysfs driver | expand |
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);
> 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).
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>/...
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?
> 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.
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`
> 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).
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; }; };