| Message ID | 1405062556-14540-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 11 Jul, at 09:09:16AM, Ard Biesheuvel wrote: > According to section 7.1 of the UEFI spec, Runtime Services are not fully > reentrant, and there are particular combinations of calls that need to be > serialized. Use a spinlock to serialize all Runtime Services with respect > to all others, even if this is more than strictly needed. It'd be good to include a point about why we're only using a spinlock, namely that anything else introduces complication to code that is unlikely to be performance critical. [...] > + * > + * In order to prevent deadlocks under NMI, the wrappers for these functions > + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi(). > + */ > +#ifndef efi_in_nmi > +#define efi_in_nmi() (0) > +#endif It shouldn't be necessary to do the NMI checking for *all* runtime services, unless you use, for instance, the timer functions on ARM64. > +/* > * 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 > @@ -30,10 +95,17 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) > { > unsigned long flags; > efi_status_t status; > + bool __in_nmi = efi_in_nmi(); > > - spin_lock_irqsave(&rtc_lock, flags); > + if (!__in_nmi) { > + spin_lock_irqsave(&rtc_lock, flags); > + spin_lock(&efi_runtime_lock); > + } > status = efi_call_virt(get_time, tm, tc); > - spin_unlock_irqrestore(&rtc_lock, flags); > + if (!__in_nmi) { > + spin_unlock(&efi_runtime_lock); > + spin_unlock_irqrestore(&rtc_lock, flags); > + } > return status; > } > > @@ -42,8 +114,11 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) > unsigned long flags; > efi_status_t status; > > + BUG_ON(efi_in_nmi()); I think we can safely drop these BUG_ON()s. I would expect things to explode pretty spectacularly anyway, even without them if we're in NMI context. BUG_ON()s are usually reserved for "this is a fatal condition and we cannot make any forward progress at all, so stop". But also see my earlier point about how most of these functions aren't called from NMI context. > static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > efi_char16_t *name, > efi_guid_t *vendor) > { > - return efi_call_virt(get_next_variable, name_size, name, vendor); > + unsigned long flags; > + efi_status_t status; > + bool __in_nmi = efi_in_nmi(); > + > + if (!__in_nmi) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(get_next_variable, name_size, name, vendor); > + if (!__in_nmi) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; We shouldn't ever be in NMI context when calling get_next_variable(), right? > @@ -119,17 +245,33 @@ static void virt_efi_reset_system(int reset_type, > unsigned long data_size, > efi_char16_t *data) > { > + unsigned long flags; > + bool __in_nmi = efi_in_nmi(); > + > + if (!__in_nmi) > + spin_lock_irqsave(&efi_runtime_lock, flags); > __efi_call_virt(reset_system, reset_type, status, data_size, data); > + if (!__in_nmi) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > } Is it even possible to perform a reset through the usual machinery in NMI context? I don't think this is possible for x86. What about for arm64? > 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; > + bool __in_nmi = efi_in_nmi(); > + > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - return efi_call_virt(update_capsule, capsules, count, sg_list); > + if (!__in_nmi) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(update_capsule, capsules, count, sg_list); > + if (!__in_nmi) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } I don't think we need to check for being in NMI context here. > static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > @@ -137,11 +279,20 @@ 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; > + bool __in_nmi = efi_in_nmi(); > + > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - return efi_call_virt(query_capsule_caps, capsules, count, max_size, > - reset_type); > + if (!__in_nmi) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(query_capsule_caps, capsules, count, max_size, > + reset_type); > + if (!__in_nmi) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } Definitely should not be perfoming QueryCapsuleCapabilities() in NMI context.
On 4 August 2014 15:00, Matt Fleming <matt@console-pimps.org> wrote: > On Fri, 11 Jul, at 09:09:16AM, Ard Biesheuvel wrote: >> According to section 7.1 of the UEFI spec, Runtime Services are not fully >> reentrant, and there are particular combinations of calls that need to be >> serialized. Use a spinlock to serialize all Runtime Services with respect >> to all others, even if this is more than strictly needed. > > It'd be good to include a point about why we're only using a spinlock, > namely that anything else introduces complication to code that is > unlikely to be performance critical. > OK > [...] > >> + * >> + * In order to prevent deadlocks under NMI, the wrappers for these functions >> + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi(). >> + */ >> +#ifndef efi_in_nmi >> +#define efi_in_nmi() (0) >> +#endif > > It shouldn't be necessary to do the NMI checking for *all* runtime > services, unless you use, for instance, the timer functions on ARM64. > No, I only added the checks to functions that are listed in the UEFI spec as requiring the special treatment. Table 32 is copied right from the spec. (And if that is not obvious, I should definitely add a mention there) >> +/* >> * 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 >> @@ -30,10 +95,17 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) >> { >> unsigned long flags; >> efi_status_t status; >> + bool __in_nmi = efi_in_nmi(); >> >> - spin_lock_irqsave(&rtc_lock, flags); >> + if (!__in_nmi) { >> + spin_lock_irqsave(&rtc_lock, flags); >> + spin_lock(&efi_runtime_lock); >> + } >> status = efi_call_virt(get_time, tm, tc); >> - spin_unlock_irqrestore(&rtc_lock, flags); >> + if (!__in_nmi) { >> + spin_unlock(&efi_runtime_lock); >> + spin_unlock_irqrestore(&rtc_lock, flags); >> + } >> return status; >> } >> >> @@ -42,8 +114,11 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) >> unsigned long flags; >> efi_status_t status; >> >> + BUG_ON(efi_in_nmi()); > > I think we can safely drop these BUG_ON()s. I would expect things to > explode pretty spectacularly anyway, even without them if we're in NMI > context. BUG_ON()s are usually reserved for "this is a fatal condition > and we cannot make any forward progress at all, so stop". > > But also see my earlier point about how most of these functions aren't > called from NMI context. > I added the BUG_ON() to functions not listed in the table, and added the treatment to functions that are listed in the table. Happy to drop the BUG_ON()'s, though. >> static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, >> efi_char16_t *name, >> efi_guid_t *vendor) >> { >> - return efi_call_virt(get_next_variable, name_size, name, vendor); >> + unsigned long flags; >> + efi_status_t status; >> + bool __in_nmi = efi_in_nmi(); >> + >> + if (!__in_nmi) >> + spin_lock_irqsave(&efi_runtime_lock, flags); >> + status = efi_call_virt(get_next_variable, name_size, name, vendor); >> + if (!__in_nmi) >> + spin_unlock_irqrestore(&efi_runtime_lock, flags); >> + return status; > > We shouldn't ever be in NMI context when calling get_next_variable(), > right? > Spec allows it ... >> @@ -119,17 +245,33 @@ static void virt_efi_reset_system(int reset_type, >> unsigned long data_size, >> efi_char16_t *data) >> { >> + unsigned long flags; >> + bool __in_nmi = efi_in_nmi(); >> + >> + if (!__in_nmi) >> + spin_lock_irqsave(&efi_runtime_lock, flags); >> __efi_call_virt(reset_system, reset_type, status, data_size, data); >> + if (!__in_nmi) >> + spin_unlock_irqrestore(&efi_runtime_lock, flags); >> } > > Is it even possible to perform a reset through the usual machinery in > NMI context? I don't think this is possible for x86. What about for > arm64? > We don't have NMI so all the in_nmi() macros evaluate to false anyway. >> 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; >> + bool __in_nmi = efi_in_nmi(); >> + >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - return efi_call_virt(update_capsule, capsules, count, sg_list); >> + if (!__in_nmi) >> + spin_lock_irqsave(&efi_runtime_lock, flags); >> + status = efi_call_virt(update_capsule, capsules, count, sg_list); >> + if (!__in_nmi) >> + spin_unlock_irqrestore(&efi_runtime_lock, flags); >> + return status; >> } > > I don't think we need to check for being in NMI context here. > Likewise, the spec says different ... >> static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, >> @@ -137,11 +279,20 @@ 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; >> + bool __in_nmi = efi_in_nmi(); >> + >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) >> return EFI_UNSUPPORTED; >> >> - return efi_call_virt(query_capsule_caps, capsules, count, max_size, >> - reset_type); >> + if (!__in_nmi) >> + spin_lock_irqsave(&efi_runtime_lock, flags); >> + status = efi_call_virt(query_capsule_caps, capsules, count, max_size, >> + reset_type); >> + if (!__in_nmi) >> + spin_unlock_irqrestore(&efi_runtime_lock, flags); >> + return status; >> } > > Definitely should not be perfoming QueryCapsuleCapabilities() in NMI > context. > Well, again, the spec allows it. But I am happy to remove it as it does not affect ARM anyway Regards, Ard.
On Mon, 04 Aug, at 03:13:28PM, Ard Biesheuvel wrote: > > Well, again, the spec allows it. But I am happy to remove it as it > does not affect ARM anyway Right, I understand why you added these now. My personal opinion is that we shouldn't do the NMI dancing unless absolutely necessary, e.g. because of corresponding kernel code paths. The fact that the spec allows it doesn't necessarily mean we should support it. But I do like the idea of documenting that the spec allows for things that we don't support, because that at least informs developers, when they come snooping around this file, that they've got some additional work to do if they want to call these functions from NMI context. So the table you included is cool, and I think some additional sentences along the lines of "... but we don't support calling all these functions from NMI context" would be a good addition. What do you think?
On 4 August 2014 16:49, Matt Fleming <matt@console-pimps.org> wrote: > On Mon, 04 Aug, at 03:13:28PM, Ard Biesheuvel wrote: >> >> Well, again, the spec allows it. But I am happy to remove it as it >> does not affect ARM anyway > > Right, I understand why you added these now. > > My personal opinion is that we shouldn't do the NMI dancing unless > absolutely necessary, e.g. because of corresponding kernel code paths. > The fact that the spec allows it doesn't necessarily mean we should > support it. > I agree. But for me, it is not obvious which functions can or cannot be called from NMI context on x86, so I played it safe by just covering everything that the spec allows, the idea being that if other uses exists, they violate the spec anyway and handling NMI in those cases may well break other stuff. > But I do like the idea of documenting that the spec allows for things > that we don't support, because that at least informs developers, when > they come snooping around this file, that they've got some additional > work to do if they want to call these functions from NMI context. > > So the table you included is cool, and I think some additional sentences > along the lines of "... but we don't support calling all these functions > from NMI context" would be a good addition. > > What do you think? > I think that makes sense. As I said, I don't have a strong preference either way regarding the NMI handling, as it does not affect the systems I am primarily concerned with (and it sounds like a big hack anyway). What I /am/ concerned with is not getting code into the kernel that turns out to be non-compliant a couple of months down the road and having to fix it urgently then. So other than GetVariable and SetVariable, or there any other services that need the NMI treatment?
On Mon, 04 Aug, at 05:05:53PM, Ard Biesheuvel wrote: > > I think that makes sense. As I said, I don't have a strong preference > either way regarding the NMI handling, as it does not affect the > systems I am primarily concerned with (and it sounds like a big hack > anyway). What I /am/ concerned with is not getting code into the > kernel that turns out to be non-compliant a couple of months down the > road and having to fix it urgently then. Right, that's a valid concern. > So other than GetVariable and SetVariable, or there any other services > that need the NMI treatment? The one and only (potential) NMI-context caller of EFI runtime services is efi_pstore_write(), which calls (as part of efivar_entry_set_safe()) QueryVariableInfo() and SetVariable().
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..35f67a253e33 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -86,6 +86,8 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size, #endif /* CONFIG_X86_32 */ +#define efi_in_nmi() in_nmi() + extern int add_efi_memmap; extern struct efi_scratch efi_scratch; extern void efi_set_executable(efi_memory_desc_t *md, bool executable); diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..55ac6d7eac1b 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -14,11 +14,76 @@ * This file is released under the GPLv2. */ +#include <linux/bug.h> #include <linux/efi.h> -#include <linux/spinlock.h> /* spinlock_t */ +#include <linux/mutex.h> +#include <linux/spinlock.h> #include <asm/efi.h> /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * +------------------------------------+-------------------------------+ + * | If previous call is busy in | Forbidden to call | + * +------------------------------------+-------------------------------+ + * | Any | SetVirtualAddressMap() | + * +------------------------------------+-------------------------------+ + * | ConvertPointer() | ConvertPointer() | + * +------------------------------------+-------------------------------+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * +------------------------------------+-------------------------------+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities() | QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * +------------------------------------+-------------------------------+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * +------------------------------------+-------------------------------+ + * + * Instead of adding locks for each of the groups, we add a single spinlock + * that serializes all runtime services calls with respect to all others. + */ +static DEFINE_SPINLOCK(efi_runtime_lock); + +/* + * Some runtime services calls can be reentrant under NMI, even if the table + * above says they are not. + * + * 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 + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi(). + */ +#ifndef efi_in_nmi +#define efi_in_nmi() (0) +#endif + +/* * 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 @@ -30,10 +95,17 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc) { unsigned long flags; efi_status_t status; + bool __in_nmi = efi_in_nmi(); - spin_lock_irqsave(&rtc_lock, flags); + if (!__in_nmi) { + spin_lock_irqsave(&rtc_lock, flags); + spin_lock(&efi_runtime_lock); + } status = efi_call_virt(get_time, tm, tc); - spin_unlock_irqrestore(&rtc_lock, flags); + if (!__in_nmi) { + spin_unlock(&efi_runtime_lock); + spin_unlock_irqrestore(&rtc_lock, flags); + } return status; } @@ -42,8 +114,11 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) unsigned long flags; efi_status_t status; + BUG_ON(efi_in_nmi()); spin_lock_irqsave(&rtc_lock, flags); + spin_lock(&efi_runtime_lock); status = efi_call_virt(set_time, tm); + spin_unlock(&efi_runtime_lock); spin_unlock_irqrestore(&rtc_lock, flags); return status; } @@ -55,8 +130,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, unsigned long flags; efi_status_t status; + BUG_ON(efi_in_nmi()); spin_lock_irqsave(&rtc_lock, flags); + spin_lock(&efi_runtime_lock); status = efi_call_virt(get_wakeup_time, enabled, pending, tm); + spin_unlock(&efi_runtime_lock); spin_unlock_irqrestore(&rtc_lock, flags); return status; } @@ -66,8 +144,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) unsigned long flags; efi_status_t status; + BUG_ON(efi_in_nmi()); spin_lock_irqsave(&rtc_lock, flags); + spin_lock(&efi_runtime_lock); status = efi_call_virt(set_wakeup_time, enabled, tm); + spin_unlock(&efi_runtime_lock); spin_unlock_irqrestore(&rtc_lock, flags); return status; } @@ -78,14 +159,33 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, unsigned long *data_size, void *data) { - return efi_call_virt(get_variable, name, vendor, attr, data_size, data); + unsigned long flags; + efi_status_t status; + bool __in_nmi = efi_in_nmi(); + + if (!__in_nmi) + spin_lock_irqsave(&efi_runtime_lock, flags); + status = efi_call_virt(get_variable, name, vendor, attr, data_size, + data); + if (!__in_nmi) + spin_unlock_irqrestore(&efi_runtime_lock, flags); + return status; } static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { - return efi_call_virt(get_next_variable, name_size, name, vendor); + unsigned long flags; + efi_status_t status; + bool __in_nmi = efi_in_nmi(); + + if (!__in_nmi) + spin_lock_irqsave(&efi_runtime_lock, flags); + status = efi_call_virt(get_next_variable, name_size, name, vendor); + if (!__in_nmi) + spin_unlock_irqrestore(&efi_runtime_lock, flags); + return status; } static efi_status_t virt_efi_set_variable(efi_char16_t *name, @@ -94,7 +194,17 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, unsigned long data_size, void *data) { - return efi_call_virt(set_variable, name, vendor, attr, data_size, data); + unsigned long flags; + efi_status_t status; + bool __in_nmi = efi_in_nmi(); + + if (!__in_nmi) + 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); + return status; } static efi_status_t virt_efi_query_variable_info(u32 attr, @@ -102,16 +212,32 @@ 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; + bool __in_nmi = efi_in_nmi(); + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(query_variable_info, attr, storage_space, - remaining_space, max_variable_size); + if (!__in_nmi) + 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); + return status; } static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) { - return efi_call_virt(get_next_high_mono_count, count); + unsigned long flags; + efi_status_t status; + + BUG_ON(efi_in_nmi()); + spin_lock_irqsave(&efi_runtime_lock, flags); + status = efi_call_virt(get_next_high_mono_count, count); + spin_unlock_irqrestore(&efi_runtime_lock, flags); + return status; } static void virt_efi_reset_system(int reset_type, @@ -119,17 +245,33 @@ static void virt_efi_reset_system(int reset_type, unsigned long data_size, efi_char16_t *data) { + unsigned long flags; + bool __in_nmi = efi_in_nmi(); + + if (!__in_nmi) + spin_lock_irqsave(&efi_runtime_lock, flags); __efi_call_virt(reset_system, reset_type, status, data_size, data); + if (!__in_nmi) + spin_unlock_irqrestore(&efi_runtime_lock, flags); } 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; + bool __in_nmi = efi_in_nmi(); + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(update_capsule, capsules, count, sg_list); + if (!__in_nmi) + spin_lock_irqsave(&efi_runtime_lock, flags); + status = efi_call_virt(update_capsule, capsules, count, sg_list); + if (!__in_nmi) + spin_unlock_irqrestore(&efi_runtime_lock, flags); + return status; } static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, @@ -137,11 +279,20 @@ 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; + bool __in_nmi = efi_in_nmi(); + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(query_capsule_caps, capsules, count, max_size, - reset_type); + if (!__in_nmi) + spin_lock_irqsave(&efi_runtime_lock, flags); + status = efi_call_virt(query_capsule_caps, capsules, count, max_size, + reset_type); + if (!__in_nmi) + spin_unlock_irqrestore(&efi_runtime_lock, flags); + return status; } void efi_native_runtime_setup(void)
According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Use a spinlock to serialize all Runtime Services with respect to all others, even if this is more than strictly needed. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- v3: Keep value of in_nmi() cached in local scope to help the compiler understand that it cannot change while the function is active v2: So this is v2 of the UEFI Runtime Services serialization patch: this time, I use a single spinlock rather than a set of mutexes, resulting in all services to be serialized with respect to all others. Also added handling of NMI state, as this results in some of the restrictions being lifted (x86, ia64 only) arch/x86/include/asm/efi.h | 2 + drivers/firmware/efi/runtime-wrappers.c | 175 +++++++++++++++++++++++++++++--- 2 files changed, 165 insertions(+), 12 deletions(-)