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

[Ingo Molnar]

* 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?

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