From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: Ingo Molnar <mingo@kernel.org> Cc: Matt Fleming <matt@codeblueprint.co.uk>, "H . Peter Anvin" <hpa@zytor.com>, Thomas Gleixner <tglx@linutronix.de>, "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>, Andy Lutomirski <luto@kernel.org>, Peter Zijlstra <a.p.zijlstra@chello.nl> Subject: Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled Date: Wed, 3 Feb 2016 10:57:24 +0100 [thread overview] Message-ID: <CAKv+Gu-AWTQNG3ObSZt0cHV1YFb8UbYpkq-uVBqMW0wryfv_ig@mail.gmail.com> (raw) In-Reply-To: <20160203094340.GA15890@gmail.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>, "H . Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Peter Zijlstra <a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org> Subject: Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled Date: Wed, 3 Feb 2016 10:57:24 +0100 [thread overview] Message-ID: <CAKv+Gu-AWTQNG3ObSZt0cHV1YFb8UbYpkq-uVBqMW0wryfv_ig@mail.gmail.com> (raw) In-Reply-To: <20160203094340.GA15890-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> On 3 February 2016 at 10:43, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > > * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote: > >> 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. > > 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.
next prev parent reply other threads:[~2016-02-03 9:57 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-01 22:06 [GIT PULL 00/14] EFI changes for v4.6 Matt Fleming 2016-02-01 22:06 ` [PATCH 01/14] efi: Expose non-blocking set_variable() wrapper to efivars Matt Fleming 2016-02-03 11:31 ` [tip:efi/core] " tip-bot for Ard Biesheuvel 2016-02-01 22:06 ` [PATCH 02/14] efi: Remove redundant efi_set_variable_nonblocking prototype Matt Fleming 2016-02-01 22:06 ` Matt Fleming 2016-02-03 11:31 ` [tip:efi/core] efi: Remove redundant efi_set_variable_nonblocking () prototype tip-bot for Ard Biesheuvel 2016-02-01 22:06 ` [PATCH 03/14] efi: runtime-wrappers: Add a nonblocking version of QueryVariableInfo Matt Fleming 2016-02-01 22:06 ` Matt Fleming 2016-02-03 11:31 ` [tip:efi/core] efi/runtime-wrappers: Add a nonblocking version of QueryVariableInfo() tip-bot for Ard Biesheuvel 2016-02-01 22:06 ` [PATCH 04/14] efi: Add nonblocking option to efi_query_variable_store() Matt Fleming 2016-02-03 11:32 ` [tip:efi/core] " tip-bot for Ard Biesheuvel 2016-02-01 22:06 ` [PATCH 05/14] efi: runtime-wrappers: Remove out of date comment regarding in_nmi() Matt Fleming 2016-02-03 11:32 ` [tip:efi/core] efi/runtime-wrappers: " tip-bot for Ard Biesheuvel 2016-02-01 22:07 ` [PATCH 06/14] efi: runtime-wrapper: Get rid of the rtc_lock spinlock Matt Fleming 2016-02-01 22:07 ` Matt Fleming 2016-02-03 11:32 ` [tip:efi/core] efi: Runtime-wrapper: " tip-bot for Ard Biesheuvel 2016-02-01 22:07 ` [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled Matt Fleming 2016-02-03 9:43 ` Ingo Molnar 2016-02-03 9:57 ` Ard Biesheuvel [this message] 2016-02-03 9:57 ` Ard Biesheuvel 2016-02-03 10:58 ` Ingo Molnar 2016-02-03 11:33 ` Ard Biesheuvel 2016-02-03 12:01 ` Matt Fleming 2016-02-04 13:58 ` [PATCH] efi: runtime-wrappers: run " Ard Biesheuvel 2016-02-08 15:16 ` Matt Fleming 2016-02-08 19:37 ` Andy Lutomirski 2016-02-09 16:52 ` Ard Biesheuvel 2016-02-09 16:52 ` Ard Biesheuvel 2016-02-11 16:03 ` Matt Fleming 2016-02-11 16:04 ` Matt Fleming 2016-02-01 22:07 ` [PATCH 08/14] efivars: Use to_efivar_entry Matt Fleming 2016-02-03 11:33 ` [tip:efi/core] " tip-bot for Geliang Tang 2016-02-01 22:07 ` [PATCH 09/14] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0 Matt Fleming 2016-02-03 11:33 ` [tip:efi/core] x86/efi/bgrt: " tip-bot for Môshe van der Sterre 2016-02-01 22:07 ` [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions Matt Fleming 2016-02-01 22:07 ` Matt Fleming 2016-02-03 10:33 ` Ingo Molnar 2016-02-03 10:33 ` Ingo Molnar 2016-02-03 10:44 ` Matt Fleming 2016-02-03 10:50 ` Ingo Molnar 2016-02-03 10:50 ` Ingo Molnar 2016-02-03 11:18 ` Matt Fleming 2016-02-03 11:27 ` Ingo Molnar 2016-02-03 11:27 ` Ingo Molnar 2016-02-03 11:09 ` Joe Perches 2016-02-01 22:07 ` [PATCH 11/14] x86/efi: Show actual ending addresses in efi_print_memmap Matt Fleming 2016-02-02 8:49 ` Laszlo Ersek 2016-02-03 11:33 ` [tip:efi/core] " tip-bot for Robert Elliott 2016-02-01 22:07 ` [PATCH 12/14] efi: Add NV memory attribute Matt Fleming 2016-02-02 8:54 ` Laszlo Ersek 2016-02-02 8:54 ` Laszlo Ersek 2016-02-03 11:34 ` [tip:efi/core] " tip-bot for Robert Elliott 2016-02-01 22:07 ` [PATCH 13/14] efi: Add Persistent Memory type name Matt Fleming 2016-02-02 8:56 ` Laszlo Ersek 2016-02-03 11:34 ` [tip:efi/core] " tip-bot for Robert Elliott 2016-02-01 22:07 ` [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap Matt Fleming 2016-02-02 9:22 ` Laszlo Ersek 2016-02-03 10:40 ` Ingo Molnar 2016-02-03 11:28 ` Matt Fleming 2016-02-03 12:36 ` Andy Shevchenko 2016-02-03 15:25 ` Elliott, Robert (Persistent Memory) 2016-02-03 15:25 ` Elliott, Robert (Persistent Memory) 2016-02-09 12:20 ` Ingo Molnar 2016-02-09 12:53 ` Laszlo Ersek 2016-02-09 12:53 ` Laszlo Ersek 2016-02-09 13:14 ` Ingo Molnar 2016-02-09 13:14 ` Ingo Molnar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKv+Gu-AWTQNG3ObSZt0cHV1YFb8UbYpkq-uVBqMW0wryfv_ig@mail.gmail.com \ --to=ard.biesheuvel@linaro.org \ --cc=a.p.zijlstra@chello.nl \ --cc=hpa@zytor.com \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=matt@codeblueprint.co.uk \ --cc=mingo@kernel.org \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.