From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web11.1046.1615910312007924321 for ; Tue, 16 Mar 2021 08:58:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cruBN/tp; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615910311; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=X4RGCleruGuriFheb+z+u3kBBrQHf61kJXpU0TpYY0M=; b=cruBN/tpMbMtko9hubGZvWxMcWIDi8Ca6DsgwScJxP3pBLyl69xPSNb5DJcjWgSV9ztRBj K68NqoQUZz0deAmRl7GwGxeC0a3hYBZAxxOcbpZeFI6Y78yKYs0Rx2JQwHgWf60I1Orbni S6zxaxF9b0GAbHNeCprwLTcco6c87kc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-478-UsegSUQHO32c7pQsfw7LEQ-1; Tue, 16 Mar 2021 11:58:27 -0400 X-MC-Unique: UsegSUQHO32c7pQsfw7LEQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id DBCD619200D0; Tue, 16 Mar 2021 15:58:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-138.ams2.redhat.com [10.36.114.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id 78DD860875; Tue, 16 Mar 2021 15:58:24 +0000 (UTC) Subject: Re: [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol To: Ard Biesheuvel , devel@edk2.groups.io Cc: liming.gao@intel.com, jon@solid-run.com, leif@nuviainc.com References: <20210312230554.292964-1-ardb@kernel.org> From: "Laszlo Ersek" Message-ID: <20a15ffd-20bb-8170-e96b-c6151083883f@redhat.com> Date: Tue, 16 Mar 2021 16:58:23 +0100 MIME-Version: 1.0 In-Reply-To: <20210312230554.292964-1-ardb@kernel.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/13/21 00:05, Ard Biesheuvel wrote: > For historical reasons, the VariableRuntimeDxe performs virtual address > conversion on the FVB protocol member pointers of the protocol instance > that backs the EFI variable store. However, the driver that produces the > actual instance should be doing this, as it is the owner, and provides > the actual implementation of those methods. > > Unfortunately, we cannot simply change this: existing drivers may rely > on this behavior, and so the variable driver should take care to only > convert the pointers when necessary, but leave them alone when the owner > is taking care of this. > > The virtual address switch event can be delivered in arbitrary order, and > so we should take care of this in a way that does not rely on whether this > driver converts its pointers either before or after the owner of the FVB > protocol receives the event. > > So let's not convert the pointers at all when the event is delivered, but > record the converted addresses in a shadow FVB protocol. This way, we can > check when the variable driver is invoked at runtime whether the switch > has occurred or not, and perform the switch if it hasn't. > > Signed-off-by: Ard Biesheuvel > --- > Build tested only. > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 50 +++++++++++++++++--- > 1 file changed, 43 insertions(+), 7 deletions(-) > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > index 0fca0bb2a9b5..3d83ac4452ee 100644 > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > @@ -37,6 +37,8 @@ EDKII_VAR_CHECK_PROTOCOL mVarCheck = { VarCheckRegis > VarCheckVariablePropertySet, > VarCheckVariablePropertyGet }; > > +STATIC EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolShadow; > + > /** > Some Secure Boot Policy Variable may update following other variable changes(SecureBoot follows PK change, etc). > Record their initial State when variable write service is ready. > @@ -105,8 +107,26 @@ AcquireLockOnlyAtBootTime ( > IN EFI_LOCK *Lock > ) > { (1) Why is AcquireLockOnlyAtBootTime() the best place to add this? (Considering especially the leading comment, "This is a temperary function that will be removed when EfiAcquireLock() in UefiLib can handle the call in UEFI Runtimer driver in RT phase".) Obviously, I don't want to create more work for you! I just feel that, if this is not the best place, we shouldn't overload this function with a new responsibility. > + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *FvbInstance; > + > if (!AtRuntime ()) { > EfiAcquireLock (Lock); > + } else { > + // > + // Check whether we need to swap in the converted pointer values for the > + // FVB protocol methods > + // > + FvbInstance = mVariableModuleGlobal->FvbInstance; > + if (FvbInstance != NULL && > + FvbInstance->GetBlockSize != mFvbProtocolShadow.GetBlockSize) { (2) It occurs to me that the following pattern (in a single-threaded environment): if (a != b) { a = b; } is just: a = b; (the small CPU cost notwithstanding). And that puts this patch in a bit different light: it's not that we may or may not convert. Instead, we *always* convert, the question is *when* we apply the result of the conversion. Functionally, there is no difference, but to me mentally, there'd be a difference, if we said "delay applying the runtime conversion until first call". Anyway... just wanted to highlight this: we could drop the GetBlockSize funcptr comparison. But, we don't have to. Given the reviews from Liming and Hao -- and thank you in the first place for writing the patch! --, I won't really ask for a v2. I'd just somehow prefer the compat logic to be elsewhere, rather than in AcquireLockOnlyAtBootTime(). In the first place, I'm not clear what we currently use AcquireLockOnlyAtBootTime() for. Up to the maintainers to decide whether this justifies v2. If not: Acked-by: Laszlo Ersek Thanks Laszlo > + FvbInstance->GetBlockSize = mFvbProtocolShadow.GetBlockSize; > + FvbInstance->GetPhysicalAddress = mFvbProtocolShadow.GetPhysicalAddress; > + FvbInstance->GetAttributes = mFvbProtocolShadow.GetAttributes; > + FvbInstance->SetAttributes = mFvbProtocolShadow.SetAttributes; > + FvbInstance->Read = mFvbProtocolShadow.Read; > + FvbInstance->Write = mFvbProtocolShadow.Write; > + FvbInstance->EraseBlocks = mFvbProtocolShadow.EraseBlocks; > + } > } > } > > @@ -247,13 +267,22 @@ VariableClassAddressChangeEvent ( > UINTN Index; > > if (mVariableModuleGlobal->FvbInstance != NULL) { > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetBlockSize); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetPhysicalAddress); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->GetAttributes); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->SetAttributes); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Read); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->Write); > - EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance->EraseBlocks); > + // > + // This module did not produce the FVB protocol instance that provides the > + // variable store, so it should not be the one performing the pointer > + // conversion on its members. However, some FVB protocol implementations > + // may rely on this behavior, which was present in older versions of this > + // driver, so we need to perform the conversion if the protocol producer > + // fails to do so. So let's record the converted values now, and swap them > + // in later if they haven't changed values. > + // > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetBlockSize); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetPhysicalAddress); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.GetAttributes); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.SetAttributes); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Read); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.Write); > + EfiConvertPointer (0x0, (VOID **)&mFvbProtocolShadow.EraseBlocks); > EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->FvbInstance); > } > EfiConvertPointer (0x0, (VOID **) &mVariableModuleGlobal->PlatformLangCodes); > @@ -454,6 +483,13 @@ FtwNotificationEvent ( > } > mVariableModuleGlobal->FvbInstance = FvbProtocol; > > + // > + // Store the boot time values of the function pointers so we can compare > + // them later. This is needed to avoid double conversion during the call > + // to SetVirtualAddressMap. > + // > + CopyMem (&mFvbProtocolShadow, FvbProtocol, sizeof mFvbProtocolShadow); > + > // > // Mark the variable storage region of the FLASH as RUNTIME. > // >