Messages in this thread | | | Date | Tue, 8 Oct 2019 14:03:17 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 1/2] firmware: coreboot: Export the binary FMAP |
| |
On Tue, Oct 08, 2019 at 01:53:25PM +0200, patrick.rudolph@9elements.com wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Expose coreboot's binary FMAP[1] to /sys/firmware/fmap. > > coreboot copies the FMAP to a CBMEM buffer at boot since CB:35377[2], > allowing an architecture independ way of exposing the FMAP to userspace. > > Will be used by fwupd[3] to determine the current firmware layout. > > [1]: https://doc.coreboot.org/lib/flashmap.html > [2]: https://review.coreboot.org/c/coreboot/+/35377 > [3]: https://fwupd.org/ > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > --- > drivers/firmware/google/Kconfig | 8 ++ > drivers/firmware/google/Makefile | 1 + > drivers/firmware/google/fmap-coreboot.c | 156 ++++++++++++++++++++++ > drivers/firmware/google/fmap-coreboot.h | 13 ++ > drivers/firmware/google/fmap_serialized.h | 59 ++++++++ > 5 files changed, 237 insertions(+) > create mode 100644 drivers/firmware/google/fmap-coreboot.c > create mode 100644 drivers/firmware/google/fmap-coreboot.h > create mode 100644 drivers/firmware/google/fmap_serialized.h > > diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig > index a3a6ca659ffa..5fbbd7b8fef6 100644 > --- a/drivers/firmware/google/Kconfig > +++ b/drivers/firmware/google/Kconfig > @@ -74,4 +74,12 @@ config GOOGLE_VPD > This option enables the kernel to expose the content of Google VPD > under /sys/firmware/vpd. > > +config GOOGLE_FMAP > + tristate "Coreboot FMAP access" > + depends on GOOGLE_COREBOOT_TABLE > + help > + This option enables the kernel to search for a Google FMAP in > + the coreboot table. If found, this binary file is exported to userland > + in the file /sys/firmware/fmap. > + > endif # GOOGLE_FIRMWARE > diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile > index d17caded5d88..6d31fe167700 100644 > --- a/drivers/firmware/google/Makefile > +++ b/drivers/firmware/google/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT) += framebuffer-coreboot.o > obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o > obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o > obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o > +obj-$(CONFIG_GOOGLE_FMAP) += fmap-coreboot.o > > vpd-sysfs-y := vpd.o vpd_decode.o > obj-$(CONFIG_GOOGLE_VPD) += vpd-sysfs.o > diff --git a/drivers/firmware/google/fmap-coreboot.c b/drivers/firmware/google/fmap-coreboot.c > new file mode 100644 > index 000000000000..14050030ebc6 > --- /dev/null > +++ b/drivers/firmware/google/fmap-coreboot.c > @@ -0,0 +1,156 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * fmap-coreboot.c > + * > + * Exports the binary FMAP through coreboot table. > + * > + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com> > + * Copyright 2017 Google Inc. > + * Copyright 2019 9elements Agency GmbH <patrick.rudolph@9elements.com> > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mm.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/string.h> > +#include <linux/io.h> > + > +#include "coreboot_table.h" > +#include "fmap_serialized.h" > + > +#define CB_TAG_FMAP 0x37 > + > +static void *fmap; > +static u32 fmap_size; > + > +/* > + * Convert FMAP region to name. > + * The caller has to free the string. > + * Return NULL if no containing region was found. > + */ > +const char *coreboot_fmap_region_to_name(const u32 start, const u32 size) > +{ > + const char *name = NULL; > + struct fmap *iter; > + u32 size_old = ~0; > + int i; > + > + iter = fmap; > + /* Find smallest containing region */ > + for (i = 0; i < iter->nareas && fmap; i++) { > + if (iter->areas[i].offset <= start && > + iter->areas[i].size >= size && > + iter->areas[i].size <= size_old) { > + size_old = iter->areas[i].size; > + name = iter->areas[i].name; > + } > + } > + > + if (name) > + return kstrdup(name, GFP_KERNEL); > + return NULL; > +} > +EXPORT_SYMBOL(coreboot_fmap_region_to_name); > + > +static ssize_t fmap_read(struct file *filp, struct kobject *kobp, > + struct bin_attribute *bin_attr, char *buf, > + loff_t pos, size_t count) > +{ > + if (!fmap) > + return -ENODEV; > + > + return memory_read_from_buffer(buf, count, &pos, fmap, fmap_size); > +} > + > +static int fmap_probe(struct coreboot_device *dev) > +{ > + struct lb_cbmem_ref *cbmem_ref = &dev->cbmem_ref; > + struct fmap *header; > + > + if (!cbmem_ref) > + return -ENODEV; > + > + header = memremap(cbmem_ref->cbmem_addr, sizeof(*header), MEMREMAP_WB); > + if (!header) { > + pr_warn("coreboot: Failed to remap FMAP\n");
Doesn't memremap print an error if it fails?
> + return -ENOMEM; > + } > + > + /* Validate FMAP signature */ > + if (memcmp(header->signature, FMAP_SIGNATURE, > + sizeof(header->signature))) { > + pr_warn("coreboot: FMAP signature mismatch\n");
How can this happen? Shouldn't it be an error?
> + memunmap(header); > + return -ENODEV; > + } > + > + /* Validate FMAP version */ > + if (header->ver_major != FMAP_VER_MAJOR) { > + pr_warn("coreboot: FMAP version not supported\n");
error?
And why are these not dev_err() and friends?
> + memunmap(header); > + return -ENODEV; > + } > + > + pr_info("coreboot: Got valid FMAP v%u.%u for 0x%x byte ROM\n", > + header->ver_major, header->ver_minor, header->size);
Do not be noisy if all goes well. This should be debugging only.
> + > + fmap_size = sizeof(*header) + header->nareas * sizeof(struct fmap_area); > + memunmap(header); > + > + fmap = devm_memremap(&dev->dev, cbmem_ref->cbmem_addr, fmap_size, > + MEMREMAP_WB); > + if (!fmap) { > + pr_warn("coreboot: Failed to remap FMAP\n");
Same here as above.
> + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int fmap_remove(struct coreboot_device *dev) > +{ > + struct platform_device *pdev = dev_get_drvdata(&dev->dev); > + > + platform_device_unregister(pdev); > + > + return 0; > +} > + > +static struct coreboot_driver fmap_driver = { > + .probe = fmap_probe, > + .remove = fmap_remove, > + .drv = { > + .name = "fmap", > + }, > + .tag = CB_TAG_FMAP, > +}; > + > +static struct bin_attribute fmap_bin_attr = { > + .attr = {.name = "fmap", .mode = 0444}, > + .read = fmap_read, > +};
BIN_ATTR_RO()?
And you forgot the Documentation/ABI/ update for your new sysfs file :(
I'm guessing all of these same issues are in your 2/2 patch, so I'll let you fix them up and resend the series.
thanks,
greg k-h
| |