From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web11.1930.1615951247383652663 for ; Tue, 16 Mar 2021 20:20:48 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([58.246.60.130]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Wed, 17 Mar 2021 11:20:44 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 58.246.60.130 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , "'Ard Biesheuvel'" Cc: , , References: <20210312230554.292964-1-ardb@kernel.org> <20a15ffd-20bb-8170-e96b-c6151083883f@redhat.com> In-Reply-To: <20a15ffd-20bb-8170-e96b-c6151083883f@redhat.com> Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIDEvMV0gTWRlTW9kdWxlUGtnL1ZhcmlhYmxlUnVudGltZUR4ZTogYXZvaWQgZG91YmxlIFZBIGNvbnZlcnNpb24gb2YgRlZCIHByb3RvY29s?= Date: Wed, 17 Mar 2021 11:20:44 +0800 Message-ID: <00cb01d71adc$84805850$8d8108f0$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQF0h9jvF1uStj3iSnT3AW56YAZKmAG9QBQNqz69HvA= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Ard: > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Laszlo Ersek > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2021=E5=B9=B43=E6=9C=8816=E6=97=A5= 23:58 > =E6=94=B6=E4=BB=B6=E4=BA=BA: Ard Biesheuvel ; devel@edk= 2.groups.io > =E6=8A=84=E9=80=81: liming.gao@intel.com; jon@solid-run.com; leif@nuviai= nc.com > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRu= ntimeDxe: > avoid double VA conversion of FVB protocol >=20 > On 03/13/21 00:05, Ard Biesheuvel wrote: > > For historical reasons, the VariableRuntimeDxe performs virtual addres= s > > conversion on the FVB protocol member pointers of the protocol instanc= e > > that backs the EFI variable store. However, the driver that produces t= he > > 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 own= er > > is taking care of this. To answer Laszlo question, I want to know the impact for the simple fix.= =20 > > > > 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 F= VB > > 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 switc= h > > 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 =3D { VarCheckRegis > > > VarCheckVariablePropertySet, > > > VarCheckVariablePropertyGet }; > > > > +STATIC EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL > mFvbProtocolShadow; > > + > > /** > > Some Secure Boot Policy Variable may update following other variabl= e > 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 > > ) > > { >=20 > (1) Why is AcquireLockOnlyAtBootTime() the best place to add this? >=20 > (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".) >=20 > 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. >=20 > > + 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 =3D mVariableModuleGlobal->FvbInstance; > > + if (FvbInstance !=3D NULL && > > + FvbInstance->GetBlockSize !=3D > mFvbProtocolShadow.GetBlockSize) { >=20 > (2) It occurs to me that the following pattern (in a single-threaded > environment): >=20 > if (a !=3D b) { > a =3D b; > } >=20 > is just: >=20 > a =3D b; >=20 > (the small CPU cost notwithstanding). >=20 > 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. >=20 > 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". >=20 > Anyway... just wanted to highlight this: we could drop the GetBlockSize > funcptr comparison. But, we don't have to. >=20 > 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. >=20 > Up to the maintainers to decide whether this justifies v2. If not: >=20 > Acked-by: Laszlo Ersek >=20 > Thanks > Laszlo >=20 > > + FvbInstance->GetBlockSize =3D > mFvbProtocolShadow.GetBlockSize; > > + FvbInstance->GetPhysicalAddress =3D > mFvbProtocolShadow.GetPhysicalAddress; > > + FvbInstance->GetAttributes =3D > mFvbProtocolShadow.GetAttributes; > > + FvbInstance->SetAttributes =3D > mFvbProtocolShadow.SetAttributes; > > + FvbInstance->Read =3D > mFvbProtocolShadow.Read; > > + FvbInstance->Write =3D mFvbProtocolShadow.Write; > > + FvbInstance->EraseBlocks =3D > mFvbProtocolShadow.EraseBlocks; > > + } > > } > > } > > > > @@ -247,13 +267,22 @@ VariableClassAddressChangeEvent ( > > UINTN Index; > > > > if (mVariableModuleGlobal->FvbInstance !=3D 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 poi= nter > > + // 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 =3D 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); > > + I think the simply change is to directly update mVariableModuleGlobal->Fvb= Instance. mVariableModuleGlobal->FvbInstance =3D &mFvbProtocolShadow; Thanks Liming > > // > > // Mark the variable storage region of the FLASH as RUNTIME. > > // > > >=20 >=20 >=20 >=20 >=20