On 26 November 2015 at 11:23, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
>
The primary concern is EFI writes on a UP system to a variable store
that is backed by RPMB/MMC storage through some marshalling layer in
the secure OS. I will add that to the commit log for v2
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
