lkml.org 
[lkml]   [2016]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled
    From
    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.

    \
     
     \ /
      Last update: 2016-02-03 11:21    [W:2.489 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site