Inline replies below:
> -----Original Message----- > From: Kubacki, Michael A > Sent: Friday, October 04, 2019 5:54 AM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; Kinney, > Michael D; Ni, Ray; Wang, Jian J; Yao, Jiewen > Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() > cache support > > #1 - The plan is to remove the polling entirely in V3. > > #2 - I'd prefer to take a definitive direction and reduce validation and > maintenance > effort but you and Laszlo both requested this so I'll add a > FeaturePCD to > control > activation of the runtime cache in this patch series. Perhaps this > can be > removed > in the future. > > #3 - Will be done in V3. > > Other replies are inline. > > > -----Original Message----- > > From: Wu, Hao A <hao.a...@intel.com> > > Sent: Thursday, October 3, 2019 1:05 AM > > 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>; 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>; Yao, Jiewen <jiewen....@intel.com> > > Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() > > cache support > > > > Before any comment on the patch, since I am not experienced in the > > Variable > > driver, I would like to ask for help from other reviewers to look into this > > patch and provide feedbacks as well. Thanks in advance. > > > > With the above fact, some comments provided below maybe wrong. So > > please help > > to kindly correct me. > > > > > > Some general comments: > > 1. I am not sure if bringing the TimerLib dependency (delay in acquiring the > > runtime cache read lock) to variable driver (a software driver for the > > most > > part) is a good idea. > > > > Hope other reviewers can provide some feedbacks for this. Thanks in > > advance. > > > > 2. In my opinion, I prefer a switch can be provided for platform owners to > > choose between using the runtime cache and going through SMM for > > GetVariable > > (and for GetNextVariableName in the next patch as well). > > > > If platform owners feel uncomfortable with using the runtime cache with > > regard to the security perspective, they can switch to the origin > > solution. > > > > 3. Please help to remove the 'EFIAPI' keyword for new driver internal > > functions; > > > > > > Inline comments below: > > > > > > > -----Original Message----- > > > From: Kubacki, Michael A > > > Sent: Saturday, September 28, 2019 9:47 AM > > > To: devel@edk2.groups.io > > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; > > Kinney, > > > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > > > Subject: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() > > > cache support > > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2220 > > > > > > This change reduces SMIs for GetVariable () by maintaining a > > > UEFI variable cache in Runtime DXE in addition to the pre- > > > existing cache in SMRAM. When the Runtime Service GetVariable() > > > is invoked, a Runtime DXE cache is used instead of triggering an > > > SMI to VariableSmm. This can improve overall system performance > > > by servicing variable read requests without rendezvousing all > > > cores into SMM. > > > > > > The following are important points regarding this change. > > > > > > 1. All of the non-volatile storage contents are loaded into the > > > cache upon driver load. This one time load operation from storage > > > is preferred as opposed to building the cache on demand. An on- > > > demand cache would require a fallback SMI to load data into the > > > cache as variables are requested. > > > > > > 2. SetVariable () requests will continue to always trigger an SMI. > > > This occurs regardless of whether the variable is volatile or > > > non-volatile. > > > > > > 3. Both volatile and non-volatile variables are cached in a runtime > > > buffer. As is the case in the current EDK II variable driver, they > > > continue to be cached in separate buffers. > > > > > > 4. The cache in Runtime DXE and SMM are intended to be exact copies > > > of one another. All SMM variable accesses only return data from the > > > SMM cache. The runtime caches are only updated after the variable I/O > > > operation is successful in SMM. The runtime caches are only updated > > > from SMM. > > > > > > 5. Synchronization mechanisms are in place to ensure the runtime cache > > > content integrity with the SMM cache. These may result in updates to > > > runtime cache that are the same in content but different in offset and > > > size from updates to the SMM cache. > > > > > > When using SMM variables, two caches will now be present. > > > 1. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to > > > service > > > Runtime Services GetVariable () and GetNextVariableName () callers. > > > 2. "SMM Cache" - Maintained in VariableSmm to service SMM > GetVariable > > () > > > and GetNextVariableName () callers. > > > a. This cache is retained so SMM modules do not operate on data > outside > > > SMRAM. > > > > > > It is possible to view UEFI variable read and write statistics by setting > > > the gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics > > > FeaturePcd > > > to TRUE and using the VariableInfo UEFI application in MdeModulePkg to > > > dump > > > variable statistics to the console. By doing so, a user can view the > > > number > > > of GetVariable () hits from the Runtime DXE variable driver (Runtime > Cache > > > hits) and the SMM variable driver (SMM Cache hits). SMM Cache hits for > > > GetVariable () will occur when SMM modules invoke GetVariable (). > > > > > > 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> > > > --- > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > > | 2 + > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > | > > 2 > > > + > > > > > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.i > > > nf | 31 +- > > > > > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf > > > | 2 + > > > MdeModulePkg/Include/Guid/SmmVariableCommon.h | > > > 29 > +- > > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | > > > 39 > > +- > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h > > > | 47 ++ > > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | > > > 44 > > +- > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c > > > | 153 +++++ > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | > > 114 > > > +++- > > > > > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe. > > > c | 608 +++++++++++++++++--- > > > 11 files changed, 966 insertions(+), 105 deletions(-) > > > > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > > index 08a5490787..ceea5d1ff9 100644 > > > --- > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > > +++ > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > > @@ -40,6 +40,8 @@ > > > VariableNonVolatile.h > > > VariableParsing.c > > > VariableParsing.h > > > + VariableRuntimeCache.c > > > + VariableRuntimeCache.h > > > > > > Per my understanding, the module specified by VariableRuntimeDxe.inf > > does not > > involve SMM/SMI for variable services (like GetVariable). It looks weird to > > me > > for this INF to include the newly introduced runtime cache codes (below > > source > > header files): > > > > VariableRuntimeCache.c > > VariableRuntimeCache.h > > > > > > This is because Variable.c is common to the runtime DXE and SMM variable > driver and it contains the code to update variable caches. The runtime cache > synchronization function (SynchronizeRuntimeVariableCache ()) will return > if the runtime cache pointer is NULL. Got it. I was previously thinking whether it is possible to drop such dependency for this non-SMM related runtime module. But if the dependency is coupled with Variable.c as well, I am fine with the change above. > > > > PrivilegePolymorphic.h > > > Measurement.c > > > TcgMorLockDxe.c > > > diff --git > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > > > index 6dc2721b81..bc3033588d 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf > > > @@ -49,6 +49,8 @@ > > > VariableNonVolatile.h > > > VariableParsing.c > > > VariableParsing.h > > > + VariableRuntimeCache.c > > > + VariableRuntimeCache.h > > > VarCheck.c > > > Variable.h > > > PrivilegePolymorphic.h > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.inf > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.inf > > > index 14894e6f13..70837ac6e0 100644 > > > --- > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.inf > > > +++ > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.inf > > > @@ -13,7 +13,7 @@ > > > # may not be modified without authorization. If platform fails to > > > protect > > > these resources, > > > # the authentication service provided in this driver will be broken, and > the > > > behavior is undefined. > > > # > > > -# Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR> > > > +# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> > > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > > # > > > ## > > > @@ -39,6 +39,10 @@ > > > VariableSmmRuntimeDxe.c > > > PrivilegePolymorphic.h > > > Measurement.c > > > + VariableParsing.c > > > + VariableParsing.h > > > + VariableRuntimeCache.c > > > + VariableRuntimeCache.h > > > > > > [Packages] > > > MdePkg/MdePkg.dec > > > @@ -49,6 +53,7 @@ > > > BaseLib > > > UefiBootServicesTableLib > > > DebugLib > > > + TimerLib > > > UefiRuntimeLib > > > DxeServicesTableLib > > > UefiDriverEntryPoint > > > @@ -65,7 +70,29 @@ > > > gEdkiiVariableLockProtocolGuid ## PRODUCES > > > gEdkiiVarCheckProtocolGuid ## PRODUCES > > > > > > +[Pcd] > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize > > > ## > > > CONSUMES > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize > > ## > > > CONSUMES > > > + > gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize > > > ## CONSUMES > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize > > > ## > > > CONSUMES > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize > ## > > > CONSUMES > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpaceSize > > > ## CONSUMES > > > + > > > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVariableSpace > > > Size ## CONSUMES > > > > > > Not sure if the above PCDs are really needed by VariableSmmRuntimeDxe. > > > > > > I will double check and remove any not required. > > > > + > > > +[FeaturePcd] > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatistics > > ## > > > CONSUMES > > > + > > > [Guids] > > > + ## PRODUCES ## GUID # Signature of Variable store header > > > + ## CONSUMES ## GUID # Signature of Variable store header > > > + ## SOMETIMES_PRODUCES ## SystemTable > > > + gEfiAuthenticatedVariableGuid > > > + > > > + ## PRODUCES ## GUID # Signature of Variable store header > > > + ## CONSUMES ## GUID # Signature of Variable store header > > > + ## SOMETIMES_PRODUCES ## SystemTable > > > + gEfiVariableGuid > > > + > > > gEfiEventVirtualAddressChangeGuid ## CONSUMES ## Event > > > gEfiEventExitBootServicesGuid ## CONSUMES ## Event > > > ## CONSUMES ## GUID # Locate protocol > > > @@ -82,6 +109,8 @@ > > > ## SOMETIMES_CONSUMES ## Variable:L"dbt" > > > gEfiImageSecurityDatabaseGuid > > > > > > + gEdkiiPiSmmCommunicationRegionTableGuid ## > > > SOMETIMES_CONSUMES ## SystemTable > > > + > > > [Depex] > > > gEfiSmmCommunicationProtocolGuid > > > > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i > > > nf > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm. > > > inf > > > index ca9d23ce9f..95c5310c0b 100644 > > > --- > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i > > > nf > > > +++ > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm. > > > inf > > > @@ -49,6 +49,8 @@ > > > VariableNonVolatile.h > > > VariableParsing.c > > > VariableParsing.h > > > + VariableRuntimeCache.c > > > + VariableRuntimeCache.h > > > VarCheck.c > > > Variable.h > > > PrivilegePolymorphic.h > > > diff --git a/MdeModulePkg/Include/Guid/SmmVariableCommon.h > > > b/MdeModulePkg/Include/Guid/SmmVariableCommon.h > > > index c527a59891..ceef44dfd2 100644 > > > --- a/MdeModulePkg/Include/Guid/SmmVariableCommon.h > > > +++ b/MdeModulePkg/Include/Guid/SmmVariableCommon.h > > > @@ -1,7 +1,7 @@ > > > /** @file > > > The file defined some common structures used for communicating > > > between SMM variable module and SMM variable wrapper module. > > > > > > -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR> > > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR> > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > **/ > > > @@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #ifndef _SMM_VARIABLE_COMMON_H_ > > > #define _SMM_VARIABLE_COMMON_H_ > > > > > > +#include <Guid/VariableFormat.h> > > > #include <Protocol/VarCheck.h> > > > > > > #define EFI_SMM_VARIABLE_WRITE_GUID \ > > > @@ -66,6 +67,16 @@ typedef struct { > > > #define > > > SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET 10 > > > > > > #define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE 11 > > > +// > > > +// The payload for this function is > > > > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > > > +// > > > +#define > > > > SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT > > > 12 > > > + > > > +#define SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE > > 13 > > > +// > > > +// The payload for this function is > > > SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO > > > +// > > > +#define SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO > > > 14 > > > > > > /// > > > /// Size of SMM communicate header, without including the payload. > > > @@ -120,4 +131,20 @@ typedef struct { > > > UINTN VariablePayloadSize; > > > } SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE; > > > > > > +typedef struct { > > > + BOOLEAN *ReadLock; > > > + BOOLEAN *PendingUpdate; > > > + BOOLEAN *HobFlushComplete; > > > + VARIABLE_STORE_HEADER *RuntimeHobCache; > > > + VARIABLE_STORE_HEADER *RuntimeNvCache; > > > + VARIABLE_STORE_HEADER *RuntimeVolatileCache; > > > +} > > > > > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT; > > > + > > > +typedef struct { > > > + UINTN TotalHobStorageSize; > > > + UINTN TotalNvStorageSize; > > > + UINTN TotalVolatileStorageSize; > > > + BOOLEAN AuthenticatedVariableUsage; > > > +} SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO; > > > + > > > #endif // _SMM_VARIABLE_COMMON_H_ > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > > > index fb574b2e32..b9723c0250 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > > > @@ -57,6 +57,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > /// > > > #define ISO_639_2_ENTRY_SIZE 3 > > > > > > +/// > > > +/// The timeout to in 10us units to wait for the > > > +/// variable runtime cache read lock to be acquired. > > > +/// > > > +#define VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT 200000 > > > + > > > typedef enum { > > > VariableStoreTypeVolatile, > > > VariableStoreTypeHob, > > > @@ -64,6 +70,21 @@ typedef enum { > > > VariableStoreTypeMax > > > } VARIABLE_STORE_TYPE; > > > > > > +typedef struct { > > > + UINT32 PendingUpdateOffset; > > > + UINT32 PendingUpdateLength; > > > + VARIABLE_STORE_HEADER *Store; > > > +} VARIABLE_RUNTIME_CACHE; > > > + > > > +typedef struct { > > > + BOOLEAN *ReadLock; > > > + BOOLEAN *PendingUpdate; > > > + BOOLEAN *HobFlushComplete; > > > + VARIABLE_RUNTIME_CACHE VariableRuntimeHobCache; > > > + VARIABLE_RUNTIME_CACHE VariableRuntimeNvCache; > > > + VARIABLE_RUNTIME_CACHE VariableRuntimeVolatileCache; > > > +} VARIABLE_RUNTIME_CACHE_CONTEXT; > > > + > > > typedef struct { > > > VARIABLE_HEADER *CurrPtr; > > > // > > > @@ -79,14 +100,16 @@ typedef struct { > > > } VARIABLE_POINTER_TRACK; > > > > > > typedef struct { > > > - EFI_PHYSICAL_ADDRESS HobVariableBase; > > > - EFI_PHYSICAL_ADDRESS VolatileVariableBase; > > > - EFI_PHYSICAL_ADDRESS NonVolatileVariableBase; > > > - EFI_LOCK VariableServicesLock; > > > - UINT32 ReentrantState; > > > - BOOLEAN AuthFormat; > > > - BOOLEAN AuthSupport; > > > - BOOLEAN EmuNvMode; > > > + EFI_PHYSICAL_ADDRESS HobVariableBase; > > > + EFI_PHYSICAL_ADDRESS HobVariableBackupBase; > > > > > > I do not see any usage of the new field "HobVariableBackupBase". > > Could you help to double confirm? > > > > > > You are correct. I removed usage of this variable before sending the > patch series and the global variable here needs to be cleaned up. > > > > + EFI_PHYSICAL_ADDRESS VolatileVariableBase; > > > + EFI_PHYSICAL_ADDRESS NonVolatileVariableBase; > > > + VARIABLE_RUNTIME_CACHE_CONTEXT > VariableRuntimeCacheContext; > > > + EFI_LOCK VariableServicesLock; > > > + UINT32 ReentrantState; > > > + BOOLEAN AuthFormat; > > > + BOOLEAN AuthSupport; > > > + BOOLEAN EmuNvMode; > > > } VARIABLE_GLOBAL; > > > > > > typedef struct { > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.h > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache. > > > h > > > new file mode 100644 > > > index 0000000000..09b83eb215 > > > --- /dev/null > > > +++ > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache. > > > h > > > @@ -0,0 +1,47 @@ > > > +/** @file > > > + The common variable volatile store routines shared by the > > DXE_RUNTIME > > > variable > > > + module and the DXE_SMM variable module. > > > + > > > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#ifndef _VARIABLE_RUNTIME_CACHE_H_ > > > +#define _VARIABLE_RUNTIME_CACHE_H_ > > > + > > > +#include "Variable.h" > > > + > > > +/** > > > + Copies any pending updates to runtime variable caches. > > > + > > > + @retval EFI_UNSUPPORTED The volatile store to be updated is not > > > initialized properly. > > > + @retval EFI_SUCCESS The volatile store was updated > > > successfully. > > > + > > > +**/ > > > +EFI_STATUS > > > +SynchronizeRuntimeVariableCacheEx ( > > > + VOID > > > + ); > > > + > > > +/** > > > + Synchronizes the runtime variable caches with all pending updates > > outside > > > runtime. > > > + > > > + Ensures all conditions are met to maintain coherency for runtime cache > > > updates. > > > + > > > + @param[in] VariableRuntimeCache Variable runtime cache structure > for > > > the runtime cache being synchronized. > > > + @param[in] Offset Offset in bytes to apply the update. > > > + @param[in] Length Length of data in bytes of the update. > > > + > > > + @retval EFI_UNSUPPORTED The volatile store to be updated is not > > > initialized properly. > > > + @retval EFI_SUCCESS The volatile store was updated > > > successfully. > > > + > > > +**/ > > > +EFI_STATUS > > > +SynchronizeRuntimeVariableCache ( > > > + IN VARIABLE_RUNTIME_CACHE *VariableRuntimeCache, > > > + IN UINTN Offset, > > > + IN UINTN Length > > > + ); > > > + > > > +#endif > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > index 5da2354aa5..bb2fa3fc19 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > @@ -25,6 +25,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #include "Variable.h" > > > #include "VariableNonVolatile.h" > > > #include "VariableParsing.h" > > > +#include "VariableRuntimeCache.h" > > > > > > VARIABLE_MODULE_GLOBAL *mVariableModuleGlobal; > > > > > > @@ -332,6 +333,12 @@ RecordVarErrorFlag ( > > > // Update the data in NV cache. > > > // > > > *VarErrFlag = TempFlag; > > > + Status = SynchronizeRuntimeVariableCache ( > > > + &mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCach > e, > > > + (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN) > > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase, > > > + sizeof (TempFlag) > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > } > > > } > > > } > > > @@ -755,12 +762,24 @@ Reclaim ( > > > > > > Done: > > > if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) > { > > > + Status = SynchronizeRuntimeVariableCache ( > > > + &mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e, > > > + 0, > > > + VariableStoreHeader->Size > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > FreePool (ValidBuffer); > > > } else { > > > // > > > // For NV variable reclaim, we use mNvVariableCache as the buffer, so > > > copy the data back. > > > // > > > - CopyMem (mNvVariableCache, (UINT8 *)(UINTN)VariableBase, > > > VariableStoreHeader->Size); > > > + CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase, > > > VariableStoreHeader->Size); > > > + Status = SynchronizeRuntimeVariableCache ( > > > + &(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCach > e), > > > + 0, > > > + VariableStoreHeader->Size > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > } > > > > > > return Status; > > > @@ -1592,6 +1611,7 @@ UpdateVariable ( > > > VARIABLE_POINTER_TRACK *Variable; > > > VARIABLE_POINTER_TRACK NvVariable; > > > VARIABLE_STORE_HEADER *VariableStoreHeader; > > > + VARIABLE_RUNTIME_CACHE *VolatileCacheInstance; > > > UINT8 *BufferForMerge; > > > UINTN MergedBufSize; > > > BOOLEAN DataReady; > > > @@ -2235,6 +2255,21 @@ UpdateVariable ( > > > } > > > > > > Done: > > > + if (!EFI_ERROR (Status)) { > > > + if (Variable->Volatile) { > > > + VolatileCacheInstance = &(mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e); > > > + } else { > > > + VolatileCacheInstance = &(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCach > e); > > > + } > > > + > > > + Status = SynchronizeRuntimeVariableCache ( > > > + VolatileCacheInstance, > > > + 0, > > > + VolatileCacheInstance->Store->Size > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > + } > > > + > > > return Status; > > > } > > > > > > @@ -3409,6 +3444,12 @@ FlushHobVariableToFlash ( > > > ErrorFlag = TRUE; > > > } > > > } > > > + Status = SynchronizeRuntimeVariableCache ( > > > + &mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache, > > > + 0, > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .S > > > tore->Size > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > if (ErrorFlag) { > > > // > > > // We still have HOB variable(s) not flushed in flash. > > > @@ -3419,6 +3460,7 @@ FlushHobVariableToFlash ( > > > // All HOB variables have been flushed in flash. > > > // > > > DEBUG ((EFI_D_INFO, "Variable driver: all HOB variables have been > > > flushed in flash.\n")); > > > + *(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.HobFlushComplete) = > > TRUE; > > > if (!AtRuntime ()) { > > > FreePool ((VOID *) VariableStoreHeader); > > > } > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c > > > new file mode 100644 > > > index 0000000000..2642d9b000 > > > --- /dev/null > > > +++ > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeCache.c > > > @@ -0,0 +1,153 @@ > > > +/** @file > > > + The common variable volatile store routines shared by the > > DXE_RUNTIME > > > variable > > > + module and the DXE_SMM variable module. > > > + > > > + Caution: This module requires additional review when modified. > > > + This driver will have external input - variable data. They may be > > > input in > > > SMM mode. > > > + This external input must be validated carefully to avoid security issue > like > > > + buffer overflow, integer overflow. > > > + > > > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > + > > > +**/ > > > + > > > +#include "VariableParsing.h" > > > +#include "VariableRuntimeCache.h" > > > + > > > +extern VARIABLE_MODULE_GLOBAL *mVariableModuleGlobal; > > > +extern VARIABLE_STORE_HEADER *mNvVariableCache; > > > + > > > +/** > > > + Copies any pending updates to runtime variable caches. > > > + > > > + @retval EFI_UNSUPPORTED The volatile store to be updated is not > > > initialized properly. > > > + @retval EFI_SUCCESS The volatile store was updated > > > successfully. > > > + > > > +**/ > > > +EFI_STATUS > > > +SynchronizeRuntimeVariableCacheEx ( > > > > > > It is not clear to me why this function is named as the "Ex" version of > function > > SynchronizeRuntimeVariableCache(). For me, this function looks more like > a > > basic > > version. > > > > I would suggest a name change for the functions provided in file > > VariableRuntimeCache.c to better reflect their usage model. > > > > > > I'll rename it in V3. > > > > + VOID > > > + ) > > > +{ > > > > > > I would recommend that at least a local variable should be introduced to > > reduce > > the duplications of: > > > > "mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext" > > > > in this function in order to make it easier to read. > > > > > > I'll add it in V3. > > > > + if ( > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > St > > > ore == NULL || > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.Store == NULL || > > > + mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate == NULL > > > + ) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (*(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate)) { > > > + if ( > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .S > > > tore != NULL && > > > + mVariableModuleGlobal->VariableGlobal.HobVariableBase > 0 > > > + ) { > > > + CopyMem ( > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .S > > > tore) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .P > > > endingUpdateOffset > > > + ), > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mVariableModuleGlobal- > > > >VariableGlobal.HobVariableBase) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .P > > > endingUpdateOffset > > > + ), > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .P > > > endingUpdateLength > > > + ); > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .P > > > endingUpdateLength = 0; > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeHobCache > .P > > > endingUpdateOffset = 0; > > > + } > > > + > > > + CopyMem ( > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > St > > > ore) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > Pe > > > ndingUpdateOffset > > > + ), > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mNvVariableCache) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > Pe > > > ndingUpdateOffset > > > + ), > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > Pe > > > ndingUpdateLength > > > + ); > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > Pe > > > ndingUpdateLength = 0; > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache. > Pe > > > ndingUpdateOffset = 0; > > > + > > > + CopyMem ( > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.Store) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.PendingUpdateOffset > > > + ), > > > + (VOID *) ( > > > + ((UINT8 *) (UINTN) mVariableModuleGlobal- > > > >VariableGlobal.VolatileVariableBase) + > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.PendingUpdateOffset > > > + ), > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.PendingUpdateLength > > > + ); > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.PendingUpdateLength = 0; > > > + mVariableModuleGlobal- > > > > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa > ch > > > e.PendingUpdateOffset = 0; > > > + *(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate) = FALSE; > > > + } > > > + > > > + return EFI_SUCCESS; > > > +} > > > + > > > +/** > > > + Synchronizes the runtime variable caches with all pending updates > > outside > > > runtime. > > > + > > > + Ensures all conditions are met to maintain coherency for runtime cache > > > updates. > > > + > > > + @param[in] VariableRuntimeCache Variable runtime cache structure > for > > > the runtime cache being synchronized. > > > + @param[in] Offset Offset in bytes to apply the update. > > > + @param[in] Length Length of data in bytes of the update. > > > + > > > + @retval EFI_UNSUPPORTED The volatile store to be updated is not > > > initialized properly. > > > + @retval EFI_SUCCESS The volatile store was updated > > > successfully. > > > + > > > +**/ > > > +EFI_STATUS > > > +SynchronizeRuntimeVariableCache ( > > > + IN VARIABLE_RUNTIME_CACHE *VariableRuntimeCache, > > > + IN UINTN Offset, > > > + IN UINTN Length > > > + ) > > > +{ > > > + if (VariableRuntimeCache == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } else if (VariableRuntimeCache->Store == NULL) { > > > + // Runtime cache is not available yet at this point, > > > + // Return EFI_SUCCESS instead of EFI_NOT_AVAILABLE_YET to let it > > > progress > > > + return EFI_SUCCESS; > > > + } > > > + > > > + if ( > > > + mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate == NULL > > || > > > + mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.ReadLock == NULL > > > + ) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if ( > > > + *(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate) && > > > + VariableRuntimeCache->PendingUpdateLength > 0 > > > + ) { > > > + VariableRuntimeCache->PendingUpdateLength = > > > + (UINT32) ( > > > + MAX ( > > > + (UINTN) (VariableRuntimeCache->PendingUpdateOffset + > > > VariableRuntimeCache->PendingUpdateLength), > > > + Offset + Length > > > + ) - MIN ((UINTN) VariableRuntimeCache->PendingUpdateOffset, > > Offset) > > > + ); > > > + VariableRuntimeCache->PendingUpdateOffset = > > > + (UINT32) MIN ((UINTN) VariableRuntimeCache- > > >PendingUpdateOffset, > > > Offset); > > > + } else { > > > + VariableRuntimeCache->PendingUpdateLength = (UINT32) Length; > > > + VariableRuntimeCache->PendingUpdateOffset = (UINT32) Offset; > > > + } > > > + *(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.PendingUpdate) = TRUE; > > > + > > > + if (*(mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext.ReadLock) == FALSE) { > > > + return SynchronizeRuntimeVariableCacheEx (); > > > + } > > > + > > > + return EFI_SUCCESS; > > > +} > > > diff --git > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > > > index ce409f22a3..8d767f75ac 100644 > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > > > @@ -31,6 +31,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #include <Guid/SmmVariableCommon.h> > > > #include "Variable.h" > > > #include "VariableParsing.h" > > > +#include "VariableRuntimeCache.h" > > > + > > > +extern VARIABLE_STORE_HEADER *mNvVariableCache; > > > > > > BOOLEAN mAtRuntime > > > = FALSE; > > > UINT8 > > > *mVariableBufferPayload = NULL; > > > @@ -451,25 +454,29 @@ SmmVariableGetStatistics ( > > > EFI_STATUS > > > EFIAPI > > > SmmVariableHandler ( > > > - IN EFI_HANDLE DispatchHandle, > > > - IN CONST VOID *RegisterContext, > > > - IN OUT VOID *CommBuffer, > > > - IN OUT UINTN *CommBufferSize > > > + IN EFI_HANDLE DispatchHandle, > > > + IN CONST VOID > > > *RegisterContext, > > > + IN OUT VOID *CommBuffer, > > > + IN OUT UINTN *CommBufferSize > > > ) > > > { > > > - EFI_STATUS Status; > > > - SMM_VARIABLE_COMMUNICATE_HEADER > > > *SmmVariableFunctionHeader; > > > - SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE > > > *SmmVariableHeader; > > > - SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME > > > *GetNextVariableName; > > > - SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO > > > *QueryVariableInfo; > > > - SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE > > > *GetPayloadSize; > > > - VARIABLE_INFO_ENTRY *VariableInfo; > > > - SMM_VARIABLE_COMMUNICATE_LOCK_VARIABLE > > *VariableToLock; > > > - SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY > > > *CommVariableProperty; > > > - UINTN InfoSize; > > > - UINTN NameBufferSize; > > > - UINTN CommBufferPayloadSize; > > > - UINTN TempCommBufferSize; > > > + EFI_STATUS Status; > > > + SMM_VARIABLE_COMMUNICATE_HEADER > > > *SmmVariableFunctionHeader; > > > + SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE > > > *SmmVariableHeader; > > > + SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME > > > *GetNextVariableName; > > > + SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO > > > *QueryVariableInfo; > > > + SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE > > > *GetPayloadSize; > > > + > > > > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > > > *RuntimeVariableCacheContext; > > > + SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO > > > *GetRuntimeCacheInfo; > > > + SMM_VARIABLE_COMMUNICATE_LOCK_VARIABLE > > > *VariableToLock; > > > + SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY > > > *CommVariableProperty; > > > + VARIABLE_INFO_ENTRY *VariableInfo; > > > + VARIABLE_RUNTIME_CACHE_CONTEXT > > > *VariableCacheContext; > > > + VARIABLE_STORE_HEADER *VariableCache; > > > + UINTN InfoSize; > > > + UINTN NameBufferSize; > > > + UINTN > > > CommBufferPayloadSize; > > > + UINTN > > > TempCommBufferSize; > > > > > > // > > > // If input is invalid, stop processing this SMI > > > @@ -789,6 +796,79 @@ SmmVariableHandler ( > > > ); > > > CopyMem (SmmVariableFunctionHeader->Data, > > mVariableBufferPayload, > > > CommBufferPayloadSize); > > > break; > > > + case > > > > > SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT: > > > + if (CommBufferPayloadSize < sizeof > > > > > > (SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT) > > ) > > > { > > > > > > The above check is not correct, I think it should be: > > > > if (CommBufferPayloadSize < sizeof > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT) > > ) { > > > > Please help to double confirm. > > Also, I recommend some security tests should be performed to these new > > cases in > > the variable SMI handler. > > > > > > You're right. The wrong macro was simply copied. > > > > + DEBUG ((DEBUG_ERROR, "InitRuntimeVariableCacheContext: SMM > > > communication buffer size invalid!\n")); > > > + } else if (mEndOfDxe) { > > > + DEBUG ((DEBUG_ERROR, "InitRuntimeVariableCacheContext: > Cannot > > > init context after end of DXE!\n")); > > > + } else { > > > + RuntimeVariableCacheContext = > > > > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > > > *) SmmVariableFunctionHeader->Data; > > > > > > Not sure on this one: > > > > Do you think it is necessary to copy the contents in the comm buffer to the > > pre-allocated SMM variable buffer payload 'mVariableBufferPayload' to > > avoid > > TOCTOU issue? Since there are some tests (sort of, a couple of ASSERTs) > > based > > on the comm buffer content. > > > > > > I understand the TOCTOU observation. But is this still a concern with all the > cores rendezvoused in SMM prior to end of DXE? Hello Jian and Jiewen, Could you help to provide your comments on this one? Thanks in advance. > > > > + VariableCacheContext = &mVariableModuleGlobal- > > > >VariableGlobal.VariableRuntimeCacheContext; > > > + > > > + ASSERT (RuntimeVariableCacheContext->RuntimeVolatileCache != > > > NULL); > > > + ASSERT (RuntimeVariableCacheContext->RuntimeNvCache != NULL); > > > + ASSERT (RuntimeVariableCacheContext->PendingUpdate != NULL); > > > + ASSERT (RuntimeVariableCacheContext->ReadLock != NULL); > > > + ASSERT (RuntimeVariableCacheContext->HobFlushComplete != > > NULL); > > > + > > > + VariableCacheContext->VariableRuntimeHobCache.Store = > > > RuntimeVariableCacheContext->RuntimeHobCache; > > > + VariableCacheContext->VariableRuntimeVolatileCache.Store = > > > RuntimeVariableCacheContext->RuntimeVolatileCache; > > > + VariableCacheContext->VariableRuntimeNvCache.Store = > > > RuntimeVariableCacheContext->RuntimeNvCache; > > > + VariableCacheContext->PendingUpdate = > > > RuntimeVariableCacheContext->PendingUpdate; > > > + VariableCacheContext->ReadLock = > > > RuntimeVariableCacheContext->ReadLock; > > > + VariableCacheContext->HobFlushComplete = > > > RuntimeVariableCacheContext->HobFlushComplete; > > > + > > > + // Set up the intial pending request since the RT cache needs to > > > be > in > > > sync with SMM cache > > > + if (mVariableModuleGlobal->VariableGlobal.HobVariableBase == 0) > { > > > + VariableCacheContext- > > > >VariableRuntimeHobCache.PendingUpdateOffset = 0; > > > + VariableCacheContext- > > > >VariableRuntimeHobCache.PendingUpdateLength = 0; > > > + } else { > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mVariableModuleGlobal->VariableGlobal.HobVariableBase; > > > + VariableCacheContext- > > > >VariableRuntimeHobCache.PendingUpdateOffset = 0; > > > + VariableCacheContext- > > > >VariableRuntimeHobCache.PendingUpdateLength = (UINT32) ((UINTN) > > > GetEndPointer (VariableCache) - (UINTN) VariableCache); > > > + CopyGuid (&(VariableCacheContext- > > > >VariableRuntimeHobCache.Store->Signature), &(VariableCache- > > > >Signature)); > > > + } > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mVariableModuleGlobal->VariableGlobal.VolatileVariableBase; > > > + VariableCacheContext- > > > >VariableRuntimeVolatileCache.PendingUpdateOffset = 0; > > > + VariableCacheContext- > > > >VariableRuntimeVolatileCache.PendingUpdateLength = (UINT32) > > ((UINTN) > > > GetEndPointer (VariableCache) - (UINTN) VariableCache); > > > + CopyGuid (&(VariableCacheContext- > > > >VariableRuntimeVolatileCache.Store->Signature), &(VariableCache- > > > >Signature)); > > > + > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mNvVariableCache; > > > + VariableCacheContext- > > > >VariableRuntimeNvCache.PendingUpdateOffset = 0; > > > + VariableCacheContext- > > > >VariableRuntimeNvCache.PendingUpdateLength = (UINT32) ((UINTN) > > > GetEndPointer (VariableCache) - (UINTN) VariableCache); > > > + CopyGuid (&(VariableCacheContext- > > >VariableRuntimeNvCache.Store- > > > >Signature), &(VariableCache->Signature)); > > > + > > > + *(VariableCacheContext->PendingUpdate) = TRUE; > > > + *(VariableCacheContext->ReadLock) = FALSE; > > > + *(VariableCacheContext->HobFlushComplete) = FALSE; > > > + } > > > + Status = EFI_SUCCESS; > > > + break; > > > + case SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE: > > > + Status = SynchronizeRuntimeVariableCacheEx (); > > > + break; > > > + case SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO: > > > + if (CommBufferPayloadSize < sizeof > > > (SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO)) { > > > + DEBUG ((DEBUG_ERROR, "GetRuntimeCacheInfo: SMM > > communication > > > buffer size invalid!\n")); > > > + return EFI_SUCCESS; > > > + } > > > + GetRuntimeCacheInfo = > > > (SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO *) > > > SmmVariableFunctionHeader->Data; > > > + > > > + if (mVariableModuleGlobal->VariableGlobal.HobVariableBase > 0) { > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mVariableModuleGlobal->VariableGlobal.HobVariableBase; > > > + GetRuntimeCacheInfo->TotalHobStorageSize = VariableCache->Size; > > > + } else { > > > + GetRuntimeCacheInfo->TotalHobStorageSize = 0; > > > + } > > > + > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mVariableModuleGlobal->VariableGlobal.VolatileVariableBase; > > > + GetRuntimeCacheInfo->TotalVolatileStorageSize = VariableCache- > > >Size; > > > + VariableCache = (VARIABLE_STORE_HEADER *) (UINTN) > > > mNvVariableCache; > > > + GetRuntimeCacheInfo->TotalNvStorageSize = (UINTN) > VariableCache- > > > >Size; > > > + GetRuntimeCacheInfo->AuthenticatedVariableUsage = > > > mVariableModuleGlobal->VariableGlobal.AuthFormat; > > > + > > > + Status = EFI_SUCCESS; > > > + break; > > > > > > default: > > > Status = EFI_UNSUPPORTED; > > > diff --git > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.c > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.c > > > index 0a1888e5ef..46f69765a4 100644 > > > --- > > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.c > > > +++ > > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > > > e.c > > > @@ -13,7 +13,7 @@ > > > > > > InitCommunicateBuffer() is really function to check the variable data > size. > > > > > > -Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR> > > > +Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR> > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > **/ > > > @@ -32,13 +32,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #include <Library/UefiRuntimeLib.h> > > > #include <Library/BaseMemoryLib.h> > > > #include <Library/DebugLib.h> > > > +#include <Library/TimerLib.h> > > > #include <Library/UefiLib.h> > > > #include <Library/BaseLib.h> > > > > > > #include <Guid/EventGroup.h> > > > +#include <Guid/PiSmmCommunicationRegionTable.h> > > > #include <Guid/SmmVariableCommon.h> > > > > > > #include "PrivilegePolymorphic.h" > > > +#include "VariableParsing.h" > > > > > > EFI_HANDLE mHandle = NULL; > > > EFI_SMM_VARIABLE_PROTOCOL *mSmmVariable = NULL; > > > @@ -46,8 +49,19 @@ EFI_EVENT > mVirtualAddressChangeEvent > > = > > > NULL; > > > EFI_SMM_COMMUNICATION_PROTOCOL *mSmmCommunication > = > > > NULL; > > > UINT8 *mVariableBuffer = NULL; > > > UINT8 *mVariableBufferPhysical = NULL; > > > +VARIABLE_INFO_ENTRY *mVariableInfo = NULL; > > > +VARIABLE_STORE_HEADER *mVariableRuntimeHobCacheBuffer > > = > > > NULL; > > > +VARIABLE_STORE_HEADER *mVariableRuntimeNvCacheBuffer > > = > > > NULL; > > > +VARIABLE_STORE_HEADER > *mVariableRuntimeVolatileCacheBuffer > > > = NULL; > > > UINTN mVariableBufferSize; > > > +UINTN mVariableRuntimeHobCacheBufferSize; > > > +UINTN mVariableRuntimeNvCacheBufferSize; > > > +UINTN mVariableRuntimeVolatileCacheBufferSize; > > > UINTN mVariableBufferPayloadSize; > > > +BOOLEAN mVariableRuntimeCachePendingUpdate; > > > +BOOLEAN mVariableRuntimeCacheReadLock; > > > +BOOLEAN mVariableAuthFormat; > > > +BOOLEAN mHobFlushComplete; > > > EFI_LOCK mVariableServicesLock; > > > EDKII_VARIABLE_LOCK_PROTOCOL mVariableLock; > > > EDKII_VAR_CHECK_PROTOCOL mVarCheck; > > > @@ -107,6 +121,73 @@ ReleaseLockOnlyAtBootTime ( > > > } > > > } > > > > > > +/** > > > + Return TRUE if ExitBootServices () has been called. > > > + > > > + @retval TRUE If ExitBootServices () has been called. > > > +**/ > > > +BOOLEAN > > > +AtRuntime ( > > > + VOID > > > + ) > > > > > > I think we can either: > > 1. Use EfiAtRuntime() for VariableSmmRuntimeDxe > > 2. Move AtRuntime() to VariableParsing.c so that the function can be > shared > > with VariableRuntimeDxe & VariableSmmRuntimeDxe. And then update > > the > > EfiAtRuntime() usages to AtRuntime() for VariableSmmRuntimeDxe. > > > > > > #1 will work fine. > > > > +{ > > > + return EfiAtRuntime (); > > > +} > > > + > > > +/** > > > + Initialize the variable cache buffer as an empty variable store. > > > + > > > + @param[out] VariableCacheBuffer A pointer to pointer of a cache > > > variable store. > > > + @param[in,out] TotalVariableCacheSize On input, the minimum size > > > needed for the UEFI variable store cache > > > + buffer that is allocated. On > > > output, the actual size > of > > > the buffer allocated. > > > + If TotalVariableCacheSize is > > > zero, a buffer will not > be > > > allocated and the > > > + function will return with > > > EFI_SUCCESS. > > > + > > > + @retval EFI_SUCCESS The variable cache was allocated and > > initialized > > > successfully. > > > + @retval EFI_INVALID_PARAMETER A given pointer is NULL or an > invalid > > > variable store size was specified. > > > + @retval EFI_OUT_OF_RESOURCES Insufficient resources are available > > to > > > allocate the variable store cache buffer. > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +InitVariableCache ( > > > + OUT VARIABLE_STORE_HEADER **VariableCacheBuffer, > > > + IN OUT UINTN *TotalVariableCacheSize > > > + ) > > > +{ > > > + VARIABLE_STORE_HEADER *VariableCacheStorePtr; > > > + > > > + if (TotalVariableCacheSize == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + if (*TotalVariableCacheSize == 0) { > > > + return EFI_SUCCESS; > > > + } > > > + if (VariableCacheBuffer == NULL || *TotalVariableCacheSize < sizeof > > > (VARIABLE_STORE_HEADER)) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + *TotalVariableCacheSize = ALIGN_VALUE (*TotalVariableCacheSize, > > sizeof > > > (UINT32)); > > > + > > > + // > > > + // Allocate NV Storage Cache and initialize it to all 1's (like an > > > erased FV) > > > + // > > > + *VariableCacheBuffer = (VARIABLE_STORE_HEADER *) > > > AllocateRuntimePages ( > > > + EFI_SIZE_TO_PAGES (*TotalVariableCacheSize) > > > + ); > > > + if (*VariableCacheBuffer == NULL) { > > > + return EFI_OUT_OF_RESOURCES; > > > + } > > > + VariableCacheStorePtr = *VariableCacheBuffer; > > > + SetMem32 ((VOID *) VariableCacheStorePtr, *TotalVariableCacheSize, > > > (UINT32) 0xFFFFFFFF); > > > + > > > + ZeroMem ((VOID *) VariableCacheStorePtr, sizeof > > > (VARIABLE_STORE_HEADER)); > > > + VariableCacheStorePtr->Size = (UINT32) *TotalVariableCacheSize; > > > + VariableCacheStorePtr->Format = VARIABLE_STORE_FORMATTED; > > > + VariableCacheStorePtr->State = VARIABLE_STORE_HEALTHY; > > > + > > > + return EFI_SUCCESS; > > > +} > > > + > > > /** > > > Initialize the communicate buffer using DataSize and Function. > > > > > > @@ -153,6 +234,69 @@ InitCommunicateBuffer ( > > > } > > > > > > > > > +/** > > > + Gets a SMM communicate buffer from the > > > EDKII_PI_SMM_COMMUNICATION_REGION_TABLE installed as an entry > in > > > the UEFI > > > + system configuration table. A generic SMM communication buffer DXE > > > driver may install the table or a custom table > > > + may be installed by a platform-specific driver. > > > + > > > + The communicate size is: SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + > > > + DataSize. > > > + > > > + @param[in,out] CommBufferSize On input, the minimum size > needed > > > for the communication buffer. > > > + On output, the SMM buffer size > > > available at > > CommBuffer. > > > + @param[out] CommBuffer A pointer to an SMM communication > > > buffer pointer. > > > + > > > + @retval EFI_SUCCESS The communication buffer was found > > > successfully. > > > + @retval EFI_INVALID_PARAMETER A given pointer is NULL or the > > > CommBufferSize is zero. > > > + @retval EFI_NOT_FOUND The > > > EDKII_PI_SMM_COMMUNICATION_REGION_TABLE was not found. > > > + @retval EFI_OUT_OF_RESOURCES A valid SMM communicate buffer > > for > > > the requested size is not available. > > > + > > > +**/ > > > +EFI_STATUS > > > +GetCommunicateBuffer ( > > > + IN OUT UINTN *CommBufferSize, > > > + OUT UINT8 **CommBuffer > > > + ) > > > > > > Minor comment: > > > > I found that the consumers of the above function are: > > GetRuntimeCacheInfo() > > SendRuntimeVariableCacheContextToSmm() > > > > Both of them get called within SmmVariableReady() when the SMM > variable > > driver > > finished initialization. I am wondering if they can simply use the pre- > allocated > > comm buffer (via InitCommunicateBuffer() and using 'mVariableBuffer'), > > instead > > of looking into the configuration table. > > > > In my opinion, this function can be dropped. > > > > > > I did that initially. It was recommended to use this method. After this patch, the driver will have 2 approaches for constructing the SMM communication buffer: A). Pre-allocate a buffer 'mVariableBuffer' as subsequent usage as SMM communication buffer via function calls InitCommunicateBuffer() and then SendCommunicateBuffer(); This approach is used by functions like: VariableLockRequestToLock() VarCheckVariablePropertySet() VarCheckVariablePropertyGet() RuntimeServiceGetVariable() RuntimeServiceGetNextVariableName() RuntimeServiceSetVariable() RuntimeServiceQueryVariableInfo() OnExitBootServices() OnReadyToBoot() B). Lookup in the system configuration table for memory ranges as SMM communication buffer. This approach is used by functions: GetRuntimeCacheInfo() SendRuntimeVariableCacheContextToSmm() Could you help to confirm with the people who gave you the recommendation that whether the driver should keep these 2 approaches or should align with 1 unified approach? Thanks. Best Regards, Hao Wu > > > > +{ > > > + EFI_STATUS Status; > > > + EDKII_PI_SMM_COMMUNICATION_REGION_TABLE > > > *PiSmmCommunicationRegionTable; > > > + EFI_MEMORY_DESCRIPTOR *Entry; > > > + UINTN EntrySize; > > > + UINT32 Index; > > > + > > > + if (CommBuffer == NULL || CommBufferSize == NULL || > > > *CommBufferSize == 0) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Status = EfiGetSystemConfigurationTable ( > > > + &gEdkiiPiSmmCommunicationRegionTableGuid, > > > + (VOID **) &PiSmmCommunicationRegionTable > > > + ); > > > + if (EFI_ERROR (Status) || PiSmmCommunicationRegionTable == NULL) { > > > + return EFI_NOT_FOUND; > > > + } > > > + > > > + Entry = (EFI_MEMORY_DESCRIPTOR *) > > (PiSmmCommunicationRegionTable > > > + 1); > > > + EntrySize = 0; > > > + for (Index = 0; Index < PiSmmCommunicationRegionTable- > > > >NumberOfEntries; Index++) { > > > + if (Entry->Type == EfiConventionalMemory) { > > > + EntrySize = EFI_PAGES_TO_SIZE ((UINTN) Entry->NumberOfPages); > > > + if (EntrySize >= *CommBufferSize) { > > > + break; > > > + } > > > + } > > > + Entry = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) Entry + > > > PiSmmCommunicationRegionTable->DescriptorSize); > > > + } > > > + > > > + if (Index < PiSmmCommunicationRegionTable->NumberOfEntries) { > > > + *CommBufferSize = EntrySize; > > > + *CommBuffer = (UINT8 *) (UINTN) Entry->PhysicalStart; > > > + return EFI_SUCCESS; > > > + } > > > + > > > + return EFI_OUT_OF_RESOURCES; > > > +} > > > + > > > /** > > > Send the data in communicate buffer to SMM. > > > > > > @@ -424,6 +568,171 @@ Done: > > > return Status; > > > } > > > > > > +/** > > > + Signals SMM to synchronize any pending variable updates with the > > > runtime cache(s). > > > + > > > +**/ > > > +VOID > > > +EFIAPI > > > +SyncRuntimeCache ( > > > + VOID > > > + ) > > > +{ > > > + // > > > + // Init the communicate buffer. The buffer data size is: > > > + // SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE. > > > + // > > > + InitCommunicateBuffer (NULL, 0, > > > SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE); > > > + > > > + // > > > + // Send data to SMM. > > > + // > > > + SendCommunicateBuffer (0); > > > +} > > > + > > > +/** > > > + Check whether a SMI must be triggered to retrieve pending cache > > updates. > > > + > > > + If the variable HOB was finished being flushed since the last check > > > for a > > > runtime cache update, this function > > > + will prevent the HOB cache from being used for future runtime cache > > hits. > > > + > > > +**/ > > > +VOID > > > +EFIAPI > > > +CheckForRuntimeCacheSync ( > > > + VOID > > > + ) > > > +{ > > > + if (mVariableRuntimeCachePendingUpdate) { > > > + SyncRuntimeCache (); > > > + } > > > + ASSERT (!mVariableRuntimeCachePendingUpdate); > > > + > > > + // > > > + // The HOB variable data may have finished being flushed in the > runtime > > > cache sync update > > > + // > > > + if (mHobFlushComplete && mVariableRuntimeHobCacheBuffer != > NULL) > > { > > > + if (!AtRuntime ()) { > > > + FreePool (mVariableRuntimeHobCacheBuffer); > > > + } > > > + mVariableRuntimeHobCacheBuffer = NULL; > > > + } > > > +} > > > + > > > +/** > > > + This code finds variable in a volatile memory store. > > > + > > > + Caution: This function may receive untrusted input. > > > + The data size is external input, so this function will validate it > > > carefully > to > > > avoid buffer overflow. > > > + > > > + @param[in] VariableName Name of Variable to be found. > > > + @param[in] VendorGuid Variable vendor GUID. > > > + @param[out] Attributes Attribute value of the variable > > > found. > > > + @param[in, out] DataSize Size of Data found. If size is less > > > than > the > > > + data, this value contains the > > > required size. > > > + @param[out] Data Data pointer. > > > + > > > + @retval EFI_SUCCESS Found the specified variable. > > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > > + @retval EFI_NOT_FOUND The specified variable could not be > > found. > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +FindVariableInRuntimeCache ( > > > + IN CHAR16 *VariableName, > > > + IN EFI_GUID *VendorGuid, > > > + OUT UINT32 *Attributes OPTIONAL, > > > + IN OUT UINTN *DataSize, > > > + OUT VOID *Data OPTIONAL > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + UINTN DelayIndex; > > > + UINTN TempDataSize; > > > + VARIABLE_POINTER_TRACK RtPtrTrack; > > > + VARIABLE_STORE_TYPE StoreType; > > > + VARIABLE_STORE_HEADER *VariableStoreList[VariableStoreTypeMax]; > > > + > > > + Status = EFI_NOT_FOUND; > > > + > > > + if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) > { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + for (DelayIndex = 0; mVariableRuntimeCacheReadLock && DelayIndex > < > > > VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT; DelayIndex++) { > > > + MicroSecondDelay (10); > > > + } > > > + if (DelayIndex < VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT) { > > > + ASSERT (!mVariableRuntimeCacheReadLock); > > > + > > > + mVariableRuntimeCacheReadLock = TRUE; > > > + CheckForRuntimeCacheSync (); > > > + > > > + if (!mVariableRuntimeCachePendingUpdate) { > > > + // > > > + // 0: Volatile, 1: HOB, 2: Non-Volatile. > > > + // The index and attributes mapping must be kept in this order as > > > FindVariable > > > + // makes use of this mapping to implement search algorithm. > > > + // > > > + VariableStoreList[VariableStoreTypeVolatile] = > > > mVariableRuntimeVolatileCacheBuffer; > > > + VariableStoreList[VariableStoreTypeHob] = > > > mVariableRuntimeHobCacheBuffer; > > > + VariableStoreList[VariableStoreTypeNv] = > > > mVariableRuntimeNvCacheBuffer; > > > + > > > + for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType < > > > VariableStoreTypeMax; StoreType++) { > > > + if (VariableStoreList[StoreType] == NULL) { > > > + continue; > > > + } > > > + > > > + RtPtrTrack.StartPtr = GetStartPointer > > > (VariableStoreList[StoreType]); > > > + RtPtrTrack.EndPtr = GetEndPointer > > > (VariableStoreList[StoreType]); > > > + RtPtrTrack.Volatile = (BOOLEAN) (StoreType == > > > VariableStoreTypeVolatile); > > > + > > > + Status = FindVariableEx (VariableName, VendorGuid, FALSE, > > > &RtPtrTrack); > > > + if (!EFI_ERROR (Status)) { > > > + break; > > > + } > > > + } > > > + > > > + if (!EFI_ERROR (Status)) { > > > + // > > > + // Get data size > > > + // > > > + TempDataSize = DataSizeOfVariable (RtPtrTrack.CurrPtr); > > > + ASSERT (TempDataSize != 0); > > > + > > > + if (*DataSize >= TempDataSize) { > > > + if (Data == NULL) { > > > + Status = EFI_INVALID_PARAMETER; > > > + goto Done; > > > + } > > > + > > > + CopyMem (Data, GetVariableDataPtr (RtPtrTrack.CurrPtr), > > > TempDataSize); > > > + if (Attributes != NULL) { > > > + *Attributes = RtPtrTrack.CurrPtr->Attributes; > > > + } > > > + > > > + *DataSize = TempDataSize; > > > + > > > + UpdateVariableInfo (VariableName, VendorGuid, > > RtPtrTrack.Volatile, > > > TRUE, FALSE, FALSE, TRUE, &mVariableInfo); > > > + > > > + Status = EFI_SUCCESS; > > > + goto Done; > > > + } else { > > > + *DataSize = TempDataSize; > > > + Status = EFI_BUFFER_TOO_SMALL; > > > + goto Done; > > > + } > > > + } > > > + } > > > + } > > > + > > > +Done: > > > + mVariableRuntimeCacheReadLock = FALSE; > > > > > > If timeout occurs when acquiring the read lock, should this flag be set to > > FALSE > > in such case? > > > > Please see reply to patch #8. > > > Best Regards, > > Hao Wu > > > > > > > + > > > + return Status; > > > +} > > > + > > > /** > > > This code finds variable in storage blocks (Volatile or Non-Volatile). > > > > > > @@ -454,91 +763,21 @@ RuntimeServiceGetVariable ( > > > ) > > > { > > > EFI_STATUS Status; > > > - UINTN PayloadSize; > > > - SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE > > > *SmmVariableHeader; > > > - UINTN TempDataSize; > > > - UINTN VariableNameSize; > > > > > > if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) > { > > > return EFI_INVALID_PARAMETER; > > > } > > > - > > > - TempDataSize = *DataSize; > > > - VariableNameSize = StrSize (VariableName); > > > - SmmVariableHeader = NULL; > > > - > > > - // > > > - // If VariableName exceeds SMM payload limit. Return failure > > > - // > > > - if (VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF > > > (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { > > > - return EFI_INVALID_PARAMETER; > > > - } > > > - > > > - AcquireLockOnlyAtBootTime(&mVariableServicesLock); > > > - > > > - // > > > - // Init the communicate buffer. The buffer data size is: > > > - // SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. > > > - // > > > - if (TempDataSize > mVariableBufferPayloadSize - OFFSET_OF > > > (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - > > > VariableNameSize) { > > > - // > > > - // If output data buffer exceed SMM payload limit. Trim output buffer > to > > > SMM payload size > > > - // > > > - TempDataSize = mVariableBufferPayloadSize - OFFSET_OF > > > (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - > > > VariableNameSize; > > > - } > > > - PayloadSize = OFFSET_OF > > > (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + > > > VariableNameSize + TempDataSize; > > > - > > > - Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, > > > PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE); > > > - if (EFI_ERROR (Status)) { > > > - goto Done; > > > - } > > > - ASSERT (SmmVariableHeader != NULL); > > > - > > > - CopyGuid (&SmmVariableHeader->Guid, VendorGuid); > > > - SmmVariableHeader->DataSize = TempDataSize; > > > - SmmVariableHeader->NameSize = VariableNameSize; > > > - if (Attributes == NULL) { > > > - SmmVariableHeader->Attributes = 0; > > > - } else { > > > - SmmVariableHeader->Attributes = *Attributes; > > > - } > > > - CopyMem (SmmVariableHeader->Name, VariableName, > > > SmmVariableHeader->NameSize); > > > - > > > - // > > > - // Send data to SMM. > > > - // > > > - Status = SendCommunicateBuffer (PayloadSize); > > > - > > > - // > > > - // Get data from SMM. > > > - // > > > - if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) { > > > - // > > > - // SMM CommBuffer DataSize can be a trimed value > > > - // Only update DataSize when needed > > > - // > > > - *DataSize = SmmVariableHeader->DataSize; > > > - } > > > - if (Attributes != NULL) { > > > - *Attributes = SmmVariableHeader->Attributes; > > > - } > > > - > > > - if (EFI_ERROR (Status)) { > > > - goto Done; > > > - } > > > - > > > - if (Data != NULL) { > > > - CopyMem (Data, (UINT8 *)SmmVariableHeader->Name + > > > SmmVariableHeader->NameSize, SmmVariableHeader->DataSize); > > > - } else { > > > - Status = EFI_INVALID_PARAMETER; > > > + if (VariableName[0] == 0) { > > > + return EFI_NOT_FOUND; > > > } > > > > > > -Done: > > > + AcquireLockOnlyAtBootTime (&mVariableServicesLock); > > > + Status = FindVariableInRuntimeCache (VariableName, VendorGuid, > > > Attributes, DataSize, Data); > > > ReleaseLockOnlyAtBootTime (&mVariableServicesLock); > > > + > > > return Status; > > > } > > > > > > - > > > /** > > > This code Finds the Next available variable. > > > > > > @@ -870,6 +1109,17 @@ OnReadyToBoot ( > > > // > > > SendCommunicateBuffer (0); > > > > > > + // > > > + // Install the system configuration table for variable info data > > > captured > > > + // > > > + if (FeaturePcdGet (PcdVariableCollectStatistics)) { > > > + if (mVariableAuthFormat) { > > > + gBS->InstallConfigurationTable (&gEfiAuthenticatedVariableGuid, > > > mVariableInfo); > > > + } else { > > > + gBS->InstallConfigurationTable (&gEfiVariableGuid, mVariableInfo); > > > + } > > > + } > > > + > > > gBS->CloseEvent (Event); > > > } > > > > > > @@ -893,6 +1143,9 @@ VariableAddressChangeEvent ( > > > { > > > EfiConvertPointer (0x0, (VOID **) &mVariableBuffer); > > > EfiConvertPointer (0x0, (VOID **) &mSmmCommunication); > > > + EfiConvertPointer (0x0, (VOID **) > &mVariableRuntimeHobCacheBuffer); > > > + EfiConvertPointer (0x0, (VOID **) &mVariableRuntimeNvCacheBuffer); > > > + EfiConvertPointer (0x0, (VOID **) > > > &mVariableRuntimeVolatileCacheBuffer); > > > } > > > > > > /** > > > @@ -969,6 +1222,173 @@ Done: > > > return Status; > > > } > > > > > > +/** > > > + This code gets information needed from SMM for runtime cache > > > initialization. > > > + > > > + @param[out] TotalHobStorageSize Output pointer for the total > HOB > > > storage size in bytes. > > > + @param[out] TotalNvStorageSize Output pointer for the total > > > non- > > > volatile storage size in bytes. > > > + @param[out] TotalVolatileStorageSize Output pointer for the total > > > volatile storage size in bytes. > > > + @param[out] AuthenticatedVariableUsage Output pointer that > indicates > > if > > > authenticated variables are to be used. > > > + > > > + @retval EFI_SUCCESS Retrieved the size > > > successfully. > > > + @retval EFI_INVALID_PARAMETER TotalNvStorageSize parameter > is > > > NULL. > > > + @retval EFI_OUT_OF_RESOURCES Could not allocate a > > CommBuffer. > > > + @retval Others Could not retrieve the size > > > successfully.; > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +GetRuntimeCacheInfo ( > > > + OUT UINTN *TotalHobStorageSize, > > > + OUT UINTN *TotalNvStorageSize, > > > + OUT UINTN *TotalVolatileStorageSize, > > > + OUT BOOLEAN *AuthenticatedVariableUsage > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO > > > *SmmGetRuntimeCacheInfo; > > > + EFI_SMM_COMMUNICATE_HEADER > > > *SmmCommunicateHeader; > > > + SMM_VARIABLE_COMMUNICATE_HEADER > > > *SmmVariableFunctionHeader; > > > + UINTN CommSize; > > > + UINTN CommBufferSize; > > > + UINT8 *CommBuffer; > > > + > > > + SmmGetRuntimeCacheInfo = NULL; > > > + CommBuffer = NULL; > > > + > > > + if (TotalHobStorageSize == NULL || TotalNvStorageSize == NULL || > > > TotalVolatileStorageSize == NULL || AuthenticatedVariableUsage == > NULL) > > { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + AcquireLockOnlyAtBootTime (&mVariableServicesLock); > > > + > > > + CommSize = SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + sizeof > > > (SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO); > > > + CommBufferSize = CommSize; > > > + Status = GetCommunicateBuffer (&CommBufferSize, &CommBuffer); > > > + if (EFI_ERROR (Status)) { > > > + goto Done; > > > + } > > > + if (CommBuffer == NULL) { > > > + Status = EFI_OUT_OF_RESOURCES; > > > + goto Done; > > > + } > > > + ZeroMem (CommBuffer, CommBufferSize); > > > + > > > + SmmCommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) > > > CommBuffer; > > > + CopyGuid (&SmmCommunicateHeader->HeaderGuid, > > > &gEfiSmmVariableProtocolGuid); > > > + SmmCommunicateHeader->MessageLength = > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + sizeof > > > (SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO); > > > + > > > + SmmVariableFunctionHeader = > > > (SMM_VARIABLE_COMMUNICATE_HEADER *) > SmmCommunicateHeader- > > > >Data; > > > + SmmVariableFunctionHeader->Function = > > > SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO; > > > + SmmGetRuntimeCacheInfo = > > > (SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO *) > > > SmmVariableFunctionHeader->Data; > > > + > > > + // > > > + // Send data to SMM. > > > + // > > > + Status = mSmmCommunication->Communicate > (mSmmCommunication, > > > CommBuffer, &CommSize); > > > + ASSERT_EFI_ERROR (Status); > > > + if (CommSize <= SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { > > > + Status = EFI_BAD_BUFFER_SIZE; > > > + goto Done; > > > + } > > > + > > > + Status = SmmVariableFunctionHeader->ReturnStatus; > > > + if (EFI_ERROR (Status)) { > > > + goto Done; > > > + } > > > + > > > + // > > > + // Get data from SMM. > > > + // > > > + *TotalHobStorageSize = SmmGetRuntimeCacheInfo- > > >TotalHobStorageSize; > > > + *TotalNvStorageSize = SmmGetRuntimeCacheInfo- > >TotalNvStorageSize; > > > + *TotalVolatileStorageSize = SmmGetRuntimeCacheInfo- > > > >TotalVolatileStorageSize; > > > + *AuthenticatedVariableUsage = SmmGetRuntimeCacheInfo- > > > >AuthenticatedVariableUsage; > > > + > > > +Done: > > > + ReleaseLockOnlyAtBootTime (&mVariableServicesLock); > > > + return Status; > > > +} > > > + > > > +/** > > > + Sends the runtime variable cache context information to SMM. > > > + > > > + @retval EFI_SUCCESS Retrieved the size successfully. > > > + @retval EFI_INVALID_PARAMETER TotalNvStorageSize parameter is > > > NULL. > > > + @retval EFI_OUT_OF_RESOURCES Could not allocate a CommBuffer. > > > + @retval Others Could not retrieve the size > > > successfully.; > > > + > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +SendRuntimeVariableCacheContextToSmm ( > > > + VOID > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + > > > > SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > > > *SmmRuntimeVarCacheContext; > > > + EFI_SMM_COMMUNICATE_HEADER > > > *SmmCommunicateHeader; > > > + SMM_VARIABLE_COMMUNICATE_HEADER > > > *SmmVariableFunctionHeader; > > > + UINTN CommSize; > > > + UINTN > > > CommBufferSize; > > > + UINT8 *CommBuffer; > > > + > > > + SmmRuntimeVarCacheContext = NULL; > > > + CommBuffer = NULL; > > > + > > > + AcquireLockOnlyAtBootTime (&mVariableServicesLock); > > > + > > > + // > > > + // Init the communicate buffer. The buffer data size is: > > > + // SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + sizeof > > > > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT) > > ; > > > + // > > > + CommSize = SMM_COMMUNICATE_HEADER_SIZE + > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + sizeof > > > > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT) > > ; > > > + CommBufferSize = CommSize; > > > + Status = GetCommunicateBuffer (&CommBufferSize, &CommBuffer); > > > + if (EFI_ERROR (Status)) { > > > + goto Done; > > > + } > > > + if (CommBuffer == NULL) { > > > + Status = EFI_OUT_OF_RESOURCES; > > > + goto Done; > > > + } > > > + ZeroMem (CommBuffer, CommBufferSize); > > > + > > > + SmmCommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) > > > CommBuffer; > > > + CopyGuid (&SmmCommunicateHeader->HeaderGuid, > > > &gEfiSmmVariableProtocolGuid); > > > + SmmCommunicateHeader->MessageLength = > > > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + sizeof > > > > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT) > > ; > > > + > > > + SmmVariableFunctionHeader = > > > (SMM_VARIABLE_COMMUNICATE_HEADER *) > SmmCommunicateHeader- > > > >Data; > > > + SmmVariableFunctionHeader->Function = > > > > > SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT; > > > + SmmRuntimeVarCacheContext = > > > > > > (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT > > > *) SmmVariableFunctionHeader->Data; > > > + > > > + SmmRuntimeVarCacheContext->RuntimeHobCache = > > > mVariableRuntimeHobCacheBuffer; > > > + SmmRuntimeVarCacheContext->RuntimeVolatileCache = > > > mVariableRuntimeVolatileCacheBuffer; > > > + SmmRuntimeVarCacheContext->RuntimeNvCache = > > > mVariableRuntimeNvCacheBuffer; > > > + SmmRuntimeVarCacheContext->PendingUpdate = > > > &mVariableRuntimeCachePendingUpdate; > > > + SmmRuntimeVarCacheContext->ReadLock = > > > &mVariableRuntimeCacheReadLock; > > > + SmmRuntimeVarCacheContext->HobFlushComplete = > > > &mHobFlushComplete; > > > + > > > + // > > > + // Send data to SMM. > > > + // > > > + Status = mSmmCommunication->Communicate > (mSmmCommunication, > > > CommBuffer, &CommSize); > > > + ASSERT_EFI_ERROR (Status); > > > + if (CommSize <= SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { > > > + Status = EFI_BAD_BUFFER_SIZE; > > > + goto Done; > > > + } > > > + > > > + Status = SmmVariableFunctionHeader->ReturnStatus; > > > + if (EFI_ERROR (Status)) { > > > + goto Done; > > > + } > > > + > > > +Done: > > > + ReleaseLockOnlyAtBootTime (&mVariableServicesLock); > > > + return Status; > > > +} > > > + > > > /** > > > Initialize variable service and install Variable Architectural > > > protocol. > > > > > > @@ -985,7 +1405,7 @@ SmmVariableReady ( > > > { > > > EFI_STATUS Status; > > > > > > - Status = gBS->LocateProtocol (&gEfiSmmVariableProtocolGuid, NULL, > > > (VOID **)&mSmmVariable); > > > + Status = gBS->LocateProtocol (&gEfiSmmVariableProtocolGuid, NULL, > > > (VOID **) &mSmmVariable); > > > if (EFI_ERROR (Status)) { > > > return; > > > } > > > @@ -1007,6 +1427,40 @@ SmmVariableReady ( > > > // > > > mVariableBufferPhysical = mVariableBuffer; > > > > > > + // > > > + // Allocate runtime variable cache memory buffers. > > > + // > > > + Status = GetRuntimeCacheInfo ( > > > + &mVariableRuntimeHobCacheBufferSize, > > > + &mVariableRuntimeNvCacheBufferSize, > > > + &mVariableRuntimeVolatileCacheBufferSize, > > > + &mVariableAuthFormat > > > + ); > > > + if (!EFI_ERROR (Status)) { > > > + Status = InitVariableCache (&mVariableRuntimeHobCacheBuffer, > > > &mVariableRuntimeHobCacheBufferSize); > > > + if (!EFI_ERROR (Status)) { > > > + Status = InitVariableCache (&mVariableRuntimeNvCacheBuffer, > > > &mVariableRuntimeNvCacheBufferSize); > > > + if (!EFI_ERROR (Status)) { > > > + Status = InitVariableCache (&mVariableRuntimeVolatileCacheBuffer, > > > &mVariableRuntimeVolatileCacheBufferSize); > > > + if (!EFI_ERROR (Status)) { > > > + Status = InitVariableParsing (mVariableAuthFormat); > > > + ASSERT_EFI_ERROR (Status); > > > + > > > + Status = SendRuntimeVariableCacheContextToSmm (); > > > + if (!EFI_ERROR (Status)) { > > > + SyncRuntimeCache (); > > > + } > > > + } > > > + } > > > + } > > > + if (EFI_ERROR (Status)) { > > > + mVariableRuntimeHobCacheBuffer = NULL; > > > + mVariableRuntimeNvCacheBuffer = NULL; > > > + mVariableRuntimeVolatileCacheBuffer = NULL; > > > + } > > > + } > > > + ASSERT_EFI_ERROR (Status); > > > + > > > gRT->GetVariable = RuntimeServiceGetVariable; > > > gRT->GetNextVariableName = RuntimeServiceGetNextVariableName; > > > gRT->SetVariable = RuntimeServiceSetVariable; > > > -- > > > 2.16.2.windows.1 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48524): https://edk2.groups.io/g/devel/message/48524 Mute This Topic: https://groups.io/mt/34318592/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
