From patchwork Tue Jul 8 11:23:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 33195 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-yh0-f70.google.com (mail-yh0-f70.google.com [209.85.213.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 3D08420C03 for ; Tue, 8 Jul 2014 11:25:48 +0000 (UTC) Received: by mail-yh0-f70.google.com with SMTP id a41sf23736021yho.1 for ; Tue, 08 Jul 2014 04:25:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:subject:from:to:cc:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-transfer-encoding; bh=jR94/PIjdfDYYIDaIVyHOzJG4ntNT/axywIIXlCy1QA=; b=ERL/WSf46wTeDlvDgrbWoV6Zu8nW2q+gIAcov2BRmep/1eF6+KUlOEWVH3q/LwXILs H00W46CMsTllt4fOCfS2dBtjU+NtrJQc9Tg1/0RIeI4W/1+FGPrgMWRaXdNR6w6WCw9W Q2q2A3wBJ6UNZvqF7cTGFHCeCqB6FUXe+kicbMfFV3jfAcL9bxLb9GbNMqMgJ75AA1Fu PYgsvgPF2p74yzIZgXu36WV5Rl9I5nUxYdYZj0n+5HcOu8J0UPxoWvqFwN70ZueRqLn5 uP4AIAAHUQZL6rVDsWUbQkEVpBe6m6axfmf1oP+3BF2Ix9KtgYd8N0kKD0NjPAeB7gKk 8crg== X-Gm-Message-State: ALoCoQlAhQNw7+B80qZZIQO8jiOdu9deBYppZ4RCBuDtN9elljPYoVPs7F8N741CIuwgDzERvxc9 X-Received: by 10.224.72.74 with SMTP id l10mr16788774qaj.4.1404818748045; Tue, 08 Jul 2014 04:25:48 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.104.177 with SMTP id a46ls2112748qgf.7.gmail; Tue, 08 Jul 2014 04:25:47 -0700 (PDT) X-Received: by 10.52.118.99 with SMTP id kl3mr28008428vdb.18.1404818747925; Tue, 08 Jul 2014 04:25:47 -0700 (PDT) Received: from mail-ve0-f180.google.com (mail-ve0-f180.google.com [209.85.128.180]) by mx.google.com with ESMTPS id sk10si20018778vec.81.2014.07.08.04.25.47 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 08 Jul 2014 04:25:47 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.128.180 as permitted sender) client-ip=209.85.128.180; Received: by mail-ve0-f180.google.com with SMTP id jw12so5517468veb.11 for ; Tue, 08 Jul 2014 04:25:47 -0700 (PDT) X-Received: by 10.58.196.232 with SMTP id ip8mr78980vec.47.1404818747824; Tue, 08 Jul 2014 04:25:47 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp761719vcb; Tue, 8 Jul 2014 04:25:47 -0700 (PDT) X-Received: by 10.68.225.74 with SMTP id ri10mr9190269pbc.116.1404818746514; Tue, 08 Jul 2014 04:25:46 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id m9si2831025pdj.50.2014.07.08.04.25.46 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Jul 2014 04:25:46 -0700 (PDT) Received-SPF: none (google.com: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org does not designate permitted sender hosts) client-ip=2001:1868:205::9; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1X4TVZ-0005rW-Az; Tue, 08 Jul 2014 11:24:25 +0000 Received: from mail-la0-f49.google.com ([209.85.215.49]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1X4TVU-0005LC-Dd for linux-arm-kernel@lists.infradead.org; Tue, 08 Jul 2014 11:24:22 +0000 Received: by mail-la0-f49.google.com with SMTP id gf5so3848126lab.36 for ; Tue, 08 Jul 2014 04:23:55 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.112.204.106 with SMTP id kx10mr1379551lbc.73.1404818635440; Tue, 08 Jul 2014 04:23:55 -0700 (PDT) Received: by 10.112.33.66 with HTTP; Tue, 8 Jul 2014 04:23:55 -0700 (PDT) In-Reply-To: <1404818508-17537-1-git-send-email-ard.biesheuvel@linaro.org> References: <1404818508-17537-1-git-send-email-ard.biesheuvel@linaro.org> Date: Tue, 8 Jul 2014 13:23:55 +0200 Message-ID: Subject: Re: [PATCH v2] efi: implement mandatory locking for UEFI Runtime Services From: Ard Biesheuvel To: Matt Fleming X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140708_042420_879027_0089D363 X-CRM114-Status: GOOD ( 25.42 ) X-Spam-Score: -0.7 (/) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-0.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.215.49 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RCVD_IN_MSPIKE_H3 RBL: Good reputation (+3) [209.85.215.49 listed in wl.mailspike.net] -0.0 RCVD_IN_MSPIKE_WL Mailspike good senders Cc: "linux-efi@vger.kernel.org" , Ard Biesheuvel , Leif Lindholm , Roy Franz , Mark Salter , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ard.biesheuvel@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.128.180 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 On 8 July 2014 13:21, Ard Biesheuvel wrote: > According to section 7.1 of the UEFI spec, Runtime Services are not fully > reentrant, and there are particular combinations of calls that need to be > serialized. > > Signed-off-by: Ard Biesheuvel > --- > > So this is v2 of the UEFI Runtime Services serialization patch: this time, I use > a single spinlock rather than a set of mutexes, resulting in all services to be > serialized with respect to all others. Also added handling of NMI state, as this > results in some of the restrictions being lifted (x86, ia64 only) > > One question remains: with the NMI deadlock handling in place, is it really > necessary to disable interrupts in all cases? > I omitted this hunk from the patch by accident: > drivers/firmware/efi/runtime-wrappers.c | 167 +++++++++++++++++++++++++++++--- > 1 file changed, 155 insertions(+), 12 deletions(-) > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > index 10daa4bbb258..5ee3b16498a2 100644 > --- a/drivers/firmware/efi/runtime-wrappers.c > +++ b/drivers/firmware/efi/runtime-wrappers.c > @@ -14,11 +14,76 @@ > * This file is released under the GPLv2. > */ > > +#include > #include > -#include /* spinlock_t */ > +#include > +#include > #include > > /* > + * According to section 7.1 of the UEFI spec, Runtime Services are not fully > + * reentrant, and there are particular combinations of calls that need to be > + * serialized. > + * > + * Table 31. Rules for Reentry Into Runtime Services > + * +------------------------------------+-------------------------------+ > + * | If previous call is busy in | Forbidden to call | > + * +------------------------------------+-------------------------------+ > + * | Any | SetVirtualAddressMap() | > + * +------------------------------------+-------------------------------+ > + * | ConvertPointer() | ConvertPointer() | > + * +------------------------------------+-------------------------------+ > + * | SetVariable() | ResetSystem() | > + * | UpdateCapsule() | | > + * | SetTime() | | > + * | SetWakeupTime() | | > + * | GetNextHighMonotonicCount() | | > + * +------------------------------------+-------------------------------+ > + * | GetVariable() | GetVariable() | > + * | GetNextVariableName() | GetNextVariableName() | > + * | SetVariable() | SetVariable() | > + * | QueryVariableInfo() | QueryVariableInfo() | > + * | UpdateCapsule() | UpdateCapsule() | > + * | QueryCapsuleCapabilities() | QueryCapsuleCapabilities() | > + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | > + * +------------------------------------+-------------------------------+ > + * | GetTime() | GetTime() | > + * | SetTime() | SetTime() | > + * | GetWakeupTime() | GetWakeupTime() | > + * | SetWakeupTime() | SetWakeupTime() | > + * +------------------------------------+-------------------------------+ > + * > + * Instead of adding locks for each of the groups, we add a single spinlock > + * that serializes all runtime services calls with respect to all others. > + */ > +static DEFINE_SPINLOCK(efi_runtime_lock); > + > +/* > + * Some runtime services calls can be reentrant under NMI, even if the table > + * above says they are not. > + * > + * Table 32. Functions that may be called after Machine Check ,INIT and NMI > + * +----------------------------+------------------------------------------+ > + * | Function | Called after Machine Check, INIT and NMI | > + * +----------------------------+------------------------------------------+ > + * | GetTime() | Yes, even if previously busy. | > + * | GetVariable() | Yes, even if previously busy | > + * | GetNextVariableName() | Yes, even if previously busy | > + * | QueryVariableInfo() | Yes, even if previously busy | > + * | SetVariable() | Yes, even if previously busy | > + * | UpdateCapsule() | Yes, even if previously busy | > + * | QueryCapsuleCapabilities() | Yes, even if previously busy | > + * | ResetSystem() | Yes, even if previously busy | > + * +----------------------------+------------------------------------------+ > + * > + * In order to prevent deadlocks under NMI, the wrappers for these functions > + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi(). > + */ > +#ifndef efi_in_nmi > +#define efi_in_nmi() (0) > +#endif > + > +/* > * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"), > * the EFI specification requires that callers of the time related runtime > * functions serialize with other CMOS accesses in the kernel, as the EFI time > @@ -31,9 +96,15 @@ 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); > + if (!efi_in_nmi()) { > + spin_lock_irqsave(&rtc_lock, flags); > + spin_lock(&efi_runtime_lock); > + } > status = efi_call_virt(get_time, tm, tc); > - spin_unlock_irqrestore(&rtc_lock, flags); > + if (!efi_in_nmi()) { > + spin_unlock(&efi_runtime_lock); > + spin_unlock_irqrestore(&rtc_lock, flags); > + } > return status; > } > > @@ -42,8 +113,11 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm) > unsigned long flags; > efi_status_t status; > > + BUG_ON(efi_in_nmi()); > spin_lock_irqsave(&rtc_lock, flags); > + spin_lock(&efi_runtime_lock); > status = efi_call_virt(set_time, tm); > + spin_unlock(&efi_runtime_lock); > spin_unlock_irqrestore(&rtc_lock, flags); > return status; > } > @@ -55,8 +129,11 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled, > unsigned long flags; > efi_status_t status; > > + BUG_ON(efi_in_nmi()); > spin_lock_irqsave(&rtc_lock, flags); > + spin_lock(&efi_runtime_lock); > status = efi_call_virt(get_wakeup_time, enabled, pending, tm); > + spin_unlock(&efi_runtime_lock); > spin_unlock_irqrestore(&rtc_lock, flags); > return status; > } > @@ -66,8 +143,11 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm) > unsigned long flags; > efi_status_t status; > > + BUG_ON(efi_in_nmi()); > spin_lock_irqsave(&rtc_lock, flags); > + spin_lock(&efi_runtime_lock); > status = efi_call_virt(set_wakeup_time, enabled, tm); > + spin_unlock(&efi_runtime_lock); > spin_unlock_irqrestore(&rtc_lock, flags); > return status; > } > @@ -78,14 +158,31 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, > unsigned long *data_size, > void *data) > { > - return efi_call_virt(get_variable, name, vendor, attr, data_size, data); > + unsigned long flags; > + efi_status_t status; > + > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(get_variable, name, vendor, attr, data_size, > + data); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > efi_char16_t *name, > efi_guid_t *vendor) > { > - return efi_call_virt(get_next_variable, name_size, name, vendor); > + unsigned long flags; > + efi_status_t status; > + > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(get_next_variable, name_size, name, vendor); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static efi_status_t virt_efi_set_variable(efi_char16_t *name, > @@ -94,7 +191,16 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, > unsigned long data_size, > void *data) > { > - return efi_call_virt(set_variable, name, vendor, attr, data_size, data); > + unsigned long flags; > + efi_status_t status; > + > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(set_variable, name, vendor, attr, data_size, > + data); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static efi_status_t virt_efi_query_variable_info(u32 attr, > @@ -102,16 +208,31 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, > u64 *remaining_space, > u64 *max_variable_size) > { > + unsigned long flags; > + efi_status_t status; > + > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - return efi_call_virt(query_variable_info, attr, storage_space, > - remaining_space, max_variable_size); > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(query_variable_info, attr, storage_space, > + remaining_space, max_variable_size); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) > { > - return efi_call_virt(get_next_high_mono_count, count); > + unsigned long flags; > + efi_status_t status; > + > + BUG_ON(efi_in_nmi()); > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(get_next_high_mono_count, count); > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static void virt_efi_reset_system(int reset_type, > @@ -119,17 +240,31 @@ static void virt_efi_reset_system(int reset_type, > unsigned long data_size, > efi_char16_t *data) > { > + unsigned long flags; > + > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > __efi_call_virt(reset_system, reset_type, status, data_size, data); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > } > > static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, > unsigned long count, > unsigned long sg_list) > { > + unsigned long flags; > + efi_status_t status; > + > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - return efi_call_virt(update_capsule, capsules, count, sg_list); > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(update_capsule, capsules, count, sg_list); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > @@ -137,11 +272,19 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, > u64 *max_size, > int *reset_type) > { > + unsigned long flags; > + efi_status_t status; > + > if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) > return EFI_UNSUPPORTED; > > - return efi_call_virt(query_capsule_caps, capsules, count, max_size, > - reset_type); > + if (!efi_in_nmi()) > + spin_lock_irqsave(&efi_runtime_lock, flags); > + status = efi_call_virt(query_capsule_caps, capsules, count, max_size, > + reset_type); > + if (!efi_in_nmi()) > + spin_unlock_irqrestore(&efi_runtime_lock, flags); > + return status; > } > > void efi_native_runtime_setup(void) > -- > 1.8.3.2 > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 55059a50a01f..d8ee5bb527e5 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -2,6 +2,7 @@ #define _ASM_X86_EFI_H #include +#include /* * We map the EFI regions needed for runtime services non-contiguously, * with preserved alignment on virtual addresses starting from -4G down @@ -52,6 +53,8 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...); kernel_fpu_end(); \ }) +#define efi_in_nmi() in_nmi() + #define efi_ioremap(addr, size, type, attr) ioremap_cache(addr, size) #else /* !CONFIG_X86_32 */