Ard:
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年3月16日 23:58
> 收件人: Ard Biesheuvel <a...@kernel.org>; devel@edk2.groups.io
> 抄送: liming....@intel.com; j...@solid-run.com; l...@nuviainc.com
> 主题: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe:
> avoid double VA conversion of FVB protocol
>
> 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.
To answer Laszlo question, I want to know the impact for the simple fix.
> >
> > 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 <a...@kernel.org>
> > ---
> > 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 <ler...@redhat.com>
>
> 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);
> > +
I think the simply change is to directly update
mVariableModuleGlobal->FvbInstance.
mVariableModuleGlobal->FvbInstance = &mFvbProtocolShadow;
Thanks
Liming
> > //
> > // Mark the variable storage region of the FLASH as RUNTIME.
> > //
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72948): https://edk2.groups.io/g/devel/message/72948
Mute This Topic: https://groups.io/mt/81395426/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
