Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323557 > unrolled thread

[PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-01 23:10 +0100
    Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services  with interrupts enabled Ingo Molnar <mingo@kernel.org> - 2016-02-03 10:50 +0100
      Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services  with interrupts enabled Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-03 11:00 +0100
        Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services  with interrupts enabled Ingo Molnar <mingo@kernel.org> - 2016-02-03 12:00 +0100
          Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services  with interrupts enabled Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-03 12:40 +0100
            Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services  with interrupts enabled Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-03 13:10 +0100
          [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-04 15:00 +0100
            Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with  interrupts enabled Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-08 16:20 +0100
            Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with  interrupts enabled Andy Lutomirski <luto@amacapital.net> - 2016-02-08 20:40 +0100

#1323557 — [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

From: Ard Biesheuvel <ard.biesheuvel@linaro.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@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 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.6.2

[toc] | [next] | [standalone]

#1325102 — Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Ard Biesheuvel <ard.biesheuvel@linaro.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.

Well, those excessive latencies would affect SMP platforms as well, just that 
there are (usually) other CPUs free to do execution, right?

More fundamentally, this makes me nervous:

 > The UEFI spec allows Runtime Services to be invoked with interrupts enabled. 
 > [...]

So what really matters is not what the spec says, but how Windows executes UEFI 
firmware code in practice.

If major versions of Windows calls UEFI firmware with interrupts disabled, then 
frankly I don't think we should interrupt them under Linux either, regardless of 
what the spec says ...

Random firmware code getting interrupted by the OS changes timings and might have 
other side effects the firmware code might not expect - so the question is, does 
Windows already de facto allow the IRQ preemption of firmware calls?

Also, this:

> -	unsigned long flags;
>  	efi_status_t status;
>  
> -	spin_lock_irqsave(&efi_runtime_lock, flags);
> +	BUG_ON(in_irq());
> +
> +	spin_lock(&efi_runtime_lock);

... how does crashing the kernel help debuggability?

Please use WARN_ON_ONCE() - or in fact, this assert is probably not needed at all, 
as lockdep will warn about IRQ unsafe lock usage.

I'd add comments to the efi_runtime_lock definition site explaining that this is 
never taken from IRQ contexts.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1325109 — Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

On 3 February 2016 at 10:43, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
>
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.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.
>
> Well, those excessive latencies would affect SMP platforms as well, just that
> there are (usually) other CPUs free to do execution, right?
>

Correct.

> More fundamentally, this makes me nervous:
>
>  > The UEFI spec allows Runtime Services to be invoked with interrupts enabled.
>  > [...]
>
> So what really matters is not what the spec says, but how Windows executes UEFI
> firmware code in practice.
>
> If major versions of Windows calls UEFI firmware with interrupts disabled, then
> frankly I don't think we should interrupt them under Linux either, regardless of
> what the spec says ...
>
> Random firmware code getting interrupted by the OS changes timings and might have
> other side effects the firmware code might not expect - so the question is, does
> Windows already de facto allow the IRQ preemption of firmware calls?
>

Good question. I will try to find out.

> Also, this:
>
>> -     unsigned long flags;
>>       efi_status_t status;
>>
>> -     spin_lock_irqsave(&efi_runtime_lock, flags);
>> +     BUG_ON(in_irq());
>> +
>> +     spin_lock(&efi_runtime_lock);
>
> ... how does crashing the kernel help debuggability?
>
> Please use WARN_ON_ONCE() - or in fact, this assert is probably not needed at all,
> as lockdep will warn about IRQ unsafe lock usage.
>

Actually, reading back the original thread, Matt had already
identified this problem, and v2/v3 of this patch removed all of them
but one, so thanks for spotting that.

> I'd add comments to the efi_runtime_lock definition site explaining that this is
> never taken from IRQ contexts.
>

OK.

[toc] | [prev] | [next] | [standalone]

#1325198 — Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > More fundamentally, this makes me nervous:
> >
> >  > The UEFI spec allows Runtime Services to be invoked with interrupts 
> >  > enabled. [...]
> >
> > So what really matters is not what the spec says, but how Windows executes 
> > UEFI firmware code in practice.
> >
> > If major versions of Windows calls UEFI firmware with interrupts disabled, 
> > then frankly I don't think we should interrupt them under Linux either, 
> > regardless of what the spec says ...
> >
> > Random firmware code getting interrupted by the OS changes timings and might 
> > have other side effects the firmware code might not expect - so the question 
> > is, does Windows already de facto allow the IRQ preemption of firmware calls?
> >
> 
> Good question. I will try to find out.

Note that if there's a reasonable (but not 100%) case in favor of keeping irqs 
enabled, we can try your patch, with the possibility that we might have to revert 
it, should it cause problems.

In practice we probably already interrupt EFI services with NMI interrupts, which 
can be pretty heavy as well if they for example generate printks.

So I'm not against this change in a strong fashion - I'm just a bit cautious and 
it would be nice to know how Windows behaves here.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1325255 — Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

On 3 February 2016 at 11:58, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > More fundamentally, this makes me nervous:
>> >
>> >  > The UEFI spec allows Runtime Services to be invoked with interrupts
>> >  > enabled. [...]
>> >
>> > So what really matters is not what the spec says, but how Windows executes
>> > UEFI firmware code in practice.
>> >
>> > If major versions of Windows calls UEFI firmware with interrupts disabled,
>> > then frankly I don't think we should interrupt them under Linux either,
>> > regardless of what the spec says ...
>> >
>> > Random firmware code getting interrupted by the OS changes timings and might
>> > have other side effects the firmware code might not expect - so the question
>> > is, does Windows already de facto allow the IRQ preemption of firmware calls?
>> >
>>
>> Good question. I will try to find out.
>
> Note that if there's a reasonable (but not 100%) case in favor of keeping irqs
> enabled, we can try your patch, with the possibility that we might have to revert
> it, should it cause problems.
>

I think this might have been the reason Matt wanted this in -next
early, but I will let him confirm whether that was the case.

> In practice we probably already interrupt EFI services with NMI interrupts, which
> can be pretty heavy as well if they for example generate printks.
>
> So I'm not against this change in a strong fashion - I'm just a bit cautious and
> it would be nice to know how Windows behaves here.
>

I am not sure how yet, but I am going to try and figure out what
Windows does. I suppose hacking OVMF to record some IRQ mask
information when RT services are being invoked should be sufficient,
but I am going to need some help from someone that understands OVMF
and x86 (Matt?)

[toc] | [prev] | [next] | [standalone]

#1325320 — Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

On Wed, 03 Feb, at 12:33:10PM, Ard Biesheuvel wrote:
> On 3 February 2016 at 11:58, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> >> > More fundamentally, this makes me nervous:
> >> >
> >> >  > The UEFI spec allows Runtime Services to be invoked with interrupts
> >> >  > enabled. [...]
> >> >
> >> > So what really matters is not what the spec says, but how Windows executes
> >> > UEFI firmware code in practice.
> >> >
> >> > If major versions of Windows calls UEFI firmware with interrupts disabled,
> >> > then frankly I don't think we should interrupt them under Linux either,
> >> > regardless of what the spec says ...
> >> >
> >> > Random firmware code getting interrupted by the OS changes timings and might
> >> > have other side effects the firmware code might not expect - so the question
> >> > is, does Windows already de facto allow the IRQ preemption of firmware calls?
> >> >
> >>
> >> Good question. I will try to find out.
> >
> > Note that if there's a reasonable (but not 100%) case in favor of keeping irqs
> > enabled, we can try your patch, with the possibility that we might have to revert
> > it, should it cause problems.
> >
> 
> I think this might have been the reason Matt wanted this in -next
> early, but I will let him confirm whether that was the case.
 
It was indeed. Additionally I didn't want the EFI material to miss the
merge window again.

> > In practice we probably already interrupt EFI services with NMI interrupts, which
> > can be pretty heavy as well if they for example generate printks.
> >
> > So I'm not against this change in a strong fashion - I'm just a bit cautious and
> > it would be nice to know how Windows behaves here.
> >
> 
> I am not sure how yet, but I am going to try and figure out what
> Windows does. I suppose hacking OVMF to record some IRQ mask
> information when RT services are being invoked should be sufficient,
> but I am going to need some help from someone that understands OVMF
> and x86 (Matt?)

Sure, I can help out with that. Hit me up on IRC. I'm also looping in
Sai who has done OVMF hacking for OS diagnostics in the past.

[toc] | [prev] | [next] | [standalone]

#1326831 — [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

OK, since Sai has confirmed that Windows leaves interrupts enabled when
calling the EFI variable store related runtime services, we should be able
to do the same for Linux, or at least be slightly more confident that we
won't have to back out this change later.

@Sai: could you please confirm on-list as well? Thanks.

Below is an updated version of the patch, rebased onto current tip/efi/core,
with the BUG_ON() removed that I left in inadvertently. I also added a mention
in the commit log that Windows leaves interrupts enabled as well. As far as
annotating the definition of efi_runtime_lock is concerned, the existing ~40
lines of documentation should be sufficient imo so I left that as is. Thanks.

--------8<----------------
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.

Note that other OSes such as Windows call UEFI Runtime Services with
interrupts enabled as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 71 ++++++++------------
 1 file changed, 28 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 7b8b2f2702ca..de6953039af6 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,11 @@ 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);
+	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 +136,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 +150,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 +167,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 +185,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 +214,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 +239,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

[toc] | [prev] | [next] | [standalone]

#1329171 — Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

On Thu, 04 Feb, at 02:58:47PM, Ard Biesheuvel wrote:
> OK, since Sai has confirmed that Windows leaves interrupts enabled when
> calling the EFI variable store related runtime services, we should be able
> to do the same for Linux, or at least be slightly more confident that we
> won't have to back out this change later.
> 
> @Sai: could you please confirm on-list as well? Thanks.
> 
> Below is an updated version of the patch, rebased onto current tip/efi/core,
> with the BUG_ON() removed that I left in inadvertently. I also added a mention
> in the commit log that Windows leaves interrupts enabled as well. As far as
> annotating the definition of efi_runtime_lock is concerned, the existing ~40
> lines of documentation should be sufficient imo so I left that as is. Thanks.
> 
> --------8<----------------
> 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.
> 
> Note that other OSes such as Windows call UEFI Runtime Services with
> interrupts enabled as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 71 ++++++++------------
>  1 file changed, 28 insertions(+), 43 deletions(-)

Ingo, if you want to pick up this patch directly you can add my,

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

Otherwise let me know and I'll send you a pull request.

[toc] | [prev] | [next] | [standalone]

#1329504 — Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

On Feb 4, 2016 5:58 AM, "Ard Biesheuvel" <ard.biesheuvel@linaro.org> wrote:
>
> OK, since Sai has confirmed that Windows leaves interrupts enabled when
> calling the EFI variable store related runtime services, we should be able
> to do the same for Linux, or at least be slightly more confident that we
> won't have to back out this change later.

Could this use a mutex instead of a spinlock?

Can someone with a mixed mode setup read a variable in a loop and make
sure it doesn't crash and burn?  It should work fine, but explicit
testing would be nice.  (It's interesting mainly because doing a mixed
mode call with interrupts on can result in a non-IST CPL0 to CPL0
exception delivery, which won't result in a stack switch.  This could
easily trigger a stack overflow, logic bug, microcode bug, or
as-yet-unknown CPU "feature".

Hmm.  We should also audit the mixed mode entry code to make sure that
the high bits of RSP are explicitly clear before switching into compat
mode.  If I had to make a guess about how CPUs behave, I'd guess
pessimistically: Intel CPUs clear the high bits of RSP when switching
into long mode due to interrupt delivery, and AMD CPUs leave them set
just to mess with us.

Also, a WARN_ON(in_interrupt()) somewhere might be a good sanity check.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web