* [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers
@ 2019-08-05 8:02 Eric Jin
2019-08-06 2:49 ` Wu, Hao A
0 siblings, 1 reply; 4+ messages in thread
From: Eric Jin @ 2019-08-05 8:02 UTC (permalink / raw)
To: devel; +Cc: Sean Brogan, Bret Barkelew, Jian J Wang, Hao A Wu,
Michael D Kinney
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers
2019-08-05 8:02 [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers Eric Jin
@ 2019-08-06 2:49 ` Wu, Hao A
2019-08-06 4:45 ` Michael D Kinney
0 siblings, 1 reply; 4+ messages in thread
From: Wu, Hao A @ 2019-08-06 2:49 UTC (permalink / raw)
To: Jin, Eric, devel@edk2.groups.io
Cc: Sean Brogan, Bret Barkelew, Wang, Jian J, Kinney, Michael D,
Gao, Liming, afish@apple.com, lersek@redhat.com,
leif.lindholm@linaro.org
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers
2019-08-06 2:49 ` Wu, Hao A
@ 2019-08-06 4:45 ` Michael D Kinney
2019-08-07 5:20 ` Wu, Hao A
0 siblings, 1 reply; 4+ messages in thread
From: Michael D Kinney @ 2019-08-06 4:45 UTC (permalink / raw)
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
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers
2019-08-06 4:45 ` Michael D Kinney
@ 2019-08-07 5:20 ` Wu, Hao A
0 siblings, 0 replies; 4+ messages in thread
From: Wu, Hao A @ 2019-08-07 5:20 UTC (permalink / raw)
To: Kinney, Michael D, Jin, Eric, devel@edk2.groups.io
Cc: Sean Brogan, Bret Barkelew, Wang, Jian J, Gao, Liming,
afish@apple.com, lersek@redhat.com, leif.lindholm@linaro.org
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-07 5:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-05 8:02 [PATCH v2] MdeModulePkg/EsrtFmpDxe: Enhance ESRT to support multiple controllers Eric Jin
2019-08-06 2:49 ` Wu, Hao A
2019-08-06 4:45 ` Michael D Kinney
2019-08-07 5:20 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox