[PATCH v3 0/7] efi: run UEFI services with interrupts enabled

[PATCH v3 0/7] efi: run UEFI services with interrupts enabled

[ard]

The UEFI spec does not require interrupts to be disabled when invoking
runtime services. The reason we have been doing so is because EFI pstore
may call SetVariable() from interrupt context, which may result in deadlock
if another runtime services call was in progress on the same cpu.

The EFI pstore has already been updated to use a non-blocking path and fail
gracefully rather than spin forever, so we can update the ordinary blocking
wrappers to run with interrupts enabled instead. This aims to prevent excessive
interrupt latencies on uniprocessor platforms with slow variable stores.

Changes since v2:
- folded Matt's suggestion for patch #4, to bail rather than try to trigger a
  garbage collection in the nonblocking case when there is insufficient space
- rebased onto v4.4-rc5

Changes since v1:
- added nonblocking QueryVariableInfo() wrapper variant, and updated
  efi_query_variable_store() to use it when called in nonblocking context
- add patch to drop the rtc_lock spinlock
- add patch to drop redundant efi_set_variable_nonblocking_t typedef
- drop BUG_ONs in patch #7

Ard Biesheuvel (7):
  efi: expose non-blocking set_variable() wrapper to efivars
  efi: remove redundant efi_set_variable_nonblocking prototype
  efi: runtime-wrappers: add a nonblocking version of QueryVariableInfo
  efi: add nonblocking option to efi_query_variable_store()
  efi: runtime-wrappers: remove out of date comment regarding in_nmi()
  efi: runtime-wrapper: get rid of the rtc_lock spinlock
  efi: runtime-wrappers: run UEFI Runtime Services with interrupts
    enabled

 arch/x86/platform/efi/quirks.c          |  33 +++++-
 drivers/firmware/efi/efi.c              |   1 +
 drivers/firmware/efi/runtime-wrappers.c | 115 +++++++-------------
 drivers/firmware/efi/vars.c             |  16 ++-
 include/linux/efi.h                     |  21 ++--
 5 files changed, 100 insertions(+), 86 deletions(-)

-- 
2.5.0

[PATCH v3 1/7] efi: expose non-blocking set_variable() wrapper to efivars

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Commit 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable()
operation") implemented a non-blocking alternative for the UEFI
SetVariable() invocation performed by efivars, since it may occur
in atomic context. However, this version of the function was never
exposed via the efivars struct, so the non-blocking versions was
not actually callable. Fix that.

Fixes: 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 027ca212179f..3b52677f459a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -180,6 +180,7 @@ static int generic_ops_register(void)
 {
 	generic_ops.get_variable = efi.get_variable;
 	generic_ops.set_variable = efi.set_variable;
+	generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
 	generic_ops.get_next_variable = efi.get_next_variable;
 	generic_ops.query_variable_store = efi_query_variable_store;
 
-- 
2.5.0

[PATCH v3 2/7] efi: remove redundant efi_set_variable_nonblocking prototype

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

There is no need for a separate nonblocking prototype definition
for the SetVariable() UEFI Runtime Service, since it is identical
to the blocking version.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 include/linux/efi.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a866bb1..8706e0aabedc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -507,10 +507,6 @@ typedef efi_status_t efi_get_next_variable_t (unsigned long *name_size, efi_char
 typedef efi_status_t efi_set_variable_t (efi_char16_t *name, efi_guid_t *vendor, 
 					 u32 attr, unsigned long data_size,
 					 void *data);
-typedef efi_status_t
-efi_set_variable_nonblocking_t(efi_char16_t *name, efi_guid_t *vendor,
-			       u32 attr, unsigned long data_size, void *data);
-
 typedef efi_status_t efi_get_next_high_mono_count_t (u32 *count);
 typedef void efi_reset_system_t (int reset_type, efi_status_t status,
 				 unsigned long data_size, efi_char16_t *data);
@@ -851,7 +847,7 @@ extern struct efi {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
@@ -1091,7 +1087,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_store_t *query_variable_store;
 };
 
-- 
2.5.0

[PATCH v3 3/7] efi: runtime-wrappers: add a nonblocking version of QueryVariableInfo

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This introduces a new runtime wrapper for the QueryVariableInfo() UEFI
Runtime Service, which gives up immediately rather than spins on failure
to grab the efi_runtime spinlock.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++
 include/linux/efi.h                     |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 228bbf910461..e9f2867f0d91 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -230,6 +230,27 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 	return status;
 }
 
+static efi_status_t
+virt_efi_query_variable_info_nonblocking(u32 attr,
+					 u64 *storage_space,
+					 u64 *remaining_space,
+					 u64 *max_variable_size)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+		return EFI_NOT_READY;
+
+	status = efi_call_virt(query_variable_info, attr, storage_space,
+			       remaining_space, max_variable_size);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	return status;
+}
+
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
 	unsigned long flags;
@@ -300,6 +321,7 @@ void efi_native_runtime_setup(void)
 	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
 	efi.reset_system = virt_efi_reset_system;
 	efi.query_variable_info = virt_efi_query_variable_info;
+	efi.query_variable_info_nonblocking = virt_efi_query_variable_info_nonblocking;
 	efi.update_capsule = virt_efi_update_capsule;
 	efi.query_capsule_caps = virt_efi_query_capsule_caps;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8706e0aabedc..ad1e177ba48e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -849,6 +849,7 @@ extern struct efi {
 	efi_set_variable_t *set_variable;
 	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
+	efi_query_variable_info_t *query_variable_info_nonblocking;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
 	efi_get_next_high_mono_count_t *get_next_high_mono_count;
-- 
2.5.0

Re: [PATCH v3 3/7] efi: runtime-wrappers: add a nonblocking version of QueryVariableInfo

[Matt Fleming]

On Mon, 14 Dec, at 11:40:27AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> This introduces a new runtime wrapper for the QueryVariableInfo() UEFI
> Runtime Service, which gives up immediately rather than spins on failure
> to grab the efi_runtime spinlock.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++
>  include/linux/efi.h                     |  1 +
>  2 files changed, 23 insertions(+)

Thanks Ard, applied with this additional piece in the commit log, 

  This is required in the non-blocking path of the efi-pstore code.

so that we don't have to wonder in 6 months why we introduced this
wrapper.

[PATCH v3 4/7] efi: add nonblocking option to efi_query_variable_store()

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

The function efi_query_variable_store() may be invoked by
efivar_entry_set_nonblocking(), which itself takes care to only call
a non-blocking version of the SetVariable() runtime wrapper. However,
efi_query_variable_store() may call the SetVariable() wrapper directly,
as well as the wrapper for QueryVariableInfo(), both of which could
deadlock in the same way we are trying to prevent by calling
efivar_entry_set_nonblocking() in the first place.

So instead, modify efi_query_variable_store() to use the non-blocking
variants of QueryVariableInfo() (and give up rather than free up space
if the available space is below EFI_MIN_RESERVE) if invoked with the
'nonblocking' argument set to true.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/x86/platform/efi/quirks.c | 33 +++++++++++++++++++-
 drivers/firmware/efi/vars.c    | 16 ++++++++--
 include/linux/efi.h            | 12 +++++--
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 1c7380da65ff..261808c21a84 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -54,13 +54,41 @@ void efi_delete_dummy_variable(void)
 }
 
 /*
+ * In the nonblocking case we do not attempt to perform garbage
+ * collection if we do not have enough free space. Rather, we do the
+ * bare minimum check and give up immediately if the available space
+ * is below EFI_MIN_RESERVE.
+ *
+ * This function is intended to be small and simple because it is
+ * invoked from crash handler paths.
+ */
+static efi_status_t query_variable_store_nonblocking(u32 attributes,
+						     unsigned long size)
+{
+	efi_status_t status;
+	u64 storage_size, remaining_size, max_size;
+
+	status = efi.query_variable_info_nonblocking(attributes, &storage_size
+						     &remaining_size,
+						     &max_size);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	if (remaining_size - size < EFI_MIN_RESERVE)
+		return EFI_OUT_OF_RESOURCES;
+
+	return EFI_SUCCESS;
+}
+
+/*
  * Some firmware implementations refuse to boot if there's insufficient space
  * in the variable store. Ensure that we never use more than a safe limit.
  *
  * Return EFI_SUCCESS if it is safe to write 'size' bytes to the variable
  * store.
  */
-efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
+				      bool nonblocking)
 {
 	efi_status_t status;
 	u64 storage_size, remaining_size, max_size;
@@ -68,6 +96,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
 	if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
 		return 0;
 
+	if (nonblocking)
+		return query_variable_store_nonblocking(attributes, size);
+
 	status = efi.query_variable_info(attributes, &storage_size,
 					 &remaining_size, &max_size);
 	if (status != EFI_SUCCESS)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb10517f..d2a49626a335 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -234,7 +234,18 @@ check_var_size(u32 attributes, unsigned long size)
 	if (!fops->query_variable_store)
 		return EFI_UNSUPPORTED;
 
-	return fops->query_variable_store(attributes, size);
+	return fops->query_variable_store(attributes, size, false);
+}
+
+static efi_status_t
+check_var_size_nonblocking(u32 attributes, unsigned long size)
+{
+	const struct efivar_operations *fops = __efivars->ops;
+
+	if (!fops->query_variable_store)
+		return EFI_UNSUPPORTED;
+
+	return fops->query_variable_store(attributes, size, true);
 }
 
 static int efi_status_to_err(efi_status_t status)
@@ -615,7 +626,8 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 	if (!spin_trylock_irqsave(&__efivars->lock, flags))
 		return -EBUSY;
 
-	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
+	status = check_var_size_nonblocking(attributes,
+					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
 		spin_unlock_irqrestore(&__efivars->lock, flags);
 		return -ENOSPC;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ad1e177ba48e..09f1559e7525 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -525,7 +525,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
 					      unsigned long count,
 					      u64 *max_size,
 					      int *reset_type);
-typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size);
+typedef efi_status_t efi_query_variable_store_t(u32 attributes,
+						unsigned long size,
+						bool nonblocking);
 
 void efi_native_runtime_setup(void);
 
@@ -881,13 +883,17 @@ extern void efi_enter_virtual_mode (void);	/* switch EFI to virtual mode, if pos
 #ifdef CONFIG_X86
 extern void efi_late_init(void);
 extern void efi_free_boot_services(void);
-extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size);
+extern efi_status_t efi_query_variable_store(u32 attributes,
+					     unsigned long size,
+					     bool nonblocking);
 extern void efi_find_mirror(void);
 #else
 static inline void efi_late_init(void) {}
 static inline void efi_free_boot_services(void) {}
 
-static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+static inline efi_status_t efi_query_variable_store(u32 attributes,
+						    unsigned long size,
+						    bool nonblocking)
 {
 	return EFI_SUCCESS;
 }
-- 
2.5.0

[PATCH v3 5/7] efi: runtime-wrappers: remove out of date comment regarding in_nmi()

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This code is long gone, so remove the comment as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 26 --------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index e9f2867f0d91..311f415bff51 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -62,32 +62,6 @@
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
 /*
- * Some runtime services calls can be reentrant under NMI, even if the table
- * above says they are not. (source: UEFI Specification v2.4A)
- *
- * Table 32. Functions that may be called after Machine Check, INIT and NMI
- * +----------------------------+------------------------------------------+
- * | Function			| Called after Machine Check, INIT and NMI |
- * +----------------------------+------------------------------------------+
- * | GetTime()			| Yes, even if previously busy.		   |
- * | GetVariable()		| Yes, even if previously busy		   |
- * | GetNextVariableName()	| Yes, even if previously busy		   |
- * | QueryVariableInfo()	| Yes, even if previously busy		   |
- * | SetVariable()		| Yes, even if previously busy		   |
- * | UpdateCapsule()		| Yes, even if previously busy		   |
- * | QueryCapsuleCapabilities()	| Yes, even if previously busy		   |
- * | ResetSystem()		| Yes, even if previously busy		   |
- * +----------------------------+------------------------------------------+
- *
- * In order to prevent deadlocks under NMI, the wrappers for these functions
- * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
- * However, not all of the services listed are reachable through NMI code paths,
- * so the the special handling as suggested by the UEFI spec is only implemented
- * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
- * context through efi_pstore_write().
- */
-
-/*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
  * the EFI specification requires that callers of the time related runtime
  * functions serialize with other CMOS accesses in the kernel, as the EFI time
-- 
2.5.0

[PATCH v3 6/7] efi: runtime-wrapper: get rid of the rtc_lock spinlock

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

The rtc_lock spinlock aims to serialize access to the CMOS RTC between
the UEFI firmware and the kernel drivers that use it directly. However,
x86 is the only arch that performs such direct accesses, and that never
uses the time related UEFI runtime services. Since no other UEFI enlightened
architectures have a legcay CMOS RTC anyway, we can remove the rtc_lock
spinlock entirely.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 32 +++++---------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 311f415bff51..7b8b2f2702ca 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -61,24 +61,14 @@
  */
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
-/*
- * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
- * the EFI specification requires that callers of the time related runtime
- * functions serialize with other CMOS accesses in the kernel, as the EFI time
- * functions may choose to also use the legacy CMOS RTC.
- */
-__weak DEFINE_SPINLOCK(rtc_lock);
-
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -87,11 +77,9 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -102,11 +90,9 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -115,11 +101,9 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
-- 
2.5.0

[PATCH v3 7/7] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

[ard]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

The UEFI spec allows Runtime Services to be invoked with interrupts
enabled. The only reason we were disabling interrupts was to prevent
recursive calls into the services on the same CPU, which will lead to
deadlock. However, the only context where such invocations may occur
legally is from efi-pstore via efivars, and that code has been updated
to call a non-blocking alternative when invoked from a non-interruptible
context.

So instead, update the ordinary, blocking UEFI Runtime Services wrappers
to execute with interrupts enabled. This aims to prevent excessive interrupt
latencies on uniprocessor platforms with slow variable stores.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 73 ++++++++------------
 1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 7b8b2f2702ca..aa3d9d0a27e6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -63,23 +63,21 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_time(efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -87,23 +85,21 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 					     efi_bool_t *pending,
 					     efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -113,13 +109,12 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -127,12 +122,13 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_variable, name_size, name, vendor);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -142,13 +138,12 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -157,15 +152,14 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 				  u32 attr, unsigned long data_size,
 				  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -175,16 +169,15 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -194,29 +187,27 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 					 u64 *remaining_space,
 					 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_high_mono_count, count);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -225,26 +216,23 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 }
 
 static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 					    unsigned long count,
 					    unsigned long sg_list)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(update_capsule, capsules, count, sg_list);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -253,16 +241,15 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 						u64 *max_size,
 						int *reset_type)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
 			       reset_type);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
-- 
2.5.0

Re: [PATCH v3 0/7] efi: run UEFI services with interrupts enabled

[Matt Fleming]

On Mon, 14 Dec, at 11:40:24AM, Ard Biesheuvel wrote:
> The UEFI spec does not require interrupts to be disabled when invoking
> runtime services. The reason we have been doing so is because EFI pstore
> may call SetVariable() from interrupt context, which may result in deadlock
> if another runtime services call was in progress on the same cpu.

> The EFI pstore has already been updated to use a non-blocking path and fail
> gracefully rather than spin forever, so we can update the ordinary blocking
> wrappers to run with interrupts enabled instead. This aims to prevent excessive
> interrupt latencies on uniprocessor platforms with slow variable stores.
> 
> Changes since v2:
> - folded Matt's suggestion for patch #4, to bail rather than try to trigger a
>   garbage collection in the nonblocking case when there is insufficient space
> - rebased onto v4.4-rc5
> 
> Changes since v1:
> - added nonblocking QueryVariableInfo() wrapper variant, and updated
>   efi_query_variable_store() to use it when called in nonblocking context
> - add patch to drop the rtc_lock spinlock
> - add patch to drop redundant efi_set_variable_nonblocking_t typedef
> - drop BUG_ONs in patch #7
> 
> Ard Biesheuvel (7):
>   efi: expose non-blocking set_variable() wrapper to efivars
>   efi: remove redundant efi_set_variable_nonblocking prototype
>   efi: runtime-wrappers: add a nonblocking version of QueryVariableInfo
>   efi: add nonblocking option to efi_query_variable_store()
>   efi: runtime-wrappers: remove out of date comment regarding in_nmi()
>   efi: runtime-wrapper: get rid of the rtc_lock spinlock
>   efi: runtime-wrappers: run UEFI Runtime Services with interrupts
>     enabled
> 
>  arch/x86/platform/efi/quirks.c          |  33 +++++-
>  drivers/firmware/efi/efi.c              |   1 +
>  drivers/firmware/efi/runtime-wrappers.c | 115 +++++++-------------
>  drivers/firmware/efi/vars.c             |  16 ++-
>  include/linux/efi.h                     |  21 ++--
>  5 files changed, 100 insertions(+), 86 deletions(-)

Thanks Ard, this all looks good to me. I've queued this up. It's a
little late for v4.5 now, so this will probably be targeting v4.6.

Re: [PATCH v3 0/7] efi: run UEFI services with interrupts enabled

[Ard Biesheuvel]

On 18 December 2015 at 14:15, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Mon, 14 Dec, at 11:40:24AM, Ard Biesheuvel wrote:
>> The UEFI spec does not require interrupts to be disabled when invoking
>> runtime services. The reason we have been doing so is because EFI pstore
>> may call SetVariable() from interrupt context, which may result in deadlock
>> if another runtime services call was in progress on the same cpu.
>
>> The EFI pstore has already been updated to use a non-blocking path and fail
>> gracefully rather than spin forever, so we can update the ordinary blocking
>> wrappers to run with interrupts enabled instead. This aims to prevent excessive
>> interrupt latencies on uniprocessor platforms with slow variable stores.
>>
>> Changes since v2:
>> - folded Matt's suggestion for patch #4, to bail rather than try to trigger a
>>   garbage collection in the nonblocking case when there is insufficient space
>> - rebased onto v4.4-rc5
>>
>> Changes since v1:
>> - added nonblocking QueryVariableInfo() wrapper variant, and updated
>>   efi_query_variable_store() to use it when called in nonblocking context
>> - add patch to drop the rtc_lock spinlock
>> - add patch to drop redundant efi_set_variable_nonblocking_t typedef
>> - drop BUG_ONs in patch #7
>>
>> Ard Biesheuvel (7):
>>   efi: expose non-blocking set_variable() wrapper to efivars
>>   efi: remove redundant efi_set_variable_nonblocking prototype
>>   efi: runtime-wrappers: add a nonblocking version of QueryVariableInfo
>>   efi: add nonblocking option to efi_query_variable_store()
>>   efi: runtime-wrappers: remove out of date comment regarding in_nmi()
>>   efi: runtime-wrapper: get rid of the rtc_lock spinlock
>>   efi: runtime-wrappers: run UEFI Runtime Services with interrupts
>>     enabled
>>
>>  arch/x86/platform/efi/quirks.c          |  33 +++++-
>>  drivers/firmware/efi/efi.c              |   1 +
>>  drivers/firmware/efi/runtime-wrappers.c | 115 +++++++-------------
>>  drivers/firmware/efi/vars.c             |  16 ++-
>>  include/linux/efi.h                     |  21 ++--
>>  5 files changed, 100 insertions(+), 86 deletions(-)
>
> Thanks Ard, this all looks good to me. I've queued this up. It's a
> little late for v4.5 now, so this will probably be targeting v4.6.

Either is fine by me, as long as it is in the pipeline.

Thanks,
Ard.