[PATCH v3 0/2] firmware: google: Expose coreboot tables and CBMEM

[PATCH v3 0/2] firmware: google: Expose coreboot tables and CBMEM

[patrick.rudolph]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

As user land tools currently use /dev/mem to access coreboot tables and
CBMEM, provide a better way by using read-only sysfs attributes.

Unconditionally expose all tables and buffers making future changes in
coreboot possible without modifying a kernel driver.

Changes in v2:
 - Add ABI documentation
 - Add 0x prefix on hex values
 - Remove wrong ioremap hint as found by CI

Changes in v3:
 - Use BIN_ATTR_RO

Patrick Rudolph (2):
  firmware: google: Expose CBMEM over sysfs
  firmware: google: Expose coreboot tables over sysfs

Patrick Rudolph (2):
  firmware: google: Expose CBMEM over sysfs
  firmware: google: Expose coreboot tables over sysfs

 Documentation/ABI/stable/sysfs-bus-coreboot |  73 +++++++++
 drivers/firmware/google/Kconfig             |   9 ++
 drivers/firmware/google/Makefile            |   1 +
 drivers/firmware/google/cbmem-coreboot.c    | 159 ++++++++++++++++++++
 drivers/firmware/google/coreboot_table.c    |  57 +++++++
 drivers/firmware/google/coreboot_table.h    |  13 ++
 6 files changed, 312 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-coreboot
 create mode 100644 drivers/firmware/google/cbmem-coreboot.c

-- 
2.21.0

[PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[patrick.rudolph]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Make all CBMEM buffers available to userland. This is useful for tools
that are currently using /dev/mem.

Make the id, size and address available, as well as the raw table data.

Tools can easily scan the right CBMEM buffer by reading
/sys/bus/coreboot/drivers/cbmem/coreboot*/cbmem_attributes/id
or
/sys/bus/coreboot/devices/coreboot*/cbmem_attributes/id

The binary table data can then be read from
/sys/bus/coreboot/drivers/cbmem/coreboot*/cbmem_attributes/data
or
/sys/bus/coreboot/devices/coreboot*/cbmem_attributes/data

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 -v2:
	- Add ABI documentation
	- Add 0x prefix on hex values
 -v3:
	- Use BIN_ATTR_RO
---
 Documentation/ABI/stable/sysfs-bus-coreboot |  43 ++++++
 drivers/firmware/google/Kconfig             |   9 ++
 drivers/firmware/google/Makefile            |   1 +
 drivers/firmware/google/cbmem-coreboot.c    | 159 ++++++++++++++++++++
 drivers/firmware/google/coreboot_table.h    |  13 ++
 5 files changed, 225 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-coreboot
 create mode 100644 drivers/firmware/google/cbmem-coreboot.c

diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/ABI/stable/sysfs-bus-coreboot
new file mode 100644
index 000000000000..1b04b8abc858
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-coreboot
@@ -0,0 +1,43 @@
+What:		/sys/bus/coreboot/devices/.../cbmem_attributes/id
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		cbmem_attributes/id if the device corresponds to a CBMEM
+		buffer.
+		The file holds an ASCII representation of the CBMEM ID in hex
+		(e.g. 0xdeadbeef).
+
+What:		/sys/bus/coreboot/devices/.../cbmem_attributes/size
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		cbmem_attributes/size if the device corresponds to a CBMEM
+		buffer.
+		The file holds an representation as decimal number of the
+		CBMEM buffer size (e.g. 64).
+
+What:		/sys/bus/coreboot/devices/.../cbmem_attributes/address
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		cbmem_attributes/address if the device corresponds to a CBMEM
+		buffer.
+		The file holds an ASCII representation of the physical address
+		of the CBMEM buffer in hex (e.g. 0x000000008000d000).
+
+What:		/sys/bus/coreboot/devices/.../cbmem_attributes/data
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		cbmem_attributes/data if the device corresponds to a CBMEM
+		buffer.
+		The file holds a read-only binary representation of the CBMEM
+		buffer.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index a3a6ca659ffa..11a67c397ab3 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -58,6 +58,15 @@ config GOOGLE_FRAMEBUFFER_COREBOOT
 	  This option enables the kernel to search for a framebuffer in
 	  the coreboot table.  If found, it is registered with simplefb.
 
+config GOOGLE_CBMEM_COREBOOT
+	tristate "Coreboot CBMEM access"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option exposes all available CBMEM buffers to userland.
+	  The CBMEM id, size and address as well as the raw table data
+	  are exported as sysfs attributes of the corresponding coreboot
+	  table.
+
 config GOOGLE_MEMCONSOLE_COREBOOT
 	tristate "Firmware Memory Console"
 	depends on GOOGLE_COREBOOT_TABLE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..62053cd6d058 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_GOOGLE_SMI)		+= gsmi.o
 obj-$(CONFIG_GOOGLE_COREBOOT_TABLE)        += coreboot_table.o
+obj-$(CONFIG_GOOGLE_CBMEM_COREBOOT)        += cbmem-coreboot.o
 obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
diff --git a/drivers/firmware/google/cbmem-coreboot.c b/drivers/firmware/google/cbmem-coreboot.c
new file mode 100644
index 000000000000..c67651a9c287
--- /dev/null
+++ b/drivers/firmware/google/cbmem-coreboot.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * fmap-coreboot.c
+ *
+ * Exports CBMEM as attributes in sysfs.
+ *
+ * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
+ * Copyright 2017 Google Inc.
+ * Copyright 2019 9elements Agency GmbH
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include "coreboot_table.h"
+
+#define CB_TAG_CBMEM_ENTRY 0x31
+
+struct cb_priv {
+	void *remap;
+	struct lb_cbmem_entry entry;
+};
+
+static ssize_t id_show(struct device *dev,
+		       struct device_attribute *attr, char *buffer)
+{
+	const struct cb_priv *priv;
+
+	priv = (const struct cb_priv *)dev_get_platdata(dev);
+
+	return sprintf(buffer, "0x%08x\n", priv->entry.id);
+}
+
+static ssize_t size_show(struct device *dev,
+			 struct device_attribute *attr, char *buffer)
+{
+	const struct cb_priv *priv;
+
+	priv = (const struct cb_priv *)dev_get_platdata(dev);
+
+	return sprintf(buffer, "%u\n", priv->entry.size);
+}
+
+static ssize_t address_show(struct device *dev,
+			    struct device_attribute *attr, char *buffer)
+{
+	const struct cb_priv *priv;
+
+	priv = (const struct cb_priv *)dev_get_platdata(dev);
+
+	return sprintf(buffer, "0x%016llx\n", priv->entry.address);
+}
+
+static DEVICE_ATTR_RO(id);
+static DEVICE_ATTR_RO(size);
+static DEVICE_ATTR_RO(address);
+
+static struct attribute *cb_mem_attrs[] = {
+	&dev_attr_address.attr,
+	&dev_attr_id.attr,
+	&dev_attr_size.attr,
+	NULL
+};
+
+static ssize_t data_read(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *bin_attr,
+			 char *buffer, loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	const struct cb_priv *priv;
+
+	priv = (const struct cb_priv *)dev_get_platdata(dev);
+
+	return memory_read_from_buffer(buffer, count, &offset,
+				       priv->remap, priv->entry.size);
+}
+
+static BIN_ATTR_RO(data, 0);
+
+static struct bin_attribute *cb_mem_bin_attrs[] = {
+	&bin_attr_data,
+	NULL
+};
+
+static struct attribute_group cb_mem_attr_group = {
+	.name = "cbmem_attributes",
+	.attrs = cb_mem_attrs,
+	.bin_attrs = cb_mem_bin_attrs,
+};
+
+static int cbmem_probe(struct coreboot_device *cdev)
+{
+	struct device *dev = &cdev->dev;
+	struct cb_priv *priv;
+	int err;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
+
+	priv->remap = memremap(priv->entry.address,
+			       priv->entry.entry_size, MEMREMAP_WB);
+	if (!priv->remap) {
+		err = -ENOMEM;
+		goto failure;
+	}
+
+	err = sysfs_create_group(&dev->kobj, &cb_mem_attr_group);
+	if (err) {
+		dev_err(dev, "failed to create sysfs attributes: %d\n", err);
+		memunmap(priv->remap);
+		goto failure;
+	}
+
+	dev->platform_data = priv;
+
+	return 0;
+failure:
+	kfree(priv);
+	return err;
+}
+
+static int cbmem_remove(struct coreboot_device *cdev)
+{
+	struct device *dev = &cdev->dev;
+	const struct cb_priv *priv;
+
+	priv = (const struct cb_priv *)dev_get_platdata(dev);
+
+	sysfs_remove_group(&dev->kobj, &cb_mem_attr_group);
+
+	if (priv)
+		memunmap(priv->remap);
+	kfree(priv);
+
+	dev->platform_data = NULL;
+
+	return 0;
+}
+
+static struct coreboot_driver cbmem_driver = {
+	.probe = cbmem_probe,
+	.remove = cbmem_remove,
+	.drv = {
+		.name = "cbmem",
+	},
+	.tag = CB_TAG_CBMEM_ENTRY,
+};
+
+module_coreboot_driver(cbmem_driver);
+
+MODULE_AUTHOR("9elements Agency GmbH");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 7b7b4a6eedda..506048c724d8 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -59,6 +59,18 @@ struct lb_framebuffer {
 	u8  reserved_mask_size;
 };
 
+/* There can be more than one of these records as there is one per cbmem entry.
+ * Describes a buffer in memory containing runtime data.
+ */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
 /* A device, additionally with information from coreboot. */
 struct coreboot_device {
 	struct device dev;
@@ -66,6 +78,7 @@ struct coreboot_device {
 		struct coreboot_table_entry entry;
 		struct lb_cbmem_ref cbmem_ref;
 		struct lb_framebuffer framebuffer;
+		struct lb_cbmem_entry cbmem_entry;
 	};
 };
 
-- 
2.21.0

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Julius Werner]

> +static int cbmem_probe(struct coreboot_device *cdev)
> +{
> +       struct device *dev = &cdev->dev;
> +       struct cb_priv *priv;
> +       int err;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> +
> +       priv->remap = memremap(priv->entry.address,
> +                              priv->entry.entry_size, MEMREMAP_WB);

We've just been discussing some problems with CBMEM areas and memory
mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
(at least not the "small" entries), but the kernel can only assign
memory attributes for a page at a time (and refuses to map the same
area twice with two different memory types, for good reason). So if
CBMEM entries sharing a page are mapped as writeback by one driver but
uncached by the other, things break.

There are some CBMEM entries that need to be mapped uncached (e.g. the
ACPI UCSI table, which isn't even handled by anything using this CBMEM
code) and others for which it would make more sense (e.g. the memory
console, where firmware may add more lines at runtime), but I don't
think there are any regions that really *need* to be writeback. None
of the stuff accessing these areas should access them often enough
that caching matters, and I think it's generally more common to map
firmware memory areas as uncached anyway. So how about we standardize
on mapping it all uncached to avoid any attribute clashes? (That would
mean changing the existing VPD and memconsole drivers to use
ioremap(), too.)

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Mat King]

On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > +static int cbmem_probe(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       struct cb_priv *priv;
> > +       int err;
> > +
> > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > +
> > +       priv->remap = memremap(priv->entry.address,
> > +                              priv->entry.entry_size, MEMREMAP_WB);
>
> We've just been discussing some problems with CBMEM areas and memory
> mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> (at least not the "small" entries), but the kernel can only assign
> memory attributes for a page at a time (and refuses to map the same
> area twice with two different memory types, for good reason). So if
> CBMEM entries sharing a page are mapped as writeback by one driver but
> uncached by the other, things break.
>
> There are some CBMEM entries that need to be mapped uncached (e.g. the
> ACPI UCSI table, which isn't even handled by anything using this CBMEM
> code) and others for which it would make more sense (e.g. the memory
> console, where firmware may add more lines at runtime), but I don't
> think there are any regions that really *need* to be writeback. None
> of the stuff accessing these areas should access them often enough
> that caching matters, and I think it's generally more common to map
> firmware memory areas as uncached anyway. So how about we standardize
> on mapping it all uncached to avoid any attribute clashes? (That would
> mean changing the existing VPD and memconsole drivers to use
> ioremap(), too.)

I don't think that uncached would work here either because the acpi
driver will have already mapped some of these regions as write-back
before this driver is loaded so the mapping will fail.

Perhaps the best way to make this work is to not call memremap at all
in the probe function but instead call memremap and memunmap every
time that data_read is called. memremap can take multiple caching mode
flags and will try each until one succeeds. So if you call it with
MEMREMAP_WB | MEMREMAP_WT in data_read it should succeed no matter how
it has been mapped by other drivers which should already be loaded
when it is called.

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Stephen Boyd]

Quoting Mat King (2019-12-13 13:31:46)
> On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > +{
> > > +       struct device *dev = &cdev->dev;
> > > +       struct cb_priv *priv;
> > > +       int err;
> > > +
> > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > +
> > > +       priv->remap = memremap(priv->entry.address,
> > > +                              priv->entry.entry_size, MEMREMAP_WB);
> >
> > We've just been discussing some problems with CBMEM areas and memory
> > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > (at least not the "small" entries), but the kernel can only assign
> > memory attributes for a page at a time (and refuses to map the same
> > area twice with two different memory types, for good reason). So if
> > CBMEM entries sharing a page are mapped as writeback by one driver but
> > uncached by the other, things break.
> >
> > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > code) and others for which it would make more sense (e.g. the memory
> > console, where firmware may add more lines at runtime), but I don't
> > think there are any regions that really *need* to be writeback. None
> > of the stuff accessing these areas should access them often enough
> > that caching matters, and I think it's generally more common to map
> > firmware memory areas as uncached anyway. So how about we standardize
> > on mapping it all uncached to avoid any attribute clashes? (That would
> > mean changing the existing VPD and memconsole drivers to use
> > ioremap(), too.)
> 
> I don't think that uncached would work here either because the acpi
> driver will have already mapped some of these regions as write-back
> before this driver is loaded so the mapping will fail.

Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
that right? And on ACPI based systems is this I/O memory or just some
carved out memory region that is used to communicate something to the
ACPI firmware? From looking at the ucsi driver it seems like it should
be mapped with memremap() instead of ioremap() given that it's not
actual I/O memory that has any sort of memory barrier or access width
constraints. It looks more like some sort of memory region that is being
copied into and out of while triggering some DSM. Can it at least be
memremap()ed with MEMREMAP_WT?

It also looks like this sort of problem has come up before, see commit
1f9f9d168ce6 ("usb: typec: ucsi: acpi: Workaround for cache mode
issue"). Ugh.

> 
> Perhaps the best way to make this work is to not call memremap at all
> in the probe function but instead call memremap and memunmap every
> time that data_read is called. memremap can take multiple caching mode
> flags and will try each until one succeeds. So if you call it with
> MEMREMAP_WB | MEMREMAP_WT in data_read it should succeed no matter how
> it has been mapped by other drivers which should already be loaded
> when it is called.

I've been wanting to change memremap() to be a "just give me memory"
type of API but haven't finished that task. It got hung up on mapping
memory specially for UEFI framebuffers to match whatever the UEFI mmap
table tells us.

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Mat King]

On Tue, Dec 17, 2019 at 12:02 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Mat King (2019-12-13 13:31:46)
> > On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > > +{
> > > > +       struct device *dev = &cdev->dev;
> > > > +       struct cb_priv *priv;
> > > > +       int err;
> > > > +
> > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > +       if (!priv)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > > +
> > > > +       priv->remap = memremap(priv->entry.address,
> > > > +                              priv->entry.entry_size, MEMREMAP_WB);
> > >
> > > We've just been discussing some problems with CBMEM areas and memory
> > > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > > (at least not the "small" entries), but the kernel can only assign
> > > memory attributes for a page at a time (and refuses to map the same
> > > area twice with two different memory types, for good reason). So if
> > > CBMEM entries sharing a page are mapped as writeback by one driver but
> > > uncached by the other, things break.
> > >
> > > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > > code) and others for which it would make more sense (e.g. the memory
> > > console, where firmware may add more lines at runtime), but I don't
> > > think there are any regions that really *need* to be writeback. None
> > > of the stuff accessing these areas should access them often enough
> > > that caching matters, and I think it's generally more common to map
> > > firmware memory areas as uncached anyway. So how about we standardize
> > > on mapping it all uncached to avoid any attribute clashes? (That would
> > > mean changing the existing VPD and memconsole drivers to use
> > > ioremap(), too.)
> >
> > I don't think that uncached would work here either because the acpi
> > driver will have already mapped some of these regions as write-back
> > before this driver is loaded so the mapping will fail.
>
> Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
> that right? And on ACPI based systems is this I/O memory or just some
> carved out memory region that is used to communicate something to the
> ACPI firmware? From looking at the ucsi driver it seems like it should
> be mapped with memremap() instead of ioremap() given that it's not
> actual I/O memory that has any sort of memory barrier or access width
> constraints. It looks more like some sort of memory region that is being
> copied into and out of while triggering some DSM. Can it at least be
> memremap()ed with MEMREMAP_WT?

Yes this is the ucsi_acpi.c driver that has caused this issue to come
up. It does just use a region of memory carved in the BIOS out for the
purpose of this device. The kernel can write to this memory and call a
_DSM to push data to an EC or call the _DSM to pull from the EC into
this memory region. See
https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/bios-implementation-of-ucsi.pdf
. The driver is very explicit about using uncached memory and I
suspect that is why memremap() was not used, but I am not sure why
uncahed memory is needed. The only consumers of this memory are the
driver itself and the ACPI asl code in the _DSM which as far as I know
is being exectued by the kernel directly. Are there any other reasons
to use uncached memory when dealing with ACPI asl code?

>
> It also looks like this sort of problem has come up before, see commit
> 1f9f9d168ce6 ("usb: typec: ucsi: acpi: Workaround for cache mode
> issue"). Ugh.
>
> >
> > Perhaps the best way to make this work is to not call memremap at all
> > in the probe function but instead call memremap and memunmap every
> > time that data_read is called. memremap can take multiple caching mode
> > flags and will try each until one succeeds. So if you call it with
> > MEMREMAP_WB | MEMREMAP_WT in data_read it should succeed no matter how
> > it has been mapped by other drivers which should already be loaded
> > when it is called.
>
> I've been wanting to change memremap() to be a "just give me memory"
> type of API but haven't finished that task. It got hung up on mapping
> memory specially for UEFI framebuffers to match whatever the UEFI mmap
> table tells us.
>

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Heikki Krogerus]

On Tue, Dec 17, 2019 at 01:16:33PM -0700, Mat King wrote:
> On Tue, Dec 17, 2019 at 12:02 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Mat King (2019-12-13 13:31:46)
> > > On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > > > +{
> > > > > +       struct device *dev = &cdev->dev;
> > > > > +       struct cb_priv *priv;
> > > > > +       int err;
> > > > > +
> > > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > > +       if (!priv)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > > > +
> > > > > +       priv->remap = memremap(priv->entry.address,
> > > > > +                              priv->entry.entry_size, MEMREMAP_WB);
> > > >
> > > > We've just been discussing some problems with CBMEM areas and memory
> > > > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > > > (at least not the "small" entries), but the kernel can only assign
> > > > memory attributes for a page at a time (and refuses to map the same
> > > > area twice with two different memory types, for good reason). So if
> > > > CBMEM entries sharing a page are mapped as writeback by one driver but
> > > > uncached by the other, things break.
> > > >
> > > > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > > > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > > > code) and others for which it would make more sense (e.g. the memory
> > > > console, where firmware may add more lines at runtime), but I don't
> > > > think there are any regions that really *need* to be writeback. None
> > > > of the stuff accessing these areas should access them often enough
> > > > that caching matters, and I think it's generally more common to map
> > > > firmware memory areas as uncached anyway. So how about we standardize
> > > > on mapping it all uncached to avoid any attribute clashes? (That would
> > > > mean changing the existing VPD and memconsole drivers to use
> > > > ioremap(), too.)
> > >
> > > I don't think that uncached would work here either because the acpi
> > > driver will have already mapped some of these regions as write-back
> > > before this driver is loaded so the mapping will fail.
> >
> > Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
> > that right? And on ACPI based systems is this I/O memory or just some
> > carved out memory region that is used to communicate something to the
> > ACPI firmware? From looking at the ucsi driver it seems like it should
> > be mapped with memremap() instead of ioremap() given that it's not
> > actual I/O memory that has any sort of memory barrier or access width
> > constraints. It looks more like some sort of memory region that is being
> > copied into and out of while triggering some DSM. Can it at least be
> > memremap()ed with MEMREMAP_WT?
> 
> Yes this is the ucsi_acpi.c driver that has caused this issue to come
> up. It does just use a region of memory carved in the BIOS out for the
> purpose of this device. The kernel can write to this memory and call a
> _DSM to push data to an EC or call the _DSM to pull from the EC into
> this memory region. See
> https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/bios-implementation-of-ucsi.pdf
> . The driver is very explicit about using uncached memory and I
> suspect that is why memremap() was not used, but I am not sure why
> uncahed memory is needed. The only consumers of this memory are the
> driver itself and the ACPI asl code in the _DSM which as far as I know
> is being exectued by the kernel directly. Are there any other reasons
> to use uncached memory when dealing with ACPI asl code?

The reason why I did not use memremap() was because I was convinced
that there will soon be physical devices such as PD controllers that
supply the interface, and with those the memory resource given to the
driver would be real bus memory. But that was already years ago,
and there still are no such devices that I know of, so if you guys
want to change the driver so that it uses memremap() instead of
ioremap(), I'm not going to be against it. But just be warned: We can
not guarantee that there isn't going to be IO side effects in every
case.

But why is the UCSI ACPI mailbox a problem for you guys? Why do you
have the UCSI ACPI device object in your ACPI tables in the first
place?


thanks,

-- 
heikki

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Mat King]

On Wed, Dec 18, 2019 at 2:47 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Dec 17, 2019 at 01:16:33PM -0700, Mat King wrote:
> > On Tue, Dec 17, 2019 at 12:02 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Mat King (2019-12-13 13:31:46)
> > > > On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > > > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > > > > +{
> > > > > > +       struct device *dev = &cdev->dev;
> > > > > > +       struct cb_priv *priv;
> > > > > > +       int err;
> > > > > > +
> > > > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > > > +       if (!priv)
> > > > > > +               return -ENOMEM;
> > > > > > +
> > > > > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > > > > +
> > > > > > +       priv->remap = memremap(priv->entry.address,
> > > > > > +                              priv->entry.entry_size, MEMREMAP_WB);
> > > > >
> > > > > We've just been discussing some problems with CBMEM areas and memory
> > > > > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > > > > (at least not the "small" entries), but the kernel can only assign
> > > > > memory attributes for a page at a time (and refuses to map the same
> > > > > area twice with two different memory types, for good reason). So if
> > > > > CBMEM entries sharing a page are mapped as writeback by one driver but
> > > > > uncached by the other, things break.
> > > > >
> > > > > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > > > > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > > > > code) and others for which it would make more sense (e.g. the memory
> > > > > console, where firmware may add more lines at runtime), but I don't
> > > > > think there are any regions that really *need* to be writeback. None
> > > > > of the stuff accessing these areas should access them often enough
> > > > > that caching matters, and I think it's generally more common to map
> > > > > firmware memory areas as uncached anyway. So how about we standardize
> > > > > on mapping it all uncached to avoid any attribute clashes? (That would
> > > > > mean changing the existing VPD and memconsole drivers to use
> > > > > ioremap(), too.)
> > > >
> > > > I don't think that uncached would work here either because the acpi
> > > > driver will have already mapped some of these regions as write-back
> > > > before this driver is loaded so the mapping will fail.
> > >
> > > Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
> > > that right? And on ACPI based systems is this I/O memory or just some
> > > carved out memory region that is used to communicate something to the
> > > ACPI firmware? From looking at the ucsi driver it seems like it should
> > > be mapped with memremap() instead of ioremap() given that it's not
> > > actual I/O memory that has any sort of memory barrier or access width
> > > constraints. It looks more like some sort of memory region that is being
> > > copied into and out of while triggering some DSM. Can it at least be
> > > memremap()ed with MEMREMAP_WT?
> >
> > Yes this is the ucsi_acpi.c driver that has caused this issue to come
> > up. It does just use a region of memory carved in the BIOS out for the
> > purpose of this device. The kernel can write to this memory and call a
> > _DSM to push data to an EC or call the _DSM to pull from the EC into
> > this memory region. See
> > https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/bios-implementation-of-ucsi.pdf
> > . The driver is very explicit about using uncached memory and I
> > suspect that is why memremap() was not used, but I am not sure why
> > uncahed memory is needed. The only consumers of this memory are the
> > driver itself and the ACPI asl code in the _DSM which as far as I know
> > is being exectued by the kernel directly. Are there any other reasons
> > to use uncached memory when dealing with ACPI asl code?
>
> The reason why I did not use memremap() was because I was convinced
> that there will soon be physical devices such as PD controllers that
> supply the interface, and with those the memory resource given to the
> driver would be real bus memory. But that was already years ago,
> and there still are no such devices that I know of, so if you guys
> want to change the driver so that it uses memremap() instead of
> ioremap(), I'm not going to be against it. But just be warned: We can
> not guarantee that there isn't going to be IO side effects in every
> case.

I am a little confused how this hypothetical PD controller would look
with regards to the ACPI table. Would it still have an OperationRegion
for the MMIO address of the controllers mailbox? Would the _CRS point
to the MMIO of the mailbox directly or would it still use physical
memory? If it is pointing to the MMIO mailbox is the _DSM essentially
a noop?

>
> But why is the UCSI ACPI mailbox a problem for you guys? Why do you
> have the UCSI ACPI device object in your ACPI tables in the first
> place?

The problem is that with our UCSI implementation in coreboot the 48
byte mailbox sometimes is in the same page as memory that gets
memremap()ed as write-back before the ucsi driver is loaded and when
it is loaded the ioremap fails.

>
>
> thanks,

>
> --
> heikki

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Heikki Krogerus]

On Wed, Dec 18, 2019 at 04:35:29PM -0700, Mat King wrote:
> On Wed, Dec 18, 2019 at 2:47 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Dec 17, 2019 at 01:16:33PM -0700, Mat King wrote:
> > > On Tue, Dec 17, 2019 at 12:02 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Mat King (2019-12-13 13:31:46)
> > > > > On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > > > > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > > > > > +{
> > > > > > > +       struct device *dev = &cdev->dev;
> > > > > > > +       struct cb_priv *priv;
> > > > > > > +       int err;
> > > > > > > +
> > > > > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > > > > +       if (!priv)
> > > > > > > +               return -ENOMEM;
> > > > > > > +
> > > > > > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > > > > > +
> > > > > > > +       priv->remap = memremap(priv->entry.address,
> > > > > > > +                              priv->entry.entry_size, MEMREMAP_WB);
> > > > > >
> > > > > > We've just been discussing some problems with CBMEM areas and memory
> > > > > > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > > > > > (at least not the "small" entries), but the kernel can only assign
> > > > > > memory attributes for a page at a time (and refuses to map the same
> > > > > > area twice with two different memory types, for good reason). So if
> > > > > > CBMEM entries sharing a page are mapped as writeback by one driver but
> > > > > > uncached by the other, things break.
> > > > > >
> > > > > > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > > > > > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > > > > > code) and others for which it would make more sense (e.g. the memory
> > > > > > console, where firmware may add more lines at runtime), but I don't
> > > > > > think there are any regions that really *need* to be writeback. None
> > > > > > of the stuff accessing these areas should access them often enough
> > > > > > that caching matters, and I think it's generally more common to map
> > > > > > firmware memory areas as uncached anyway. So how about we standardize
> > > > > > on mapping it all uncached to avoid any attribute clashes? (That would
> > > > > > mean changing the existing VPD and memconsole drivers to use
> > > > > > ioremap(), too.)
> > > > >
> > > > > I don't think that uncached would work here either because the acpi
> > > > > driver will have already mapped some of these regions as write-back
> > > > > before this driver is loaded so the mapping will fail.
> > > >
> > > > Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
> > > > that right? And on ACPI based systems is this I/O memory or just some
> > > > carved out memory region that is used to communicate something to the
> > > > ACPI firmware? From looking at the ucsi driver it seems like it should
> > > > be mapped with memremap() instead of ioremap() given that it's not
> > > > actual I/O memory that has any sort of memory barrier or access width
> > > > constraints. It looks more like some sort of memory region that is being
> > > > copied into and out of while triggering some DSM. Can it at least be
> > > > memremap()ed with MEMREMAP_WT?
> > >
> > > Yes this is the ucsi_acpi.c driver that has caused this issue to come
> > > up. It does just use a region of memory carved in the BIOS out for the
> > > purpose of this device. The kernel can write to this memory and call a
> > > _DSM to push data to an EC or call the _DSM to pull from the EC into
> > > this memory region. See
> > > https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/bios-implementation-of-ucsi.pdf
> > > . The driver is very explicit about using uncached memory and I
> > > suspect that is why memremap() was not used, but I am not sure why
> > > uncahed memory is needed. The only consumers of this memory are the
> > > driver itself and the ACPI asl code in the _DSM which as far as I know
> > > is being exectued by the kernel directly. Are there any other reasons
> > > to use uncached memory when dealing with ACPI asl code?
> >
> > The reason why I did not use memremap() was because I was convinced
> > that there will soon be physical devices such as PD controllers that
> > supply the interface, and with those the memory resource given to the
> > driver would be real bus memory. But that was already years ago,
> > and there still are no such devices that I know of, so if you guys
> > want to change the driver so that it uses memremap() instead of
> > ioremap(), I'm not going to be against it. But just be warned: We can
> > not guarantee that there isn't going to be IO side effects in every
> > case.
> 
> I am a little confused how this hypothetical PD controller would look
> with regards to the ACPI table. Would it still have an OperationRegion
> for the MMIO address of the controllers mailbox? Would the _CRS point
> to the MMIO of the mailbox directly or would it still use physical
> memory? If it is pointing to the MMIO mailbox is the _DSM essentially
> a noop?

In this case I believe the idea is exactly like you said. The OpRegion
would just be SystemIO OpRegion instead of SystemMemory OpRegion, and
the _DSD may or may not do something.

The goal appears to be to be able to support the same UCSI device
driver (in Windows) regardless of which component actually supplies
the interface (USB PD controller, EC firmware...), and regardless how
that component is attached to the SOC (I2C, PCI, mailbox...). The real
hardware is meant to be hidden in AML. The problem with that is of
course that UCSI now always depends on ACPI.

I think the main problem is that the mailbox was never separated from
the UCSI interface like it should have. If the ACPI mailbox was
separated from the interface (or generalized) it could have been
handled as a bus in the operating system, fake bus that looks like
SMBus (or I2C). We would have needed a separate "fake I2C host
adapter" driver for the mailbox, but now if a real USB PD controller
I2C slave device supplies the UCSI interface (most of them are I2C
slave devices), the same UCSI driver would still work without any
changes needed.

But I guess it's too late to change that now :-(.

thanks,

-- 
heikki

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[patrick.rudolph]

On Mon, 2019-12-09 at 22:54 -0800, Julius Werner wrote:
> > +static int cbmem_probe(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       struct cb_priv *priv;
> > +       int err;
> > +
> > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv-
> > >entry));
> > +
> > +       priv->remap = memremap(priv->entry.address,
> > +                              priv->entry.entry_size,
> > MEMREMAP_WB);
> 
> We've just been discussing some problems with CBMEM areas and memory
> mapping types in Chrome OS. CBMEM is not guaranteed to be page-
> aligned
> (at least not the "small" entries), but the kernel can only assign
> memory attributes for a page at a time (and refuses to map the same
> area twice with two different memory types, for good reason). So if
> CBMEM entries sharing a page are mapped as writeback by one driver
> but
> uncached by the other, things break.
> 
> There are some CBMEM entries that need to be mapped uncached (e.g.
> the
> ACPI UCSI table, which isn't even handled by anything using this
> CBMEM
> code) and others for which it would make more sense (e.g. the memory
> console, where firmware may add more lines at runtime), but I don't
> think there are any regions that really *need* to be writeback. None
> of the stuff accessing these areas should access them often enough
> that caching matters, and I think it's generally more common to map
> firmware memory areas as uncached anyway. So how about we standardize
> on mapping it all uncached to avoid any attribute clashes? (That
> would
> mean changing the existing VPD and memconsole drivers to use
> ioremap(), too.)

I wasn't aware that CBMEM is used for DMA as there's no such concept in
coreboot yet. For me it looks like the UCSI is regular DRAM mapped as
WB accessed by the ACPI interpreter.
I'll prepare a new patch-set using ioremap in all drivers that access
CBMEM.

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Stephen Boyd]

Quoting patrick.rudolph@9elements.com (2019-12-19 23:12:22)
> On Mon, 2019-12-09 at 22:54 -0800, Julius Werner wrote:
> > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > +{
> > > +       struct device *dev = &cdev->dev;
> > > +       struct cb_priv *priv;
> > > +       int err;
> > > +
> > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv-
> > > >entry));
> > > +
> > > +       priv->remap = memremap(priv->entry.address,
> > > +                              priv->entry.entry_size,
> > > MEMREMAP_WB);
> > 
> > We've just been discussing some problems with CBMEM areas and memory
> > mapping types in Chrome OS. CBMEM is not guaranteed to be page-
> > aligned
> > (at least not the "small" entries), but the kernel can only assign
> > memory attributes for a page at a time (and refuses to map the same
> > area twice with two different memory types, for good reason). So if
> > CBMEM entries sharing a page are mapped as writeback by one driver
> > but
> > uncached by the other, things break.
> > 
> > There are some CBMEM entries that need to be mapped uncached (e.g.
> > the
> > ACPI UCSI table, which isn't even handled by anything using this
> > CBMEM
> > code) and others for which it would make more sense (e.g. the memory
> > console, where firmware may add more lines at runtime), but I don't
> > think there are any regions that really *need* to be writeback. None
> > of the stuff accessing these areas should access them often enough
> > that caching matters, and I think it's generally more common to map
> > firmware memory areas as uncached anyway. So how about we standardize
> > on mapping it all uncached to avoid any attribute clashes? (That
> > would
> > mean changing the existing VPD and memconsole drivers to use
> > ioremap(), too.)
> 
> I wasn't aware that CBMEM is used for DMA as there's no such concept in
> coreboot yet. For me it looks like the UCSI is regular DRAM mapped as
> WB accessed by the ACPI interpreter.
> I'll prepare a new patch-set using ioremap in all drivers that access
> CBMEM.
> 

We shouldn't use ioremap() here as this isn't I/O memory. It's just
regular memory that wants be mapped with some particular set of
attributes, hence the use of memremap().

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Stephen Boyd]

Quoting patrick.rudolph@9elements.com (2019-11-28 04:50:50)
> diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/ABI/stable/sysfs-bus-coreboot
> new file mode 100644
> index 000000000000..1b04b8abc858
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> @@ -0,0 +1,43 @@
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/id
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/id if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds an ASCII representation of the CBMEM ID in hex
> +               (e.g. 0xdeadbeef).
> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/size
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/size if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds an representation as decimal number of the
> +               CBMEM buffer size (e.g. 64).
> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/address
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/address if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds an ASCII representation of the physical address
> +               of the CBMEM buffer in hex (e.g. 0x000000008000d000).

What is the purpose of this field? Can't userspace read the 'data'
attribute to get the data out?

> +
> +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/data
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               cbmem_attributes/data if the device corresponds to a CBMEM
> +               buffer.
> +               The file holds a read-only binary representation of the CBMEM
> +               buffer.
> diff --git a/drivers/firmware/google/cbmem-coreboot.c b/drivers/firmware/google/cbmem-coreboot.c
> new file mode 100644
> index 000000000000..c67651a9c287
> --- /dev/null
> +++ b/drivers/firmware/google/cbmem-coreboot.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * fmap-coreboot.c
> + *
> + * Exports CBMEM as attributes in sysfs.
> + *
> + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
> + * Copyright 2017 Google Inc.
> + * Copyright 2019 9elements Agency GmbH
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/io.h>

Is this include used? Should probably include linux/memremap.h instead.

> +#include <linux/slab.h>
> +
> +#include "coreboot_table.h"
> +
> +#define CB_TAG_CBMEM_ENTRY 0x31
> +
> +struct cb_priv {
> +       void *remap;
> +       struct lb_cbmem_entry entry;
> +};
> +
> +static ssize_t id_show(struct device *dev,
> +                      struct device_attribute *attr, char *buffer)
> +{
> +       const struct cb_priv *priv;
> +
> +       priv = (const struct cb_priv *)dev_get_platdata(dev);

Please drop useless casts from void *.

> +
> +       return sprintf(buffer, "0x%08x\n", priv->entry.id);

Use %#08x. Also not sure we need newlines but I guess it's OK.

> +}
> +
> +static ssize_t size_show(struct device *dev,
> +                        struct device_attribute *attr, char *buffer)
> +{
> +       const struct cb_priv *priv;
> +
> +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> +
> +       return sprintf(buffer, "%u\n", priv->entry.size);
> +}
> +
> +static ssize_t address_show(struct device *dev,
> +                           struct device_attribute *attr, char *buffer)
> +{
> +       const struct cb_priv *priv;
> +
> +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> +
> +       return sprintf(buffer, "0x%016llx\n", priv->entry.address);
> +}
> +
> +static DEVICE_ATTR_RO(id);
> +static DEVICE_ATTR_RO(size);
> +static DEVICE_ATTR_RO(address);
> +
> +static struct attribute *cb_mem_attrs[] = {
> +       &dev_attr_address.attr,
> +       &dev_attr_id.attr,
> +       &dev_attr_size.attr,
> +       NULL
> +};
> +
> +static ssize_t data_read(struct file *filp, struct kobject *kobj,
> +                        struct bin_attribute *bin_attr,
> +                        char *buffer, loff_t offset, size_t count)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       const struct cb_priv *priv;
> +
> +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> +
> +       return memory_read_from_buffer(buffer, count, &offset,
> +                                      priv->remap, priv->entry.size);
> +}
> +
> +static BIN_ATTR_RO(data, 0);
> +
> +static struct bin_attribute *cb_mem_bin_attrs[] = {
> +       &bin_attr_data,
> +       NULL
> +};
> +
> +static struct attribute_group cb_mem_attr_group = {

Can it be const?

> +       .name = "cbmem_attributes",
> +       .attrs = cb_mem_attrs,
> +       .bin_attrs = cb_mem_bin_attrs,
> +};
> +
> +static int cbmem_probe(struct coreboot_device *cdev)
> +{
> +       struct device *dev = &cdev->dev;
> +       struct cb_priv *priv;
> +       int err;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> +
> +       priv->remap = memremap(priv->entry.address,
> +                              priv->entry.entry_size, MEMREMAP_WB);
> +       if (!priv->remap) {
> +               err = -ENOMEM;
> +               goto failure;
> +       }
> +
> +       err = sysfs_create_group(&dev->kobj, &cb_mem_attr_group);
> +       if (err) {
> +               dev_err(dev, "failed to create sysfs attributes: %d\n", err);
> +               memunmap(priv->remap);
> +               goto failure;
> +       }
> +
> +       dev->platform_data = priv;

Can we use dev_{set,get}_drvdata() instead?

> +
> +       return 0;
> +failure:
> +       kfree(priv);
> +       return err;
> +}
> +
> +static int cbmem_remove(struct coreboot_device *cdev)
> +{
> +       struct device *dev = &cdev->dev;
> +       const struct cb_priv *priv;
> +
> +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> +
> +       sysfs_remove_group(&dev->kobj, &cb_mem_attr_group);
> +
> +       if (priv)
> +               memunmap(priv->remap);
> +       kfree(priv);
> +
> +       dev->platform_data = NULL;

This shouldn't be necessary if the driver_data APIs are used instead.
The driver core does it for us then.

> +
> +       return 0;
> +}
> +
> +static struct coreboot_driver cbmem_driver = {
> +       .probe = cbmem_probe,
> +       .remove = cbmem_remove,
> +       .drv = {
> +               .name = "cbmem",
> +       },
> +       .tag = CB_TAG_CBMEM_ENTRY,
> +};
> +
> +module_coreboot_driver(cbmem_driver);
> +
> +MODULE_AUTHOR("9elements Agency GmbH");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index 7b7b4a6eedda..506048c724d8 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -59,6 +59,18 @@ struct lb_framebuffer {
>         u8  reserved_mask_size;
>  };
>  
> +/* There can be more than one of these records as there is one per cbmem entry.

Please add a bare /* for multi-line comments.

> + * Describes a buffer in memory containing runtime data.
> + */
> +struct lb_cbmem_entry {
> +       u32 tag;
> +       u32 size;
> +
> +       u64 address;
> +       u32 entry_size;
> +       u32 id;
> +};
> +
>  /* A device, additionally with information from coreboot. */
>  struct coreboot_device {
>         struct device dev;

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[patrick.rudolph]

On Mon, 2019-12-16 at 23:38 -0800, Stephen Boyd wrote:
> Quoting patrick.rudolph@9elements.com (2019-11-28 04:50:50)
> > diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot
> > b/Documentation/ABI/stable/sysfs-bus-coreboot
> > new file mode 100644
> > index 000000000000..1b04b8abc858
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> > @@ -0,0 +1,43 @@
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/id
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/id if the device corresponds to a
> > CBMEM
> > +               buffer.
> > +               The file holds an ASCII representation of the CBMEM
> > ID in hex
> > +               (e.g. 0xdeadbeef).
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/size
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/size if the device corresponds to
> > a CBMEM
> > +               buffer.
> > +               The file holds an representation as decimal number
> > of the
> > +               CBMEM buffer size (e.g. 64).
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/addr
> > ess
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/address if the device corresponds
> > to a CBMEM
> > +               buffer.
> > +               The file holds an ASCII representation of the
> > physical address
> > +               of the CBMEM buffer in hex (e.g.
> > 0x000000008000d000).
> 
> What is the purpose of this field? Can't userspace read the 'data'
> attribute to get the data out?

It's the linear address of the CBMEM buffer. It's more for debugging
purposes and could be matched with entries from the coreboot tables.
The 'data' field only returns the data itself, but says nothing about the
address stored. I'll update the documentation.

> 
> > +
> > +What:          /sys/bus/coreboot/devices/.../cbmem_attributes/data
> > +Date:          Nov 2019
> > +KernelVersion: 5.5
> > +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> > +Description:
> > +               coreboot device directory can contain a file named
> > +               cbmem_attributes/data if the device corresponds to
> > a CBMEM
> > +               buffer.
> > +               The file holds a read-only binary representation of
> > the CBMEM
> > +               buffer.
> > diff --git a/drivers/firmware/google/cbmem-coreboot.c
> > b/drivers/firmware/google/cbmem-coreboot.c
> > new file mode 100644
> > index 000000000000..c67651a9c287
> > --- /dev/null
> > +++ b/drivers/firmware/google/cbmem-coreboot.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * fmap-coreboot.c
> > + *
> > + * Exports CBMEM as attributes in sysfs.
> > + *
> > + * Copyright 2012-2013 David Herrmann <dh.herrmann@gmail.com>
> > + * Copyright 2017 Google Inc.
> > + * Copyright 2019 9elements Agency GmbH
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> 
> Is this include used? Should probably include linux/memremap.h
> instead.
> 
> > +#include <linux/slab.h>
> > +
> > +#include "coreboot_table.h"
> > +
> > +#define CB_TAG_CBMEM_ENTRY 0x31
> > +
> > +struct cb_priv {
> > +       void *remap;
> > +       struct lb_cbmem_entry entry;
> > +};
> > +
> > +static ssize_t id_show(struct device *dev,
> > +                      struct device_attribute *attr, char *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> 
> Please drop useless casts from void *.
> 
> > +
> > +       return sprintf(buffer, "0x%08x\n", priv->entry.id);
> 
> Use %#08x. Also not sure we need newlines but I guess it's OK.
> 
> > +}
> > +
> > +static ssize_t size_show(struct device *dev,
> > +                        struct device_attribute *attr, char
> > *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return sprintf(buffer, "%u\n", priv->entry.size);
> > +}
> > +
> > +static ssize_t address_show(struct device *dev,
> > +                           struct device_attribute *attr, char
> > *buffer)
> > +{
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return sprintf(buffer, "0x%016llx\n", priv->entry.address);
> > +}
> > +
> > +static DEVICE_ATTR_RO(id);
> > +static DEVICE_ATTR_RO(size);
> > +static DEVICE_ATTR_RO(address);
> > +
> > +static struct attribute *cb_mem_attrs[] = {
> > +       &dev_attr_address.attr,
> > +       &dev_attr_id.attr,
> > +       &dev_attr_size.attr,
> > +       NULL
> > +};
> > +
> > +static ssize_t data_read(struct file *filp, struct kobject *kobj,
> > +                        struct bin_attribute *bin_attr,
> > +                        char *buffer, loff_t offset, size_t count)
> > +{
> > +       struct device *dev = kobj_to_dev(kobj);
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       return memory_read_from_buffer(buffer, count, &offset,
> > +                                      priv->remap, priv-
> > >entry.size);
> > +}
> > +
> > +static BIN_ATTR_RO(data, 0);
> > +
> > +static struct bin_attribute *cb_mem_bin_attrs[] = {
> > +       &bin_attr_data,
> > +       NULL
> > +};
> > +
> > +static struct attribute_group cb_mem_attr_group = {
> 
> Can it be const?
> 
> > +       .name = "cbmem_attributes",
> > +       .attrs = cb_mem_attrs,
> > +       .bin_attrs = cb_mem_bin_attrs,
> > +};
> > +
> > +static int cbmem_probe(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       struct cb_priv *priv;
> > +       int err;
> > +
> > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv-
> > >entry));
> > +
> > +       priv->remap = memremap(priv->entry.address,
> > +                              priv->entry.entry_size,
> > MEMREMAP_WB);
> > +       if (!priv->remap) {
> > +               err = -ENOMEM;
> > +               goto failure;
> > +       }
> > +
> > +       err = sysfs_create_group(&dev->kobj, &cb_mem_attr_group);
> > +       if (err) {
> > +               dev_err(dev, "failed to create sysfs attributes:
> > %d\n", err);
> > +               memunmap(priv->remap);
> > +               goto failure;
> > +       }
> > +
> > +       dev->platform_data = priv;
> 
> Can we use dev_{set,get}_drvdata() instead?
> 
> > +
> > +       return 0;
> > +failure:
> > +       kfree(priv);
> > +       return err;
> > +}
> > +
> > +static int cbmem_remove(struct coreboot_device *cdev)
> > +{
> > +       struct device *dev = &cdev->dev;
> > +       const struct cb_priv *priv;
> > +
> > +       priv = (const struct cb_priv *)dev_get_platdata(dev);
> > +
> > +       sysfs_remove_group(&dev->kobj, &cb_mem_attr_group);
> > +
> > +       if (priv)
> > +               memunmap(priv->remap);
> > +       kfree(priv);
> > +
> > +       dev->platform_data = NULL;
> 
> This shouldn't be necessary if the driver_data APIs are used instead.
> The driver core does it for us then.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static struct coreboot_driver cbmem_driver = {
> > +       .probe = cbmem_probe,
> > +       .remove = cbmem_remove,
> > +       .drv = {
> > +               .name = "cbmem",
> > +       },
> > +       .tag = CB_TAG_CBMEM_ENTRY,
> > +};
> > +
> > +module_coreboot_driver(cbmem_driver);
> > +
> > +MODULE_AUTHOR("9elements Agency GmbH");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/firmware/google/coreboot_table.h
> > b/drivers/firmware/google/coreboot_table.h
> > index 7b7b4a6eedda..506048c724d8 100644
> > --- a/drivers/firmware/google/coreboot_table.h
> > +++ b/drivers/firmware/google/coreboot_table.h
> > @@ -59,6 +59,18 @@ struct lb_framebuffer {
> >         u8  reserved_mask_size;
> >  };
> >  
> > +/* There can be more than one of these records as there is one per
> > cbmem entry.
> 
> Please add a bare /* for multi-line comments.
> 
> > + * Describes a buffer in memory containing runtime data.
> > + */
> > +struct lb_cbmem_entry {
> > +       u32 tag;
> > +       u32 size;
> > +
> > +       u64 address;
> > +       u32 entry_size;
> > +       u32 id;
> > +};
> > +
> >  /* A device, additionally with information from coreboot. */
> >  struct coreboot_device {
> >         struct device dev;

Will send a new patch-set with the requested changes.

Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Greg Kroah-Hartman]

On Thu, Nov 28, 2019 at 01:50:50PM +0100, patrick.rudolph@9elements.com wrote:
> +static int cbmem_probe(struct coreboot_device *cdev)
> +{
> +	struct device *dev = &cdev->dev;
> +	struct cb_priv *priv;
> +	int err;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> +
> +	priv->remap = memremap(priv->entry.address,
> +			       priv->entry.entry_size, MEMREMAP_WB);
> +	if (!priv->remap) {
> +		err = -ENOMEM;
> +		goto failure;
> +	}
> +
> +	err = sysfs_create_group(&dev->kobj, &cb_mem_attr_group);

You just raced with userspace and lost :(

Set the driver's default attribute group up to point at
cb_mem_attr_group and the driver core will handle creating and removing
the group automatically for you correctly.

thanks,

greg k-h

[PATCH v3 2/2] firmware: google: Expose coreboot tables over sysfs

[patrick.rudolph]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Make all coreboot table entries available to userland. This is useful for
tools that are currently using /dev/mem.

Besides the tag and size also expose the raw table data itself.

Update the ABI documentation to explain the new sysfs interface.

Tools can easily scan for the right coreboot table by reading
/sys/bus/coreboot/devices/coreboot*/attributes/id
The binary table data can then be read from
/sys/bus/coreboot/devices/coreboot*/attributes/data

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
 -v2:
	- Add ABI documentation
	- Add 0x prefix on hex values
	- Remove wrong ioremap hint as found by CI
 -v3:
	- Use BIN_ATTR_RO
---
 Documentation/ABI/stable/sysfs-bus-coreboot | 30 +++++++++++
 drivers/firmware/google/coreboot_table.c    | 57 +++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/ABI/stable/sysfs-bus-coreboot
index 1b04b8abc858..0f28601229f3 100644
--- a/Documentation/ABI/stable/sysfs-bus-coreboot
+++ b/Documentation/ABI/stable/sysfs-bus-coreboot
@@ -41,3 +41,33 @@ Description:
 		buffer.
 		The file holds a read-only binary representation of the CBMEM
 		buffer.
+
+What:		/sys/bus/coreboot/devices/.../attributes/id
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named attributes/id.
+		The file holds an ASCII representation of the coreboot table ID
+		in hex (e.g. 0x000000ef). On coreboot enabled platforms the ID is
+		usually called TAG.
+
+What:		/sys/bus/coreboot/devices/.../attributes/size
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		attributes/size.
+		The file holds an ASCII representation as decimal number of the
+		coreboot table size (e.g. 64).
+
+What:		/sys/bus/coreboot/devices/.../attributes/data
+Date:		Nov 2019
+KernelVersion:	5.5
+Contact:	Patrick Rudolph <patrick.rudolph@9elements.com>
+Description:
+		coreboot device directory can contain a file named
+		attributes/data.
+		The file holds a read-only binary representation of the coreboot
+		table.
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 8d132e4f008a..1f7329d72f54 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -6,6 +6,7 @@
  *
  * Copyright 2017 Google Inc.
  * Copyright 2017 Samuel Holland <samuel@sholland.org>
+ * Copyright 2019 9elements Agency GmbH
  */
 
 #include <linux/acpi.h>
@@ -84,6 +85,60 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
 }
 EXPORT_SYMBOL(coreboot_driver_unregister);
 
+static ssize_t id_show(struct device *dev,
+		       struct device_attribute *attr, char *buffer)
+{
+	struct coreboot_device *device = CB_DEV(dev);
+
+	return sprintf(buffer, "0x%08x\n", device->entry.tag);
+}
+
+static ssize_t size_show(struct device *dev,
+			 struct device_attribute *attr, char *buffer)
+{
+	struct coreboot_device *device = CB_DEV(dev);
+
+	return sprintf(buffer, "%u\n", device->entry.size);
+}
+
+static DEVICE_ATTR_RO(id);
+static DEVICE_ATTR_RO(size);
+
+static struct attribute *cb_dev_attrs[] = {
+	&dev_attr_id.attr,
+	&dev_attr_size.attr,
+	NULL
+};
+
+static ssize_t data_read(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *bin_attr,
+			 char *buffer, loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct coreboot_device *device = CB_DEV(dev);
+
+	return memory_read_from_buffer(buffer, count, &offset,
+				       &device->entry, device->entry.size);
+}
+
+static BIN_ATTR_RO(data, 0);
+
+static struct bin_attribute *cb_dev_bin_attrs[] = {
+	&bin_attr_data,
+	NULL
+};
+
+static const struct attribute_group cb_dev_attr_group = {
+	.name = "attributes",
+	.attrs = cb_dev_attrs,
+	.bin_attrs = cb_dev_bin_attrs,
+};
+
+static const struct attribute_group *cb_dev_attr_groups[] = {
+	&cb_dev_attr_group,
+	NULL
+};
+
 static int coreboot_table_populate(struct device *dev, void *ptr)
 {
 	int i, ret;
@@ -104,6 +159,8 @@ static int coreboot_table_populate(struct device *dev, void *ptr)
 		device->dev.parent = dev;
 		device->dev.bus = &coreboot_bus_type;
 		device->dev.release = coreboot_device_release;
+		device->dev.groups = cb_dev_attr_groups;
+
 		memcpy(&device->entry, ptr_entry, entry->size);
 
 		ret = device_register(&device->dev);
-- 
2.21.0

Re: [PATCH v3 2/2] firmware: google: Expose coreboot tables over sysfs

[Stephen Boyd]

Quoting patrick.rudolph@9elements.com (2019-11-28 04:50:51)
> diff --git a/Documentation/ABI/stable/sysfs-bus-coreboot b/Documentation/ABI/stable/sysfs-bus-coreboot
> index 1b04b8abc858..0f28601229f3 100644
> --- a/Documentation/ABI/stable/sysfs-bus-coreboot
> +++ b/Documentation/ABI/stable/sysfs-bus-coreboot
> @@ -41,3 +41,33 @@ Description:
>                 buffer.
>                 The file holds a read-only binary representation of the CBMEM
>                 buffer.
> +
> +What:          /sys/bus/coreboot/devices/.../attributes/id
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named attributes/id.
> +               The file holds an ASCII representation of the coreboot table ID
> +               in hex (e.g. 0x000000ef). On coreboot enabled platforms the ID is
> +               usually called TAG.

Why don't we call it 'tag' then?

> +
> +What:          /sys/bus/coreboot/devices/.../attributes/size
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               attributes/size.

Can we drop this first sentence in all the places? I don't see what
value it adds.

> +               The file holds an ASCII representation as decimal number of the
> +               coreboot table size (e.g. 64).
> +
> +What:          /sys/bus/coreboot/devices/.../attributes/data
> +Date:          Nov 2019
> +KernelVersion: 5.5
> +Contact:       Patrick Rudolph <patrick.rudolph@9elements.com>
> +Description:
> +               coreboot device directory can contain a file named
> +               attributes/data.
> +               The file holds a read-only binary representation of the coreboot
> +               table.

Maybe the attributes directory should be called 'table'? Given that
we're exposing the coreboot table contents directly to userspace?

> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 8d132e4f008a..1f7329d72f54 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -6,6 +6,7 @@
>   *
>   * Copyright 2017 Google Inc.
>   * Copyright 2017 Samuel Holland <samuel@sholland.org>
> + * Copyright 2019 9elements Agency GmbH
>   */
>  
>  #include <linux/acpi.h>
> @@ -84,6 +85,60 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>  
> +static ssize_t id_show(struct device *dev,
> +                      struct device_attribute *attr, char *buffer)
> +{
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return sprintf(buffer, "0x%08x\n", device->entry.tag);

Use %#08x instead?

> +}
> +
> +static ssize_t size_show(struct device *dev,
> +                        struct device_attribute *attr, char *buffer)
> +{
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return sprintf(buffer, "%u\n", device->entry.size);
> +}
> +
> +static DEVICE_ATTR_RO(id);
> +static DEVICE_ATTR_RO(size);
> +
> +static struct attribute *cb_dev_attrs[] = {
> +       &dev_attr_id.attr,
> +       &dev_attr_size.attr,
> +       NULL
> +};
> +
> +static ssize_t data_read(struct file *filp, struct kobject *kobj,
> +                        struct bin_attribute *bin_attr,
> +                        char *buffer, loff_t offset, size_t count)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct coreboot_device *device = CB_DEV(dev);
> +
> +       return memory_read_from_buffer(buffer, count, &offset,
> +                                      &device->entry, device->entry.size);
> +}
> +
> +static BIN_ATTR_RO(data, 0);

Can we fill in the size of the data at runtime somehow? Might require
making a different struct bin_attribute for each coreboot entry I
suppose but then we can probably leave out the 'size' attribute and only
have the 'data' and 'tag' attributes.

> +
> +static struct bin_attribute *cb_dev_bin_attrs[] = {
> +       &bin_attr_data,
> +       NULL
> +};