Hello Michael, On 11/01/19 18:34, Michael Kubacki wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > V7 Changes: > [PATCH V6 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support > * Remove VariableRuntimeCache.c and VariableRuntimeCache.h from > VariableSmmRuntimeDxe.inf since they are not needed to build the module. > > V6 Changes: > [PATCH V5 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support > The most significant change is: > * Free mVariableRuntimeHobCacheBuffer in CheckForRuntimeCacheSync () in > VariableSmmRuntimeDxe.c with FreePages () instead of FreePool (). > This issue was not found in earlier testing because on the initial set of > platforms tested, the variable HOB flush was finished prior to the variable > HOB runtime cache buffer being allocated so the FreePages () call was not > executed. > > The remaining changes did not affect testing but are included for robustness: > * Pass EFI_OPTIONAL_PTR for the DebugDisposition type in the > EfiConvertPointer () > calls for mVariableRuntimeHobCacheBuffer, mVariableRuntimeNvCacheBuffer, > and > mVariableRuntimeVolatileCacheBuffer in VariableAddressChangeEvent () in > VariableSmmRuntimeDxe.c as these buffers will not be allocated if the > runtime > cache is disabled. > * In the SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT case in > SmmVariableHandler () in VariableSmm.c, explicitly verify that > VariableRuntimeHobCache.Store is not NULL in addition to checking that > VariableGlobal.HobVariableBase is not set to zero (variable HOB is flushed) > before writing to VariableRuntimeHobCache.Store. > > 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.
I gave my R-b for the OvmfPkg patch in the v4 posting: https://edk2.groups.io/g/devel/message/48979 (alternative link: b89583ed-06ef-ccd2-2e29-d054f581ea6a@redhat.com">http://mid.mail-archive.com/b89583ed-06ef-ccd2-2e29-d054f581ea6a@redhat.com ) In the v5 posting -- assuming you had not changed that specific OvmfPkg patch, relative to v4 -- you should have picked up my R-b, and carried it forward ever since (to the present v7). Basically, do a git-rebase, select the "reword" action for the patch, then cut&paste my R-b from my v4 review email to the bottom of the commit message. Then every further posting will contain it. If there *have* been changes to the patch, relative to v4, then it's indeed right to drop (or simply not pick up) my R-b. FWIW, the changelog above does not suggest the particular patch has seen any changes since v4. Thanks! Laszlo > > 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 | 18 > +- > 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 | 195 > ++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 655 > +++++++++- > 21 files changed, 2924 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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49873): https://edk2.groups.io/g/devel/message/49873 Mute This Topic: https://groups.io/mt/40450233/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
