From: Matt Fleming <[email protected]>
These two patches should address the concerns raised by Ingo and Peter
in relation to the EFI pull request containing v3.18 material,
https://lkml.kernel.org/r/[email protected]
We can drop the in_nmi() checks altogether and just provide normal
locking semantics if we introduce a non-blocking SetVariable() operation
for the particular case of writing pstore data to the EFI backend from
the kdump callback, which aborts in the contended case.
This is currently holding up merging of the v3.18 EFI patches, so please
be timely with comments, if any.
@Linaro guys, I don't think you'll actually care all that much about
these changes since the in_nmi() goo was for x86's benefit, but I'm
Cc'ing you anyway to make sure everything looks OK.
Matt Fleming (2):
efi: Provide a non-blocking SetVariable() operation
efi: Delete the in_nmi() conditional runtime locking
arch/x86/include/asm/efi.h | 2 --
drivers/firmware/efi/runtime-wrappers.c | 36 ++++++++++++++++---------
drivers/firmware/efi/vars.c | 47 +++++++++++++++++++++++++++++++++
include/linux/efi.h | 6 +++++
4 files changed, 76 insertions(+), 15 deletions(-)
--
1.9.3
From: Matt Fleming <[email protected]>
There are some circumstances that call for trying to write an EFI
variable in a non-blocking way. One such scenario is when writing pstore
data in efi_pstore_write() via the pstore_dump() kdump callback.
Now that we have an EFI runtime spinlock we need a way of aborting if
there is contention instead of spinning, since when writing pstore data
from the kdump callback, the runtime lock may already be held by the CPU
that's running the callback if we crashed in the middle of an EFI
variable operation.
The situation is sufficiently special that a new EFI variable operation
is warranted.
Introduce ->set_variable_nonblocking() for this use case. It is an
optional EFI backend operation, and need only be implemented by those
backends that usually acquire locks to serialize access to EFI
variables, as is the case for virt_efi_set_variable() where we now grab
the EFI runtime spinlock.
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Matthew Garrett <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
drivers/firmware/efi/runtime-wrappers.c | 19 +++++++++++++
drivers/firmware/efi/vars.c | 47 +++++++++++++++++++++++++++++++++
include/linux/efi.h | 6 +++++
3 files changed, 72 insertions(+)
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 9694cba665c4..4349206198b2 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -200,6 +200,24 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
return status;
}
+static efi_status_t
+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))
+ return EFI_NOT_READY;
+
+ status = efi_call_virt(set_variable, name, vendor, attr, data_size,
+ data);
+ spin_unlock_irqrestore(&efi_runtime_lock, flags);
+ return status;
+}
+
+
static efi_status_t virt_efi_query_variable_info(u32 attr,
u64 *storage_space,
u64 *remaining_space,
@@ -287,6 +305,7 @@ void efi_native_runtime_setup(void)
efi.get_variable = virt_efi_get_variable;
efi.get_next_variable = virt_efi_get_next_variable;
efi.set_variable = virt_efi_set_variable;
+ efi.set_variable_nonblocking = virt_efi_set_variable_nonblocking;
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;
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 1fa724f31b0e..fa3c66bdc1e5 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -595,6 +595,39 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
}
EXPORT_SYMBOL_GPL(efivar_entry_set);
+/*
+ * efivar_entry_set_nonblocking - call set_variable_nonblocking()
+ *
+ * This function is guaranteed to not block and is suitable for calling
+ * from crash/panic handlers.
+ *
+ * Crucially, this function will not block if it cannot acquire
+ * __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))
+ return -EBUSY;
+
+ status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
+ if (status != EFI_SUCCESS) {
+ spin_unlock_irqrestore(&__efivars->lock, flags);
+ return -ENOSPC;
+ }
+
+ status = ops->set_variable_nonblocking(name, &vendor, attributes,
+ size, data);
+
+ spin_unlock_irqrestore(&__efivars->lock, flags);
+ return efi_status_to_err(status);
+}
+
/**
* efivar_entry_set_safe - call set_variable() if enough space in firmware
* @name: buffer containing the variable name
@@ -622,6 +655,20 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
if (!ops->query_variable_store)
return -ENOSYS;
+ /*
+ * If the EFI variable backend provides a non-blocking
+ * ->set_variable() operation and we're in a context where we
+ * cannot block, then we need to use it to avoid live-locks,
+ * since the implication is that the regular ->set_variable()
+ * will block.
+ *
+ * If no ->set_variable_nonblocking() is provided then
+ * ->set_variable() is assumed to be non-blocking.
+ */
+ if (!block && ops->set_variable_nonblocking)
+ return efivar_entry_set_nonblocking(name, vendor, attributes,
+ size, data);
+
if (!block) {
if (!spin_trylock_irqsave(&__efivars->lock, flags))
return -EBUSY;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6e84881d146f..1c70cf5e3301 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -503,6 +503,10 @@ 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);
@@ -822,6 +826,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_query_variable_info_t *query_variable_info;
efi_update_capsule_t *update_capsule;
efi_query_capsule_caps_t *query_capsule_caps;
@@ -1042,6 +1047,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_query_variable_store_t *query_variable_store;
};
--
1.9.3
From: Matt Fleming <[email protected]>
commit 5dc3826d9f08 ("efi: Implement mandatory locking for UEFI Runtime
Services") implemented some conditional locking when accessing variable
runtime services that Ingo described as "pretty disgusting".
The intention with the !efi_in_nmi() checks was to avoid live-locks when
trying to write pstore crash data into an EFI variable. Such lockless
accesses are allowed according to the UEFI specification when we're in a
"non-recoverable" state, but whether or not things are implemented
correctly in actual firmware implementations remains an unanswered
question, and so it would seem sensible to avoid doing any kind of
unsynchronized variable accesses.
Furthermore, the efi_in_nmi() tests are inadequate because they don't
account for the case where we call EFI variable services from panic or
oops callbacks and aren't executing in NMI context. In other words,
live-locking is still possible.
Let's just remove the conditional locking altogether. Now we've got the
->set_variable_nonblocking() EFI variable operation we can abort if the
runtime lock is already held. Aborting is by far the safest option.
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Matthew Garrett <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/include/asm/efi.h | 2 --
drivers/firmware/efi/runtime-wrappers.c | 17 ++++-------------
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index abba1fe1db5e..408ad6d3222e 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -86,8 +86,6 @@ extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
#endif /* CONFIG_X86_32 */
-#define efi_in_nmi() in_nmi()
-
extern struct efi_scratch efi_scratch;
extern void __init efi_set_executable(efi_memory_desc_t *md, bool executable);
extern int __init efi_memblock_x86_reserve_range(void);
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 4349206198b2..228bbf910461 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -86,9 +86,6 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
* for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
* context through efi_pstore_write().
*/
-#ifndef efi_in_nmi
-#define efi_in_nmi() (0)
-#endif
/*
* As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
@@ -189,14 +186,11 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
{
unsigned long flags;
efi_status_t status;
- bool __in_nmi = efi_in_nmi();
- if (!__in_nmi)
- spin_lock_irqsave(&efi_runtime_lock, flags);
+ spin_lock_irqsave(&efi_runtime_lock, flags);
status = efi_call_virt(set_variable, name, vendor, attr, data_size,
data);
- if (!__in_nmi)
- spin_unlock_irqrestore(&efi_runtime_lock, flags);
+ spin_unlock_irqrestore(&efi_runtime_lock, flags);
return status;
}
@@ -225,17 +219,14 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
{
unsigned long flags;
efi_status_t status;
- bool __in_nmi = efi_in_nmi();
if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
return EFI_UNSUPPORTED;
- if (!__in_nmi)
- spin_lock_irqsave(&efi_runtime_lock, flags);
+ spin_lock_irqsave(&efi_runtime_lock, flags);
status = efi_call_virt(query_variable_info, attr, storage_space,
remaining_space, max_variable_size);
- if (!__in_nmi)
- spin_unlock_irqrestore(&efi_runtime_lock, flags);
+ spin_unlock_irqrestore(&efi_runtime_lock, flags);
return status;
}
--
1.9.3
On Wed, Oct 01, 2014 at 01:06:39PM +0100, Matt Fleming wrote:
> From: Matt Fleming <[email protected]>
>
> There are some circumstances that call for trying to write an EFI
> variable in a non-blocking way. One such scenario is when writing pstore
> data in efi_pstore_write() via the pstore_dump() kdump callback.
>
> Now that we have an EFI runtime spinlock we need a way of aborting if
> there is contention instead of spinning, since when writing pstore data
> from the kdump callback, the runtime lock may already be held by the CPU
> that's running the callback if we crashed in the middle of an EFI
> variable operation.
>
> The situation is sufficiently special that a new EFI variable operation
> is warranted.
>
> Introduce ->set_variable_nonblocking() for this use case. It is an
> optional EFI backend operation, and need only be implemented by those
> backends that usually acquire locks to serialize access to EFI
> variables, as is the case for virt_efi_set_variable() where we now grab
> the EFI runtime spinlock.
>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> drivers/firmware/efi/runtime-wrappers.c | 19 +++++++++++++
> drivers/firmware/efi/vars.c | 47 +++++++++++++++++++++++++++++++++
> include/linux/efi.h | 6 +++++
> 3 files changed, 72 insertions(+)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 9694cba665c4..4349206198b2 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -200,6 +200,24 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
> return status;
> }
>
> +static efi_status_t
> +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))
> + return EFI_NOT_READY;
> +
> + status = efi_call_virt(set_variable, name, vendor, attr, data_size,
> + data);
> + spin_unlock_irqrestore(&efi_runtime_lock, flags);
> + return status;
> +}
If you want to have this usable from NMI context, you need to convert
efi_runtime_lock to a raw_spinlock_t.
Also, it would probably be a good idea to have some selftest like thing
that actually calls this from NMI context, right?
On Wed, 01 Oct, at 05:26:24PM, Peter Zijlstra wrote:
>
> If you want to have this usable from NMI context, you need to convert
> efi_runtime_lock to a raw_spinlock_t.
Hmm.. I note that none of the spinlocks in the pstore code path that we
execute to get here are raw_spinlock_t. And that's the only code path
that calls this function.
They need to be raw_spinlock_t for -rt?
> Also, it would probably be a good idea to have some selftest like thing
> that actually calls this from NMI context, right?
Yeah probably, although that'd need to be a core pstore test since
there's nothing really interesting going on in
virt_efi_set_variable_nonblocking(), and it would be good to test it out
in the larger pstore context.
--
Matt Fleming, Intel Open Source Technology Center