Re: [PATCH 4/4] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

[Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]

On Thu, 19 Nov, at 02:36:31PM, Ard Biesheuvel wrote:
> 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.
 
Please document the justification for this change; why do you *need*
interrupts enabled as opposed to doing it because the spec allows it.

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 88 +++++++++++---------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index a624f7445d11..31e131f09530 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -71,27 +71,29 @@ __weak DEFINE_SPINLOCK(rtc_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(&rtc_lock, flags);
> +	BUG_ON(in_irq());
> +
> +	spin_lock(&rtc_lock);
>  	spin_lock(&efi_runtime_lock);
>  	status = efi_call_virt(get_time, tm, tc);
>  	spin_unlock(&efi_runtime_lock);
> -	spin_unlock_irqrestore(&rtc_lock, flags);
> +	spin_unlock(&rtc_lock);
>  	return status;
>  }

This looks wrong. rtc_lock can be grabbed from IRQ context. Maybe now
is the time to rip out rtc_lock? It's __weak for arm64 anyway and x86
doesn't even use these functions. Though I notice that arm also has an
rtc_lock.

>  static efi_status_t virt_efi_set_time(efi_time_t *tm)
>  {
> -	unsigned long flags;
>  	efi_status_t status;
>  
> -	spin_lock_irqsave(&rtc_lock, flags);
> +	BUG_ON(in_irq());
> +
> +	spin_lock(&rtc_lock);

The thing about using BUG_ON() here is that you get no LOCKDEP info if
you trigger it, nor any stack traces from the softlockup detector.
It's much better to leave spin_lock() alone to do its thing.