Re: [edk2-devel] [PATCH 9/9] MdeModulePkg:Add global variable mVariableRtCacheInfo

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 13134 bytes --]

I didn't read through all changes.
2 comments:

  1.
Please do not remember to call ConvertPointer() for any pointers accessed at runtime. I think your patch contains 4 pointers in the mVariableRtCacheInfo structure.
  2.
Please do not call FreePages() to free a PEI-allocated runtime memory. It won't succeed as PEI and DXE memory services do not share one database.
     *
The FreePages() call should be removed in earlier patch when you let the Variable driver consume the PEI-allocated memory.

Thanks,
Ray
________________________________
From: Tan, Dun <dun.tan@intel.com>
Sent: Friday, May 17, 2024 17:49
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: [PATCH 9/9] MdeModulePkg:Add global variable mVariableRtCacheInfo

Add global variable mVariableRtCacheInfo to save the
content in gEdkiiVariableRuntimeCacheInfoHobGuid. With
this new global variable, 7 global variables can be
removed.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 97 +++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------
 1 file changed, 41 insertions(+), 56 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
index 6efe5cee10..de39462d68 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -44,26 +44,20 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "PrivilegePolymorphic.h"
 #include "VariableParsing.h"

-EFI_HANDLE                      mHandle                              = NULL;
-EFI_SMM_VARIABLE_PROTOCOL       *mSmmVariable                        = NULL;
-EFI_EVENT                       mVirtualAddressChangeEvent           = NULL;
-EFI_MM_COMMUNICATION2_PROTOCOL  *mMmCommunication2                   = 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;
+EFI_HANDLE                      mHandle                    = NULL;
+EFI_SMM_VARIABLE_PROTOCOL       *mSmmVariable              = NULL;
+EFI_EVENT                       mVirtualAddressChangeEvent = NULL;
+EFI_MM_COMMUNICATION2_PROTOCOL  *mMmCommunication2         = NULL;
+UINT8                           *mVariableBuffer           = NULL;
+UINT8                           *mVariableBufferPhysical   = NULL;
+VARIABLE_INFO_ENTRY             *mVariableInfo             = NULL;
 UINTN                           mVariableBufferSize;
-UINTN                           mVariableRuntimeHobCacheBufferSize;
 UINTN                           mVariableBufferPayloadSize;
-BOOLEAN                         *mVariableRuntimeCachePendingUpdate;
-BOOLEAN                         *mVariableRuntimeCacheReadLock;
 BOOLEAN                         mVariableAuthFormat;
-BOOLEAN                         *mHobFlushComplete;
 EFI_LOCK                        mVariableServicesLock;
 EDKII_VARIABLE_LOCK_PROTOCOL    mVariableLock;
 EDKII_VAR_CHECK_PROTOCOL        mVarCheck;
+VARIABLE_RUNTIME_CACHE_INFO     mVariableRtCacheInfo;

 /**
   The logic to initialize the VariablePolicy engine is in its own file.
@@ -500,21 +494,21 @@ CheckForRuntimeCacheSync (
   VOID
   )
 {
-  if (*mVariableRuntimeCachePendingUpdate) {
+  if (mVariableRtCacheInfo.CacheInfoFlag->PendingUpdate) {
     SyncRuntimeCache ();
   }

-  ASSERT (!(*mVariableRuntimeCachePendingUpdate));
+  ASSERT (!(mVariableRtCacheInfo.CacheInfoFlag->PendingUpdate));

   //
   // The HOB variable data may have finished being flushed in the runtime cache sync update
   //
-  if ((*mHobFlushComplete) && (mVariableRuntimeHobCacheBuffer != NULL)) {
+  if ((mVariableRtCacheInfo.CacheInfoFlag->HobFlushComplete) && (mVariableRtCacheInfo.RuntimeHobCacheBuffer != 0)) {
     if (!EfiAtRuntime ()) {
-      FreePages (mVariableRuntimeHobCacheBuffer, EFI_SIZE_TO_PAGES (mVariableRuntimeHobCacheBufferSize));
+      FreePages ((VOID *)(UINTN)mVariableRtCacheInfo.RuntimeHobCacheBuffer, mVariableRtCacheInfo.RuntimeHobCachePages);
     }

-    mVariableRuntimeHobCacheBuffer = NULL;
+    mVariableRtCacheInfo.RuntimeHobCacheBuffer = 0;
   }
 }

@@ -565,20 +559,20 @@ FindVariableInRuntimeCache (
   // a GetVariable () or GetNextVariable () call from being issued until a prior call has returned. The runtime
   // cache read lock should always be free when entering this function.
   //
-  ASSERT (!(*mVariableRuntimeCacheReadLock));
+  ASSERT (!(mVariableRtCacheInfo.CacheInfoFlag->ReadLock));

-  *mVariableRuntimeCacheReadLock = TRUE;
+  mVariableRtCacheInfo.CacheInfoFlag->ReadLock = TRUE;
   CheckForRuntimeCacheSync ();

-  if (!(*mVariableRuntimeCachePendingUpdate)) {
+  if (!(mVariableRtCacheInfo.CacheInfoFlag->PendingUpdate)) {
     //
     // 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;
+    VariableStoreList[VariableStoreTypeVolatile] = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeVolatileCacheBuffer;
+    VariableStoreList[VariableStoreTypeHob]      = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeHobCacheBuffer;
+    VariableStoreList[VariableStoreTypeNv]       = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeNvCacheBuffer;

     for (StoreType = (VARIABLE_STORE_TYPE)0; StoreType < VariableStoreTypeMax; StoreType++) {
       if (VariableStoreList[StoreType] == NULL) {
@@ -630,7 +624,7 @@ Done:
     }
   }

-  *mVariableRuntimeCacheReadLock = FALSE;
+  mVariableRtCacheInfo.CacheInfoFlag->ReadLock = FALSE;

   return Status;
 }
@@ -853,20 +847,20 @@ GetNextVariableNameInRuntimeCache (
   // a GetVariable () or GetNextVariable () call from being issued until a prior call has returned. The runtime
   // cache read lock should always be free when entering this function.
   //
-  ASSERT (!(*mVariableRuntimeCacheReadLock));
+  ASSERT (!(mVariableRtCacheInfo.CacheInfoFlag->ReadLock));

   CheckForRuntimeCacheSync ();

-  *mVariableRuntimeCacheReadLock = TRUE;
-  if (!(*mVariableRuntimeCachePendingUpdate)) {
+  mVariableRtCacheInfo.CacheInfoFlag->ReadLock = TRUE;
+  if (!(mVariableRtCacheInfo.CacheInfoFlag->PendingUpdate)) {
     //
     // 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.
     //
-    VariableStoreHeader[VariableStoreTypeVolatile] = mVariableRuntimeVolatileCacheBuffer;
-    VariableStoreHeader[VariableStoreTypeHob]      = mVariableRuntimeHobCacheBuffer;
-    VariableStoreHeader[VariableStoreTypeNv]       = mVariableRuntimeNvCacheBuffer;
+    VariableStoreHeader[VariableStoreTypeVolatile] = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeVolatileCacheBuffer;
+    VariableStoreHeader[VariableStoreTypeHob]      = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeHobCacheBuffer;
+    VariableStoreHeader[VariableStoreTypeNv]       = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeNvCacheBuffer;

     Status =  VariableServiceGetNextVariableInternal (
                 VariableName,
@@ -890,7 +884,7 @@ GetNextVariableNameInRuntimeCache (
     }
   }

-  *mVariableRuntimeCacheReadLock = FALSE;
+  mVariableRtCacheInfo.CacheInfoFlag->ReadLock = FALSE;

   return Status;
 }
@@ -1345,9 +1339,9 @@ VariableAddressChangeEvent (
 {
   EfiConvertPointer (0x0, (VOID **)&mVariableBuffer);
   EfiConvertPointer (0x0, (VOID **)&mMmCommunication2);
-  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRuntimeHobCacheBuffer);
-  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRuntimeNvCacheBuffer);
-  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRuntimeVolatileCacheBuffer);
+  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRtCacheInfo.RuntimeHobCacheBuffer);
+  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRtCacheInfo.RuntimeNvCacheBuffer);
+  EfiConvertPointer (EFI_OPTIONAL_PTR, (VOID **)&mVariableRtCacheInfo.RuntimeVolatileCacheBuffer);
 }

 /**
@@ -1576,17 +1570,10 @@ InitVariableCache (
       (AllocatedVolatileCacheSize >= ExpectedVolatileCacheSize)
       );

-    mVariableRuntimeHobCacheBuffer      = (VARIABLE_STORE_HEADER *)(UINTN)VariableRuntimeCacheHob->RuntimeHobCacheBuffer;
-    mVariableRuntimeNvCacheBuffer       = (VARIABLE_STORE_HEADER *)(UINTN)VariableRuntimeCacheHob->RuntimeNvCacheBuffer;
-    mVariableRuntimeVolatileCacheBuffer = (VARIABLE_STORE_HEADER *)(UINTN)VariableRuntimeCacheHob->RuntimeVolatileCacheBuffer;
-    mVariableRuntimeCachePendingUpdate  = &VariableRuntimeCacheHob->CacheInfoFlag->PendingUpdate;
-    mVariableRuntimeCacheReadLock       = &VariableRuntimeCacheHob->CacheInfoFlag->ReadLock;
-    mHobFlushComplete                   = &VariableRuntimeCacheHob->CacheInfoFlag->HobFlushComplete;
-    mVariableRuntimeHobCacheBufferSize  = AllocatedHobCacheSize;
-
-    InitVariableStoreHeader (mVariableRuntimeHobCacheBuffer, AllocatedHobCacheSize);
-    InitVariableStoreHeader (mVariableRuntimeNvCacheBuffer, AllocatedNvCacheSize);
-    InitVariableStoreHeader (mVariableRuntimeVolatileCacheBuffer, AllocatedVolatileCacheSize);
+    CopyMem (&mVariableRtCacheInfo, VariableRuntimeCacheHob, sizeof (VARIABLE_RUNTIME_CACHE_INFO));
+    InitVariableStoreHeader ((VOID *)(UINTN)mVariableRtCacheInfo.RuntimeHobCacheBuffer, AllocatedHobCacheSize);
+    InitVariableStoreHeader ((VOID *)(UINTN)mVariableRtCacheInfo.RuntimeNvCacheBuffer, AllocatedNvCacheSize);
+    InitVariableStoreHeader ((VOID *)(UINTN)mVariableRtCacheInfo.RuntimeVolatileCacheBuffer, AllocatedVolatileCacheSize);
   }

   return Status;
@@ -1637,12 +1624,12 @@ SendRuntimeVariableCacheContextToSmm (
   SmmVariableFunctionHeader->Function = SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT;
   SmmRuntimeVarCacheContext           = (SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT *)SmmVariableFunctionHeader->Data;

-  SmmRuntimeVarCacheContext->RuntimeHobCache      = mVariableRuntimeHobCacheBuffer;
-  SmmRuntimeVarCacheContext->RuntimeVolatileCache = mVariableRuntimeVolatileCacheBuffer;
-  SmmRuntimeVarCacheContext->RuntimeNvCache       = mVariableRuntimeNvCacheBuffer;
-  SmmRuntimeVarCacheContext->PendingUpdate        = mVariableRuntimeCachePendingUpdate;
-  SmmRuntimeVarCacheContext->ReadLock             = mVariableRuntimeCacheReadLock;
-  SmmRuntimeVarCacheContext->HobFlushComplete     = mHobFlushComplete;
+  SmmRuntimeVarCacheContext->RuntimeHobCache      = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeHobCacheBuffer;
+  SmmRuntimeVarCacheContext->RuntimeVolatileCache = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeVolatileCacheBuffer;
+  SmmRuntimeVarCacheContext->RuntimeNvCache       = (VARIABLE_STORE_HEADER *)(UINTN)mVariableRtCacheInfo.RuntimeNvCacheBuffer;
+  SmmRuntimeVarCacheContext->PendingUpdate        = &mVariableRtCacheInfo.CacheInfoFlag->PendingUpdate;
+  SmmRuntimeVarCacheContext->ReadLock             = &mVariableRtCacheInfo.CacheInfoFlag->ReadLock;
+  SmmRuntimeVarCacheContext->HobFlushComplete     = &mVariableRtCacheInfo.CacheInfoFlag->HobFlushComplete;

   //
   // Send data to SMM.
@@ -1712,9 +1699,7 @@ SmmVariableReady (
     }

     if (EFI_ERROR (Status)) {
-      mVariableRuntimeHobCacheBuffer      = NULL;
-      mVariableRuntimeNvCacheBuffer       = NULL;
-      mVariableRuntimeVolatileCacheBuffer = NULL;
+      ZeroMem (&mVariableRtCacheInfo, sizeof (VARIABLE_RUNTIME_CACHE_INFO));
     }

     ASSERT_EFI_ERROR (Status);
--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119048): https://edk2.groups.io/g/devel/message/119048
Mute This Topic: https://groups.io/mt/106150807/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 23565 bytes --]