Hi Laszlo, I did not make any changes to the OvmfPkg patch and I forgot to carry forward the R-b. I'll keep that in mind in the future.
For V7, I request the MdeModulePkg maintainers please add the R-b for the patches not changed. If this is not acceptable, I will be happy to send out a patch series with all of the R-b present. Thanks, Michael > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Friday, November 1, 2019 3:19 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>; 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: Re: [PATCH V7 00/10] UEFI Variable SMI Reduction > > 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: > > http://mid.mail-archive.com/b89583ed-06ef-ccd2-2e29- > d054f581e...@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.i > nf | 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 (#49876): https://edk2.groups.io/g/devel/message/49876 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] -=-=-=-=-=-=-=-=-=-=-=-
