[edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the CapsulePei

Gao, Zhichao posted 1 patch 4 years, 10 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
MdeModulePkg/Universal/CapsulePei/Capsule.h   |   3 +-
.../Universal/CapsulePei/CapsulePei.inf       |   3 +-
.../Universal/CapsulePei/UefiCapsule.c        | 355 ++++++++++--------
3 files changed, 192 insertions(+), 169 deletions(-)
[edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the CapsulePei
Posted by Gao, Zhichao 4 years, 10 months ago
From: Bret Barkelew <Bret.Barkelew@microsoft.com>

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

Optimize some function in CapsulePei to make it easier
to maintian.
1. Separate the capsule check function form GetCapsuleDescriptors
to AreCapsulesStaged. The original logic is unclear.
2. Avoid querying the capsule variable twice. First time to count
the number of SG list and allocate a buffer to save SG list data.
Second time to save the SG list data to the buffer. Modified:
Using a template buffer to save the SG list data. After query,
we get the number of SG list, then allocate memory and copy
data form template buffer to the allocated memory.
3. Using MemoryAllocationLib instead of memory function in Pei
services.
4, Remain 2 byte(CHAR16) to be the null-terminate of CapsuleVarName.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao gao <zhichao.gao@intel.com>
---

Code change from https://github.com/microsoft/mu_basecore/blob/release/201903/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c#L801

Note:
1. Change the AreCapsulesStaged: directly return the BOOLEAN result.
2. While the template buffer is to small, double its size through memory function.

 MdeModulePkg/Universal/CapsulePei/Capsule.h   |   3 +-
 .../Universal/CapsulePei/CapsulePei.inf       |   3 +-
 .../Universal/CapsulePei/UefiCapsule.c        | 355 ++++++++++--------
 3 files changed, 192 insertions(+), 169 deletions(-)

diff --git a/MdeModulePkg/Universal/CapsulePei/Capsule.h b/MdeModulePkg/Universal/CapsulePei/Capsule.h
index baf40423af..fc20dd8b92 100644
--- a/MdeModulePkg/Universal/CapsulePei/Capsule.h
+++ b/MdeModulePkg/Universal/CapsulePei/Capsule.h
@@ -1,6 +1,6 @@
 /** @file
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -30,6 +30,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/PcdLib.h>
 #include <Library/ReportStatusCodeLib.h>
 #include <Library/DebugAgentLib.h>
+#include <Library/MemoryAllocationLib.h>
 #include <IndustryStandard/PeImage.h>
 #include "Common/CommonHeader.h"
 
diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index 5d43df3075..9c88b3986f 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -6,7 +6,7 @@
 #  This external input must be validated carefully to avoid security issue like
 #  buffer overflow, integer overflow.
 #
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -43,6 +43,7 @@
   BaseLib
   HobLib
   BaseMemoryLib
+  MemoryAllocationLib
   PeiServicesLib
   PeimEntryPoint
   DebugLib
diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
index e967599e96..b3014478a3 100644
--- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
+++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
@@ -1,7 +1,7 @@
 /** @file
   Capsule update PEIM for UEFI2.0
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "Capsule.h"
 
+#define DEFAULT_SG_LIST_HEADS       (20)
+
 #ifdef MDE_CPU_IA32
 //
 // Global Descriptor Table (GDT)
@@ -791,30 +793,87 @@ BuildMemoryResourceDescriptor (
 }
 
 /**
-  Checks for the presence of capsule descriptors.
-  Get capsule descriptors from variable CapsuleUpdateData, CapsuleUpdateData1, CapsuleUpdateData2...
-  and save to DescriptorBuffer.
+  Check if the capsules are staged.
 
-  @param DescriptorBuffer        Pointer to the capsule descriptors
+  @retval TRUE              The capsules are staged.
+  @retval FALSE             The capsules are not staged.
+
+**/
+BOOLEAN
+AreCapsulesStaged (
+  VOID
+  )
+{
+  EFI_STATUS                        Status;
+  UINTN                             Size;
+  EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
+  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
+
+  CapsuleDataPtr64 = 0;
+
+  Status = PeiServicesLocatePpi (
+            &gEfiPeiReadOnlyVariable2PpiGuid,
+            0,
+            NULL,
+            (VOID **)&PPIVariableServices
+            );
+
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n"));
+    return FALSE;
+  }
+
+  //
+  // Check for Update capsule
+  //
+  Size = sizeof (CapsuleDataPtr64);
+  Status = PPIVariableServices->GetVariable(
+                                  PPIVariableServices,
+                                  EFI_CAPSULE_VARIABLE_NAME,
+                                  &gEfiCapsuleVendorGuid,
+                                  NULL,
+                                  &Size,
+                                  (VOID *)&CapsuleDataPtr64
+                                  );
+
+  if (!EFI_ERROR (Status)) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
+
+/**
+  Check all the variables for SG list heads and get the count and addresses.
+
+  @param ListLength               A pointer would return the SG list length.
+  @param HeadList                 A ponter to the capsule SG list.
+
+  @retval EFI_SUCCESS             a valid capsule is present
+  @retval EFI_NOT_FOUND           if a valid capsule is not present
+  @retval EFI_INVALID_PARAMETER   the input parameter is invalid
+  @retval EFI_OUT_OF_RESOURCE     fail to allocate memory
 
-  @retval EFI_SUCCESS     a valid capsule is present
-  @retval EFI_NOT_FOUND   if a valid capsule is not present
 **/
 EFI_STATUS
-GetCapsuleDescriptors (
-  IN EFI_PHYSICAL_ADDRESS     *DescriptorBuffer
+GetScatterGatherHeadEntries (
+  OUT UINTN                 *ListLength,
+  OUT EFI_PHYSICAL_ADDRESS  **HeadList
   )
 {
-  EFI_STATUS                       Status;
-  UINTN                            Size;
-  UINTN                            Index;
-  UINTN                            TempIndex;
-  UINTN                            ValidIndex;
-  BOOLEAN                          Flag;
-  CHAR16                           CapsuleVarName[30];
-  CHAR16                           *TempVarName;
-  EFI_PHYSICAL_ADDRESS             CapsuleDataPtr64;
-  EFI_PEI_READ_ONLY_VARIABLE2_PPI  *PPIVariableServices;
+  EFI_STATUS                        Status;
+  UINTN                             Size;
+  UINTN                             Index;
+  UINTN                             TempIndex;
+  UINTN                             ValidIndex;
+  BOOLEAN                           Flag;
+  CHAR16                            CapsuleVarName[30];
+  CHAR16                            *TempVarName;
+  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
+  EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
+  EFI_PHYSICAL_ADDRESS              *TempList;
+  EFI_PHYSICAL_ADDRESS              *EnlargedTempList;
+  UINTN                             TempListLength;
 
   Index             = 0;
   TempVarName       = NULL;
@@ -822,87 +881,118 @@ GetCapsuleDescriptors (
   ValidIndex        = 0;
   CapsuleDataPtr64  = 0;
 
+  if ((ListLength == NULL) || (HeadList == NULL)) {
+    DEBUG ((DEBUG_ERROR, "%a Invalid parameters.  Inputs can't be NULL\n", __FUNCTION__));
+    ASSERT (ListLength != NULL);
+    ASSERT (HeadList != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *ListLength = 0;
+  *HeadList = NULL;
+
   Status = PeiServicesLocatePpi (
               &gEfiPeiReadOnlyVariable2PpiGuid,
               0,
               NULL,
               (VOID **) &PPIVariableServices
               );
-  if (Status == EFI_SUCCESS) {
-    StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), EFI_CAPSULE_VARIABLE_NAME);
-    TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
+
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI: %r\n", Status));
+    return Status;
+  }
+
+  //
+  // Allocate memory for sg list head
+  //
+  TempListLength = DEFAULT_SG_LIST_HEADS * sizeof (EFI_PHYSICAL_ADDRESS);
+  TempList = AllocateZeroPool (TempListLength);
+  if (TempList == NULL) {
+    DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Setup var name buffer for update capsules
+  //
+  StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16), EFI_CAPSULE_VARIABLE_NAME);
+  TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
+  while (TRUE) {
+    if (Index != 0) {
+      UnicodeValueToStringS (
+        TempVarName,
+        (sizeof(CapsuleVarName) - ((StrLen(CapsuleVarName) + 1) * sizeof(CHAR16))),
+        0,
+        Index,
+        0
+        );
+    }
+
     Size = sizeof (CapsuleDataPtr64);
-    while (1) {
-      if (Index == 0) {
-        //
-        // For the first Capsule Image
-        //
-        Status = PPIVariableServices->GetVariable (
-                                        PPIVariableServices,
-                                        CapsuleVarName,
-                                        &gEfiCapsuleVendorGuid,
-                                        NULL,
-                                        &Size,
-                                        (VOID *) &CapsuleDataPtr64
-                                        );
-        if (EFI_ERROR (Status)) {
-          DEBUG ((DEBUG_INFO, "Capsule -- capsule variable not set\n"));
-          return EFI_NOT_FOUND;
-        }
-        //
-        // We have a chicken/egg situation where the memory init code needs to
-        // know the boot mode prior to initializing memory. For this case, our
-        // validate function will fail. We can detect if this is the case if blocklist
-        // pointer is null. In that case, return success since we know that the
-        // variable is set.
-        //
-        if (DescriptorBuffer == NULL) {
-          return EFI_SUCCESS;
-        }
-      } else {
-        UnicodeValueToStringS (
-          TempVarName,
-          sizeof (CapsuleVarName) - ((UINTN)TempVarName - (UINTN)CapsuleVarName),
-          0,
-          Index,
-          0
-          );
-        Status = PPIVariableServices->GetVariable (
-                                        PPIVariableServices,
-                                        CapsuleVarName,
-                                        &gEfiCapsuleVendorGuid,
-                                        NULL,
-                                        &Size,
-                                        (VOID *) &CapsuleDataPtr64
-                                        );
-        if (EFI_ERROR (Status)) {
-          break;
-        }
+    Status = PPIVariableServices->GetVariable (
+                                    PPIVariableServices,
+                                    CapsuleVarName,
+                                    &gEfiCapsuleVendorGuid,
+                                    NULL,
+                                    &Size,
+                                    (VOID *)&CapsuleDataPtr64
+                                    );
 
-        //
-        // If this BlockList has been linked before, skip this variable
-        //
-        Flag = FALSE;
-        for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
-          if (DescriptorBuffer[TempIndex] == CapsuleDataPtr64)  {
-            Flag = TRUE;
-            break;
-          }
-        }
-        if (Flag) {
-          Index ++;
-          continue;
-        }
+    if (EFI_ERROR (Status)) {
+      if (Status != EFI_NOT_FOUND) {
+        DEBUG ((DEBUG_ERROR, "Unexpected error getting Capsule Update variable.  Status = %r\n"));
+      }
+      break;
+    }
+
+    //
+    // If this BlockList has been linked before, skip this variable
+    //
+    Flag = FALSE;
+    for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
+      if (TempList[TempIndex] == CapsuleDataPtr64) {
+        Flag = TRUE;
+        break;
       }
+    }
+    if (Flag) {
+      Index++;
+      continue;
+    }
 
-      //
-      // Cache BlockList which has been processed
-      //
-      DescriptorBuffer[ValidIndex++] = CapsuleDataPtr64;
-      Index ++;
+    //
+    // The TempList is full, enlarge it
+    //
+    if ((ValidIndex + 1) >= TempListLength) {
+      EnlargedTempList = AllocateZeroPool (TempListLength * 2);
+      CopyMem (EnlargedTempList, TempList, TempListLength);
+      FreePool (TempList);
+      TempList = EnlargedTempList;
+      TempListLength *= 2;
     }
+
+    //
+    // Cache BlockList which has been processed
+    //
+    TempList[ValidIndex++] = CapsuleDataPtr64;
+    Index++;
+  }
+
+  if (ValidIndex == 0) {
+    DEBUG((DEBUG_ERROR, "%a didn't find any SG lists in variables\n", __FUNCTION__));
+    return EFI_NOT_FOUND;
   }
 
+  *HeadList = AllocateZeroPool ((ValidIndex + 1) * sizeof (EFI_PHYSICAL_ADDRESS));
+  if (*HeadList == NULL) {
+    DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  CopyMem (*HeadList, TempList, (ValidIndex) * sizeof (EFI_PHYSICAL_ADDRESS));
+  *ListLength = ValidIndex; 
+
   return EFI_SUCCESS;
 }
 
@@ -937,17 +1027,11 @@ CapsuleCoalesce (
   IN OUT UINTN                       *MemorySize
   )
 {
-  UINTN                                Index;
-  UINTN                                Size;
-  UINTN                                VariableCount;
-  CHAR16                               CapsuleVarName[30];
-  CHAR16                               *TempVarName;
-  EFI_PHYSICAL_ADDRESS                 CapsuleDataPtr64;
   EFI_STATUS                           Status;
   EFI_BOOT_MODE                        BootMode;
-  EFI_PEI_READ_ONLY_VARIABLE2_PPI      *PPIVariableServices;
   EFI_PHYSICAL_ADDRESS                 *VariableArrayAddress;
   MEMORY_RESOURCE_DESCRIPTOR           *MemoryResource;
+  UINTN                                ListLength;
 #ifdef MDE_CPU_IA32
   UINT16                               CoalesceImageMachineType;
   EFI_PHYSICAL_ADDRESS                 CoalesceImageEntryPoint;
@@ -955,10 +1039,8 @@ CapsuleCoalesce (
   EFI_CAPSULE_LONG_MODE_BUFFER         LongModeBuffer;
 #endif
 
-  Index                   = 0;
-  VariableCount           = 0;
-  CapsuleVarName[0]       = 0;
-  CapsuleDataPtr64        = 0;
+  ListLength            = 0;
+  VariableArrayAddress  = NULL;
 
   //
   // Someone should have already ascertained the boot mode. If it's not
@@ -972,74 +1054,11 @@ CapsuleCoalesce (
   }
 
   //
-  // User may set the same ScatterGatherList with several different variables,
-  // so cache all ScatterGatherList for check later.
-  //
-  Status = PeiServicesLocatePpi (
-              &gEfiPeiReadOnlyVariable2PpiGuid,
-              0,
-              NULL,
-              (VOID **) &PPIVariableServices
-              );
-  if (EFI_ERROR (Status)) {
-    goto Done;
-  }
-  Size = sizeof (CapsuleDataPtr64);
-  StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), EFI_CAPSULE_VARIABLE_NAME);
-  TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
-  while (TRUE) {
-    if (Index > 0) {
-      UnicodeValueToStringS (
-        TempVarName,
-        sizeof (CapsuleVarName) - ((UINTN)TempVarName - (UINTN)CapsuleVarName),
-        0,
-        Index,
-        0
-        );
-    }
-    Status = PPIVariableServices->GetVariable (
-                                    PPIVariableServices,
-                                    CapsuleVarName,
-                                    &gEfiCapsuleVendorGuid,
-                                    NULL,
-                                    &Size,
-                                    (VOID *) &CapsuleDataPtr64
-                                    );
-    if (EFI_ERROR (Status)) {
-      //
-      // There is no capsule variables, quit
-      //
-      DEBUG ((DEBUG_INFO,"Capsule variable Index = %d\n", Index));
-      break;
-    }
-    VariableCount++;
-    Index++;
-  }
-
-  DEBUG ((DEBUG_INFO,"Capsule variable count = %d\n", VariableCount));
-
-  //
-  // The last entry is the end flag.
-  //
-  Status = PeiServicesAllocatePool (
-             (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS),
-             (VOID **)&VariableArrayAddress
-             );
-
-  if (Status != EFI_SUCCESS) {
-    DEBUG ((DEBUG_ERROR, "AllocatePages Failed!, Status = %x\n", Status));
-    goto Done;
-  }
-
-  ZeroMem (VariableArrayAddress, (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS));
-
-  //
-  // Find out if we actually have a capsule.
-  // GetCapsuleDescriptors depends on variable PPI, so it should run in 32-bit environment.
+  // Get ScatterGatherList
   //
-  Status = GetCapsuleDescriptors (VariableArrayAddress);
+  Status = GetScatterGatherHeadEntries (&ListLength, &VariableArrayAddress);
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "Fail to find capsule variables.\n"));
+    DEBUG ((DEBUG_ERROR, "%a failed to get Scatter Gather List Head Entries.  Status = %r\n", __FUNCTION__, Status));
     goto Done;
   }
 
@@ -1117,9 +1136,11 @@ CheckCapsuleUpdate (
   IN EFI_PEI_SERVICES           **PeiServices
   )
 {
-  EFI_STATUS  Status;
-  Status = GetCapsuleDescriptors (NULL);
-  return Status;
+  if (AreCapsulesStaged ()) {
+    return EFI_SUCCESS;
+  } else {
+    return EFI_NOT_FOUND;
+  }
 }
 /**
   This function will look at a capsule and determine if it's a test pattern.
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41702): https://edk2.groups.io/g/devel/message/41702
Mute This Topic: https://groups.io/mt/31875980/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the CapsulePei
Posted by Wu, Hao A 4 years, 10 months ago
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Gao, Zhichao
> Sent: Friday, May 31, 2019 9:48 AM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming;
> Sean Brogan; Michael Turner; Gao, Zhichao
> Subject: [edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the
> CapsulePei
> 
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
> 
> Optimize some function in CapsulePei to make it easier
> to maintian.

maintian -> maintain

> 1. Separate the capsule check function form GetCapsuleDescriptors
> to AreCapsulesStaged. The original logic is unclear.
> 2. Avoid querying the capsule variable twice. First time to count
> the number of SG list and allocate a buffer to save SG list data.
> Second time to save the SG list data to the buffer. Modified:
> Using a template buffer to save the SG list data. After query,
> we get the number of SG list, then allocate memory and copy
> data form template buffer to the allocated memory.
> 3. Using MemoryAllocationLib instead of memory function in Pei
> services.

Not directly related with this patch.

Now the PEIM has a mixed usage of both the PEI service and memory
allocation library to allocate memory.

Maybe a later patch can unify such usage.

> 4, Remain 2 byte(CHAR16) to be the null-terminate of CapsuleVarName.

Sorry for missing your reply in the V1 patch.

Upon successful return of UnicodeValueToStringS(), the input string buffer
is guaranteed to be null-terminated. I think the origin logic is fine.


One more minor comment below.

> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao gao <zhichao.gao@intel.com>
> ---
> 
> Code change from
> https://github.com/microsoft/mu_basecore/blob/release/201903/MdeMod
> ulePkg/Universal/CapsulePei/UefiCapsule.c#L801
> 
> Note:
> 1. Change the AreCapsulesStaged: directly return the BOOLEAN result.
> 2. While the template buffer is to small, double its size through memory
> function.
> 
>  MdeModulePkg/Universal/CapsulePei/Capsule.h   |   3 +-
>  .../Universal/CapsulePei/CapsulePei.inf       |   3 +-
>  .../Universal/CapsulePei/UefiCapsule.c        | 355 ++++++++++--------
>  3 files changed, 192 insertions(+), 169 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/CapsulePei/Capsule.h
> b/MdeModulePkg/Universal/CapsulePei/Capsule.h
> index baf40423af..fc20dd8b92 100644
> --- a/MdeModulePkg/Universal/CapsulePei/Capsule.h
> +++ b/MdeModulePkg/Universal/CapsulePei/Capsule.h
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -30,6 +30,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/PcdLib.h>
>  #include <Library/ReportStatusCodeLib.h>
>  #include <Library/DebugAgentLib.h>
> +#include <Library/MemoryAllocationLib.h>
>  #include <IndustryStandard/PeImage.h>
>  #include "Common/CommonHeader.h"
> 
> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> index 5d43df3075..9c88b3986f 100644
> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> @@ -6,7 +6,7 @@
>  #  This external input must be validated carefully to avoid security issue like
>  #  buffer overflow, integer overflow.
>  #
> -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -43,6 +43,7 @@
>    BaseLib
>    HobLib
>    BaseMemoryLib
> +  MemoryAllocationLib
>    PeiServicesLib
>    PeimEntryPoint
>    DebugLib
> diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> index e967599e96..b3014478a3 100644
> --- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> +++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Capsule update PEIM for UEFI2.0
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #include "Capsule.h"
> 
> +#define DEFAULT_SG_LIST_HEADS       (20)
> +
>  #ifdef MDE_CPU_IA32
>  //
>  // Global Descriptor Table (GDT)
> @@ -791,30 +793,87 @@ BuildMemoryResourceDescriptor (
>  }
> 
>  /**
> -  Checks for the presence of capsule descriptors.
> -  Get capsule descriptors from variable CapsuleUpdateData,
> CapsuleUpdateData1, CapsuleUpdateData2...
> -  and save to DescriptorBuffer.
> +  Check if the capsules are staged.
> 
> -  @param DescriptorBuffer        Pointer to the capsule descriptors
> +  @retval TRUE              The capsules are staged.
> +  @retval FALSE             The capsules are not staged.
> +
> +**/
> +BOOLEAN
> +AreCapsulesStaged (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  UINTN                             Size;
> +  EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
> +  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
> +
> +  CapsuleDataPtr64 = 0;
> +
> +  Status = PeiServicesLocatePpi (
> +            &gEfiPeiReadOnlyVariable2PpiGuid,
> +            0,
> +            NULL,
> +            (VOID **)&PPIVariableServices
> +            );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n"));
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check for Update capsule
> +  //
> +  Size = sizeof (CapsuleDataPtr64);
> +  Status = PPIVariableServices->GetVariable(
> +                                  PPIVariableServices,
> +                                  EFI_CAPSULE_VARIABLE_NAME,
> +                                  &gEfiCapsuleVendorGuid,
> +                                  NULL,
> +                                  &Size,
> +                                  (VOID *)&CapsuleDataPtr64
> +                                  );
> +
> +  if (!EFI_ERROR (Status)) {
> +    return TRUE;
> +  }
> +
> +  return FALSE;
> +}
> +
> +/**
> +  Check all the variables for SG list heads and get the count and addresses.
> +
> +  @param ListLength               A pointer would return the SG list length.
> +  @param HeadList                 A ponter to the capsule SG list.
> +
> +  @retval EFI_SUCCESS             a valid capsule is present
> +  @retval EFI_NOT_FOUND           if a valid capsule is not present
> +  @retval EFI_INVALID_PARAMETER   the input parameter is invalid
> +  @retval EFI_OUT_OF_RESOURCE     fail to allocate memory
> 
> -  @retval EFI_SUCCESS     a valid capsule is present
> -  @retval EFI_NOT_FOUND   if a valid capsule is not present
>  **/
>  EFI_STATUS
> -GetCapsuleDescriptors (
> -  IN EFI_PHYSICAL_ADDRESS     *DescriptorBuffer
> +GetScatterGatherHeadEntries (
> +  OUT UINTN                 *ListLength,
> +  OUT EFI_PHYSICAL_ADDRESS  **HeadList
>    )
>  {
> -  EFI_STATUS                       Status;
> -  UINTN                            Size;
> -  UINTN                            Index;
> -  UINTN                            TempIndex;
> -  UINTN                            ValidIndex;
> -  BOOLEAN                          Flag;
> -  CHAR16                           CapsuleVarName[30];
> -  CHAR16                           *TempVarName;
> -  EFI_PHYSICAL_ADDRESS             CapsuleDataPtr64;
> -  EFI_PEI_READ_ONLY_VARIABLE2_PPI  *PPIVariableServices;
> +  EFI_STATUS                        Status;
> +  UINTN                             Size;
> +  UINTN                             Index;
> +  UINTN                             TempIndex;
> +  UINTN                             ValidIndex;
> +  BOOLEAN                           Flag;
> +  CHAR16                            CapsuleVarName[30];
> +  CHAR16                            *TempVarName;
> +  EFI_PHYSICAL_ADDRESS              CapsuleDataPtr64;
> +  EFI_PEI_READ_ONLY_VARIABLE2_PPI   *PPIVariableServices;
> +  EFI_PHYSICAL_ADDRESS              *TempList;
> +  EFI_PHYSICAL_ADDRESS              *EnlargedTempList;
> +  UINTN                             TempListLength;
> 
>    Index             = 0;
>    TempVarName       = NULL;
> @@ -822,87 +881,118 @@ GetCapsuleDescriptors (
>    ValidIndex        = 0;
>    CapsuleDataPtr64  = 0;
> 
> +  if ((ListLength == NULL) || (HeadList == NULL)) {
> +    DEBUG ((DEBUG_ERROR, "%a Invalid parameters.  Inputs can't be
> NULL\n", __FUNCTION__));
> +    ASSERT (ListLength != NULL);
> +    ASSERT (HeadList != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *ListLength = 0;
> +  *HeadList = NULL;
> +
>    Status = PeiServicesLocatePpi (
>                &gEfiPeiReadOnlyVariable2PpiGuid,
>                0,
>                NULL,
>                (VOID **) &PPIVariableServices
>                );
> -  if (Status == EFI_SUCCESS) {
> -    StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16),
> EFI_CAPSULE_VARIABLE_NAME);
> -    TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI: %r\n",
> Status));
> +    return Status;
> +  }
> +
> +  //
> +  // Allocate memory for sg list head
> +  //
> +  TempListLength = DEFAULT_SG_LIST_HEADS * sizeof
> (EFI_PHYSICAL_ADDRESS);
> +  TempList = AllocateZeroPool (TempListLength);
> +  if (TempList == NULL) {
> +    DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Setup var name buffer for update capsules
> +  //
> +  StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16),
> EFI_CAPSULE_VARIABLE_NAME);
> +  TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
> +  while (TRUE) {
> +    if (Index != 0) {
> +      UnicodeValueToStringS (
> +        TempVarName,
> +        (sizeof(CapsuleVarName) - ((StrLen(CapsuleVarName) + 1) *
> sizeof(CHAR16))),
> +        0,
> +        Index,
> +        0
> +        );
> +    }
> +
>      Size = sizeof (CapsuleDataPtr64);
> -    while (1) {
> -      if (Index == 0) {
> -        //
> -        // For the first Capsule Image
> -        //
> -        Status = PPIVariableServices->GetVariable (
> -                                        PPIVariableServices,
> -                                        CapsuleVarName,
> -                                        &gEfiCapsuleVendorGuid,
> -                                        NULL,
> -                                        &Size,
> -                                        (VOID *) &CapsuleDataPtr64
> -                                        );
> -        if (EFI_ERROR (Status)) {
> -          DEBUG ((DEBUG_INFO, "Capsule -- capsule variable not set\n"));
> -          return EFI_NOT_FOUND;
> -        }
> -        //
> -        // We have a chicken/egg situation where the memory init code needs
> to
> -        // know the boot mode prior to initializing memory. For this case, our
> -        // validate function will fail. We can detect if this is the case if blocklist
> -        // pointer is null. In that case, return success since we know that the
> -        // variable is set.
> -        //
> -        if (DescriptorBuffer == NULL) {
> -          return EFI_SUCCESS;
> -        }
> -      } else {
> -        UnicodeValueToStringS (
> -          TempVarName,
> -          sizeof (CapsuleVarName) - ((UINTN)TempVarName -
> (UINTN)CapsuleVarName),
> -          0,
> -          Index,
> -          0
> -          );
> -        Status = PPIVariableServices->GetVariable (
> -                                        PPIVariableServices,
> -                                        CapsuleVarName,
> -                                        &gEfiCapsuleVendorGuid,
> -                                        NULL,
> -                                        &Size,
> -                                        (VOID *) &CapsuleDataPtr64
> -                                        );
> -        if (EFI_ERROR (Status)) {
> -          break;
> -        }
> +    Status = PPIVariableServices->GetVariable (
> +                                    PPIVariableServices,
> +                                    CapsuleVarName,
> +                                    &gEfiCapsuleVendorGuid,
> +                                    NULL,
> +                                    &Size,
> +                                    (VOID *)&CapsuleDataPtr64
> +                                    );
> 
> -        //
> -        // If this BlockList has been linked before, skip this variable
> -        //
> -        Flag = FALSE;
> -        for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
> -          if (DescriptorBuffer[TempIndex] == CapsuleDataPtr64)  {
> -            Flag = TRUE;
> -            break;
> -          }
> -        }
> -        if (Flag) {
> -          Index ++;
> -          continue;
> -        }
> +    if (EFI_ERROR (Status)) {
> +      if (Status != EFI_NOT_FOUND) {
> +        DEBUG ((DEBUG_ERROR, "Unexpected error getting Capsule Update
> variable.  Status = %r\n"));
> +      }
> +      break;
> +    }
> +
> +    //
> +    // If this BlockList has been linked before, skip this variable
> +    //
> +    Flag = FALSE;
> +    for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
> +      if (TempList[TempIndex] == CapsuleDataPtr64) {
> +        Flag = TRUE;
> +        break;
>        }
> +    }
> +    if (Flag) {
> +      Index++;
> +      continue;
> +    }
> 
> -      //
> -      // Cache BlockList which has been processed
> -      //
> -      DescriptorBuffer[ValidIndex++] = CapsuleDataPtr64;
> -      Index ++;
> +    //
> +    // The TempList is full, enlarge it
> +    //
> +    if ((ValidIndex + 1) >= TempListLength) {
> +      EnlargedTempList = AllocateZeroPool (TempListLength * 2);
> +      CopyMem (EnlargedTempList, TempList, TempListLength);
> +      FreePool (TempList);
> +      TempList = EnlargedTempList;
> +      TempListLength *= 2;
>      }
> +
> +    //
> +    // Cache BlockList which has been processed
> +    //
> +    TempList[ValidIndex++] = CapsuleDataPtr64;
> +    Index++;
> +  }
> +
> +  if (ValidIndex == 0) {
> +    DEBUG((DEBUG_ERROR, "%a didn't find any SG lists in variables\n",
> __FUNCTION__));
> +    return EFI_NOT_FOUND;
>    }
> 
> +  *HeadList = AllocateZeroPool ((ValidIndex + 1) * sizeof
> (EFI_PHYSICAL_ADDRESS));
> +  if (*HeadList == NULL) {
> +    DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  CopyMem (*HeadList, TempList, (ValidIndex) * sizeof
> (EFI_PHYSICAL_ADDRESS));
> +  *ListLength = ValidIndex;

Please help to remove the tailing white space.

With the above comments addressed,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> +
>    return EFI_SUCCESS;
>  }
> 
> @@ -937,17 +1027,11 @@ CapsuleCoalesce (
>    IN OUT UINTN                       *MemorySize
>    )
>  {
> -  UINTN                                Index;
> -  UINTN                                Size;
> -  UINTN                                VariableCount;
> -  CHAR16                               CapsuleVarName[30];
> -  CHAR16                               *TempVarName;
> -  EFI_PHYSICAL_ADDRESS                 CapsuleDataPtr64;
>    EFI_STATUS                           Status;
>    EFI_BOOT_MODE                        BootMode;
> -  EFI_PEI_READ_ONLY_VARIABLE2_PPI      *PPIVariableServices;
>    EFI_PHYSICAL_ADDRESS                 *VariableArrayAddress;
>    MEMORY_RESOURCE_DESCRIPTOR           *MemoryResource;
> +  UINTN                                ListLength;
>  #ifdef MDE_CPU_IA32
>    UINT16                               CoalesceImageMachineType;
>    EFI_PHYSICAL_ADDRESS                 CoalesceImageEntryPoint;
> @@ -955,10 +1039,8 @@ CapsuleCoalesce (
>    EFI_CAPSULE_LONG_MODE_BUFFER         LongModeBuffer;
>  #endif
> 
> -  Index                   = 0;
> -  VariableCount           = 0;
> -  CapsuleVarName[0]       = 0;
> -  CapsuleDataPtr64        = 0;
> +  ListLength            = 0;
> +  VariableArrayAddress  = NULL;
> 
>    //
>    // Someone should have already ascertained the boot mode. If it's not
> @@ -972,74 +1054,11 @@ CapsuleCoalesce (
>    }
> 
>    //
> -  // User may set the same ScatterGatherList with several different variables,
> -  // so cache all ScatterGatherList for check later.
> -  //
> -  Status = PeiServicesLocatePpi (
> -              &gEfiPeiReadOnlyVariable2PpiGuid,
> -              0,
> -              NULL,
> -              (VOID **) &PPIVariableServices
> -              );
> -  if (EFI_ERROR (Status)) {
> -    goto Done;
> -  }
> -  Size = sizeof (CapsuleDataPtr64);
> -  StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16),
> EFI_CAPSULE_VARIABLE_NAME);
> -  TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
> -  while (TRUE) {
> -    if (Index > 0) {
> -      UnicodeValueToStringS (
> -        TempVarName,
> -        sizeof (CapsuleVarName) - ((UINTN)TempVarName -
> (UINTN)CapsuleVarName),
> -        0,
> -        Index,
> -        0
> -        );
> -    }
> -    Status = PPIVariableServices->GetVariable (
> -                                    PPIVariableServices,
> -                                    CapsuleVarName,
> -                                    &gEfiCapsuleVendorGuid,
> -                                    NULL,
> -                                    &Size,
> -                                    (VOID *) &CapsuleDataPtr64
> -                                    );
> -    if (EFI_ERROR (Status)) {
> -      //
> -      // There is no capsule variables, quit
> -      //
> -      DEBUG ((DEBUG_INFO,"Capsule variable Index = %d\n", Index));
> -      break;
> -    }
> -    VariableCount++;
> -    Index++;
> -  }
> -
> -  DEBUG ((DEBUG_INFO,"Capsule variable count = %d\n", VariableCount));
> -
> -  //
> -  // The last entry is the end flag.
> -  //
> -  Status = PeiServicesAllocatePool (
> -             (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS),
> -             (VOID **)&VariableArrayAddress
> -             );
> -
> -  if (Status != EFI_SUCCESS) {
> -    DEBUG ((DEBUG_ERROR, "AllocatePages Failed!, Status = %x\n", Status));
> -    goto Done;
> -  }
> -
> -  ZeroMem (VariableArrayAddress, (VariableCount + 1) * sizeof
> (EFI_PHYSICAL_ADDRESS));
> -
> -  //
> -  // Find out if we actually have a capsule.
> -  // GetCapsuleDescriptors depends on variable PPI, so it should run in 32-bit
> environment.
> +  // Get ScatterGatherList
>    //
> -  Status = GetCapsuleDescriptors (VariableArrayAddress);
> +  Status = GetScatterGatherHeadEntries (&ListLength,
> &VariableArrayAddress);
>    if (EFI_ERROR (Status)) {
> -    DEBUG ((DEBUG_ERROR, "Fail to find capsule variables.\n"));
> +    DEBUG ((DEBUG_ERROR, "%a failed to get Scatter Gather List Head
> Entries.  Status = %r\n", __FUNCTION__, Status));
>      goto Done;
>    }
> 
> @@ -1117,9 +1136,11 @@ CheckCapsuleUpdate (
>    IN EFI_PEI_SERVICES           **PeiServices
>    )
>  {
> -  EFI_STATUS  Status;
> -  Status = GetCapsuleDescriptors (NULL);
> -  return Status;
> +  if (AreCapsulesStaged ()) {
> +    return EFI_SUCCESS;
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
>  }
>  /**
>    This function will look at a capsule and determine if it's a test pattern.
> --
> 2.21.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41711): https://edk2.groups.io/g/devel/message/41711
Mute This Topic: https://groups.io/mt/31875980/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-