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

Stephen Boyd
2019-12-17T07:19:30Z

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
> +};