[PATCH 1/2] efi: Don't use spinlocks for efi vars

[PATCH 1/2] efi: Don't use spinlocks for efi vars

[sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

All efivars operations are protected by a spinlock which prevents
interruptions and preemption. This is too restricted, we just need a
lock preventing concurency.
The idea is to use a semaphore of count 1 and to have two ways of
locking, depending on the context:
 - In interrupt context, we call down_trylock(), if it fails we return
 an error
 - In normal context, we call down_interruptible()

We don't use a mutex here because the mutex_trylock() function must not
be called from interrupt context, whereas the down_trylock() can.

This patch also remove the efivars lock to use a single lock for the
whole vars.c file. The goal of this lock is to protect concurrent calls
to efi variable services, registering and unregistering.
This allows to register new efivars operations without having
in-progress call.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/efi-pstore.c |  34 +++++++---
 drivers/firmware/efi/efivars.c    |  10 ++-
 drivers/firmware/efi/vars.c       | 130 +++++++++++++++++++++++++-------------
 fs/efivarfs/inode.c               |   5 +-
 fs/efivarfs/super.c               |   8 ++-
 include/linux/efi.h               |  12 +---
 6 files changed, 130 insertions(+), 69 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index eac76a79a880..c604af1243fd 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -121,16 +121,19 @@ static void efi_pstore_scan_sysfs_enter(struct efivar_entry *pos,
  * @entry: deleting entry
  * @turn_off_scanning: Check if a scanning flag should be turned off
  */
-static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
+static inline int __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
 						bool turn_off_scanning)
 {
 	if (entry->deleting) {
 		list_del(&entry->list);
 		efivar_entry_iter_end();
 		efivar_unregister(entry);
-		efivar_entry_iter_begin();
+		if (efivar_entry_iter_begin())
+			return -EINTR;
 	} else if (turn_off_scanning)
 		entry->scanning = false;
+
+	return 0;
 }
 
 /**
@@ -140,13 +143,16 @@ static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
  * @head: list head
  * @stop: a flag checking if scanning will stop
  */
-static void efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
+static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
 				       struct efivar_entry *next,
 				       struct list_head *head, bool stop)
 {
-	__efi_pstore_scan_sysfs_exit(pos, true);
+	int ret = __efi_pstore_scan_sysfs_exit(pos, true);
+	if (ret)
+		return ret;
 	if (stop)
-		__efi_pstore_scan_sysfs_exit(next, &next->list != head);
+		ret = __efi_pstore_scan_sysfs_exit(next, &next->list != head);
+	return ret;
 }
 
 /**
@@ -168,13 +174,17 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 	struct efivar_entry *entry, *n;
 	struct list_head *head = &efivar_sysfs_list;
 	int size = 0;
+	int ret;
 
 	if (!*pos) {
 		list_for_each_entry_safe(entry, n, head, list) {
 			efi_pstore_scan_sysfs_enter(entry, n, head);
 
 			size = efi_pstore_read_func(entry, data);
-			efi_pstore_scan_sysfs_exit(entry, n, head, size < 0);
+			ret = efi_pstore_scan_sysfs_exit(entry, n, head,
+							 size < 0);
+			if (ret)
+				return ret;
 			if (size)
 				break;
 		}
@@ -186,7 +196,9 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 		efi_pstore_scan_sysfs_enter((*pos), n, head);
 
 		size = efi_pstore_read_func((*pos), data);
-		efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		if (ret)
+			return ret;
 		if (size)
 			break;
 	}
@@ -226,7 +238,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 	if (!*data.buf)
 		return -ENOMEM;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin()) {
+		kfree(*data.buf);
+		return -EINTR;
+	}
 	size = efi_pstore_sysfs_entry_iter(&data,
 					   (struct efivar_entry **)&psi->data);
 	efivar_entry_iter_end();
@@ -341,7 +356,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 	edata.time = time;
 	edata.name = efi_name;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, &edata, &entry);
 
 	if (found && !entry->scanning) {
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8c4cf8..3998373d92ef 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -509,7 +509,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 		vendor = del_var->VendorGuid;
 	}
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true);
 	if (!entry)
 		err = -EINVAL;
@@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 		return ret;
 
 	kobject_uevent(&new_var->kobj, KOBJ_ADD);
-	efivar_entry_add(new_var, &efivar_sysfs_list);
+	if (efivar_entry_add(new_var, &efivar_sysfs_list))
+		return -EINTR;
 
 	return 0;
 }
@@ -697,7 +699,9 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor,
 
 static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+	if (err)
+		return err;
 	efivar_unregister(entry);
 	return 0;
 }
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb10517f..8a44351211e5 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -37,6 +37,13 @@
 /* Private pointer to registered efivars */
 static struct efivars *__efivars;
 
+/*
+ * ->lock protects two things:
+ * 1) efivarfs_list and efivars_sysfs_list
+ * 2) ->ops calls
+ */
+DEFINE_SEMAPHORE(efivars_lock);
+
 static bool efivar_wq_enabled = true;
 DECLARE_WORK(efivar_work, NULL);
 EXPORT_SYMBOL_GPL(efivar_work);
@@ -376,7 +383,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		return -ENOMEM;
 	}
 
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock)) {
+		err = -EINTR;
+		goto free;
+	}
 
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
@@ -392,7 +402,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		switch (status) {
 		case EFI_SUCCESS:
 			if (!atomic)
-				spin_unlock_irq(&__efivars->lock);
+				up(&efivars_lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
@@ -410,7 +420,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
 				if (!atomic)
-					spin_lock_irq(&__efivars->lock);
+					if (down_interruptible(&efivars_lock)) {
+						err = -EINTR;
+						goto free;
+					}
 
 				status = EFI_NOT_FOUND;
 				break;
@@ -421,7 +434,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				status = EFI_NOT_FOUND;
 
 			if (!atomic)
-				spin_lock_irq(&__efivars->lock);
+				if (down_interruptible(&efivars_lock)) {
+					err = -EINTR;
+					goto free;
+				}
 
 			break;
 		case EFI_NOT_FOUND:
@@ -435,8 +451,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 
 	} while (status != EFI_NOT_FOUND);
 
-	spin_unlock_irq(&__efivars->lock);
-
+	up(&efivars_lock);
+free:
 	kfree(variable_name);
 
 	return err;
@@ -447,24 +463,34 @@ EXPORT_SYMBOL_GPL(efivar_init);
  * efivar_entry_add - add entry to variable list
  * @entry: entry to add to list
  * @head: list head
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 {
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_add(&entry->list, head);
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
 /**
  * efivar_entry_remove - remove entry from variable list
  * @entry: entry to remove from list
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_remove(struct efivar_entry *entry)
+int efivar_entry_remove(struct efivar_entry *entry)
 {
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_remove);
 
@@ -481,10 +507,10 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 }
 
 /**
@@ -507,7 +533,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -533,12 +559,14 @@ int efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
+
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   0, 0, NULL);
 	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
-		spin_unlock_irq(&__efivars->lock);
+		up(&efivars_lock);
 		return efi_status_to_err(status);
 	}
 
@@ -576,10 +604,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 	efi_char16_t *name = entry->var.VariableName;
 	efi_guid_t vendor = entry->var.VendorGuid;
 
-	spin_lock_irq(&__efivars->lock);
-
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	if (head && efivar_entry_find(name, vendor, head, false)) {
-		spin_unlock_irq(&__efivars->lock);
+		up(&efivars_lock);
 		return -EEXIST;
 	}
 
@@ -588,7 +616,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		status = ops->set_variable(name, &vendor,
 					   attributes, size, data);
 
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 
@@ -602,29 +630,28 @@ EXPORT_SYMBOL_GPL(efivar_entry_set);
  * from crash/panic handlers.
  *
  * Crucially, this function will not block if it cannot acquire
- * __efivars->lock. Instead, it returns -EBUSY.
+ * efivars_lock. Instead, it returns -EBUSY.
  */
 static int
 efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 			     u32 attributes, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&__efivars->lock, flags))
+	if (down_trylock(&efivars_lock))
 		return -EBUSY;
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	up(&efivars_lock);
 	return efi_status_to_err(status);
 }
 
@@ -649,7 +676,6 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
 	if (!ops->query_variable_store)
@@ -670,21 +696,22 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 						    size, data);
 
 	if (!block) {
-		if (!spin_trylock_irqsave(&__efivars->lock, flags))
+		if (down_trylock(&efivars_lock))
 			return -EBUSY;
 	} else {
-		spin_lock_irqsave(&__efivars->lock, flags);
+		if (down_interruptible(&efivars_lock))
+			return -EINTR;
 	}
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable(name, &vendor, attributes, size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -714,7 +741,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -757,10 +784,11 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
 
 	*size = 0;
 
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid, NULL, size, NULL);
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -786,7 +814,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -809,11 +837,12 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   attributes, size, data);
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -860,7 +889,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	 * set_variable call, and removal of the variable from the efivars
 	 * list (in the case of an authenticated delete).
 	 */
-	spin_lock_irq(&__efivars->lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 
 	/*
 	 * Ensure that the available space hasn't shrunk below the safe level
@@ -900,7 +930,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	if (status == EFI_NOT_FOUND)
 		efivar_entry_list_del_unlock(entry);
 	else
-		spin_unlock_irq(&__efivars->lock);
+		up(&efivars_lock);
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -908,7 +938,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return 0;
 
 out:
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 	return err;
 
 }
@@ -921,9 +951,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size);
  * efivar_entry_iter_end() is called. This function is usually used in
  * conjunction with __efivar_entry_iter() or efivar_entry_iter().
  */
-void efivar_entry_iter_begin(void)
+int efivar_entry_iter_begin(void)
 {
-	spin_lock_irq(&__efivars->lock);
+	return down_interruptible(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
 
@@ -934,7 +964,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
  */
 void efivar_entry_iter_end(void)
 {
-	spin_unlock_irq(&__efivars->lock);
+	up(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
 
@@ -1010,7 +1040,9 @@ int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
 {
 	int err = 0;
 
-	efivar_entry_iter_begin();
+	err = efivar_entry_iter_begin();
+	if (err)
+		return err;
 	err = __efivar_entry_iter(func, head, data, NULL);
 	efivar_entry_iter_end();
 
@@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
-	spin_lock_init(&efivars->lock);
+	if (down_trylock(&efivars_lock))
+		return -EBUSY;
+
 	efivars->ops = ops;
 	efivars->kobject = kobject;
 
 	__efivars = efivars;
 
+	up(&efivars_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(efivars_register);
@@ -1076,6 +1112,9 @@ int efivars_unregister(struct efivars *efivars)
 {
 	int rv;
 
+	if (down_trylock(&efivars_lock))
+		return -EBUSY;
+
 	if (!__efivars) {
 		printk(KERN_ERR "efivars not registered\n");
 		rv = -EINVAL;
@@ -1091,6 +1130,7 @@ int efivars_unregister(struct efivars *efivars)
 
 	rv = 0;
 out:
+	up(&efivars_lock);
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_unregister);
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9da9ee6..5cea84ef7a44 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -132,7 +132,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 
 	inode->i_private = var;
 
-	efivar_entry_add(var, &efivarfs_list);
+	err = efivar_entry_add(var, &efivarfs_list);
+	if (err)
+		goto out;
+
 	d_instantiate(dentry, inode);
 	dget(dentry);
 out:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 86a2121828c3..45ef421f545f 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -158,7 +158,9 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	kfree(name);
 
 	efivar_entry_size(entry, &size);
-	efivar_entry_add(entry, &efivarfs_list);
+	err = efivar_entry_add(entry, &efivarfs_list);
+	if (err)
+		goto fail_inode;
 
 	mutex_lock(&inode->i_mutex);
 	inode->i_private = entry;
@@ -179,7 +181,9 @@ fail:
 
 static int efivarfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+	if (err)
+		return err;
 	kfree(entry);
 	return 0;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a866bb1..2e60803fa4ba 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1096,12 +1096,6 @@ struct efivar_operations {
 };
 
 struct efivars {
-	/*
-	 * ->lock protects two things:
-	 * 1) efivarfs_list and efivars_sysfs_list
-	 * 2) ->ops calls
-	 */
-	spinlock_t lock;
 	struct kset *kset;
 	struct kobject *kobject;
 	const struct efivar_operations *ops;
@@ -1169,8 +1163,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool atomic, bool duplicates,
 		struct list_head *head);
 
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
-void efivar_entry_remove(struct efivar_entry *entry);
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
+int efivar_entry_remove(struct efivar_entry *entry);
 
 int __efivar_entry_delete(struct efivar_entry *entry);
 int efivar_entry_delete(struct efivar_entry *entry);
@@ -1187,7 +1181,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data);
 
-void efivar_entry_iter_begin(void);
+int efivar_entry_iter_begin(void);
 void efivar_entry_iter_end(void);
 
 int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
-- 
2.6.3

[PATCH 2/2] efi: implement interruptible runtime services

[sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch adds an implementation of EFI runtime services wappers which
allow interrupts and preemption during execution of BIOS code.

It's needed by Broxton platform which requires the OS to do the write
of the non volatile variables. To do that, the OS will receive an
interrupt coming from the firmware to notify that it must write the
data.

BIOS code must be executed using a specific PGD. This is currently
done by efi_call() which force value of CR3 before calling BIOS and
restore previous value after.

We use a dedicated kthread which will execute all interruptible runtime
services. This efi_kthread is special because it has it's own mm_struct
whereas kthread should not have one. This mm_struct contains the EFI PGD
so the scheduler will load it before running any BIOS code.

Obviously, an interruptible runtime service must never be called from an
interrupt context. EFI pstore is the only use case here, that's why we
require it to be disabled when activating interruptible runtime services.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/Kconfig                          |  14 +++
 arch/x86/include/asm/efi.h                |   1 +
 arch/x86/platform/efi/Makefile            |   1 +
 arch/x86/platform/efi/efi_32.c            |   5 +
 arch/x86/platform/efi/efi_64.c            |   5 +
 arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
 6 files changed, 217 insertions(+)
 create mode 100644 arch/x86/platform/efi/efi_interruptible.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..3ddd5a9cab30 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1720,6 +1720,20 @@ config EFI_MIXED
 
 	   If unsure, say N.
 
+if X86_64
+config EFI_INTERRUPTIBLE
+	bool "Interruptible Efi Runtime Services"
+	depends on EFI && !EFI_VARS_PSTORE
+	default n
+	help
+          This is an interruptible implementation of efi runtime
+	  services wrappers. If you say Y, BIOS code could be
+	  interrupted and preempted as a standard process. Enable this
+	  only if you are sure that your BIOS will support
+	  interruptible efi calls
+	  In doubt, say "N".
+endif
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0010c78c4998..5e9620b3688b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -111,6 +111,7 @@ extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
+extern pgd_t *efi_get_pgd(void);
 extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 2846aaab5103..dcc894d1e603 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_INTERRUPTIBLE)		+= efi_interruptible.o
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index ed5b67338294..02a3c616c59e 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -56,6 +56,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
+pgd_t *efi_get_pgd(void)
+{
+	return initial_page_table;
+}
+
 pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index d347e854a5e4..67cffb69d405 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -143,6 +143,11 @@ void efi_sync_low_kernel_mappings(void)
 		sizeof(pgd_t) * num_pgds);
 }
 
+pgd_t *efi_get_pgd(void)
+{
+	return __va(efi_scratch.efi_pgt);
+}
+
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
 	unsigned long text;
diff --git a/arch/x86/platform/efi/efi_interruptible.c b/arch/x86/platform/efi/efi_interruptible.c
new file mode 100644
index 000000000000..c3cf72cd2ed6
--- /dev/null
+++ b/arch/x86/platform/efi/efi_interruptible.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * Author: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
+ *
+ * Distributed under the terms of the GNU GPL, version 2
+ */
+
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+static DEFINE_KTHREAD_WORKER(efi_kworker);
+
+#define efi_call_interruptible(f, ...)					\
+({									\
+	efi_status_t __s;						\
+	efi_sync_low_kernel_mappings();					\
+	__kernel_fpu_begin();						\
+	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
+	__kernel_fpu_end();						\
+	__s;								\
+})
+
+struct efi_work {
+	struct kthread_work work;
+	efi_status_t status;
+	unsigned long *name_size;
+	efi_char16_t *name;
+	efi_guid_t *vendor;
+	u32 *attr;
+	unsigned long *data_size;
+	void *data;
+};
+
+static void do_get_next_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_next_variable, ew->name_size,
+					    ew->name, ew->vendor);
+}
+
+static void do_set_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(set_variable, ew->name, ew->vendor,
+					    *ew->attr, *ew->data_size,
+					    ew->data);
+}
+
+static void do_get_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_variable, ew->name, ew->vendor,
+					    ew->attr, ew->data_size, ew->data);
+}
+
+static efi_status_t execute_service(kthread_work_func_t service,
+				    unsigned long *name_size,
+				    efi_char16_t *name, efi_guid_t *vendor,
+				    u32 *attr, unsigned long *data_size,
+				    void *data)
+{
+	struct efi_work work = {
+		.name_size = name_size,
+		.name = name,
+		.vendor = vendor,
+		.attr = attr,
+		.data_size = data_size,
+		.data = data,
+	};
+
+	init_kthread_work(&work.work, service);
+	queue_kthread_work(&efi_kworker, &work.work);
+
+	flush_kthread_work(&work.work);
+	return work.status;
+}
+
+static efi_status_t get_next_variable_interruptible(unsigned long *name_size,
+					       efi_char16_t *name,
+					       efi_guid_t *vendor)
+{
+	return execute_service(do_get_next_variable_interruptible,
+			       name_size, name, vendor, NULL, NULL, NULL);
+}
+
+static efi_status_t set_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 attr,
+					  unsigned long data_size,
+					  void *data)
+{
+	return execute_service(do_set_variable_interruptible,
+			       NULL, name, vendor, &attr, &data_size, data);
+}
+
+static efi_status_t
+non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
+			 __attribute__((unused)) efi_guid_t *vendor,
+			 __attribute__((unused)) u32 attr,
+			 __attribute__((unused)) unsigned long data_size,
+			 __attribute__((unused)) void *data)
+{
+	pr_err("efi_interruptible: non bloking operation is not allowed\n");
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t get_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 *attr,
+					  unsigned long *data_size,
+					  void *data)
+{
+	return execute_service(do_get_variable_interruptible,
+			       NULL, name, vendor, attr, data_size, data);
+}
+
+static struct efivars interruptible_efivars;
+
+static int efi_interruptible_panic_notifier_call(
+	struct notifier_block *notifier,
+	unsigned long what, void *data)
+{
+	static struct efivars generic_efivars;
+	static struct efivar_operations generic_ops;
+
+	generic_ops.get_variable = efi.get_variable;
+	generic_ops.set_variable = efi.set_variable;
+	generic_ops.get_next_variable = efi.get_next_variable;
+	generic_ops.query_variable_store = efi_query_variable_store;
+
+	efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_nb = {
+	.notifier_call = efi_interruptible_panic_notifier_call,
+	.priority = 100,
+};
+
+static struct task_struct *efi_kworker_task;
+static struct efivar_operations interruptible_ops;
+static __init int efi_interruptible_init(void)
+{
+	int ret;
+
+	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
+					  "efi_kthread");
+	if (IS_ERR(efi_kworker_task)) {
+		pr_err("efi_interruptible: Failed to create kthread\n");
+		ret = PTR_ERR(efi_kworker_task);
+		efi_kworker_task = NULL;
+		return ret;
+	}
+
+	efi_kworker_task->mm = mm_alloc();
+	efi_kworker_task->active_mm = efi_kworker_task->mm;
+	efi_kworker_task->mm->pgd = efi_get_pgd();
+	wake_up_process(efi_kworker_task);
+
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
+
+	interruptible_ops.get_variable = get_variable_interruptible;
+	interruptible_ops.set_variable = set_variable_interruptible;
+	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
+	interruptible_ops.get_next_variable = get_next_variable_interruptible;
+	interruptible_ops.query_variable_store = efi_query_variable_store;
+	return efivars_register(&interruptible_efivars, &interruptible_ops,
+				efivars_kobject());
+}
+
+static void __exit efi_interruptible_exit(void)
+{
+	efivars_unregister(&interruptible_efivars);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &panic_nb);
+	if (efi_kworker_task) {
+		kthread_stop(efi_kworker_task);
+		mmdrop(efi_kworker_task->mm);
+	}
+}
+
+module_init(efi_interruptible_init);
+module_exit(efi_interruptible_exit);
+
+MODULE_AUTHOR("Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
2.6.3

Re: [PATCH 1/2] efi: Don't use spinlocks for efi vars

[Matt Fleming]

(Cc'ing Ard since he has recently been in this area)

On Fri, 18 Dec, at 11:29:50AM, sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> All efivars operations are protected by a spinlock which prevents
> interruptions and preemption. This is too restricted, we just need a
> lock preventing concurency.
> The idea is to use a semaphore of count 1 and to have two ways of
> locking, depending on the context:
>  - In interrupt context, we call down_trylock(), if it fails we return
>  an error
>  - In normal context, we call down_interruptible()
> 
> We don't use a mutex here because the mutex_trylock() function must not
> be called from interrupt context, whereas the down_trylock() can.
> 
> This patch also remove the efivars lock to use a single lock for the
> whole vars.c file. The goal of this lock is to protect concurrent calls
> to efi variable services, registering and unregistering.
> This allows to register new efivars operations without having
> in-progress call.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/efi-pstore.c |  34 +++++++---
>  drivers/firmware/efi/efivars.c    |  10 ++-
>  drivers/firmware/efi/vars.c       | 130 +++++++++++++++++++++++++-------------
>  fs/efivarfs/inode.c               |   5 +-
>  fs/efivarfs/super.c               |   8 ++-
>  include/linux/efi.h               |  12 +---
>  6 files changed, 130 insertions(+), 69 deletions(-)
 
This needs splitting into more than 1 patch.

You need separate patches to,

  - Split out __efivars->lock into a file local lock
  - Convert the lock to a semaphore
  - Print a message when efi_operations are registered

Further comments below.

> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 756eca8c4cf8..3998373d92ef 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -509,7 +509,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
>  		vendor = del_var->VendorGuid;
>  	}
>  
> -	efivar_entry_iter_begin();
> +	if (efivar_entry_iter_begin())
> +		return -EINTR;
>  	entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true);
>  	if (!entry)
>  		err = -EINVAL;
> @@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
>  		return ret;
>  
>  	kobject_uevent(&new_var->kobj, KOBJ_ADD);
> -	efivar_entry_add(new_var, &efivar_sysfs_list);
> +	if (efivar_entry_add(new_var, &efivar_sysfs_list))
> +		return -EINTR;
>  
>  	return 0;
>  }

This looks like it's missing a call to efivar_unregister() in the
-EINTR case.

> @@ -697,7 +699,9 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor,
>  
>  static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
>  {
> -	efivar_entry_remove(entry);
> +	int err = efivar_entry_remove(entry);
> +	if (err)
> +		return err;
>  	efivar_unregister(entry);
>  	return 0;
>  }

You now need to return early from efivars_sysfs_exit() if you hit the
error path in efivar_sysfs_destroy().

> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 70a0fb10517f..8a44351211e5 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -37,6 +37,13 @@
>  /* Private pointer to registered efivars */
>  static struct efivars *__efivars;
>  
> +/*
> + * ->lock protects two things:
> + * 1) efivarfs_list and efivars_sysfs_list
> + * 2) ->ops calls
> + */
> +DEFINE_SEMAPHORE(efivars_lock);
> +

Now it also protects registration of __efivars, so that needs to be
documented too.

>  static bool efivar_wq_enabled = true;
>  DECLARE_WORK(efivar_work, NULL);
>  EXPORT_SYMBOL_GPL(efivar_work);
> @@ -376,7 +383,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  		return -ENOMEM;
>  	}
>  
> -	spin_lock_irq(&__efivars->lock);
> +	if (down_interruptible(&efivars_lock)) {
> +		err = -EINTR;
> +		goto free;
> +	}
>  
>  	/*
>  	 * Per EFI spec, the maximum storage allocated for both
> @@ -392,7 +402,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  		switch (status) {
>  		case EFI_SUCCESS:
>  			if (!atomic)
> -				spin_unlock_irq(&__efivars->lock);
> +				up(&efivars_lock);
>  
>  			variable_name_size = var_name_strnsize(variable_name,
>  							       variable_name_size);
> @@ -410,7 +420,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  				dup_variable_bug(variable_name, &vendor_guid,
>  						 variable_name_size);
>  				if (!atomic)
> -					spin_lock_irq(&__efivars->lock);
> +					if (down_interruptible(&efivars_lock)) {
> +						err = -EINTR;
> +						goto free;
> +					}
>  
>  				status = EFI_NOT_FOUND;
>  				break;

Add braces to the if(!atomic) clause please to help with readability.

> @@ -421,7 +434,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  				status = EFI_NOT_FOUND;
>  
>  			if (!atomic)
> -				spin_lock_irq(&__efivars->lock);
> +				if (down_interruptible(&efivars_lock)) {
> +					err = -EINTR;
> +					goto free;
> +				}
>  
>  			break;
>  		case EFI_NOT_FOUND:

Ditto.

> @@ -533,12 +559,14 @@ int efivar_entry_delete(struct efivar_entry *entry)
>  	const struct efivar_operations *ops = __efivars->ops;
>  	efi_status_t status;
>  
> -	spin_lock_irq(&__efivars->lock);
> +	if (down_interruptible(&efivars_lock))
> +		return -EINTR;
> +
>  	status = ops->set_variable(entry->var.VariableName,
>  				   &entry->var.VendorGuid,
>  				   0, 0, NULL);
>  	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
> -		spin_unlock_irq(&__efivars->lock);
> +		up(&efivars_lock);
>  		return efi_status_to_err(status);
>  	}
>  

Please update the documentation for this function to note that we
return -EINTR if we can't grab the semaphore.

> @@ -576,10 +604,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
>  	efi_char16_t *name = entry->var.VariableName;
>  	efi_guid_t vendor = entry->var.VendorGuid;
>  
> -	spin_lock_irq(&__efivars->lock);
> -
> +	if (down_interruptible(&efivars_lock))
> +		return -EINTR;
>  	if (head && efivar_entry_find(name, vendor, head, false)) {
> -		spin_unlock_irq(&__efivars->lock);
> +		up(&efivars_lock);
>  		return -EEXIST;
>  	}
>  
> @@ -588,7 +616,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
>  		status = ops->set_variable(name, &vendor,
>  					   attributes, size, data);
>  
> -	spin_unlock_irq(&__efivars->lock);
> +	up(&efivars_lock);
>  
>  	return efi_status_to_err(status);
>  

Function documentation for the return values needs updating here too.

> @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
>  		     const struct efivar_operations *ops,
>  		     struct kobject *kobject)
>  {
> -	spin_lock_init(&efivars->lock);
> +	if (down_trylock(&efivars_lock))
> +		return -EBUSY;
> +

Is this correct? I would have assumed that you'd want to return -EINTR
here, not -EBUSY since if an EFI variable operation is currently
running we should spin waiting for the semaphore to be released.

>  	efivars->ops = ops;
>  	efivars->kobject = kobject;
>  
>  	__efivars = efivars;
>  
> +	up(&efivars_lock);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(efivars_register);
> @@ -1076,6 +1112,9 @@ int efivars_unregister(struct efivars *efivars)
>  {
>  	int rv;
>  
> +	if (down_trylock(&efivars_lock))
> +		return -EBUSY;
> +

Same logic applies in the unregister case.

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
> 
> This patch adds an implementation of EFI runtime services wappers which
> allow interrupts and preemption during execution of BIOS code.
> 
> It's needed by Broxton platform which requires the OS to do the write
> of the non volatile variables. To do that, the OS will receive an
> interrupt coming from the firmware to notify that it must write the
> data.
> 
> BIOS code must be executed using a specific PGD. This is currently
> done by efi_call() which force value of CR3 before calling BIOS and
> restore previous value after.
> 
> We use a dedicated kthread which will execute all interruptible runtime
> services. This efi_kthread is special because it has it's own mm_struct
> whereas kthread should not have one. This mm_struct contains the EFI PGD
> so the scheduler will load it before running any BIOS code.
> 
> Obviously, an interruptible runtime service must never be called from an
> interrupt context. EFI pstore is the only use case here, that's why we
> require it to be disabled when activating interruptible runtime services.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
> ---
>  arch/x86/Kconfig                          |  14 +++
>  arch/x86/include/asm/efi.h                |   1 +
>  arch/x86/platform/efi/Makefile            |   1 +
>  arch/x86/platform/efi/efi_32.c            |   5 +
>  arch/x86/platform/efi/efi_64.c            |   5 +
>  arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
>  6 files changed, 217 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..3ddd5a9cab30 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,20 @@ config EFI_MIXED
>  
>  	   If unsure, say N.
>  
> +if X86_64
> +config EFI_INTERRUPTIBLE
> +	bool "Interruptible Efi Runtime Services"
> +	depends on EFI && !EFI_VARS_PSTORE
> +	default n
> +	help
> +          This is an interruptible implementation of efi runtime
> +	  services wrappers. If you say Y, BIOS code could be
> +	  interrupted and preempted as a standard process. Enable this
> +	  only if you are sure that your BIOS will support
> +	  interruptible efi calls
> +	  In doubt, say "N".
> +endif
> +

These words should be a little more assertive. Almost everyone is
going to want to turn this feature off.

> +static efi_status_t
> +non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
> +			 __attribute__((unused)) efi_guid_t *vendor,
> +			 __attribute__((unused)) u32 attr,
> +			 __attribute__((unused)) unsigned long data_size,
> +			 __attribute__((unused)) void *data)
> +{
> +	pr_err("efi_interruptible: non bloking operation is not allowed\n");
> +	return EFI_UNSUPPORTED;
> +}

Typo, s/bloking/blocking/

> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	static struct efivars generic_efivars;
> +	static struct efivar_operations generic_ops;
> +
> +	generic_ops.get_variable = efi.get_variable;
> +	generic_ops.set_variable = efi.set_variable;
> +	generic_ops.get_next_variable = efi.get_next_variable;
> +	generic_ops.query_variable_store = efi_query_variable_store;
> +
> +	efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +

What's the purpose of this? The only reason you'd want to do this that
I can think of is because you'd want efi-pstore to run and attempt to
save some crash data for you. But you can't build with efi-pstore
enable and CONFIG_EFI_INTERRUPTIBLE.

I'd be tempted to just delete this notifier code.

> +static struct task_struct *efi_kworker_task;
> +static struct efivar_operations interruptible_ops;
> +static __init int efi_interruptible_init(void)
> +{
> +	int ret;
> +
> +	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> +					  "efi_kthread");
> +	if (IS_ERR(efi_kworker_task)) {
> +		pr_err("efi_interruptible: Failed to create kthread\n");
> +		ret = PTR_ERR(efi_kworker_task);
> +		efi_kworker_task = NULL;
> +		return ret;
> +	}
> +
> +	efi_kworker_task->mm = mm_alloc();
> +	efi_kworker_task->active_mm = efi_kworker_task->mm;
> +	efi_kworker_task->mm->pgd = efi_get_pgd();
> +	wake_up_process(efi_kworker_task);
> +
> +	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
> +
> +	interruptible_ops.get_variable = get_variable_interruptible;
> +	interruptible_ops.set_variable = set_variable_interruptible;
> +	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
> +	interruptible_ops.get_next_variable = get_next_variable_interruptible;
> +	interruptible_ops.query_variable_store = efi_query_variable_store;
> +	return efivars_register(&interruptible_efivars, &interruptible_ops,
> +				efivars_kobject());
> +}
> +

Is there some way we can match DMI/PCI identifiers for Broxton so that
we don't start seeing people accidentally running with preemptible EFI
services on non-BXT hardware?

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch adds an implementation of EFI runtime services wappers which
> allow interrupts and preemption during execution of BIOS code.
> 
> It's needed by Broxton platform which requires the OS to do the write
> of the non volatile variables. To do that, the OS will receive an
> interrupt coming from the firmware to notify that it must write the
> data.
> 
> BIOS code must be executed using a specific PGD. This is currently
> done by efi_call() which force value of CR3 before calling BIOS and
> restore previous value after.
> 
> We use a dedicated kthread which will execute all interruptible runtime
> services. This efi_kthread is special because it has it's own mm_struct
> whereas kthread should not have one. This mm_struct contains the EFI PGD
> so the scheduler will load it before running any BIOS code.
> 
> Obviously, an interruptible runtime service must never be called from an
> interrupt context. EFI pstore is the only use case here, that's why we
> require it to be disabled when activating interruptible runtime services.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/Kconfig                          |  14 +++
>  arch/x86/include/asm/efi.h                |   1 +
>  arch/x86/platform/efi/Makefile            |   1 +
>  arch/x86/platform/efi/efi_32.c            |   5 +
>  arch/x86/platform/efi/efi_64.c            |   5 +
>  arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
>  6 files changed, 217 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..3ddd5a9cab30 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,20 @@ config EFI_MIXED
>  
>  	   If unsure, say N.
>  
> +if X86_64
> +config EFI_INTERRUPTIBLE
> +	bool "Interruptible Efi Runtime Services"
> +	depends on EFI && !EFI_VARS_PSTORE
> +	default n
> +	help
> +          This is an interruptible implementation of efi runtime
> +	  services wrappers. If you say Y, BIOS code could be
> +	  interrupted and preempted as a standard process. Enable this
> +	  only if you are sure that your BIOS will support
> +	  interruptible efi calls
> +	  In doubt, say "N".
> +endif
> +

These words should be a little more assertive. Almost everyone is
going to want to turn this feature off.

> +static efi_status_t
> +non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
> +			 __attribute__((unused)) efi_guid_t *vendor,
> +			 __attribute__((unused)) u32 attr,
> +			 __attribute__((unused)) unsigned long data_size,
> +			 __attribute__((unused)) void *data)
> +{
> +	pr_err("efi_interruptible: non bloking operation is not allowed\n");
> +	return EFI_UNSUPPORTED;
> +}

Typo, s/bloking/blocking/

> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	static struct efivars generic_efivars;
> +	static struct efivar_operations generic_ops;
> +
> +	generic_ops.get_variable = efi.get_variable;
> +	generic_ops.set_variable = efi.set_variable;
> +	generic_ops.get_next_variable = efi.get_next_variable;
> +	generic_ops.query_variable_store = efi_query_variable_store;
> +
> +	efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +

What's the purpose of this? The only reason you'd want to do this that
I can think of is because you'd want efi-pstore to run and attempt to
save some crash data for you. But you can't build with efi-pstore
enable and CONFIG_EFI_INTERRUPTIBLE.

I'd be tempted to just delete this notifier code.

> +static struct task_struct *efi_kworker_task;
> +static struct efivar_operations interruptible_ops;
> +static __init int efi_interruptible_init(void)
> +{
> +	int ret;
> +
> +	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> +					  "efi_kthread");
> +	if (IS_ERR(efi_kworker_task)) {
> +		pr_err("efi_interruptible: Failed to create kthread\n");
> +		ret = PTR_ERR(efi_kworker_task);
> +		efi_kworker_task = NULL;
> +		return ret;
> +	}
> +
> +	efi_kworker_task->mm = mm_alloc();
> +	efi_kworker_task->active_mm = efi_kworker_task->mm;
> +	efi_kworker_task->mm->pgd = efi_get_pgd();
> +	wake_up_process(efi_kworker_task);
> +
> +	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
> +
> +	interruptible_ops.get_variable = get_variable_interruptible;
> +	interruptible_ops.set_variable = set_variable_interruptible;
> +	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
> +	interruptible_ops.get_next_variable = get_next_variable_interruptible;
> +	interruptible_ops.query_variable_store = efi_query_variable_store;
> +	return efivars_register(&interruptible_efivars, &interruptible_ops,
> +				efivars_kobject());
> +}
> +

Is there some way we can match DMI/PCI identifiers for Broxton so that
we don't start seeing people accidentally running with preemptible EFI
services on non-BXT hardware?

Re: [PATCH 1/2] efi: Don't use spinlocks for efi vars

[Sylvain Chouleur]

2016-01-06 13:24 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
>
> (Cc'ing Ard since he has recently been in this area)
>
> On Fri, 18 Dec, at 11:29:50AM, sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > All efivars operations are protected by a spinlock which prevents
> > interruptions and preemption. This is too restricted, we just need a
> > lock preventing concurency.
> > The idea is to use a semaphore of count 1 and to have two ways of
> > locking, depending on the context:
> >  - In interrupt context, we call down_trylock(), if it fails we return
> >  an error
> >  - In normal context, we call down_interruptible()
> >
> > We don't use a mutex here because the mutex_trylock() function must not
> > be called from interrupt context, whereas the down_trylock() can.
> >
> > This patch also remove the efivars lock to use a single lock for the
> > whole vars.c file. The goal of this lock is to protect concurrent calls
> > to efi variable services, registering and unregistering.
> > This allows to register new efivars operations without having
> > in-progress call.
> >
> > Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/firmware/efi/efi-pstore.c |  34 +++++++---
> >  drivers/firmware/efi/efivars.c    |  10 ++-
> >  drivers/firmware/efi/vars.c       | 130 +++++++++++++++++++++++++-------------
> >  fs/efivarfs/inode.c               |   5 +-
> >  fs/efivarfs/super.c               |   8 ++-
> >  include/linux/efi.h               |  12 +---
> >  6 files changed, 130 insertions(+), 69 deletions(-)
>
> This needs splitting into more than 1 patch.
>
> You need separate patches to,
>
>   - Split out __efivars->lock into a file local lock
>   - Convert the lock to a semaphore
>   - Print a message when efi_operations are registered
>
> Further comments below.
>
> > diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> > index 756eca8c4cf8..3998373d92ef 100644
> > --- a/drivers/firmware/efi/efivars.c
> > +++ b/drivers/firmware/efi/efivars.c
> > @@ -509,7 +509,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
> >               vendor = del_var->VendorGuid;
> >       }
> >
> > -     efivar_entry_iter_begin();
> > +     if (efivar_entry_iter_begin())
> > +             return -EINTR;
> >       entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true);
> >       if (!entry)
> >               err = -EINVAL;
> > @@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
> >               return ret;
> >
> >       kobject_uevent(&new_var->kobj, KOBJ_ADD);
> > -     efivar_entry_add(new_var, &efivar_sysfs_list);
> > +     if (efivar_entry_add(new_var, &efivar_sysfs_list))
> > +             return -EINTR;
> >
> >       return 0;
> >  }
>
> This looks like it's missing a call to efivar_unregister() in the
> -EINTR case.

I don't see why
It's returning an error code as other lines in the same function.
Can you develop your thoughts?

>
> > @@ -697,7 +699,9 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor,
> >
> >  static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
> >  {
> > -     efivar_entry_remove(entry);
> > +     int err = efivar_entry_remove(entry);
> > +     if (err)
> > +             return err;
> >       efivar_unregister(entry);
> >       return 0;
> >  }
>
> You now need to return early from efivars_sysfs_exit() if you hit the
> error path in efivar_sysfs_destroy().
>
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 70a0fb10517f..8a44351211e5 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -37,6 +37,13 @@
> >  /* Private pointer to registered efivars */
> >  static struct efivars *__efivars;
> >
> > +/*
> > + * ->lock protects two things:
> > + * 1) efivarfs_list and efivars_sysfs_list
> > + * 2) ->ops calls
> > + */
> > +DEFINE_SEMAPHORE(efivars_lock);
> > +
>
> Now it also protects registration of __efivars, so that needs to be
> documented too.
>
> >  static bool efivar_wq_enabled = true;
> >  DECLARE_WORK(efivar_work, NULL);
> >  EXPORT_SYMBOL_GPL(efivar_work);
> > @@ -376,7 +383,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> >               return -ENOMEM;
> >       }
> >
> > -     spin_lock_irq(&__efivars->lock);
> > +     if (down_interruptible(&efivars_lock)) {
> > +             err = -EINTR;
> > +             goto free;
> > +     }
> >
> >       /*
> >        * Per EFI spec, the maximum storage allocated for both
> > @@ -392,7 +402,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> >               switch (status) {
> >               case EFI_SUCCESS:
> >                       if (!atomic)
> > -                             spin_unlock_irq(&__efivars->lock);
> > +                             up(&efivars_lock);
> >
> >                       variable_name_size = var_name_strnsize(variable_name,
> >                                                              variable_name_size);
> > @@ -410,7 +420,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> >                               dup_variable_bug(variable_name, &vendor_guid,
> >                                                variable_name_size);
> >                               if (!atomic)
> > -                                     spin_lock_irq(&__efivars->lock);
> > +                                     if (down_interruptible(&efivars_lock)) {
> > +                                             err = -EINTR;
> > +                                             goto free;
> > +                                     }
> >
> >                               status = EFI_NOT_FOUND;
> >                               break;
>
> Add braces to the if(!atomic) clause please to help with readability.
>
> > @@ -421,7 +434,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> >                               status = EFI_NOT_FOUND;
> >
> >                       if (!atomic)
> > -                             spin_lock_irq(&__efivars->lock);
> > +                             if (down_interruptible(&efivars_lock)) {
> > +                                     err = -EINTR;
> > +                                     goto free;
> > +                             }
> >
> >                       break;
> >               case EFI_NOT_FOUND:
>
> Ditto.
>
> > @@ -533,12 +559,14 @@ int efivar_entry_delete(struct efivar_entry *entry)
> >       const struct efivar_operations *ops = __efivars->ops;
> >       efi_status_t status;
> >
> > -     spin_lock_irq(&__efivars->lock);
> > +     if (down_interruptible(&efivars_lock))
> > +             return -EINTR;
> > +
> >       status = ops->set_variable(entry->var.VariableName,
> >                                  &entry->var.VendorGuid,
> >                                  0, 0, NULL);
> >       if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
> > -             spin_unlock_irq(&__efivars->lock);
> > +             up(&efivars_lock);
> >               return efi_status_to_err(status);
> >       }
> >
>
> Please update the documentation for this function to note that we
> return -EINTR if we can't grab the semaphore.
>
> > @@ -576,10 +604,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> >       efi_char16_t *name = entry->var.VariableName;
> >       efi_guid_t vendor = entry->var.VendorGuid;
> >
> > -     spin_lock_irq(&__efivars->lock);
> > -
> > +     if (down_interruptible(&efivars_lock))
> > +             return -EINTR;
> >       if (head && efivar_entry_find(name, vendor, head, false)) {
> > -             spin_unlock_irq(&__efivars->lock);
> > +             up(&efivars_lock);
> >               return -EEXIST;
> >       }
> >
> > @@ -588,7 +616,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> >               status = ops->set_variable(name, &vendor,
> >                                          attributes, size, data);
> >
> > -     spin_unlock_irq(&__efivars->lock);
> > +     up(&efivars_lock);
> >
> >       return efi_status_to_err(status);
> >
>
> Function documentation for the return values needs updating here too.
>
> > @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
> >                    const struct efivar_operations *ops,
> >                    struct kobject *kobject)
> >  {
> > -     spin_lock_init(&efivars->lock);
> > +     if (down_trylock(&efivars_lock))
> > +             return -EBUSY;
> > +
>
> Is this correct? I would have assumed that you'd want to return -EINTR
> here, not -EBUSY since if an EFI variable operation is currently
> running we should spin waiting for the semaphore to be released.

No because this is a trylock, it will not wait for the semaphore to be release,
it will return as soon as it sees that the semaphore is locked.
We don't want to use down_interruptible() here because we want to be able to
(un)register efivars operation in an interrupt context.

>
> >       efivars->ops = ops;
> >       efivars->kobject = kobject;
> >
> >       __efivars = efivars;
> >
> > +     up(&efivars_lock);
> > +
> >       return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(efivars_register);
> > @@ -1076,6 +1112,9 @@ int efivars_unregister(struct efivars *efivars)
> >  {
> >       int rv;
> >
> > +     if (down_trylock(&efivars_lock))
> > +             return -EBUSY;
> > +
>
> Same logic applies in the unregister case.


Thanks Matt,

I'll provide updated patches soon
-- 
Sylvain

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
>> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
>>
>> This patch adds an implementation of EFI runtime services wappers which
>> allow interrupts and preemption during execution of BIOS code.
>>
>> It's needed by Broxton platform which requires the OS to do the write
>> of the non volatile variables. To do that, the OS will receive an
>> interrupt coming from the firmware to notify that it must write the
>> data.
>>
>> BIOS code must be executed using a specific PGD. This is currently
>> done by efi_call() which force value of CR3 before calling BIOS and
>> restore previous value after.
>>
>> We use a dedicated kthread which will execute all interruptible runtime
>> services. This efi_kthread is special because it has it's own mm_struct
>> whereas kthread should not have one. This mm_struct contains the EFI PGD
>> so the scheduler will load it before running any BIOS code.
>>
>> Obviously, an interruptible runtime service must never be called from an
>> interrupt context. EFI pstore is the only use case here, that's why we
>> require it to be disabled when activating interruptible runtime services.
>>
>> Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
>> ---
>>  arch/x86/Kconfig                          |  14 +++
>>  arch/x86/include/asm/efi.h                |   1 +
>>  arch/x86/platform/efi/Makefile            |   1 +
>>  arch/x86/platform/efi/efi_32.c            |   5 +
>>  arch/x86/platform/efi/efi_64.c            |   5 +
>>  arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
>>  6 files changed, 217 insertions(+)
>>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index db3622f22b61..3ddd5a9cab30 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -1720,6 +1720,20 @@ config EFI_MIXED
>>
>>          If unsure, say N.
>>
>> +if X86_64
>> +config EFI_INTERRUPTIBLE
>> +     bool "Interruptible Efi Runtime Services"
>> +     depends on EFI && !EFI_VARS_PSTORE
>> +     default n
>> +     help
>> +          This is an interruptible implementation of efi runtime
>> +       services wrappers. If you say Y, BIOS code could be
>> +       interrupted and preempted as a standard process. Enable this
>> +       only if you are sure that your BIOS will support
>> +       interruptible efi calls
>> +       In doubt, say "N".
>> +endif
>> +
>
> These words should be a little more assertive. Almost everyone is
> going to want to turn this feature off.
>
>> +static efi_status_t
>> +non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
>> +                      __attribute__((unused)) efi_guid_t *vendor,
>> +                      __attribute__((unused)) u32 attr,
>> +                      __attribute__((unused)) unsigned long data_size,
>> +                      __attribute__((unused)) void *data)
>> +{
>> +     pr_err("efi_interruptible: non bloking operation is not allowed\n");
>> +     return EFI_UNSUPPORTED;
>> +}
>
> Typo, s/bloking/blocking/
>
>> +static int efi_interruptible_panic_notifier_call(
>> +     struct notifier_block *notifier,
>> +     unsigned long what, void *data)
>> +{
>> +     static struct efivars generic_efivars;
>> +     static struct efivar_operations generic_ops;
>> +
>> +     generic_ops.get_variable = efi.get_variable;
>> +     generic_ops.set_variable = efi.set_variable;
>> +     generic_ops.get_next_variable = efi.get_next_variable;
>> +     generic_ops.query_variable_store = efi_query_variable_store;
>> +
>> +     efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
>> +
>> +     return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block panic_nb = {
>> +     .notifier_call = efi_interruptible_panic_notifier_call,
>> +     .priority = 100,
>> +};
>> +
>
> What's the purpose of this? The only reason you'd want to do this that
> I can think of is because you'd want efi-pstore to run and attempt to
> save some crash data for you. But you can't build with efi-pstore
> enable and CONFIG_EFI_INTERRUPTIBLE.
>
> I'd be tempted to just delete this notifier code.

This is indeed to be able to try write some crash data when we
are in a panic context.  In fact with this restoration of legacy
efi handlers on a panic we should be able to have pstore working
with CONFIG_EFI_INTERRUPTIBLE.

I say should because there is still issues with BIOS to have the
panic flow working.

>
>> +static struct task_struct *efi_kworker_task;
>> +static struct efivar_operations interruptible_ops;
>> +static __init int efi_interruptible_init(void)
>> +{
>> +     int ret;
>> +
>> +     efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
>> +                                       "efi_kthread");
>> +     if (IS_ERR(efi_kworker_task)) {
>> +             pr_err("efi_interruptible: Failed to create kthread\n");
>> +             ret = PTR_ERR(efi_kworker_task);
>> +             efi_kworker_task = NULL;
>> +             return ret;
>> +     }
>> +
>> +     efi_kworker_task->mm = mm_alloc();
>> +     efi_kworker_task->active_mm = efi_kworker_task->mm;
>> +     efi_kworker_task->mm->pgd = efi_get_pgd();
>> +     wake_up_process(efi_kworker_task);
>> +
>> +     atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
>> +
>> +     interruptible_ops.get_variable = get_variable_interruptible;
>> +     interruptible_ops.set_variable = set_variable_interruptible;
>> +     interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
>> +     interruptible_ops.get_next_variable = get_next_variable_interruptible;
>> +     interruptible_ops.query_variable_store = efi_query_variable_store;
>> +     return efivars_register(&interruptible_efivars, &interruptible_ops,
>> +                             efivars_kobject());
>> +}
>> +
>
> Is there some way we can match DMI/PCI identifiers for Broxton so that
> we don't start seeing people accidentally running with preemptible EFI
> services on non-BXT hardware?

I don't see how since this depends on a storage strategy more
than the SoC itself, it can be used on future platforms or we can
also get rid of it on BXT if an other storage is used for efi
variables.
Can't the KConfig protection be enough?

-- 
Sylvain

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-01-06 13:58 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> This patch adds an implementation of EFI runtime services wappers which
>> allow interrupts and preemption during execution of BIOS code.
>>
>> It's needed by Broxton platform which requires the OS to do the write
>> of the non volatile variables. To do that, the OS will receive an
>> interrupt coming from the firmware to notify that it must write the
>> data.
>>
>> BIOS code must be executed using a specific PGD. This is currently
>> done by efi_call() which force value of CR3 before calling BIOS and
>> restore previous value after.
>>
>> We use a dedicated kthread which will execute all interruptible runtime
>> services. This efi_kthread is special because it has it's own mm_struct
>> whereas kthread should not have one. This mm_struct contains the EFI PGD
>> so the scheduler will load it before running any BIOS code.
>>
>> Obviously, an interruptible runtime service must never be called from an
>> interrupt context. EFI pstore is the only use case here, that's why we
>> require it to be disabled when activating interruptible runtime services.
>>
>> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/x86/Kconfig                          |  14 +++
>>  arch/x86/include/asm/efi.h                |   1 +
>>  arch/x86/platform/efi/Makefile            |   1 +
>>  arch/x86/platform/efi/efi_32.c            |   5 +
>>  arch/x86/platform/efi/efi_64.c            |   5 +
>>  arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
>>  6 files changed, 217 insertions(+)
>>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index db3622f22b61..3ddd5a9cab30 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -1720,6 +1720,20 @@ config EFI_MIXED
>>
>>          If unsure, say N.
>>
>> +if X86_64
>> +config EFI_INTERRUPTIBLE
>> +     bool "Interruptible Efi Runtime Services"
>> +     depends on EFI && !EFI_VARS_PSTORE
>> +     default n
>> +     help
>> +          This is an interruptible implementation of efi runtime
>> +       services wrappers. If you say Y, BIOS code could be
>> +       interrupted and preempted as a standard process. Enable this
>> +       only if you are sure that your BIOS will support
>> +       interruptible efi calls
>> +       In doubt, say "N".
>> +endif
>> +
>
> These words should be a little more assertive. Almost everyone is
> going to want to turn this feature off.
>
>> +static efi_status_t
>> +non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
>> +                      __attribute__((unused)) efi_guid_t *vendor,
>> +                      __attribute__((unused)) u32 attr,
>> +                      __attribute__((unused)) unsigned long data_size,
>> +                      __attribute__((unused)) void *data)
>> +{
>> +     pr_err("efi_interruptible: non bloking operation is not allowed\n");
>> +     return EFI_UNSUPPORTED;
>> +}
>
> Typo, s/bloking/blocking/
>
>> +static int efi_interruptible_panic_notifier_call(
>> +     struct notifier_block *notifier,
>> +     unsigned long what, void *data)
>> +{
>> +     static struct efivars generic_efivars;
>> +     static struct efivar_operations generic_ops;
>> +
>> +     generic_ops.get_variable = efi.get_variable;
>> +     generic_ops.set_variable = efi.set_variable;
>> +     generic_ops.get_next_variable = efi.get_next_variable;
>> +     generic_ops.query_variable_store = efi_query_variable_store;
>> +
>> +     efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
>> +
>> +     return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block panic_nb = {
>> +     .notifier_call = efi_interruptible_panic_notifier_call,
>> +     .priority = 100,
>> +};
>> +
>
> What's the purpose of this? The only reason you'd want to do this that
> I can think of is because you'd want efi-pstore to run and attempt to
> save some crash data for you. But you can't build with efi-pstore
> enable and CONFIG_EFI_INTERRUPTIBLE.
>
> I'd be tempted to just delete this notifier code.

This is indeed to be able to try write some crash data when we
are in a panic context.  In fact with this restoration of legacy
efi handlers on a panic we should be able to have pstore working
with CONFIG_EFI_INTERRUPTIBLE.

I say should because there is still issues with BIOS to have the
panic flow working.

>
>> +static struct task_struct *efi_kworker_task;
>> +static struct efivar_operations interruptible_ops;
>> +static __init int efi_interruptible_init(void)
>> +{
>> +     int ret;
>> +
>> +     efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
>> +                                       "efi_kthread");
>> +     if (IS_ERR(efi_kworker_task)) {
>> +             pr_err("efi_interruptible: Failed to create kthread\n");
>> +             ret = PTR_ERR(efi_kworker_task);
>> +             efi_kworker_task = NULL;
>> +             return ret;
>> +     }
>> +
>> +     efi_kworker_task->mm = mm_alloc();
>> +     efi_kworker_task->active_mm = efi_kworker_task->mm;
>> +     efi_kworker_task->mm->pgd = efi_get_pgd();
>> +     wake_up_process(efi_kworker_task);
>> +
>> +     atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
>> +
>> +     interruptible_ops.get_variable = get_variable_interruptible;
>> +     interruptible_ops.set_variable = set_variable_interruptible;
>> +     interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
>> +     interruptible_ops.get_next_variable = get_next_variable_interruptible;
>> +     interruptible_ops.query_variable_store = efi_query_variable_store;
>> +     return efivars_register(&interruptible_efivars, &interruptible_ops,
>> +                             efivars_kobject());
>> +}
>> +
>
> Is there some way we can match DMI/PCI identifiers for Broxton so that
> we don't start seeing people accidentally running with preemptible EFI
> services on non-BXT hardware?

I don't see how since this depends on a storage strategy more
than the SoC itself, it can be used on future platforms or we can
also get rid of it on BXT if an other storage is used for efi
variables.
Can't the KConfig protection be enough?

-- 
Sylvain

[PATCH v2 0/3] efi interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

Changes in v2:
 - Split patch 1 in patches 1 and 2
 - Document (un)registration of __efivars protection
 - Return early from efivars_sysfs_exit() in case of failure
 - Improve readability
 - Update functions documentation
 - Fix typo
 - Fix checkpatch warnings
 - Warning in KConfig description

Sylvain Chouleur (3):
 efi: implement interruptible runtime services
 efi: don't use spinlocks for efi vars
 efi: use a file local lock for efivars

 arch/x86/Kconfig                          |   17 +++++
 arch/x86/include/asm/efi.h                |    1 
 arch/x86/platform/efi/Makefile            |    1 
 arch/x86/platform/efi/efi_32.c            |    5 +
 arch/x86/platform/efi/efi_64.c            |    5 +
 arch/x86/platform/efi/efi_interruptible.c |  191 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c         |   36 +++++++---
 drivers/firmware/efi/efivars.c            |   20 ++++-
 drivers/firmware/efi/vars.c               |  230 ++++++++++++++++++++++++++++++++++++++++----------------------------
 fs/efivarfs/inode.c                       |    5 +
 fs/efivarfs/super.c                       |    9 ++
 include/linux/efi.h                       |   12 ---
 12 files changed, 415 insertions(+), 117 deletions(-)

[PATCH v2 0/3] efi interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Changes in v2:
 - Split patch 1 in patches 1 and 2
 - Document (un)registration of __efivars protection
 - Return early from efivars_sysfs_exit() in case of failure
 - Improve readability
 - Update functions documentation
 - Fix typo
 - Fix checkpatch warnings
 - Warning in KConfig description

Sylvain Chouleur (3):
 efi: implement interruptible runtime services
 efi: don't use spinlocks for efi vars
 efi: use a file local lock for efivars

 arch/x86/Kconfig                          |   17 +++++
 arch/x86/include/asm/efi.h                |    1 
 arch/x86/platform/efi/Makefile            |    1 
 arch/x86/platform/efi/efi_32.c            |    5 +
 arch/x86/platform/efi/efi_64.c            |    5 +
 arch/x86/platform/efi/efi_interruptible.c |  191 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c         |   36 +++++++---
 drivers/firmware/efi/efivars.c            |   20 ++++-
 drivers/firmware/efi/vars.c               |  230 ++++++++++++++++++++++++++++++++++++++++----------------------------
 fs/efivarfs/inode.c                       |    5 +
 fs/efivarfs/super.c                       |    9 ++
 include/linux/efi.h                       |   12 ---
 12 files changed, 415 insertions(+), 117 deletions(-)

[PATCH v2 1/3] efi: use a file local lock for efivars

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch remove the efivars lock to use a single lock for the whole
vars.c file.  The goal of this lock is to protect concurrent calls to
efi variable services, registering and unregistering.  This allows to
register new efivars operations without having in-progress call.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 85 +++++++++++++++++++++++++--------------------
 include/linux/efi.h         |  6 ----
 2 files changed, 48 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index d2a49626a335..1851f492368f 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -37,6 +37,14 @@
 /* Private pointer to registered efivars */
 static struct efivars *__efivars;
 
+/*
+ * efivars_lock protects three things:
+ * 1) efivarfs_list and efivars_sysfs_list
+ * 2) ->ops calls
+ * 3) (un)registration of __efivars
+ */
+static DEFINE_SPINLOCK(efivars_lock);
+
 static bool efivar_wq_enabled = true;
 DECLARE_WORK(efivar_work, NULL);
 EXPORT_SYMBOL_GPL(efivar_work);
@@ -387,7 +395,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		return -ENOMEM;
 	}
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
@@ -403,7 +411,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		switch (status) {
 		case EFI_SUCCESS:
 			if (!atomic)
-				spin_unlock_irq(&__efivars->lock);
+				spin_unlock_irq(&efivars_lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
@@ -421,7 +429,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
 				if (!atomic)
-					spin_lock_irq(&__efivars->lock);
+					spin_lock_irq(&efivars_lock);
 
 				status = EFI_NOT_FOUND;
 				break;
@@ -432,7 +440,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				status = EFI_NOT_FOUND;
 
 			if (!atomic)
-				spin_lock_irq(&__efivars->lock);
+				spin_lock_irq(&efivars_lock);
 
 			break;
 		case EFI_NOT_FOUND:
@@ -446,7 +454,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 
 	} while (status != EFI_NOT_FOUND);
 
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	kfree(variable_name);
 
@@ -461,9 +469,9 @@ EXPORT_SYMBOL_GPL(efivar_init);
  */
 void efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	list_add(&entry->list, head);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
@@ -473,9 +481,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_add);
  */
 void efivar_entry_remove(struct efivar_entry *entry)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_remove);
 
@@ -492,10 +500,10 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 
 /**
@@ -518,7 +526,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -544,12 +552,12 @@ int efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   0, 0, NULL);
 	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 		return efi_status_to_err(status);
 	}
 
@@ -587,10 +595,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 	efi_char16_t *name = entry->var.VariableName;
 	efi_guid_t vendor = entry->var.VendorGuid;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	if (head && efivar_entry_find(name, vendor, head, false)) {
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 		return -EEXIST;
 	}
 
@@ -599,7 +607,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		status = ops->set_variable(name, &vendor,
 					   attributes, size, data);
 
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	return efi_status_to_err(status);
 
@@ -613,7 +621,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_set);
  * from crash/panic handlers.
  *
  * Crucially, this function will not block if it cannot acquire
- * __efivars->lock. Instead, it returns -EBUSY.
+ * efivars_lock. Instead, it returns -EBUSY.
  */
 static int
 efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
@@ -623,20 +631,20 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&__efivars->lock, flags))
+	if (!spin_trylock_irqsave(&efivars_lock, flags))
 		return -EBUSY;
 
 	status = check_var_size_nonblocking(attributes,
 					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		spin_unlock_irqrestore(&efivars_lock, flags);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	spin_unlock_irqrestore(&efivars_lock, flags);
 	return efi_status_to_err(status);
 }
 
@@ -682,21 +690,21 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 						    size, data);
 
 	if (!block) {
-		if (!spin_trylock_irqsave(&__efivars->lock, flags))
+		if (!spin_trylock_irqsave(&efivars_lock, flags))
 			return -EBUSY;
 	} else {
-		spin_lock_irqsave(&__efivars->lock, flags);
+		spin_lock_irqsave(&efivars_lock, flags);
 	}
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		spin_unlock_irqrestore(&efivars_lock, flags);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable(name, &vendor, attributes, size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	spin_unlock_irqrestore(&efivars_lock, flags);
 
 	return efi_status_to_err(status);
 }
@@ -726,7 +734,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -769,10 +777,10 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
 
 	*size = 0;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid, NULL, size, NULL);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -798,7 +806,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -821,11 +829,11 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   attributes, size, data);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -872,7 +880,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	 * set_variable call, and removal of the variable from the efivars
 	 * list (in the case of an authenticated delete).
 	 */
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	/*
 	 * Ensure that the available space hasn't shrunk below the safe level
@@ -912,7 +920,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	if (status == EFI_NOT_FOUND)
 		efivar_entry_list_del_unlock(entry);
 	else
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -920,7 +928,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return 0;
 
 out:
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 	return err;
 
 }
@@ -935,7 +943,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size);
  */
 void efivar_entry_iter_begin(void)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
 
@@ -946,7 +954,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
  */
 void efivar_entry_iter_end(void)
 {
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
 
@@ -1067,11 +1075,12 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
-	spin_lock_init(&efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	efivars->ops = ops;
 	efivars->kobject = kobject;
 
 	__efivars = efivars;
+	spin_unlock_irq(&efivars_lock);
 
 	return 0;
 }
@@ -1088,6 +1097,7 @@ int efivars_unregister(struct efivars *efivars)
 {
 	int rv;
 
+	spin_lock_irq(&efivars_lock);
 	if (!__efivars) {
 		printk(KERN_ERR "efivars not registered\n");
 		rv = -EINVAL;
@@ -1103,6 +1113,7 @@ int efivars_unregister(struct efivars *efivars)
 
 	rv = 0;
 out:
+	spin_unlock_irq(&efivars_lock);
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_unregister);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 09f1559e7525..fba46fb6c4a8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1099,12 +1099,6 @@ struct efivar_operations {
 };
 
 struct efivars {
-	/*
-	 * ->lock protects two things:
-	 * 1) efivarfs_list and efivars_sysfs_list
-	 * 2) ->ops calls
-	 */
-	spinlock_t lock;
 	struct kset *kset;
 	struct kobject *kobject;
 	const struct efivar_operations *ops;
-- 
2.6.4

[PATCH v2 2/3] efi: don't use spinlocks for efi vars

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

All efivars operations are protected by a spinlock which prevents
interruptions and preemption. This is too restricted, we just need a
lock preventing concurency.
The idea is to use a semaphore of count 1 and to have two ways of
locking, depending on the context:
 - In interrupt context, we call down_trylock(), if it fails we return
 an error
 - In normal context, we call down_interruptible()

We don't use a mutex here because the mutex_trylock() function must not
be called from interrupt context, whereas the down_trylock() can.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/efi-pstore.c |  36 +++++++---
 drivers/firmware/efi/efivars.c    |  20 ++++--
 drivers/firmware/efi/vars.c       | 145 +++++++++++++++++++++++---------------
 fs/efivarfs/inode.c               |   5 +-
 fs/efivarfs/super.c               |   9 ++-
 include/linux/efi.h               |   6 +-
 6 files changed, 147 insertions(+), 74 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index eac76a79a880..7bfaffb0e672 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -121,16 +121,19 @@ static void efi_pstore_scan_sysfs_enter(struct efivar_entry *pos,
  * @entry: deleting entry
  * @turn_off_scanning: Check if a scanning flag should be turned off
  */
-static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
+static inline int __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
 						bool turn_off_scanning)
 {
 	if (entry->deleting) {
 		list_del(&entry->list);
 		efivar_entry_iter_end();
 		efivar_unregister(entry);
-		efivar_entry_iter_begin();
+		if (efivar_entry_iter_begin())
+			return -EINTR;
 	} else if (turn_off_scanning)
 		entry->scanning = false;
+
+	return 0;
 }
 
 /**
@@ -140,13 +143,18 @@ static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
  * @head: list head
  * @stop: a flag checking if scanning will stop
  */
-static void efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
+static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
 				       struct efivar_entry *next,
 				       struct list_head *head, bool stop)
 {
-	__efi_pstore_scan_sysfs_exit(pos, true);
+	int ret = __efi_pstore_scan_sysfs_exit(pos, true);
+
+	if (ret)
+		return ret;
+
 	if (stop)
-		__efi_pstore_scan_sysfs_exit(next, &next->list != head);
+		ret = __efi_pstore_scan_sysfs_exit(next, &next->list != head);
+	return ret;
 }
 
 /**
@@ -168,13 +176,17 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 	struct efivar_entry *entry, *n;
 	struct list_head *head = &efivar_sysfs_list;
 	int size = 0;
+	int ret;
 
 	if (!*pos) {
 		list_for_each_entry_safe(entry, n, head, list) {
 			efi_pstore_scan_sysfs_enter(entry, n, head);
 
 			size = efi_pstore_read_func(entry, data);
-			efi_pstore_scan_sysfs_exit(entry, n, head, size < 0);
+			ret = efi_pstore_scan_sysfs_exit(entry, n, head,
+							 size < 0);
+			if (ret)
+				return ret;
 			if (size)
 				break;
 		}
@@ -186,7 +198,9 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 		efi_pstore_scan_sysfs_enter((*pos), n, head);
 
 		size = efi_pstore_read_func((*pos), data);
-		efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		if (ret)
+			return ret;
 		if (size)
 			break;
 	}
@@ -226,7 +240,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 	if (!*data.buf)
 		return -ENOMEM;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin()) {
+		kfree(*data.buf);
+		return -EINTR;
+	}
 	size = efi_pstore_sysfs_entry_iter(&data,
 					   (struct efivar_entry **)&psi->data);
 	efivar_entry_iter_end();
@@ -341,7 +358,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 	edata.time = time;
 	edata.name = efi_name;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, &edata, &entry);
 
 	if (found && !entry->scanning) {
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8c4cf8..705feb129c97 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -509,7 +509,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 		vendor = del_var->VendorGuid;
 	}
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true);
 	if (!entry)
 		err = -EINVAL;
@@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 		return ret;
 
 	kobject_uevent(&new_var->kobj, KOBJ_ADD);
-	efivar_entry_add(new_var, &efivar_sysfs_list);
+	if (efivar_entry_add(new_var, &efivar_sysfs_list))
+		return -EINTR;
 
 	return 0;
 }
@@ -697,7 +699,10 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor,
 
 static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+
+	if (err)
+		return err;
 	efivar_unregister(entry);
 	return 0;
 }
@@ -705,7 +710,14 @@ static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
 static void efivars_sysfs_exit(void)
 {
 	/* Remove all entries and destroy */
-	__efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, NULL, NULL);
+	int err;
+
+	err = __efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list,
+				  NULL, NULL);
+	if (err) {
+		pr_err("efivars: Failed to destroy sysfs entries\n");
+		return;
+	}
 
 	if (efivars_new_var)
 		sysfs_remove_bin_file(&efivars_kset->kobj, efivars_new_var);
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 1851f492368f..3af309a1416d 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -43,7 +43,7 @@ static struct efivars *__efivars;
  * 2) ->ops calls
  * 3) (un)registration of __efivars
  */
-static DEFINE_SPINLOCK(efivars_lock);
+static DEFINE_SEMAPHORE(efivars_lock);
 
 static bool efivar_wq_enabled = true;
 DECLARE_WORK(efivar_work, NULL);
@@ -395,7 +395,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		return -ENOMEM;
 	}
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock)) {
+		err = -EINTR;
+		goto free;
+	}
 
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
@@ -411,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		switch (status) {
 		case EFI_SUCCESS:
 			if (!atomic)
-				spin_unlock_irq(&efivars_lock);
+				up(&efivars_lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
@@ -428,8 +431,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			    variable_is_present(variable_name, &vendor_guid, head)) {
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
-				if (!atomic)
-					spin_lock_irq(&efivars_lock);
+				if (!atomic) {
+					if (down_interruptible(&efivars_lock)) {
+						err = -EINTR;
+						goto free;
+					}
+				}
 
 				status = EFI_NOT_FOUND;
 				break;
@@ -439,8 +446,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			if (err)
 				status = EFI_NOT_FOUND;
 
-			if (!atomic)
-				spin_lock_irq(&efivars_lock);
+			if (!atomic) {
+				if (down_interruptible(&efivars_lock)) {
+					err = -EINTR;
+					goto free;
+				}
+			}
 
 			break;
 		case EFI_NOT_FOUND:
@@ -454,8 +465,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 
 	} while (status != EFI_NOT_FOUND);
 
-	spin_unlock_irq(&efivars_lock);
-
+	up(&efivars_lock);
+free:
 	kfree(variable_name);
 
 	return err;
@@ -466,24 +477,34 @@ EXPORT_SYMBOL_GPL(efivar_init);
  * efivar_entry_add - add entry to variable list
  * @entry: entry to add to list
  * @head: list head
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_add(&entry->list, head);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
 /**
  * efivar_entry_remove - remove entry from variable list
  * @entry: entry to remove from list
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_remove(struct efivar_entry *entry)
+int efivar_entry_remove(struct efivar_entry *entry)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_del(&entry->list);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_remove);
 
@@ -500,10 +521,10 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_del(&entry->list);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 }
 
 /**
@@ -526,7 +547,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -544,20 +565,22 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete);
  * variable list. It is the caller's responsibility to free @entry
  * once we return.
  *
- * Returns 0 on success, or a converted EFI status code if
- * set_variable() fails.
+ * Returns 0 on success, -EINTR if we can't grab the semaphore,
+ * converted EFI status code if set_variable() fails.
  */
 int efivar_entry_delete(struct efivar_entry *entry)
 {
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
+
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   0, 0, NULL);
 	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 		return efi_status_to_err(status);
 	}
 
@@ -583,9 +606,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
  * If @head is not NULL a lookup is performed to determine whether
  * the entry is already on the list.
  *
- * Returns 0 on success, -EEXIST if a lookup is performed and the entry
- * already exists on the list, or a converted EFI status code if
- * set_variable() fails.
+ * Returns 0 on success, -EINTR if we can't grab the semaphore,
+ * -EEXIST if a lookup is performed and the entry already exists on
+ * the list, or a converted EFI status code if set_variable() fails.
  */
 int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		     unsigned long size, void *data, struct list_head *head)
@@ -595,10 +618,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 	efi_char16_t *name = entry->var.VariableName;
 	efi_guid_t vendor = entry->var.VendorGuid;
 
-	spin_lock_irq(&efivars_lock);
-
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	if (head && efivar_entry_find(name, vendor, head, false)) {
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 		return -EEXIST;
 	}
 
@@ -607,7 +630,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		status = ops->set_variable(name, &vendor,
 					   attributes, size, data);
 
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 
@@ -628,23 +651,22 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 			     u32 attributes, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efivars_lock, flags))
+	if (down_trylock(&efivars_lock))
 		return -EBUSY;
 
 	status = check_var_size_nonblocking(attributes,
 					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&efivars_lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
-	spin_unlock_irqrestore(&efivars_lock, flags);
+	up(&efivars_lock);
 	return efi_status_to_err(status);
 }
 
@@ -669,7 +691,6 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
 	if (!ops->query_variable_store)
@@ -690,21 +711,22 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 						    size, data);
 
 	if (!block) {
-		if (!spin_trylock_irqsave(&efivars_lock, flags))
+		if (down_trylock(&efivars_lock))
 			return -EBUSY;
 	} else {
-		spin_lock_irqsave(&efivars_lock, flags);
+		if (down_interruptible(&efivars_lock))
+			return -EINTR;
 	}
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&efivars_lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable(name, &vendor, attributes, size, data);
 
-	spin_unlock_irqrestore(&efivars_lock, flags);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -734,7 +756,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -777,10 +799,11 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
 
 	*size = 0;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid, NULL, size, NULL);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -806,7 +829,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -829,11 +852,12 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   attributes, size, data);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -880,7 +904,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	 * set_variable call, and removal of the variable from the efivars
 	 * list (in the case of an authenticated delete).
 	 */
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 
 	/*
 	 * Ensure that the available space hasn't shrunk below the safe level
@@ -920,7 +945,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	if (status == EFI_NOT_FOUND)
 		efivar_entry_list_del_unlock(entry);
 	else
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -928,7 +953,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return 0;
 
 out:
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 	return err;
 
 }
@@ -941,9 +966,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size);
  * efivar_entry_iter_end() is called. This function is usually used in
  * conjunction with __efivar_entry_iter() or efivar_entry_iter().
  */
-void efivar_entry_iter_begin(void)
+int efivar_entry_iter_begin(void)
 {
-	spin_lock_irq(&efivars_lock);
+	return down_interruptible(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
 
@@ -954,7 +979,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
  */
 void efivar_entry_iter_end(void)
 {
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
 
@@ -1030,7 +1055,9 @@ int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
 {
 	int err = 0;
 
-	efivar_entry_iter_begin();
+	err = efivar_entry_iter_begin();
+	if (err)
+		return err;
 	err = __efivar_entry_iter(func, head, data, NULL);
 	efivar_entry_iter_end();
 
@@ -1075,12 +1102,17 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_trylock(&efivars_lock))
+		return -EBUSY;
+
 	efivars->ops = ops;
 	efivars->kobject = kobject;
 
 	__efivars = efivars;
-	spin_unlock_irq(&efivars_lock);
+
+	pr_info("Registered efivars operations\n");
+
+	up(&efivars_lock);
 
 	return 0;
 }
@@ -1097,7 +1129,9 @@ int efivars_unregister(struct efivars *efivars)
 {
 	int rv;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_trylock(&efivars_lock))
+		return -EBUSY;
+
 	if (!__efivars) {
 		printk(KERN_ERR "efivars not registered\n");
 		rv = -EINVAL;
@@ -1109,11 +1143,12 @@ int efivars_unregister(struct efivars *efivars)
 		goto out;
 	}
 
+	pr_info("Unregistered efivars operations\n");
 	__efivars = NULL;
 
 	rv = 0;
 out:
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_unregister);
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9da9ee6..5cea84ef7a44 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -132,7 +132,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 
 	inode->i_private = var;
 
-	efivar_entry_add(var, &efivarfs_list);
+	err = efivar_entry_add(var, &efivarfs_list);
+	if (err)
+		goto out;
+
 	d_instantiate(dentry, inode);
 	dget(dentry);
 out:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 86a2121828c3..6825f1e9718b 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -158,7 +158,9 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	kfree(name);
 
 	efivar_entry_size(entry, &size);
-	efivar_entry_add(entry, &efivarfs_list);
+	err = efivar_entry_add(entry, &efivarfs_list);
+	if (err)
+		goto fail_inode;
 
 	mutex_lock(&inode->i_mutex);
 	inode->i_private = entry;
@@ -179,7 +181,10 @@ fail:
 
 static int efivarfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+
+	if (err)
+		return err;
 	kfree(entry);
 	return 0;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index fba46fb6c4a8..6b38829795a9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1166,8 +1166,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool atomic, bool duplicates,
 		struct list_head *head);
 
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
-void efivar_entry_remove(struct efivar_entry *entry);
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
+int efivar_entry_remove(struct efivar_entry *entry);
 
 int __efivar_entry_delete(struct efivar_entry *entry);
 int efivar_entry_delete(struct efivar_entry *entry);
@@ -1184,7 +1184,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data);
 
-void efivar_entry_iter_begin(void);
+int efivar_entry_iter_begin(void);
 void efivar_entry_iter_end(void);
 
 int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
-- 
2.6.4

[PATCH v2 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

This patch adds an implementation of EFI runtime services wappers which
allow interrupts and preemption during execution of BIOS code.

It's needed by Broxton platform which requires the OS to do the write
of the non volatile variables. To do that, the OS will receive an
interrupt coming from the firmware to notify that it must write the
data.

BIOS code must be executed using a specific PGD. This is currently
done by efi_call() which force value of CR3 before calling BIOS and
restore previous value after.

We use a dedicated kthread which will execute all interruptible runtime
services. This efi_kthread is special because it has it's own mm_struct
whereas kthread should not have one. This mm_struct contains the EFI PGD
so the scheduler will load it before running any BIOS code.

Obviously, an interruptible runtime service must never be called from an
interrupt context. EFI pstore is the only use case here, that's why we
require it to be disabled when activating interruptible runtime services.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
---
 arch/x86/Kconfig                          |  17 +++
 arch/x86/include/asm/efi.h                |   1 +
 arch/x86/platform/efi/Makefile            |   1 +
 arch/x86/platform/efi/efi_32.c            |   5 +
 arch/x86/platform/efi/efi_64.c            |   5 +
 arch/x86/platform/efi/efi_interruptible.c | 191 ++++++++++++++++++++++++++++++
 6 files changed, 220 insertions(+)
 create mode 100644 arch/x86/platform/efi/efi_interruptible.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..37301516f4e6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1720,6 +1720,23 @@ config EFI_MIXED
 
 	   If unsure, say N.
 
+if X86_64
+config EFI_INTERRUPTIBLE
+	bool "Interruptible Efi Runtime Services"
+	depends on EFI && !EFI_VARS_PSTORE
+	default n
+	---help---
+	  Only use this if you really know what you are doing, you
+	  possibly risk to break your BIOS.
+
+	  This is an interruptible implementation of efi runtime
+	  services wrappers. If you say Y, BIOS code could be
+	  interrupted and preempted as a standard process. Enable this
+	  only if you are sure that your BIOS will support
+	  interruptible efi calls
+	  In doubt, say "N".
+endif
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0010c78c4998..5e9620b3688b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -111,6 +111,7 @@ extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
+extern pgd_t *efi_get_pgd(void);
 extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 2846aaab5103..dcc894d1e603 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_INTERRUPTIBLE)		+= efi_interruptible.o
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index ed5b67338294..02a3c616c59e 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -56,6 +56,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
+pgd_t *efi_get_pgd(void)
+{
+	return initial_page_table;
+}
+
 pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index d347e854a5e4..67cffb69d405 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -143,6 +143,11 @@ void efi_sync_low_kernel_mappings(void)
 		sizeof(pgd_t) * num_pgds);
 }
 
+pgd_t *efi_get_pgd(void)
+{
+	return __va(efi_scratch.efi_pgt);
+}
+
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
 	unsigned long text;
diff --git a/arch/x86/platform/efi/efi_interruptible.c b/arch/x86/platform/efi/efi_interruptible.c
new file mode 100644
index 000000000000..d1f14fcb12a4
--- /dev/null
+++ b/arch/x86/platform/efi/efi_interruptible.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * Author: Sylvain Chouleur <sylvain.chouleur@intel.com>
+ *
+ * Distributed under the terms of the GNU GPL, version 2
+ */
+
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+static DEFINE_KTHREAD_WORKER(efi_kworker);
+
+#define efi_call_interruptible(f, ...)					\
+({									\
+	efi_status_t __s;						\
+	efi_sync_low_kernel_mappings();					\
+	__kernel_fpu_begin();						\
+	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
+	__kernel_fpu_end();						\
+	__s;								\
+})
+
+struct efi_work {
+	struct kthread_work work;
+	efi_status_t status;
+	unsigned long *name_size;
+	efi_char16_t *name;
+	efi_guid_t *vendor;
+	u32 *attr;
+	unsigned long *data_size;
+	void *data;
+};
+
+static void do_get_next_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_next_variable, ew->name_size,
+					    ew->name, ew->vendor);
+}
+
+static void do_set_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(set_variable, ew->name, ew->vendor,
+					    *ew->attr, *ew->data_size,
+					    ew->data);
+}
+
+static void do_get_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_variable, ew->name, ew->vendor,
+					    ew->attr, ew->data_size, ew->data);
+}
+
+static efi_status_t execute_service(kthread_work_func_t service,
+				    unsigned long *name_size,
+				    efi_char16_t *name, efi_guid_t *vendor,
+				    u32 *attr, unsigned long *data_size,
+				    void *data)
+{
+	struct efi_work work = {
+		.name_size = name_size,
+		.name = name,
+		.vendor = vendor,
+		.attr = attr,
+		.data_size = data_size,
+		.data = data,
+	};
+
+	init_kthread_work(&work.work, service);
+	queue_kthread_work(&efi_kworker, &work.work);
+
+	flush_kthread_work(&work.work);
+	return work.status;
+}
+
+static efi_status_t get_next_variable_interruptible(unsigned long *name_size,
+					       efi_char16_t *name,
+					       efi_guid_t *vendor)
+{
+	return execute_service(do_get_next_variable_interruptible,
+			       name_size, name, vendor, NULL, NULL, NULL);
+}
+
+static efi_status_t set_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 attr,
+					  unsigned long data_size,
+					  void *data)
+{
+	return execute_service(do_set_variable_interruptible,
+			       NULL, name, vendor, &attr, &data_size, data);
+}
+
+static efi_status_t
+non_blocking_not_allowed(__attribute__((unused)) efi_char16_t *name,
+			 __attribute__((unused)) efi_guid_t *vendor,
+			 __attribute__((unused)) u32 attr,
+			 __attribute__((unused)) unsigned long data_size,
+			 __attribute__((unused)) void *data)
+{
+	pr_err("efi_interruptible: non blocking operation is not allowed\n");
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t get_variable_interruptible(efi_char16_t *name,
+					  efi_guid_t *vendor,
+					  u32 *attr,
+					  unsigned long *data_size,
+					  void *data)
+{
+	return execute_service(do_get_variable_interruptible,
+			       NULL, name, vendor, attr, data_size, data);
+}
+
+static struct efivars interruptible_efivars;
+
+static int efi_interruptible_panic_notifier_call(
+	struct notifier_block *notifier,
+	unsigned long what, void *data)
+{
+	static struct efivars generic_efivars;
+	static struct efivar_operations generic_ops;
+
+	generic_ops.get_variable = efi.get_variable;
+	generic_ops.set_variable = efi.set_variable;
+	generic_ops.get_next_variable = efi.get_next_variable;
+	generic_ops.query_variable_store = efi_query_variable_store;
+
+	efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_nb = {
+	.notifier_call = efi_interruptible_panic_notifier_call,
+	.priority = 100,
+};
+
+static struct task_struct *efi_kworker_task;
+static struct efivar_operations interruptible_ops;
+static __init int efi_interruptible_init(void)
+{
+	int ret;
+
+	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
+					  "efi_kthread");
+	if (IS_ERR(efi_kworker_task)) {
+		pr_err("efi_interruptible: Failed to create kthread\n");
+		ret = PTR_ERR(efi_kworker_task);
+		efi_kworker_task = NULL;
+		return ret;
+	}
+
+	efi_kworker_task->mm = mm_alloc();
+	efi_kworker_task->active_mm = efi_kworker_task->mm;
+	efi_kworker_task->mm->pgd = efi_get_pgd();
+	wake_up_process(efi_kworker_task);
+
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
+
+	interruptible_ops.get_variable = get_variable_interruptible;
+	interruptible_ops.set_variable = set_variable_interruptible;
+	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
+	interruptible_ops.get_next_variable = get_next_variable_interruptible;
+	interruptible_ops.query_variable_store = efi_query_variable_store;
+	return efivars_register(&interruptible_efivars, &interruptible_ops,
+				efivars_kobject());
+}
+
+static void __exit efi_interruptible_exit(void)
+{
+	efivars_unregister(&interruptible_efivars);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &panic_nb);
+	if (efi_kworker_task) {
+		kthread_stop(efi_kworker_task);
+		mmdrop(efi_kworker_task->mm);
+	}
+}
+
+module_init(efi_interruptible_init);
+module_exit(efi_interruptible_exit);
+
+MODULE_AUTHOR("Sylvain Chouleur <sylvain.chouleur@intel.com>");
+MODULE_LICENSE("GPL");
-- 
2.6.4

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Wed, 06 Jan, at 04:57:41PM, Sylvain Chouleur wrote:
> 2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> > On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
> >> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
> >>
> >> +static int efi_interruptible_panic_notifier_call(
> >> +     struct notifier_block *notifier,
> >> +     unsigned long what, void *data)
> >> +{
> >> +     static struct efivars generic_efivars;
> >> +     static struct efivar_operations generic_ops;
> >> +
> >> +     generic_ops.get_variable = efi.get_variable;
> >> +     generic_ops.set_variable = efi.set_variable;
> >> +     generic_ops.get_next_variable = efi.get_next_variable;
> >> +     generic_ops.query_variable_store = efi_query_variable_store;
> >> +
> >> +     efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
> >> +
> >> +     return NOTIFY_DONE;
> >> +}
> >> +
> >> +static struct notifier_block panic_nb = {
> >> +     .notifier_call = efi_interruptible_panic_notifier_call,
> >> +     .priority = 100,
> >> +};
> >> +
> >
> > What's the purpose of this? The only reason you'd want to do this that
> > I can think of is because you'd want efi-pstore to run and attempt to
> > save some crash data for you. But you can't build with efi-pstore
> > enable and CONFIG_EFI_INTERRUPTIBLE.
> >
> > I'd be tempted to just delete this notifier code.
> 
> This is indeed to be able to try write some crash data when we
> are in a panic context.  In fact with this restoration of legacy
> efi handlers on a panic we should be able to have pstore working
> with CONFIG_EFI_INTERRUPTIBLE.
 
This makes the efivar registration more complicated because now it can
fail if someone else is holding efivars_lock. Which is a strange
semantic for a registration function.

> I say should because there is still issues with BIOS to have the
> panic flow working.

How would it work though? You'd need the kernel thread controlling
access to the flash to be run in order for any data to appear in the
EFI variable store. Except you're in the middle of a panic and all
bets are off regarding which tasks are going to be run by the
scheduler.

Until those issues are resolved, please delete the notifier code. But
honestly, I doubt this will ever be useful in practice. Setting up
panic handlers *once* you've crashed is far too late.

> >> +static struct task_struct *efi_kworker_task;
> >> +static struct efivar_operations interruptible_ops;
> >> +static __init int efi_interruptible_init(void)
> >> +{
> >> +     int ret;
> >> +
> >> +     efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> >> +                                       "efi_kthread");
> >> +     if (IS_ERR(efi_kworker_task)) {
> >> +             pr_err("efi_interruptible: Failed to create kthread\n");
> >> +             ret = PTR_ERR(efi_kworker_task);
> >> +             efi_kworker_task = NULL;
> >> +             return ret;
> >> +     }
> >> +
> >> +     efi_kworker_task->mm = mm_alloc();
> >> +     efi_kworker_task->active_mm = efi_kworker_task->mm;
> >> +     efi_kworker_task->mm->pgd = efi_get_pgd();
> >> +     wake_up_process(efi_kworker_task);
> >> +
> >> +     atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
> >> +
> >> +     interruptible_ops.get_variable = get_variable_interruptible;
> >> +     interruptible_ops.set_variable = set_variable_interruptible;
> >> +     interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
> >> +     interruptible_ops.get_next_variable = get_next_variable_interruptible;
> >> +     interruptible_ops.query_variable_store = efi_query_variable_store;
> >> +     return efivars_register(&interruptible_efivars, &interruptible_ops,
> >> +                             efivars_kobject());
> >> +}
> >> +
> >
> > Is there some way we can match DMI/PCI identifiers for Broxton so that
> > we don't start seeing people accidentally running with preemptible EFI
> > services on non-BXT hardware?
> 
> I don't see how since this depends on a storage strategy more
> than the SoC itself, it can be used on future platforms or we can
> also get rid of it on BXT if an other storage is used for efi
> variables.
> Can't the KConfig protection be enough?

Kconfig is a last resort because it's a build-time decision and
greatly limits the flexibility of the kernel. It becomes no longer
possible to run a single kernel image with various CONFIG_* enabled on
x86 hardware - you now need a special EFI_INTERRUPTIBLE build.

Which apart from being a major headache for distributions in general
is generally frowned upon for the x86 architecture.

If there's any way at all of making this a runtime decision that would
be much better.

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Wed, 06 Jan, at 04:57:41PM, Sylvain Chouleur wrote:
> 2016-01-06 13:58 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> > On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>
> >> +static int efi_interruptible_panic_notifier_call(
> >> +     struct notifier_block *notifier,
> >> +     unsigned long what, void *data)
> >> +{
> >> +     static struct efivars generic_efivars;
> >> +     static struct efivar_operations generic_ops;
> >> +
> >> +     generic_ops.get_variable = efi.get_variable;
> >> +     generic_ops.set_variable = efi.set_variable;
> >> +     generic_ops.get_next_variable = efi.get_next_variable;
> >> +     generic_ops.query_variable_store = efi_query_variable_store;
> >> +
> >> +     efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
> >> +
> >> +     return NOTIFY_DONE;
> >> +}
> >> +
> >> +static struct notifier_block panic_nb = {
> >> +     .notifier_call = efi_interruptible_panic_notifier_call,
> >> +     .priority = 100,
> >> +};
> >> +
> >
> > What's the purpose of this? The only reason you'd want to do this that
> > I can think of is because you'd want efi-pstore to run and attempt to
> > save some crash data for you. But you can't build with efi-pstore
> > enable and CONFIG_EFI_INTERRUPTIBLE.
> >
> > I'd be tempted to just delete this notifier code.
> 
> This is indeed to be able to try write some crash data when we
> are in a panic context.  In fact with this restoration of legacy
> efi handlers on a panic we should be able to have pstore working
> with CONFIG_EFI_INTERRUPTIBLE.
 
This makes the efivar registration more complicated because now it can
fail if someone else is holding efivars_lock. Which is a strange
semantic for a registration function.

> I say should because there is still issues with BIOS to have the
> panic flow working.

How would it work though? You'd need the kernel thread controlling
access to the flash to be run in order for any data to appear in the
EFI variable store. Except you're in the middle of a panic and all
bets are off regarding which tasks are going to be run by the
scheduler.

Until those issues are resolved, please delete the notifier code. But
honestly, I doubt this will ever be useful in practice. Setting up
panic handlers *once* you've crashed is far too late.

> >> +static struct task_struct *efi_kworker_task;
> >> +static struct efivar_operations interruptible_ops;
> >> +static __init int efi_interruptible_init(void)
> >> +{
> >> +     int ret;
> >> +
> >> +     efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> >> +                                       "efi_kthread");
> >> +     if (IS_ERR(efi_kworker_task)) {
> >> +             pr_err("efi_interruptible: Failed to create kthread\n");
> >> +             ret = PTR_ERR(efi_kworker_task);
> >> +             efi_kworker_task = NULL;
> >> +             return ret;
> >> +     }
> >> +
> >> +     efi_kworker_task->mm = mm_alloc();
> >> +     efi_kworker_task->active_mm = efi_kworker_task->mm;
> >> +     efi_kworker_task->mm->pgd = efi_get_pgd();
> >> +     wake_up_process(efi_kworker_task);
> >> +
> >> +     atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
> >> +
> >> +     interruptible_ops.get_variable = get_variable_interruptible;
> >> +     interruptible_ops.set_variable = set_variable_interruptible;
> >> +     interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
> >> +     interruptible_ops.get_next_variable = get_next_variable_interruptible;
> >> +     interruptible_ops.query_variable_store = efi_query_variable_store;
> >> +     return efivars_register(&interruptible_efivars, &interruptible_ops,
> >> +                             efivars_kobject());
> >> +}
> >> +
> >
> > Is there some way we can match DMI/PCI identifiers for Broxton so that
> > we don't start seeing people accidentally running with preemptible EFI
> > services on non-BXT hardware?
> 
> I don't see how since this depends on a storage strategy more
> than the SoC itself, it can be used on future platforms or we can
> also get rid of it on BXT if an other storage is used for efi
> variables.
> Can't the KConfig protection be enough?

Kconfig is a last resort because it's a build-time decision and
greatly limits the flexibility of the kernel. It becomes no longer
possible to run a single kernel image with various CONFIG_* enabled on
x86 hardware - you now need a special EFI_INTERRUPTIBLE build.

Which apart from being a major headache for distributions in general
is generally frowned upon for the x86 architecture.

If there's any way at all of making this a runtime decision that would
be much better.

Re: [PATCH 1/2] efi: Don't use spinlocks for efi vars

[Matt Fleming]

On Wed, 06 Jan, at 04:22:40PM, Sylvain Chouleur wrote:
> 2016-01-06 13:24 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> > > @@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
> > >               return ret;
> > >
> > >       kobject_uevent(&new_var->kobj, KOBJ_ADD);
> > > -     efivar_entry_add(new_var, &efivar_sysfs_list);
> > > +     if (efivar_entry_add(new_var, &efivar_sysfs_list))
> > > +             return -EINTR;
> > >
> > >       return 0;
> > >  }
> >
> > This looks like it's missing a call to efivar_unregister() in the
> > -EINTR case.
> 
> I don't see why
> It's returning an error code as other lines in the same function.
> Can you develop your thoughts?
 
kobject_uevent() just notified userspace that a new object was
available. But because it hasn't been put onto 'efivar_sysfs_list'
when efivar_entry_add() fails, we'll never ever call kobject_put().

Before your changes efivar_entry_add() never failed and 'new_var' was
always added to the list, and always removed in efivar_sysfs_destroy().

That invariant no longer holds. Now we leak new_var->kobj.

> > > @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
> > >                    const struct efivar_operations *ops,
> > >                    struct kobject *kobject)
> > >  {
> > > -     spin_lock_init(&efivars->lock);
> > > +     if (down_trylock(&efivars_lock))
> > > +             return -EBUSY;
> > > +
> >
> > Is this correct? I would have assumed that you'd want to return -EINTR
> > here, not -EBUSY since if an EFI variable operation is currently
> > running we should spin waiting for the semaphore to be released.
> 
> No because this is a trylock, it will not wait for the semaphore to be release,
> it will return as soon as it sees that the semaphore is locked.
> We don't want to use down_interruptible() here because we want to be able to
> (un)register efivars operation in an interrupt context.

Like I said in the other mail, I think supporting registration from
within a panic context is a bad idea. Using down_interruptible() here
would be much better. 

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-01-08 11:38 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> On Wed, 06 Jan, at 04:57:41PM, Sylvain Chouleur wrote:
>> 2016-01-06 13:58 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
>> > On Fri, 18 Dec, at 11:29:51AM, sylvain.chouleur@gmail.com wrote:
>> >> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
>> >>
>> >> +static int efi_interruptible_panic_notifier_call(
>> >> +     struct notifier_block *notifier,
>> >> +     unsigned long what, void *data)
>> >> +{
>> >> +     static struct efivars generic_efivars;
>> >> +     static struct efivar_operations generic_ops;
>> >> +
>> >> +     generic_ops.get_variable = efi.get_variable;
>> >> +     generic_ops.set_variable = efi.set_variable;
>> >> +     generic_ops.get_next_variable = efi.get_next_variable;
>> >> +     generic_ops.query_variable_store = efi_query_variable_store;
>> >> +
>> >> +     efivars_register(&generic_efivars, &generic_ops, efivars_kobject());
>> >> +
>> >> +     return NOTIFY_DONE;
>> >> +}
>> >> +
>> >> +static struct notifier_block panic_nb = {
>> >> +     .notifier_call = efi_interruptible_panic_notifier_call,
>> >> +     .priority = 100,
>> >> +};
>> >> +
>> >
>> > What's the purpose of this? The only reason you'd want to do this that
>> > I can think of is because you'd want efi-pstore to run and attempt to
>> > save some crash data for you. But you can't build with efi-pstore
>> > enable and CONFIG_EFI_INTERRUPTIBLE.
>> >
>> > I'd be tempted to just delete this notifier code.
>>
>> This is indeed to be able to try write some crash data when we
>> are in a panic context.  In fact with this restoration of legacy
>> efi handlers on a panic we should be able to have pstore working
>> with CONFIG_EFI_INTERRUPTIBLE.
>
> This makes the efivar registration more complicated because now it can
> fail if someone else is holding efivars_lock. Which is a strange
> semantic for a registration function.

Right, I'll change the code to call legacy handlers from efi_interruptible code
directly it will be more consistent with a panic specific behavior.

>
>> I say should because there is still issues with BIOS to have the
>> panic flow working.
>
> How would it work though? You'd need the kernel thread controlling
> access to the flash to be run in order for any data to appear in the
> EFI variable store. Except you're in the middle of a panic and all
> bets are off regarding which tasks are going to be run by the
> scheduler.

The trick here is that in panic context, the cse is notified of this special
context and is asked to write data itself to the storage instead of sending
interrupt to the kernel. Then we don't need interruptible handler in
panic context. That's why we want to use legacy efi variable handlers.

>
> Until those issues are resolved, please delete the notifier code. But
> honestly, I doubt this will ever be useful in practice. Setting up
> panic handlers *once* you've crashed is far too late.

I understand, like I said above I'll modify efi_interruptible handlers to
call legacy ones in case of panic context.
I would like to avoid removing the panic part of this patch and take time
to clean it before merging the whole.

>
>> >> +static struct task_struct *efi_kworker_task;
>> >> +static struct efivar_operations interruptible_ops;
>> >> +static __init int efi_interruptible_init(void)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
>> >> +                                       "efi_kthread");
>> >> +     if (IS_ERR(efi_kworker_task)) {
>> >> +             pr_err("efi_interruptible: Failed to create kthread\n");
>> >> +             ret = PTR_ERR(efi_kworker_task);
>> >> +             efi_kworker_task = NULL;
>> >> +             return ret;
>> >> +     }
>> >> +
>> >> +     efi_kworker_task->mm = mm_alloc();
>> >> +     efi_kworker_task->active_mm = efi_kworker_task->mm;
>> >> +     efi_kworker_task->mm->pgd = efi_get_pgd();
>> >> +     wake_up_process(efi_kworker_task);
>> >> +
>> >> +     atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
>> >> +
>> >> +     interruptible_ops.get_variable = get_variable_interruptible;
>> >> +     interruptible_ops.set_variable = set_variable_interruptible;
>> >> +     interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
>> >> +     interruptible_ops.get_next_variable = get_next_variable_interruptible;
>> >> +     interruptible_ops.query_variable_store = efi_query_variable_store;
>> >> +     return efivars_register(&interruptible_efivars, &interruptible_ops,
>> >> +                             efivars_kobject());
>> >> +}
>> >> +
>> >
>> > Is there some way we can match DMI/PCI identifiers for Broxton so that
>> > we don't start seeing people accidentally running with preemptible EFI
>> > services on non-BXT hardware?
>>
>> I don't see how since this depends on a storage strategy more
>> than the SoC itself, it can be used on future platforms or we can
>> also get rid of it on BXT if an other storage is used for efi
>> variables.
>> Can't the KConfig protection be enough?
>
> Kconfig is a last resort because it's a build-time decision and
> greatly limits the flexibility of the kernel. It becomes no longer
> possible to run a single kernel image with various CONFIG_* enabled on
> x86 hardware - you now need a special EFI_INTERRUPTIBLE build.
>
> Which apart from being a major headache for distributions in general
> is generally frowned upon for the x86 architecture.
>
> If there's any way at all of making this a runtime decision that would
> be much better.

I think the best would be to bind this driver with the one which receives the
interrupts from CSE to write the variables. Then we would have a consistency
on the feature.
Does this seems ok for you?

-- 
Sylvain

Re: [PATCH 1/2] efi: Don't use spinlocks for efi vars

[Sylvain Chouleur]

2016-01-08 12:08 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> On Wed, 06 Jan, at 04:22:40PM, Sylvain Chouleur wrote:
>> 2016-01-06 13:24 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
>> > > @@ -582,7 +583,8 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
>> > >               return ret;
>> > >
>> > >       kobject_uevent(&new_var->kobj, KOBJ_ADD);
>> > > -     efivar_entry_add(new_var, &efivar_sysfs_list);
>> > > +     if (efivar_entry_add(new_var, &efivar_sysfs_list))
>> > > +             return -EINTR;
>> > >
>> > >       return 0;
>> > >  }
>> >
>> > This looks like it's missing a call to efivar_unregister() in the
>> > -EINTR case.
>>
>> I don't see why
>> It's returning an error code as other lines in the same function.
>> Can you develop your thoughts?
>
> kobject_uevent() just notified userspace that a new object was
> available. But because it hasn't been put onto 'efivar_sysfs_list'
> when efivar_entry_add() fails, we'll never ever call kobject_put().
>
> Before your changes efivar_entry_add() never failed and 'new_var' was
> always added to the list, and always removed in efivar_sysfs_destroy().
>
> That invariant no longer holds. Now we leak new_var->kobj.

Got it.

>
>> > > @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
>> > >                    const struct efivar_operations *ops,
>> > >                    struct kobject *kobject)
>> > >  {
>> > > -     spin_lock_init(&efivars->lock);
>> > > +     if (down_trylock(&efivars_lock))
>> > > +             return -EBUSY;
>> > > +
>> >
>> > Is this correct? I would have assumed that you'd want to return -EINTR
>> > here, not -EBUSY since if an EFI variable operation is currently
>> > running we should spin waiting for the semaphore to be released.
>>
>> No because this is a trylock, it will not wait for the semaphore to be release,
>> it will return as soon as it sees that the semaphore is locked.
>> We don't want to use down_interruptible() here because we want to be able to
>> (un)register efivars operation in an interrupt context.
>
> Like I said in the other mail, I think supporting registration from
> within a panic context is a bad idea. Using down_interruptible() here
> would be much better.

Ok

[PATCH v3 0/3] efi interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

Changes in v3:
 - efivars_(un)register: use down_interruptible()
 - efivar_create_sysfs_entry: unregister new_var in case of failure
 - efi_interruptible: in case of panic call directly legacy handlers

Sylvain Chouleur (3):
 efi: implement interruptible runtime services
 efi: don't use spinlocks for efi vars
 efi: use a file local lock for efivars

 arch/x86/Kconfig                          |   17 +++
 arch/x86/include/asm/efi.h                |    1 
 arch/x86/platform/efi/Makefile            |    1 
 arch/x86/platform/efi/efi_32.c            |    5 +
 arch/x86/platform/efi/efi_64.c            |    5 +
 arch/x86/platform/efi/efi_interruptible.c |  189 ++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c         |   36 +++++--
 drivers/firmware/efi/efivars.c            |   22 +++-
 drivers/firmware/efi/vars.c               |  315 ++++++++++++++++++++++++++++++++++++++++----------------------------
 fs/efivarfs/inode.c                       |    5 -
 fs/efivarfs/super.c                       |    9 +
 include/linux/efi.h                       |   18 ---
 12 files changed, 463 insertions(+), 160 deletions(-)

[PATCH v3 1/3] efi: use a file local lock for efivars

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch remove the efivars lock to use a single lock for the whole
vars.c file.  The goal of this lock is to protect concurrent calls to
efi variable services, registering and unregistering.  This allows to
register new efivars operations without having in-progress call.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 85 +++++++++++++++++++++++++--------------------
 include/linux/efi.h         |  6 ----
 2 files changed, 48 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index d2a49626a335..1851f492368f 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -37,6 +37,14 @@
 /* Private pointer to registered efivars */
 static struct efivars *__efivars;
 
+/*
+ * efivars_lock protects three things:
+ * 1) efivarfs_list and efivars_sysfs_list
+ * 2) ->ops calls
+ * 3) (un)registration of __efivars
+ */
+static DEFINE_SPINLOCK(efivars_lock);
+
 static bool efivar_wq_enabled = true;
 DECLARE_WORK(efivar_work, NULL);
 EXPORT_SYMBOL_GPL(efivar_work);
@@ -387,7 +395,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		return -ENOMEM;
 	}
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
@@ -403,7 +411,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		switch (status) {
 		case EFI_SUCCESS:
 			if (!atomic)
-				spin_unlock_irq(&__efivars->lock);
+				spin_unlock_irq(&efivars_lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
@@ -421,7 +429,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
 				if (!atomic)
-					spin_lock_irq(&__efivars->lock);
+					spin_lock_irq(&efivars_lock);
 
 				status = EFI_NOT_FOUND;
 				break;
@@ -432,7 +440,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 				status = EFI_NOT_FOUND;
 
 			if (!atomic)
-				spin_lock_irq(&__efivars->lock);
+				spin_lock_irq(&efivars_lock);
 
 			break;
 		case EFI_NOT_FOUND:
@@ -446,7 +454,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 
 	} while (status != EFI_NOT_FOUND);
 
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	kfree(variable_name);
 
@@ -461,9 +469,9 @@ EXPORT_SYMBOL_GPL(efivar_init);
  */
 void efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	list_add(&entry->list, head);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
@@ -473,9 +481,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_add);
  */
 void efivar_entry_remove(struct efivar_entry *entry)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_remove);
 
@@ -492,10 +500,10 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	list_del(&entry->list);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 
 /**
@@ -518,7 +526,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -544,12 +552,12 @@ int efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   0, 0, NULL);
 	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 		return efi_status_to_err(status);
 	}
 
@@ -587,10 +595,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 	efi_char16_t *name = entry->var.VariableName;
 	efi_guid_t vendor = entry->var.VendorGuid;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	if (head && efivar_entry_find(name, vendor, head, false)) {
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 		return -EEXIST;
 	}
 
@@ -599,7 +607,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		status = ops->set_variable(name, &vendor,
 					   attributes, size, data);
 
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	return efi_status_to_err(status);
 
@@ -613,7 +621,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_set);
  * from crash/panic handlers.
  *
  * Crucially, this function will not block if it cannot acquire
- * __efivars->lock. Instead, it returns -EBUSY.
+ * efivars_lock. Instead, it returns -EBUSY.
  */
 static int
 efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
@@ -623,20 +631,20 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&__efivars->lock, flags))
+	if (!spin_trylock_irqsave(&efivars_lock, flags))
 		return -EBUSY;
 
 	status = check_var_size_nonblocking(attributes,
 					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		spin_unlock_irqrestore(&efivars_lock, flags);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	spin_unlock_irqrestore(&efivars_lock, flags);
 	return efi_status_to_err(status);
 }
 
@@ -682,21 +690,21 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 						    size, data);
 
 	if (!block) {
-		if (!spin_trylock_irqsave(&__efivars->lock, flags))
+		if (!spin_trylock_irqsave(&efivars_lock, flags))
 			return -EBUSY;
 	} else {
-		spin_lock_irqsave(&__efivars->lock, flags);
+		spin_lock_irqsave(&efivars_lock, flags);
 	}
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&__efivars->lock, flags);
+		spin_unlock_irqrestore(&efivars_lock, flags);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable(name, &vendor, attributes, size, data);
 
-	spin_unlock_irqrestore(&__efivars->lock, flags);
+	spin_unlock_irqrestore(&efivars_lock, flags);
 
 	return efi_status_to_err(status);
 }
@@ -726,7 +734,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -769,10 +777,10 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
 
 	*size = 0;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid, NULL, size, NULL);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -798,7 +806,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&__efivars->lock);
+	lockdep_assert_held(&efivars_lock);
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -821,11 +829,11 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   attributes, size, data);
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -872,7 +880,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	 * set_variable call, and removal of the variable from the efivars
 	 * list (in the case of an authenticated delete).
 	 */
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 
 	/*
 	 * Ensure that the available space hasn't shrunk below the safe level
@@ -912,7 +920,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	if (status == EFI_NOT_FOUND)
 		efivar_entry_list_del_unlock(entry);
 	else
-		spin_unlock_irq(&__efivars->lock);
+		spin_unlock_irq(&efivars_lock);
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -920,7 +928,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return 0;
 
 out:
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 	return err;
 
 }
@@ -935,7 +943,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size);
  */
 void efivar_entry_iter_begin(void)
 {
-	spin_lock_irq(&__efivars->lock);
+	spin_lock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
 
@@ -946,7 +954,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
  */
 void efivar_entry_iter_end(void)
 {
-	spin_unlock_irq(&__efivars->lock);
+	spin_unlock_irq(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
 
@@ -1067,11 +1075,12 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
-	spin_lock_init(&efivars->lock);
+	spin_lock_irq(&efivars_lock);
 	efivars->ops = ops;
 	efivars->kobject = kobject;
 
 	__efivars = efivars;
+	spin_unlock_irq(&efivars_lock);
 
 	return 0;
 }
@@ -1088,6 +1097,7 @@ int efivars_unregister(struct efivars *efivars)
 {
 	int rv;
 
+	spin_lock_irq(&efivars_lock);
 	if (!__efivars) {
 		printk(KERN_ERR "efivars not registered\n");
 		rv = -EINVAL;
@@ -1103,6 +1113,7 @@ int efivars_unregister(struct efivars *efivars)
 
 	rv = 0;
 out:
+	spin_unlock_irq(&efivars_lock);
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_unregister);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 3c6cbbdae4aa..ca005eac825d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1100,12 +1100,6 @@ struct efivar_operations {
 };
 
 struct efivars {
-	/*
-	 * ->lock protects two things:
-	 * 1) efivarfs_list and efivars_sysfs_list
-	 * 2) ->ops calls
-	 */
-	spinlock_t lock;
 	struct kset *kset;
 	struct kobject *kobject;
 	const struct efivar_operations *ops;
-- 
2.6.3

[PATCH v3 2/3] efi: don't use spinlocks for efi vars

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

All efivars operations are protected by a spinlock which prevents
interruptions and preemption. This is too restricted, we just need a
lock preventing concurency.
The idea is to use a semaphore of count 1 and to have two ways of
locking, depending on the context:
 - In interrupt context, we call down_trylock(), if it fails we return
 an error
 - In normal context, we call down_interruptible()

We don't use a mutex here because the mutex_trylock() function must not
be called from interrupt context, whereas the down_trylock() can.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/efi-pstore.c |  36 +++++++---
 drivers/firmware/efi/efivars.c    |  22 ++++--
 drivers/firmware/efi/vars.c       | 145 +++++++++++++++++++++++---------------
 fs/efivarfs/inode.c               |   5 +-
 fs/efivarfs/super.c               |   9 ++-
 include/linux/efi.h               |   6 +-
 6 files changed, 149 insertions(+), 74 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index eac76a79a880..7bfaffb0e672 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -121,16 +121,19 @@ static void efi_pstore_scan_sysfs_enter(struct efivar_entry *pos,
  * @entry: deleting entry
  * @turn_off_scanning: Check if a scanning flag should be turned off
  */
-static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
+static inline int __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
 						bool turn_off_scanning)
 {
 	if (entry->deleting) {
 		list_del(&entry->list);
 		efivar_entry_iter_end();
 		efivar_unregister(entry);
-		efivar_entry_iter_begin();
+		if (efivar_entry_iter_begin())
+			return -EINTR;
 	} else if (turn_off_scanning)
 		entry->scanning = false;
+
+	return 0;
 }
 
 /**
@@ -140,13 +143,18 @@ static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry,
  * @head: list head
  * @stop: a flag checking if scanning will stop
  */
-static void efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
+static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
 				       struct efivar_entry *next,
 				       struct list_head *head, bool stop)
 {
-	__efi_pstore_scan_sysfs_exit(pos, true);
+	int ret = __efi_pstore_scan_sysfs_exit(pos, true);
+
+	if (ret)
+		return ret;
+
 	if (stop)
-		__efi_pstore_scan_sysfs_exit(next, &next->list != head);
+		ret = __efi_pstore_scan_sysfs_exit(next, &next->list != head);
+	return ret;
 }
 
 /**
@@ -168,13 +176,17 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 	struct efivar_entry *entry, *n;
 	struct list_head *head = &efivar_sysfs_list;
 	int size = 0;
+	int ret;
 
 	if (!*pos) {
 		list_for_each_entry_safe(entry, n, head, list) {
 			efi_pstore_scan_sysfs_enter(entry, n, head);
 
 			size = efi_pstore_read_func(entry, data);
-			efi_pstore_scan_sysfs_exit(entry, n, head, size < 0);
+			ret = efi_pstore_scan_sysfs_exit(entry, n, head,
+							 size < 0);
+			if (ret)
+				return ret;
 			if (size)
 				break;
 		}
@@ -186,7 +198,9 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
 		efi_pstore_scan_sysfs_enter((*pos), n, head);
 
 		size = efi_pstore_read_func((*pos), data);
-		efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		if (ret)
+			return ret;
 		if (size)
 			break;
 	}
@@ -226,7 +240,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 	if (!*data.buf)
 		return -ENOMEM;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin()) {
+		kfree(*data.buf);
+		return -EINTR;
+	}
 	size = efi_pstore_sysfs_entry_iter(&data,
 					   (struct efivar_entry **)&psi->data);
 	efivar_entry_iter_end();
@@ -341,7 +358,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 	edata.time = time;
 	edata.name = efi_name;
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, &edata, &entry);
 
 	if (found && !entry->scanning) {
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index c5b0d2bc1111..6e5871f19c91 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -509,7 +509,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 		vendor = del_var->VendorGuid;
 	}
 
-	efivar_entry_iter_begin();
+	if (efivar_entry_iter_begin())
+		return -EINTR;
 	entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true);
 	if (!entry)
 		err = -EINVAL;
@@ -582,7 +583,10 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
 		return ret;
 
 	kobject_uevent(&new_var->kobj, KOBJ_ADD);
-	efivar_entry_add(new_var, &efivar_sysfs_list);
+	if (efivar_entry_add(new_var, &efivar_sysfs_list)) {
+		efivar_unregister(new_var);
+		return -EINTR;
+	}
 
 	return 0;
 }
@@ -697,7 +701,10 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor,
 
 static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+
+	if (err)
+		return err;
 	efivar_unregister(entry);
 	return 0;
 }
@@ -705,7 +712,14 @@ static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data)
 static void efivars_sysfs_exit(void)
 {
 	/* Remove all entries and destroy */
-	__efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, NULL, NULL);
+	int err;
+
+	err = __efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list,
+				  NULL, NULL);
+	if (err) {
+		pr_err("efivars: Failed to destroy sysfs entries\n");
+		return;
+	}
 
 	if (efivars_new_var)
 		sysfs_remove_bin_file(&efivars_kset->kobj, efivars_new_var);
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 1851f492368f..d3c1526d17f5 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -43,7 +43,7 @@ static struct efivars *__efivars;
  * 2) ->ops calls
  * 3) (un)registration of __efivars
  */
-static DEFINE_SPINLOCK(efivars_lock);
+static DEFINE_SEMAPHORE(efivars_lock);
 
 static bool efivar_wq_enabled = true;
 DECLARE_WORK(efivar_work, NULL);
@@ -395,7 +395,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		return -ENOMEM;
 	}
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock)) {
+		err = -EINTR;
+		goto free;
+	}
 
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
@@ -411,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		switch (status) {
 		case EFI_SUCCESS:
 			if (!atomic)
-				spin_unlock_irq(&efivars_lock);
+				up(&efivars_lock);
 
 			variable_name_size = var_name_strnsize(variable_name,
 							       variable_name_size);
@@ -428,8 +431,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			    variable_is_present(variable_name, &vendor_guid, head)) {
 				dup_variable_bug(variable_name, &vendor_guid,
 						 variable_name_size);
-				if (!atomic)
-					spin_lock_irq(&efivars_lock);
+				if (!atomic) {
+					if (down_interruptible(&efivars_lock)) {
+						err = -EINTR;
+						goto free;
+					}
+				}
 
 				status = EFI_NOT_FOUND;
 				break;
@@ -439,8 +446,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			if (err)
 				status = EFI_NOT_FOUND;
 
-			if (!atomic)
-				spin_lock_irq(&efivars_lock);
+			if (!atomic) {
+				if (down_interruptible(&efivars_lock)) {
+					err = -EINTR;
+					goto free;
+				}
+			}
 
 			break;
 		case EFI_NOT_FOUND:
@@ -454,8 +465,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 
 	} while (status != EFI_NOT_FOUND);
 
-	spin_unlock_irq(&efivars_lock);
-
+	up(&efivars_lock);
+free:
 	kfree(variable_name);
 
 	return err;
@@ -466,24 +477,34 @@ EXPORT_SYMBOL_GPL(efivar_init);
  * efivar_entry_add - add entry to variable list
  * @entry: entry to add to list
  * @head: list head
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_add(&entry->list, head);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
 /**
  * efivar_entry_remove - remove entry from variable list
  * @entry: entry to remove from list
+ *
+ * Returns 0 on success, or a kernel error code on failure.
  */
-void efivar_entry_remove(struct efivar_entry *entry)
+int efivar_entry_remove(struct efivar_entry *entry)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	list_del(&entry->list);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(efivar_entry_remove);
 
@@ -500,10 +521,10 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove);
  */
 static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
 {
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_del(&entry->list);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 }
 
 /**
@@ -526,7 +547,7 @@ int __efivar_entry_delete(struct efivar_entry *entry)
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -544,20 +565,22 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete);
  * variable list. It is the caller's responsibility to free @entry
  * once we return.
  *
- * Returns 0 on success, or a converted EFI status code if
- * set_variable() fails.
+ * Returns 0 on success, -EINTR if we can't grab the semaphore,
+ * converted EFI status code if set_variable() fails.
  */
 int efivar_entry_delete(struct efivar_entry *entry)
 {
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
+
 	status = ops->set_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   0, 0, NULL);
 	if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) {
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 		return efi_status_to_err(status);
 	}
 
@@ -583,9 +606,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
  * If @head is not NULL a lookup is performed to determine whether
  * the entry is already on the list.
  *
- * Returns 0 on success, -EEXIST if a lookup is performed and the entry
- * already exists on the list, or a converted EFI status code if
- * set_variable() fails.
+ * Returns 0 on success, -EINTR if we can't grab the semaphore,
+ * -EEXIST if a lookup is performed and the entry already exists on
+ * the list, or a converted EFI status code if set_variable() fails.
  */
 int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		     unsigned long size, void *data, struct list_head *head)
@@ -595,10 +618,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 	efi_char16_t *name = entry->var.VariableName;
 	efi_guid_t vendor = entry->var.VendorGuid;
 
-	spin_lock_irq(&efivars_lock);
-
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	if (head && efivar_entry_find(name, vendor, head, false)) {
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 		return -EEXIST;
 	}
 
@@ -607,7 +630,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
 		status = ops->set_variable(name, &vendor,
 					   attributes, size, data);
 
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 
@@ -628,23 +651,22 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 			     u32 attributes, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efivars_lock, flags))
+	if (down_trylock(&efivars_lock))
 		return -EBUSY;
 
 	status = check_var_size_nonblocking(attributes,
 					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&efivars_lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
-	spin_unlock_irqrestore(&efivars_lock, flags);
+	up(&efivars_lock);
 	return efi_status_to_err(status);
 }
 
@@ -669,7 +691,6 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data)
 {
 	const struct efivar_operations *ops = __efivars->ops;
-	unsigned long flags;
 	efi_status_t status;
 
 	if (!ops->query_variable_store)
@@ -690,21 +711,22 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 						    size, data);
 
 	if (!block) {
-		if (!spin_trylock_irqsave(&efivars_lock, flags))
+		if (down_trylock(&efivars_lock))
 			return -EBUSY;
 	} else {
-		spin_lock_irqsave(&efivars_lock, flags);
+		if (down_interruptible(&efivars_lock))
+			return -EINTR;
 	}
 
 	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
-		spin_unlock_irqrestore(&efivars_lock, flags);
+		up(&efivars_lock);
 		return -ENOSPC;
 	}
 
 	status = ops->set_variable(name, &vendor, attributes, size, data);
 
-	spin_unlock_irqrestore(&efivars_lock, flags);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -734,7 +756,7 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 	int strsize1, strsize2;
 	bool found = false;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	list_for_each_entry_safe(entry, n, head, list) {
 		strsize1 = ucs2_strsize(name, 1024);
@@ -777,10 +799,11 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
 
 	*size = 0;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid, NULL, size, NULL);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -806,7 +829,7 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	lockdep_assert_held(&efivars_lock);
+	lockdep_assert_held(&efivars_lock.lock);
 
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
@@ -829,11 +852,12 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
 	const struct efivar_operations *ops = __efivars->ops;
 	efi_status_t status;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 	status = ops->get_variable(entry->var.VariableName,
 				   &entry->var.VendorGuid,
 				   attributes, size, data);
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 
 	return efi_status_to_err(status);
 }
@@ -880,7 +904,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	 * set_variable call, and removal of the variable from the efivars
 	 * list (in the case of an authenticated delete).
 	 */
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
 
 	/*
 	 * Ensure that the available space hasn't shrunk below the safe level
@@ -920,7 +945,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	if (status == EFI_NOT_FOUND)
 		efivar_entry_list_del_unlock(entry);
 	else
-		spin_unlock_irq(&efivars_lock);
+		up(&efivars_lock);
 
 	if (status && status != EFI_BUFFER_TOO_SMALL)
 		return efi_status_to_err(status);
@@ -928,7 +953,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 	return 0;
 
 out:
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 	return err;
 
 }
@@ -941,9 +966,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size);
  * efivar_entry_iter_end() is called. This function is usually used in
  * conjunction with __efivar_entry_iter() or efivar_entry_iter().
  */
-void efivar_entry_iter_begin(void)
+int efivar_entry_iter_begin(void)
 {
-	spin_lock_irq(&efivars_lock);
+	return down_interruptible(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
 
@@ -954,7 +979,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin);
  */
 void efivar_entry_iter_end(void)
 {
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 }
 EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
 
@@ -1030,7 +1055,9 @@ int efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
 {
 	int err = 0;
 
-	efivar_entry_iter_begin();
+	err = efivar_entry_iter_begin();
+	if (err)
+		return err;
 	err = __efivar_entry_iter(func, head, data, NULL);
 	efivar_entry_iter_end();
 
@@ -1075,12 +1102,17 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
+
 	efivars->ops = ops;
 	efivars->kobject = kobject;
 
 	__efivars = efivars;
-	spin_unlock_irq(&efivars_lock);
+
+	pr_info("Registered efivars operations\n");
+
+	up(&efivars_lock);
 
 	return 0;
 }
@@ -1097,7 +1129,9 @@ int efivars_unregister(struct efivars *efivars)
 {
 	int rv;
 
-	spin_lock_irq(&efivars_lock);
+	if (down_interruptible(&efivars_lock))
+		return -EINTR;
+
 	if (!__efivars) {
 		printk(KERN_ERR "efivars not registered\n");
 		rv = -EINVAL;
@@ -1109,11 +1143,12 @@ int efivars_unregister(struct efivars *efivars)
 		goto out;
 	}
 
+	pr_info("Unregistered efivars operations\n");
 	__efivars = NULL;
 
 	rv = 0;
 out:
-	spin_unlock_irq(&efivars_lock);
+	up(&efivars_lock);
 	return rv;
 }
 EXPORT_SYMBOL_GPL(efivars_unregister);
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 3381b9da9ee6..5cea84ef7a44 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -132,7 +132,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 
 	inode->i_private = var;
 
-	efivar_entry_add(var, &efivarfs_list);
+	err = efivar_entry_add(var, &efivarfs_list);
+	if (err)
+		goto out;
+
 	d_instantiate(dentry, inode);
 	dget(dentry);
 out:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 86a2121828c3..6825f1e9718b 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -158,7 +158,9 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 	kfree(name);
 
 	efivar_entry_size(entry, &size);
-	efivar_entry_add(entry, &efivarfs_list);
+	err = efivar_entry_add(entry, &efivarfs_list);
+	if (err)
+		goto fail_inode;
 
 	mutex_lock(&inode->i_mutex);
 	inode->i_private = entry;
@@ -179,7 +181,10 @@ fail:
 
 static int efivarfs_destroy(struct efivar_entry *entry, void *data)
 {
-	efivar_entry_remove(entry);
+	int err = efivar_entry_remove(entry);
+
+	if (err)
+		return err;
 	kfree(entry);
 	return 0;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ca005eac825d..1a2363fb69b6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1167,8 +1167,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 		void *data, bool atomic, bool duplicates,
 		struct list_head *head);
 
-void efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
-void efivar_entry_remove(struct efivar_entry *entry);
+int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
+int efivar_entry_remove(struct efivar_entry *entry);
 
 int __efivar_entry_delete(struct efivar_entry *entry);
 int efivar_entry_delete(struct efivar_entry *entry);
@@ -1185,7 +1185,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
 int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 			  bool block, unsigned long size, void *data);
 
-void efivar_entry_iter_begin(void);
+int efivar_entry_iter_begin(void);
 void efivar_entry_iter_end(void);
 
 int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
-- 
2.6.3

[PATCH v3 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

From: Sylvain Chouleur <sylvain.chouleur@intel.com>

This patch adds an implementation of EFI runtime services wappers which
allow interrupts and preemption during execution of BIOS code.

It's needed by Broxton platform which requires the OS to do the write
of the non volatile variables. To do that, the OS will receive an
interrupt coming from the firmware to notify that it must write the
data.

BIOS code must be executed using a specific PGD. This is currently
done by efi_call() which force value of CR3 before calling BIOS and
restore previous value after.

We use a dedicated kthread which will execute all interruptible runtime
services. This efi_kthread is special because it has it's own mm_struct
whereas kthread should not have one. This mm_struct contains the EFI PGD
so the scheduler will load it before running any BIOS code.

Obviously, an interruptible runtime service must never be called from an
interrupt context. EFI pstore is the only use case here, that's why we
require it to be disabled when activating interruptible runtime services.

Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
---
 arch/x86/Kconfig                          |  17 +++
 arch/x86/include/asm/efi.h                |   1 +
 arch/x86/platform/efi/Makefile            |   1 +
 arch/x86/platform/efi/efi_32.c            |   5 +
 arch/x86/platform/efi/efi_64.c            |   5 +
 arch/x86/platform/efi/efi_interruptible.c | 189 ++++++++++++++++++++++++++++++
 6 files changed, 218 insertions(+)
 create mode 100644 arch/x86/platform/efi/efi_interruptible.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..37301516f4e6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1720,6 +1720,23 @@ config EFI_MIXED
 
 	   If unsure, say N.
 
+if X86_64
+config EFI_INTERRUPTIBLE
+	bool "Interruptible Efi Runtime Services"
+	depends on EFI && !EFI_VARS_PSTORE
+	default n
+	---help---
+	  Only use this if you really know what you are doing, you
+	  possibly risk to break your BIOS.
+
+	  This is an interruptible implementation of efi runtime
+	  services wrappers. If you say Y, BIOS code could be
+	  interrupted and preempted as a standard process. Enable this
+	  only if you are sure that your BIOS will support
+	  interruptible efi calls
+	  In doubt, say "N".
+endif
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 8fd9e637629a..5b94d9dd9409 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -137,6 +137,7 @@ extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
 extern int __init efi_alloc_page_tables(void);
+extern pgd_t *efi_get_pgd(void);
 extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 2846aaab5103..ff57d3840842 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_INTERRUPTIBLE)	+= efi_interruptible.o
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 58d669bc8250..f2403e16a8bb 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -61,6 +61,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
+pgd_t *efi_get_pgd(void)
+{
+	return initial_page_table;
+}
+
 pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index b492521503fe..7886c7527cdf 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -212,6 +212,11 @@ void efi_sync_low_kernel_mappings(void)
 	memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
 }
 
+pgd_t *efi_get_pgd(void)
+{
+	return __va(efi_scratch.efi_pgt);
+}
+
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
 	unsigned long pfn, text;
diff --git a/arch/x86/platform/efi/efi_interruptible.c b/arch/x86/platform/efi/efi_interruptible.c
new file mode 100644
index 000000000000..2b24f915fa19
--- /dev/null
+++ b/arch/x86/platform/efi/efi_interruptible.c
@@ -0,0 +1,189 @@
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * Author: Sylvain Chouleur <sylvain.chouleur@intel.com>
+ *
+ * Distributed under the terms of the GNU GPL, version 2
+ */
+
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+static DEFINE_KTHREAD_WORKER(efi_kworker);
+static bool in_panic = false;
+
+#define efi_call_interruptible(f, ...)					\
+({									\
+	efi_status_t __s;						\
+	efi_sync_low_kernel_mappings();					\
+	__kernel_fpu_begin();						\
+	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
+	__kernel_fpu_end();						\
+	__s;								\
+})
+
+struct efi_work {
+	struct kthread_work work;
+	efi_status_t status;
+	unsigned long *name_size;
+	efi_char16_t *name;
+	efi_guid_t *vendor;
+	u32 *attr;
+	unsigned long *data_size;
+	void *data;
+};
+
+static void do_get_next_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_next_variable, ew->name_size,
+					    ew->name, ew->vendor);
+}
+
+static void do_set_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(set_variable, ew->name, ew->vendor,
+					    *ew->attr, *ew->data_size,
+					    ew->data);
+}
+
+static void do_get_variable_interruptible(struct kthread_work *work)
+{
+	struct efi_work *ew = container_of(work, struct efi_work, work);
+
+	ew->status = efi_call_interruptible(get_variable, ew->name, ew->vendor,
+					    ew->attr, ew->data_size, ew->data);
+}
+
+static efi_status_t execute_service(kthread_work_func_t service,
+				    unsigned long *name_size,
+				    efi_char16_t *name, efi_guid_t *vendor,
+				    u32 *attr, unsigned long *data_size,
+				    void *data)
+{
+	struct efi_work work = {
+		.name_size = name_size,
+		.name = name,
+		.vendor = vendor,
+		.attr = attr,
+		.data_size = data_size,
+		.data = data,
+	};
+
+	init_kthread_work(&work.work, service);
+	queue_kthread_work(&efi_kworker, &work.work);
+
+	flush_kthread_work(&work.work);
+	return work.status;
+}
+
+static efi_status_t get_next_variable_interruptible(unsigned long *name_size,
+						    efi_char16_t *name,
+						    efi_guid_t *vendor)
+{
+	return execute_service(do_get_next_variable_interruptible,
+			       name_size, name, vendor, NULL, NULL, NULL);
+}
+
+static efi_status_t set_variable_interruptible(efi_char16_t *name,
+					       efi_guid_t *vendor,
+					       u32 attr,
+					       unsigned long data_size,
+					       void *data)
+{
+	if (in_panic)
+		return efi.set_variable(name, vendor, attr, data_size, data);
+	return execute_service(do_set_variable_interruptible,
+			       NULL, name, vendor, &attr, &data_size, data);
+}
+
+static efi_status_t non_blocking_not_allowed(efi_char16_t *name,
+					     efi_guid_t *vendor,
+					     u32 attr,
+					     unsigned long data_size,
+					     void *data)
+{
+	if (in_panic)
+		return efi.set_variable_nonblocking(name, vendor, attr,
+						    data_size, data);
+	pr_err("efi_interruptible: non blocking operation is not allowed\n");
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t get_variable_interruptible(efi_char16_t *name,
+					       efi_guid_t *vendor,
+					       u32 *attr,
+					       unsigned long *data_size,
+					       void *data)
+{
+	if (in_panic)
+		return efi.get_variable(name, vendor, attr, data_size, data);
+	return execute_service(do_get_variable_interruptible,
+			       NULL, name, vendor, attr, data_size, data);
+}
+
+static struct efivars interruptible_efivars;
+
+static int efi_interruptible_panic_notifier_call(
+	struct notifier_block *notifier,
+	unsigned long what, void *data)
+{
+	in_panic = true;
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_nb = {
+	.notifier_call = efi_interruptible_panic_notifier_call,
+	.priority = 100,
+};
+
+static struct task_struct *efi_kworker_task;
+static struct efivar_operations interruptible_ops;
+static __init int efi_interruptible_init(void)
+{
+	int ret;
+
+	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
+					  "efi_kthread");
+	if (IS_ERR(efi_kworker_task)) {
+		pr_err("efi_interruptible: Failed to create kthread\n");
+		ret = PTR_ERR(efi_kworker_task);
+		efi_kworker_task = NULL;
+		return ret;
+	}
+
+	efi_kworker_task->mm = mm_alloc();
+	efi_kworker_task->active_mm = efi_kworker_task->mm;
+	efi_kworker_task->mm->pgd = efi_get_pgd();
+	wake_up_process(efi_kworker_task);
+
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_nb);
+
+	interruptible_ops.get_variable = get_variable_interruptible;
+	interruptible_ops.set_variable = set_variable_interruptible;
+	interruptible_ops.set_variable_nonblocking = non_blocking_not_allowed;
+	interruptible_ops.get_next_variable = get_next_variable_interruptible;
+	interruptible_ops.query_variable_store = efi_query_variable_store;
+	return efivars_register(&interruptible_efivars, &interruptible_ops,
+				efivars_kobject());
+}
+
+static void __exit efi_interruptible_exit(void)
+{
+	efivars_unregister(&interruptible_efivars);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &panic_nb);
+	if (efi_kworker_task) {
+		kthread_stop(efi_kworker_task);
+		mmdrop(efi_kworker_task->mm);
+	}
+}
+
+module_init(efi_interruptible_init);
+module_exit(efi_interruptible_exit);
+
+MODULE_AUTHOR("Sylvain Chouleur <sylvain.chouleur@intel.com>");
+MODULE_LICENSE("GPL");
-- 
2.6.3

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Fri, 08 Jan, at 02:57:13PM, Sylvain Chouleur wrote:
> 
> I understand, like I said above I'll modify efi_interruptible handlers to
> call legacy ones in case of panic context.
> I would like to avoid removing the panic part of this patch and take time
> to clean it before merging the whole.
 
OK, well at least split out the panic diddling into a separate patch
so that we can discuss the merits of it separately to the other, less
contentious changes.

> > Kconfig is a last resort because it's a build-time decision and
> > greatly limits the flexibility of the kernel. It becomes no longer
> > possible to run a single kernel image with various CONFIG_* enabled on
> > x86 hardware - you now need a special EFI_INTERRUPTIBLE build.
> >
> > Which apart from being a major headache for distributions in general
> > is generally frowned upon for the x86 architecture.
> >
> > If there's any way at all of making this a runtime decision that would
> > be much better.
> 
> I think the best would be to bind this driver with the one which receives the
> interrupts from CSE to write the variables. Then we would have a consistency
> on the feature.
> Does this seems ok for you?

I think that'd be an improvement, yeah.

Re: [PATCH 2/2] efi: implement interruptible runtime services

[Matt Fleming]

On Fri, 08 Jan, at 02:57:13PM, Sylvain Chouleur wrote:
> 
> I understand, like I said above I'll modify efi_interruptible handlers to
> call legacy ones in case of panic context.
> I would like to avoid removing the panic part of this patch and take time
> to clean it before merging the whole.
 
OK, well at least split out the panic diddling into a separate patch
so that we can discuss the merits of it separately to the other, less
contentious changes.

> > Kconfig is a last resort because it's a build-time decision and
> > greatly limits the flexibility of the kernel. It becomes no longer
> > possible to run a single kernel image with various CONFIG_* enabled on
> > x86 hardware - you now need a special EFI_INTERRUPTIBLE build.
> >
> > Which apart from being a major headache for distributions in general
> > is generally frowned upon for the x86 architecture.
> >
> > If there's any way at all of making this a runtime decision that would
> > be much better.
> 
> I think the best would be to bind this driver with the one which receives the
> interrupts from CSE to write the variables. Then we would have a consistency
> on the feature.
> Does this seems ok for you?

I think that'd be an improvement, yeah.

Re: [PATCH v3 1/3] efi: use a file local lock for efivars

[Matt Fleming]

On Wed, 13 Jan, at 05:32:40PM, Sylvain Chouleur wrote:
> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch remove the efivars lock to use a single lock for the whole
> vars.c file.  The goal of this lock is to protect concurrent calls to
> efi variable services, registering and unregistering.  This allows to
> register new efivars operations without having in-progress call.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/vars.c | 85 +++++++++++++++++++++++++--------------------
>  include/linux/efi.h         |  6 ----
>  2 files changed, 48 insertions(+), 43 deletions(-)

Sorry for the delay in reviewing this. I wasn't on Cc so I only found
it when searching through the linux-efi archive.

This change looks fine to me and a nice clean up. Ard any comments?

Re: [PATCH v3 1/3] efi: use a file local lock for efivars

[Ard Biesheuvel]

On 11 February 2016 at 14:14, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Wed, 13 Jan, at 05:32:40PM, Sylvain Chouleur wrote:
>> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> This patch remove the efivars lock to use a single lock for the whole
>> vars.c file.  The goal of this lock is to protect concurrent calls to
>> efi variable services, registering and unregistering.  This allows to
>> register new efivars operations without having in-progress call.
>>
>> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/firmware/efi/vars.c | 85 +++++++++++++++++++++++++--------------------
>>  include/linux/efi.h         |  6 ----
>>  2 files changed, 48 insertions(+), 43 deletions(-)
>
> Sorry for the delay in reviewing this. I wasn't on Cc so I only found
> it when searching through the linux-efi archive.
>
> This change looks fine to me and a nice clean up. Ard any comments?

No looks good to me

Re: [PATCH v3 2/3] efi: don't use spinlocks for efi vars

[Matt Fleming]

On Wed, 13 Jan, at 05:32:41PM, Sylvain Chouleur wrote:
> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> All efivars operations are protected by a spinlock which prevents
> interruptions and preemption. This is too restricted, we just need a
> lock preventing concurency.
> The idea is to use a semaphore of count 1 and to have two ways of
> locking, depending on the context:
>  - In interrupt context, we call down_trylock(), if it fails we return
>  an error
>  - In normal context, we call down_interruptible()
> 
> We don't use a mutex here because the mutex_trylock() function must not
> be called from interrupt context, whereas the down_trylock() can.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/efi-pstore.c |  36 +++++++---
>  drivers/firmware/efi/efivars.c    |  22 ++++--
>  drivers/firmware/efi/vars.c       | 145 +++++++++++++++++++++++---------------
>  fs/efivarfs/inode.c               |   5 +-
>  fs/efivarfs/super.c               |   9 ++-
>  include/linux/efi.h               |   6 +-
>  6 files changed, 149 insertions(+), 74 deletions(-)

Looks fine to me.

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Matt Fleming]

On Wed, 13 Jan, at 05:32:42PM, Sylvain Chouleur wrote:
> From: Sylvain Chouleur <sylvain.chouleur@intel.com>
> 
> This patch adds an implementation of EFI runtime services wappers which
> allow interrupts and preemption during execution of BIOS code.
> 
> It's needed by Broxton platform which requires the OS to do the write
> of the non volatile variables. To do that, the OS will receive an
> interrupt coming from the firmware to notify that it must write the
> data.
> 
> BIOS code must be executed using a specific PGD. This is currently
> done by efi_call() which force value of CR3 before calling BIOS and
> restore previous value after.
> 
> We use a dedicated kthread which will execute all interruptible runtime
> services. This efi_kthread is special because it has it's own mm_struct
> whereas kthread should not have one. This mm_struct contains the EFI PGD
> so the scheduler will load it before running any BIOS code.
> 
> Obviously, an interruptible runtime service must never be called from an
> interrupt context. EFI pstore is the only use case here, that's why we
> require it to be disabled when activating interruptible runtime services.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur@intel.com>
> ---
>  arch/x86/Kconfig                          |  17 +++
>  arch/x86/include/asm/efi.h                |   1 +
>  arch/x86/platform/efi/Makefile            |   1 +
>  arch/x86/platform/efi/efi_32.c            |   5 +
>  arch/x86/platform/efi/efi_64.c            |   5 +
>  arch/x86/platform/efi/efi_interruptible.c | 189 ++++++++++++++++++++++++++++++
>  6 files changed, 218 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c

Last time Peter and I talked about this patch (sometime last year), he
said he had a different approach in mind.

Peter, do you have any comments on this patch?

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..37301516f4e6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,23 @@ config EFI_MIXED
>  
>  	   If unsure, say N.
>  
> +if X86_64
> +config EFI_INTERRUPTIBLE
> +	bool "Interruptible Efi Runtime Services"
> +	depends on EFI && !EFI_VARS_PSTORE
> +	default n
> +	---help---
> +	  Only use this if you really know what you are doing, you
> +	  possibly risk to break your BIOS.
> +
> +	  This is an interruptible implementation of efi runtime
> +	  services wrappers. If you say Y, BIOS code could be
> +	  interrupted and preempted as a standard process. Enable this
> +	  only if you are sure that your BIOS will support
> +	  interruptible efi calls
> +	  In doubt, say "N".
> +endif
> +
>  config SECCOMP
>  	def_bool y
>  	prompt "Enable seccomp to safely compute untrusted bytecode"
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 8fd9e637629a..5b94d9dd9409 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -137,6 +137,7 @@ extern void __init efi_map_region(efi_memory_desc_t *md);
>  extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
>  extern void efi_sync_low_kernel_mappings(void);
>  extern int __init efi_alloc_page_tables(void);
> +extern pgd_t *efi_get_pgd(void);
>  extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init old_map_region(efi_memory_desc_t *md);
> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
> index 2846aaab5103..ff57d3840842 100644
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
>  obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>  obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
>  obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
> +obj-$(CONFIG_EFI_INTERRUPTIBLE)	+= efi_interruptible.o
> diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
> index 58d669bc8250..f2403e16a8bb 100644
> --- a/arch/x86/platform/efi/efi_32.c
> +++ b/arch/x86/platform/efi/efi_32.c
> @@ -61,6 +61,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
>  void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
>  void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return initial_page_table;
> +}
> +
>  pgd_t * __init efi_call_phys_prolog(void)
>  {
>  	struct desc_ptr gdt_descr;

Shouldn't be necessary. You can't build this for 32-bit because of the
Kconfig magic.

> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index b492521503fe..7886c7527cdf 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -212,6 +212,11 @@ void efi_sync_low_kernel_mappings(void)
>  	memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
>  }
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return __va(efi_scratch.efi_pgt);
> +}
> +

You could make this easier and just return 'efi_pgd', since we now
have entirely separate EFI page tables.

> +static struct efivars interruptible_efivars;
> +
> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	in_panic = true;
> +	return NOTIFY_DONE;
> +}

Please make the panic notifier changes a separate patch.

> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +
> +static struct task_struct *efi_kworker_task;
> +static struct efivar_operations interruptible_ops;
> +static __init int efi_interruptible_init(void)
> +{
> +	int ret;
> +
> +	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> +					  "efi_kthread");
> +	if (IS_ERR(efi_kworker_task)) {
> +		pr_err("efi_interruptible: Failed to create kthread\n");
> +		ret = PTR_ERR(efi_kworker_task);
> +		efi_kworker_task = NULL;
> +		return ret;
> +	}
> +
> +	efi_kworker_task->mm = mm_alloc();
> +	efi_kworker_task->active_mm = efi_kworker_task->mm;
> +	efi_kworker_task->mm->pgd = efi_get_pgd();
> +	wake_up_process(efi_kworker_task);

Did you have any luck binding this driver to the other
Broxton-specific drivers?

Additionally, you're going to want to make sure that
efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
otherwise you can't use the new EFI page table scheme, e.g. if someone
boots with efi=old_map.

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Matt Fleming]

On Wed, 13 Jan, at 05:32:42PM, Sylvain Chouleur wrote:
> From: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch adds an implementation of EFI runtime services wappers which
> allow interrupts and preemption during execution of BIOS code.
> 
> It's needed by Broxton platform which requires the OS to do the write
> of the non volatile variables. To do that, the OS will receive an
> interrupt coming from the firmware to notify that it must write the
> data.
> 
> BIOS code must be executed using a specific PGD. This is currently
> done by efi_call() which force value of CR3 before calling BIOS and
> restore previous value after.
> 
> We use a dedicated kthread which will execute all interruptible runtime
> services. This efi_kthread is special because it has it's own mm_struct
> whereas kthread should not have one. This mm_struct contains the EFI PGD
> so the scheduler will load it before running any BIOS code.
> 
> Obviously, an interruptible runtime service must never be called from an
> interrupt context. EFI pstore is the only use case here, that's why we
> require it to be disabled when activating interruptible runtime services.
> 
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/Kconfig                          |  17 +++
>  arch/x86/include/asm/efi.h                |   1 +
>  arch/x86/platform/efi/Makefile            |   1 +
>  arch/x86/platform/efi/efi_32.c            |   5 +
>  arch/x86/platform/efi/efi_64.c            |   5 +
>  arch/x86/platform/efi/efi_interruptible.c | 189 ++++++++++++++++++++++++++++++
>  6 files changed, 218 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efi_interruptible.c

Last time Peter and I talked about this patch (sometime last year), he
said he had a different approach in mind.

Peter, do you have any comments on this patch?

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index db3622f22b61..37301516f4e6 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1720,6 +1720,23 @@ config EFI_MIXED
>  
>  	   If unsure, say N.
>  
> +if X86_64
> +config EFI_INTERRUPTIBLE
> +	bool "Interruptible Efi Runtime Services"
> +	depends on EFI && !EFI_VARS_PSTORE
> +	default n
> +	---help---
> +	  Only use this if you really know what you are doing, you
> +	  possibly risk to break your BIOS.
> +
> +	  This is an interruptible implementation of efi runtime
> +	  services wrappers. If you say Y, BIOS code could be
> +	  interrupted and preempted as a standard process. Enable this
> +	  only if you are sure that your BIOS will support
> +	  interruptible efi calls
> +	  In doubt, say "N".
> +endif
> +
>  config SECCOMP
>  	def_bool y
>  	prompt "Enable seccomp to safely compute untrusted bytecode"
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 8fd9e637629a..5b94d9dd9409 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -137,6 +137,7 @@ extern void __init efi_map_region(efi_memory_desc_t *md);
>  extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
>  extern void efi_sync_low_kernel_mappings(void);
>  extern int __init efi_alloc_page_tables(void);
> +extern pgd_t *efi_get_pgd(void);
>  extern int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
>  extern void __init old_map_region(efi_memory_desc_t *md);
> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
> index 2846aaab5103..ff57d3840842 100644
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
>  obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>  obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
>  obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
> +obj-$(CONFIG_EFI_INTERRUPTIBLE)	+= efi_interruptible.o
> diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
> index 58d669bc8250..f2403e16a8bb 100644
> --- a/arch/x86/platform/efi/efi_32.c
> +++ b/arch/x86/platform/efi/efi_32.c
> @@ -61,6 +61,11 @@ void __init efi_map_region(efi_memory_desc_t *md)
>  void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
>  void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return initial_page_table;
> +}
> +
>  pgd_t * __init efi_call_phys_prolog(void)
>  {
>  	struct desc_ptr gdt_descr;

Shouldn't be necessary. You can't build this for 32-bit because of the
Kconfig magic.

> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index b492521503fe..7886c7527cdf 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -212,6 +212,11 @@ void efi_sync_low_kernel_mappings(void)
>  	memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
>  }
>  
> +pgd_t *efi_get_pgd(void)
> +{
> +	return __va(efi_scratch.efi_pgt);
> +}
> +

You could make this easier and just return 'efi_pgd', since we now
have entirely separate EFI page tables.

> +static struct efivars interruptible_efivars;
> +
> +static int efi_interruptible_panic_notifier_call(
> +	struct notifier_block *notifier,
> +	unsigned long what, void *data)
> +{
> +	in_panic = true;
> +	return NOTIFY_DONE;
> +}

Please make the panic notifier changes a separate patch.

> +static struct notifier_block panic_nb = {
> +	.notifier_call = efi_interruptible_panic_notifier_call,
> +	.priority = 100,
> +};
> +
> +static struct task_struct *efi_kworker_task;
> +static struct efivar_operations interruptible_ops;
> +static __init int efi_interruptible_init(void)
> +{
> +	int ret;
> +
> +	efi_kworker_task = kthread_create(kthread_worker_fn, &efi_kworker,
> +					  "efi_kthread");
> +	if (IS_ERR(efi_kworker_task)) {
> +		pr_err("efi_interruptible: Failed to create kthread\n");
> +		ret = PTR_ERR(efi_kworker_task);
> +		efi_kworker_task = NULL;
> +		return ret;
> +	}
> +
> +	efi_kworker_task->mm = mm_alloc();
> +	efi_kworker_task->active_mm = efi_kworker_task->mm;
> +	efi_kworker_task->mm->pgd = efi_get_pgd();
> +	wake_up_process(efi_kworker_task);

Did you have any luck binding this driver to the other
Broxton-specific drivers?

Additionally, you're going to want to make sure that
efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
otherwise you can't use the new EFI page table scheme, e.g. if someone
boots with efi=old_map.

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-02-11 15:19 GMT+01:00 Matt Fleming <matt@codeblueprint.co.uk>:
> Did you have any luck binding this driver to the other
> Broxton-specific drivers?
>
> Additionally, you're going to want to make sure that
> efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
> otherwise you can't use the new EFI page table scheme, e.g. if someone
> boots with efi=old_map.

I didn't have very much time to work on this but yes, we should be
able to make efi_interruptible a platform driver which would be probed
by SPD driver or something like that.
I'll upload it as soon as I've something

-- 
Sylvain

Re: [PATCH v3 3/3] efi: implement interruptible runtime services

[Sylvain Chouleur]

2016-02-11 15:19 GMT+01:00 Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>:
> Did you have any luck binding this driver to the other
> Broxton-specific drivers?
>
> Additionally, you're going to want to make sure that
> efi_enabled(EFI_OLD_MEMMAP) is false in efi_interruptible_init(),
> otherwise you can't use the new EFI page table scheme, e.g. if someone
> boots with efi=old_map.

I didn't have very much time to work on this but yes, we should be
able to make efi_interruptible a platform driver which would be probed
by SPD driver or something like that.
I'll upload it as soon as I've something

-- 
Sylvain

[PATCH v2 0/3] efi interruptible runtime services

[Ard Biesheuvel]

This is a resend of the generic part of the series 'efi interruptible runtime
services', of which v3 was sent out on January 13th. I have added an additional
patch on top which gives runtime-wrappers.c the same treatment as efivars.c,
i.e., replacing the spinlock with a semaphore.

v2: rebase onto v4.7-rc3
    pr_warn() on failure to acquire the rt services semaphore before invoking
    the reset_system runtime service
    map EFI_ABORTED onto -EINTR

Ard Biesheuvel (1):
  efi: replace runtime services spinlock with semaphore

Sylvain Chouleur (2):
  efi: use a file local lock for efivars
  efi: don't use spinlocks for efi vars

 drivers/firmware/efi/efi-pstore.c       |  36 +++--
 drivers/firmware/efi/efi.c              |   3 +
 drivers/firmware/efi/efivars.c          |  22 ++-
 drivers/firmware/efi/runtime-wrappers.c |  81 ++++++-----
 drivers/firmware/efi/vars.c             | 142 +++++++++++++-------
 fs/efivarfs/inode.c                     |   5 +-
 fs/efivarfs/super.c                     |   9 +-
 include/linux/efi.h                     |  13 +-
 8 files changed, 204 insertions(+), 107 deletions(-)

-- 
1.9.1