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

[Ard Biesheuvel]

On 3 February 2016 at 10:43, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
>
>> From: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>>
>> 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.