> On 10/04/19 01:31, Kubacki, Michael A wrote: > > I agree, I will make the default to enable the runtime cache. > > I've just made a request for the opposite :) , before reading this part > of the thread. > > Let me revise my request then, seeing the above preference. From the > following three options: > > (1) set DEC default to FALSE, > > (2) set the DEC default to TRUE, but set the PCD in OvmfPkg DSC files to > FALSE, > > (3) set the DEC default to TRUE, and inherit it in OvmfPkg, > > I'd ask for (2) in the short or mid term, and entertain (3) in the long > term (dependent on the upstream kernel patch I linked elsewhere in the > thread: > <http://mid.mail-archive.com/20191003100712.31045-1- > javi...@redhat.com>). > > > With the PCD available in the DEC file, I agree that downstream OVMF > packages could easily be built with the PCD set to FALSE, e.g. on the > "build" command line, regardless of the upstream OvmfPkg DSC setting. > Given that, option (2) might appear superfluous. > > However, I'd like upstream OvmfPkg's DSC setting to reflect that as yet, > there is no alternative to GetVariable(), for exercising the SMM stack > built into OVMF without side effects to the variable store. Once the > kernel patch is merged and QueryVariableInfo() becomes *recommendable* > as a practical substitute for GetVariable(), we can switch upstream > OvmfPkg from option (2) to option (3). > > Does that sound acceptable? >
I have no problem setting the PCD to FALSE in all the OvmfPkg DSC files. > Thanks! > Laszlo > > > > > >> -----Original Message----- > >> From: Kinney, Michael D <michael.d.kin...@intel.com> > >> Sent: Thursday, October 3, 2019 3:01 PM > >> To: Kubacki, Michael A <michael.a.kuba...@intel.com>; Wu, Hao A > >> <hao.a...@intel.com>; devel@edk2.groups.io; Kinney, Michael D > >> <michael.d.kin...@intel.com> > >> 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>; 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 > >> > >> Michael, > >> > >> Perhaps the FeaturePCD for #2 should be enabled by default > >> so the platform DSC only needs to set this PCD for some > >> validation tests. > >> > >> Mike > >> > >> > >>> -----Original Message----- > >>> From: Kubacki, Michael A <michael.a.kuba...@intel.com> > >>> Sent: Thursday, October 3, 2019 2:54 PM > >>> To: Wu, Hao A <hao.a...@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 > >>> > >>> #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.PcdVariableCollectStatist > >>> ics > >>>>> 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/VariableRunti > >>> meDxe.inf > >>>>> | 2 + > >>>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.i > >>> nf | > >>>> 2 > >>>>> + > >>>>> > >>>>> > >>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRu > >>> ntimeDxe.i > >>>>> nf | 31 +- > >>>>> > >>>>> > >>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStand > >>> aloneMm.inf > >>>>> | 2 + > >>>>> MdeModulePkg/Include/Guid/SmmVariableCommon.h > >>> | 29 +- > >>>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h > >>> | 39 > >>>> +- > >>>>> > >>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRunti > >>> meCache.h > >>>>> | 47 ++ > >>>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > >>> | 44 > >>>> +- > >>>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRunti > >>> meCache.c > >>>>> | 153 +++++ > >>>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > >>> | > >>>> 114 > >>>>> +++- > >>>>> > >>>>> > >>>> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRu > >>> ntimeDxe. > >>>>> c | 608 +++++++++++++++++--- > >>>>> 11 files changed, 966 insertions(+), 105 > >>> deletions(-) > >>>>> > >>>>> diff --git > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeDxe.inf > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeDxe.inf > >>>>> index 08a5490787..ceea5d1ff9 100644 > >>>>> --- > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeDxe.inf > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeDxe.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. > >>> > >>>>> 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/VariableSmm > >>> RuntimeDx > >>>>> e.inf > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> e.inf > >>>>> index 14894e6f13..70837ac6e0 100644 > >>>>> --- > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> e.inf > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> 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.PcdMaxHardwareErrorVariab > >>> leSize > >>>>> ## CONSUMES > >>>>> + > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize > >>> ## > >>>>> CONSUMES > >>>>> + > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize > >>> ## > >>>>> CONSUMES > >>>>> + > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxUserNvVariableSpace > >>> Size > >>>>> ## CONSUMES > >>>>> + > >>>>> > >>>> > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdBoottimeReservedNvVari > >>> ableSpace > >>>>> Size ## CONSUMES > >>>> > >>>> > >>>> Not sure if the above PCDs are really needed by > >>> VariableSmmRuntimeDxe. > >>>> > >>>> > >>> > >>> I will double check and remove any not required. > >>> > >>>>> + > >>>>> +[FeaturePcd] > >>>>> + > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableCollectStatist > >>> ics > >>>> ## > >>>>> 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/VariableSta > >>> ndaloneMm.i > >>>>> nf > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSta > >>> ndaloneMm. > >>>>> inf > >>>>> index ca9d23ce9f..95c5310c0b 100644 > >>>>> --- > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSta > >>> ndaloneMm.i > >>>>> nf > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSta > >>> ndaloneMm. > >>>>> 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_CONTEX > >>> T > >>>>> 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/VariableRun > >>> timeCache.h > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeCache. > >>>>> h > >>>>> new file mode 100644 > >>>>> index 0000000000..09b83eb215 > >>>>> --- /dev/null > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeCache. > >>>>> 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.VariableRunt > >>> imeNvCache, > >>>>> + (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.VariableRunt > >>> imeVolatileCach > >>>>> 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.VariableRunt > >>> imeNvCache), > >>>>> + 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.VariableRunt > >>> imeVolatileCach > >>>>> e); > >>>>> + } else { > >>>>> + VolatileCacheInstance = > >>> &(mVariableModuleGlobal- > >>>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache); > >>>>> + } > >>>>> + > >>>>> + 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.VariableRunt > >>> imeHobCache, > >>>>> + 0, > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.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.HobFlushComp > >>> lete) = > >>>> TRUE; > >>>>> if (!AtRuntime ()) { > >>>>> FreePool ((VOID *) VariableStoreHeader); > >>>>> } > >>>>> diff --git > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeCache.c > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeCache.c > >>>>> new file mode 100644 > >>>>> index 0000000000..2642d9b000 > >>>>> --- /dev/null > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRun > >>> timeCache.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.VariableRunt > >>> imeNvCache.St > >>>>> ore == NULL || > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.Store == NULL || > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.PendingUpdat > >>> e == NULL > >>>>> + ) { > >>>>> + return EFI_UNSUPPORTED; > >>>>> + } > >>>>> + > >>>>> + if (*(mVariableModuleGlobal- > >>>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.PendingUpdat > >>> e)) { > >>>>> + if ( > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.S > >>>>> tore != NULL && > >>>>> + mVariableModuleGlobal- > >>>> VariableGlobal.HobVariableBase > 0 > >>>>> + ) { > >>>>> + CopyMem ( > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.S > >>>>> tore) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.P > >>>>> endingUpdateOffset > >>>>> + ), > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mVariableModuleGlobal- > >>>>>> VariableGlobal.HobVariableBase) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.P > >>>>> endingUpdateOffset > >>>>> + ), > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.P > >>>>> endingUpdateLength > >>>>> + ); > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.P > >>>>> endingUpdateLength = 0; > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeHobCache.P > >>>>> endingUpdateOffset = 0; > >>>>> + } > >>>>> + > >>>>> + CopyMem ( > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.St > >>>>> ore) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.Pe > >>>>> ndingUpdateOffset > >>>>> + ), > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mNvVariableCache) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.Pe > >>>>> ndingUpdateOffset > >>>>> + ), > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.Pe > >>>>> ndingUpdateLength > >>>>> + ); > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.Pe > >>>>> ndingUpdateLength = 0; > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeNvCache.Pe > >>>>> ndingUpdateOffset = 0; > >>>>> + > >>>>> + CopyMem ( > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.Store) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.PendingUpdateOffset > >>>>> + ), > >>>>> + (VOID *) ( > >>>>> + ((UINT8 *) (UINTN) mVariableModuleGlobal- > >>>>>> VariableGlobal.VolatileVariableBase) + > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.PendingUpdateOffset > >>>>> + ), > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.PendingUpdateLength > >>>>> + ); > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.PendingUpdateLength = 0; > >>>>> + mVariableModuleGlobal- > >>>>> > >>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.VariableRunt > >>> imeVolatileCach > >>>>> e.PendingUpdateOffset = 0; > >>>>> + *(mVariableModuleGlobal- > >>>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.PendingUpdat > >>> e) = 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.PendingUpdat > >>> e == NULL > >>>> || > >>>>> + mVariableModuleGlobal- > >>>>>> VariableGlobal.VariableRuntimeCacheContext.ReadLock > >>> == NULL > >>>>> + ) { > >>>>> + return EFI_UNSUPPORTED; > >>>>> + } > >>>>> + > >>>>> + if ( > >>>>> + *(mVariableModuleGlobal- > >>>>> > >>>> VariableGlobal.VariableRuntimeCacheContext.PendingUpdat > >>> e) && > >>>>> + 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.PendingUpdat > >>> e) = 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_CONTEX > >>> T: > >>>>> + if (CommBufferPayloadSize < sizeof > >>>>> > >>>> > >>> > (SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTE > >>> XT) > >>>> ) > >>>>> { > >>>> > >>>> > >>>> 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? > >>> > >>>>> + 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/VariableSmm > >>> RuntimeDx > >>>>> e.c > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> e.c > >>>>> index 0a1888e5ef..46f69765a4 100644 > >>>>> --- > >>>>> > >>>> > >>> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> e.c > >>>>> +++ > >>>>> > >>>> > >>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm > >>> RuntimeDx > >>>>> 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. > >>> > >>>>> +{ > >>>>> + 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_CONTEX > >>> T; > >>>>> + 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 (#48480): https://edk2.groups.io/g/devel/message/48480 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] -=-=-=-=-=-=-=-=-=-=-=-
