From: "Laszlo Ersek" <lersek@redhat.com>
To: "Kubacki, Michael A" <michael.a.kubacki@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Wu, Hao A" <hao.a.wu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Bi, Dandan" <dandan.bi@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
"Dong, Eric" <eric.dong@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Ni, Ray" <ray.ni@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support
Date: Fri, 4 Oct 2019 08:50:40 +0200 [thread overview]
Message-ID: <d0c57218-5ff4-3875-f98e-bde154199808@redhat.com> (raw)
In-Reply-To: <DM6PR11MB38346D1FD8FF928E000492D8B59F0@DM6PR11MB3834.namprd11.prod.outlook.com>
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-javierm@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?
Thanks!
Laszlo
>
>> -----Original Message-----
>> From: Kinney, Michael D <michael.d.kinney@intel.com>
>> Sent: Thursday, October 3, 2019 3:01 PM
>> To: Kubacki, Michael A <michael.a.kubacki@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
>> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
>> <jiewen.yao@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.kubacki@intel.com>
>>> Sent: Thursday, October 3, 2019 2:54 PM
>>> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
>>> Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Dong, Eric
>>> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Ni, Ray
>>> <ray.ni@intel.com>; Wang, Jian J
>>> <jian.j.wang@intel.com>; Yao, Jiewen
>>> <jiewen.yao@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.wu@intel.com>
>>>> Sent: Thursday, October 3, 2019 1:05 AM
>>>> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
>>>> devel@edk2.groups.io
>>>> Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org>; Dong, Eric
>>> <eric.dong@intel.com>; Laszlo Ersek
>>>> <lersek@redhat.com>; Gao, Liming
>>> <liming.gao@intel.com>; Kinney, Michael
>>>> D <michael.d.kinney@intel.com>; Ni, Ray
>>> <ray.ni@intel.com>; Wang, Jian J
>>>> <jian.j.wang@intel.com>; Yao, Jiewen
>>> <jiewen.yao@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.bi@intel.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Signed-off-by: Michael Kubacki
>>> <michael.a.kubacki@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
>>>>
>>>
>>
>
next prev parent reply other threads:[~2019-10-04 6:50 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-28 1:47 [PATCH V2 0/9] UEFI Variable SMI Reduction Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing functions Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A
2019-10-03 17:35 ` Kubacki, Michael A
2019-10-08 2:11 ` Wu, Hao A
2019-10-08 21:53 ` Kubacki, Michael A
2019-10-08 6:07 ` Wang, Jian J
2019-10-08 22:00 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A
2019-10-03 18:04 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A
2019-10-03 18:05 ` Kubacki, Michael A
2019-10-08 2:11 ` [edk2-devel] " Wu, Hao A
2019-10-08 21:49 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Kubacki, Michael A
2019-10-03 8:04 ` [edk2-devel] " Wu, Hao A
2019-10-03 18:35 ` Kubacki, Michael A
2019-10-16 7:55 ` Wu, Hao A
2019-10-16 16:37 ` Kubacki, Michael A
2019-10-17 1:00 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV variable functions Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 18:43 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 6/9] MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 11:00 ` Laszlo Ersek
2019-10-03 20:53 ` Kubacki, Michael A
2019-10-03 21:53 ` Kubacki, Michael A
2019-10-03 22:01 ` Michael D Kinney
2019-10-03 23:31 ` Kubacki, Michael A
2019-10-04 6:50 ` Laszlo Ersek [this message]
2019-10-04 16:48 ` Kubacki, Michael A
2019-10-04 6:38 ` Laszlo Ersek
2019-10-04 16:48 ` Kubacki, Michael A
2019-10-08 2:12 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() " Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 18:52 ` Kubacki, Michael A
2019-10-03 18:59 ` [edk2-devel] " Andrew Fish
2019-10-03 20:12 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 9/9] MdeModulePkg/VariableSmm: Remove unused SMI handler functions Kubacki, Michael A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0c57218-5ff4-3875-f98e-bde154199808@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox