[PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol

[PATCH 1/1] MdeModulePkg/VariableRuntimeDxe: avoid double VA conversion of FVB protocol

[Ard Biesheuvel]

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

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

[Jon Nettleton]

On Sat, Mar 13, 2021 at 12:06 AM Ard Biesheuvel <ardb@kernel.org> 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 <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
>
You can add my

Tested-By: Jon Nettleton <jon@solid-run.com>

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

[gaoliming]

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
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]
> -=-=-=-=-=-=
> 

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

[Wu, Hao A]

> -----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]
> > -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

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 <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
>    )
>  {

(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 <lersek@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);
> +
>    //
>    // Mark the variable storage region of the FLASH as RUNTIME.
>    //
> 

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

[Samer El-Haj-Mahmoud]

Late to the party, but I confirm that this fixes the SetVariable() runtime calls on Solid Run Honeycomb LX2 (confirmed from multiple distros)

Tested-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek via groups.io
> Sent: Tuesday, March 16, 2021 11:58 AM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> Cc: liming.gao@intel.com; Jon (jon@solid-run.com) <jon@solid-run.com>;
> leif@nuviainc.com
> Subject: 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.
> >
> > 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
> >    )
> >  {
>
> (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 <lersek@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);
> > +
> >    //
> >    // Mark the variable storage region of the FLASH as RUNTIME.
> >    //
> >
>
>
>
> 
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[gaoliming]

Ard:

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年3月16日 23:58
> 收件人: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> 抄送: liming.gao@intel.com; jon@solid-run.com; leif@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 <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
> >    )
> >  {
> 
> (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 <lersek@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.
> >    //
> >
> 
> 
> 
> 
> 

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

[Marcin Wojtas]

Hi Ard,

śr., 17 mar 2021 o 04:20 gaoliming <gaoliming@byosoft.com.cn> napisał(a):
>
> Ard:
>
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> > 发送时间: 2021年3月16日 23:58
> > 收件人: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> > 抄送: liming.gao@intel.com; jon@solid-run.com; leif@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 <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
> > >    )
> > >  {
> >
> > (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 <lersek@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;
>

I'm digging up the patch from abyss. Do you think it's possible to
answer Liming's questions and merge it upstream somehow?

Best regards,
Marcin