2011-02-23 01:53:31

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 0/5] Exporting DMI entries via sysfs

This patchset applies to v2.6.38-rc6.

The following series exports information the DMI / SMBIOS tables via
sysfs under the path /sys/firmware/dmi.

For our purposes, we actively use the System Event Log, as described by
DMI entry Type 15. Currently, we have to grub around /dev/mem to find
this thing, and potentially need to issue IOs directly from userland to
get at the event log, which is suboptimal in terms of security and
architecture.

This series implements the basics needed to iterate through the DMI
entries safely from userland. Each entry has the handle, the formatted
length and the raw bytes exposed in their own sub-directory under
/sys/firmware/dmi/entries/<type>-<instance>.

As well, this series explodes type 15 as a proof-of-concept of how we
can expose kernel interpretations of the entry data. For most entries,
there isn't much sense in parsing the entry and exposing each field
as a sysfs attribute as they can be interpreted from userland by
accessing the raw entry itself. Type 15 however describes an
indirection (pointing at the system event log), and it is generally
useful to export this log in raw form to the user (which requires
interpreting the DMI entry).

This driver relies solely on dmi_walk() for access to the dmi entries.
They are not kept mapped in the "rest" state. The only bits memo-ized
by the objects in sysfs are the dmi headers, which are used to the find
the entries via dmi_walk on read. Reading the raw bytes however is
'uncached' by this driver, so that any changes to the entries themselves
are reflected properly by the user-exposed interface.

We intend to use this on our production servers, though it seems to
also be sufficient in suiting Chrome OS's needs
(http://code.google.com/p/chromium-os/issues/detail?id=6795).

Thanks,

Mike Waychison

Related discussions
===================

- Andi Kleen suggesting that perhaps this data could be exported via
sysfs:

http://kerneltrap.org/mailarchive/linux-kernel/2010/9/28/4625643/thread

- Tim Hockin suggesting a potential layout:

https://lkml.org/lkml/2011/2/10/550


Patchset summary
================

firmware: Add DMI entry types to the headers
firmware: Basic dmi-sysfs support
firmware: Break out system_event_log in dmi-sysfs
firmware: Expose DMI type 15 System Event Log
firmware: Add documentation for /sys/firmware/dmi

Diffstat
========

Documentation/ABI/testing/sysfs-firmware-dmi | 110 ++++
drivers/firmware/Kconfig | 11
drivers/firmware/Makefile | 1
drivers/firmware/dmi-sysfs.c | 698 +++++++++++++++++++++++++++
include/linux/dmi.h | 47 +
5 files changed, 867 insertions(+)

Changelog:
==========
- v2
- Added 'type', 'instance' and 'position' attributes to each DMI entry.
- Don't printk KERN_INFO on module load/unload.
- Default to 'n' for Kconfig.
- Grammar fixes.
- Fixed bug reading raw files when multiple instances of the same
type exist.
- v1
- Initial public send-out.


2011-02-23 01:53:40

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 2/5] firmware: Basic dmi-sysfs support

Introduce a new module "dmi-sysfs" that exports the broken out entries
of the DMI table through sysfs.

Entries are enumerated via dmi_walk() on module load, and are populated
as kobjects rooted at /sys/firmware/dmi/entries.

Entries are named "<type>-<instance>", where:
<type> : is the type of the entry, and
<instance> : is the ordinal count within the DMI table of that
entry type. This instance is used in lieu the DMI
entry's handle as no assurances are made by the kernel
that handles are unique.

All entries export the following attributes:
length : The length of the formatted portion of the entry
handle : The handle given to this entry by the firmware
raw : The raw bytes of the entire entry, including the
formatted portion, the unformatted (strings) portion,
and the two terminating nul characters.
type : The DMI entry type
instance : The ordinal instance of this entry given its type.
position : The position ordinal of the entry within the table in
its entirety.

Entries in dmi-sysfs are kobject backed members called "struct
dmi_sysfs_entry" and belong to dmi_kset. They are threaded through
entry_list (protected by entry_list_lock) so that we can find them at
cleanup time.

Signed-off-by: Mike Waychison <[email protected]>
---
Changelog:
- v2
- Added 'type', 'instance' and 'position' attributes to each DMI entry.
- Fixed module to default to 'n'.
- Fixed copyright year and module author.
- Replace module load/unload messages with pr_debug().
- Fixed a bug in handling multiple instance of the same type where
reading the raw file for earlier instances would result in 0 bytes
read due to the instance_count getting stuck at '0'.
---
drivers/firmware/Kconfig | 11 +
drivers/firmware/Makefile | 1
drivers/firmware/dmi-sysfs.c | 396 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 408 insertions(+), 0 deletions(-)
create mode 100644 drivers/firmware/dmi-sysfs.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index e710424..3c56afc 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -113,6 +113,17 @@ config DMIID
information from userspace through /sys/class/dmi/id/ or if you want
DMI-based module auto-loading.

+config DMI_SYSFS
+ tristate "DMI table support in sysfs"
+ depends on SYSFS && DMI
+ default n
+ help
+ Say Y or M here to enable the exporting of the raw DMI table
+ data via sysfs. This is useful for consuming the data without
+ requiring any access to /dev/mem at all. Tables are found
+ under /sys/firmware/dmi when this option is enabled and
+ loaded.
+
config ISCSI_IBFT_FIND
bool "iSCSI Boot Firmware Table Attributes"
depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 1c3c173..20c17fc 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -2,6 +2,7 @@
# Makefile for the linux kernel.
#
obj-$(CONFIG_DMI) += dmi_scan.o
+obj-$(CONFIG_DMI_SYSFS) += dmi-sysfs.o
obj-$(CONFIG_EDD) += edd.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_PCDP) += pcdp.o
diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
new file mode 100644
index 0000000..2d8a04a
--- /dev/null
+++ b/drivers/firmware/dmi-sysfs.c
@@ -0,0 +1,396 @@
+/*
+ * dmi-sysfs.c
+ *
+ * This module exports the DMI tables read-only to userspace through the
+ * sysfs file system.
+ *
+ * Data is currently found below
+ * /sys/firmware/dmi/...
+ *
+ * DMI attributes are presented in attribute files with names
+ * formatted using %d-%d, so that the first integer indicates the
+ * structure type (0-255), and the second field is the instance of that
+ * entry.
+ *
+ * Copyright 2011 Google, Inc.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kobject.h>
+#include <linux/dmi.h>
+#include <linux/capability.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/io.h>
+
+#define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
+ the top entry type is only 8 bits */
+
+struct dmi_sysfs_entry {
+ struct dmi_header dh;
+ struct kobject kobj;
+ int instance;
+ int position;
+ struct list_head list;
+};
+
+/*
+ * Global list of dmi_sysfs_entry. Even though this should only be
+ * manipulated at setup and teardown, the lazy nature of the kobject
+ * system means we get lazy removes.
+ */
+static LIST_HEAD(entry_list);
+static DEFINE_SPINLOCK(entry_list_lock);
+
+/* dmi_sysfs_attribute - Top level attribute. used by all entries. */
+struct dmi_sysfs_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct dmi_sysfs_entry *entry, char *buf);
+};
+
+#define DMI_SYSFS_ATTR(_entry, _name) \
+struct dmi_sysfs_attribute dmi_sysfs_attr_##_entry##_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = 0400}, \
+ .show = dmi_sysfs_##_entry##_##_name, \
+}
+
+/*
+ * dmi_sysfs_mapped_attribute - Attribute where we require the entry be
+ * mapped in. Use in conjunction with dmi_sysfs_specialize_attr_ops.
+ */
+struct dmi_sysfs_mapped_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct dmi_sysfs_entry *entry,
+ const struct dmi_header *dh,
+ char *buf);
+};
+
+#define DMI_SYSFS_MAPPED_ATTR(_entry, _name) \
+struct dmi_sysfs_mapped_attribute dmi_sysfs_attr_##_entry##_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = 0400}, \
+ .show = dmi_sysfs_##_entry##_##_name, \
+}
+
+/*************************************************
+ * Generic DMI entry support.
+ *************************************************/
+
+static struct dmi_sysfs_entry *to_entry(struct kobject *kobj)
+{
+ return container_of(kobj, struct dmi_sysfs_entry, kobj);
+}
+
+static struct dmi_sysfs_attribute *to_attr(struct attribute *attr)
+{
+ return container_of(attr, struct dmi_sysfs_attribute, attr);
+}
+
+static ssize_t dmi_sysfs_attr_show(struct kobject *kobj,
+ struct attribute *_attr, char *buf)
+{
+ struct dmi_sysfs_entry *entry = to_entry(kobj);
+ struct dmi_sysfs_attribute *attr = to_attr(_attr);
+
+ /* DMI stuff is only ever admin visible */
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ return attr->show(entry, buf);
+}
+
+static const struct sysfs_ops dmi_sysfs_attr_ops = {
+ .show = dmi_sysfs_attr_show,
+};
+
+typedef ssize_t (*dmi_callback)(struct dmi_sysfs_entry *,
+ const struct dmi_header *dh, void *);
+
+struct find_dmi_data {
+ struct dmi_sysfs_entry *entry;
+ dmi_callback callback;
+ void *private;
+ int instance_countdown;
+ ssize_t ret;
+};
+
+static void find_dmi_entry_helper(const struct dmi_header *dh,
+ void *_data)
+{
+ struct find_dmi_data *data = _data;
+ struct dmi_sysfs_entry *entry = data->entry;
+
+ /* Is this the entry we want? */
+ if (dh->type != entry->dh.type)
+ return;
+
+ if (data->instance_countdown != 0) {
+ /* try the next instance? */
+ data->instance_countdown--;
+ return;
+ }
+
+ /*
+ * Don't ever revisit the instance. Short circuit later
+ * instances by letting the instance_countdown run negative
+ */
+ data->instance_countdown--;
+
+ /* Found the entry */
+ data->ret = data->callback(entry, dh, data->private);
+}
+
+/* State for passing the read parameters through dmi_find_entry() */
+struct dmi_read_state {
+ char *buf;
+ loff_t pos;
+ size_t count;
+};
+
+static ssize_t find_dmi_entry(struct dmi_sysfs_entry *entry,
+ dmi_callback callback, void *private)
+{
+ struct find_dmi_data data = {
+ .entry = entry,
+ .callback = callback,
+ .private = private,
+ .instance_countdown = entry->instance,
+ .ret = -EIO, /* To signal the entry disappeared */
+ };
+ int ret;
+
+ ret = dmi_walk(find_dmi_entry_helper, &data);
+ /* This shouldn't happen, but just in case. */
+ if (ret)
+ return -EINVAL;
+ return data.ret;
+}
+
+/*
+ * Calculate and return the byte length of the dmi entry identified by
+ * dh. This includes both the formatted portion as well as the
+ * unformatted string space, including the two trailing nul characters.
+ */
+static size_t dmi_entry_length(const struct dmi_header *dh)
+{
+ const char *p = (const char *)dh;
+
+ p += dh->length;
+
+ while (p[0] || p[1])
+ p++;
+
+ return 2 + p - (const char *)dh;
+}
+
+/*************************************************
+ * Generic DMI entry support.
+ *************************************************/
+
+static ssize_t dmi_sysfs_entry_length(struct dmi_sysfs_entry *entry, char *buf)
+{
+ return sprintf(buf, "%d\n", entry->dh.length);
+}
+
+static ssize_t dmi_sysfs_entry_handle(struct dmi_sysfs_entry *entry, char *buf)
+{
+ return sprintf(buf, "%d\n", entry->dh.handle);
+}
+
+static ssize_t dmi_sysfs_entry_type(struct dmi_sysfs_entry *entry, char *buf)
+{
+ return sprintf(buf, "%d\n", entry->dh.type);
+}
+
+static ssize_t dmi_sysfs_entry_instance(struct dmi_sysfs_entry *entry,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", entry->instance);
+}
+
+static ssize_t dmi_sysfs_entry_position(struct dmi_sysfs_entry *entry,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", entry->position);
+}
+
+static DMI_SYSFS_ATTR(entry, length);
+static DMI_SYSFS_ATTR(entry, handle);
+static DMI_SYSFS_ATTR(entry, type);
+static DMI_SYSFS_ATTR(entry, instance);
+static DMI_SYSFS_ATTR(entry, position);
+
+static struct attribute *dmi_sysfs_entry_attrs[] = {
+ &dmi_sysfs_attr_entry_length.attr,
+ &dmi_sysfs_attr_entry_handle.attr,
+ &dmi_sysfs_attr_entry_type.attr,
+ &dmi_sysfs_attr_entry_instance.attr,
+ &dmi_sysfs_attr_entry_position.attr,
+ NULL,
+};
+
+static ssize_t dmi_entry_raw_read_helper(struct dmi_sysfs_entry *entry,
+ const struct dmi_header *dh,
+ void *_state)
+{
+ struct dmi_read_state *state = _state;
+ size_t entry_length;
+
+ entry_length = dmi_entry_length(dh);
+
+ return memory_read_from_buffer(state->buf, state->count,
+ &state->pos, dh, entry_length);
+}
+
+static ssize_t dmi_entry_raw_read(struct file *filp,
+ struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t pos, size_t count)
+{
+ struct dmi_sysfs_entry *entry = to_entry(kobj);
+ struct dmi_read_state state = {
+ .buf = buf,
+ .pos = pos,
+ .count = count,
+ };
+
+ return find_dmi_entry(entry, dmi_entry_raw_read_helper, &state);
+}
+
+static const struct bin_attribute dmi_entry_raw_attr = {
+ .attr = {.name = "raw", .mode = 0400},
+ .read = dmi_entry_raw_read,
+};
+
+static void dmi_sysfs_entry_release(struct kobject *kobj)
+{
+ struct dmi_sysfs_entry *entry = to_entry(kobj);
+ sysfs_remove_bin_file(&entry->kobj, &dmi_entry_raw_attr);
+ spin_lock(&entry_list_lock);
+ list_del(&entry->list);
+ spin_unlock(&entry_list_lock);
+ kfree(entry);
+}
+
+static struct kobj_type dmi_sysfs_entry_ktype = {
+ .release = dmi_sysfs_entry_release,
+ .sysfs_ops = &dmi_sysfs_attr_ops,
+ .default_attrs = dmi_sysfs_entry_attrs,
+};
+
+static struct kobject *dmi_kobj;
+static struct kset *dmi_kset;
+
+/* Global count of all instances seen. Only for setup */
+static int __initdata instance_counts[MAX_ENTRY_TYPE + 1];
+
+/* Global positional count of all entries seen. Only for setup */
+static int __initdata position_count;
+
+static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,
+ void *_ret)
+{
+ struct dmi_sysfs_entry *entry;
+ int *ret = _ret;
+
+ /* If a previous entry saw an error, short circuit */
+ if (*ret)
+ return;
+
+ /* Allocate and register a new entry into the entries set */
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry) {
+ *ret = -ENOMEM;
+ return;
+ }
+
+ /* Set the key */
+ entry->dh = *dh;
+ entry->instance = instance_counts[dh->type]++;
+ entry->position = position_count++;
+
+ entry->kobj.kset = dmi_kset;
+ *ret = kobject_init_and_add(&entry->kobj, &dmi_sysfs_entry_ktype, NULL,
+ "%d-%d", dh->type, entry->instance);
+
+ if (*ret) {
+ kfree(entry);
+ return;
+ }
+
+ /* Thread on the global list for cleanup */
+ spin_lock(&entry_list_lock);
+ list_add_tail(&entry->list, &entry_list);
+ spin_unlock(&entry_list_lock);
+
+ /* Create the raw binary file to access the entry */
+ *ret = sysfs_create_bin_file(&entry->kobj, &dmi_entry_raw_attr);
+ if (*ret)
+ goto out_err;
+
+ return;
+out_err:
+ kobject_put(&entry->kobj);
+ return;
+}
+
+static void cleanup_entry_list(void)
+{
+ struct dmi_sysfs_entry *entry, *next;
+
+ /* No locks, we are on our way out */
+ list_for_each_entry_safe(entry, next, &entry_list, list) {
+ kobject_put(&entry->kobj);
+ }
+}
+
+static int __init dmi_sysfs_init(void)
+{
+ int error = -ENOMEM;
+ int val;
+
+ /* Set up our directory */
+ dmi_kobj = kobject_create_and_add("dmi", firmware_kobj);
+ if (!dmi_kobj)
+ goto err;
+
+ dmi_kset = kset_create_and_add("entries", NULL, dmi_kobj);
+ if (!dmi_kset)
+ goto err;
+
+ val = 0;
+ error = dmi_walk(dmi_sysfs_register_handle, &val);
+ if (error)
+ goto err;
+ if (val) {
+ error = val;
+ goto err;
+ }
+
+ pr_debug("dmi-sysfs: loaded.\n");
+
+ return 0;
+err:
+ cleanup_entry_list();
+ kset_unregister(dmi_kset);
+ kobject_put(dmi_kobj);
+ return error;
+}
+
+/* clean up everything. */
+static void __exit dmi_sysfs_exit(void)
+{
+ pr_debug("dmi-sysfs: unloading.\n");
+ cleanup_entry_list();
+ kset_unregister(dmi_kset);
+ kobject_put(dmi_kobj);
+}
+
+module_init(dmi_sysfs_init);
+module_exit(dmi_sysfs_exit);
+
+MODULE_AUTHOR("Mike Waychison <[email protected]>");
+MODULE_DESCRIPTION("DMI sysfs support");
+MODULE_LICENSE("GPL");

2011-02-23 01:53:47

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 4/5] firmware: Expose DMI type 15 System Event Log

The System Event Log described by DMI entry type 15 may be backed by
either memory or may be indirectly accessed via an IO index/data
register pair.

In order to get read access to this log, expose it in the
"system_event_log" sub-directory of type 15 DMI entries, ie:
/sys/firmware/dmi/entries/15-0/system_event_log/raw_event_log.

This commit handles both IO accessed and memory access system event
logs. OEM specific access and GPNV support is explicitly not handled
and we error out in the logs when we do not recognize the access method.

Signed-off-by: Mike Waychison <[email protected]>
---
drivers/firmware/dmi-sysfs.c | 143 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 143 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index d209610..a5afd80 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -312,6 +312,140 @@ static struct kobj_type dmi_system_event_log_ktype = {
.default_attrs = dmi_sysfs_sel_attrs,
};

+typedef u8 (*sel_io_reader)(const struct dmi_system_event_log *sel,
+ loff_t offset);
+
+static DEFINE_MUTEX(io_port_lock);
+
+static u8 read_sel_8bit_indexed_io(const struct dmi_system_event_log *sel,
+ loff_t offset)
+{
+ u8 ret;
+
+ mutex_lock(&io_port_lock);
+ outb((u8)offset, sel->io.index_addr);
+ ret = inb(sel->io.data_addr);
+ mutex_unlock(&io_port_lock);
+ return ret;
+}
+
+static u8 read_sel_2x8bit_indexed_io(const struct dmi_system_event_log *sel,
+ loff_t offset)
+{
+ u8 ret;
+
+ mutex_lock(&io_port_lock);
+ outb((u8)offset, sel->io.index_addr);
+ outb((u8)(offset >> 8), sel->io.index_addr + 1);
+ ret = inb(sel->io.data_addr);
+ mutex_unlock(&io_port_lock);
+ return ret;
+}
+
+static u8 read_sel_16bit_indexed_io(const struct dmi_system_event_log *sel,
+ loff_t offset)
+{
+ u8 ret;
+
+ mutex_lock(&io_port_lock);
+ outw((u16)offset, sel->io.index_addr);
+ ret = inb(sel->io.data_addr);
+ mutex_unlock(&io_port_lock);
+ return ret;
+}
+
+static sel_io_reader sel_io_readers[] = {
+ [DMI_SEL_ACCESS_METHOD_IO8] = read_sel_8bit_indexed_io,
+ [DMI_SEL_ACCESS_METHOD_IO2x8] = read_sel_2x8bit_indexed_io,
+ [DMI_SEL_ACCESS_METHOD_IO16] = read_sel_16bit_indexed_io,
+};
+
+static ssize_t dmi_sel_raw_read_io(struct dmi_sysfs_entry *entry,
+ const struct dmi_system_event_log *sel,
+ char *buf, loff_t pos, size_t count)
+{
+ ssize_t wrote = 0;
+
+ sel_io_reader io_reader = sel_io_readers[sel->access_method];
+
+ while (count && pos < sel->area_length) {
+ count--;
+ *(buf++) = io_reader(sel, pos++);
+ wrote++;
+ }
+
+ return wrote;
+}
+
+static ssize_t dmi_sel_raw_read_phys32(struct dmi_sysfs_entry *entry,
+ const struct dmi_system_event_log *sel,
+ char *buf, loff_t pos, size_t count)
+{
+ u8 __iomem *mapped;
+ ssize_t wrote = 0;
+
+ mapped = ioremap(sel->access_method_address, sel->area_length);
+ if (!mapped)
+ return -EIO;
+
+ while (count && pos < sel->area_length) {
+ count--;
+ *(buf++) = readb(mapped + pos++);
+ wrote++;
+ }
+
+ iounmap(mapped);
+ return wrote;
+}
+
+static ssize_t dmi_sel_raw_read_helper(struct dmi_sysfs_entry *entry,
+ const struct dmi_header *dh,
+ void *_state)
+{
+ struct dmi_read_state *state = _state;
+ const struct dmi_system_event_log *sel = to_sel(dh);
+
+ if (sizeof(*sel) > dmi_entry_length(dh))
+ return -EIO;
+
+ switch (sel->access_method) {
+ case DMI_SEL_ACCESS_METHOD_IO8:
+ case DMI_SEL_ACCESS_METHOD_IO2x8:
+ case DMI_SEL_ACCESS_METHOD_IO16:
+ return dmi_sel_raw_read_io(entry, sel, state->buf,
+ state->pos, state->count);
+ case DMI_SEL_ACCESS_METHOD_PHYS32:
+ return dmi_sel_raw_read_phys32(entry, sel, state->buf,
+ state->pos, state->count);
+ case DMI_SEL_ACCESS_METHOD_GPNV:
+ pr_info("dmi-sysfs: GPNV support missing.\n");
+ return -EIO;
+ default:
+ pr_info("dmi-sysfs: Unknown access method %02x\n",
+ sel->access_method);
+ return -EIO;
+ }
+}
+
+static ssize_t dmi_sel_raw_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t pos, size_t count)
+{
+ struct dmi_sysfs_entry *entry = to_entry(kobj->parent);
+ struct dmi_read_state state = {
+ .buf = buf,
+ .pos = pos,
+ .count = count,
+ };
+
+ return find_dmi_entry(entry, dmi_sel_raw_read_helper, &state);
+}
+
+static struct bin_attribute dmi_sel_raw_attr = {
+ .attr = {.name = "raw_event_log", .mode = 0400},
+ .read = dmi_sel_raw_read,
+};
+
static int dmi_system_event_log(struct dmi_sysfs_entry *entry)
{
int ret;
@@ -325,6 +459,15 @@ static int dmi_system_event_log(struct dmi_sysfs_entry *entry)
"system_event_log");
if (ret)
goto out_free;
+
+ ret = sysfs_create_bin_file(entry->child, &dmi_sel_raw_attr);
+ if (ret)
+ goto out_del;
+
+ return 0;
+
+out_del:
+ kobject_del(entry->child);
out_free:
kfree(entry->child);
return ret;

2011-02-23 01:53:35

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 1/5] firmware: Add DMI entry types to the headers

In preparation for the upcoming commits, introduce the DMI entry types to
the headers. These type names are based on those specified in the DMTF
SMBIOS specification version 2.7.1.

Signed-off-by: Mike Waychison <[email protected]>
---
include/linux/dmi.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 90e087f..f156cca 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -23,6 +23,53 @@ enum dmi_device_type {
DMI_DEV_TYPE_DEV_ONBOARD = -3,
};

+enum dmi_entry_type {
+ DMI_ENTRY_BIOS = 0,
+ DMI_ENTRY_SYSTEM,
+ DMI_ENTRY_BASEBOARD,
+ DMI_ENTRY_CHASSIS,
+ DMI_ENTRY_PROCESSOR,
+ DMI_ENTRY_MEM_CONTROLLER,
+ DMI_ENTRY_MEM_MODULE,
+ DMI_ENTRY_CACHE,
+ DMI_ENTRY_PORT_CONNECTOR,
+ DMI_ENTRY_SYSTEM_SLOT,
+ DMI_ENTRY_ONBOARD_DEVICE,
+ DMI_ENTRY_OEMSTRINGS,
+ DMI_ENTRY_SYSCONF,
+ DMI_ENTRY_BIOS_LANG,
+ DMI_ENTRY_GROUP_ASSOC,
+ DMI_ENTRY_SYSTEM_EVENT_LOG,
+ DMI_ENTRY_PHYS_MEM_ARRAY,
+ DMI_ENTRY_MEM_DEVICE,
+ DMI_ENTRY_32_MEM_ERROR,
+ DMI_ENTRY_MEM_ARRAY_MAPPED_ADDR,
+ DMI_ENTRY_MEM_DEV_MAPPED_ADDR,
+ DMI_ENTRY_BUILTIN_POINTING_DEV,
+ DMI_ENTRY_PORTABLE_BATTERY,
+ DMI_ENTRY_SYSTEM_RESET,
+ DMI_ENTRY_HW_SECURITY,
+ DMI_ENTRY_SYSTEM_POWER_CONTROLS,
+ DMI_ENTRY_VOLTAGE_PROBE,
+ DMI_ENTRY_COOLING_DEV,
+ DMI_ENTRY_TEMP_PROBE,
+ DMI_ENTRY_ELECTRICAL_CURRENT_PROBE,
+ DMI_ENTRY_OOB_REMOTE_ACCESS,
+ DMI_ENTRY_BIS_ENTRY,
+ DMI_ENTRY_SYSTEM_BOOT,
+ DMI_ENTRY_MGMT_DEV,
+ DMI_ENTRY_MGMT_DEV_COMPONENT,
+ DMI_ENTRY_MGMT_DEV_THRES,
+ DMI_ENTRY_MEM_CHANNEL,
+ DMI_ENTRY_IPMI_DEV,
+ DMI_ENTRY_SYS_POWER_SUPPLY,
+ DMI_ENTRY_ADDITIONAL,
+ DMI_ENTRY_ONBOARD_DEV_EXT,
+ DMI_ENTRY_MGMT_CONTROLLER_HOST,
+ DMI_ENTRY_INACTIVE = 126,
+ DMI_ENTRY_END_OF_TABLE = 127,
+};
+
struct dmi_header {
u8 type;
u8 length;

2011-02-23 01:53:57

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 5/5] firmware: Add documentation for /sys/firmware/dmi

Document the new ABI added by the dmi-sysfs module.

Signed-off-by: Mike Waychison <[email protected]>
---
Changelog:
- v2
- Added blurbs about 'type' and 'instance' attributes added in this
version.
- Grammar fixes from Tim Hockin.
---
Documentation/ABI/testing/sysfs-firmware-dmi | 110 ++++++++++++++++++++++++++
1 files changed, 110 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-dmi

diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
new file mode 100644
index 0000000..ba9da95
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-dmi
@@ -0,0 +1,110 @@
+What: /sys/firmware/dmi/
+Date: February 2011
+Contact: Mike Waychison <[email protected]>
+Description:
+ Many machines' firmware (x86 and ia64) export DMI /
+ SMBIOS tables to the operating system. Getting at this
+ information is often valuable to userland, especially in
+ cases where there are OEM extensions used.
+
+ The kernel itself does not rely on the majority of the
+ information in these tables being correct. It equally
+ cannot ensure that the data as exported to userland is
+ without error either.
+
+ DMI is structured as a large table of entries, where
+ each entry has a common header indicating the type and
+ length of the entry, as well as 'handle' that is
+ supposed to be unique amongst all entries.
+
+ Some entries are required by the specification, but many
+ others are optional. In general though, users should
+ never expect to find a specific entry type on their
+ system unless they know for certain what their firmware
+ is doing. Machine to machine will vary.
+
+ Multiple entries of the same type are allowed. In order
+ to handle these duplicate entry types, each entry is
+ assigned by the operating system an 'instance', which is
+ derived from an entry type's ordinal position. That is
+ to say, if there are 'N' multiple entries with the same type
+ 'T' in the DMI tables (adjacent or spread apart, it
+ doesn't matter), they will be represented in sysfs as
+ entries "T-0" through "T-(N-1)":
+
+ Example entry directories:
+
+ /sys/firmware/dmi/entries/17-0
+ /sys/firmware/dmi/entries/17-1
+ /sys/firmware/dmi/entries/17-2
+ /sys/firmware/dmi/entries/17-3
+ ...
+
+ Instance numbers are used in lieu of the firmware
+ assigned entry handles as the kernel itself makes no
+ guarantees that handles as exported are unique, and
+ there are likely firmware images that get this wrong in
+ the wild.
+
+ Each DMI entry in sysfs has the common header values
+ exported as attributes:
+
+ handle : The 16bit 'handle' that is assigned to this
+ entry by the firmware. This handle may be
+ referred to by other entries.
+ length : The length of the entry, as presented in the
+ entry itself. Note that this is _not the
+ total count of bytes associated with the
+ entry_. This value represents the length of
+ the "formatted" portion of the entry. This
+ "formatted" region is sometimes followed by
+ the "unformatted" region composed of nul
+ terminated strings, with termination signalled
+ by a two nul characters in series.
+ raw : The raw bytes of the entry. This includes the
+ "formatted" portion of the entry, the
+ "unformatted" strings portion of the entry,
+ and the two terminating nul characters.
+ type : The type of the entry. This value is the same
+ as found in the directory name. It indicates
+ how the rest of the entry should be
+ interpreted.
+ instance: The instance ordinal of the entry for the
+ given type. This value is the same as found
+ in the parent directory name.
+ position: The position of the entry within the entirety
+ of the entirety.
+
+ === Entry Specialization ===
+
+ Some entry types may have other information available in
+ sysfs.
+
+ --- Type 15 - System Event Log ---
+
+ This entry allows the firmware to export a log of
+ events the system has taken. This information is
+ typically backed by nvram, but the implementation
+ details are abstracted by this table. This entries data
+ is exported in the directory:
+
+ /sys/firmware/dmi/entries/15-0/system_event_log
+
+ and has the following attributes (documented in the
+ SMBIOS / DMI specification under "System Event Log (Type 15)":
+
+ area_length
+ header_start_offset
+ data_start_offset
+ access_method
+ status
+ change_token
+ access_method_address
+ header_format
+ per_log_type_descriptor_length
+ type_descriptors_supported_count
+
+ As well, the kernel exports the binary attribute:
+
+ raw_event_log : The raw binary bits of the event log
+ as described by the DMI entry.

2011-02-23 01:54:08

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 3/5] firmware: Break out system_event_log in dmi-sysfs

The optional type 15 entry of the DMI table describes a non-volatile
storage-backed system event log.

In preparation for the next commit which exposes the raw bits of the
event log to userland, create a new sub-directory within the dmi entry
called "system_event_log" and expose attribute files that describe the
event log itself.

Currently, only a single child object is permitted within a
dmi_sysfs_entry. We simply point at this child from the dmi_sysfs_entry
if it exists.

Signed-off-by: Mike Waychison <[email protected]>
---
drivers/firmware/dmi-sysfs.c | 159 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 159 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index 2d8a04a..d209610 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -35,6 +35,7 @@ struct dmi_sysfs_entry {
int instance;
int position;
struct list_head list;
+ struct kobject *child;
};

/*
@@ -77,6 +78,10 @@ struct dmi_sysfs_mapped_attribute dmi_sysfs_attr_##_entry##_##_name = { \
/*************************************************
* Generic DMI entry support.
*************************************************/
+static void dmi_entry_free(struct kobject *kobj)
+{
+ kfree(kobj);
+}

static struct dmi_sysfs_entry *to_entry(struct kobject *kobj)
{
@@ -186,6 +191,146 @@ static size_t dmi_entry_length(const struct dmi_header *dh)
}

/*************************************************
+ * Support bits for specialized DMI entry support
+ *************************************************/
+struct dmi_entry_attr_show_data {
+ struct attribute *attr;
+ char *buf;
+};
+
+static ssize_t dmi_entry_attr_show_helper(struct dmi_sysfs_entry *entry,
+ const struct dmi_header *dh,
+ void *_data)
+{
+ struct dmi_entry_attr_show_data *data = _data;
+ struct dmi_sysfs_mapped_attribute *attr;
+
+ attr = container_of(data->attr,
+ struct dmi_sysfs_mapped_attribute, attr);
+ return attr->show(entry, dh, data->buf);
+}
+
+static ssize_t dmi_entry_attr_show(struct kobject *kobj,
+ struct attribute *attr,
+ char *buf)
+{
+ struct dmi_entry_attr_show_data data = {
+ .attr = attr,
+ .buf = buf,
+ };
+ /* Find the entry according to our parent and call the
+ * normalized show method hanging off of the attribute */
+ return find_dmi_entry(to_entry(kobj->parent),
+ dmi_entry_attr_show_helper, &data);
+}
+
+static const struct sysfs_ops dmi_sysfs_specialize_attr_ops = {
+ .show = dmi_entry_attr_show,
+};
+
+/*************************************************
+ * Specialized DMI entry support.
+ *************************************************/
+
+/*** Type 15 - System Event Table ***/
+
+#define DMI_SEL_ACCESS_METHOD_IO8 0x00
+#define DMI_SEL_ACCESS_METHOD_IO2x8 0x01
+#define DMI_SEL_ACCESS_METHOD_IO16 0x02
+#define DMI_SEL_ACCESS_METHOD_PHYS32 0x03
+#define DMI_SEL_ACCESS_METHOD_GPNV 0x04
+
+struct dmi_system_event_log {
+ struct dmi_header header;
+ u16 area_length;
+ u16 header_start_offset;
+ u16 data_start_offset;
+ u8 access_method;
+ u8 status;
+ u32 change_token;
+ union {
+ struct {
+ u16 index_addr;
+ u16 data_addr;
+ } io;
+ u32 phys_addr32;
+ u16 gpnv_handle;
+ u32 access_method_address;
+ };
+ u8 header_format;
+ u8 type_descriptors_supported_count;
+ u8 per_log_type_descriptor_length;
+ u8 supported_log_type_descriptos[0];
+} __packed;
+
+static const struct dmi_system_event_log *to_sel(const struct dmi_header *dh)
+{
+ return (const struct dmi_system_event_log *)dh;
+}
+
+#define DMI_SYSFS_SEL_FIELD(_field) \
+static ssize_t dmi_sysfs_sel_##_field(struct dmi_sysfs_entry *entry, \
+ const struct dmi_header *dh, \
+ char *buf) \
+{ \
+ const struct dmi_system_event_log *sel = to_sel(dh); \
+ if (sizeof(*sel) > dmi_entry_length(dh)) \
+ return -EIO; \
+ return sprintf(buf, "%u\n", sel->_field); \
+} \
+static DMI_SYSFS_MAPPED_ATTR(sel, _field)
+
+DMI_SYSFS_SEL_FIELD(area_length);
+DMI_SYSFS_SEL_FIELD(header_start_offset);
+DMI_SYSFS_SEL_FIELD(data_start_offset);
+DMI_SYSFS_SEL_FIELD(access_method);
+DMI_SYSFS_SEL_FIELD(status);
+DMI_SYSFS_SEL_FIELD(change_token);
+DMI_SYSFS_SEL_FIELD(access_method_address);
+DMI_SYSFS_SEL_FIELD(header_format);
+DMI_SYSFS_SEL_FIELD(type_descriptors_supported_count);
+DMI_SYSFS_SEL_FIELD(per_log_type_descriptor_length);
+
+static struct attribute *dmi_sysfs_sel_attrs[] = {
+ &dmi_sysfs_attr_sel_area_length.attr,
+ &dmi_sysfs_attr_sel_header_start_offset.attr,
+ &dmi_sysfs_attr_sel_data_start_offset.attr,
+ &dmi_sysfs_attr_sel_access_method.attr,
+ &dmi_sysfs_attr_sel_status.attr,
+ &dmi_sysfs_attr_sel_change_token.attr,
+ &dmi_sysfs_attr_sel_access_method_address.attr,
+ &dmi_sysfs_attr_sel_header_format.attr,
+ &dmi_sysfs_attr_sel_type_descriptors_supported_count.attr,
+ &dmi_sysfs_attr_sel_per_log_type_descriptor_length.attr,
+ NULL,
+};
+
+
+static struct kobj_type dmi_system_event_log_ktype = {
+ .release = dmi_entry_free,
+ .sysfs_ops = &dmi_sysfs_specialize_attr_ops,
+ .default_attrs = dmi_sysfs_sel_attrs,
+};
+
+static int dmi_system_event_log(struct dmi_sysfs_entry *entry)
+{
+ int ret;
+
+ entry->child = kzalloc(sizeof(*entry->child), GFP_KERNEL);
+ if (!entry->child)
+ return -ENOMEM;
+ ret = kobject_init_and_add(entry->child,
+ &dmi_system_event_log_ktype,
+ &entry->kobj,
+ "system_event_log");
+ if (ret)
+ goto out_free;
+out_free:
+ kfree(entry->child);
+ return ret;
+}
+
+/*************************************************
* Generic DMI entry support.
*************************************************/

@@ -325,6 +470,18 @@ static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,
list_add_tail(&entry->list, &entry_list);
spin_unlock(&entry_list_lock);

+ /* Handle specializations by type */
+ switch (dh->type) {
+ case DMI_ENTRY_SYSTEM_EVENT_LOG:
+ *ret = dmi_system_event_log(entry);
+ break;
+ default:
+ /* No specialization */
+ break;
+ }
+ if (*ret)
+ goto out_err;
+
/* Create the raw binary file to access the entry */
*ret = sysfs_create_bin_file(&entry->kobj, &dmi_entry_raw_attr);
if (*ret)
@@ -332,6 +489,7 @@ static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,

return;
out_err:
+ kobject_put(entry->child);
kobject_put(&entry->kobj);
return;
}
@@ -342,6 +500,7 @@ static void cleanup_entry_list(void)

/* No locks, we are on our way out */
list_for_each_entry_safe(entry, next, &entry_list, list) {
+ kobject_put(entry->child);
kobject_put(&entry->kobj);
}
}

2011-02-23 19:46:05

by Luck, Tony

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] firmware: Basic dmi-sysfs support

On Tue, Feb 22, 2011 at 5:53 PM, Mike Waychison <[email protected]> wrote:
> +static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *_ret)
> +{

I'm seeing some unaligned references from this function
on ia64. Code is loading a 2-byte value from an odd
address. We are dereferencing "dh", so looking at the
dmi_header definition, I'd have thought that we must
be touching dh->handle:

struct dmi_header {
u8 type;
u8 length;
u16 handle;
};

I don't see any code in this function that does this,
so I assume there is some inlining or macro stuff
happening.

-Tony

2011-02-23 20:29:12

by Mike Waychison

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] firmware: Basic dmi-sysfs support

On Wed, Feb 23, 2011 at 11:43 AM, Tony Luck <[email protected]> wrote:
> On Tue, Feb 22, 2011 at 5:53 PM, Mike Waychison <[email protected]> wrote:
>> +static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *_ret)
>> +{
>
> I'm seeing some unaligned references from this function
> on ia64. ?Code is loading a 2-byte value from an odd
> address. We are dereferencing "dh", so looking at the
> dmi_header definition, I'd have thought that we must
> be touching dh->handle:
>
> struct dmi_header {
> ? ? ? ?u8 type;
> ? ? ? ?u8 length;
> ? ? ? ?u16 handle;
> };
>
> I don't see any code in this function that does this,
> so I assume there is some inlining or macro stuff
> happening.

Ya, I don't think there is anything that guarantees that dmi_header is
aligned. The access is probably the struct copy I did here:

/* Set the key */
entry->dh = *dh;

What if we changed that guy to use memcpy? We'd probably need to
memcpy the dmi_system_event_log as well. Patch attached (applies on
top of the entire patchset).


Attachments:
patch (2.32 kB)

2011-02-23 21:29:49

by Luck, Tony

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] firmware: Basic dmi-sysfs support

On Wed, Feb 23, 2011 at 12:28 PM, Mike Waychison <[email protected]> wrote:
> Ya, I don't think there is anything that guarantees that dmi_header is
> aligned. ? The access is probably the struct copy I did here:
>
> ? ? ? ?/* Set the key */
> ? ? ? ?entry->dh = *dh;

Yup - looking back at the assembly I see that one of the 2-byte
accesses is offset 2
(picking up ->handle), but the second is offset 0 (picking up ->type
and ->length). The
compiler assumed the structure was 2-byte aligned because of ->handle.

> What if we changed that guy to use memcpy? ?We'd probably need to
> memcpy the dmi_system_event_log as well. ?Patch attached (applies on
> top of the entire patchset).

With the patch, all the unaligned accesses go away.

Tested-by: Tony Luck <[email protected]>

-Tony

2011-02-25 20:00:08

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] firmware: Basic dmi-sysfs support

On Wed, Feb 23, 2011 at 01:29:46PM -0800, Tony Luck wrote:
> On Wed, Feb 23, 2011 at 12:28 PM, Mike Waychison <[email protected]> wrote:
> > Ya, I don't think there is anything that guarantees that dmi_header is
> > aligned. ? The access is probably the struct copy I did here:
> >
> > ? ? ? ?/* Set the key */
> > ? ? ? ?entry->dh = *dh;
>
> Yup - looking back at the assembly I see that one of the 2-byte
> accesses is offset 2
> (picking up ->handle), but the second is offset 0 (picking up ->type
> and ->length). The
> compiler assumed the structure was 2-byte aligned because of ->handle.
>
> > What if we changed that guy to use memcpy? ?We'd probably need to
> > memcpy the dmi_system_event_log as well. ?Patch attached (applies on
> > top of the entire patchset).
>
> With the patch, all the unaligned accesses go away.
>
> Tested-by: Tony Luck <[email protected]>

Great!

Mike, care to resend this last patch as a 6/5 patch with a proper
subject, changelog comment, and signed-off-by: section so that I can
queue it up?

thanks,

greg k-h

2011-02-25 20:03:42

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v2 0/5] Exporting DMI entries via sysfs

On Tue, Feb 22, 2011 at 05:53:07PM -0800, Mike Waychison wrote:
> This patchset applies to v2.6.38-rc6.
>
> The following series exports information the DMI / SMBIOS tables via
> sysfs under the path /sys/firmware/dmi.

Looks good, all queued up now, thanks for following through with this.

greg k-h

2011-02-25 23:11:53

by Mike Waychison

[permalink] [raw]
Subject: [PATCH v2 6/5] Fix unaligned memory accesses in dmi-sysfs

DMI entries are arranged in memory back to back with no alignment
guarantees. This means that the struct dmi_header passed to callbacks
from dmi_walk() itself isn't byte aligned. This causes problems on
architectures that expect aligned data, such as IA64.

The dmi-sysfs patchset introduced structure member accesses through
this passed in dmi_header. Fix this by memcpy()ing the structures to
temporary locations on stack when inspecting/copying them.

Signed-off-by: Mike Waychison <[email protected]>
Tested-by: Tony Luck <[email protected]>
---
dmi-sysfs.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index a5afd80..eb26d62 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -263,20 +263,16 @@ struct dmi_system_event_log {
u8 supported_log_type_descriptos[0];
} __packed;

-static const struct dmi_system_event_log *to_sel(const struct
dmi_header *dh)
-{
- return (const struct dmi_system_event_log *)dh;
-}
-
#define DMI_SYSFS_SEL_FIELD(_field) \
static ssize_t dmi_sysfs_sel_##_field(struct dmi_sysfs_entry *entry, \
const struct dmi_header *dh, \
char *buf) \
{ \
- const struct dmi_system_event_log *sel = to_sel(dh); \
- if (sizeof(*sel) > dmi_entry_length(dh)) \
+ struct dmi_system_event_log sel; \
+ if (sizeof(sel) > dmi_entry_length(dh)) \
return -EIO; \
- return sprintf(buf, "%u\n", sel->_field); \
+ memcpy(&sel, dh, sizeof(sel)); \
+ return sprintf(buf, "%u\n", sel._field); \
} \
static DMI_SYSFS_MAPPED_ATTR(sel, _field)

@@ -403,26 +399,28 @@ static ssize_t dmi_sel_raw_read_helper(struct
dmi_sysfs_entry *entry,
void *_state)
{
struct dmi_read_state *state = _state;
- const struct dmi_system_event_log *sel = to_sel(dh);
+ struct dmi_system_event_log sel;

- if (sizeof(*sel) > dmi_entry_length(dh))
+ if (sizeof(sel) > dmi_entry_length(dh))
return -EIO;

- switch (sel->access_method) {
+ memcpy(&sel, dh, sizeof(sel));
+
+ switch (sel.access_method) {
case DMI_SEL_ACCESS_METHOD_IO8:
case DMI_SEL_ACCESS_METHOD_IO2x8:
case DMI_SEL_ACCESS_METHOD_IO16:
- return dmi_sel_raw_read_io(entry, sel, state->buf,
+ return dmi_sel_raw_read_io(entry, &sel, state->buf,
state->pos, state->count);
case DMI_SEL_ACCESS_METHOD_PHYS32:
- return dmi_sel_raw_read_phys32(entry, sel, state->buf,
+ return dmi_sel_raw_read_phys32(entry, &sel, state->buf,
state->pos, state->count);
case DMI_SEL_ACCESS_METHOD_GPNV:
pr_info("dmi-sysfs: GPNV support missing.\n");
return -EIO;
default:
pr_info("dmi-sysfs: Unknown access method %02x\n",
- sel->access_method);
+ sel.access_method);
return -EIO;
}
}
@@ -595,7 +593,7 @@ static void __init dmi_sysfs_register_handle(const
struct dmi_header *dh,
}

/* Set the key */
- entry->dh = *dh;
+ memcpy(&entry->dh, dh, sizeof(*dh));
entry->instance = instance_counts[dh->type]++;
entry->position = position_count++;

2011-02-25 23:20:54

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v2 6/5] Fix unaligned memory accesses in dmi-sysfs

On Fri, Feb 25, 2011 at 03:06:25PM -0800, Mike Waychison wrote:
> DMI entries are arranged in memory back to back with no alignment
> guarantees. This means that the struct dmi_header passed to callbacks
> from dmi_walk() itself isn't byte aligned. This causes problems on
> architectures that expect aligned data, such as IA64.
>
> The dmi-sysfs patchset introduced structure member accesses through
> this passed in dmi_header. Fix this by memcpy()ing the structures to
> temporary locations on stack when inspecting/copying them.
>
> Signed-off-by: Mike Waychison <[email protected]>
> Tested-by: Tony Luck <[email protected]>
> ---
> dmi-sysfs.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> index a5afd80..eb26d62 100644
> --- a/drivers/firmware/dmi-sysfs.c
> +++ b/drivers/firmware/dmi-sysfs.c
> @@ -263,20 +263,16 @@ struct dmi_system_event_log {
> u8 supported_log_type_descriptos[0];
> } __packed;
>
> -static const struct dmi_system_event_log *to_sel(const struct
> dmi_header *dh)

This patch is corrupted. I tried to edit it by hand, but git still
didn't like it.

Please resend using whatever you did for your other patches, as this way
was not correct :(

thanks,

greg k-h

2011-02-25 23:42:15

by Mike Waychison

[permalink] [raw]
Subject: [resend PATCH v2 6/5] Fix unaligned memory accesses in dmi-sysfs

DMI entries are arranged in memory back to back with no alignment
guarantees. This means that the struct dmi_header passed to callbacks
from dmi_walk() itself isn't byte aligned. This causes problems on
architectures that expect aligned data, such as IA64.

The dmi-sysfs patchset introduced structure member accesses through this
passed in dmi_header. Fix this by memcpy()ing the structures to
temporary locations on stack when inspecting/copying them.

Signed-off-by: Mike Waychison <[email protected]>
Tested-by: Tony Luck <[email protected]>
---
drivers/firmware/dmi-sysfs.c | 28 +++++++++++++---------------
1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
index a5afd80..eb26d62 100644
--- a/drivers/firmware/dmi-sysfs.c
+++ b/drivers/firmware/dmi-sysfs.c
@@ -263,20 +263,16 @@ struct dmi_system_event_log {
u8 supported_log_type_descriptos[0];
} __packed;

-static const struct dmi_system_event_log *to_sel(const struct dmi_header *dh)
-{
- return (const struct dmi_system_event_log *)dh;
-}
-
#define DMI_SYSFS_SEL_FIELD(_field) \
static ssize_t dmi_sysfs_sel_##_field(struct dmi_sysfs_entry *entry, \
const struct dmi_header *dh, \
char *buf) \
{ \
- const struct dmi_system_event_log *sel = to_sel(dh); \
- if (sizeof(*sel) > dmi_entry_length(dh)) \
+ struct dmi_system_event_log sel; \
+ if (sizeof(sel) > dmi_entry_length(dh)) \
return -EIO; \
- return sprintf(buf, "%u\n", sel->_field); \
+ memcpy(&sel, dh, sizeof(sel)); \
+ return sprintf(buf, "%u\n", sel._field); \
} \
static DMI_SYSFS_MAPPED_ATTR(sel, _field)

@@ -403,26 +399,28 @@ static ssize_t dmi_sel_raw_read_helper(struct dmi_sysfs_entry *entry,
void *_state)
{
struct dmi_read_state *state = _state;
- const struct dmi_system_event_log *sel = to_sel(dh);
+ struct dmi_system_event_log sel;

- if (sizeof(*sel) > dmi_entry_length(dh))
+ if (sizeof(sel) > dmi_entry_length(dh))
return -EIO;

- switch (sel->access_method) {
+ memcpy(&sel, dh, sizeof(sel));
+
+ switch (sel.access_method) {
case DMI_SEL_ACCESS_METHOD_IO8:
case DMI_SEL_ACCESS_METHOD_IO2x8:
case DMI_SEL_ACCESS_METHOD_IO16:
- return dmi_sel_raw_read_io(entry, sel, state->buf,
+ return dmi_sel_raw_read_io(entry, &sel, state->buf,
state->pos, state->count);
case DMI_SEL_ACCESS_METHOD_PHYS32:
- return dmi_sel_raw_read_phys32(entry, sel, state->buf,
+ return dmi_sel_raw_read_phys32(entry, &sel, state->buf,
state->pos, state->count);
case DMI_SEL_ACCESS_METHOD_GPNV:
pr_info("dmi-sysfs: GPNV support missing.\n");
return -EIO;
default:
pr_info("dmi-sysfs: Unknown access method %02x\n",
- sel->access_method);
+ sel.access_method);
return -EIO;
}
}
@@ -595,7 +593,7 @@ static void __init dmi_sysfs_register_handle(const struct dmi_header *dh,
}

/* Set the key */
- entry->dh = *dh;
+ memcpy(&entry->dh, dh, sizeof(*dh));
entry->instance = instance_counts[dh->type]++;
entry->position = position_count++;