For the whole patch series (1-10),
Reviewed-by: Jian J Wang <jian.j.w...@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kubacki,
> Michael A
> Sent: Friday, October 18, 2019 8:14 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan...@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Dong, Eric <eric.d...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; Gao, Liming <liming....@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Ni, Ray <ray...@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>
> Subject: [edk2-devel] [PATCH V5 00/10] UEFI Variable SMI Reduction
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220
>
> V5 Changes:
> [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support
> * Increased validation of the runtime buffers passed in the SMM comm buffer
> SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
> structure to the
> SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT
> function in
> SmmVariableHandler () in VariableSmm.c.
> * Most notably, each runtime buffer given is checked to ensure its memory
> range does not overlap with SMRAM ranges via
> VariableSmmIsBufferOutsideSmmValid ().
>
> 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...@intel.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Hao A Wu <hao.a...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kuba...@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/VariableSmmRuntimeDxe.inf |
> 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 | 196
> ++-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c |
> 655 +++++++++-
> 21 files changed, 2927 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
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49213): https://edk2.groups.io/g/devel/message/49213
Mute This Topic: https://groups.io/mt/34950246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
