[PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers

[PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers

[Eric Jin]

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

The patch is to merge multiple FMP instances into single ESRT entry
when they have the same GUID.

The policy to LastAttemptStatus/LastAttemptVersion of ESRT entry is:
If all the LastAttemptStatus are LAST_ATTEMPT_STATUS_SUCCESS, then
LastAttemptVersion should be the smallest of LastAttemptVersion. If
any of the LastAttemptStatus is not LAST_ATTEMPT_STATUS_SUCCESS,
then the LastAttemptVersion/LastAttemptStatus should be the values
of the first FMP instance whose LastAttemptStatus is not
LAST_ATTEMPT_STATUS_SUCCESS.

To detect possible duplicated GUID/HardwareInstance, a table of
GUID/HardwareInstance pairs from all the EFI_FIRMWARE_IMAGE_DESCRIPTORs
from all FMP instances is built. If a duplicate is found, then generate
a DEBUG_ERROR message, generate an ASSERT(), and ignore the duplicate
EFI_FIRMWARE_IMAGE_DESCRIPTOR.

Add an internal worker function called FmpGetFirmwareImageDescriptor()
that retrieves the list of EFI_FIRMWARE_IMAGE_DESCRIPTORs from a single
FMP instance and returns the descriptors in an allocated buffer. This
function is used to get the descriptors used to build the table of
unique GUID/HardwareInstance pairs.  It is then used again to generate
the ESRT Table from all the EFI_FIRMWARE_IMAGE_DESCRIPTORs from all the
FMP instances. 2 passes are performed so the total number of
descriptors is known. This allows the correct sized buffers to always
be allocated.

Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Eric Jin <eric.jin@intel.com>
---
 MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 394 +++++++++++++-------
 1 file changed, 257 insertions(+), 137 deletions(-)

diff --git a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
index 2356da662e..4670349f89 100644
--- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
+++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
@@ -2,7 +2,7 @@
   Publishes ESRT table from Firmware Management Protocol instances
 
   Copyright (c) 2016, Microsoft Corporation
-  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
 
   All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -21,6 +21,22 @@
 #include <Guid/EventGroup.h>
 #include <Guid/SystemResourceTable.h>
 
+///
+/// Structure for array of unique GUID/HardwareInstance pairs from the
+/// current set of EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP Protocols.
+///
+typedef struct {
+  ///
+  /// A unique GUID identifying the firmware image type.
+  ///
+  EFI_GUID                      ImageTypeGuid;
+  ///
+  /// An optional number to identify the unique hardware instance within the
+  /// system for devices that may have multiple instances whenever possible.
+  ///
+  UINT64                        HardwareInstance;
+} GUID_HARDWAREINSTANCE_PAIR;
+
 /**
  Print ESRT to debug console.
 
@@ -33,11 +49,6 @@ PrintTable (
   IN EFI_SYSTEM_RESOURCE_TABLE  *Table
   );
 
-//
-// Number of ESRT entries to grow by each time we run out of room
-//
-#define GROWTH_STEP  10
-
 /**
   Install EFI System Resource Table into the UEFI Configuration Table
 
@@ -101,90 +112,129 @@ IsSystemFmp (
 }
 
 /**
-  Function to create a single ESRT Entry and add it to the ESRT
-  given a FMP descriptor.  If the guid is already in the ESRT it
-  will be ignored.  The ESRT will grow if it does not have enough room.
-
-  @param[in, out] Table             On input, pointer to the pointer to the ESRT.
-                                    On output, same as input or pointer to the pointer
-                                    to new enlarged ESRT.
-  @param[in]      FmpImageInfoBuf   Pointer to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.
-  @param[in]      FmpVersion        FMP Version.
-
-  @return  Status code.
+  Function to create a single ESRT Entry and add it to the ESRT with
+  a given FMP descriptor.  If the GUID is already in the ESRT, then the ESRT
+  entry is updated.
+
+  @param[in,out] Table                Pointer to the ESRT Table.
+  @param[in,out] HardwareInstances    Pointer to the GUID_HARDWAREINSTANCE_PAIR.
+  @param[in,out] NumberOfDescriptors  The number of EFI_FIRMWARE_IMAGE_DESCRIPTORs.
+  @param[in]     FmpImageInfoBuf      Pointer to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+  @param[in]     FmpVersion           FMP Version.
+
+  @retval  EFI_SUCCESS     FmpImageInfoBuf was use to fill in a new ESRT entry
+                           in Table.
+  @retval  EFI_SUCCESS     The ImageTypeId GUID in FmpImageInfoBuf matches an
+                           existing ESRT entry in Table, and the information
+                           from FmpImageInfoBuf was merged into the the existing
+                           ESRT entry.
+  @retval  EFI_UNSPOORTED  The GUID/HardareInstance in FmpImageInfoBuf has is a
+                           duplicate.
 
 **/
 EFI_STATUS
 CreateEsrtEntry (
-  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
-  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
-  IN UINT32                         FmpVersion
+  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
+  IN OUT GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances,
+  IN OUT UINT32                         *NumberOfDescriptors,
+  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
+  IN     UINT32                         FmpVersion
   )
 {
   UINTN                      Index;
   EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
-  UINTN                      NewSize;
-  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
+  UINT64                     FmpHardwareInstance;
 
-  Index = 0;
-  Entry = NULL;
+  FmpHardwareInstance = 0;
+  if (FmpVersion >= 3) {
+    FmpHardwareInstance = FmpImageInfoBuf->HardwareInstance;
+  }
 
-  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) + 1);
   //
-  // Make sure Guid isn't already in the list
+  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
   //
-  for (Index = 0; Index < (*Table)->FwResourceCount; Index++) {
-    if (CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId)) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry already exists for FMP Instance with GUID %g\n", &Entry->FwClass));
-      return EFI_INVALID_PARAMETER;
+  for (Index = 0; Index < *NumberOfDescriptors; Index++) {
+    if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId)) {
+      if (HardwareInstances[Index].HardwareInstance == FmpHardwareInstance) {
+        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descriptor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
+        ASSERT (
+          !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId) ||
+          HardwareInstances[Index].HardwareInstance != FmpHardwareInstance
+          );
+        return EFI_UNSUPPORTED;
+      }
     }
-    Entry++;
   }
 
   //
-  // Grow table if needed
+  // Record new GUID/HardwareInstance pair
   //
-  if ((*Table)->FwResourceCount >= (*Table)->FwResourceCountMax) {
-    NewSize  = (((*Table)->FwResourceCountMax + GROWTH_STEP) * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE);
-    NewTable = AllocateZeroPool (NewSize);
-    if (NewTable == NULL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory larger table for ESRT. \n"));
-      return EFI_OUT_OF_RESOURCES;
+  CopyGuid (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId);
+  HardwareInstances[*NumberOfDescriptors].HardwareInstance = FmpHardwareInstance;
+  *NumberOfDescriptors = *NumberOfDescriptors + 1;
+
+  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image descriptor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
+
+  //
+  // Check to see if GUID is already in the ESRT table
+  //
+  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);
+  for (Index = 0; Index < Table->FwResourceCount; Index++, Entry++) {
+    if (!CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId)) {
+      continue;
     }
+    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry already exists for FMP Instance with GUID %g\n", &Entry->FwClass));
+
     //
-    // Copy the whole old table into new table buffer
-    //
-    CopyMem (
-      NewTable,
-      (*Table),
-      (((*Table)->FwResourceCountMax) * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
-      );
-    //
-    // Update max
+    // Set ESRT FwVersion to the smaller of the two values
     //
-    NewTable->FwResourceCountMax = NewTable->FwResourceCountMax + GROWTH_STEP;
+    Entry->FwVersion = MIN (FmpImageInfoBuf->Version, Entry->FwVersion);
+
     //
-    // Free old table
+    // VERSION 2 has Lowest Supported
     //
-    FreePool ((*Table));
+    if (FmpVersion >= 2) {
+      //
+      // Set ESRT LowestSupportedFwVersion to the smaller of the two values
+      //
+      Entry->LowestSupportedFwVersion =
+        MIN (
+          FmpImageInfoBuf->LowestSupportedImageVersion,
+          Entry->LowestSupportedFwVersion
+          );
+    }
+
     //
-    // Reassign pointer to new table.
+    // VERSION 3 supports last attempt values
     //
-    (*Table) = NewTable;
+    if (FmpVersion >= 3) {
+      //
+      // Update the ESRT entry with the last attempt status and last attempt
+      // version from the first FMP instance whose last attempt status is not
+      // SUCCESS.  If all FMP instances are SUCCESS, then set version to the
+      // smallest value from all FMP instances.
+      //
+      if (Entry->LastAttemptStatus == LAST_ATTEMPT_STATUS_SUCCESS) {
+        if (FmpImageInfoBuf->LastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+          Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
+          Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
+        } else {
+          Entry->LastAttemptVersion =
+            MIN (
+              FmpImageInfoBuf->LastAttemptVersion,
+              Entry->LastAttemptVersion
+              );
+        }
+      }
+    }
+
+    return EFI_SUCCESS;
   }
 
   //
-  // ESRT table has enough room for the new entry so add new entry
-  //
-  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8 *)(*Table)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE));
-  //
-  // Move to the location of new entry
-  //
-  Entry = Entry + (*Table)->FwResourceCount;
+  // Add a new ESRT Table Entry
   //
-  // Increment resource count
-  //
-  (*Table)->FwResourceCount++;
+  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) + Table->FwResourceCount;
 
   CopyGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId);
 
@@ -195,11 +245,11 @@ CreateEsrtEntry (
     Entry->FwType = (UINT32)(ESRT_FW_TYPE_DEVICEFIRMWARE);
   }
 
-  Entry->FwVersion = FmpImageInfoBuf->Version;
+  Entry->FwVersion                = FmpImageInfoBuf->Version;
   Entry->LowestSupportedFwVersion = 0;
-  Entry->CapsuleFlags = 0;
-  Entry->LastAttemptVersion = 0;
-  Entry->LastAttemptStatus = 0;
+  Entry->CapsuleFlags             = 0;
+  Entry->LastAttemptVersion       = 0;
+  Entry->LastAttemptStatus        = 0;
 
   //
   // VERSION 2 has Lowest Supported
@@ -213,12 +263,93 @@ CreateEsrtEntry (
   //
   if (FmpVersion >= 3) {
     Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
-    Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
+    Entry->LastAttemptStatus  = FmpImageInfoBuf->LastAttemptStatus;
   }
 
+  //
+  // Increment the number of active ESRT Table Entries
+  //
+  Table->FwResourceCount++;
+
   return EFI_SUCCESS;
 }
 
+/**
+  Function to retrieve the EFI_FIRMWARE_IMAGE_DESCRIPTOR from an FMP Instance.
+  The returned buffer is allocated using AllocatePool() and must be freed by the
+  caller using FreePool().
+
+  @param[in]  Fmp                        Pointer to an EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
+  @param[out] FmpImageInfoDescriptorVer  Pointer to the version number associated
+                                         with the returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+  @param[out] FmpImageInfoCount          Pointer to the number of the returned
+                                         EFI_FIRMWARE_IMAGE_DESCRIPTORs.
+  @param[out] DescriptorSize             Pointer to the size, in bytes, of each
+                                         returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+
+  @return  Pointer to the retrieved EFI_FIRMWARE_IMAGE_DESCRIPTOR.  If the
+           descriptor can not be retrieved, then NULL is returned.
+
+**/
+EFI_FIRMWARE_IMAGE_DESCRIPTOR *
+FmpGetFirmwareImageDescriptor (
+  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
+  OUT UINT32                            *FmpImageInfoDescriptorVer,
+  OUT UINT8                             *FmpImageInfoCount,
+  OUT UINTN                             *DescriptorSize
+  )
+{
+  EFI_STATUS                     Status;
+  UINTN                          ImageInfoSize;
+  UINT32                         PackageVersion;
+  CHAR16                         *PackageVersionName;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
+
+  ImageInfoSize = 0;
+  Status = Fmp->GetImageInfo (
+                  Fmp,                        // FMP Pointer
+                  &ImageInfoSize,             // Buffer Size (in this case 0)
+                  NULL,                       // NULL so we can get size
+                  FmpImageInfoDescriptorVer,  // DescriptorVersion
+                  FmpImageInfoCount,          // DescriptorCount
+                  DescriptorSize,             // DescriptorSize
+                  &PackageVersion,            // PackageVersion
+                  &PackageVersionName         // PackageVersionName
+                  );
+  if (Status != EFI_BUFFER_TOO_SMALL) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in GetImageInfo.  Status = %r\n", Status));
+    return NULL;
+  }
+
+  FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
+  if (FmpImageInfoBuf == NULL) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for FMP descriptor.\n"));
+    return NULL;
+  }
+
+  PackageVersionName = NULL;
+  Status = Fmp->GetImageInfo (
+                  Fmp,                        // FMP Pointer
+                  &ImageInfoSize,             // ImageInfoSize
+                  FmpImageInfoBuf,            // ImageInfo
+                  FmpImageInfoDescriptorVer,  // DescriptorVersion
+                  FmpImageInfoCount,          // DescriptorCount
+                  DescriptorSize,             // DescriptorSize
+                  &PackageVersion,            // PackageVersion
+                  &PackageVersionName         // PackageVersionName
+                  );
+  if (PackageVersionName != NULL) {
+    FreePool (PackageVersionName);
+  }
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status = %r\n", Status));
+    FreePool (FmpImageInfoBuf);
+    return NULL;
+  }
+
+  return FmpImageInfoBuf;
+}
+
 /**
   Function to create ESRT based on FMP Instances.
   Create ESRT table, get the descriptors from FMP Instance and
@@ -232,29 +363,26 @@ CreateFmpBasedEsrt (
   VOID
   )
 {
-  EFI_STATUS                        Status;
-  EFI_SYSTEM_RESOURCE_TABLE         *Table;
-  UINTN                             NoProtocols;
-  VOID                              **Buffer;
-  UINTN                             Index;
-  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
-  UINTN                             DescriptorSize;
-  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBuf;
-  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBufOrg;
-  UINT8                             FmpImageInfoCount;
-  UINT32                            FmpImageInfoDescriptorVer;
-  UINTN                             ImageInfoSize;
-  UINT32                            PackageVersion;
-  CHAR16                            *PackageVersionName;
+  EFI_STATUS                     Status;
+  UINTN                          NoProtocols;
+  VOID                           **Buffer;
+  UINTN                          Index;
+  UINT32                         FmpImageInfoDescriptorVer;
+  UINT8                          FmpImageInfoCount;
+  UINTN                          DescriptorSize;
+  UINT32                         NumberOfDescriptors;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
+  EFI_SYSTEM_RESOURCE_TABLE      *Table;
+  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
 
   Status             = EFI_SUCCESS;
-  Table              = NULL;
   NoProtocols        = 0;
   Buffer             = NULL;
-  PackageVersionName = NULL;
   FmpImageInfoBuf    = NULL;
-  FmpImageInfoBufOrg = NULL;
-  Fmp                = NULL;
+  OrgFmpImageInfoBuf = NULL;
+  Table              = NULL;
+  HardwareInstances  = NULL;
 
   Status = EfiLocateProtocolBuffer (
              &gEfiFirmwareManagementProtocolGuid,
@@ -266,69 +394,64 @@ CreateFmpBasedEsrt (
   }
 
   //
-  // Allocate Memory for table
+  // Count the total number of EFI_FIRMWARE_IMAGE_DESCRIPTORs
+  //
+  for (Index = 0, NumberOfDescriptors = 0; Index < NoProtocols; Index++) {
+    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
+                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
+                        &FmpImageInfoDescriptorVer,
+                        &FmpImageInfoCount,
+                        &DescriptorSize
+                        );
+    if (FmpImageInfoBuf != NULL) {
+      NumberOfDescriptors += FmpImageInfoCount;
+      FreePool (FmpImageInfoBuf);
+    }
+  }
+
+  //
+  // Allocate ESRT Table and GUID/HardwareInstance table
   //
   Table = AllocateZeroPool (
-             (GROWTH_STEP * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
+             (NumberOfDescriptors * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
              );
   if (Table == NULL) {
     DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for ESRT.\n"));
-    gBS->FreePool (Buffer);
+    FreePool (Buffer);
     return NULL;
   }
 
+  HardwareInstances = AllocateZeroPool (NumberOfDescriptors * sizeof (GUID_HARDWAREINSTANCE_PAIR));
+  if (HardwareInstances == NULL) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for HW Instance Table.\n"));
+    FreePool (Table);
+    FreePool (Buffer);
+    return NULL;
+  }
+
+  //
+  // Initialize ESRT Table
+  //
   Table->FwResourceCount    = 0;
-  Table->FwResourceCountMax = GROWTH_STEP;
+  Table->FwResourceCountMax = NumberOfDescriptors;
   Table->FwResourceVersion  = EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
 
+  NumberOfDescriptors = 0;
   for (Index = 0; Index < NoProtocols; Index++) {
-    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index];
-
-    ImageInfoSize = 0;
-    Status = Fmp->GetImageInfo (
-                    Fmp,                         // FMP Pointer
-                    &ImageInfoSize,              // Buffer Size (in this case 0)
-                    NULL,                        // NULL so we can get size
-                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
-                    &FmpImageInfoCount,          // DescriptorCount
-                    &DescriptorSize,             // DescriptorSize
-                    &PackageVersion,             // PackageVersion
-                    &PackageVersionName          // PackageVersionName
-                    );
-
-    if (Status != EFI_BUFFER_TOO_SMALL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in GetImageInfo.  Status = %r\n", Status));
-      continue;
-    }
-
-    FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
+    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
+                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
+                        &FmpImageInfoDescriptorVer,
+                        &FmpImageInfoCount,
+                        &DescriptorSize
+                        );
     if (FmpImageInfoBuf == NULL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for descriptors.\n"));
-      continue;
-    }
-
-    FmpImageInfoBufOrg = FmpImageInfoBuf;
-    PackageVersionName = NULL;
-    Status = Fmp->GetImageInfo (
-                    Fmp,
-                    &ImageInfoSize,              // ImageInfoSize
-                    FmpImageInfoBuf,             // ImageInfo
-                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
-                    &FmpImageInfoCount,          // DescriptorCount
-                    &DescriptorSize,             // DescriptorSize
-                    &PackageVersion,             // PackageVersion
-                    &PackageVersionName          // PackageVersionName
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status = %r\n", Status));
-      FreePool (FmpImageInfoBufOrg);
-      FmpImageInfoBufOrg = NULL;
       continue;
     }
 
     //
     // Check each descriptor and read from the one specified
     //
+    OrgFmpImageInfoBuf = FmpImageInfoBuf;
     while (FmpImageInfoCount > 0) {
       //
       // If the descriptor has the IN USE bit set, create ESRT entry otherwise ignore.
@@ -337,7 +460,7 @@ CreateFmpBasedEsrt (
         //
         // Create ESRT entry
         //
-        CreateEsrtEntry (&Table, FmpImageInfoBuf, FmpImageInfoDescriptorVer);
+        CreateEsrtEntry (Table, HardwareInstances, &NumberOfDescriptors, FmpImageInfoBuf, FmpImageInfoDescriptorVer);
       }
       FmpImageInfoCount--;
       //
@@ -346,15 +469,12 @@ CreateFmpBasedEsrt (
       FmpImageInfoBuf = (EFI_FIRMWARE_IMAGE_DESCRIPTOR *)(((UINT8 *)FmpImageInfoBuf) + DescriptorSize);
     }
 
-    if (PackageVersionName != NULL) {
-      FreePool (PackageVersionName);
-      PackageVersionName = NULL;
-    }
-    FreePool (FmpImageInfoBufOrg);
-    FmpImageInfoBufOrg = NULL;
+    FreePool (OrgFmpImageInfoBuf);
+    OrgFmpImageInfoBuf = NULL;
   }
 
-  gBS->FreePool (Buffer);
+  FreePool (Buffer);
+  FreePool (HardwareInstances);
   return Table;
 }
 
-- 
2.20.1.windows.1

Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers

[Wu, Hao A]

> -----Original Message-----
> From: Jin, Eric
> Sent: Monday, August 05, 2019 4:03 PM
> To: devel@edk2.groups.io
> Cc: Sean Brogan; Bret Barkelew; Wang, Jian J; Wu, Hao A; Kinney, Michael D
> Subject: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support
> multiple controllers
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1525
> 
> The patch is to merge multiple FMP instances into single ESRT entry
> when they have the same GUID.
> 
> The policy to LastAttemptStatus/LastAttemptVersion of ESRT entry is:
> If all the LastAttemptStatus are LAST_ATTEMPT_STATUS_SUCCESS, then
> LastAttemptVersion should be the smallest of LastAttemptVersion. If
> any of the LastAttemptStatus is not LAST_ATTEMPT_STATUS_SUCCESS,
> then the LastAttemptVersion/LastAttemptStatus should be the values
> of the first FMP instance whose LastAttemptStatus is not
> LAST_ATTEMPT_STATUS_SUCCESS.
> 
> To detect possible duplicated GUID/HardwareInstance, a table of
> GUID/HardwareInstance pairs from all the
> EFI_FIRMWARE_IMAGE_DESCRIPTORs
> from all FMP instances is built. If a duplicate is found, then generate
> a DEBUG_ERROR message, generate an ASSERT(), and ignore the duplicate
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> 
> Add an internal worker function called FmpGetFirmwareImageDescriptor()
> that retrieves the list of EFI_FIRMWARE_IMAGE_DESCRIPTORs from a single
> FMP instance and returns the descriptors in an allocated buffer. This
> function is used to get the descriptors used to build the table of
> unique GUID/HardwareInstance pairs.  It is then used again to generate
> the ESRT Table from all the EFI_FIRMWARE_IMAGE_DESCRIPTORs from all
> the
> FMP instances. 2 passes are performed so the total number of
> descriptors is known. This allows the correct sized buffers to always
> be allocated.


The patch looks good to me.
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

I will wait a little longer to push the patch to see if there is
additional feedbacks from stewards with regard to the approach when
merging changes from the edk2-staging repo.

Best Regards,
Hao Wu


> 
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Eric Jin <eric.jin@intel.com>
> ---
>  MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 394 +++++++++++++---
> ----
>  1 file changed, 257 insertions(+), 137 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> index 2356da662e..4670349f89 100644
> --- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> +++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> @@ -2,7 +2,7 @@
>    Publishes ESRT table from Firmware Management Protocol instances
> 
>    Copyright (c) 2016, Microsoft Corporation
> -  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
> 
>    All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -21,6 +21,22 @@
>  #include <Guid/EventGroup.h>
>  #include <Guid/SystemResourceTable.h>
> 
> +///
> +/// Structure for array of unique GUID/HardwareInstance pairs from the
> +/// current set of EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP
> Protocols.
> +///
> +typedef struct {
> +  ///
> +  /// A unique GUID identifying the firmware image type.
> +  ///
> +  EFI_GUID                      ImageTypeGuid;
> +  ///
> +  /// An optional number to identify the unique hardware instance within
> the
> +  /// system for devices that may have multiple instances whenever
> possible.
> +  ///
> +  UINT64                        HardwareInstance;
> +} GUID_HARDWAREINSTANCE_PAIR;
> +
>  /**
>   Print ESRT to debug console.
> 
> @@ -33,11 +49,6 @@ PrintTable (
>    IN EFI_SYSTEM_RESOURCE_TABLE  *Table
>    );
> 
> -//
> -// Number of ESRT entries to grow by each time we run out of room
> -//
> -#define GROWTH_STEP  10
> -
>  /**
>    Install EFI System Resource Table into the UEFI Configuration Table
> 
> @@ -101,90 +112,129 @@ IsSystemFmp (
>  }
> 
>  /**
> -  Function to create a single ESRT Entry and add it to the ESRT
> -  given a FMP descriptor.  If the guid is already in the ESRT it
> -  will be ignored.  The ESRT will grow if it does not have enough room.
> -
> -  @param[in, out] Table             On input, pointer to the pointer to the ESRT.
> -                                    On output, same as input or pointer to the pointer
> -                                    to new enlarged ESRT.
> -  @param[in]      FmpImageInfoBuf   Pointer to the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> -  @param[in]      FmpVersion        FMP Version.
> -
> -  @return  Status code.
> +  Function to create a single ESRT Entry and add it to the ESRT with
> +  a given FMP descriptor.  If the GUID is already in the ESRT, then the ESRT
> +  entry is updated.
> +
> +  @param[in,out] Table                Pointer to the ESRT Table.
> +  @param[in,out] HardwareInstances    Pointer to the
> GUID_HARDWAREINSTANCE_PAIR.
> +  @param[in,out] NumberOfDescriptors  The number of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> +  @param[in]     FmpImageInfoBuf      Pointer to the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +  @param[in]     FmpVersion           FMP Version.
> +
> +  @retval  EFI_SUCCESS     FmpImageInfoBuf was use to fill in a new ESRT
> entry
> +                           in Table.
> +  @retval  EFI_SUCCESS     The ImageTypeId GUID in FmpImageInfoBuf
> matches an
> +                           existing ESRT entry in Table, and the information
> +                           from FmpImageInfoBuf was merged into the the existing
> +                           ESRT entry.
> +  @retval  EFI_UNSPOORTED  The GUID/HardareInstance in
> FmpImageInfoBuf has is a
> +                           duplicate.
> 
>  **/
>  EFI_STATUS
>  CreateEsrtEntry (
> -  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
> -  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
> -  IN UINT32                         FmpVersion
> +  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
> +  IN OUT GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances,
> +  IN OUT UINT32                         *NumberOfDescriptors,
> +  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
> +  IN     UINT32                         FmpVersion
>    )
>  {
>    UINTN                      Index;
>    EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
> -  UINTN                      NewSize;
> -  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
> +  UINT64                     FmpHardwareInstance;
> 
> -  Index = 0;
> -  Entry = NULL;
> +  FmpHardwareInstance = 0;
> +  if (FmpVersion >= 3) {
> +    FmpHardwareInstance = FmpImageInfoBuf->HardwareInstance;
> +  }
> 
> -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) + 1);
>    //
> -  // Make sure Guid isn't already in the list
> +  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
>    //
> -  for (Index = 0; Index < (*Table)->FwResourceCount; Index++) {
> -    if (CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId)) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry already exists for FMP
> Instance with GUID %g\n", &Entry->FwClass));
> -      return EFI_INVALID_PARAMETER;
> +  for (Index = 0; Index < *NumberOfDescriptors; Index++) {
> +    if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid,
> &FmpImageInfoBuf->ImageTypeId)) {
> +      if (HardwareInstances[Index].HardwareInstance ==
> FmpHardwareInstance) {
> +        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image
> descriptor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf-
> >ImageTypeId, FmpHardwareInstance));
> +        ASSERT (
> +          !CompareGuid (&HardwareInstances[Index].ImageTypeGuid,
> &FmpImageInfoBuf->ImageTypeId) ||
> +          HardwareInstances[Index].HardwareInstance !=
> FmpHardwareInstance
> +          );
> +        return EFI_UNSUPPORTED;
> +      }
>      }
> -    Entry++;
>    }
> 
>    //
> -  // Grow table if needed
> +  // Record new GUID/HardwareInstance pair
>    //
> -  if ((*Table)->FwResourceCount >= (*Table)->FwResourceCountMax) {
> -    NewSize  = (((*Table)->FwResourceCountMax + GROWTH_STEP) * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE);
> -    NewTable = AllocateZeroPool (NewSize);
> -    if (NewTable == NULL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory larger
> table for ESRT. \n"));
> -      return EFI_OUT_OF_RESOURCES;
> +  CopyGuid (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid,
> &FmpImageInfoBuf->ImageTypeId);
> +  HardwareInstances[*NumberOfDescriptors].HardwareInstance =
> FmpHardwareInstance;
> +  *NumberOfDescriptors = *NumberOfDescriptors + 1;
> +
> +  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image descriptor with
> GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId,
> FmpHardwareInstance));
> +
> +  //
> +  // Check to see if GUID is already in the ESRT table
> +  //
> +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);
> +  for (Index = 0; Index < Table->FwResourceCount; Index++, Entry++) {
> +    if (!CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId))
> {
> +      continue;
>      }
> +    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry already exists for FMP
> Instance with GUID %g\n", &Entry->FwClass));
> +
>      //
> -    // Copy the whole old table into new table buffer
> -    //
> -    CopyMem (
> -      NewTable,
> -      (*Table),
> -      (((*Table)->FwResourceCountMax) * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
> -      );
> -    //
> -    // Update max
> +    // Set ESRT FwVersion to the smaller of the two values
>      //
> -    NewTable->FwResourceCountMax = NewTable->FwResourceCountMax +
> GROWTH_STEP;
> +    Entry->FwVersion = MIN (FmpImageInfoBuf->Version, Entry-
> >FwVersion);
> +
>      //
> -    // Free old table
> +    // VERSION 2 has Lowest Supported
>      //
> -    FreePool ((*Table));
> +    if (FmpVersion >= 2) {
> +      //
> +      // Set ESRT LowestSupportedFwVersion to the smaller of the two values
> +      //
> +      Entry->LowestSupportedFwVersion =
> +        MIN (
> +          FmpImageInfoBuf->LowestSupportedImageVersion,
> +          Entry->LowestSupportedFwVersion
> +          );
> +    }
> +
>      //
> -    // Reassign pointer to new table.
> +    // VERSION 3 supports last attempt values
>      //
> -    (*Table) = NewTable;
> +    if (FmpVersion >= 3) {
> +      //
> +      // Update the ESRT entry with the last attempt status and last attempt
> +      // version from the first FMP instance whose last attempt status is not
> +      // SUCCESS.  If all FMP instances are SUCCESS, then set version to the
> +      // smallest value from all FMP instances.
> +      //
> +      if (Entry->LastAttemptStatus == LAST_ATTEMPT_STATUS_SUCCESS) {
> +        if (FmpImageInfoBuf->LastAttemptStatus !=
> LAST_ATTEMPT_STATUS_SUCCESS) {
> +          Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
> +          Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
> +        } else {
> +          Entry->LastAttemptVersion =
> +            MIN (
> +              FmpImageInfoBuf->LastAttemptVersion,
> +              Entry->LastAttemptVersion
> +              );
> +        }
> +      }
> +    }
> +
> +    return EFI_SUCCESS;
>    }
> 
>    //
> -  // ESRT table has enough room for the new entry so add new entry
> -  //
> -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8 *)(*Table)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE));
> -  //
> -  // Move to the location of new entry
> -  //
> -  Entry = Entry + (*Table)->FwResourceCount;
> +  // Add a new ESRT Table Entry
>    //
> -  // Increment resource count
> -  //
> -  (*Table)->FwResourceCount++;
> +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) + Table-
> >FwResourceCount;
> 
>    CopyGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId);
> 
> @@ -195,11 +245,11 @@ CreateEsrtEntry (
>      Entry->FwType = (UINT32)(ESRT_FW_TYPE_DEVICEFIRMWARE);
>    }
> 
> -  Entry->FwVersion = FmpImageInfoBuf->Version;
> +  Entry->FwVersion                = FmpImageInfoBuf->Version;
>    Entry->LowestSupportedFwVersion = 0;
> -  Entry->CapsuleFlags = 0;
> -  Entry->LastAttemptVersion = 0;
> -  Entry->LastAttemptStatus = 0;
> +  Entry->CapsuleFlags             = 0;
> +  Entry->LastAttemptVersion       = 0;
> +  Entry->LastAttemptStatus        = 0;
> 
>    //
>    // VERSION 2 has Lowest Supported
> @@ -213,12 +263,93 @@ CreateEsrtEntry (
>    //
>    if (FmpVersion >= 3) {
>      Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
> -    Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
> +    Entry->LastAttemptStatus  = FmpImageInfoBuf->LastAttemptStatus;
>    }
> 
> +  //
> +  // Increment the number of active ESRT Table Entries
> +  //
> +  Table->FwResourceCount++;
> +
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Function to retrieve the EFI_FIRMWARE_IMAGE_DESCRIPTOR from an
> FMP Instance.
> +  The returned buffer is allocated using AllocatePool() and must be freed by
> the
> +  caller using FreePool().
> +
> +  @param[in]  Fmp                        Pointer to an
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> +  @param[out] FmpImageInfoDescriptorVer  Pointer to the version number
> associated
> +                                         with the returned
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +  @param[out] FmpImageInfoCount          Pointer to the number of the
> returned
> +                                         EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> +  @param[out] DescriptorSize             Pointer to the size, in bytes, of each
> +                                         returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +
> +  @return  Pointer to the retrieved EFI_FIRMWARE_IMAGE_DESCRIPTOR.  If
> the
> +           descriptor can not be retrieved, then NULL is returned.
> +
> +**/
> +EFI_FIRMWARE_IMAGE_DESCRIPTOR *
> +FmpGetFirmwareImageDescriptor (
> +  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
> +  OUT UINT32                            *FmpImageInfoDescriptorVer,
> +  OUT UINT8                             *FmpImageInfoCount,
> +  OUT UINTN                             *DescriptorSize
> +  )
> +{
> +  EFI_STATUS                     Status;
> +  UINTN                          ImageInfoSize;
> +  UINT32                         PackageVersion;
> +  CHAR16                         *PackageVersionName;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> +
> +  ImageInfoSize = 0;
> +  Status = Fmp->GetImageInfo (
> +                  Fmp,                        // FMP Pointer
> +                  &ImageInfoSize,             // Buffer Size (in this case 0)
> +                  NULL,                       // NULL so we can get size
> +                  FmpImageInfoDescriptorVer,  // DescriptorVersion
> +                  FmpImageInfoCount,          // DescriptorCount
> +                  DescriptorSize,             // DescriptorSize
> +                  &PackageVersion,            // PackageVersion
> +                  &PackageVersionName         // PackageVersionName
> +                  );
> +  if (Status != EFI_BUFFER_TOO_SMALL) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in
> GetImageInfo.  Status = %r\n", Status));
> +    return NULL;
> +  }
> +
> +  FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
> +  if (FmpImageInfoBuf == NULL) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for FMP
> descriptor.\n"));
> +    return NULL;
> +  }
> +
> +  PackageVersionName = NULL;
> +  Status = Fmp->GetImageInfo (
> +                  Fmp,                        // FMP Pointer
> +                  &ImageInfoSize,             // ImageInfoSize
> +                  FmpImageInfoBuf,            // ImageInfo
> +                  FmpImageInfoDescriptorVer,  // DescriptorVersion
> +                  FmpImageInfoCount,          // DescriptorCount
> +                  DescriptorSize,             // DescriptorSize
> +                  &PackageVersion,            // PackageVersion
> +                  &PackageVersionName         // PackageVersionName
> +                  );
> +  if (PackageVersionName != NULL) {
> +    FreePool (PackageVersionName);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status
> = %r\n", Status));
> +    FreePool (FmpImageInfoBuf);
> +    return NULL;
> +  }
> +
> +  return FmpImageInfoBuf;
> +}
> +
>  /**
>    Function to create ESRT based on FMP Instances.
>    Create ESRT table, get the descriptors from FMP Instance and
> @@ -232,29 +363,26 @@ CreateFmpBasedEsrt (
>    VOID
>    )
>  {
> -  EFI_STATUS                        Status;
> -  EFI_SYSTEM_RESOURCE_TABLE         *Table;
> -  UINTN                             NoProtocols;
> -  VOID                              **Buffer;
> -  UINTN                             Index;
> -  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
> -  UINTN                             DescriptorSize;
> -  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBuf;
> -  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBufOrg;
> -  UINT8                             FmpImageInfoCount;
> -  UINT32                            FmpImageInfoDescriptorVer;
> -  UINTN                             ImageInfoSize;
> -  UINT32                            PackageVersion;
> -  CHAR16                            *PackageVersionName;
> +  EFI_STATUS                     Status;
> +  UINTN                          NoProtocols;
> +  VOID                           **Buffer;
> +  UINTN                          Index;
> +  UINT32                         FmpImageInfoDescriptorVer;
> +  UINT8                          FmpImageInfoCount;
> +  UINTN                          DescriptorSize;
> +  UINT32                         NumberOfDescriptors;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
> +  EFI_SYSTEM_RESOURCE_TABLE      *Table;
> +  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
> 
>    Status             = EFI_SUCCESS;
> -  Table              = NULL;
>    NoProtocols        = 0;
>    Buffer             = NULL;
> -  PackageVersionName = NULL;
>    FmpImageInfoBuf    = NULL;
> -  FmpImageInfoBufOrg = NULL;
> -  Fmp                = NULL;
> +  OrgFmpImageInfoBuf = NULL;
> +  Table              = NULL;
> +  HardwareInstances  = NULL;
> 
>    Status = EfiLocateProtocolBuffer (
>               &gEfiFirmwareManagementProtocolGuid,
> @@ -266,69 +394,64 @@ CreateFmpBasedEsrt (
>    }
> 
>    //
> -  // Allocate Memory for table
> +  // Count the total number of EFI_FIRMWARE_IMAGE_DESCRIPTORs
> +  //
> +  for (Index = 0, NumberOfDescriptors = 0; Index < NoProtocols; Index++) {
> +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
> +                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> +                        &FmpImageInfoDescriptorVer,
> +                        &FmpImageInfoCount,
> +                        &DescriptorSize
> +                        );
> +    if (FmpImageInfoBuf != NULL) {
> +      NumberOfDescriptors += FmpImageInfoCount;
> +      FreePool (FmpImageInfoBuf);
> +    }
> +  }
> +
> +  //
> +  // Allocate ESRT Table and GUID/HardwareInstance table
>    //
>    Table = AllocateZeroPool (
> -             (GROWTH_STEP * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE)
> +             (NumberOfDescriptors * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) +
> sizeof (EFI_SYSTEM_RESOURCE_TABLE)
>               );
>    if (Table == NULL) {
>      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for
> ESRT.\n"));
> -    gBS->FreePool (Buffer);
> +    FreePool (Buffer);
>      return NULL;
>    }
> 
> +  HardwareInstances = AllocateZeroPool (NumberOfDescriptors * sizeof
> (GUID_HARDWAREINSTANCE_PAIR));
> +  if (HardwareInstances == NULL) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for HW
> Instance Table.\n"));
> +    FreePool (Table);
> +    FreePool (Buffer);
> +    return NULL;
> +  }
> +
> +  //
> +  // Initialize ESRT Table
> +  //
>    Table->FwResourceCount    = 0;
> -  Table->FwResourceCountMax = GROWTH_STEP;
> +  Table->FwResourceCountMax = NumberOfDescriptors;
>    Table->FwResourceVersion  =
> EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
> 
> +  NumberOfDescriptors = 0;
>    for (Index = 0; Index < NoProtocols; Index++) {
> -    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index];
> -
> -    ImageInfoSize = 0;
> -    Status = Fmp->GetImageInfo (
> -                    Fmp,                         // FMP Pointer
> -                    &ImageInfoSize,              // Buffer Size (in this case 0)
> -                    NULL,                        // NULL so we can get size
> -                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
> -                    &FmpImageInfoCount,          // DescriptorCount
> -                    &DescriptorSize,             // DescriptorSize
> -                    &PackageVersion,             // PackageVersion
> -                    &PackageVersionName          // PackageVersionName
> -                    );
> -
> -    if (Status != EFI_BUFFER_TOO_SMALL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in
> GetImageInfo.  Status = %r\n", Status));
> -      continue;
> -    }
> -
> -    FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
> +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
> +                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> +                        &FmpImageInfoDescriptorVer,
> +                        &FmpImageInfoCount,
> +                        &DescriptorSize
> +                        );
>      if (FmpImageInfoBuf == NULL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for
> descriptors.\n"));
> -      continue;
> -    }
> -
> -    FmpImageInfoBufOrg = FmpImageInfoBuf;
> -    PackageVersionName = NULL;
> -    Status = Fmp->GetImageInfo (
> -                    Fmp,
> -                    &ImageInfoSize,              // ImageInfoSize
> -                    FmpImageInfoBuf,             // ImageInfo
> -                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
> -                    &FmpImageInfoCount,          // DescriptorCount
> -                    &DescriptorSize,             // DescriptorSize
> -                    &PackageVersion,             // PackageVersion
> -                    &PackageVersionName          // PackageVersionName
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status
> = %r\n", Status));
> -      FreePool (FmpImageInfoBufOrg);
> -      FmpImageInfoBufOrg = NULL;
>        continue;
>      }
> 
>      //
>      // Check each descriptor and read from the one specified
>      //
> +    OrgFmpImageInfoBuf = FmpImageInfoBuf;
>      while (FmpImageInfoCount > 0) {
>        //
>        // If the descriptor has the IN USE bit set, create ESRT entry otherwise
> ignore.
> @@ -337,7 +460,7 @@ CreateFmpBasedEsrt (
>          //
>          // Create ESRT entry
>          //
> -        CreateEsrtEntry (&Table, FmpImageInfoBuf,
> FmpImageInfoDescriptorVer);
> +        CreateEsrtEntry (Table, HardwareInstances, &NumberOfDescriptors,
> FmpImageInfoBuf, FmpImageInfoDescriptorVer);
>        }
>        FmpImageInfoCount--;
>        //
> @@ -346,15 +469,12 @@ CreateFmpBasedEsrt (
>        FmpImageInfoBuf = (EFI_FIRMWARE_IMAGE_DESCRIPTOR *)(((UINT8
> *)FmpImageInfoBuf) + DescriptorSize);
>      }
> 
> -    if (PackageVersionName != NULL) {
> -      FreePool (PackageVersionName);
> -      PackageVersionName = NULL;
> -    }
> -    FreePool (FmpImageInfoBufOrg);
> -    FmpImageInfoBufOrg = NULL;
> +    FreePool (OrgFmpImageInfoBuf);
> +    OrgFmpImageInfoBuf = NULL;
>    }
> 
> -  gBS->FreePool (Buffer);
> +  FreePool (Buffer);
> +  FreePool (HardwareInstances);
>    return Table;
>  }
> 
> --
> 2.20.1.windows.1

Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers

[Michael D Kinney]

Hao Wu,

I agree that patches should be cleaned up for 
submission to edk2 repo.  The work in edk2-staging 
may contain bug fixes and design changes in the
patch history that can be consolidated to a smaller
patch set.  Also, all feedback to a patch set from 
edk2-staging must be addressed just like any other
submission which may include splitting or merging
patches.

Mike

> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, August 5, 2019 7:49 PM
> To: Jin, Eric <eric.jin@intel.com>;
> devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret
> Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; afish@apple.com;
> lersek@redhat.com; leif.lindholm@linaro.org
> Subject: RE: [PATCH v2] MdeModulePkg/EsrtFmpDxe:
> Enhance ESRT to support multiple controllers
> 
> > -----Original Message-----
> > From: Jin, Eric
> > Sent: Monday, August 05, 2019 4:03 PM
> > To: devel@edk2.groups.io
> > Cc: Sean Brogan; Bret Barkelew; Wang, Jian J; Wu, Hao
> A; Kinney,
> > Michael D
> > Subject: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance
> ESRT to support
> > multiple controllers
> >
> > REF:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1525
> >
> > The patch is to merge multiple FMP instances into
> single ESRT entry
> > when they have the same GUID.
> >
> > The policy to LastAttemptStatus/LastAttemptVersion of
> ESRT entry is:
> > If all the LastAttemptStatus are
> LAST_ATTEMPT_STATUS_SUCCESS, then
> > LastAttemptVersion should be the smallest of
> LastAttemptVersion. If
> > any of the LastAttemptStatus is not
> LAST_ATTEMPT_STATUS_SUCCESS, then
> > the LastAttemptVersion/LastAttemptStatus should be
> the values of the
> > first FMP instance whose LastAttemptStatus is not
> > LAST_ATTEMPT_STATUS_SUCCESS.
> >
> > To detect possible duplicated GUID/HardwareInstance,
> a table of
> > GUID/HardwareInstance pairs from all the
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP instances
> is built. If a
> > duplicate is found, then generate a DEBUG_ERROR
> message, generate an
> > ASSERT(), and ignore the duplicate
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> >
> > Add an internal worker function called
> FmpGetFirmwareImageDescriptor()
> > that retrieves the list of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs from a
> > single FMP instance and returns the descriptors in an
> allocated
> > buffer. This function is used to get the descriptors
> used to build the
> > table of unique GUID/HardwareInstance pairs.  It is
> then used again to
> > generate the ESRT Table from all the
> EFI_FIRMWARE_IMAGE_DESCRIPTORs
> > from all the FMP instances. 2 passes are performed so
> the total number
> > of descriptors is known. This allows the correct
> sized buffers to
> > always be allocated.
> 
> 
> The patch looks good to me.
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> 
> I will wait a little longer to push the patch to see if
> there is additional feedbacks from stewards with regard
> to the approach when merging changes from the edk2-
> staging repo.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > Signed-off-by: Eric Jin <eric.jin@intel.com>
> > ---
> >  MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 394
> +++++++++++++---
> > ----
> >  1 file changed, 257 insertions(+), 137 deletions(-)
> >
> > diff --git
> a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > index 2356da662e..4670349f89 100644
> > --- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > +++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > @@ -2,7 +2,7 @@
> >    Publishes ESRT table from Firmware Management
> Protocol instances
> >
> >    Copyright (c) 2016, Microsoft Corporation
> > -  Copyright (c) 2018, Intel Corporation. All rights
> reserved.<BR>
> > +  Copyright (c) 2018 - 2019, Intel Corporation. All
> rights
> > + reserved.<BR>
> >
> >    All rights reserved.
> >    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -
> 21,6 +21,22 @@
> > #include <Guid/EventGroup.h>  #include
> <Guid/SystemResourceTable.h>
> >
> > +///
> > +/// Structure for array of unique
> GUID/HardwareInstance pairs from
> > +the /// current set of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP
> > Protocols.
> > +///
> > +typedef struct {
> > +  ///
> > +  /// A unique GUID identifying the firmware image
> type.
> > +  ///
> > +  EFI_GUID                      ImageTypeGuid;
> > +  ///
> > +  /// An optional number to identify the unique
> hardware instance
> > +within
> > the
> > +  /// system for devices that may have multiple
> instances whenever
> > possible.
> > +  ///
> > +  UINT64                        HardwareInstance;
> > +} GUID_HARDWAREINSTANCE_PAIR;
> > +
> >  /**
> >   Print ESRT to debug console.
> >
> > @@ -33,11 +49,6 @@ PrintTable (
> >    IN EFI_SYSTEM_RESOURCE_TABLE  *Table
> >    );
> >
> > -//
> > -// Number of ESRT entries to grow by each time we
> run out of room -//
> > -#define GROWTH_STEP  10
> > -
> >  /**
> >    Install EFI System Resource Table into the UEFI
> Configuration Table
> >
> > @@ -101,90 +112,129 @@ IsSystemFmp (
> >  }
> >
> >  /**
> > -  Function to create a single ESRT Entry and add it
> to the ESRT
> > -  given a FMP descriptor.  If the guid is already in
> the ESRT it
> > -  will be ignored.  The ESRT will grow if it does
> not have enough room.
> > -
> > -  @param[in, out] Table             On input,
> pointer to the pointer to the ESRT.
> > -                                    On output, same
> as input or pointer to the pointer
> > -                                    to new enlarged
> ESRT.
> > -  @param[in]      FmpImageInfoBuf   Pointer to the
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > -  @param[in]      FmpVersion        FMP Version.
> > -
> > -  @return  Status code.
> > +  Function to create a single ESRT Entry and add it
> to the ESRT with
> > + a given FMP descriptor.  If the GUID is already in
> the ESRT, then
> > + the ESRT  entry is updated.
> > +
> > +  @param[in,out] Table                Pointer to the
> ESRT Table.
> > +  @param[in,out] HardwareInstances    Pointer to the
> > GUID_HARDWAREINSTANCE_PAIR.
> > +  @param[in,out] NumberOfDescriptors  The number of
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > +  @param[in]     FmpImageInfoBuf      Pointer to the
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > +  @param[in]     FmpVersion           FMP Version.
> > +
> > +  @retval  EFI_SUCCESS     FmpImageInfoBuf was use
> to fill in a new ESRT
> > entry
> > +                           in Table.
> > +  @retval  EFI_SUCCESS     The ImageTypeId GUID in
> FmpImageInfoBuf
> > matches an
> > +                           existing ESRT entry in
> Table, and the information
> > +                           from FmpImageInfoBuf was
> merged into the the existing
> > +                           ESRT entry.
> > +  @retval  EFI_UNSPOORTED  The GUID/HardareInstance
> in
> > FmpImageInfoBuf has is a
> > +                           duplicate.
> >
> >  **/
> >  EFI_STATUS
> >  CreateEsrtEntry (
> > -  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
> > -  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBuf,
> > -  IN UINT32                         FmpVersion
> > +  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
> > +  IN OUT GUID_HARDWAREINSTANCE_PAIR
> *HardwareInstances,
> > +  IN OUT UINT32
> *NumberOfDescriptors,
> > +  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBuf,
> > +  IN     UINT32                         FmpVersion
> >    )
> >  {
> >    UINTN                      Index;
> >    EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
> > -  UINTN                      NewSize;
> > -  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
> > +  UINT64                     FmpHardwareInstance;
> >
> > -  Index = 0;
> > -  Entry = NULL;
> > +  FmpHardwareInstance = 0;
> > +  if (FmpVersion >= 3) {
> > +    FmpHardwareInstance = FmpImageInfoBuf-
> >HardwareInstance;
> > +  }
> >
> > -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) +
> 1);
> >    //
> > -  // Make sure Guid isn't already in the list
> > +  // Check to see of FmpImageInfoBuf
> GUID/HardwareInstance is unique
> >    //
> > -  for (Index = 0; Index < (*Table)->FwResourceCount;
> Index++) {
> > -    if (CompareGuid (&Entry->FwClass,
> &FmpImageInfoBuf->ImageTypeId)) {
> > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry
> already exists for FMP
> > Instance with GUID %g\n", &Entry->FwClass));
> > -      return EFI_INVALID_PARAMETER;
> > +  for (Index = 0; Index < *NumberOfDescriptors;
> Index++) {
> > +    if (CompareGuid
> (&HardwareInstances[Index].ImageTypeGuid,
> > &FmpImageInfoBuf->ImageTypeId)) {
> > +      if (HardwareInstances[Index].HardwareInstance
> ==
> > FmpHardwareInstance) {
> > +        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate
> firmware image
> > descriptor with GUID %g HardwareInstance:0x%x\n",
> &FmpImageInfoBuf-
> > >ImageTypeId, FmpHardwareInstance));
> > +        ASSERT (
> > +          !CompareGuid
> (&HardwareInstances[Index].ImageTypeGuid,
> > &FmpImageInfoBuf->ImageTypeId) ||
> > +          HardwareInstances[Index].HardwareInstance
> !=
> > FmpHardwareInstance
> > +          );
> > +        return EFI_UNSUPPORTED;
> > +      }
> >      }
> > -    Entry++;
> >    }
> >
> >    //
> > -  // Grow table if needed
> > +  // Record new GUID/HardwareInstance pair
> >    //
> > -  if ((*Table)->FwResourceCount >= (*Table)-
> >FwResourceCountMax) {
> > -    NewSize  = (((*Table)->FwResourceCountMax +
> GROWTH_STEP) * sizeof
> > (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE);
> > -    NewTable = AllocateZeroPool (NewSize);
> > -    if (NewTable == NULL) {
> > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory larger
> > table for ESRT. \n"));
> > -      return EFI_OUT_OF_RESOURCES;
> > +  CopyGuid
> (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid
> ,
> > &FmpImageInfoBuf->ImageTypeId);
> > +
> HardwareInstances[*NumberOfDescriptors].HardwareInstanc
> e =
> > FmpHardwareInstance;
> > +  *NumberOfDescriptors = *NumberOfDescriptors + 1;
> > +
> > +  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image
> descriptor with
> > GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf-
> >ImageTypeId,
> > FmpHardwareInstance));
> > +
> > +  //
> > +  // Check to see if GUID is already in the ESRT
> table  //  Entry =
> > + (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);  for
> (Index = 0; Index <
> > + Table->FwResourceCount; Index++, Entry++) {
> > +    if (!CompareGuid (&Entry->FwClass,
> > + &FmpImageInfoBuf->ImageTypeId))
> > {
> > +      continue;
> >      }
> > +    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry
> already exists for
> > + FMP
> > Instance with GUID %g\n", &Entry->FwClass));
> > +
> >      //
> > -    // Copy the whole old table into new table
> buffer
> > -    //
> > -    CopyMem (
> > -      NewTable,
> > -      (*Table),
> > -      (((*Table)->FwResourceCountMax) * sizeof
> > (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE)
> > -      );
> > -    //
> > -    // Update max
> > +    // Set ESRT FwVersion to the smaller of the two
> values
> >      //
> > -    NewTable->FwResourceCountMax = NewTable-
> >FwResourceCountMax +
> > GROWTH_STEP;
> > +    Entry->FwVersion = MIN (FmpImageInfoBuf-
> >Version, Entry-
> > >FwVersion);
> > +
> >      //
> > -    // Free old table
> > +    // VERSION 2 has Lowest Supported
> >      //
> > -    FreePool ((*Table));
> > +    if (FmpVersion >= 2) {
> > +      //
> > +      // Set ESRT LowestSupportedFwVersion to the
> smaller of the two values
> > +      //
> > +      Entry->LowestSupportedFwVersion =
> > +        MIN (
> > +          FmpImageInfoBuf-
> >LowestSupportedImageVersion,
> > +          Entry->LowestSupportedFwVersion
> > +          );
> > +    }
> > +
> >      //
> > -    // Reassign pointer to new table.
> > +    // VERSION 3 supports last attempt values
> >      //
> > -    (*Table) = NewTable;
> > +    if (FmpVersion >= 3) {
> > +      //
> > +      // Update the ESRT entry with the last attempt
> status and last attempt
> > +      // version from the first FMP instance whose
> last attempt status is not
> > +      // SUCCESS.  If all FMP instances are SUCCESS,
> then set version to the
> > +      // smallest value from all FMP instances.
> > +      //
> > +      if (Entry->LastAttemptStatus ==
> LAST_ATTEMPT_STATUS_SUCCESS) {
> > +        if (FmpImageInfoBuf->LastAttemptStatus !=
> > LAST_ATTEMPT_STATUS_SUCCESS) {
> > +          Entry->LastAttemptStatus =
> FmpImageInfoBuf->LastAttemptStatus;
> > +          Entry->LastAttemptVersion =
> FmpImageInfoBuf->LastAttemptVersion;
> > +        } else {
> > +          Entry->LastAttemptVersion =
> > +            MIN (
> > +              FmpImageInfoBuf->LastAttemptVersion,
> > +              Entry->LastAttemptVersion
> > +              );
> > +        }
> > +      }
> > +    }
> > +
> > +    return EFI_SUCCESS;
> >    }
> >
> >    //
> > -  // ESRT table has enough room for the new entry so
> add new entry
> > -  //
> > -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8
> *)(*Table)) + sizeof
> > (EFI_SYSTEM_RESOURCE_TABLE));
> > -  //
> > -  // Move to the location of new entry
> > -  //
> > -  Entry = Entry + (*Table)->FwResourceCount;
> > +  // Add a new ESRT Table Entry
> >    //
> > -  // Increment resource count
> > -  //
> > -  (*Table)->FwResourceCount++;
> > +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) +
> Table-
> > >FwResourceCount;
> >
> >    CopyGuid (&Entry->FwClass, &FmpImageInfoBuf-
> >ImageTypeId);
> >
> > @@ -195,11 +245,11 @@ CreateEsrtEntry (
> >      Entry->FwType =
> (UINT32)(ESRT_FW_TYPE_DEVICEFIRMWARE);
> >    }
> >
> > -  Entry->FwVersion = FmpImageInfoBuf->Version;
> > +  Entry->FwVersion                = FmpImageInfoBuf-
> >Version;
> >    Entry->LowestSupportedFwVersion = 0;
> > -  Entry->CapsuleFlags = 0;
> > -  Entry->LastAttemptVersion = 0;
> > -  Entry->LastAttemptStatus = 0;
> > +  Entry->CapsuleFlags             = 0;
> > +  Entry->LastAttemptVersion       = 0;
> > +  Entry->LastAttemptStatus        = 0;
> >
> >    //
> >    // VERSION 2 has Lowest Supported
> > @@ -213,12 +263,93 @@ CreateEsrtEntry (
> >    //
> >    if (FmpVersion >= 3) {
> >      Entry->LastAttemptVersion = FmpImageInfoBuf-
> >LastAttemptVersion;
> > -    Entry->LastAttemptStatus = FmpImageInfoBuf-
> >LastAttemptStatus;
> > +    Entry->LastAttemptStatus  = FmpImageInfoBuf-
> >LastAttemptStatus;
> >    }
> >
> > +  //
> > +  // Increment the number of active ESRT Table
> Entries  //
> > + Table->FwResourceCount++;
> > +
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Function to retrieve the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR from an
> > FMP Instance.
> > +  The returned buffer is allocated using
> AllocatePool() and must be
> > + freed by
> > the
> > +  caller using FreePool().
> > +
> > +  @param[in]  Fmp                        Pointer to
> an
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> > +  @param[out] FmpImageInfoDescriptorVer  Pointer to
> the version
> > + number
> > associated
> > +                                         with the
> returned
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > +  @param[out] FmpImageInfoCount          Pointer to
> the number of the
> > returned
> > +
> EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > +  @param[out] DescriptorSize             Pointer to
> the size, in bytes, of each
> > +                                         returned
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > +
> > +  @return  Pointer to the retrieved
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > + If
> > the
> > +           descriptor can not be retrieved, then
> NULL is returned.
> > +
> > +**/
> > +EFI_FIRMWARE_IMAGE_DESCRIPTOR *
> > +FmpGetFirmwareImageDescriptor (
> > +  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
> > +  OUT UINT32
> *FmpImageInfoDescriptorVer,
> > +  OUT UINT8
> *FmpImageInfoCount,
> > +  OUT UINTN
> *DescriptorSize
> > +  )
> > +{
> > +  EFI_STATUS                     Status;
> > +  UINTN                          ImageInfoSize;
> > +  UINT32                         PackageVersion;
> > +  CHAR16
> *PackageVersionName;
> > +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> > +
> > +  ImageInfoSize = 0;
> > +  Status = Fmp->GetImageInfo (
> > +                  Fmp,                        // FMP
> Pointer
> > +                  &ImageInfoSize,             //
> Buffer Size (in this case 0)
> > +                  NULL,                       //
> NULL so we can get size
> > +                  FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> > +                  FmpImageInfoCount,          //
> DescriptorCount
> > +                  DescriptorSize,             //
> DescriptorSize
> > +                  &PackageVersion,            //
> PackageVersion
> > +                  &PackageVersionName         //
> PackageVersionName
> > +                  );
> > +  if (Status != EFI_BUFFER_TOO_SMALL) {
> > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> Failure in
> > GetImageInfo.  Status = %r\n", Status));
> > +    return NULL;
> > +  }
> > +
> > +  FmpImageInfoBuf = AllocateZeroPool
> (ImageInfoSize);  if
> > + (FmpImageInfoBuf == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get
> memory for FMP
> > descriptor.\n"));
> > +    return NULL;
> > +  }
> > +
> > +  PackageVersionName = NULL;
> > +  Status = Fmp->GetImageInfo (
> > +                  Fmp,                        // FMP
> Pointer
> > +                  &ImageInfoSize,             //
> ImageInfoSize
> > +                  FmpImageInfoBuf,            //
> ImageInfo
> > +                  FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> > +                  FmpImageInfoCount,          //
> DescriptorCount
> > +                  DescriptorSize,             //
> DescriptorSize
> > +                  &PackageVersion,            //
> PackageVersion
> > +                  &PackageVersionName         //
> PackageVersionName
> > +                  );
> > +  if (PackageVersionName != NULL) {
> > +    FreePool (PackageVersionName);
> > +  }
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> GetImageInfo.
> > + Status
> > = %r\n", Status));
> > +    FreePool (FmpImageInfoBuf);
> > +    return NULL;
> > +  }
> > +
> > +  return FmpImageInfoBuf;
> > +}
> > +
> >  /**
> >    Function to create ESRT based on FMP Instances.
> >    Create ESRT table, get the descriptors from FMP
> Instance and @@
> > -232,29 +363,26 @@ CreateFmpBasedEsrt (
> >    VOID
> >    )
> >  {
> > -  EFI_STATUS                        Status;
> > -  EFI_SYSTEM_RESOURCE_TABLE         *Table;
> > -  UINTN                             NoProtocols;
> > -  VOID                              **Buffer;
> > -  UINTN                             Index;
> > -  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
> > -  UINTN                             DescriptorSize;
> > -  EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBuf;
> > -  EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBufOrg;
> > -  UINT8
> FmpImageInfoCount;
> > -  UINT32
> FmpImageInfoDescriptorVer;
> > -  UINTN                             ImageInfoSize;
> > -  UINT32                            PackageVersion;
> > -  CHAR16
> *PackageVersionName;
> > +  EFI_STATUS                     Status;
> > +  UINTN                          NoProtocols;
> > +  VOID                           **Buffer;
> > +  UINTN                          Index;
> > +  UINT32
> FmpImageInfoDescriptorVer;
> > +  UINT8                          FmpImageInfoCount;
> > +  UINTN                          DescriptorSize;
> > +  UINT32
> NumberOfDescriptors;
> > +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> > + EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
> > +  EFI_SYSTEM_RESOURCE_TABLE      *Table;
> > +  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
> >
> >    Status             = EFI_SUCCESS;
> > -  Table              = NULL;
> >    NoProtocols        = 0;
> >    Buffer             = NULL;
> > -  PackageVersionName = NULL;
> >    FmpImageInfoBuf    = NULL;
> > -  FmpImageInfoBufOrg = NULL;
> > -  Fmp                = NULL;
> > +  OrgFmpImageInfoBuf = NULL;
> > +  Table              = NULL;
> > +  HardwareInstances  = NULL;
> >
> >    Status = EfiLocateProtocolBuffer (
> >               &gEfiFirmwareManagementProtocolGuid,
> > @@ -266,69 +394,64 @@ CreateFmpBasedEsrt (
> >    }
> >
> >    //
> > -  // Allocate Memory for table
> > +  // Count the total number of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs  //
> > + for (Index = 0, NumberOfDescriptors = 0; Index <
> NoProtocols; Index++) {
> > +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor
> (
> > +
> (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> > +                        &FmpImageInfoDescriptorVer,
> > +                        &FmpImageInfoCount,
> > +                        &DescriptorSize
> > +                        );
> > +    if (FmpImageInfoBuf != NULL) {
> > +      NumberOfDescriptors += FmpImageInfoCount;
> > +      FreePool (FmpImageInfoBuf);
> > +    }
> > +  }
> > +
> > +  //
> > +  // Allocate ESRT Table and GUID/HardwareInstance
> table
> >    //
> >    Table = AllocateZeroPool (
> > -             (GROWTH_STEP * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> > (EFI_SYSTEM_RESOURCE_TABLE)
> > +             (NumberOfDescriptors * sizeof
> > + (EFI_SYSTEM_RESOURCE_ENTRY)) +
> > sizeof (EFI_SYSTEM_RESOURCE_TABLE)
> >               );
> >    if (Table == NULL) {
> >      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory for
> > ESRT.\n"));
> > -    gBS->FreePool (Buffer);
> > +    FreePool (Buffer);
> >      return NULL;
> >    }
> >
> > +  HardwareInstances = AllocateZeroPool
> (NumberOfDescriptors * sizeof
> > (GUID_HARDWAREINSTANCE_PAIR));
> > +  if (HardwareInstances == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory for
> > + HW
> > Instance Table.\n"));
> > +    FreePool (Table);
> > +    FreePool (Buffer);
> > +    return NULL;
> > +  }
> > +
> > +  //
> > +  // Initialize ESRT Table
> > +  //
> >    Table->FwResourceCount    = 0;
> > -  Table->FwResourceCountMax = GROWTH_STEP;
> > +  Table->FwResourceCountMax = NumberOfDescriptors;
> >    Table->FwResourceVersion  =
> > EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
> >
> > +  NumberOfDescriptors = 0;
> >    for (Index = 0; Index < NoProtocols; Index++) {
> > -    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *)
> Buffer[Index];
> > -
> > -    ImageInfoSize = 0;
> > -    Status = Fmp->GetImageInfo (
> > -                    Fmp,                         //
> FMP Pointer
> > -                    &ImageInfoSize,              //
> Buffer Size (in this case 0)
> > -                    NULL,                        //
> NULL so we can get size
> > -                    &FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> > -                    &FmpImageInfoCount,          //
> DescriptorCount
> > -                    &DescriptorSize,             //
> DescriptorSize
> > -                    &PackageVersion,             //
> PackageVersion
> > -                    &PackageVersionName          //
> PackageVersionName
> > -                    );
> > -
> > -    if (Status != EFI_BUFFER_TOO_SMALL) {
> > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> Failure in
> > GetImageInfo.  Status = %r\n", Status));
> > -      continue;
> > -    }
> > -
> > -    FmpImageInfoBuf = AllocateZeroPool
> (ImageInfoSize);
> > +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor
> (
> > +
> (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> > +                        &FmpImageInfoDescriptorVer,
> > +                        &FmpImageInfoCount,
> > +                        &DescriptorSize
> > +                        );
> >      if (FmpImageInfoBuf == NULL) {
> > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> get memory for
> > descriptors.\n"));
> > -      continue;
> > -    }
> > -
> > -    FmpImageInfoBufOrg = FmpImageInfoBuf;
> > -    PackageVersionName = NULL;
> > -    Status = Fmp->GetImageInfo (
> > -                    Fmp,
> > -                    &ImageInfoSize,              //
> ImageInfoSize
> > -                    FmpImageInfoBuf,             //
> ImageInfo
> > -                    &FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> > -                    &FmpImageInfoCount,          //
> DescriptorCount
> > -                    &DescriptorSize,             //
> DescriptorSize
> > -                    &PackageVersion,             //
> PackageVersion
> > -                    &PackageVersionName          //
> PackageVersionName
> > -                    );
> > -    if (EFI_ERROR (Status)) {
> > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> GetImageInfo.  Status
> > = %r\n", Status));
> > -      FreePool (FmpImageInfoBufOrg);
> > -      FmpImageInfoBufOrg = NULL;
> >        continue;
> >      }
> >
> >      //
> >      // Check each descriptor and read from the one
> specified
> >      //
> > +    OrgFmpImageInfoBuf = FmpImageInfoBuf;
> >      while (FmpImageInfoCount > 0) {
> >        //
> >        // If the descriptor has the IN USE bit set,
> create ESRT entry
> > otherwise ignore.
> > @@ -337,7 +460,7 @@ CreateFmpBasedEsrt (
> >          //
> >          // Create ESRT entry
> >          //
> > -        CreateEsrtEntry (&Table, FmpImageInfoBuf,
> > FmpImageInfoDescriptorVer);
> > +        CreateEsrtEntry (Table, HardwareInstances,
> > + &NumberOfDescriptors,
> > FmpImageInfoBuf, FmpImageInfoDescriptorVer);
> >        }
> >        FmpImageInfoCount--;
> >        //
> > @@ -346,15 +469,12 @@ CreateFmpBasedEsrt (
> >        FmpImageInfoBuf =
> (EFI_FIRMWARE_IMAGE_DESCRIPTOR *)(((UINT8
> > *)FmpImageInfoBuf) + DescriptorSize);
> >      }
> >
> > -    if (PackageVersionName != NULL) {
> > -      FreePool (PackageVersionName);
> > -      PackageVersionName = NULL;
> > -    }
> > -    FreePool (FmpImageInfoBufOrg);
> > -    FmpImageInfoBufOrg = NULL;
> > +    FreePool (OrgFmpImageInfoBuf);
> > +    OrgFmpImageInfoBuf = NULL;
> >    }
> >
> > -  gBS->FreePool (Buffer);
> > +  FreePool (Buffer);
> > +  FreePool (HardwareInstances);
> >    return Table;
> >  }
> >
> > --
> > 2.20.1.windows.1

Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers

[Wu, Hao A]

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Tuesday, August 06, 2019 12:46 PM
> To: Wu, Hao A; Jin, Eric; devel@edk2.groups.io; Kinney, Michael D
> Cc: Sean Brogan; Bret Barkelew; Wang, Jian J; Gao, Liming; afish@apple.com;
> lersek@redhat.com; leif.lindholm@linaro.org
> Subject: RE: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to
> support multiple controllers
> 
> Hao Wu,
> 
> I agree that patches should be cleaned up for
> submission to edk2 repo.  The work in edk2-staging
> may contain bug fixes and design changes in the
> patch history that can be consolidated to a smaller
> patch set.  Also, all feedback to a patch set from
> edk2-staging must be addressed just like any other
> submission which may include splitting or merging
> patches.


Hello Mike,

Thanks for the confirmation.

Since there is no other feedback received, the patch has been pushed via
commit e314132fea.

Best Regards,
Hao Wu


> 
> Mike
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Monday, August 5, 2019 7:49 PM
> > To: Jin, Eric <eric.jin@intel.com>;
> > devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret
> > Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; afish@apple.com;
> > lersek@redhat.com; leif.lindholm@linaro.org
> > Subject: RE: [PATCH v2] MdeModulePkg/EsrtFmpDxe:
> > Enhance ESRT to support multiple controllers
> >
> > > -----Original Message-----
> > > From: Jin, Eric
> > > Sent: Monday, August 05, 2019 4:03 PM
> > > To: devel@edk2.groups.io
> > > Cc: Sean Brogan; Bret Barkelew; Wang, Jian J; Wu, Hao
> > A; Kinney,
> > > Michael D
> > > Subject: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance
> > ESRT to support
> > > multiple controllers
> > >
> > > REF:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1525
> > >
> > > The patch is to merge multiple FMP instances into
> > single ESRT entry
> > > when they have the same GUID.
> > >
> > > The policy to LastAttemptStatus/LastAttemptVersion of
> > ESRT entry is:
> > > If all the LastAttemptStatus are
> > LAST_ATTEMPT_STATUS_SUCCESS, then
> > > LastAttemptVersion should be the smallest of
> > LastAttemptVersion. If
> > > any of the LastAttemptStatus is not
> > LAST_ATTEMPT_STATUS_SUCCESS, then
> > > the LastAttemptVersion/LastAttemptStatus should be
> > the values of the
> > > first FMP instance whose LastAttemptStatus is not
> > > LAST_ATTEMPT_STATUS_SUCCESS.
> > >
> > > To detect possible duplicated GUID/HardwareInstance,
> > a table of
> > > GUID/HardwareInstance pairs from all the
> > > EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP instances
> > is built. If a
> > > duplicate is found, then generate a DEBUG_ERROR
> > message, generate an
> > > ASSERT(), and ignore the duplicate
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > >
> > > Add an internal worker function called
> > FmpGetFirmwareImageDescriptor()
> > > that retrieves the list of
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs from a
> > > single FMP instance and returns the descriptors in an
> > allocated
> > > buffer. This function is used to get the descriptors
> > used to build the
> > > table of unique GUID/HardwareInstance pairs.  It is
> > then used again to
> > > generate the ESRT Table from all the
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs
> > > from all the FMP instances. 2 passes are performed so
> > the total number
> > > of descriptors is known. This allows the correct
> > sized buffers to
> > > always be allocated.
> >
> >
> > The patch looks good to me.
> > Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> >
> > I will wait a little longer to push the patch to see if
> > there is additional feedbacks from stewards with regard
> > to the approach when merging changes from the edk2-
> > staging repo.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Signed-off-by: Michael D Kinney
> > <michael.d.kinney@intel.com>
> > > Signed-off-by: Eric Jin <eric.jin@intel.com>
> > > ---
> > >  MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 394
> > +++++++++++++---
> > > ----
> > >  1 file changed, 257 insertions(+), 137 deletions(-)
> > >
> > > diff --git
> > a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > index 2356da662e..4670349f89 100644
> > > --- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > +++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> > > @@ -2,7 +2,7 @@
> > >    Publishes ESRT table from Firmware Management
> > Protocol instances
> > >
> > >    Copyright (c) 2016, Microsoft Corporation
> > > -  Copyright (c) 2018, Intel Corporation. All rights
> > reserved.<BR>
> > > +  Copyright (c) 2018 - 2019, Intel Corporation. All
> > rights
> > > + reserved.<BR>
> > >
> > >    All rights reserved.
> > >    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -
> > 21,6 +21,22 @@
> > > #include <Guid/EventGroup.h>  #include
> > <Guid/SystemResourceTable.h>
> > >
> > > +///
> > > +/// Structure for array of unique
> > GUID/HardwareInstance pairs from
> > > +the /// current set of
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP
> > > Protocols.
> > > +///
> > > +typedef struct {
> > > +  ///
> > > +  /// A unique GUID identifying the firmware image
> > type.
> > > +  ///
> > > +  EFI_GUID                      ImageTypeGuid;
> > > +  ///
> > > +  /// An optional number to identify the unique
> > hardware instance
> > > +within
> > > the
> > > +  /// system for devices that may have multiple
> > instances whenever
> > > possible.
> > > +  ///
> > > +  UINT64                        HardwareInstance;
> > > +} GUID_HARDWAREINSTANCE_PAIR;
> > > +
> > >  /**
> > >   Print ESRT to debug console.
> > >
> > > @@ -33,11 +49,6 @@ PrintTable (
> > >    IN EFI_SYSTEM_RESOURCE_TABLE  *Table
> > >    );
> > >
> > > -//
> > > -// Number of ESRT entries to grow by each time we
> > run out of room -//
> > > -#define GROWTH_STEP  10
> > > -
> > >  /**
> > >    Install EFI System Resource Table into the UEFI
> > Configuration Table
> > >
> > > @@ -101,90 +112,129 @@ IsSystemFmp (
> > >  }
> > >
> > >  /**
> > > -  Function to create a single ESRT Entry and add it
> > to the ESRT
> > > -  given a FMP descriptor.  If the guid is already in
> > the ESRT it
> > > -  will be ignored.  The ESRT will grow if it does
> > not have enough room.
> > > -
> > > -  @param[in, out] Table             On input,
> > pointer to the pointer to the ESRT.
> > > -                                    On output, same
> > as input or pointer to the pointer
> > > -                                    to new enlarged
> > ESRT.
> > > -  @param[in]      FmpImageInfoBuf   Pointer to the
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > > -  @param[in]      FmpVersion        FMP Version.
> > > -
> > > -  @return  Status code.
> > > +  Function to create a single ESRT Entry and add it
> > to the ESRT with
> > > + a given FMP descriptor.  If the GUID is already in
> > the ESRT, then
> > > + the ESRT  entry is updated.
> > > +
> > > +  @param[in,out] Table                Pointer to the
> > ESRT Table.
> > > +  @param[in,out] HardwareInstances    Pointer to the
> > > GUID_HARDWAREINSTANCE_PAIR.
> > > +  @param[in,out] NumberOfDescriptors  The number of
> > > EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > > +  @param[in]     FmpImageInfoBuf      Pointer to the
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > > +  @param[in]     FmpVersion           FMP Version.
> > > +
> > > +  @retval  EFI_SUCCESS     FmpImageInfoBuf was use
> > to fill in a new ESRT
> > > entry
> > > +                           in Table.
> > > +  @retval  EFI_SUCCESS     The ImageTypeId GUID in
> > FmpImageInfoBuf
> > > matches an
> > > +                           existing ESRT entry in
> > Table, and the information
> > > +                           from FmpImageInfoBuf was
> > merged into the the existing
> > > +                           ESRT entry.
> > > +  @retval  EFI_UNSPOORTED  The GUID/HardareInstance
> > in
> > > FmpImageInfoBuf has is a
> > > +                           duplicate.
> > >
> > >  **/
> > >  EFI_STATUS
> > >  CreateEsrtEntry (
> > > -  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
> > > -  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR
> > *FmpImageInfoBuf,
> > > -  IN UINT32                         FmpVersion
> > > +  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
> > > +  IN OUT GUID_HARDWAREINSTANCE_PAIR
> > *HardwareInstances,
> > > +  IN OUT UINT32
> > *NumberOfDescriptors,
> > > +  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR
> > *FmpImageInfoBuf,
> > > +  IN     UINT32                         FmpVersion
> > >    )
> > >  {
> > >    UINTN                      Index;
> > >    EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
> > > -  UINTN                      NewSize;
> > > -  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
> > > +  UINT64                     FmpHardwareInstance;
> > >
> > > -  Index = 0;
> > > -  Entry = NULL;
> > > +  FmpHardwareInstance = 0;
> > > +  if (FmpVersion >= 3) {
> > > +    FmpHardwareInstance = FmpImageInfoBuf-
> > >HardwareInstance;
> > > +  }
> > >
> > > -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) +
> > 1);
> > >    //
> > > -  // Make sure Guid isn't already in the list
> > > +  // Check to see of FmpImageInfoBuf
> > GUID/HardwareInstance is unique
> > >    //
> > > -  for (Index = 0; Index < (*Table)->FwResourceCount;
> > Index++) {
> > > -    if (CompareGuid (&Entry->FwClass,
> > &FmpImageInfoBuf->ImageTypeId)) {
> > > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry
> > already exists for FMP
> > > Instance with GUID %g\n", &Entry->FwClass));
> > > -      return EFI_INVALID_PARAMETER;
> > > +  for (Index = 0; Index < *NumberOfDescriptors;
> > Index++) {
> > > +    if (CompareGuid
> > (&HardwareInstances[Index].ImageTypeGuid,
> > > &FmpImageInfoBuf->ImageTypeId)) {
> > > +      if (HardwareInstances[Index].HardwareInstance
> > ==
> > > FmpHardwareInstance) {
> > > +        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate
> > firmware image
> > > descriptor with GUID %g HardwareInstance:0x%x\n",
> > &FmpImageInfoBuf-
> > > >ImageTypeId, FmpHardwareInstance));
> > > +        ASSERT (
> > > +          !CompareGuid
> > (&HardwareInstances[Index].ImageTypeGuid,
> > > &FmpImageInfoBuf->ImageTypeId) ||
> > > +          HardwareInstances[Index].HardwareInstance
> > !=
> > > FmpHardwareInstance
> > > +          );
> > > +        return EFI_UNSUPPORTED;
> > > +      }
> > >      }
> > > -    Entry++;
> > >    }
> > >
> > >    //
> > > -  // Grow table if needed
> > > +  // Record new GUID/HardwareInstance pair
> > >    //
> > > -  if ((*Table)->FwResourceCount >= (*Table)-
> > >FwResourceCountMax) {
> > > -    NewSize  = (((*Table)->FwResourceCountMax +
> > GROWTH_STEP) * sizeof
> > > (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> > (EFI_SYSTEM_RESOURCE_TABLE);
> > > -    NewTable = AllocateZeroPool (NewSize);
> > > -    if (NewTable == NULL) {
> > > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> > allocate memory larger
> > > table for ESRT. \n"));
> > > -      return EFI_OUT_OF_RESOURCES;
> > > +  CopyGuid
> > (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid
> > ,
> > > &FmpImageInfoBuf->ImageTypeId);
> > > +
> > HardwareInstances[*NumberOfDescriptors].HardwareInstanc
> > e =
> > > FmpHardwareInstance;
> > > +  *NumberOfDescriptors = *NumberOfDescriptors + 1;
> > > +
> > > +  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image
> > descriptor with
> > > GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf-
> > >ImageTypeId,
> > > FmpHardwareInstance));
> > > +
> > > +  //
> > > +  // Check to see if GUID is already in the ESRT
> > table  //  Entry =
> > > + (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);  for
> > (Index = 0; Index <
> > > + Table->FwResourceCount; Index++, Entry++) {
> > > +    if (!CompareGuid (&Entry->FwClass,
> > > + &FmpImageInfoBuf->ImageTypeId))
> > > {
> > > +      continue;
> > >      }
> > > +    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry
> > already exists for
> > > + FMP
> > > Instance with GUID %g\n", &Entry->FwClass));
> > > +
> > >      //
> > > -    // Copy the whole old table into new table
> > buffer
> > > -    //
> > > -    CopyMem (
> > > -      NewTable,
> > > -      (*Table),
> > > -      (((*Table)->FwResourceCountMax) * sizeof
> > > (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> > (EFI_SYSTEM_RESOURCE_TABLE)
> > > -      );
> > > -    //
> > > -    // Update max
> > > +    // Set ESRT FwVersion to the smaller of the two
> > values
> > >      //
> > > -    NewTable->FwResourceCountMax = NewTable-
> > >FwResourceCountMax +
> > > GROWTH_STEP;
> > > +    Entry->FwVersion = MIN (FmpImageInfoBuf-
> > >Version, Entry-
> > > >FwVersion);
> > > +
> > >      //
> > > -    // Free old table
> > > +    // VERSION 2 has Lowest Supported
> > >      //
> > > -    FreePool ((*Table));
> > > +    if (FmpVersion >= 2) {
> > > +      //
> > > +      // Set ESRT LowestSupportedFwVersion to the
> > smaller of the two values
> > > +      //
> > > +      Entry->LowestSupportedFwVersion =
> > > +        MIN (
> > > +          FmpImageInfoBuf-
> > >LowestSupportedImageVersion,
> > > +          Entry->LowestSupportedFwVersion
> > > +          );
> > > +    }
> > > +
> > >      //
> > > -    // Reassign pointer to new table.
> > > +    // VERSION 3 supports last attempt values
> > >      //
> > > -    (*Table) = NewTable;
> > > +    if (FmpVersion >= 3) {
> > > +      //
> > > +      // Update the ESRT entry with the last attempt
> > status and last attempt
> > > +      // version from the first FMP instance whose
> > last attempt status is not
> > > +      // SUCCESS.  If all FMP instances are SUCCESS,
> > then set version to the
> > > +      // smallest value from all FMP instances.
> > > +      //
> > > +      if (Entry->LastAttemptStatus ==
> > LAST_ATTEMPT_STATUS_SUCCESS) {
> > > +        if (FmpImageInfoBuf->LastAttemptStatus !=
> > > LAST_ATTEMPT_STATUS_SUCCESS) {
> > > +          Entry->LastAttemptStatus =
> > FmpImageInfoBuf->LastAttemptStatus;
> > > +          Entry->LastAttemptVersion =
> > FmpImageInfoBuf->LastAttemptVersion;
> > > +        } else {
> > > +          Entry->LastAttemptVersion =
> > > +            MIN (
> > > +              FmpImageInfoBuf->LastAttemptVersion,
> > > +              Entry->LastAttemptVersion
> > > +              );
> > > +        }
> > > +      }
> > > +    }
> > > +
> > > +    return EFI_SUCCESS;
> > >    }
> > >
> > >    //
> > > -  // ESRT table has enough room for the new entry so
> > add new entry
> > > -  //
> > > -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8
> > *)(*Table)) + sizeof
> > > (EFI_SYSTEM_RESOURCE_TABLE));
> > > -  //
> > > -  // Move to the location of new entry
> > > -  //
> > > -  Entry = Entry + (*Table)->FwResourceCount;
> > > +  // Add a new ESRT Table Entry
> > >    //
> > > -  // Increment resource count
> > > -  //
> > > -  (*Table)->FwResourceCount++;
> > > +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) +
> > Table-
> > > >FwResourceCount;
> > >
> > >    CopyGuid (&Entry->FwClass, &FmpImageInfoBuf-
> > >ImageTypeId);
> > >
> > > @@ -195,11 +245,11 @@ CreateEsrtEntry (
> > >      Entry->FwType =
> > (UINT32)(ESRT_FW_TYPE_DEVICEFIRMWARE);
> > >    }
> > >
> > > -  Entry->FwVersion = FmpImageInfoBuf->Version;
> > > +  Entry->FwVersion                = FmpImageInfoBuf-
> > >Version;
> > >    Entry->LowestSupportedFwVersion = 0;
> > > -  Entry->CapsuleFlags = 0;
> > > -  Entry->LastAttemptVersion = 0;
> > > -  Entry->LastAttemptStatus = 0;
> > > +  Entry->CapsuleFlags             = 0;
> > > +  Entry->LastAttemptVersion       = 0;
> > > +  Entry->LastAttemptStatus        = 0;
> > >
> > >    //
> > >    // VERSION 2 has Lowest Supported
> > > @@ -213,12 +263,93 @@ CreateEsrtEntry (
> > >    //
> > >    if (FmpVersion >= 3) {
> > >      Entry->LastAttemptVersion = FmpImageInfoBuf-
> > >LastAttemptVersion;
> > > -    Entry->LastAttemptStatus = FmpImageInfoBuf-
> > >LastAttemptStatus;
> > > +    Entry->LastAttemptStatus  = FmpImageInfoBuf-
> > >LastAttemptStatus;
> > >    }
> > >
> > > +  //
> > > +  // Increment the number of active ESRT Table
> > Entries  //
> > > + Table->FwResourceCount++;
> > > +
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > +/**
> > > +  Function to retrieve the
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR from an
> > > FMP Instance.
> > > +  The returned buffer is allocated using
> > AllocatePool() and must be
> > > + freed by
> > > the
> > > +  caller using FreePool().
> > > +
> > > +  @param[in]  Fmp                        Pointer to
> > an
> > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> > > +  @param[out] FmpImageInfoDescriptorVer  Pointer to
> > the version
> > > + number
> > > associated
> > > +                                         with the
> > returned
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > > +  @param[out] FmpImageInfoCount          Pointer to
> > the number of the
> > > returned
> > > +
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > > +  @param[out] DescriptorSize             Pointer to
> > the size, in bytes, of each
> > > +                                         returned
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > > +
> > > +  @return  Pointer to the retrieved
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > > + If
> > > the
> > > +           descriptor can not be retrieved, then
> > NULL is returned.
> > > +
> > > +**/
> > > +EFI_FIRMWARE_IMAGE_DESCRIPTOR *
> > > +FmpGetFirmwareImageDescriptor (
> > > +  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
> > > +  OUT UINT32
> > *FmpImageInfoDescriptorVer,
> > > +  OUT UINT8
> > *FmpImageInfoCount,
> > > +  OUT UINTN
> > *DescriptorSize
> > > +  )
> > > +{
> > > +  EFI_STATUS                     Status;
> > > +  UINTN                          ImageInfoSize;
> > > +  UINT32                         PackageVersion;
> > > +  CHAR16
> > *PackageVersionName;
> > > +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> > > +
> > > +  ImageInfoSize = 0;
> > > +  Status = Fmp->GetImageInfo (
> > > +                  Fmp,                        // FMP
> > Pointer
> > > +                  &ImageInfoSize,             //
> > Buffer Size (in this case 0)
> > > +                  NULL,                       //
> > NULL so we can get size
> > > +                  FmpImageInfoDescriptorVer,  //
> > DescriptorVersion
> > > +                  FmpImageInfoCount,          //
> > DescriptorCount
> > > +                  DescriptorSize,             //
> > DescriptorSize
> > > +                  &PackageVersion,            //
> > PackageVersion
> > > +                  &PackageVersionName         //
> > PackageVersionName
> > > +                  );
> > > +  if (Status != EFI_BUFFER_TOO_SMALL) {
> > > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> > Failure in
> > > GetImageInfo.  Status = %r\n", Status));
> > > +    return NULL;
> > > +  }
> > > +
> > > +  FmpImageInfoBuf = AllocateZeroPool
> > (ImageInfoSize);  if
> > > + (FmpImageInfoBuf == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get
> > memory for FMP
> > > descriptor.\n"));
> > > +    return NULL;
> > > +  }
> > > +
> > > +  PackageVersionName = NULL;
> > > +  Status = Fmp->GetImageInfo (
> > > +                  Fmp,                        // FMP
> > Pointer
> > > +                  &ImageInfoSize,             //
> > ImageInfoSize
> > > +                  FmpImageInfoBuf,            //
> > ImageInfo
> > > +                  FmpImageInfoDescriptorVer,  //
> > DescriptorVersion
> > > +                  FmpImageInfoCount,          //
> > DescriptorCount
> > > +                  DescriptorSize,             //
> > DescriptorSize
> > > +                  &PackageVersion,            //
> > PackageVersion
> > > +                  &PackageVersionName         //
> > PackageVersionName
> > > +                  );
> > > +  if (PackageVersionName != NULL) {
> > > +    FreePool (PackageVersionName);
> > > +  }
> > > +  if (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> > GetImageInfo.
> > > + Status
> > > = %r\n", Status));
> > > +    FreePool (FmpImageInfoBuf);
> > > +    return NULL;
> > > +  }
> > > +
> > > +  return FmpImageInfoBuf;
> > > +}
> > > +
> > >  /**
> > >    Function to create ESRT based on FMP Instances.
> > >    Create ESRT table, get the descriptors from FMP
> > Instance and @@
> > > -232,29 +363,26 @@ CreateFmpBasedEsrt (
> > >    VOID
> > >    )
> > >  {
> > > -  EFI_STATUS                        Status;
> > > -  EFI_SYSTEM_RESOURCE_TABLE         *Table;
> > > -  UINTN                             NoProtocols;
> > > -  VOID                              **Buffer;
> > > -  UINTN                             Index;
> > > -  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
> > > -  UINTN                             DescriptorSize;
> > > -  EFI_FIRMWARE_IMAGE_DESCRIPTOR
> > *FmpImageInfoBuf;
> > > -  EFI_FIRMWARE_IMAGE_DESCRIPTOR
> > *FmpImageInfoBufOrg;
> > > -  UINT8
> > FmpImageInfoCount;
> > > -  UINT32
> > FmpImageInfoDescriptorVer;
> > > -  UINTN                             ImageInfoSize;
> > > -  UINT32                            PackageVersion;
> > > -  CHAR16
> > *PackageVersionName;
> > > +  EFI_STATUS                     Status;
> > > +  UINTN                          NoProtocols;
> > > +  VOID                           **Buffer;
> > > +  UINTN                          Index;
> > > +  UINT32
> > FmpImageInfoDescriptorVer;
> > > +  UINT8                          FmpImageInfoCount;
> > > +  UINTN                          DescriptorSize;
> > > +  UINT32
> > NumberOfDescriptors;
> > > +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> > > + EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
> > > +  EFI_SYSTEM_RESOURCE_TABLE      *Table;
> > > +  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
> > >
> > >    Status             = EFI_SUCCESS;
> > > -  Table              = NULL;
> > >    NoProtocols        = 0;
> > >    Buffer             = NULL;
> > > -  PackageVersionName = NULL;
> > >    FmpImageInfoBuf    = NULL;
> > > -  FmpImageInfoBufOrg = NULL;
> > > -  Fmp                = NULL;
> > > +  OrgFmpImageInfoBuf = NULL;
> > > +  Table              = NULL;
> > > +  HardwareInstances  = NULL;
> > >
> > >    Status = EfiLocateProtocolBuffer (
> > >               &gEfiFirmwareManagementProtocolGuid,
> > > @@ -266,69 +394,64 @@ CreateFmpBasedEsrt (
> > >    }
> > >
> > >    //
> > > -  // Allocate Memory for table
> > > +  // Count the total number of
> > EFI_FIRMWARE_IMAGE_DESCRIPTORs  //
> > > + for (Index = 0, NumberOfDescriptors = 0; Index <
> > NoProtocols; Index++) {
> > > +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor
> > (
> > > +
> > (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> > > +                        &FmpImageInfoDescriptorVer,
> > > +                        &FmpImageInfoCount,
> > > +                        &DescriptorSize
> > > +                        );
> > > +    if (FmpImageInfoBuf != NULL) {
> > > +      NumberOfDescriptors += FmpImageInfoCount;
> > > +      FreePool (FmpImageInfoBuf);
> > > +    }
> > > +  }
> > > +
> > > +  //
> > > +  // Allocate ESRT Table and GUID/HardwareInstance
> > table
> > >    //
> > >    Table = AllocateZeroPool (
> > > -             (GROWTH_STEP * sizeof
> > (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> > > (EFI_SYSTEM_RESOURCE_TABLE)
> > > +             (NumberOfDescriptors * sizeof
> > > + (EFI_SYSTEM_RESOURCE_ENTRY)) +
> > > sizeof (EFI_SYSTEM_RESOURCE_TABLE)
> > >               );
> > >    if (Table == NULL) {
> > >      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> > allocate memory for
> > > ESRT.\n"));
> > > -    gBS->FreePool (Buffer);
> > > +    FreePool (Buffer);
> > >      return NULL;
> > >    }
> > >
> > > +  HardwareInstances = AllocateZeroPool
> > (NumberOfDescriptors * sizeof
> > > (GUID_HARDWAREINSTANCE_PAIR));
> > > +  if (HardwareInstances == NULL) {
> > > +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> > allocate memory for
> > > + HW
> > > Instance Table.\n"));
> > > +    FreePool (Table);
> > > +    FreePool (Buffer);
> > > +    return NULL;
> > > +  }
> > > +
> > > +  //
> > > +  // Initialize ESRT Table
> > > +  //
> > >    Table->FwResourceCount    = 0;
> > > -  Table->FwResourceCountMax = GROWTH_STEP;
> > > +  Table->FwResourceCountMax = NumberOfDescriptors;
> > >    Table->FwResourceVersion  =
> > > EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
> > >
> > > +  NumberOfDescriptors = 0;
> > >    for (Index = 0; Index < NoProtocols; Index++) {
> > > -    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *)
> > Buffer[Index];
> > > -
> > > -    ImageInfoSize = 0;
> > > -    Status = Fmp->GetImageInfo (
> > > -                    Fmp,                         //
> > FMP Pointer
> > > -                    &ImageInfoSize,              //
> > Buffer Size (in this case 0)
> > > -                    NULL,                        //
> > NULL so we can get size
> > > -                    &FmpImageInfoDescriptorVer,  //
> > DescriptorVersion
> > > -                    &FmpImageInfoCount,          //
> > DescriptorCount
> > > -                    &DescriptorSize,             //
> > DescriptorSize
> > > -                    &PackageVersion,             //
> > PackageVersion
> > > -                    &PackageVersionName          //
> > PackageVersionName
> > > -                    );
> > > -
> > > -    if (Status != EFI_BUFFER_TOO_SMALL) {
> > > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> > Failure in
> > > GetImageInfo.  Status = %r\n", Status));
> > > -      continue;
> > > -    }
> > > -
> > > -    FmpImageInfoBuf = AllocateZeroPool
> > (ImageInfoSize);
> > > +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor
> > (
> > > +
> > (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> > > +                        &FmpImageInfoDescriptorVer,
> > > +                        &FmpImageInfoCount,
> > > +                        &DescriptorSize
> > > +                        );
> > >      if (FmpImageInfoBuf == NULL) {
> > > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> > get memory for
> > > descriptors.\n"));
> > > -      continue;
> > > -    }
> > > -
> > > -    FmpImageInfoBufOrg = FmpImageInfoBuf;
> > > -    PackageVersionName = NULL;
> > > -    Status = Fmp->GetImageInfo (
> > > -                    Fmp,
> > > -                    &ImageInfoSize,              //
> > ImageInfoSize
> > > -                    FmpImageInfoBuf,             //
> > ImageInfo
> > > -                    &FmpImageInfoDescriptorVer,  //
> > DescriptorVersion
> > > -                    &FmpImageInfoCount,          //
> > DescriptorCount
> > > -                    &DescriptorSize,             //
> > DescriptorSize
> > > -                    &PackageVersion,             //
> > PackageVersion
> > > -                    &PackageVersionName          //
> > PackageVersionName
> > > -                    );
> > > -    if (EFI_ERROR (Status)) {
> > > -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> > GetImageInfo.  Status
> > > = %r\n", Status));
> > > -      FreePool (FmpImageInfoBufOrg);
> > > -      FmpImageInfoBufOrg = NULL;
> > >        continue;
> > >      }
> > >
> > >      //
> > >      // Check each descriptor and read from the one
> > specified
> > >      //
> > > +    OrgFmpImageInfoBuf = FmpImageInfoBuf;
> > >      while (FmpImageInfoCount > 0) {
> > >        //
> > >        // If the descriptor has the IN USE bit set,
> > create ESRT entry
> > > otherwise ignore.
> > > @@ -337,7 +460,7 @@ CreateFmpBasedEsrt (
> > >          //
> > >          // Create ESRT entry
> > >          //
> > > -        CreateEsrtEntry (&Table, FmpImageInfoBuf,
> > > FmpImageInfoDescriptorVer);
> > > +        CreateEsrtEntry (Table, HardwareInstances,
> > > + &NumberOfDescriptors,
> > > FmpImageInfoBuf, FmpImageInfoDescriptorVer);
> > >        }
> > >        FmpImageInfoCount--;
> > >        //
> > > @@ -346,15 +469,12 @@ CreateFmpBasedEsrt (
> > >        FmpImageInfoBuf =
> > (EFI_FIRMWARE_IMAGE_DESCRIPTOR *)(((UINT8
> > > *)FmpImageInfoBuf) + DescriptorSize);
> > >      }
> > >
> > > -    if (PackageVersionName != NULL) {
> > > -      FreePool (PackageVersionName);
> > > -      PackageVersionName = NULL;
> > > -    }
> > > -    FreePool (FmpImageInfoBufOrg);
> > > -    FmpImageInfoBufOrg = NULL;
> > > +    FreePool (OrgFmpImageInfoBuf);
> > > +    OrgFmpImageInfoBuf = NULL;
> > >    }
> > >
> > > -  gBS->FreePool (Buffer);
> > > +  FreePool (Buffer);
> > > +  FreePool (HardwareInstances);
> > >    return Table;
> > >  }
> > >
> > > --
> > > 2.20.1.windows.1