Re: [PATCH 06/12] MdeModulePkg Variable: Add emulated variable NV mode support

[Laszlo Ersek <lersek@redhat.com>]

Hi Star,

On 01/13/19 16:37, Star Zeng wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1323
> Merge EmuVariable and Real variable driver.
> 
> Add emulated variable NV mode support in real variable driver.
> Platform can configure PcdEmuVariableNvModeEnable statically
> (build time) or dynamically (boot time) to support emulated
> variable NV mode.
> 
> Then EmuVariableRuntimeDxe could be removed, the removal of
> EmuVariableRuntimeDxe will be done after platforms are migrated
> to use the merged variable driver.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  .../Universal/Variable/RuntimeDxe/Variable.c       | 319 +++++++++++++++------
>  .../Universal/Variable/RuntimeDxe/Variable.h       |   1 +
>  .../Universal/Variable/RuntimeDxe/VariableDxe.c    |  27 +-
>  .../Variable/RuntimeDxe/VariableRuntimeDxe.inf     |   4 +-
>  .../Universal/Variable/RuntimeDxe/VariableSmm.c    |  29 +-
>  .../Universal/Variable/RuntimeDxe/VariableSmm.inf  |   4 +-
>  6 files changed, 271 insertions(+), 113 deletions(-)

What I did for (briefly) reviewing this patch was the following. I
fetched the branch you noted in the blurb, and the ran

  git show --color -W -b 9b509dea1227

Because, a large part of this patch just re-indents existent code, due
to new conditionals.

My main goal with this review was to see whether "EmuNvMode==FALSE"
would imply that the changes are a "no-op". Because OVMF, and the
non-Xen ArmVirt DSC, will inherit the default FALSE setting for the PCD.
So far, things look fine.

However, I noticed two small things that I believe would improve the
readability of the patch. I suggest that you please split them out to
separate patches, in the "preparation" section of the series. Namely:


> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 424f92a53757..845276d891ae 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -34,6 +34,7 @@ VARIABLE_MODULE_GLOBAL  *mVariableModuleGlobal;
>  
>  ///
>  /// Define a memory cache that improves the search performance for a variable.
> +/// For EmuNvMode == TRUE, it will be equal to NonVolatileVariableBase.
>  ///
>  VARIABLE_STORE_HEADER  *mNvVariableCache      = NULL;
>  
> @@ -273,7 +274,7 @@ UpdateVariableStore (
>    //
>    // Check if the Data is Volatile.
>    //
> -  if (!Volatile) {
> +  if (!Volatile && !mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>      if (Fvb == NULL) {
>        return EFI_UNSUPPORTED;
>      }
> @@ -296,17 +297,30 @@ UpdateVariableStore (
>      // Data Pointer should point to the actual Address where data is to be
>      // written.
>      //
> -    VolatileBase = (VARIABLE_STORE_HEADER *) ((UINTN) mVariableModuleGlobal->VariableGlobal.VolatileVariableBase);
> -    if (SetByIndex) {
> -      DataPtr += mVariableModuleGlobal->VariableGlobal.VolatileVariableBase;
> -    }
> +    if (Volatile) {
> +      VolatileBase = (VARIABLE_STORE_HEADER *) ((UINTN) mVariableModuleGlobal->VariableGlobal.VolatileVariableBase);
> +      if (SetByIndex) {
> +        DataPtr += mVariableModuleGlobal->VariableGlobal.VolatileVariableBase;
> +      }
>  
> -    if ((DataPtr + DataSize) > ((UINTN) ((UINT8 *) VolatileBase + VolatileBase->Size))) {
> -      return EFI_OUT_OF_RESOURCES;
> +      if ((DataPtr + DataSize) > ((UINTN) VolatileBase + VolatileBase->Size)) {

(1) Here the "if" statement is not just re-indented, but the controlling
expression is also modified. The change looks OK to me (and I understand
why you were motivated to simplify it); however, it would be better to
split this change to a separate refactoring patch. That patch will be
simple to review in isolation, and in turn it will make this patch
easier to read, especially with "git show -b".

> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +    } else {
> +      //
> +      // Emulated non-volatile variable mode.
> +      //
> +      if (SetByIndex) {
> +        DataPtr += (UINTN) mNvVariableCache;
> +      }
> +
> +      if ((DataPtr + DataSize) > ((UINTN) mNvVariableCache + mNvVariableCache->Size)) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
>      }
>  
>      //
> -    // If Volatile Variable just do a simple mem copy.
> +    // If Volatile/Emulated Non-volatile Variable just do a simple mem copy.
>      //
>      CopyMem ((UINT8 *)(UINTN)DataPtr, Buffer, DataSize);
>      return EFI_SUCCESS;
> @@ -987,7 +1001,7 @@ Reclaim (
>    CommonUserVariableTotalSize = 0;
>    HwErrVariableTotalSize  = 0;
>  
> -  if (IsVolatile) {
> +  if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>      //
>      // Start Pointers for the variable.
>      //
> @@ -1155,13 +1169,21 @@ Reclaim (
>      CurrPtr += NewVariableSize;
>    }
>  
> -  if (IsVolatile) {
> +  if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>      //
> -    // If volatile variable store, just copy valid buffer.
> +    // If volatile/emulated non-volatile variable store, just copy valid buffer.
>      //
>      SetMem ((UINT8 *) (UINTN) VariableBase, VariableStoreHeader->Size, 0xff);
>      CopyMem ((UINT8 *) (UINTN) VariableBase, ValidBuffer, (UINTN) CurrPtr - (UINTN) ValidBuffer);
>      *LastVariableOffset = (UINTN) CurrPtr - (UINTN) ValidBuffer;
> +    if (!IsVolatile) {
> +      //
> +      // Emulated non-volatile variable mode.
> +      //
> +      mVariableModuleGlobal->HwErrVariableTotalSize = HwErrVariableTotalSize;
> +      mVariableModuleGlobal->CommonVariableTotalSize = CommonVariableTotalSize;
> +      mVariableModuleGlobal->CommonUserVariableTotalSize = CommonUserVariableTotalSize;
> +    }
>      Status  = EFI_SUCCESS;
>    } else {
>      //
> @@ -1200,7 +1222,7 @@ Reclaim (
>    }
>  
>  Done:
> -  if (IsVolatile) {
> +  if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>      FreePool (ValidBuffer);
>    } else {
>      //
> @@ -2139,7 +2161,6 @@ UpdateVariable (
>    VARIABLE_POINTER_TRACK              *Variable;
>    VARIABLE_POINTER_TRACK              NvVariable;
>    VARIABLE_STORE_HEADER               *VariableStoreHeader;
> -  UINTN                               CacheOffset;
>    UINT8                               *BufferForMerge;
>    UINTN                               MergedBufSize;
>    BOOLEAN                             DataReady;
> @@ -2148,7 +2169,7 @@ UpdateVariable (
>    BOOLEAN                             IsCommonUserVariable;
>    AUTHENTICATED_VARIABLE_HEADER       *AuthVariable;
>  
> -  if (mVariableModuleGlobal->FvbInstance == NULL) {
> +  if (mVariableModuleGlobal->FvbInstance == NULL && !mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
>      //
>      // The FVB protocol is not ready, so the EFI_VARIABLE_WRITE_ARCH_PROTOCOL is not installed.
>      //
> @@ -2567,80 +2588,105 @@ UpdateVariable (
>        }
>        goto Done;
>      }
> -    //
> -    // Four steps
> -    // 1. Write variable header
> -    // 2. Set variable state to header valid
> -    // 3. Write variable data
> -    // 4. Set variable state to valid
> -    //
> -    //
> -    // Step 1:
> -    //
> -    CacheOffset = mVariableModuleGlobal->NonVolatileLastVariableOffset;
> -    Status = UpdateVariableStore (
> -               &mVariableModuleGlobal->VariableGlobal,
> -               FALSE,
> -               TRUE,
> -               Fvb,
> -               mVariableModuleGlobal->NonVolatileLastVariableOffset,
> -               (UINT32) GetVariableHeaderSize (),
> -               (UINT8 *) NextVariable
> -               );
>  
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> +    if (!mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
> +      //
> +      // Four steps
> +      // 1. Write variable header
> +      // 2. Set variable state to header valid
> +      // 3. Write variable data
> +      // 4. Set variable state to valid
> +      //
> +      //
> +      // Step 1:
> +      //
> +      Status = UpdateVariableStore (
> +                 &mVariableModuleGlobal->VariableGlobal,
> +                 FALSE,
> +                 TRUE,
> +                 Fvb,
> +                 mVariableModuleGlobal->NonVolatileLastVariableOffset,
> +                 (UINT32) GetVariableHeaderSize (),
> +                 (UINT8 *) NextVariable
> +                 );
>  
> -    //
> -    // Step 2:
> -    //
> -    NextVariable->State = VAR_HEADER_VALID_ONLY;
> -    Status = UpdateVariableStore (
> -               &mVariableModuleGlobal->VariableGlobal,
> -               FALSE,
> -               TRUE,
> -               Fvb,
> -               mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF (VARIABLE_HEADER, State),
> -               sizeof (UINT8),
> -               &NextVariable->State
> -               );
> +      if (EFI_ERROR (Status)) {
> +        goto Done;
> +      }
>  
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    //
> -    // Step 3:
> -    //
> -    Status = UpdateVariableStore (
> -               &mVariableModuleGlobal->VariableGlobal,
> -               FALSE,
> -               TRUE,
> -               Fvb,
> -               mVariableModuleGlobal->NonVolatileLastVariableOffset + GetVariableHeaderSize (),
> -               (UINT32) (VarSize - GetVariableHeaderSize ()),
> -               (UINT8 *) NextVariable + GetVariableHeaderSize ()
> -               );
> +      //
> +      // Step 2:
> +      //
> +      NextVariable->State = VAR_HEADER_VALID_ONLY;
> +      Status = UpdateVariableStore (
> +                 &mVariableModuleGlobal->VariableGlobal,
> +                 FALSE,
> +                 TRUE,
> +                 Fvb,
> +                 mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF (VARIABLE_HEADER, State),
> +                 sizeof (UINT8),
> +                 &NextVariable->State
> +                 );
>  
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    //
> -    // Step 4:
> -    //
> -    NextVariable->State = VAR_ADDED;
> -    Status = UpdateVariableStore (
> -               &mVariableModuleGlobal->VariableGlobal,
> -               FALSE,
> -               TRUE,
> -               Fvb,
> -               mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF (VARIABLE_HEADER, State),
> -               sizeof (UINT8),
> -               &NextVariable->State
> -               );
> +      if (EFI_ERROR (Status)) {
> +        goto Done;
> +      }
> +      //
> +      // Step 3:
> +      //
> +      Status = UpdateVariableStore (
> +                 &mVariableModuleGlobal->VariableGlobal,
> +                 FALSE,
> +                 TRUE,
> +                 Fvb,
> +                 mVariableModuleGlobal->NonVolatileLastVariableOffset + GetVariableHeaderSize (),
> +                 (UINT32) (VarSize - GetVariableHeaderSize ()),
> +                 (UINT8 *) NextVariable + GetVariableHeaderSize ()
> +                 );
>  
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> +      if (EFI_ERROR (Status)) {
> +        goto Done;
> +      }
> +      //
> +      // Step 4:
> +      //
> +      NextVariable->State = VAR_ADDED;
> +      Status = UpdateVariableStore (
> +                 &mVariableModuleGlobal->VariableGlobal,
> +                 FALSE,
> +                 TRUE,
> +                 Fvb,
> +                 mVariableModuleGlobal->NonVolatileLastVariableOffset + OFFSET_OF (VARIABLE_HEADER, State),
> +                 sizeof (UINT8),
> +                 &NextVariable->State
> +                 );
> +
> +      if (EFI_ERROR (Status)) {
> +        goto Done;
> +      }
> +
> +      //
> +      // update the memory copy of Flash region.
> +      //
> +      CopyMem ((UINT8 *)mNvVariableCache + mVariableModuleGlobal->NonVolatileLastVariableOffset, (UINT8 *)NextVariable, VarSize);

(2) For this function too, please write a separate "prep" patch, that:

(a) removes the "CacheOffset" variable, and

(b) moves the CopyMem() call that currently uses "CacheOffset" to its
final location -- i.e., just above

  NonVolatileLastVariableOffset += HEADER_ALIGN (VarSize);

-- and final parameter list as well. So that *this* patch need at most
reindent the function call, ultimately.

Again, such a patch should not be hard to review in isolation, and it
will improve the readability of this patch a lot.


In summary, I *think* the current changes are all fine, from the
perspective anyway that I mention at the top; but a reviewer's
confidence is higher if the patch is easier to read.

Thanks,
Laszlo


> +    } else {
> +      //
> +      // Emulated non-volatile variable mode.
> +      //
> +      NextVariable->State = VAR_ADDED;
> +      Status = UpdateVariableStore (
> +                 &mVariableModuleGlobal->VariableGlobal,
> +                 FALSE,
> +                 TRUE,
> +                 Fvb,
> +                 mVariableModuleGlobal->NonVolatileLastVariableOffset,
> +                 (UINT32) VarSize,
> +                 (UINT8 *) NextVariable
> +                 );
> +
> +      if (EFI_ERROR (Status)) {
> +        goto Done;
> +      }
>      }
>  
>      mVariableModuleGlobal->NonVolatileLastVariableOffset += HEADER_ALIGN (VarSize);
> @@ -2653,10 +2699,6 @@ UpdateVariable (
>          mVariableModuleGlobal->CommonUserVariableTotalSize += HEADER_ALIGN (VarSize);
>        }
>      }
> -    //
> -    // update the memory copy of Flash region.
> -    //
> -    CopyMem ((UINT8 *)mNvVariableCache + CacheOffset, (UINT8 *)NextVariable, VarSize);
>    } else {
>      //
>      // Create a volatile variable.
> @@ -3877,6 +3919,93 @@ InitRealNonVolatileVariableStore (
>  }
>  
>  /**
> +  Init emulated non-volatile variable store.
> +
> +  @param[out] VariableStoreBase Output pointer to emulated non-volatile variable store base.
> +
> +  @retval EFI_SUCCESS           Function successfully executed.
> +  @retval EFI_OUT_OF_RESOURCES  Fail to allocate enough memory resource.
> +
> +**/
> +EFI_STATUS
> +InitEmuNonVolatileVariableStore (
> +  EFI_PHYSICAL_ADDRESS  *VariableStoreBase
> +  )
> +{
> +  VARIABLE_STORE_HEADER *VariableStore;
> +  UINT32                VariableStoreLength;
> +  BOOLEAN               FullyInitializeStore;
> +  UINT32                HwErrStorageSize;
> +
> +  FullyInitializeStore = TRUE;
> +
> +  VariableStoreLength = PcdGet32 (PcdVariableStoreSize);
> +  ASSERT (sizeof (VARIABLE_STORE_HEADER) <= VariableStoreLength);
> +
> +  //
> +  // Allocate memory for variable store.
> +  //
> +  if (PcdGet64 (PcdEmuVariableNvStoreReserved) == 0) {
> +    VariableStore = (VARIABLE_STORE_HEADER *) AllocateRuntimePool (VariableStoreLength);
> +    if (VariableStore == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +  } else {
> +    //
> +    // A memory location has been reserved for the NV variable store.  Certain
> +    // platforms may be able to preserve a memory range across system resets,
> +    // thereby providing better NV variable emulation.
> +    //
> +    VariableStore =
> +      (VARIABLE_STORE_HEADER *)(VOID*)(UINTN)
> +        PcdGet64 (PcdEmuVariableNvStoreReserved);
> +    if ((VariableStore->Size == VariableStoreLength) &&
> +        (CompareGuid (&VariableStore->Signature, &gEfiAuthenticatedVariableGuid) ||
> +         CompareGuid (&VariableStore->Signature, &gEfiVariableGuid)) &&
> +        (VariableStore->Format == VARIABLE_STORE_FORMATTED) &&
> +        (VariableStore->State == VARIABLE_STORE_HEALTHY)) {
> +      DEBUG((
> +        DEBUG_INFO,
> +        "Variable Store reserved at %p appears to be valid\n",
> +        VariableStore
> +        ));
> +      FullyInitializeStore = FALSE;
> +    }
> +  }
> +
> +  if (FullyInitializeStore) {
> +    SetMem (VariableStore, VariableStoreLength, 0xff);
> +    //
> +    // Use gEfiAuthenticatedVariableGuid for potential auth variable support.
> +    //
> +    CopyGuid (&VariableStore->Signature, &gEfiAuthenticatedVariableGuid);
> +    VariableStore->Size       = VariableStoreLength;
> +    VariableStore->Format     = VARIABLE_STORE_FORMATTED;
> +    VariableStore->State      = VARIABLE_STORE_HEALTHY;
> +    VariableStore->Reserved   = 0;
> +    VariableStore->Reserved1  = 0;
> +  }
> +
> +  *VariableStoreBase = (EFI_PHYSICAL_ADDRESS) (UINTN) VariableStore;
> +
> +  HwErrStorageSize = PcdGet32 (PcdHwErrStorageSize);
> +
> +  //
> +  // Note that in EdkII variable driver implementation, Hardware Error Record type variable
> +  // is stored with common variable in the same NV region. So the platform integrator should
> +  // ensure that the value of PcdHwErrStorageSize is less than the value of
> +  // (VariableStoreLength - sizeof (VARIABLE_STORE_HEADER)).
> +  //
> +  ASSERT (HwErrStorageSize < (VariableStoreLength - sizeof (VARIABLE_STORE_HEADER)));
> +
> +  mVariableModuleGlobal->CommonVariableSpace = ((UINTN) VariableStoreLength - sizeof (VARIABLE_STORE_HEADER) - HwErrStorageSize);
> +  mVariableModuleGlobal->CommonMaxUserVariableSpace = mVariableModuleGlobal->CommonVariableSpace;
> +  mVariableModuleGlobal->CommonRuntimeVariableSpace = mVariableModuleGlobal->CommonVariableSpace;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Init non-volatile variable store.
>  
>    @retval EFI_SUCCESS           Function successfully executed.
> @@ -3895,9 +4024,19 @@ InitNonVolatileVariableStore (
>    UINTN                                 VariableSize;
>    EFI_STATUS                            Status;
>  
> -  Status = InitRealNonVolatileVariableStore (&VariableStoreBase);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> +  if (!PcdGetBool (PcdEmuVariableNvModeEnable)) {
> +    Status = InitRealNonVolatileVariableStore (&VariableStoreBase);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    mVariableModuleGlobal->VariableGlobal.EmuNvMode = FALSE;
> +  } else {
> +    Status = InitEmuNonVolatileVariableStore (&VariableStoreBase);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    mVariableModuleGlobal->VariableGlobal.EmuNvMode = TRUE;
> +    DEBUG ((DEBUG_INFO, "Variable driver will work at emulated non-volatile variable mode!\n"));
>    }
>  
>    mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase = VariableStoreBase;
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> index 566e7268d187..fdc92b2b017c 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> @@ -92,6 +92,7 @@ typedef struct {
>    UINT32                ReentrantState;
>    BOOLEAN               AuthFormat;
>    BOOLEAN               AuthSupport;
> +  BOOLEAN               EmuNvMode;
>  } VARIABLE_GLOBAL;
>  
>  typedef struct {
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> index 5131e6f351e4..7d5c8b3f842d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> @@ -533,16 +533,23 @@ VariableServiceInitialize (
>                    );
>    ASSERT_EFI_ERROR (Status);
>  
> -  //
> -  // Register FtwNotificationEvent () notify function.
> -  //
> -  EfiCreateProtocolNotifyEvent (
> -    &gEfiFaultTolerantWriteProtocolGuid,
> -    TPL_CALLBACK,
> -    FtwNotificationEvent,
> -    (VOID *)SystemTable,
> -    &mFtwRegistration
> -    );
> +  if (!PcdGetBool (PcdEmuVariableNvModeEnable)) {
> +    //
> +    // Register FtwNotificationEvent () notify function.
> +    //
> +    EfiCreateProtocolNotifyEvent (
> +      &gEfiFaultTolerantWriteProtocolGuid,
> +      TPL_CALLBACK,
> +      FtwNotificationEvent,
> +      (VOID *)SystemTable,
> +      &mFtwRegistration
> +      );
> +  } else {
> +    //
> +    // Emulated non-volatile variable mode does not depend on FVB and FTW.
> +    //
> +    VariableWriteServiceInitializeDxe ();
> +  }
>  
>    Status = gBS->CreateEventEx (
>                    EVT_NOTIFY_SIGNAL,
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index 7ef8a97f5d6a..273067753c25 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -9,7 +9,7 @@
>  #  This external input must be validated carefully to avoid security issues such as
>  #  buffer overflow or integer overflow.
>  #
> -# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  # This program and the accompanying materials
>  # are licensed and made available under the terms and conditions of the BSD License
>  # which accompanies this distribution. The full text of the license may be found at
> @@ -131,6 +131,8 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpaceSize           ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVariableSpaceSize  ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable         ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved      ## SOMETIMES_CONSUMES
>  
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics  ## CONSUMES # statistic the information of variable.
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index e63af812534e..d47493c891e5 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -1034,17 +1034,24 @@ VariableServiceInitialize (
>                      );
>    ASSERT_EFI_ERROR (Status);
>  
> -  //
> -  // Register FtwNotificationEvent () notify function.
> -  //
> -  Status = gSmst->SmmRegisterProtocolNotify (
> -                    &gEfiSmmFaultTolerantWriteProtocolGuid,
> -                    SmmFtwNotificationEvent,
> -                    &SmmFtwRegistration
> -                    );
> -  ASSERT_EFI_ERROR (Status);
> -
> -  SmmFtwNotificationEvent (NULL, NULL, NULL);
> +  if (!PcdGetBool (PcdEmuVariableNvModeEnable)) {
> +    //
> +    // Register FtwNotificationEvent () notify function.
> +    //
> +    Status = gSmst->SmmRegisterProtocolNotify (
> +                      &gEfiSmmFaultTolerantWriteProtocolGuid,
> +                      SmmFtwNotificationEvent,
> +                      &SmmFtwRegistration
> +                      );
> +    ASSERT_EFI_ERROR (Status);
> +
> +    SmmFtwNotificationEvent (NULL, NULL, NULL);
> +  } else {
> +    //
> +    // Emulated non-volatile variable mode does not depend on FVB and FTW.
> +    //
> +    VariableWriteServiceInitializeSmm ();
> +  }
>  
>    return EFI_SUCCESS;
>  }
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index db7d220e06df..30d446d06e0d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -18,7 +18,7 @@
>  #  may not be modified without authorization. If platform fails to protect these resources,
>  #  the authentication service provided in this driver will be broken, and the behavior is undefined.
>  #
> -# Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
>  # This program and the accompanying materials
>  # are licensed and made available under the terms and conditions of the BSD License
>  # which accompanies this distribution. The full text of the license may be found at
> @@ -133,6 +133,8 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpaceSize           ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVariableSpaceSize  ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdReclaimVariableSpaceAtEndOfDxe   ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable          ## SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved       ## SOMETIMES_CONSUMES
>  
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics        ## CONSUMES  # statistic the information of variable.
>