Messages in this thread | | | Date | Fri, 30 Sep 2022 08:32:46 +0200 | From | Greg KH <> | Subject | Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver |
| |
On Thu, Sep 29, 2022 at 05:44:32PM -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/firmware/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/firmware/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 v11: > * Changed /sys/firmware/coreboot/cbmem -> /sys/firmware/cbmem > * cbmem.c uses attribute groups to initialize files, which is much > cleaner. The attributes are added under the device kobject, which > is now symlinked into /sys/firmware/cbmem.
symlink? Ick, no, do not do that at all please.
As these are device attributes, just stick with them. Don't do a crazy symlink into a non-device-attribute portion of the sysfs tree, by doing that you break all userspace tools and stuff like libudev will never even see these attributes.
> * Changed documentation text as suggested by greg k-h > > .../ABI/testing/sysfs-firmware-cbmem | 43 +++++ > drivers/firmware/google/Kconfig | 8 + > drivers/firmware/google/Makefile | 3 + > drivers/firmware/google/cbmem.c | 180 ++++++++++++++++++ > drivers/firmware/google/coreboot_table.h | 16 ++ > 5 files changed, 250 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-firmware-cbmem > create mode 100644 drivers/firmware/google/cbmem.c > > diff --git a/Documentation/ABI/testing/sysfs-firmware-cbmem b/Documentation/ABI/testing/sysfs-firmware-cbmem > new file mode 100644 > index 000000000000..f769104ac4cd > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-firmware-cbmem > @@ -0,0 +1,43 @@ > +What: /sys/firmware/cbmem/ > +Date: August 2022 > +Contact: Jack Rosenthal <jrosenth@chromium.org> > +Description: > + Coreboot provides a variety of data structures in CBMEM. This > + directory contains each CBMEM entry, which can be found via > + Coreboot tables.
What happened to the coreboot name?
Why cbmem? What is CBMEM?
And just stick with the attributes under the cbmem coreboot device in the device tree, don't use /sys/firmware/.
Also, I asked before, but some note about "exposing all of these bios values to userspace is not a security issue at all" would be nice, if only to point at in a few years and say "wow we were naive"...
thanks,
greg k-h
| |