> -----Original Message----- > From: Gao, Zhichao > Sent: Wednesday, May 29, 2019 8:46 AM > To: devel@edk2.groups.io > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming; > Sean Brogan; Michael Turner; Gao, Zhichao > Subject: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei > > From: Bret Barkelew <bret.barke...@microsoft.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > Sperate the capsule check function from GetCapsuleDescriptors
Sperate -> Separate > and name it to AreCapsulesStaged. > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > And optimize its to remove the duplicated code. > > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Sean Brogan <sean.bro...@microsoft.com> > Cc: Michael Turner <michael.tur...@microsoft.com> > Cc: Bret Barkelew <bret.barke...@microsoft.com> > Signed-off-by: Zhichao gao <zhichao....@intel.com> > --- > MdeModulePkg/Universal/CapsulePei/Capsule.h | 3 +- > .../Universal/CapsulePei/CapsulePei.inf | 3 +- > .../Universal/CapsulePei/UefiCapsule.c | 357 ++++++++++-------- > 3 files changed, 194 insertions(+), 169 deletions(-) I am a bit confused for the purpose of this patch. My understanding is that this patch will refine this driver to remove duplicated code by abstract common codes into a new function. And there will be no functional impact. However, after the change, the line of codes of this driver increased by 20+ lines. Did I miss something for the purpose of this patch? Some additional comments below. > > diff --git a/MdeModulePkg/Universal/CapsulePei/Capsule.h > b/MdeModulePkg/Universal/CapsulePei/Capsule.h > index baf40423af..fc20dd8b92 100644 > --- a/MdeModulePkg/Universal/CapsulePei/Capsule.h > +++ b/MdeModulePkg/Universal/CapsulePei/Capsule.h > @@ -1,6 +1,6 @@ > /** @file > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -30,6 +30,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <Library/PcdLib.h> > #include <Library/ReportStatusCodeLib.h> > #include <Library/DebugAgentLib.h> > +#include <Library/MemoryAllocationLib.h> > #include <IndustryStandard/PeImage.h> > #include "Common/CommonHeader.h" > > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > index 5d43df3075..9c88b3986f 100644 > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > @@ -6,7 +6,7 @@ > # This external input must be validated carefully to avoid security issue > like > # buffer overflow, integer overflow. > # > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -43,6 +43,7 @@ > BaseLib > HobLib > BaseMemoryLib > + MemoryAllocationLib > PeiServicesLib > PeimEntryPoint > DebugLib > diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > index e967599e96..2d003369ca 100644 > --- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > +++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > @@ -1,7 +1,7 @@ > /** @file > Capsule update PEIM for UEFI2.0 > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "Capsule.h" > > +#define DEFAULT_SG_LIST_HEADS (20) > + > #ifdef MDE_CPU_IA32 > // > // Global Descriptor Table (GDT) > @@ -791,30 +793,89 @@ BuildMemoryResourceDescriptor ( > } > > /** > - Checks for the presence of capsule descriptors. > - Get capsule descriptors from variable CapsuleUpdateData, > CapsuleUpdateData1, CapsuleUpdateData2... > - and save to DescriptorBuffer. > + Check if the capsules are staged. > > - @param DescriptorBuffer Pointer to the capsule descriptors > + @retval TRUE The capsules are staged. > + @retval FALSE The capsules are not staged. > + > +**/ > +BOOLEAN > +EFIAPI > +AreCapsulesStaged ( Keyword 'EFIAPI' seems not needed here. AreCapsulesStaged() is an internal function here. > + VOID > + ) > +{ > + EFI_STATUS Status; > + UINTN Size; > + EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > + > + CapsuleDataPtr64 = 0; > + > + Status = PeiServicesLocatePpi ( > + &gEfiPeiReadOnlyVariable2PpiGuid, > + 0, > + NULL, > + (VOID **)&PPIVariableServices > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n")); > + return FALSE; > + } > + > + // > + // Check for Update capsule > + // > + Size = sizeof (CapsuleDataPtr64); > + Status = PPIVariableServices->GetVariable( > + PPIVariableServices, > + EFI_CAPSULE_VARIABLE_NAME, > + &gEfiCapsuleVendorGuid, > + NULL, > + &Size, > + (VOID *)&CapsuleDataPtr64 > + ); > + > + if (!EFI_ERROR (Status)) { > + return TRUE; > + } > + > + return FALSE; > +} > + > +/** > + Check all the variables for SG list heads and get the count and addresses. > + > + @param ListLength A pointer would return the SG list length. > + @param HeadList A ponter to the capsule SG list. > + > + @retval EFI_SUCCESS a valid capsule is present > + @retval EFI_NOT_FOUND if a valid capsule is not present > + @retval EFI_INVALID_PARAMETER the input parameter is invalid > + @retval EFI_OUT_OF_RESOURCE fail to allocate memory > > - @retval EFI_SUCCESS a valid capsule is present > - @retval EFI_NOT_FOUND if a valid capsule is not present > **/ > EFI_STATUS > -GetCapsuleDescriptors ( > - IN EFI_PHYSICAL_ADDRESS *DescriptorBuffer > +EFIAPI > +GetScatterGatherHeadEntries ( Keyword 'EFIAPI' seems not needed here. GetScatterGatherHeadEntries() is an internal function here. > + OUT UINTN *ListLength, > + OUT EFI_PHYSICAL_ADDRESS **HeadList > ) > { > - EFI_STATUS Status; > - UINTN Size; > - UINTN Index; > - UINTN TempIndex; > - UINTN ValidIndex; > - BOOLEAN Flag; > - CHAR16 CapsuleVarName[30]; > - CHAR16 *TempVarName; > - EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > - EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_STATUS Status; > + UINTN Size; > + UINTN Index; > + UINTN TempIndex; > + UINTN ValidIndex; > + BOOLEAN Flag; > + CHAR16 CapsuleVarName[30]; > + CHAR16 *TempVarName; > + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > + EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_PHYSICAL_ADDRESS *TempList; > + EFI_PHYSICAL_ADDRESS *EnlargedTempList; > + UINTN TempListLength; > > Index = 0; > TempVarName = NULL; > @@ -822,87 +883,118 @@ GetCapsuleDescriptors ( > ValidIndex = 0; > CapsuleDataPtr64 = 0; > > + if ((ListLength == NULL) || (HeadList == NULL)) { > + DEBUG ((DEBUG_ERROR, "%a Invalid parameters. Inputs can't be > NULL\n", __FUNCTION__)); > + ASSERT (ListLength != NULL); > + ASSERT (HeadList != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + *ListLength = 0; > + *HeadList = NULL; > + > Status = PeiServicesLocatePpi ( > &gEfiPeiReadOnlyVariable2PpiGuid, > 0, > NULL, > (VOID **) &PPIVariableServices > ); > - if (Status == EFI_SUCCESS) { > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > - TempVarName = CapsuleVarName + StrLen (CapsuleVarName); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI: %r\n", > Status)); > + return Status; > + } > + > + // > + // Allocate memory for sg list head > + // > + TempListLength = DEFAULT_SG_LIST_HEADS * sizeof > (EFI_PHYSICAL_ADDRESS); > + TempList = AllocateZeroPool (TempListLength); > + if (TempList == NULL) { > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + // > + // Setup var name buffer for update capsules > + // > + StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > + TempVarName = CapsuleVarName + StrLen (CapsuleVarName); > + while (TRUE) { > + if (Index != 0) { > + UnicodeValueToStringS ( > + TempVarName, > + (sizeof(CapsuleVarName) - ((StrLen(CapsuleVarName) + 1) * > sizeof(CHAR16))), Compared with the origin code: ''' sizeof (CapsuleVarName) - ((UINTN)TempVarName - (UINTN)CapsuleVarName), ''' The size of buffer passed into function UnicodeValueToStringS() is 2 bytes smaller for the modified code. Is there a reason for such change? Best Regards, Hao Wu > + 0, > + Index, > + 0 > + ); > + } > + > Size = sizeof (CapsuleDataPtr64); > - while (1) { > - if (Index == 0) { > - // > - // For the first Capsule Image > - // > - Status = PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_INFO, "Capsule -- capsule variable not set\n")); > - return EFI_NOT_FOUND; > - } > - // > - // We have a chicken/egg situation where the memory init code needs > to > - // know the boot mode prior to initializing memory. For this case, > our > - // validate function will fail. We can detect if this is the case if > blocklist > - // pointer is null. In that case, return success since we know that > the > - // variable is set. > - // > - if (DescriptorBuffer == NULL) { > - return EFI_SUCCESS; > - } > - } else { > - UnicodeValueToStringS ( > - TempVarName, > - sizeof (CapsuleVarName) - ((UINTN)TempVarName - > (UINTN)CapsuleVarName), > - 0, > - Index, > - 0 > - ); > - Status = PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - break; > - } > + Status = PPIVariableServices->GetVariable ( > + PPIVariableServices, > + CapsuleVarName, > + &gEfiCapsuleVendorGuid, > + NULL, > + &Size, > + (VOID *)&CapsuleDataPtr64 > + ); > > - // > - // If this BlockList has been linked before, skip this variable > - // > - Flag = FALSE; > - for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) { > - if (DescriptorBuffer[TempIndex] == CapsuleDataPtr64) { > - Flag = TRUE; > - break; > - } > - } > - if (Flag) { > - Index ++; > - continue; > - } > + if (EFI_ERROR (Status)) { > + if (Status != EFI_NOT_FOUND) { > + DEBUG ((DEBUG_ERROR, "Unexpected error getting Capsule Update > variable. Status = %r\n")); > } > + break; > + } > > - // > - // Cache BlockList which has been processed > - // > - DescriptorBuffer[ValidIndex++] = CapsuleDataPtr64; > - Index ++; > + // > + // If this BlockList has been linked before, skip this variable > + // > + Flag = FALSE; > + for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) { > + if (TempList[TempIndex] == CapsuleDataPtr64) { > + Flag = TRUE; > + break; > + } > } > + if (Flag) { > + Index++; > + continue; > + } > + > + // > + // The TempList is full, enlarge it > + // > + if ((ValidIndex + 1) >= TempListLength) { > + EnlargedTempList = AllocateZeroPool (TempListLength * 2); > + CopyMem (EnlargedTempList, TempList, TempListLength); > + FreePool (TempList); > + TempList = EnlargedTempList; > + TempListLength *= 2; > + } > + > + // > + // Cache BlockList which has been processed > + // > + TempList[ValidIndex++] = CapsuleDataPtr64; > + Index++; > + } > + > + if (ValidIndex == 0) { > + DEBUG((DEBUG_ERROR, "%a didn't find any SG lists in variables\n", > __FUNCTION__)); > + return EFI_NOT_FOUND; > } > > + *HeadList = AllocateZeroPool ((ValidIndex + 1) * sizeof > (EFI_PHYSICAL_ADDRESS)); > + if (*HeadList == NULL) { > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + CopyMem (*HeadList, TempList, (ValidIndex) * sizeof > (EFI_PHYSICAL_ADDRESS)); > + *ListLength = ValidIndex; > + > return EFI_SUCCESS; > } > > @@ -937,17 +1029,11 @@ CapsuleCoalesce ( > IN OUT UINTN *MemorySize > ) > { > - UINTN Index; > - UINTN Size; > - UINTN VariableCount; > - CHAR16 CapsuleVarName[30]; > - CHAR16 *TempVarName; > - EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > EFI_STATUS Status; > EFI_BOOT_MODE BootMode; > - EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > EFI_PHYSICAL_ADDRESS *VariableArrayAddress; > MEMORY_RESOURCE_DESCRIPTOR *MemoryResource; > + UINTN ListLength; > #ifdef MDE_CPU_IA32 > UINT16 CoalesceImageMachineType; > EFI_PHYSICAL_ADDRESS CoalesceImageEntryPoint; > @@ -955,10 +1041,8 @@ CapsuleCoalesce ( > EFI_CAPSULE_LONG_MODE_BUFFER LongModeBuffer; > #endif > > - Index = 0; > - VariableCount = 0; > - CapsuleVarName[0] = 0; > - CapsuleDataPtr64 = 0; > + ListLength = 0; > + VariableArrayAddress = NULL; > > // > // Someone should have already ascertained the boot mode. If it's not > @@ -972,74 +1056,11 @@ CapsuleCoalesce ( > } > > // > - // User may set the same ScatterGatherList with several different > variables, > - // so cache all ScatterGatherList for check later. > + // Get ScatterGatherList > // > - Status = PeiServicesLocatePpi ( > - &gEfiPeiReadOnlyVariable2PpiGuid, > - 0, > - NULL, > - (VOID **) &PPIVariableServices > - ); > + Status = GetScatterGatherHeadEntries (&ListLength, > &VariableArrayAddress); > if (EFI_ERROR (Status)) { > - goto Done; > - } > - Size = sizeof (CapsuleDataPtr64); > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > - TempVarName = CapsuleVarName + StrLen (CapsuleVarName); > - while (TRUE) { > - if (Index > 0) { > - UnicodeValueToStringS ( > - TempVarName, > - sizeof (CapsuleVarName) - ((UINTN)TempVarName - > (UINTN)CapsuleVarName), > - 0, > - Index, > - 0 > - ); > - } > - Status = PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - // > - // There is no capsule variables, quit > - // > - DEBUG ((DEBUG_INFO,"Capsule variable Index = %d\n", Index)); > - break; > - } > - VariableCount++; > - Index++; > - } > - > - DEBUG ((DEBUG_INFO,"Capsule variable count = %d\n", VariableCount)); > - > - // > - // The last entry is the end flag. > - // > - Status = PeiServicesAllocatePool ( > - (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS), > - (VOID **)&VariableArrayAddress > - ); > - > - if (Status != EFI_SUCCESS) { > - DEBUG ((DEBUG_ERROR, "AllocatePages Failed!, Status = %x\n", Status)); > - goto Done; > - } > - > - ZeroMem (VariableArrayAddress, (VariableCount + 1) * sizeof > (EFI_PHYSICAL_ADDRESS)); > - > - // > - // Find out if we actually have a capsule. > - // GetCapsuleDescriptors depends on variable PPI, so it should run in > 32-bit > environment. > - // > - Status = GetCapsuleDescriptors (VariableArrayAddress); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Fail to find capsule variables.\n")); > + DEBUG ((DEBUG_ERROR, "%a failed to get Scatter Gather List Head > Entries. Status = %r\n", __FUNCTION__, Status)); > goto Done; > } > > @@ -1117,9 +1138,11 @@ CheckCapsuleUpdate ( > IN EFI_PEI_SERVICES **PeiServices > ) > { > - EFI_STATUS Status; > - Status = GetCapsuleDescriptors (NULL); > - return Status; > + if (AreCapsulesStaged ()) { > + return EFI_SUCCESS; > + } else { > + return EFI_NOT_FOUND; > + } > } > /** > This function will look at a capsule and determine if it's a test pattern. > -- > 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41576): https://edk2.groups.io/g/devel/message/41576 Mute This Topic: https://groups.io/mt/31828852/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-