Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> gaoliming
> Sent: Monday, March 15, 2021 10:41 AM
> To: devel@edk2.groups.io; ardb@kernel.org
> Cc: lersek@redhat.com; liming.gao@intel.com; jon@solid-run.com;
> leif@nuviainc.com
> Subject: 回复: [edk2-devel] [PATCH 1/1]
> MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB
> protocol
> 
> Ard:
>   Thanks for your fix. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> 
>   Can you include BZ into the commit message? BZ can mention the correct
> conversion solution. Other modules should follow the same way to convert
> FVB protocol VA.


Thanks Ard and Liming,

With above suggestion from Liming handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> Biesheuvel
> > 发送时间: 2021年3月13日 7:06
> > 收件人: devel@edk2.groups.io
> > 抄送: lersek@redhat.com; liming.gao@intel.com; jon@solid-run.com;
> > leif@nuviainc.com; Ard Biesheuvel <ardb@kernel.org>
> > 主题: [edk2-devel] [PATCH 1/1] MdeModulePkg/VariableRuntimeDxe:
> avoid
> > double VA conversion of FVB protocol
> >
> > 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 <ardb@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
> >    )
> >  {
> > +  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)
> > {
> > +      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.
> >    //
> > --
> > 2.30.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#72734):
> > https://edk2.groups.io/g/devel/message/72734
> > Mute This Topic: https://groups.io/mt/81292874/4905953
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [gaoliming@byosoft.com.cn]
> > -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 
> 
>