That sounds good to me. Thanks, Michael
> -----Original Message----- > From: Gao, Liming <liming....@intel.com> > Sent: Monday, October 14, 2019 5:50 PM > To: Kubacki, Michael A <michael.a.kuba...@intel.com>; > 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>; 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: 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...@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: [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...@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/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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49017): https://edk2.groups.io/g/devel/message/49017 Mute This Topic: https://groups.io/mt/34540086/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
