[patch 0/2] MdeModulePkg/HiiDatabaseDxe: Make sure database update behaviors are atomic

[patch 0/2] MdeModulePkg/HiiDatabaseDxe: Make sure database update behaviors are atomic

[Dandan Bi]

The main purpose of this task is to make sure the operations
that update the HiiDatabase atomic the avoid the potential
risk that the one update operation with higher TPL may interrupt
another.

Patch 1 is to reorgnize the existing code logic and make it's
easy to add EfiAcquireLock/EfiReleaseLock function in patch 2.

Patch 2 is to add EfiAcquireLock/EfiReleaseLock function to
make sure the HiiDatabse update operations atomic.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Dandan Bi (2):
  MdeModulePkg/HiiDB: Reorganize codes of exporting HII settings
  MdeModulePkg/HiiDB: Make sure database update behaviors are atomic

 .../Universal/HiiDatabaseDxe/Database.c       | 128 ++++++++++++------
 .../Universal/HiiDatabaseDxe/HiiDatabase.h    |   8 +-
 .../HiiDatabaseDxe/HiiDatabaseEntry.c         |   3 +-
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c |  12 ++
 .../Universal/HiiDatabaseDxe/String.c         |   9 ++
 5 files changed, 112 insertions(+), 48 deletions(-)

-- 
2.18.0.windows.1

[patch 1/2] MdeModulePkg/HiiDB: Reorganize codes of exporting HII settings

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1235

Function "HiiGetConfigurationSetting" may be called
when HiiDatabase contents have been updated.
And it is just to call function "HiiGetDatabaseInfo" to
get the contents in HiiDatabase and call function
"HiiGetConfigRespInfo" and get the configuration response
string form HII drivers.

So here we can remove function "HiiGetConfigurationSetting"
and make caller to call "HiiGetDatabaseInfo" and
"HiiGetConfigRespInfo" directly.

And thus it also can distinguish which code blocks are to
operate HiiDatabase contents and which code blocks are not.
Then it's easy to know which code blocks should be atomic
when updating HiiDatabase contents.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 .../Universal/HiiDatabaseDxe/Database.c       | 75 ++++++++-----------
 .../Universal/HiiDatabaseDxe/HiiDatabase.h    |  6 +-
 .../HiiDatabaseDxe/HiiDatabaseEntry.c         |  3 +-
 3 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
index 664687796f..1e2724f126 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
@@ -19,11 +19,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 EFI_HII_PACKAGE_LIST_HEADER    *gRTDatabaseInfoBuffer = NULL;
 EFI_STRING                     gRTConfigRespBuffer    = NULL;
 UINTN                          gDatabaseInfoSize = 0;
 UINTN                          gConfigRespSize = 0;
-BOOLEAN                        gExportConfigResp = TRUE;
+BOOLEAN                        gExportConfigResp = FALSE;
 UINTN                          gNvDefaultStoreSize = 0;
 SKU_ID                         gSkuId              = 0xFFFFFFFFFFFFFFFF;
 LIST_ENTRY                     gVarStorageList     = INITIALIZE_LIST_HEAD_VARIABLE (gVarStorageList);
 
 /**
@@ -3433,43 +3433,10 @@ HiiGetDatabaseInfo(
 
   return EFI_SUCCESS;
 
 }
 
-/**
-This  function mainly use to get and update configuration settings information.
-
-@param  This                   A pointer to the EFI_HII_DATABASE_PROTOCOL instance.
-
-@retval EFI_SUCCESS            Get the information successfully.
-@retval EFI_OUT_OF_RESOURCES   Not enough memory to store the Configuration Setting data.
-
-**/
-EFI_STATUS
-HiiGetConfigurationSetting(
-  IN CONST EFI_HII_DATABASE_PROTOCOL        *This
-  )
-{
-  EFI_STATUS                          Status;
-
-  //
-  // Get the HiiDatabase info.
-  //
-  Status = HiiGetDatabaseInfo(This);
-
-  //
-  // Get ConfigResp string
-  //
-  if (gExportConfigResp) {
-    Status = HiiGetConfigRespInfo (This);
-    gExportConfigResp = FALSE;
-  }
-  return Status;
-
-}
-
-
 /**
   This function adds the packages in the package list to the database and returns a handle. If there is a
   EFI_DEVICE_PATH_PROTOCOL associated with the DriverHandle, then this function will
   create a package of type EFI_PACKAGE_TYPE_DEVICE_PATH and add it to the package list.
 
@@ -3559,15 +3526,23 @@ HiiNewPackageList (
   }
 
   *Handle = DatabaseRecord->Handle;
 
   //
-  // Check whether need to get the Database and configuration setting info.
+  // Check whether need to get the Database info.
   // Only after ReadyToBoot, need to do the export.
   //
   if (gExportAfterReadyToBoot) {
-    HiiGetConfigurationSetting(This);
+    HiiGetDatabaseInfo (This);
+  }
+
+  //
+  // Check whether need to get the configuration setting info from HII drivers.
+  // When after ReadyToBoot and need to do the export for form package add.
+  //
+  if (gExportAfterReadyToBoot && gExportConfigResp) {
+    HiiGetConfigRespInfo (This);
   }
 
   return EFI_SUCCESS;
 }
 
@@ -3672,15 +3647,23 @@ HiiRemovePackageList (
       FreePool (HiiHandle);
       FreePool (Node->PackageList);
       FreePool (Node);
 
       //
-      // Check whether need to get the Database and configuration setting info.
+      // Check whether need to get the Database info.
       // Only after ReadyToBoot, need to do the export.
       //
       if (gExportAfterReadyToBoot) {
-        HiiGetConfigurationSetting(This);
+        HiiGetDatabaseInfo (This);
+      }
+
+      //
+      // Check whether need to get the configuration setting info from HII drivers.
+      // When after ReadyToBoot and need to do the export for form package remove.
+      //
+      if (gExportAfterReadyToBoot && gExportConfigResp) {
+        HiiGetConfigRespInfo (This);
       }
       return EFI_SUCCESS;
     }
   }
 
@@ -3788,17 +3771,23 @@ HiiUpdatePackageList (
       // Add all of the packages within the new package list
       //
       Status = AddPackages (Private, EFI_HII_DATABASE_NOTIFY_ADD_PACK, PackageList, Node);
 
       //
-      // Check whether need to get the Database and configuration setting info.
+      // Check whether need to get the Database info.
       // Only after ReadyToBoot, need to do the export.
       //
-      if (gExportAfterReadyToBoot) {
-        if (Status == EFI_SUCCESS){
-          HiiGetConfigurationSetting(This);
-        }
+      if (gExportAfterReadyToBoot && Status == EFI_SUCCESS) {
+        HiiGetDatabaseInfo (This);
+      }
+
+      //
+      // Check whether need to get the configuration setting info from HII drivers.
+      // When after ReadyToBoot and need to do the export for form package update.
+      //
+      if (gExportAfterReadyToBoot && gExportConfigResp && Status == EFI_SUCCESS) {
+        HiiGetConfigRespInfo (This);
       }
 
       return Status;
     }
   }
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
index 4391625a9f..3a6eb8a55c 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
@@ -2327,25 +2327,25 @@ This function mainly use to get HiiDatabase information.
 @retval EFI_SUCCESS            Get the information successfully.
 @retval EFI_OUT_OF_RESOURCES   Not enough memory to store the Hiidatabase data.
 
 **/
 EFI_STATUS
-HiiGetDatabaseInfo(
+HiiGetDatabaseInfo (
   IN CONST EFI_HII_DATABASE_PROTOCOL        *This
   );
 
 /**
-This is an internal function,mainly use to get and update configuration settings information.
+This function mainly use to get and update ConfigResp string.
 
 @param  This                   A pointer to the EFI_HII_DATABASE_PROTOCOL instance.
 
 @retval EFI_SUCCESS            Get the information successfully.
 @retval EFI_OUT_OF_RESOURCES   Not enough memory to store the Configuration Setting data.
 
 **/
 EFI_STATUS
-HiiGetConfigurationSetting(
+HiiGetConfigRespInfo (
   IN CONST EFI_HII_DATABASE_PROTOCOL        *This
   );
 
 //
 // Global variables
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseEntry.c b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseEntry.c
index 5951b5e304..1a71798ac0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseEntry.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseEntry.c
@@ -142,11 +142,12 @@ OnReadyToBoot (
 {
   //
   // When ready to boot, we begin to export the HiiDatabase date.
   // And hook all the possible HiiDatabase change actions to export data.
   //
-  HiiGetConfigurationSetting(&mPrivate.HiiDatabase);
+  HiiGetDatabaseInfo (&mPrivate.HiiDatabase);
+  HiiGetConfigRespInfo (&mPrivate.HiiDatabase);
   gExportAfterReadyToBoot = TRUE;
 
   gBS->CloseEvent (Event);
 }
 
-- 
2.18.0.windows.1

[patch 2/2] MdeModulePkg/HiiDB: Make sure database update behaviors are atomic

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1235

When update contents in HiiDatabase, like:
1. Add/update/remove package list
2. Add/update string
3. Add/update image
We should make these operations atomic to prevent the
potential issue that the one update operation with
higher TPL may interrupt another.
This commit is to make the HiiDatabase update behaviors
atomic by adding EfiAcquireLock/EfiReleaseLock function.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 .../Universal/HiiDatabaseDxe/Database.c       | 53 ++++++++++++++++++-
 .../Universal/HiiDatabaseDxe/HiiDatabase.h    |  2 +
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 12 +++++
 .../Universal/HiiDatabaseDxe/String.c         |  9 ++++
 4 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
index 1e2724f126..85fd10b63b 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
@@ -24,10 +24,15 @@ UINTN                          gConfigRespSize = 0;
 BOOLEAN                        gExportConfigResp = FALSE;
 UINTN                          gNvDefaultStoreSize = 0;
 SKU_ID                         gSkuId              = 0xFFFFFFFFFFFFFFFF;
 LIST_ENTRY                     gVarStorageList     = INITIALIZE_LIST_HEAD_VARIABLE (gVarStorageList);
 
+//
+// HII database lock.
+//
+EFI_LOCK mHiiDatabaseLock = EFI_INITIALIZE_LOCK_VARIABLE(TPL_NOTIFY);
+
 /**
   This function generates a HII_DATABASE_RECORD node and adds into hii database.
   This is a internal function.
 
   @param  Private                hii database private structure
@@ -3491,24 +3496,28 @@ HiiNewPackageList (
         DatabaseRecord->DriverHandle == DriverHandle) {
       return EFI_INVALID_PARAMETER;
     }
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   //
   // Build a PackageList node
   //
   Status = GenerateHiiDatabaseRecord (Private, &DatabaseRecord);
   if (EFI_ERROR (Status)) {
+    EfiReleaseLock (&mHiiDatabaseLock);
     return Status;
   }
 
   //
   // Fill in information of the created Package List node
   // according to incoming package list.
   //
   Status = AddPackages (Private, EFI_HII_DATABASE_NOTIFY_NEW_PACK, PackageList, DatabaseRecord);
   if (EFI_ERROR (Status)) {
+    EfiReleaseLock (&mHiiDatabaseLock);
     return Status;
   }
 
   DatabaseRecord->DriverHandle = DriverHandle;
 
@@ -3532,12 +3541,21 @@ HiiNewPackageList (
   // Only after ReadyToBoot, need to do the export.
   //
   if (gExportAfterReadyToBoot) {
     HiiGetDatabaseInfo (This);
   }
+  EfiReleaseLock (&mHiiDatabaseLock);
 
   //
+  // Notes:
+  // HiiGetDatabaseInfo () will get the contents of HII data base,
+  // belong to the atomic behavior of Hii Database update.
+  // And since HiiGetConfigRespInfo () will get the configuration setting info from HII drivers
+  // we can not think it belong to the atomic behavior of Hii Database update.
+  // That's why EfiReleaseLock (&mHiiDatabaseLock) is callled before HiiGetConfigRespInfo ().
+  //
+
   // Check whether need to get the configuration setting info from HII drivers.
   // When after ReadyToBoot and need to do the export for form package add.
   //
   if (gExportAfterReadyToBoot && gExportConfigResp) {
     HiiGetConfigRespInfo (This);
@@ -3583,10 +3601,12 @@ HiiRemovePackageList (
 
   if (!IsHiiHandleValid (Handle)) {
     return EFI_NOT_FOUND;
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   Private = HII_DATABASE_DATABASE_PRIVATE_DATA_FROM_THIS (This);
 
   //
   // Get the packagelist to be removed.
   //
@@ -3600,38 +3620,46 @@ HiiRemovePackageList (
       // Call registered functions with REMOVE_PACK before removing packages
       // then remove them.
       //
       Status = RemoveGuidPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveFormPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveKeyboardLayoutPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveStringPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveFontPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveImagePackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveSimpleFontPackages (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
       Status = RemoveDevicePathPackage (Private, Handle, PackageList);
       if (EFI_ERROR (Status)) {
+        EfiReleaseLock (&mHiiDatabaseLock);
         return Status;
       }
 
       //
       // Free resources of the package list
@@ -3653,10 +3681,20 @@ HiiRemovePackageList (
       // Only after ReadyToBoot, need to do the export.
       //
       if (gExportAfterReadyToBoot) {
         HiiGetDatabaseInfo (This);
       }
+      EfiReleaseLock (&mHiiDatabaseLock);
+
+      //
+      // Notes:
+      // HiiGetDatabaseInfo () will get the contents of HII data base,
+      // belong to the atomic behavior of Hii Database update.
+      // And since HiiGetConfigRespInfo () will get the configuration setting info from HII drivers
+      // we can not think it belong to the atomic behavior of Hii Database update.
+      // That's why EfiReleaseLock (&mHiiDatabaseLock) is callled before HiiGetConfigRespInfo ().
+      //
 
       //
       // Check whether need to get the configuration setting info from HII drivers.
       // When after ReadyToBoot and need to do the export for form package remove.
       //
@@ -3665,10 +3703,11 @@ HiiRemovePackageList (
       }
       return EFI_SUCCESS;
     }
   }
 
+  EfiReleaseLock (&mHiiDatabaseLock);
   return EFI_NOT_FOUND;
 }
 
 
 /**
@@ -3717,10 +3756,11 @@ HiiUpdatePackageList (
 
   PackageHdrPtr = (EFI_HII_PACKAGE_HEADER *) ((UINT8 *) PackageList + sizeof (EFI_HII_PACKAGE_LIST_HEADER));
 
   Status = EFI_SUCCESS;
 
+  EfiAcquireLock (&mHiiDatabaseLock);
   //
   // Get original packagelist to be updated
   //
   for (Link = Private->DatabaseList.ForwardLink; Link != &Private->DatabaseList; Link = Link->ForwardLink) {
     Node = CR (Link, HII_DATABASE_RECORD, DatabaseEntry, HII_DATABASE_RECORD_SIGNATURE);
@@ -3758,10 +3798,11 @@ HiiUpdatePackageList (
           Status = RemoveDevicePathPackage (Private, Handle, OldPackageList);
           break;
         }
 
         if (EFI_ERROR (Status)) {
+          EfiReleaseLock (&mHiiDatabaseLock);
           return Status;
         }
 
         PackageHdrPtr = (EFI_HII_PACKAGE_HEADER *) ((UINT8 *) PackageHdrPtr + PackageHeader.Length);
         CopyMem (&PackageHeader, PackageHdrPtr, sizeof (EFI_HII_PACKAGE_HEADER));
@@ -3777,10 +3818,20 @@ HiiUpdatePackageList (
       // Only after ReadyToBoot, need to do the export.
       //
       if (gExportAfterReadyToBoot && Status == EFI_SUCCESS) {
         HiiGetDatabaseInfo (This);
       }
+      EfiReleaseLock (&mHiiDatabaseLock);
+
+      //
+      // Notes:
+      // HiiGetDatabaseInfo () will get the contents of HII data base,
+      // belong to the atomic behavior of Hii Database update.
+      // And since HiiGetConfigRespInfo () will get the configuration setting info from HII drivers
+      // we can not think it belong to the atomic behavior of Hii Database update.
+      // That's why EfiReleaseLock (&mHiiDatabaseLock) is callled before HiiGetConfigRespInfo ().
+      //
 
       //
       // Check whether need to get the configuration setting info from HII drivers.
       // When after ReadyToBoot and need to do the export for form package update.
       //
@@ -3789,11 +3840,11 @@ HiiUpdatePackageList (
       }
 
       return Status;
     }
   }
-
+  EfiReleaseLock (&mHiiDatabaseLock);
   return EFI_NOT_FOUND;
 }
 
 
 /**
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
index 3a6eb8a55c..154e36e88f 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
@@ -59,10 +59,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define BITMAP_LEN_1_BIT(Width, Height)  (((Width) + 7) / 8 * (Height))
 #define BITMAP_LEN_4_BIT(Width, Height)  (((Width) + 1) / 2 * (Height))
 #define BITMAP_LEN_8_BIT(Width, Height)  ((Width) * (Height))
 #define BITMAP_LEN_24_BIT(Width, Height) ((Width) * (Height) * 3)
 
+extern EFI_LOCK mHiiDatabaseLock;
+
 //
 // IFR data structure
 //
 // BASE_CR (a, IFR_DEFAULT_VALUE_DATA, Entry) to get the whole structure.
 
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index d1010820e0..71ebc559c0 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -647,10 +647,12 @@ HiiNewImage (
   PackageListNode = LocatePackageList (&Private->DatabaseList, PackageList);
   if (PackageListNode == NULL) {
     return EFI_NOT_FOUND;
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL) +
                  BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
 
   //
   // Get the image package in the package list,
@@ -669,10 +671,11 @@ HiiNewImage (
     //
     // Update the package's image block by appending the new block to the end.
     //
     ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize + NewBlockSize);
     if (ImageBlocks == NULL) {
+      EfiReleaseLock (&mHiiDatabaseLock);
       return EFI_OUT_OF_RESOURCES;
     }
     //
     // Copy the original content.
     //
@@ -702,10 +705,11 @@ HiiNewImage (
     // The specified package list does not contain image package.
     // Create one to add this image block.
     //
     ImagePackage = (HII_IMAGE_PACKAGE_INSTANCE *) AllocateZeroPool (sizeof (HII_IMAGE_PACKAGE_INSTANCE));
     if (ImagePackage == NULL) {
+      EfiReleaseLock (&mHiiDatabaseLock);
       return EFI_OUT_OF_RESOURCES;
     }
     //
     // Output the image id of the incoming image being inserted, which is the
     // first image block so that id is initially to one.
@@ -730,10 +734,11 @@ HiiNewImage (
     //
     ImagePackage->ImageBlockSize = NewBlockSize + sizeof (EFI_HII_IIBT_END_BLOCK);
     ImagePackage->ImageBlock = AllocateZeroPool (NewBlockSize + sizeof (EFI_HII_IIBT_END_BLOCK));
     if (ImagePackage->ImageBlock == NULL) {
       FreePool (ImagePackage);
+      EfiReleaseLock (&mHiiDatabaseLock);
       return EFI_OUT_OF_RESOURCES;
     }
     ImageBlocks = ImagePackage->ImageBlock;
 
     //
@@ -767,10 +772,12 @@ HiiNewImage (
   //
   if (gExportAfterReadyToBoot) {
     HiiGetDatabaseInfo(&Private->HiiDatabase);
   }
 
+  EfiReleaseLock (&mHiiDatabaseLock);
+
   return EFI_SUCCESS;
 }
 
 
 /**
@@ -1062,10 +1069,12 @@ HiiSetImage (
   CurrentImageBlock = GetImageIdOrAddress (ImagePackage->ImageBlock, &ImageId);
   if (CurrentImageBlock == NULL) {
     return EFI_NOT_FOUND;
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   //
   // Get the size of original image block. Use some common block code here
   // since the definition of some structures is the same.
   //
   switch (CurrentImageBlock->BlockType) {
@@ -1106,10 +1115,11 @@ HiiSetImage (
                      (UINT32) ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)->Bitmap.Width),
                      ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)->Bitmap.Height)
                      );
     break;
   default:
+    EfiReleaseLock (&mHiiDatabaseLock);
     return EFI_NOT_FOUND;
   }
 
   //
   // Create the new image block according to input image.
@@ -1119,10 +1129,11 @@ HiiSetImage (
   //
   // Adjust the image package to remove the original block firstly then add the new block.
   //
   ImageBlocks = AllocateZeroPool (ImagePackage->ImageBlockSize + NewBlockSize - OldBlockSize);
   if (ImageBlocks == NULL) {
+    EfiReleaseLock (&mHiiDatabaseLock);
     return EFI_OUT_OF_RESOURCES;
   }
 
   Part1Size = (UINT32) ((UINTN) CurrentImageBlock - (UINTN) ImagePackage->ImageBlock);
   Part2Size = ImagePackage->ImageBlockSize - Part1Size - OldBlockSize;
@@ -1156,10 +1167,11 @@ HiiSetImage (
   //
   if (gExportAfterReadyToBoot) {
     HiiGetDatabaseInfo(&Private->HiiDatabase);
   }
 
+  EfiReleaseLock (&mHiiDatabaseLock);
   return EFI_SUCCESS;
 
 }
 
 
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/String.c b/MdeModulePkg/Universal/HiiDatabaseDxe/String.c
index aeda47430f..a8178bdca2 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/String.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/String.c
@@ -1208,10 +1208,12 @@ HiiNewString (
   }
   if (PackageListNode == NULL) {
     return EFI_NOT_FOUND;
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   Status = EFI_SUCCESS;
   NewStringPackageCreated = FALSE;
   NewStringId   = 0;
   NextStringId  = 0;
   StringPackage = NULL;
@@ -1571,10 +1573,12 @@ Done:
     if (!EFI_ERROR (Status)) {
       HiiGetDatabaseInfo(&Private->HiiDatabase);
     }
   }
 
+  EfiReleaseLock (&mHiiDatabaseLock);
+
   return Status;
 }
 
 
 /**
@@ -1736,10 +1740,12 @@ HiiSetString (
 
   if (!IsHiiHandleValid (PackageList)) {
     return EFI_NOT_FOUND;
   }
 
+  EfiAcquireLock (&mHiiDatabaseLock);
+
   Private = HII_STRING_DATABASE_PRIVATE_DATA_FROM_THIS (This);
   PackageListNode = NULL;
 
   for (Link = Private->DatabaseList.ForwardLink; Link != &Private->DatabaseList; Link = Link->ForwardLink) {
     DatabaseRecord = CR (Link, HII_DATABASE_RECORD, DatabaseEntry, HII_DATABASE_RECORD_SIGNATURE);
@@ -1762,25 +1768,28 @@ HiiSetString (
                    StringId,
                    (EFI_STRING) String,
                    (EFI_FONT_INFO *) StringFontInfo
                    );
         if (EFI_ERROR (Status)) {
+          EfiReleaseLock (&mHiiDatabaseLock);
           return Status;
         }
         PackageListNode->PackageListHdr.PackageLength += StringPackage->StringPkgHdr->Header.Length - OldPackageLen;
         //
         // Check whether need to get the contents of HiiDataBase.
         // Only after ReadyToBoot to do the export.
         //
         if (gExportAfterReadyToBoot) {
           HiiGetDatabaseInfo(&Private->HiiDatabase);
         }
+        EfiReleaseLock (&mHiiDatabaseLock);
         return EFI_SUCCESS;
       }
     }
   }
 
+  EfiReleaseLock (&mHiiDatabaseLock);
   return EFI_NOT_FOUND;
 }
 
 
 
-- 
2.18.0.windows.1

Re: [patch 0/2] MdeModulePkg/HiiDatabaseDxe: Make sure database update behaviors are atomic

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Bi, Dandan
> Sent: Friday, October 12, 2018 7:26 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [patch 0/2] MdeModulePkg/HiiDatabaseDxe: Make sure database update behaviors are atomic
> 
> The main purpose of this task is to make sure the operations
> that update the HiiDatabase atomic the avoid the potential
> risk that the one update operation with higher TPL may interrupt
> another.
> 
> Patch 1 is to reorgnize the existing code logic and make it's
> easy to add EfiAcquireLock/EfiReleaseLock function in patch 2.
> 
> Patch 2 is to add EfiAcquireLock/EfiReleaseLock function to
> make sure the HiiDatabse update operations atomic.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Dandan Bi (2):
>   MdeModulePkg/HiiDB: Reorganize codes of exporting HII settings
>   MdeModulePkg/HiiDB: Make sure database update behaviors are atomic
> 
>  .../Universal/HiiDatabaseDxe/Database.c       | 128 ++++++++++++------
>  .../Universal/HiiDatabaseDxe/HiiDatabase.h    |   8 +-
>  .../HiiDatabaseDxe/HiiDatabaseEntry.c         |   3 +-
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c |  12 ++
>  .../Universal/HiiDatabaseDxe/String.c         |   9 ++
>  5 files changed, 112 insertions(+), 48 deletions(-)
> 
> --
> 2.18.0.windows.1