LinuxLists.cc - [PATCH 1/3] pstore: Extend API

[permalink] [raw]

Subject: [PATCH 2/3] pstore: Add extra context for writes and erases

EFI only provides small amounts of individual storage, and conventionally
puts metadata in the storage variable name. Rather than add a metadata
header to the (already limited) variable storage, it's easier for us to
modify pstore to pass all the information we need to construct a unique
variable name to the appropriate functions.

Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/acpi/apei/erst.c | 10 ++++++----
fs/pstore/inode.c | 6 ++++--
fs/pstore/platform.c | 14 +++++---------
include/linux/pstore.h | 5 +++--
4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index de3ae92..d842ac4 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -933,9 +933,10 @@ static int erst_open_pstore(struct pstore_info *psi);
static int erst_close_pstore(struct pstore_info *psi);
static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
struct timespec *time, struct pstore_info *psi);
-static u64 erst_writer(enum pstore_type_id type, size_t size,
+static u64 erst_writer(enum pstore_type_id type, int part, size_t size,
struct pstore_info *psi);
-static int erst_clearer(u64 id, struct pstore_info *psi);
+static int erst_clearer(enum pstore_type_id type, u64 id,
+ struct pstore_info *psi);

static struct pstore_info erst_info = {
.owner = THIS_MODULE,
@@ -1039,7 +1040,7 @@ out:
return (rc < 0) ? rc : (len - sizeof(*rcd));
}

-static u64 erst_writer(enum pstore_type_id type, size_t size,
+static u64 erst_writer(enum pstore_type_id type, int part, size_t size,
struct pstore_info *psi)
{
struct cper_pstore_record *rcd = (struct cper_pstore_record *)
@@ -1083,7 +1084,8 @@ static u64 erst_writer(enum pstore_type_id type, size_t size,
return rcd->hdr.record_id;
}

-static int erst_clearer(u64 id, struct pstore_info *psi)
+static int erst_clearer(enum pstore_type_id type, u64 id,
+ struct pstore_info *psi)
{
return erst_clear(id);
}
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index b19884a..893b961 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -39,8 +39,9 @@
#define PSTORE_NAMELEN 64

struct pstore_private {
- u64 id;
struct pstore_info *psi;
+ enum pstore_type_id type;
+ u64 id;
ssize_t size;
char data[];
};
@@ -73,7 +74,7 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
{
struct pstore_private *p = dentry->d_inode->i_private;

- p->psi->erase(p->id, p->psi);
+ p->psi->erase(p->type, p->id, p->psi);

return simple_unlink(dir, dentry);
}
@@ -192,6 +193,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
private = kmalloc(sizeof *private + size, GFP_KERNEL);
if (!private)
goto fail_alloc;
+ private->type = type;
private->id = id;
private->psi = psi;

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 221c04e..958397c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -78,7 +78,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
oopscount++;
while (total < kmsg_bytes) {
dst = psinfo->buf;
- hsize = sprintf(dst, "%s#%d Part%d\n", why, oopscount, part++);
+ hsize = sprintf(dst, "%s#%d Part%d\n", why, oopscount, part);
size = psinfo->bufsize - hsize;
dst += hsize;

@@ -94,8 +94,8 @@ static void pstore_dump(struct kmsg_dumper *dumper,
memcpy(dst, s1 + s1_start, l1_cpy);
memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);

- id = psinfo->write(PSTORE_TYPE_DMESG, hsize + l1_cpy + l2_cpy,
- psinfo);
+ id = psinfo->write(PSTORE_TYPE_DMESG, part,
+ hsize + l1_cpy + l2_cpy, psinfo);
if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
psinfo->buf, hsize + l1_cpy + l2_cpy,
@@ -103,6 +103,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
l1 -= l1_cpy;
l2 -= l2_cpy;
total += l1_cpy + l2_cpy;
+ part++;
}
mutex_unlock(&psinfo->buf_mutex);
}
@@ -132,11 +133,6 @@ int pstore_register(struct pstore_info *psi)
psinfo = psi;
spin_unlock(&pstore_lock);

- if (owner && !try_module_get(owner)) {
- psinfo = NULL;
- return -EINVAL;
- }
-
if (pstore_is_mounted())
pstore_get_records();

@@ -197,7 +193,7 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)

mutex_lock(&psinfo->buf_mutex);
memcpy(psinfo->buf, buf, size);
- id = psinfo->write(type, size, psinfo);
+ id = psinfo->write(type, 0, size, psinfo);
if (pstore_is_mounted())
pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
size, CURRENT_TIME, psinfo);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index b2f1d97..12be8f1 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -39,9 +39,10 @@ struct pstore_info {
int (*close)(struct pstore_info *psi);
ssize_t (*read)(u64 *id, enum pstore_type_id *type,
struct timespec *time, struct pstore_info *psi);
- u64 (*write)(enum pstore_type_id type, size_t size,
+ u64 (*write)(enum pstore_type_id type, int part,
+ size_t size, struct pstore_info *psi);
+ int (*erase)(enum pstore_type_id type, u64 id,
struct pstore_info *psi);
- int (*erase)(u64 id, struct pstore_info *psi);
void *data;
};

--
1.7.5.2

2011-06-06 19:39:21

[permalink] [raw]

Subject: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

EFI provides an area of nonvolatile storage managed by the firmware. We
can use this as a pstore backend to maintain copies of oopses, aiding
diagnosis.

Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/firmware/efivars.c | 158 +++++++++++++++++++++++++++++++++++++++++++-
include/linux/efi.h | 3 +
2 files changed, 159 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5f29aaf..89e6a3a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -78,6 +78,7 @@
#include <linux/kobject.h>
#include <linux/device.h>
#include <linux/slab.h>
+#include <linux/pstore.h>

#include <asm/uaccess.h>

@@ -89,6 +90,8 @@ MODULE_DESCRIPTION("sysfs interface to EFI Variables");
MODULE_LICENSE("GPL");
MODULE_VERSION(EFIVARS_VERSION);

+#define DUMP_NAME_LEN 52
+
/*
* The maximum size of VariableName + Data = 1024
* Therefore, it's reasonable to save that much
@@ -161,18 +164,28 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
}

static efi_status_t
-get_var_data(struct efivars *efivars, struct efi_variable *var)
+get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
{
efi_status_t status;

- spin_lock(&efivars->lock);
var->DataSize = 1024;
status = efivars->ops->get_variable(var->VariableName,
&var->VendorGuid,
&var->Attributes,
&var->DataSize,
var->Data);
+ return status;
+}
+
+static efi_status_t
+get_var_data(struct efivars *efivars, struct efi_variable *var)
+{
+ efi_status_t status;
+
+ spin_lock(&efivars->lock);
+ status = get_var_data_locked(efivars, var);
spin_unlock(&efivars->lock);
+
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
status);
@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
.default_attrs = def_attrs,
};

+static struct efivar_entry *walk_entry;
+
+static struct pstore_info efi_pstore_info;
+
static inline void
efivar_unregister(struct efivar_entry *var)
{
kobject_put(&var->kobj);
}

+static int efi_pstore_open(struct pstore_info *psi)
+{
+ struct efivars *efivars = psi->data;
+
+ spin_lock(&efivars->lock);
+ walk_entry = list_first_entry(&efivars->list, struct efivar_entry,
+ list);
+ return 0;
+}
+
+static int efi_pstore_close(struct pstore_info *psi)
+{
+ struct efivars *efivars = psi->data;
+
+ spin_unlock(&efivars->lock);
+ return 0;
+}
+
+static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
+ struct timespec *timespec, struct pstore_info *psi)
+{
+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+ struct efivars *efivars = psi->data;
+ char name[DUMP_NAME_LEN];
+ int i;
+ unsigned int part, size;
+ unsigned long time;
+
+ while (&walk_entry->list != &efivars->list) {
+ if (!efi_guidcmp(walk_entry->var.VendorGuid, vendor)) {
+ for (i = 0; i < DUMP_NAME_LEN; i++) {
+ name[i] = walk_entry->var.VariableName[i];
+ }
+ if (sscanf(name, "dump-type%u-%u-%lu", type, &part, &time) == 3) {
+ *id = part;
+ timespec->tv_sec = time;
+ timespec->tv_nsec = 0;
+ get_var_data_locked(efivars, &walk_entry->var);
+ size = walk_entry->var.DataSize;
+ memcpy(psi->buf, walk_entry->var.Data, size);
+ walk_entry = list_entry(walk_entry->list.next,
+ struct efivar_entry, list);
+ return size;
+ }
+ }
+ walk_entry = list_entry(walk_entry->list.next,
+ struct efivar_entry, list);
+ }
+ return 0;
+}
+
+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
+ struct pstore_info *psi)
+{
+ char name[DUMP_NAME_LEN];
+ char stub_name[DUMP_NAME_LEN];
+ efi_char16_t efi_name[DUMP_NAME_LEN];
+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+ struct efivars *efivars = psi->data;
+ struct efivar_entry *entry, *found = NULL;
+ int i;
+
+ sprintf(stub_name, "dump-type%u-%u-", type, part);
+ sprintf(name, "%s%lu", stub_name, get_seconds());
+
+ spin_lock(&efivars->lock);
+
+ for (i = 0; i < DUMP_NAME_LEN; i++)
+ efi_name[i] = stub_name[i];
+
+ /*
+ * Clean up any entries with the same name
+ */
+
+ list_for_each_entry(entry, &efivars->list, list) {
+ get_var_data_locked(efivars, &entry->var);
+
+ for (i = 0; i < DUMP_NAME_LEN; i++) {
+ if (efi_name[i] == 0) {
+ found = entry;
+ efi.set_variable(entry->var.VariableName, &entry->var.VendorGuid,
+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
+ break;
+ } else if (efi_name[i] != entry->var.VariableName[i]) {
+ break;
+ }
+ }
+ }
+
+ if (found)
+ list_del(&found->list);
+
+ for (i = 0; i < DUMP_NAME_LEN; i++)
+ efi_name[i] = name[i];
+
+ efi.set_variable(efi_name, &vendor,
+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ size, psi->buf);
+
+ spin_unlock(&efivars->lock);
+
+ if (found)
+ efivar_unregister(found);
+
+ if (size)
+ efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
+ efi_name, &vendor);
+
+ return part;
+};
+
+static int efi_pstore_erase(enum pstore_type_id type, u64 id,
+ struct pstore_info *psi)
+{
+ efi_pstore_write(type, id, 0, psi);
+
+ return 0;
+}
+
+static struct pstore_info efi_pstore_info = {
+ .owner = THIS_MODULE,
+ .name = "efi",
+ .open = efi_pstore_open,
+ .close = efi_pstore_close,
+ .read = efi_pstore_read,
+ .write = efi_pstore_write,
+ .erase = efi_pstore_erase,
+};

static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr,
@@ -763,6 +909,14 @@ int register_efivars(struct efivars *efivars,
if (error)
unregister_efivars(efivars);

+ efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+ if (efi_pstore_info.buf) {
+ efi_pstore_info.bufsize = 1024;
+ efi_pstore_info.data = efivars;
+ mutex_init(&efi_pstore_info.buf_mutex);
+ pstore_register(&efi_pstore_info);
+ }
+
out:
kfree(variable_name);

diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec25726..8dd9a01 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -232,6 +232,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
#define UV_SYSTEM_TABLE_GUID \
EFI_GUID( 0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93 )

+#define LINUX_EFI_CRASH_GUID \
+ EFI_GUID( 0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0 )
+
typedef struct {
efi_guid_t guid;
unsigned long table;
--
1.7.5.2

2011-06-06 21:43:48

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Mon, Jun 6, 2011 at 12:38 PM, Matthew Garrett <[email protected]> wrote:
- if (owner && !try_module_get(owner)) {
- psinfo = NULL;
- return -EINVAL;
- }
-

Apart from the accidental deletion of this chunk (as discussed on
IRC), it all looks
good to me. The ERST stuff still works, I didn't try the new EFI bits. So you
can have:

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

to add to these ... unless I'm the one pushing them to Linus, in which case I'll
add my Signed-off-by for parts 1 & 2

Does someone claim maintainership of drivers/firmware/efivars.c for part3?

-Tony

2011-06-06 21:47:43

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:

> Does someone claim maintainership of drivers/firmware/efivars.c for part3?

I think that'd be Matt.

--
Matthew Garrett | [email protected]

2011-06-06 23:11:11

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On Mon, Jun 6, 2011 at 12:38 PM, Matthew Garrett <[email protected]> wrote:
> EFI provides an area of nonvolatile storage managed by the firmware. We
> can use this as a pstore backend to maintain copies of oopses, aiding
> diagnosis.

Most of the new lines you add to efivars.c should be inside

#ifdef CONFIG_PSTORE
...
#endif

so that they don't bloat the kernel if PSTORE isn't configured.

I may have made a poor decision having drivers/acpi/apei/Kconfig
do a "select PSTORE" for the ACPI_APEI option.
It should probably be a normal "def_bool n" sort of thing.

-Tony

2011-06-07 15:48:50

by Seiji Aguchi

[permalink] [raw]

Subject: RE: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

Hi,

Recently, I have suggested similar patches.
Does your patch satisfy my purpose?

[RFC][PATCH] kmsg_dumper for NVRAM
http://choon.net/forum/read.php?21,84718

[RFC][PATCH] pstore: EFI Support
http://choon.net/forum/read.php?21,84718,85162

Seiji

>-----Original Message-----
>From: [email protected] [mailto:[email protected]] On Behalf Of Matthew Garrett
>Sent: Monday, June 06, 2011 3:39 PM
>To: [email protected]
>Cc: [email protected]; [email protected]; Matthew Garrett
>Subject: [PATCH 3/3] efi: Add support for using efivars as a pstore backend
>
>EFI provides an area of nonvolatile storage managed by the firmware. We
>can use this as a pstore backend to maintain copies of oopses, aiding
>diagnosis.
>
>Signed-off-by: Matthew Garrett <[email protected]>
>---
> drivers/firmware/efivars.c | 158 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/efi.h | 3 +
> 2 files changed, 159 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
>index 5f29aaf..89e6a3a 100644
>--- a/drivers/firmware/efivars.c
>+++ b/drivers/firmware/efivars.c
>@@ -78,6 +78,7 @@
> #include <linux/kobject.h>
> #include <linux/device.h>
> #include <linux/slab.h>
>+#include <linux/pstore.h>
>
> #include <asm/uaccess.h>
>
>@@ -89,6 +90,8 @@ MODULE_DESCRIPTION("sysfs interface to EFI Variables");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(EFIVARS_VERSION);
>
>+#define DUMP_NAME_LEN 52
>+
> /*
> * The maximum size of VariableName + Data = 1024
> * Therefore, it's reasonable to save that much
>@@ -161,18 +164,28 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
> }
>
> static efi_status_t
>-get_var_data(struct efivars *efivars, struct efi_variable *var)
>+get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
> {
> efi_status_t status;
>
>- spin_lock(&efivars->lock);
> var->DataSize = 1024;
> status = efivars->ops->get_variable(var->VariableName,
> &var->VendorGuid,
> &var->Attributes,
> &var->DataSize,
> var->Data);
>+ return status;
>+}
>+
>+static efi_status_t
>+get_var_data(struct efivars *efivars, struct efi_variable *var)
>+{
>+ efi_status_t status;
>+
>+ spin_lock(&efivars->lock);
>+ status = get_var_data_locked(efivars, var);
> spin_unlock(&efivars->lock);
>+
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
> status);
>@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
> .default_attrs = def_attrs,
> };
>
>+static struct efivar_entry *walk_entry;
>+
>+static struct pstore_info efi_pstore_info;
>+
> static inline void
> efivar_unregister(struct efivar_entry *var)
> {
> kobject_put(&var->kobj);
> }
>
>+static int efi_pstore_open(struct pstore_info *psi)
>+{
>+ struct efivars *efivars = psi->data;
>+
>+ spin_lock(&efivars->lock);
>+ walk_entry = list_first_entry(&efivars->list, struct efivar_entry,
>+ list);
>+ return 0;
>+}
>+
>+static int efi_pstore_close(struct pstore_info *psi)
>+{
>+ struct efivars *efivars = psi->data;
>+
>+ spin_unlock(&efivars->lock);
>+ return 0;
>+}
>+
>+static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>+ struct timespec *timespec, struct pstore_info *psi)
>+{
>+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>+ struct efivars *efivars = psi->data;
>+ char name[DUMP_NAME_LEN];
>+ int i;
>+ unsigned int part, size;
>+ unsigned long time;
>+
>+ while (&walk_entry->list != &efivars->list) {
>+ if (!efi_guidcmp(walk_entry->var.VendorGuid, vendor)) {
>+ for (i = 0; i < DUMP_NAME_LEN; i++) {
>+ name[i] = walk_entry->var.VariableName[i];
>+ }
>+ if (sscanf(name, "dump-type%u-%u-%lu", type, &part, &time) == 3) {
>+ *id = part;
>+ timespec->tv_sec = time;
>+ timespec->tv_nsec = 0;
>+ get_var_data_locked(efivars, &walk_entry->var);
>+ size = walk_entry->var.DataSize;
>+ memcpy(psi->buf, walk_entry->var.Data, size);
>+ walk_entry = list_entry(walk_entry->list.next,
>+ struct efivar_entry, list);
>+ return size;
>+ }
>+ }
>+ walk_entry = list_entry(walk_entry->list.next,
>+ struct efivar_entry, list);
>+ }
>+ return 0;
>+}
>+
>+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
>+ struct pstore_info *psi)
>+{
>+ char name[DUMP_NAME_LEN];
>+ char stub_name[DUMP_NAME_LEN];
>+ efi_char16_t efi_name[DUMP_NAME_LEN];
>+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>+ struct efivars *efivars = psi->data;
>+ struct efivar_entry *entry, *found = NULL;
>+ int i;
>+
>+ sprintf(stub_name, "dump-type%u-%u-", type, part);
>+ sprintf(name, "%s%lu", stub_name, get_seconds());
>+
>+ spin_lock(&efivars->lock);
>+
>+ for (i = 0; i < DUMP_NAME_LEN; i++)
>+ efi_name[i] = stub_name[i];
>+
>+ /*
>+ * Clean up any entries with the same name
>+ */
>+
>+ list_for_each_entry(entry, &efivars->list, list) {
>+ get_var_data_locked(efivars, &entry->var);
>+
>+ for (i = 0; i < DUMP_NAME_LEN; i++) {
>+ if (efi_name[i] == 0) {
>+ found = entry;
>+ efi.set_variable(entry->var.VariableName, &entry->var.VendorGuid,
>+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
>EFI_VARIABLE_RUNTIME_ACCESS,
>+ 0, NULL);
>+ break;
>+ } else if (efi_name[i] != entry->var.VariableName[i]) {
>+ break;
>+ }
>+ }
>+ }
>+
>+ if (found)
>+ list_del(&found->list);
>+
>+ for (i = 0; i < DUMP_NAME_LEN; i++)
>+ efi_name[i] = name[i];
>+
>+ efi.set_variable(efi_name, &vendor,
>+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
>+ size, psi->buf);
>+
>+ spin_unlock(&efivars->lock);
>+
>+ if (found)
>+ efivar_unregister(found);
>+
>+ if (size)
>+ efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
>+ efi_name, &vendor);
>+
>+ return part;
>+};
>+
>+static int efi_pstore_erase(enum pstore_type_id type, u64 id,
>+ struct pstore_info *psi)
>+{
>+ efi_pstore_write(type, id, 0, psi);
>+
>+ return 0;
>+}
>+
>+static struct pstore_info efi_pstore_info = {
>+ .owner = THIS_MODULE,
>+ .name = "efi",
>+ .open = efi_pstore_open,
>+ .close = efi_pstore_close,
>+ .read = efi_pstore_read,
>+ .write = efi_pstore_write,
>+ .erase = efi_pstore_erase,
>+};
>
> static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr,
>@@ -763,6 +909,14 @@ int register_efivars(struct efivars *efivars,
> if (error)
> unregister_efivars(efivars);
>
>+ efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
>+ if (efi_pstore_info.buf) {
>+ efi_pstore_info.bufsize = 1024;
>+ efi_pstore_info.data = efivars;
>+ mutex_init(&efi_pstore_info.buf_mutex);
>+ pstore_register(&efi_pstore_info);
>+ }
>+
> out:
> kfree(variable_name);
>
>diff --git a/include/linux/efi.h b/include/linux/efi.h
>index ec25726..8dd9a01 100644
>--- a/include/linux/efi.h
>+++ b/include/linux/efi.h
>@@ -232,6 +232,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
> #define UV_SYSTEM_TABLE_GUID \
> EFI_GUID( 0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93 )
>
>+#define LINUX_EFI_CRASH_GUID \
>+ EFI_GUID( 0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0 )
>+
> typedef struct {
> efi_guid_t guid;
> unsigned long table;
>--
>1.7.5.2
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at http://www.tux.org/lkml/

2011-06-07 15:58:44

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On Tue, Jun 07, 2011 at 11:47:38AM -0400, Seiji Aguchi wrote:
> Hi,
>
> Recently, I have suggested similar patches.
> Does your patch satisfy my purpose?

I think so. I've been testing it without problems on one machine and I'm
about to try it on a different vendor. It seemed easier to reuse the
efivars infrastructure than writing another implementation, and the
efivars code has at least been tested enough for boot configuration to
work.

--
Matthew Garrett | [email protected]

2011-06-07 18:04:34

by Randy Dunlap

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On Mon, 6 Jun 2011 15:38:55 -0400 Matthew Garrett wrote:

> EFI provides an area of nonvolatile storage managed by the firmware. We
> can use this as a pstore backend to maintain copies of oopses, aiding
> diagnosis.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/firmware/efivars.c | 158 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/efi.h | 3 +
> 2 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 5f29aaf..89e6a3a 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -161,18 +164,28 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
> }
>
> static efi_status_t
> -get_var_data(struct efivars *efivars, struct efi_variable *var)
> +get_var_data_locked(struct efivars *efivars, struct efi_variable *var)

__get_var_data() would be a more common/typical name for this (unlocked,
needs locking version), I think.

> {
> efi_status_t status;
>
> - spin_lock(&efivars->lock);
> var->DataSize = 1024;
> status = efivars->ops->get_variable(var->VariableName,
> &var->VendorGuid,
> &var->Attributes,
> &var->DataSize,
> var->Data);
> + return status;
> +}
> +
> +static efi_status_t
> +get_var_data(struct efivars *efivars, struct efi_variable *var)
> +{
> + efi_status_t status;
> +
> + spin_lock(&efivars->lock);
> + status = get_var_data_locked(efivars, var);
> spin_unlock(&efivars->lock);
> +
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
> status);

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

2011-06-10 04:00:17

by Matt Domsch

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>
> > Does someone claim maintainership of drivers/firmware/efivars.c for part3?
>
> I think that'd be Matt.

I'm out of the office this week and next, and haven't looked at it
closely. Mike Waychison has done most of the editing in there
recently, I'd be happy if he wishes to be considered the maintainer
thereof.

Thanks,
Matt

--
Matt Domsch
Technology Strategist
Dell | Office of the CTO

Attachments:

(No filename) (526.00 B)
(No filename) (481.00 B)
Download all attachments

2011-06-10 07:13:06

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <[email protected]> wrote:
> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>
>> > Does someone claim maintainership of ?drivers/firmware/efivars.c for part3?
>>
>> I think that'd be Matt.
>
> I'm out of the office this week and next, and haven't looked at it
> closely. ?Mike Waychison has done most of the editing in there
> recently, I'd be happy if he wishes to be considered the maintainer
> thereof.

I'm out of the office too until next Tuesday. I should be able to
look at this between now and mid next-week.

>
> Thanks,
> Matt
>
> --
> Matt Domsch
> Technology Strategist
> Dell | Office of the CTO
>

2011-06-20 22:27:32

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Fri, Jun 10, 2011 at 12:12 AM, Mike Waychison <[email protected]> wrote:
> On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <[email protected]> wrote:
>> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>>
>>> > Does someone claim maintainership of ?drivers/firmware/efivars.c for part3?
>>>
>>> I think that'd be Matt.
>>
>> I'm out of the office this week and next, and haven't looked at it
>> closely. ?Mike Waychison has done most of the editing in there
>> recently, I'd be happy if he wishes to be considered the maintainer
>> thereof.
>
> I'm out of the office too until next Tuesday. ?I should be able to
> look at this between now and mid next-week.

Mike; Did you find time to look at Matthew's efi-for-pstore patches yet?
If we want to hit the 3.1 merge window we should be trying to get these
beaten into shape pretty soon (Linus will presumably declare -rc4 tonight,
so not much time left).

-Tony

2011-06-20 22:30:30

[permalink] [raw]

Subject: Re: [PATCH 2/3] pstore: Add extra context for writes and erases

On Mon, Jun 20, 2011 at 3:27 PM, Tony Luck <[email protected]> wrote:
> On Fri, Jun 10, 2011 at 12:12 AM, Mike Waychison <[email protected]> wrote:
>> On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <[email protected]> wrote:
>>> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>>>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>>>
>>>> > Does someone claim maintainership of ?drivers/firmware/efivars.c for part3?
>>>>
>>>> I think that'd be Matt.
>>>
>>> I'm out of the office this week and next, and haven't looked at it
>>> closely. ?Mike Waychison has done most of the editing in there
>>> recently, I'd be happy if he wishes to be considered the maintainer
>>> thereof.
>>
>> I'm out of the office too until next Tuesday. ?I should be able to
>> look at this between now and mid next-week.
>
> Mike; Did you find time to look at Matthew's efi-for-pstore patches yet?
> If we want to hit the 3.1 merge window we should be trying to get these
> beaten into shape pretty soon (Linus will presumably declare -rc4 tonight,
> so not much time left).
>

My bad. This fell off my plate :( Looking now.

2011-06-21 00:56:35

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On 06/06/11 12:38, Matthew Garrett wrote:
> EFI provides an area of nonvolatile storage managed by the firmware. We
> can use this as a pstore backend to maintain copies of oopses, aiding
> diagnosis.

How do I configure this thing?

Inline notes below. Sorry for the delayed response.

>
> Signed-off-by: Matthew Garrett<[email protected]>
> ---
> drivers/firmware/efivars.c | 158 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/efi.h | 3 +
> 2 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 5f29aaf..89e6a3a 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -78,6 +78,7 @@
> #include<linux/kobject.h>
> #include<linux/device.h>
> #include<linux/slab.h>
> +#include<linux/pstore.h>
>
> #include<asm/uaccess.h>
>
> @@ -89,6 +90,8 @@ MODULE_DESCRIPTION("sysfs interface to EFI Variables");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(EFIVARS_VERSION);
>
> +#define DUMP_NAME_LEN 52
> +
> /*
> * The maximum size of VariableName + Data = 1024
> * Therefore, it's reasonable to save that much
> @@ -161,18 +164,28 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
> }
>
> static efi_status_t
> -get_var_data(struct efivars *efivars, struct efi_variable *var)
> +get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
> {
> efi_status_t status;
>
> - spin_lock(&efivars->lock);
> var->DataSize = 1024;
> status = efivars->ops->get_variable(var->VariableName,
> &var->VendorGuid,
> &var->Attributes,
> &var->DataSize,
> var->Data);
> + return status;
> +}
> +
> +static efi_status_t
> +get_var_data(struct efivars *efivars, struct efi_variable *var)
> +{
> + efi_status_t status;
> +
> + spin_lock(&efivars->lock);
> + status = get_var_data_locked(efivars, var);
> spin_unlock(&efivars->lock);
> +
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
> status);
> @@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
> .default_attrs = def_attrs,
> };
>
> +static struct efivar_entry *walk_entry;
> +
> +static struct pstore_info efi_pstore_info;

Can you move these into struct efivars in efi.h?

> +
> static inline void
> efivar_unregister(struct efivar_entry *var)
> {
> kobject_put(&var->kobj);
> }
>
> +static int efi_pstore_open(struct pstore_info *psi)
> +{
> + struct efivars *efivars = psi->data;
> +
> + spin_lock(&efivars->lock);
> + walk_entry = list_first_entry(&efivars->list, struct efivar_entry,
> + list);
> + return 0;
> +}
> +
> +static int efi_pstore_close(struct pstore_info *psi)
> +{
> + struct efivars *efivars = psi->data;
> +
> + spin_unlock(&efivars->lock);
> + return 0;
> +}
> +
> +static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> + struct timespec *timespec, struct pstore_info *psi)
> +{
> + efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> + struct efivars *efivars = psi->data;
> + char name[DUMP_NAME_LEN];
> + int i;
> + unsigned int part, size;
> + unsigned long time;
> +
> + while (&walk_entry->list !=&efivars->list) {
> + if (!efi_guidcmp(walk_entry->var.VendorGuid, vendor)) {
> + for (i = 0; i< DUMP_NAME_LEN; i++) {
> + name[i] = walk_entry->var.VariableName[i];
> + }
> + if (sscanf(name, "dump-type%u-%u-%lu", type,&part,&time) == 3) {
> + *id = part;
> + timespec->tv_sec = time;
> + timespec->tv_nsec = 0;
> + get_var_data_locked(efivars,&walk_entry->var);
> + size = walk_entry->var.DataSize;
> + memcpy(psi->buf, walk_entry->var.Data, size);
> + walk_entry = list_entry(walk_entry->list.next,
> + struct efivar_entry, list);
> + return size;
> + }
> + }
> + walk_entry = list_entry(walk_entry->list.next,
> + struct efivar_entry, list);
> + }
> + return 0;
> +}
> +
> +static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
> + struct pstore_info *psi)
> +{
> + char name[DUMP_NAME_LEN];
> + char stub_name[DUMP_NAME_LEN];
> + efi_char16_t efi_name[DUMP_NAME_LEN];
> + efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> + struct efivars *efivars = psi->data;
> + struct efivar_entry *entry, *found = NULL;
> + int i;
> +
> + sprintf(stub_name, "dump-type%u-%u-", type, part);

The format specifier here uses an unsigned, but your series passes part
around as a signed int. If part is truely non-negative, consider
changing it to unsigned?

> + sprintf(name, "%s%lu", stub_name, get_seconds());
> +
> + spin_lock(&efivars->lock);
> +
> + for (i = 0; i< DUMP_NAME_LEN; i++)
> + efi_name[i] = stub_name[i];
> +
> + /*
> + * Clean up any entries with the same name
> + */
> +
> + list_for_each_entry(entry,&efivars->list, list) {
> + get_var_data_locked(efivars,&entry->var);
> +
> + for (i = 0; i< DUMP_NAME_LEN; i++) {
> + if (efi_name[i] == 0) {

Test for entry->var.VariableName[i] == 0 too. Actually, could we just
turn this string comparing loop into a strncmp test?

> + found = entry;
> + efi.set_variable(entry->var.VariableName,&entry->var.VendorGuid,

space after comma

> + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> + 0, NULL);

We shouldn't be calling the global efi ops structure here. Instead, we
should be using efivars->ops->set_variable(...)

> + break;
> + } else if (efi_name[i] != entry->var.VariableName[i]) {
> + break;
> + }
> + }
> + }
> +
> + if (found)
> + list_del(&found->list);
> +
> + for (i = 0; i< DUMP_NAME_LEN; i++)
> + efi_name[i] = name[i];
> +
> + efi.set_variable(efi_name,&vendor,
> + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> + size, psi->buf);

efivars->ops->set_variable.

> +
> + spin_unlock(&efivars->lock);
> +
> + if (found)
> + efivar_unregister(found);
> +
> + if (size)
> + efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
> + efi_name,&vendor);
> +
> + return part;
> +};
> +
> +static int efi_pstore_erase(enum pstore_type_id type, u64 id,
> + struct pstore_info *psi)
> +{
> + efi_pstore_write(type, id, 0, psi);
> +
> + return 0;
> +}
> +
> +static struct pstore_info efi_pstore_info = {
> + .owner = THIS_MODULE,
> + .name = "efi",
> + .open = efi_pstore_open,
> + .close = efi_pstore_close,
> + .read = efi_pstore_read,
> + .write = efi_pstore_write,
> + .erase = efi_pstore_erase,
> +};
>
> static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr,
> @@ -763,6 +909,14 @@ int register_efivars(struct efivars *efivars,
> if (error)
> unregister_efivars(efivars);
>
> + efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
> + if (efi_pstore_info.buf) {
> + efi_pstore_info.bufsize = 1024;
> + efi_pstore_info.data = efivars;
> + mutex_init(&efi_pstore_info.buf_mutex);
> + pstore_register(&efi_pstore_info);
> + }
> +

Hmm. pstore doesn't have a pstore_unregister? This is unfortunate
because this breaks efivars module unloading :(

> out:
> kfree(variable_name);
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index ec25726..8dd9a01 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -232,6 +232,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
> #define UV_SYSTEM_TABLE_GUID \
> EFI_GUID( 0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93 )
>
> +#define LINUX_EFI_CRASH_GUID \
> + EFI_GUID( 0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0 )
> +
> typedef struct {
> efi_guid_t guid;
> unsigned long table;

2011-06-21 15:10:54

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On Mon, Jun 20, 2011 at 05:56:27PM -0700, Mike Waychison wrote:
> On 06/06/11 12:38, Matthew Garrett wrote:
> >EFI provides an area of nonvolatile storage managed by the firmware. We
> >can use this as a pstore backend to maintain copies of oopses, aiding
> >diagnosis.
>
> How do I configure this thing?

You don't. I'll be posting a patch for pstore that lets you choose the
backend - that can be used to disable this functionality at boot time.

> >@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
> > .default_attrs = def_attrs,
> > };
> >
> >+static struct efivar_entry *walk_entry;
> >+
> >+static struct pstore_info efi_pstore_info;
>
> Can you move these into struct efivars in efi.h?

In theory, but I don't really understand the benefit. You can't have
more than one efivars implementation on a system.

> >+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
> >+ struct pstore_info *psi)
> >+{
> >+ char name[DUMP_NAME_LEN];
> >+ char stub_name[DUMP_NAME_LEN];
> >+ efi_char16_t efi_name[DUMP_NAME_LEN];
> >+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> >+ struct efivars *efivars = psi->data;
> >+ struct efivar_entry *entry, *found = NULL;
> >+ int i;
> >+
> >+ sprintf(stub_name, "dump-type%u-%u-", type, part);
>
> The format specifier here uses an unsigned, but your series passes
> part around as a signed int. If part is truely non-negative,
> consider changing it to unsigned?

The variable name is fundamentally meaningless. Just think of it as a
binary representation of the data. Formatting it as a signed integer
would break the parsing. But you're right that there's probably no
reason for it to be signed in the first place - Tony?

> >+ list_for_each_entry(entry,&efivars->list, list) {
> >+ get_var_data_locked(efivars,&entry->var);
> >+
> >+ for (i = 0; i< DUMP_NAME_LEN; i++) {
> >+ if (efi_name[i] == 0) {
>
> Test for entry->var.VariableName[i] == 0 too. Actually, could we
> just turn this string comparing loop into a strncmp test?

The idea is to check for prefixes. If efi_name[i] is non-zero but
VariableName[i] is zero then we'll break due to them not matching, which
is the desired behaviour.

> >+ found = entry;
> >+ efi.set_variable(entry->var.VariableName,&entry->var.VendorGuid,
>
> space after comma

Oops.

> >+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> >+ 0, NULL);
>
> We shouldn't be calling the global efi ops structure here. Instead,
> we should be using efivars->ops->set_variable(...)

Ok.

> > static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> > struct bin_attribute *bin_attr,
> >@@ -763,6 +909,14 @@ int register_efivars(struct efivars *efivars,
> > if (error)
> > unregister_efivars(efivars);
> >
> >+ efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
> >+ if (efi_pstore_info.buf) {
> >+ efi_pstore_info.bufsize = 1024;
> >+ efi_pstore_info.data = efivars;
> >+ mutex_init(&efi_pstore_info.buf_mutex);
> >+ pstore_register(&efi_pstore_info);
> >+ }
> >+
>
> Hmm. pstore doesn't have a pstore_unregister? This is unfortunate
> because this breaks efivars module unloading :(

Userspace really ought to depend on efivars being available on EFI
systems. I don't think losing the ability to unload it is a big loss.

--
Matthew Garrett | [email protected]

2011-06-21 17:12:25

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

>> How do I configure this thing?
>
> You don't. I'll be posting a patch for pstore that lets you choose the
> backend - that can be used to disable this functionality at boot time.

For EFI - you need to configure CONFIG_PSTORE (which you may not be
able to see until you first set CONFIG_MISCFILESYSTEMS) ... and of course
you'll need CONFIG_EFIVARS. At the moment (until Matthew provides the
new pstore boot time choosing argument, you might need to set CONFIG_ACPI_APEI=n
because otherwise its a link order race to see who gets to register
their back end first.

Once you boot into that kernel you can mount pstore:

# mount -t pstore - /dev/pstore

and once you crash/shutdown/reboot and mount it again - you should see
some files
in /dev/pstore with the tail of the saved console log from the
previous kernel. Remove
the files to clear the underlying store (in your case delete the efi
variables that saved
the text of the console log).

>> >+ ? ?sprintf(stub_name, "dump-type%u-%u-", type, part);
>>
>> The format specifier here uses an unsigned, but your series passes
>> part around as a signed int. ?If part is truely non-negative,
>> consider changing it to unsigned?
>
> The variable name is fundamentally meaningless. Just think of it as a
> binary representation of the data. Formatting it as a signed integer
> would break the parsing. But you're right that there's probably no
> reason for it to be signed in the first place - Tony?

part numbers are integers (1, 2, 3 ...) - so making it unsigned makes sense.

>> Hmm. ?pstore doesn't have a pstore_unregister? ?This is unfortunate
>> because this breaks efivars module unloading :(
>
> Userspace really ought to depend on efivars being available on EFI
> systems. I don't think losing the ability to unload it is a big loss.

If there is a strong demand for this - maybe someone will write a patch
for it? But in general, once you attach a backend to pstore, I'd expect
you to leave it attached - you don't know whether the system is about
to crash, and you'd lose the kernel log if you crashed while the backend
was not registered.

-Tony

2011-06-21 20:16:59

[permalink] [raw]

Subject: Re: [PATCH 3/3] efi: Add support for using efivars as a pstore backend

On Tue, Jun 21, 2011 at 8:10 AM, Matthew Garrett <[email protected]> wrote:
> On Mon, Jun 20, 2011 at 05:56:27PM -0700, Mike Waychison wrote:
>> On 06/06/11 12:38, Matthew Garrett wrote:
>> >EFI provides an area of nonvolatile storage managed by the firmware. We
>> >can use this as a pstore backend to maintain copies of oopses, aiding
>> >diagnosis.
>>
>> How do I configure this thing?
>
> You don't. I'll be posting a patch for pstore that lets you choose the
> backend - that can be used to disable this functionality at boot time.

Hmm. Okay.

>
>> >@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
>> > ? ? .default_attrs = def_attrs,
>> > ?};
>> >
>> >+static struct efivar_entry *walk_entry;
>> >+
>> >+static struct pstore_info efi_pstore_info;
>>
>> Can you move these into struct efivars in efi.h?
>
> In theory, but I don't really understand the benefit. You can't have
> more than one efivars implementation on a system.

It's just cleaner and keeps the state of things in a single place,
rather than in globals. I just cleaned all this global state up, and
would like to keep it clean.

>
>> >+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
>> >+ ? ? ? ? ? ? ? ? ? ? ? ?struct pstore_info *psi)
>> >+{
>> >+ ? ?char name[DUMP_NAME_LEN];
>> >+ ? ?char stub_name[DUMP_NAME_LEN];
>> >+ ? ?efi_char16_t efi_name[DUMP_NAME_LEN];
>> >+ ? ?efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
>> >+ ? ?struct efivars *efivars = psi->data;
>> >+ ? ?struct efivar_entry *entry, *found = NULL;
>> >+ ? ?int i;
>> >+
>> >+ ? ?sprintf(stub_name, "dump-type%u-%u-", type, part);
>>
>> The format specifier here uses an unsigned, but your series passes
>> part around as a signed int. ?If part is truely non-negative,
>> consider changing it to unsigned?
>
> The variable name is fundamentally meaningless. Just think of it as a
> binary representation of the data. Formatting it as a signed integer
> would break the parsing.

What do you mean it would break the parsing?

> But you're right that there's probably no
> reason for it to be signed in the first place - Tony?
>
>> >+ ? ?list_for_each_entry(entry,&efivars->list, list) {
>> >+ ? ? ? ? ? ?get_var_data_locked(efivars,&entry->var);
>> >+
>> >+ ? ? ? ? ? ?for (i = 0; i< ?DUMP_NAME_LEN; i++) {
>> >+ ? ? ? ? ? ? ? ? ? ?if (efi_name[i] == 0) {
>>
>> Test for entry->var.VariableName[i] == 0 too. ?Actually, could we
>> just turn this string comparing loop into a strncmp test?
>
> The idea is to check for prefixes. If efi_name[i] is non-zero but
> VariableName[i] is zero then we'll break due to them not matching, which
> is the desired behaviour.

That's fine, but it's not what the code does. It's also a lot clearer
to the reader if this isn't open coded. We should also be checking
that this variable is ours with a guid check. I'd be happier with a
utf16_strncmp here. Will follow up with patches that stack on top of
yours.

>> > ?static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
>> > ? ? ? ? ? ? ? ? ? ? ? ? ?struct bin_attribute *bin_attr,
>> >@@ -763,6 +909,14 @@ int register_efivars(struct efivars *efivars,
>> > ? ? if (error)
>> > ? ? ? ? ? ? unregister_efivars(efivars);
>> >
>> >+ ? ?efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
>> >+ ? ?if (efi_pstore_info.buf) {
>> >+ ? ? ? ? ? ?efi_pstore_info.bufsize = 1024;
>> >+ ? ? ? ? ? ?efi_pstore_info.data = efivars;
>> >+ ? ? ? ? ? ?mutex_init(&efi_pstore_info.buf_mutex);
>> >+ ? ? ? ? ? ?pstore_register(&efi_pstore_info);
>> >+ ? ?}
>> >+
>>
>> Hmm. ?pstore doesn't have a pstore_unregister? ?This is unfortunate
>> because this breaks efivars module unloading :(
>
> Userspace really ought to depend on efivars being available on EFI
> systems. I don't think losing the ability to unload it is a big loss.

I don't know of any such dependencies. Am I missing something?

I'll follow up with more patches that apply on top of yours. I'm not
happy with the string operations in the driver as it is, and I've
cleaned some of this up. Feel free to collapse whatever is needed
from them into your series or pick them up as is.

2011-06-21 20:19:18

[permalink] [raw]

Subject: [PATCH 1/4] efivars: String functions

Fix the string functions in the efivars driver to be called utf16_*
instead of utf8_* as the encoding is utf16, not utf8.

As well, rename utf16_strlen to utf16_strnlen as it takes a maxlength
argument and the name should be consistent with the standard C function
names. utf16_strlen is still provided for convenience in a subsequent
patch.

Signed-off-by: Mike Waychison <[email protected]>
---
drivers/firmware/efivars.c | 30 +++++++++++++++++++-----------
1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 89e6a3a..f10c760 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -144,23 +144,29 @@ efivar_create_sysfs_entry(struct efivars *efivars,

/* Return the number of unicode characters in data */
static unsigned long
-utf8_strlen(efi_char16_t *data, unsigned long maxlength)
+utf16_strnlen(efi_char16_t *s, size_t maxlength)
{
unsigned long length = 0;

- while (*data++ != 0 && length < maxlength)
+ while (*s++ != 0 && length < maxlength)
length++;
return length;
}

+static unsigned long
+utf16_strlen(efi_char16_t *s)
+{
+ return utf16_strnlen(s, ~0UL);
+}
+
/*
* Return the number of bytes is the length of this string
* Note: this is NOT the same as the number of unicode characters
*/
static inline unsigned long
-utf8_strsize(efi_char16_t *data, unsigned long maxlength)
+utf16_strsize(efi_char16_t *data, unsigned long maxlength)
{
- return utf8_strlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
+ return utf16_strnlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
}

static efi_status_t
@@ -516,7 +522,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
efivar_unregister(found);

if (size)
- efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
+ efivar_create_sysfs_entry(efivars,
+ utf16_strsize(efi_name,
+ DUMP_NAME_LEN * 2),
efi_name, &vendor);

return part;
@@ -560,8 +568,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
* Does this variable already exist?
*/
list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
- strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
- strsize2 = utf8_strsize(new_var->VariableName, 1024);
+ strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
+ strsize2 = utf16_strsize(new_var->VariableName, 1024);
if (strsize1 == strsize2 &&
!memcmp(&(search_efivar->var.VariableName),
new_var->VariableName, strsize1) &&
@@ -593,8 +601,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,

/* Create the entry in sysfs. Locking is not required here */
status = efivar_create_sysfs_entry(efivars,
- utf8_strsize(new_var->VariableName,
- 1024),
+ utf16_strsize(new_var->VariableName,
+ 1024),
new_var->VariableName,
&new_var->VendorGuid);
if (status) {
@@ -623,8 +631,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
* Does this variable already exist?
*/
list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
- strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
- strsize2 = utf8_strsize(del_var->VariableName, 1024);
+ strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
+ strsize2 = utf16_strsize(del_var->VariableName, 1024);
if (strsize1 == strsize2 &&
!memcmp(&(search_efivar->var.VariableName),
del_var->VariableName, strsize1) &&
--
1.7.3.1

2011-06-21 20:18:45

[permalink] [raw]

Subject: [PATCH 2/4] efivars: introduce utf16_strncmp

Introduce utf16_strncmp which is used in the next patch. Semantics
should be the same as the strncmp C function.

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

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index f10c760..fb1219a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -169,6 +169,24 @@ utf16_strsize(efi_char16_t *data, unsigned long maxlength)
return utf16_strnlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
}

+static inline int
+utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len)
+{
+ while (1) {
+ if (len == 0)
+ return 0;
+ if (*a < *b)
+ return -1;
+ if (*a > *b)
+ return 1;
+ if (*a == 0) /* implies *b == 0 */
+ return 0;
+ a++;
+ b++;
+ len--;
+ }
+}
+
static efi_status_t
get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
{
--
1.7.3.1

2011-06-21 20:18:43

[permalink] [raw]

Subject: [PATCH 3/4] efivars: Use string functions in pstore_write

Instead of open-coding the string operations for comparing the prefix of
the variable names, use the provided utf16_* string functions.

This patch also changes the calls to efi.set_variable to
efivars->ops->set_variable so that the right function gets called in the
case of gsmi (which doesn't have a valid efi structure).

As well, make sure that we only consider variables with the right vendor
string.

Signed-off-by: Mike Waychison <[email protected]>
---
drivers/firmware/efivars.c | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fb1219a..f424b10 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -511,17 +511,20 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
list_for_each_entry(entry, &efivars->list, list) {
get_var_data_locked(efivars, &entry->var);

- for (i = 0; i < DUMP_NAME_LEN; i++) {
- if (efi_name[i] == 0) {
- found = entry;
- efi.set_variable(entry->var.VariableName, &entry->var.VendorGuid,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
- 0, NULL);
- break;
- } else if (efi_name[i] != entry->var.VariableName[i]) {
- break;
- }
- }
+ if (efi_guidcmp(entry->var.VendorGuid, vendor))
+ continue;
+ if (utf16_strncmp(entry->var.VariableName, efi_name,
+ utf16_strlen(efi_name)))
+ continue;
+ /* Needs to be a prefix */
+ if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
+ continue;
+
+ /* found */
+ found = entry;
+ efivars->ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid,
+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ 0, NULL);
}

if (found)
@@ -530,9 +533,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];

- efi.set_variable(efi_name, &vendor,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
- size, psi->buf);
+ efivars->ops->set_variable(efi_name, &vendor,
+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ size, psi->buf);

spin_unlock(&efivars->lock);

--
1.7.3.1

2011-06-21 20:18:41

[permalink] [raw]

Subject: [PATCH 4/4] efivars: Introduce PSTORE_EFI_ATTRIBUTES

Consolidate the attributes listed for pstore operations in one place,
PSTORE_EFI_ATTRIBUTES.

Signed-off-by: Mike Waychison <[email protected]>
---
drivers/firmware/efivars.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index f424b10..739994b8 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -122,6 +122,10 @@ struct efivar_attribute {
ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
};

+#define PSTORE_EFI_ATTRIBUTES \
+ (EFI_VARIABLE_NON_VOLATILE | \
+ EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+ EFI_VARIABLE_RUNTIME_ACCESS)

#define EFIVAR_ATTR(_name, _mode, _show, _store) \
struct efivar_attribute efivar_attr_##_name = { \
@@ -522,8 +526,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,

/* found */
found = entry;
- efivars->ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ PSTORE_EFI_ATTRIBUTES,
0, NULL);
}

@@ -533,8 +538,7 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];

- efivars->ops->set_variable(efi_name, &vendor,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+ efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
size, psi->buf);

spin_unlock(&efivars->lock);
--
1.7.3.1

2011-06-21 20:23:14