Re: [PATCH V4 00/10] UEFI Variable SMI Reduction

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

That sounds good to me.

Thanks,
Michael

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Monday, October 14, 2019 5:50 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH V4 00/10] UEFI Variable SMI Reduction
> 
> Michael:
>   I add this feature into edk2-stable2019011 tag planning. Is it ok to you?
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Kubacki, Michael A
> >Sent: Tuesday, October 15, 2019 7:30 AM
> >To: devel@edk2.groups.io
> >Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
> ><ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Laszlo
> >Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
> >Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> >Yao, Jiewen <jiewen.yao@intel.com>
> >Subject: [PATCH V4 00/10] UEFI Variable SMI Reduction
> >
> >REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
> >
> >V4 Changes:
> > [PATCH V3 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache
> >support
> > * Set
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >to FALSE
> >   by default in MdeModulePkg.dec.
> >
> > * Added a new patch to set
> >gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >   to TRUE at the end of the patch series. This allows someone to
> >bisect an issue at
> >   patch #7 or patch #8 in the series with no change in variable caching
> behavior.
> >The
> >   runtime cache variable logic would be applied explicitly in V4 patch #10.
> >
> >V3 Changes:
> > [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing
> >functions
> > * Removed GUIDs added to VariableStandaloneMm.inf that are not
> required.
> > * Added more details to the commit message describing the criteria of
> >   moving the chosen functions to VariableParsing.c.
> >
> > [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> GetNextVariableEx()
> >store list
> > * RenamedGetNextVariableEx () to
> >VariableServiceGetNextVariableInternal ()
> > * Updated comments in VariableServiceGetNextVariableInternal () to
> >refer to
> >   "FindVariablEx ()" instead of "FindVariable ()" since FindVariableEx ()
> >   is not used in the function.
> >
> > [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize
> VARIABLE_INFO_ENTRY
> >buffer
> > * Updated the commit message to clarify the message "structure can be
> >updated
> >   outside the fixed global variable".
> >
> > [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth
> >status in VariableParsing
> > * Remove the function InitVariableParsing ()
> > * Remove the mAuthFormat global variable and instead add a BOOLEAN
> >parameter
> >   to all functions that require variable authentication information  to
> >   indicate if authenticated variables are used.
> >   * This allows the authenticated variable status to be tracked in one place
> in
> >     each variable driver in the SMM variable solution
> (VariableSmmRuntimeDxe
> >     and VariableSmm).
> >
> > [edk2-devel] [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV
> >variable functions
> > * Added the following non-volatile related functions to
> VariableNonVolatile.c
> >   from Variable.c:
> >   * InitRealNonVolatileVariableStore ()
> >   * InitEmuNonVolatileVariableStore ()
> >   * InitNonVolatileVariableStore ()
> >
> > [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT
> >GetVariable() cache support
> > * Added a FeaturePCD to control enabling the runtime variable cache -
> >   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache.
> > * Removed usage of the TimerLib and the wait to acquire
> >   mVariableRuntimeCacheReadLock. Can rely on the UEFI specification
> >   restrictions on Runtime Services callers.
> > * Removed the EFIAPI keyword from internal functions.
> > * Removed PCDs in VariableSmmRuntimeDxe.inf not required.
> > * Removed the HobVariableBackupBase variable no longer required.
> > * Renamed SynchronizeRuntimeVariableCacheEx () to better reflect usage.
> > * Renamed functions in VariableRuntimeCache.c to better reflect usage.
> > * Introduced a local variable in
> FlushPendingRuntimeVariableCacheUpdates ()
> >   to reduce duplication of
> >   mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.
> > * Corrected the macro used in SmmVariableHandler () to
> >
> SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT.
> > * Remove usage of the
> EDKII_PI_SMM_COMMUNICATION_REGION_TABLE
> >to acquire a
> >   CommBuffer from the EFI System Table and use the same runtime
> >CommBuffer
> >   allocated for variable SMM communicate calls.
> >
> > [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName()
> >cache support
> > * Removed usage of the TimerLib and the wait to acquire
> >   mVariableRuntimeCacheReadLock. Can rely on the UEFI specification
> >restrictions
> >   on Runtime Services callers.
> >
> > * Added a new patch to disable
> >gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >   for all OvmfPkg package builds as requested by maintainer Laszlo Ersek.
> >
> >V2 Changes:
> >
> >Patch #1 in V1 both moved functions to VariableParsing.c and modified
> >some functionality in those functions. In V2, the functions are first
> >moved and then functionality is modified in subsequent patches. This
> >resulted in the following new patches in the V2 patch series:
> >
> > 1. MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list
> > 2. MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer
> 3.
> > MdeModulePkg/Variable: Add local auth status in VariableParsing 4.
> > MdeModulePkg/Variable: Add a file for NV variable functions
> >
> >Apart from this refactor in the patches, no functionally impacting
> >changes were made.
> >
> >Overview
> >---------
> >This patch series reduces SMM usage when using VariableSmmRuntimeDxe
> >with VariableSmm. It does so by eliminating SMM usage for runtime
> >service GetVariable () and GetNextVariableName () invocations. Most
> >UEFI variable usage in typical systems after the variable store is
> >initialized (e.g. manufacturing boots) is due to GetVariable ( ) and
> >GetNextVariableName () not SetVariable (). GetVariable () calls can
> >regularly exceed 100 per boot while SetVariable () calls typically
> >remain less than 10 per boot. By focusing on the common case, the
> >majority of overhead associated with SMM can be avoided while still
> >using existing and proven code for operations such as variable
> >authentication that require an isolated execution environment.
> >
> > * Advantage: Reduces overall system SMM usage
> > * Disadvantage: Requires more Runtime data memory usage
> >
> >The runtime cache behavior described for this patch series is enabled
> >by default but can be disabled with a new FeaturePCD added to
> >MdeModulePkg.dec:
> >  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> >
> >The reaminder of this blurb describes the changes and behavior when the
> >PCD is set to TRUE.
> >
> >Initial Performance Observations
> >---------------------------------
> > * With these proposed changes, an Intel Atom based SoC saw GetVariable
> ( )
> >   time for an existing variable reduce from ~220us to ~5us.
> >
> >Major Changes
> >--------------
> > 1. Two UEFI variable caches will be maintained.
> >     a. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to
> >serve
> >         runtime service GetVariable () and GetNextVariableName () callers.
> >     b. "SMM cache" - Maintained in VariableSmm to service SMM
> GetVariable ()
> >         and GetNextVariableName () callers.
> >         i. A cache in SMRAM is retained so SMM modules do not operate on
> data
> >            outside SMRAM.
> > 2. A new UEFI variable read and write flow will be used as described below.
> >
> >At any given time, the two caches would be coherent. On a variable
> >write, the runtime cache is only updated after validation in SMM and,
> >in the case of a non-volatile UEFI variable, the variable must also be
> >successfully written to non-volatile storage.
> >
> >Prior RFC Feedback Addressed
> >-----------------------------
> >RFC sent Sept. 5, 2019: https://edk2.groups.io/g/devel/message/46939
> >
> >1. UEFI variable data retrieval from a ring 0 buffer
> >
> >   A common concern with this proposed set of changes is the potential
> >security
> >   threat presented by serving runtime services callers from a ring 0 memory
> >   buffer of EfiRuntimeServicesData type. The conclusion was that this
> change
> >   does not fundamentally alter the attack surface. The UEFI variable
> Runtime
> >   Services are invoked from ring 0 and the data already travels through ring
> >   0 buffers (such as the SMM communicate buffer) to reach the caller. Even
> >   today if ring 0 is assumed to be malicious, the malicious code may keep
> one
> >   AP in a loop to monitor the communication data, when the BSP gets an
> >   (authenticated) variable. When the communication buffer is updated
> >and the
> >   status is set to EFI_SUCCESS, the AP may modify the communication
> buffer
> >   contents such the tampered data is returned to the BSP caller. Or an
> >   interrupt handler on the BSP may alter the communication buffer
> contents
> >   before the data is returned to the caller. In summary, this was not found
> to
> >   introduce any attack not possible today.
> >
> >2. VarCheckLib impact
> >
> >   VarCheckLib plays a role in SetVariable () calls. This patch series only
> >   changes GetVariable () behavior. Therefore, VarCheckLib is expected to
> >   have no impact due to these changes.
> >
> >Testing Performed
> >------------------
> >This code was tested with the master branch of edk2 on an Intel Kaby
> >Lake U and Intel Whiskey Lake U reference validation platform. The set
> >of tests performed included:
> >
> >1.  Boot from S5 to Windows 10 OS with SMM variables enabled and
> >    the variable runtime cache enabled.
> >2.  Boot from S5 to Ubuntu 18.04.1 LTS with SMM variable enabled
> >    and the variable runtime cache enabled.
> >3.  Boot from S5 to Windows 10 OS with SMM variables enabled and
> >    the variable runtime cache disabled.
> >4.  Boot from S5 to Ubuntu 18.04.1 LTS with SMM variable enabled
> >    and the variable runtime cache disabled.
> >5.  Boot from S5 to EFI shell with DXE variables enabled.
> >    (commit 2de1f611be broke OvmfPkgIa32X64 boot regardless of
> >     this patch series; testing without this commit was successful) 6.
> >Dump UEFI variable store at shell with dmpstore to verify contents.
> >7.  Dump NvStorage FV from SPI flash after boot to verify contents written.
> >8.  Dump UEFI variable statistics with VariableInfo at shell.
> >9.  Boot with emulated variables enabled.
> >10. Cycles of adding and deleting a UEFI variable to verify cache correctness.
> >11. Set OsIndications to stop at FW UI to verify cache load of non-volatile
> >    contents.
> >
> >Why Keep SMM on Variable Writes
> >--------------------------------
> > * SMM provides a ubiquitous isolated execution environment in x86 for
> >   authenticated UEFI variables.
> > * BIOS region SPI flash write restrictions to SMM in platforms today can
> >   be retained.
> >
> >Today's UEFI Variable Cache (for reference)
> >--------------------------------------------
> > * Maintained in SMRAM via VariableSmm.
> > * A "write-through" cache of variable data in the form of a UEFI variable
> >   store.
> > * Non-volatile and volatile variables are maintained in separate
> >buffers
> >  (variable stores).
> >
> >Runtime & SMM Cache Coherency
> >------------------------------
> >The non-volatile cache should always accurately reflect non-volatile
> >storage contents (done today) and the "SMM cache" and "Runtime cache"
> >should always be coherent on access. The runtime cache is updated by
> >VariableSmm.
> >
> >Updating both caches from within a SMM SetVariable () operation is
> >fairly straightforward but a race condition can occur if an SMI occurs
> >during the execution of runtime code reading from the runtime cache. To
> >handle this case, a runtime cache read lock is introduced that
> >explicitly moves pending updates from SMM to the runtime cache if an
> >SMM update occurs while the runtime cache is locked. Note that it is
> >not expected a Runtime services call will interrupt SMM processing
> >since all CPU cores rendezvous in SMM.
> >
> >New Key Elements for Coherence
> >-------------------------------
> >Runtime DXE (VariableSmmRuntimeDxe)
> > 1. RuntimeCacheReadLock - A global lock used to lock read access to the
> >                           runtime cache.
> > 2. RuntimeCachePendingUpdate - A global flag used to notify runtime
> >code of a
> >                                pending cache update in SMM.
> >
> >SMM (VariableSmm)
> > 1. FlushRuntimeCachePendingUpdate SMI - A SW SMI handler that
> >synchronizes
> >                                         the runtime cache buffer with the SMM
> >                                         cache buffer.
> >
> >Proposed Runtime DXE Read Flow
> >-------------------------------
> > 1. Acquire RuntimeCacheReadLock
> > 2. If RuntimeCachePendingUpdate flag (rare) is set then:
> >     2.a. Trigger FlushRuntimeCachePendingUpdate SMI
> >     2.b. Verify RuntimeCachePendingUpdate flag is cleared  3. Perform
> >read from RuntimeCache  4. Release RuntimeCacheReadLock
> >
> >Proposed FlushRuntimeCachePendingUpdate SMI
> >--------------------------------------------
> > 1. If RuntimeCachePendingUpdate flag is not set:
> >     1.a. Return
> > 2. Copy the data at RuntimeCachePendingOffset of
> >RuntimeCachePendingLength to
> >    RuntimeCache
> > 3. Clear the RuntimeCachePendingUpdate flag
> >
> >Proposed SMM Write Flow
> >------------------------
> > 1. Perform variable authentication and non-volatile write. If either fail,
> >    return an error to the caller.
> > 2. If RuntimeCacheReadLock is set then:
> >     2.a. Set RuntimeCachePendingUpdate flag
> >     2.b. Update RuntimeCachePendingOffset and
> >RuntimeCachePendingLength to
> >          cover the a superset of the pending chunk (for simplicity, the
> >          entire variable store is currently synchronized).
> >3. Else:
> >     3.a. Update RuntimeCache
> >4. Update SmmCache
> >     - Note: RT read cannot occur during SMI processing since all cores are
> >             locked in SMM.
> >
> >Cc: Dandan Bi <dandan.bi@intel.com>
> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >Cc: Eric Dong <eric.dong@intel.com>
> >Cc: Laszlo Ersek <lersek@redhat.com>
> >Cc: Liming Gao <liming.gao@intel.com>
> >Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >Cc: Ray Ni <ray.ni@intel.com>
> >Cc: Jian J Wang <jian.j.wang@intel.com>
> >Cc: Hao A Wu <hao.a.wu@intel.com>
> >Cc: Jiewen Yao <jiewen.yao@intel.com>
> >Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> >
> >Michael Kubacki (10):
> >  MdeModulePkg/Variable: Consolidate common parsing functions
> >  MdeModulePkg/Variable: Parameterize GetNextVariableInternal () stores
> >  MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer
> >  MdeModulePkg/Variable: Parameterize auth status in VariableParsing
> >  MdeModulePkg/Variable: Add a file for NV variable functions
> >  MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats
> >  MdeModulePkg/Variable: Add RT GetVariable() cache support
> >  MdeModulePkg/Variable: Add RT GetNextVariableName() cache support
> >  OvmfPkg: Disable variable runtime cache
> >  MdeModulePkg: Enable variable runtime cache by default
> >
> > MdeModulePkg/MdeModulePkg.dec                                        |   12 +
> > OvmfPkg/OvmfPkgIa32.dsc                                              |    1 +
> > OvmfPkg/OvmfPkgIa32X64.dsc                                           |    1 +
> > OvmfPkg/OvmfPkgX64.dsc                                               |    1 +
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> |
> >6 +
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf           |
> 6
> >+
> >
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.i
> >nf |   20 +-
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> >|    6 +
> > MdeModulePkg/Include/Guid/SmmVariableCommon.h                        |   29 +-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                |  151
> +--
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> |
> >67 +
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h         |
> 347
> >+++++
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h
> >|   51 +
> > MdeModulePkg/Application/VariableInfo/VariableInfo.c                 |   37 +-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c                | 1373
> >++++----------------
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c           |
> 24
> >+-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> |
> >334 +++++
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c         |
> 786
> >+++++++++++
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c
> >|  153 +++
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c             |
> 120
> >+-
> >
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
> c
> >|  655 +++++++++-
> > 21 files changed, 2851 insertions(+), 1329 deletions(-)  create mode
> >100644
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.h
> > create mode 100644
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > create mode 100644
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h
> > create mode 100644
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> > create mode 100644
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> > create mode 100644
> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c
> >
> >--
> >2.16.2.windows.1
>